Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47769 )
Change subject: util/amdfwtool: Fix EFS generation polarity ......................................................................
util/amdfwtool: Fix EFS generation polarity
The DWORD used to indicate the Embedded Firmware Structure's generation uses 1 to indicate a first-gen structure, e.g. a SPI device's erased value of 0xffffffff. A 0 in bit 0 is how Client PSPs will interpret the structure as designed for second-gen.
This change and the original addition should have no effects on any current products as none interpret offset 0x24.
BUG=b:158755102 TEST=inspect EFS in coreboot.rom
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: If391f356a1811ed04acdfe9ab9de2e146f6ef5fd --- M util/amdfwtool/amdfwtool.c 1 file changed, 2 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/47769/1
diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index 9cf6a4f..36d669b 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -333,7 +333,7 @@ uint32_t bios0_entry; /* todo: add way to select correct entry */ uint32_t bios1_entry; uint32_t bios2_entry; - uint32_t second_gen_efs; + uint32_t second_gen_efs; /* Client SKUs b0=1 is Gen1, b1=0 is Gen2, Servers TBD */ uint32_t bios3_entry; uint32_t reserved_2Ch; uint32_t promontory_fw_ptr; @@ -1182,13 +1182,11 @@ } switch (soc_id) { case PLATFORM_STONEYRIDGE: - amd_romsig->second_gen_efs = 0; amd_romsig->spi_readmode_f15_mod_60_6f = efs_spi_readmode; amd_romsig->fast_speed_new_f15_mod_60_6f = efs_spi_speed; break; case PLATFORM_RAVEN: case PLATFORM_PICASSO: - amd_romsig->second_gen_efs = 0; amd_romsig->spi_readmode_f17_mod_00_2f = efs_spi_readmode; amd_romsig->spi_fastspeed_f17_mod_00_2f = efs_spi_speed; switch (efs_spi_micron_flag) { @@ -1205,7 +1203,7 @@ break; case PLATFORM_RENOIR: case PLATFORM_LUCIENNE: - amd_romsig->second_gen_efs = 1; + amd_romsig->second_gen_efs = 0xfffffffe; amd_romsig->spi_readmode_f17_mod_30_3f = efs_spi_readmode; amd_romsig->spi_fastspeed_f17_mod_30_3f = efs_spi_speed; switch (efs_spi_micron_flag) {
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47769 )
Change subject: util/amdfwtool: Fix EFS generation polarity ......................................................................
Patch Set 1: Code-Review+2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47769 )
Change subject: util/amdfwtool: Fix EFS generation polarity ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47769/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/47769/1/util/amdfwtool/amdfwtool.c@... PS1, Line 1206: amd_romsig->second_gen_efs = 0xfffffffe; i'd add and use defines for the EFS generation values and assign them to the struct field. i'd also keep the assignments in the other generations that get removed here, so that isn't implicitly set
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47769 )
Change subject: util/amdfwtool: Fix EFS generation polarity ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47769/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/47769/1/util/amdfwtool/amdfwtool.c@... PS1, Line 1206: amd_romsig->second_gen_efs = 0xfffffffe;
i'd add and use defines for the EFS generation values and assign them to the struct field. […]
already commented on the internal brach, but I'd like to keep all assignments so that you don't have to read the calling function rather closely and use ~0 and ~1 as right hand side in the assignments, since that makes it clearer where that value comes from. no need for defines here imho
Hello build bot (Jenkins), Martin Roth, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47769
to look at the new patch set (#3).
Change subject: util/amdfwtool: Fix EFS generation polarity ......................................................................
util/amdfwtool: Fix EFS generation polarity
The DWORD used to indicate the Embedded Firmware Structure's generation uses 1 to indicate a first-gen structure, e.g. a SPI device's erased value of 0xffffffff. A 0 in bit 0 is how Client PSPs will interpret the structure as designed for second-gen.
This change and the original addition should have no effects on any current products as none interpret offset 0x24.
BUG=b:158755102 TEST=inspect EFS in coreboot.rom
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: If391f356a1811ed04acdfe9ab9de2e146f6ef5fd --- M util/amdfwtool/amdfwtool.c 1 file changed, 11 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/47769/3
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47769 )
Change subject: util/amdfwtool: Fix EFS generation polarity ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47769/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/47769/1/util/amdfwtool/amdfwtool.c@... PS1, Line 1206: amd_romsig->second_gen_efs = 0xfffffffe;
already commented on the internal brach, but I'd like to keep all assignments so that you don't have […]
Done
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47769 )
Change subject: util/amdfwtool: Fix EFS generation polarity ......................................................................
Patch Set 3: Code-Review+2
Marshall Dawson has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47769 )
Change subject: util/amdfwtool: Fix EFS generation polarity ......................................................................
util/amdfwtool: Fix EFS generation polarity
The DWORD used to indicate the Embedded Firmware Structure's generation uses 1 to indicate a first-gen structure, e.g. a SPI device's erased value of 0xffffffff. A 0 in bit 0 is how Client PSPs will interpret the structure as designed for second-gen.
This change and the original addition should have no effects on any current products as none interpret offset 0x24.
BUG=b:158755102 TEST=inspect EFS in coreboot.rom
Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Change-Id: If391f356a1811ed04acdfe9ab9de2e146f6ef5fd Reviewed-on: https://review.coreboot.org/c/coreboot/+/47769 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Felix Held felix-coreboot@felixheld.de --- M util/amdfwtool/amdfwtool.c 1 file changed, 11 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved
diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index 9cf6a4f..401cf20 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -323,6 +323,13 @@ { .type = AMD_BIOS_INVALID }, };
+struct second_gen_efs { /* todo: expand for Server products */ + int gen:1; /* Client products only use bit 0 */ + int reserved:31; +} __attribute__((packed)); + +#define EFS_SECOND_GEN 0 + typedef struct _embedded_firmware { uint32_t signature; /* 0x55aa55aa */ uint32_t imc_entry; @@ -333,7 +340,7 @@ uint32_t bios0_entry; /* todo: add way to select correct entry */ uint32_t bios1_entry; uint32_t bios2_entry; - uint32_t second_gen_efs; + struct second_gen_efs efs_gen; uint32_t bios3_entry; uint32_t reserved_2Ch; uint32_t promontory_fw_ptr; @@ -1182,13 +1189,13 @@ } switch (soc_id) { case PLATFORM_STONEYRIDGE: - amd_romsig->second_gen_efs = 0; amd_romsig->spi_readmode_f15_mod_60_6f = efs_spi_readmode; amd_romsig->fast_speed_new_f15_mod_60_6f = efs_spi_speed; break; case PLATFORM_RAVEN: case PLATFORM_PICASSO: - amd_romsig->second_gen_efs = 0; + /* amd_romsig->efs_gen introduced after RAVEN/PICASSO. + * Leave as 0xffffffff for first gen */ amd_romsig->spi_readmode_f17_mod_00_2f = efs_spi_readmode; amd_romsig->spi_fastspeed_f17_mod_00_2f = efs_spi_speed; switch (efs_spi_micron_flag) { @@ -1205,7 +1212,7 @@ break; case PLATFORM_RENOIR: case PLATFORM_LUCIENNE: - amd_romsig->second_gen_efs = 1; + amd_romsig->efs_gen.gen = EFS_SECOND_GEN; amd_romsig->spi_readmode_f17_mod_30_3f = efs_spi_readmode; amd_romsig->spi_fastspeed_f17_mod_30_3f = efs_spi_speed; switch (efs_spi_micron_flag) {