Change in coreboot[master]: soc/amd/common/block/spi/fch_spi_ctrl.c: Fix SPI vendor id code
Richard Spiegel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35589 )
Change subject: soc/amd/common/block/spi/fch_spi_ctrl.c: Fix SPI vendor id code ......................................................................
soc/amd/common/block/spi/fch_spi_ctrl.c: Fix SPI vendor id code
All solid state devices have vendor id defined by JEDEC specification JEP106, which originally allocated only 7 bits for it plus parity. When number of vendors exploded beyond 126, a banking proposition came maintaining compatibility with older vendors while allowing for 4 extra bits (16 banks) through the introduction of the concept "Continuation code", denoted by the byte value of 0x7f. Examples: 0xfe, 0x60, 0x18, 0x00, 0x00 => vendor 0xfe of bank o 0x7f, 0x7f, 0xfe, 0x60, 0x18 => vendor 0xfe of bank 2
BUG=b:141535133 TEST=Build and boot grunt.
Change-Id: I16c5df70b8ba65017d1a45c79e90a76d1f78550c Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/soc/amd/common/block/include/amdblocks/fch_spi.h M src/soc/amd/common/block/spi/fch_spi_ctrl.c M src/soc/amd/common/block/spi/fch_spi_table.c 3 files changed, 27 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/35589/1
diff --git a/src/soc/amd/common/block/include/amdblocks/fch_spi.h b/src/soc/amd/common/block/include/amdblocks/fch_spi.h index cfbdf19..8e28828 100644 --- a/src/soc/amd/common/block/include/amdblocks/fch_spi.h +++ b/src/soc/amd/common/block/include/amdblocks/fch_spi.h @@ -23,7 +23,14 @@ #define WORD_TO_DWORD_UPPER(x) ((x << 16) & 0xffff0000) #define SPI_PAGE_WRITE 0x02 #define SPI_WRITE_ENABLE 0x06 -#define IDCODE_CONT_LEN 0 +/* + * 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. + */ +#define IDCODE_CONT_CODE 0x7f +#define IDCODE_CONT_LEN 1 /* currently support only bank 0 */ #define IDCODE_PART_LEN 5 #define IDCODE_LEN (IDCODE_CONT_LEN + IDCODE_PART_LEN)
diff --git a/src/soc/amd/common/block/spi/fch_spi_ctrl.c b/src/soc/amd/common/block/spi/fch_spi_ctrl.c index 4299e4f..a7d9c77 100644 --- a/src/soc/amd/common/block/spi/fch_spi_ctrl.c +++ b/src/soc/amd/common/block/spi/fch_spi_ctrl.c @@ -278,12 +278,26 @@ printk(BIOS_SPEW, "\n"); }
- /* count the number of continuation bytes */ - for (shift = 0, idp = idcode; shift < IDCODE_CONT_LEN && *idp == 0x7f; - ++shift, ++idp) - continue; + /* + * All solid state devices have vendor id defined by JEDEC specification JEP106, + * which originally allocated only 7 bits for it plus parity. When number of + * vendors exploded beyond 126, a banking proposition came maintaining + * compatibility with older vendors while allowing for 4 extra bits (16 banks) + * through the introduction of the concept "Continuation Code", denoted by the + * byte value of 0x7f. + * Examples: + * 0xfe, 0x60, 0x18, 0x00, 0x00, 0x00 => vendor 0xfe of bank o + * 0x7f, 0x7f, 0xfe, 0x60, 0x18, 0x00 => vendor 0xfe of bank 2 + * count the number of continuation code bytes + */ + for (shift = 0, idp = idcode; *idp == IDCODE_CONT_CODE; ++shift, ++idp) { + if (shift < IDCODE_CONT_LEN) + continue; + printk(BIOS_ERR, "unsupported ID code bank\n"); + return -1; + }
- printk(BIOS_INFO, "Manufacturer: %02x\n", *idp); + printk(BIOS_INFO, "Manufacturer: %02x on bank %d\n", *idp, shift);
/* search the table for matches in shift and id */ for (i = 0; i < table_size; ++i) { diff --git a/src/soc/amd/common/block/spi/fch_spi_table.c b/src/soc/amd/common/block/spi/fch_spi_table.c index 8c0108a..acea241 100644 --- a/src/soc/amd/common/block/spi/fch_spi_table.c +++ b/src/soc/amd/common/block/spi/fch_spi_table.c @@ -30,11 +30,6 @@ * 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).
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35589 )
Change subject: soc/amd/common/block/spi/fch_spi_ctrl.c: Fix SPI vendor id code ......................................................................
Patch Set 1:
Fixing code to make it clear and avoid false positive from coverity.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35589 )
Change subject: soc/amd/common/block/spi/fch_spi_ctrl.c: Fix SPI vendor id code ......................................................................
Patch Set 1: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35589 )
Change subject: soc/amd/common/block/spi/fch_spi_ctrl.c: Fix SPI vendor id code ......................................................................
soc/amd/common/block/spi/fch_spi_ctrl.c: Fix SPI vendor id code
All solid state devices have vendor id defined by JEDEC specification JEP106, which originally allocated only 7 bits for it plus parity. When number of vendors exploded beyond 126, a banking proposition came maintaining compatibility with older vendors while allowing for 4 extra bits (16 banks) through the introduction of the concept "Continuation code", denoted by the byte value of 0x7f. Examples: 0xfe, 0x60, 0x18, 0x00, 0x00 => vendor 0xfe of bank o 0x7f, 0x7f, 0xfe, 0x60, 0x18 => vendor 0xfe of bank 2
BUG=b:141535133 TEST=Build and boot grunt.
Change-Id: I16c5df70b8ba65017d1a45c79e90a76d1f78550c Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35589 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Edward O'Callaghan quasisec@chromium.org --- M src/soc/amd/common/block/include/amdblocks/fch_spi.h M src/soc/amd/common/block/spi/fch_spi_ctrl.c M src/soc/amd/common/block/spi/fch_spi_table.c 3 files changed, 27 insertions(+), 11 deletions(-)
Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved
diff --git a/src/soc/amd/common/block/include/amdblocks/fch_spi.h b/src/soc/amd/common/block/include/amdblocks/fch_spi.h index cfbdf19..8e28828 100644 --- a/src/soc/amd/common/block/include/amdblocks/fch_spi.h +++ b/src/soc/amd/common/block/include/amdblocks/fch_spi.h @@ -23,7 +23,14 @@ #define WORD_TO_DWORD_UPPER(x) ((x << 16) & 0xffff0000) #define SPI_PAGE_WRITE 0x02 #define SPI_WRITE_ENABLE 0x06 -#define IDCODE_CONT_LEN 0 +/* + * 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. + */ +#define IDCODE_CONT_CODE 0x7f +#define IDCODE_CONT_LEN 1 /* currently support only bank 0 */ #define IDCODE_PART_LEN 5 #define IDCODE_LEN (IDCODE_CONT_LEN + IDCODE_PART_LEN)
diff --git a/src/soc/amd/common/block/spi/fch_spi_ctrl.c b/src/soc/amd/common/block/spi/fch_spi_ctrl.c index 4299e4f..a7d9c77 100644 --- a/src/soc/amd/common/block/spi/fch_spi_ctrl.c +++ b/src/soc/amd/common/block/spi/fch_spi_ctrl.c @@ -278,12 +278,26 @@ printk(BIOS_SPEW, "\n"); }
- /* count the number of continuation bytes */ - for (shift = 0, idp = idcode; shift < IDCODE_CONT_LEN && *idp == 0x7f; - ++shift, ++idp) - continue; + /* + * All solid state devices have vendor id defined by JEDEC specification JEP106, + * which originally allocated only 7 bits for it plus parity. When number of + * vendors exploded beyond 126, a banking proposition came maintaining + * compatibility with older vendors while allowing for 4 extra bits (16 banks) + * through the introduction of the concept "Continuation Code", denoted by the + * byte value of 0x7f. + * Examples: + * 0xfe, 0x60, 0x18, 0x00, 0x00, 0x00 => vendor 0xfe of bank o + * 0x7f, 0x7f, 0xfe, 0x60, 0x18, 0x00 => vendor 0xfe of bank 2 + * count the number of continuation code bytes + */ + for (shift = 0, idp = idcode; *idp == IDCODE_CONT_CODE; ++shift, ++idp) { + if (shift < IDCODE_CONT_LEN) + continue; + printk(BIOS_ERR, "unsupported ID code bank\n"); + return -1; + }
- printk(BIOS_INFO, "Manufacturer: %02x\n", *idp); + printk(BIOS_INFO, "Manufacturer: %02x on bank %d\n", *idp, shift);
/* search the table for matches in shift and id */ for (i = 0; i < table_size; ++i) { diff --git a/src/soc/amd/common/block/spi/fch_spi_table.c b/src/soc/amd/common/block/spi/fch_spi_table.c index 8c0108a..acea241 100644 --- a/src/soc/amd/common/block/spi/fch_spi_table.c +++ b/src/soc/amd/common/block/spi/fch_spi_table.c @@ -30,11 +30,6 @@ * 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).
-
Edward O'Callaghan (Code Review) -
Patrick Georgi (Code Review)
-
Richard Spiegel (Code Review)