Change in coreboot[master]: util/amdfwtool: Fix EFS generation polarity

[Marshall Dawson (Code Review)]

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 
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 (Code Review)]

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 (Code Review)]

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 (Code Review)]

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

[Marshall Dawson (Code Review)]

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 
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 (Code Review)]

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 (Code Review)]

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 (Code Review)]

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 
Change-Id: If391f356a1811ed04acdfe9ab9de2e146f6ef5fd
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47769
Tested-by: build bot (Jenkins) 
Reviewed-by: Felix Held 
---
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) {