Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/48133 )
Change subject: ichspi.c: Implement ich_hwseq_get_flash_id() ......................................................................
ichspi.c: Implement ich_hwseq_get_flash_id()
Allow for decoding the SPI flash vid:did and to allow lookup and copying it from the flashchips database. This allows Flashrom to provide the user the real SPI flash chip name instead of 'Opaque flash chip'.
before: ``` $ sudo ./flashrom -p internal --flash-name 2>&1 | grep 'Found ' Found chipset "Intel Kaby Lake U w/ iHDCP2.2 Prem.". Found Programmer flash chip "Opaque flash chip" (16384 kB, Programmer-specific) mapped at physical address 0x0000000000000000. ```
after: ``` $ sudo ./flashrom -p internal --flash-name 2>&1 | grep 'Found ' Found chipset "Intel Kaby Lake U w/ iHDCP2.2 Prem.". Found Winbond flash chip "W25Q128.V" (16384 kB, Programmer-specific) mapped at physical address 0x0000000000000000. ```
BUG=none TEST=`flashrom -p internal --flash-name`
Change-Id: Icdf684e21b19ee87b797092f3cf0353e1184291f Signed-off-by: Edward O'Callaghan quasisec@google.com --- M ichspi.c 1 file changed, 82 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/33/48133/1
diff --git a/ichspi.c b/ichspi.c index 4209d60..45afe5b 100644 --- a/ichspi.c +++ b/ichspi.c @@ -78,8 +78,17 @@ #define DLOCK_SSEQ_LOCKDN_OFF 16 #define DLOCK_SSEQ_LOCKDN (0x1 << DLOCK_SSEQ_LOCKDN_OFF)
+/* Control bits */ +#define HSFSC_FGO_OFF 16 /* 0: Flash Cycle Go */ +#define HSFSC_FGO (0x1 << HSFSC_FGO_OFF) +#define HSFSC_FCYCLE_OFF 17 /* 17-20: FLASH Cycle */ +#define HSFSC_FCYCLE (0xf << HSFSC_FCYCLE_OFF) +#define HSFSC_FDBC_OFF 24 /* 24-29 : Flash Data Byte Count */ +#define HSFSC_FDBC (0x3f << HSFSC_FDBC_OFF) + #define PCH100_REG_FPR0 0x84 /* 32 Bits Protected Range 0 */ #define PCH100_REG_GPR0 0x98 /* 32 Bits Global Protected Range 0 */ +#define PCH100_REG_HSFSC 0x04
#define PCH100_REG_SSFSC 0xA0 /* 32 Bits Status (8) + Control (24) */ #define PCH100_REG_PREOP 0xA4 /* 16 Bits */ @@ -1321,12 +1330,85 @@ return 0; }
+/* Given RDID info, return pointer to entry in flashchips[] */ +static const struct flashchip *flash_id_to_entry(uint32_t mfg_id, uint32_t model_id) +{ + const struct flashchip *chip; + + for (chip = &flashchips[0]; chip->vendor; chip++) { + if ((chip->manufacture_id == mfg_id) && + (chip->model_id == model_id)) + return chip; + } + + return NULL; +} + +static int ich_hwseq_get_flash_id(struct flashctx *flash, enum ich_chipset ich_gen) +{ + uint32_t hsfsc, data, mfg_id, model_id; + const struct flashchip *entry; + const int len = sizeof(data); + + /* make sure FDONE, FCERR, & AEL are cleared */ + REGWRITE32(ICH9_REG_HSFS, REGREAD32(ICH9_REG_HSFS)); + + /* Set RDID as flash cycle and FGO */ + hsfsc = REGREAD32(ICH9_REG_HSFS); + hsfsc &= ~HSFSC_FCYCLE; + hsfsc &= ~HSFSC_FDBC; + hsfsc |= ((len - 1) << HSFSC_FDBC_OFF) & HSFSC_FDBC; + hsfsc |= (0x6 << HSFSC_FCYCLE_OFF) | HSFSC_FGO; + REGWRITE32(ICH9_REG_HSFS, hsfsc); + /* poll for 100ms */ + if (ich_hwseq_wait_for_cycle_complete(100 * 1000, len, ich_gen)) { + msg_perr("Timed out waiting for RDID to complete.\n"); + return 0; + } + + /* + * Data will appear in reverse order: + * Byte 0: Manufacturer ID + * Byte 1: Model ID (MSB) + * Byte 2: Model ID (LSB) + */ + ich_read_data((uint8_t *)&data, len, ICH9_REG_FDATA0); + mfg_id = data & 0xff; + model_id = (data & 0xff00) | ((data >> 16) & 0xff); + + entry = flash_id_to_entry(mfg_id, model_id); + if (entry == NULL) { + msg_perr("Unable to identify chip, mfg_id: 0x%02x, " + "model_id: 0x%02x\n", mfg_id, model_id); + return 0; + } else { + msg_pdbg("Chip identified: %s\n", entry->name); + /* Update informational flash chip entries only */ + flash->chip->vendor = entry->vendor; + flash->chip->name = entry->name; + flash->chip->manufacture_id = entry->manufacture_id; + flash->chip->model_id = entry->model_id; + /* total_size read from flash descriptor */ + flash->chip->page_size = entry->page_size; + flash->chip->feature_bits = entry->feature_bits; + flash->chip->tested = entry->tested; + flash->chip->wp = entry->wp; + } + + return 1; +} + static int ich_hwseq_probe(struct flashctx *flash) { uint32_t total_size, boundary; uint32_t erase_size_low, size_low, erase_size_high, size_high; struct block_eraser *eraser;
+ if (ich_hwseq_get_flash_id(flash, ich_generation) != 1) { + msg_perr("Unable to read flash chip ID\n"); + return 0; + } + total_size = hwseq_data.size_comp0 + hwseq_data.size_comp1; msg_cdbg("Hardware sequencing reports %d attached SPI flash chip", (hwseq_data.size_comp1 != 0) ? 2 : 1);
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48133 )
Change subject: ichspi.c: Implement ich_hwseq_get_flash_id() ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/flashrom/+/48133/1/ichspi.c File ichspi.c:
https://review.coreboot.org/c/flashrom/+/48133/1/ichspi.c@1393 PS1, Line 1393: flash->chip->feature_bits = entry->feature_bits; Won't this break chips with 4ba support?
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48133 )
Change subject: ichspi.c: Implement ich_hwseq_get_flash_id() ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/flashrom/+/48133/1/ichspi.c File ichspi.c:
https://review.coreboot.org/c/flashrom/+/48133/1/ichspi.c@1393 PS1, Line 1393: flash->chip->feature_bits = entry->feature_bits;
Won't this break chips with 4ba support?
Well it's the other way around right, 4BA is currently broken and this just surfaces the underlying issues.
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48133 )
Change subject: ichspi.c: Implement ich_hwseq_get_flash_id() ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/flashrom/+/48133/1/ichspi.c File ichspi.c:
https://review.coreboot.org/c/flashrom/+/48133/1/ichspi.c@1393 PS1, Line 1393: flash->chip->feature_bits = entry->feature_bits;
Well it's the other way around right, 4BA is currently broken and this just surfaces the underlying […]
Without this CL flashrom remains ignorant of 4ba support and with ich_hwseq reading from a 32MB chip works (assuming the map_flash() error is ignored). With this CL it fails or crashes (again with the map_flash() error ignored).
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48133 )
Change subject: ichspi.c: Implement ich_hwseq_get_flash_id() ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/flashrom/+/48133/1/ichspi.c File ichspi.c:
https://review.coreboot.org/c/flashrom/+/48133/1/ichspi.c@1375 PS1, Line 1375: ich_read_data((uint8_t *)&data, len, ICH9_REG_FDATA0); This seems to return 0 to data on my ich9 device here, which reports "Chip identified: KB9012 (EDI)" (it should be a W25X64)
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48133 )
Change subject: ichspi.c: Implement ich_hwseq_get_flash_id() ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/flashrom/+/48133/1/ichspi.c File ichspi.c:
https://review.coreboot.org/c/flashrom/+/48133/1/ichspi.c@1375 PS1, Line 1375: ich_read_data((uint8_t *)&data, len, ICH9_REG_FDATA0);
This seems to return 0 to data on my ich9 device here, which reports "Chip identified: KB9012 (EDI)" (it should be a W25X64)
And after running the software sequencing, it all returns 0xffffff, so it breaks it.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48133 )
Change subject: ichspi.c: Implement ich_hwseq_get_flash_id() ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/flashrom/+/48133/1/ichspi.c File ichspi.c:
https://review.coreboot.org/c/flashrom/+/48133/1/ichspi.c@1375 PS1, Line 1375: ich_read_data((uint8_t *)&data, len, ICH9_REG_FDATA0);
This seems to return 0 to data on my ich9 device here, which reports "Chip identified: KB9012 (EDI)" […]
Hey Arthur, that's very weird. Could you pastebin somewhere the full verbose output of a run so we can to try to inspect what is happening?
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48133 )
Change subject: ichspi.c: Implement ich_hwseq_get_flash_id() ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/flashrom/+/48133/1/ichspi.c File ichspi.c:
https://review.coreboot.org/c/flashrom/+/48133/1/ichspi.c@1398 PS1, Line 1398: 1; also we should use 0 for non-error paths as it currently is a bit confused as well.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48133 )
Change subject: ichspi.c: Implement ich_hwseq_get_flash_id() ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/flashrom/+/48133/1/ichspi.c File ichspi.c:
https://review.coreboot.org/c/flashrom/+/48133/1/ichspi.c@1375 PS1, Line 1375: ich_read_data((uint8_t *)&data, len, ICH9_REG_FDATA0);
Hey Arthur, that's very weird. Could you pastebin somewhere the full verbose output of a run so we can to try to inspect what is happening?
https://paste.flashrom.org/view.php?id=3411
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48133 )
Change subject: ichspi.c: Implement ich_hwseq_get_flash_id() ......................................................................
Patch Set 1: Code-Review-2
(1 comment)
https://review.coreboot.org/c/flashrom/+/48133/1/ichspi.c File ichspi.c:
https://review.coreboot.org/c/flashrom/+/48133/1/ichspi.c@1375 PS1, Line 1375: ich_read_data((uint8_t *)&data, len, ICH9_REG_FDATA0);
Hey Arthur, that's very weird. […]
Thanks! That's very helpful and thanks for taking the time to help test!
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48133 )
Change subject: ichspi.c: Implement ich_hwseq_get_flash_id() ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/flashrom/+/48133/1/ichspi.c File ichspi.c:
https://review.coreboot.org/c/flashrom/+/48133/1/ichspi.c@1375 PS1, Line 1375: ich_read_data((uint8_t *)&data, len, ICH9_REG_FDATA0);
Thanks! That's very helpful and thanks for taking the time to help test!
No worries. Let me know if you need any further testing.