Aaron Durbin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38361 )
Change subject: drives/spi/spi_flash: remove continuation byte support ......................................................................
drives/spi/spi_flash: remove continuation byte support
There was code to handle the case of continuation bytes for identifying the manufacturer id to a jedec rdid command. However, all the parts that currently supported have this defined to be 0. Remove the unused continuation byte support.
Change-Id: Ia7c63162e4ef9dc46ef916ca8c31ebd721cbeca7 Signed-off-by: Aaron Durbin adurbin@chromium.org --- M src/drivers/spi/spi_flash.c 1 file changed, 23 insertions(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/38361/1
diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c index 4a86146..36ae81b 100644 --- a/src/drivers/spi/spi_flash.c +++ b/src/drivers/spi/spi_flash.c @@ -312,7 +312,6 @@ /* * The following table holds all device probe functions * - * shift: number of continuation bytes before the ID * idcode: the expected IDCODE or 0xff for non JEDEC devices * probe: the function to call * @@ -322,63 +321,55 @@ * Several matching entries are permitted, they will be tried * in sequence until a probe function returns non NULL. * - * IDCODE_CONT_LEN may be redefined if a device needs to declare a - * larger "shift" value. IDCODE_PART_LEN generally shouldn't be - * changed. This is the max number of bytes probe functions may - * examine when looking up part-specific identification info. - * * Probe functions will be given the idcode buffer starting at their - * manu id byte (the "idcode" in the table below). In other words, - * all of the continuation bytes will be skipped (the "shift" below). + * manu id byte (the "idcode" in the table below). */ -#define IDCODE_CONT_LEN 0 -#define IDCODE_PART_LEN 5 static struct { - const u8 shift; const u8 idcode; int (*probe) (const struct spi_slave *spi, u8 *idcode, struct spi_flash *flash); } flashes[] = { /* Keep it sorted by define name */ #if CONFIG(SPI_FLASH_AMIC) - { 0, VENDOR_ID_AMIC, spi_flash_probe_amic, }, + { VENDOR_ID_AMIC, spi_flash_probe_amic, }, #endif #if CONFIG(SPI_FLASH_ATMEL) - { 0, VENDOR_ID_ATMEL, spi_flash_probe_atmel, }, + { VENDOR_ID_ATMEL, spi_flash_probe_atmel, }, #endif #if CONFIG(SPI_FLASH_EON) - { 0, VENDOR_ID_EON, spi_flash_probe_eon, }, + { VENDOR_ID_EON, spi_flash_probe_eon, }, #endif #if CONFIG(SPI_FLASH_GIGADEVICE) - { 0, VENDOR_ID_GIGADEVICE, spi_flash_probe_gigadevice, }, + { VENDOR_ID_GIGADEVICE, spi_flash_probe_gigadevice, }, #endif #if CONFIG(SPI_FLASH_MACRONIX) - { 0, VENDOR_ID_MACRONIX, spi_flash_probe_macronix, }, + { VENDOR_ID_MACRONIX, spi_flash_probe_macronix, }, #endif #if CONFIG(SPI_FLASH_SPANSION) - { 0, VENDOR_ID_SPANSION, spi_flash_probe_spansion, }, + { VENDOR_ID_SPANSION, spi_flash_probe_spansion, }, #endif #if CONFIG(SPI_FLASH_SST) - { 0, VENDOR_ID_SST, spi_flash_probe_sst, }, + { VENDOR_ID_SST, spi_flash_probe_sst, }, #endif #if CONFIG(SPI_FLASH_STMICRO) - { 0, VENDOR_ID_STMICRO, spi_flash_probe_stmicro, }, + { VENDOR_ID_STMICRO, spi_flash_probe_stmicro, }, #endif #if CONFIG(SPI_FLASH_WINBOND) - { 0, VENDOR_ID_WINBOND, spi_flash_probe_winbond, }, + { VENDOR_ID_WINBOND, spi_flash_probe_winbond, }, #endif /* Keep it sorted by best detection */ #if CONFIG(SPI_FLASH_ADESTO) - { 0, VENDOR_ID_ADESTO, spi_flash_probe_adesto, }, + { VENDOR_ID_ADESTO, spi_flash_probe_adesto, }, #endif }; -#define IDCODE_LEN (IDCODE_CONT_LEN + IDCODE_PART_LEN) +#define IDCODE_LEN 5
int spi_flash_generic_probe(const struct spi_slave *spi, struct spi_flash *flash) { - int ret, i, shift; - u8 idcode[IDCODE_LEN], *idp; + int ret, i; + u8 idcode[IDCODE_LEN]; + u8 manuf_id;
/* Read the ID codes */ ret = spi_flash_cmd(spi, CMD_READ_ID, idcode, sizeof(idcode)); @@ -392,29 +383,25 @@ printk(BIOS_SPEW, "\n"); }
- /* count the number of continuation bytes */ - for (shift = 0, idp = idcode; shift < IDCODE_CONT_LEN && *idp == 0x7f; - ++shift, ++idp) - continue; + manuf_id = idcode[0];
- printk(BIOS_INFO, "Manufacturer: %02x\n", *idp); + printk(BIOS_INFO, "Manufacturer: %02x\n", manuf_id);
/* If no result from RDID command and STMicro parts are enabled attempt to wake the part from deep sleep and obtain alternative id info. */ - if (CONFIG(SPI_FLASH_STMICRO) && *idp == 0xff) { + if (CONFIG(SPI_FLASH_STMICRO) && manuf_id == 0xff) { if (stmicro_release_deep_sleep_identify(spi, idcode)) return -1; - idp = idcode; - shift = 0; + manuf_id = idcode[0]; }
/* search the table for matches in shift and id */ for (i = 0; i < (int)ARRAY_SIZE(flashes); ++i) - if (flashes[i].shift == shift && flashes[i].idcode == *idp) { + if (flashes[i].idcode == manuf_id) { /* we have a match, call probe */ - if (flashes[i].probe(spi, idp, flash) == 0) { - flash->vendor = idp[0]; - flash->model = (idp[1] << 8) | idp[2]; + if (flashes[i].probe(spi, idcode, flash) == 0) { + flash->vendor = idcode[0]; + flash->model = (idcode[1] << 8) | idcode[2]; return 0; } }
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38361 )
Change subject: drives/spi/spi_flash: remove continuation byte support ......................................................................
Patch Set 1: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/38361/1/src/drivers/spi/spi_flash.c File src/drivers/spi/spi_flash.c:
https://review.coreboot.org/c/coreboot/+/38361/1/src/drivers/spi/spi_flash.c... PS1, Line 315: or 0xff for non JEDEC devices Not part of this change, but I think this needs to be removed now?
https://review.coreboot.org/c/coreboot/+/38361/1/src/drivers/spi/spi_flash.c... PS1, Line 318: Non JEDEC devices should be ordered in the table such that : * the probe functions with best detection algorithms come first. Do we want to just get rid of non-JEDEC device support in this table?
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38361 )
Change subject: drives/spi/spi_flash: remove continuation byte support ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38361/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38361/1//COMMIT_MSG@7 PS1, Line 7: drives nit: drivers
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38361 )
Change subject: drives/spi/spi_flash: remove continuation byte support ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38361/1/src/drivers/spi/spi_flash.c File src/drivers/spi/spi_flash.c:
https://review.coreboot.org/c/coreboot/+/38361/1/src/drivers/spi/spi_flash.c... PS1, Line 315: or 0xff for non JEDEC devices
Not part of this change, but I think this needs to be removed now?
I can do that. I nuke this whole thing in a followup patch.
https://review.coreboot.org/c/coreboot/+/38361/1/src/drivers/spi/spi_flash.c... PS1, Line 318: Non JEDEC devices should be ordered in the table such that : * the probe functions with best detection algorithms come first.
Do we want to just get rid of non-JEDEC device support in this table?
I can remove this commentary as well.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38361 )
Change subject: drives/spi/spi_flash: remove continuation byte support ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38361/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38361/1//COMMIT_MSG@7 PS1, Line 7: drives
nit: drivers
Done
Hello build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38361
to look at the new patch set (#2).
Change subject: drivers/spi/spi_flash: remove continuation byte support ......................................................................
drivers/spi/spi_flash: remove continuation byte support
There was code to handle the case of continuation bytes for identifying the manufacturer id to a jedec rdid command. However, all the parts that currently supported have this defined to be 0. Remove the unused continuation byte support.
Change-Id: Ia7c63162e4ef9dc46ef916ca8c31ebd721cbeca7 Signed-off-by: Aaron Durbin adurbin@chromium.org --- M src/drivers/spi/spi_flash.c 1 file changed, 24 insertions(+), 40 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/38361/2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38361 )
Change subject: drivers/spi/spi_flash: remove continuation byte support ......................................................................
Patch Set 3: Code-Review+2
Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38361 )
Change subject: drivers/spi/spi_flash: remove continuation byte support ......................................................................
drivers/spi/spi_flash: remove continuation byte support
There was code to handle the case of continuation bytes for identifying the manufacturer id to a jedec rdid command. However, all the parts that currently supported have this defined to be 0. Remove the unused continuation byte support.
Change-Id: Ia7c63162e4ef9dc46ef916ca8c31ebd721cbeca7 Signed-off-by: Aaron Durbin adurbin@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/38361 Reviewed-by: Julius Werner jwerner@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/drivers/spi/spi_flash.c 1 file changed, 24 insertions(+), 40 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c index 312f2f0..0024e9b 100644 --- a/src/drivers/spi/spi_flash.c +++ b/src/drivers/spi/spi_flash.c @@ -312,73 +312,61 @@ /* * The following table holds all device probe functions * - * shift: number of continuation bytes before the ID - * idcode: the expected IDCODE or 0xff for non JEDEC devices + * idcode: the expected IDCODE * probe: the function to call * - * Non JEDEC devices should be ordered in the table such that - * the probe functions with best detection algorithms come first. - * * Several matching entries are permitted, they will be tried * in sequence until a probe function returns non NULL. * - * IDCODE_CONT_LEN may be redefined if a device needs to declare a - * larger "shift" value. IDCODE_PART_LEN generally shouldn't be - * changed. This is the max number of bytes probe functions may - * examine when looking up part-specific identification info. - * * Probe functions will be given the idcode buffer starting at their - * manu id byte (the "idcode" in the table below). In other words, - * all of the continuation bytes will be skipped (the "shift" below). + * manu id byte (the "idcode" in the table below). */ -#define IDCODE_CONT_LEN 0 -#define IDCODE_PART_LEN 5 static struct { - const u8 shift; const u8 idcode; int (*probe) (const struct spi_slave *spi, u8 *idcode, struct spi_flash *flash); } flashes[] = { /* Keep it sorted by define name */ #if CONFIG(SPI_FLASH_AMIC) - { 0, VENDOR_ID_AMIC, spi_flash_probe_amic, }, + { VENDOR_ID_AMIC, spi_flash_probe_amic, }, #endif #if CONFIG(SPI_FLASH_ATMEL) - { 0, VENDOR_ID_ATMEL, spi_flash_probe_atmel, }, + { VENDOR_ID_ATMEL, spi_flash_probe_atmel, }, #endif #if CONFIG(SPI_FLASH_EON) - { 0, VENDOR_ID_EON, spi_flash_probe_eon, }, + { VENDOR_ID_EON, spi_flash_probe_eon, }, #endif #if CONFIG(SPI_FLASH_GIGADEVICE) - { 0, VENDOR_ID_GIGADEVICE, spi_flash_probe_gigadevice, }, + { VENDOR_ID_GIGADEVICE, spi_flash_probe_gigadevice, }, #endif #if CONFIG(SPI_FLASH_MACRONIX) - { 0, VENDOR_ID_MACRONIX, spi_flash_probe_macronix, }, + { VENDOR_ID_MACRONIX, spi_flash_probe_macronix, }, #endif #if CONFIG(SPI_FLASH_SPANSION) - { 0, VENDOR_ID_SPANSION, spi_flash_probe_spansion, }, + { VENDOR_ID_SPANSION, spi_flash_probe_spansion, }, #endif #if CONFIG(SPI_FLASH_SST) - { 0, VENDOR_ID_SST, spi_flash_probe_sst, }, + { VENDOR_ID_SST, spi_flash_probe_sst, }, #endif #if CONFIG(SPI_FLASH_STMICRO) - { 0, VENDOR_ID_STMICRO, spi_flash_probe_stmicro, }, + { VENDOR_ID_STMICRO, spi_flash_probe_stmicro, }, #endif #if CONFIG(SPI_FLASH_WINBOND) - { 0, VENDOR_ID_WINBOND, spi_flash_probe_winbond, }, + { VENDOR_ID_WINBOND, spi_flash_probe_winbond, }, #endif /* Keep it sorted by best detection */ #if CONFIG(SPI_FLASH_ADESTO) - { 0, VENDOR_ID_ADESTO, spi_flash_probe_adesto, }, + { VENDOR_ID_ADESTO, spi_flash_probe_adesto, }, #endif }; -#define IDCODE_LEN (IDCODE_CONT_LEN + IDCODE_PART_LEN) +#define IDCODE_LEN 5
int spi_flash_generic_probe(const struct spi_slave *spi, struct spi_flash *flash) { - int ret, i, shift; - u8 idcode[IDCODE_LEN], *idp; + int ret, i; + u8 idcode[IDCODE_LEN]; + u8 manuf_id;
/* Read the ID codes */ ret = spi_flash_cmd(spi, CMD_READ_ID, idcode, sizeof(idcode)); @@ -392,29 +380,25 @@ printk(BIOS_SPEW, "\n"); }
- /* count the number of continuation bytes */ - for (shift = 0, idp = idcode; shift < IDCODE_CONT_LEN && *idp == 0x7f; - ++shift, ++idp) - continue; + manuf_id = idcode[0];
- printk(BIOS_INFO, "Manufacturer: %02x\n", *idp); + printk(BIOS_INFO, "Manufacturer: %02x\n", manuf_id);
/* If no result from RDID command and STMicro parts are enabled attempt to wake the part from deep sleep and obtain alternative id info. */ - if (CONFIG(SPI_FLASH_STMICRO) && *idp == 0xff) { + if (CONFIG(SPI_FLASH_STMICRO) && manuf_id == 0xff) { if (stmicro_release_deep_sleep_identify(spi, idcode)) return -1; - idp = idcode; - shift = 0; + manuf_id = idcode[0]; }
/* search the table for matches in shift and id */ for (i = 0; i < (int)ARRAY_SIZE(flashes); ++i) - if (flashes[i].shift == shift && flashes[i].idcode == *idp) { + if (flashes[i].idcode == manuf_id) { /* we have a match, call probe */ - if (flashes[i].probe(spi, idp, flash) == 0) { - flash->vendor = idp[0]; - flash->model = (idp[1] << 8) | idp[2]; + if (flashes[i].probe(spi, idcode, flash) == 0) { + flash->vendor = idcode[0]; + flash->model = (idcode[1] << 8) | idcode[2]; return 0; } }
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)