Change in coreboot[master]: util/amdfwtool: Add support for EFS SPI values for F17h and F15h
Matt Papageorge has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
util/amdfwtool: Add support for EFS SPI values for F17h and F15h
The Embedded Firmware Structure contains various SPI parameters for the PSP to program. This change adds support to amdfwtool for populating these values as well specifying SOC Family and Model.
BUG=b:158755102 TEST=Read EFS values at appropriate offsets using a hex editor. Boot test on Tremblye and Morpheus.
Change-Id: I87c4d44183ca65a5570de5e0c7f9b44aa6dd82f9 Signed-off-by: Matt Papageorge matt.papageorge@amd.corp-partner.google.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 100 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/42566/1
diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index a5e5110..0241ba2 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -215,7 +215,14 @@ printf("-q | --anywhere Use any 64-byte aligned addr for Directory\n"); printf("-R | --sharedmem Location of PSP/FW shared memory\n"); printf("-P | --sharedmem-size Maximum size of the PSP/FW shared memory area\n"); + printf("-C | --soc-family <HEX_VAL> Specify SOC Family\n"); + printf("-D | --soc-model-min <HEX_VAL> Specify minimum SOC Model support\n"); + printf("-E | --soc-model-max <HEX_VAL> Specify maximum SOC Model support\n"); printf("-h | --help show this help\n"); + printf("\Embedded Firmware Structure options:\n"); + printf("--spi-speed <HEX_VAL> SPI speed to place in EFS Table\n"); + printf("--spi-read-mode <HEX_VAL> SPI read mode to place in EFS Table\n"); + printf("--spi-micron-flag <HEX_VAL> SPI Micron override flag to place in EFS Table\n"); }
typedef enum _amd_bios_type { @@ -413,7 +420,26 @@ uint32_t bios0_entry; /* todo: add way to select correct entry */ uint32_t bios1_entry; uint32_t bios2_entry; - uint32_t reserved[0x2c]; /* 0x24 - 0x4f */ + uint32_t second_gen_efs; + uint32_t bios3_entry; + uint32_t reserved_2Ch; + uint32_t promontory_fw_ptr; + uint32_t lp_promontory_fw_ptr; + uint32_t reserved_38h; + uint32_t reserved_3Ch; + uint8_t spi_readmode_f15_mod_60_6f; + uint8_t fast_speed_new_f15_mod_60_6f; + uint8_t reserved_42h; + uint8_t spi_readmode_f17_mod_00_2f; + uint8_t spi_fastspeed_f17_mod_00_2f; + uint8_t qpr_dummy_cycle_f17_mod_00_2f; + uint8_t reserved_46h; + uint8_t spi_readmode_f17_mod_30_3f; + uint8_t spi_fastspeed_f17_mod_30_3f; + uint8_t micron_detect_f17_mod_30_3f; + uint8_t reserved_4Ah; + uint8_t reserved_4Bh; + uint32_t reserved_4Ch; } __attribute__((packed, aligned(16))) embedded_firmware;
typedef struct _psp_directory_header { @@ -1038,8 +1064,18 @@
fill_dir_header(biosdir, count, cookie); } -// Unused values: CDE -static const char *optstring = "x:i:g:AMS:p:b:s:r:k:c:n:d:t:u:w:m:T:z:J:B:K:L:Y:N:UW:I:a:Q:V:e:v:j:y:G:O:X:F:H:o:f:l:hZ:qR:P:"; + +enum { + /* begin after ASCII characters */ + LONGOPT_FAMILY = 256, + LONGOPT_MODEL = 257, + LONGOPT_SPI_READ_MODE = 258, + LONGOPT_SPI_SPEED = 259, + LONGOPT_SPI_MICRON_FLAG = 260, +}; + +// Use non-ASCII values for additional input parameters +static const char *optstring = "x:i:g:AMS:p:b:s:r:k:c:n:d:t:u:w:m:T:z:J:B:K:L:Y:N:UW:I:a:Q:V:e:v:j:y:G:O:X:F:H:o:f:l:hZ:qR:P:C:D:E:";
static struct option long_options[] = { {"xhci", required_argument, 0, 'x' }, @@ -1086,6 +1122,10 @@ {"mp2-config", required_argument, 0, 'X' }, {"apob-nv-base", required_argument, 0, 'F' }, {"apob-nv-size", required_argument, 0, 'H' }, + /* Embedded Firmware Structure items*/ + {"spi-read-mode", required_argument, 0, LONGOPT_SPI_READ_MODE }, + {"spi-speed", required_argument, 0, LONGOPT_SPI_SPEED }, + {"spi-micron-flag", required_argument, 0, LONGOPT_SPI_MICRON_FLAG }, /* other */ {"output", required_argument, 0, 'o' }, {"flashsize", required_argument, 0, 'f' }, @@ -1093,6 +1133,9 @@ {"anywhere", no_argument, 0, 'q' }, {"sharedmem", required_argument, 0, 'R' }, {"sharedmem-size", required_argument, 0, 'P' }, + {"soc-family", required_argument, 0, 'C' }, + {"soc-model-min", required_argument, 0, 'D' }, + {"soc-model-max", required_argument, 0, 'E' }, {"help", no_argument, 0, 'h' }, {NULL, 0, 0, 0 } }; @@ -1200,6 +1243,13 @@ bool any_location = 0; uint32_t romsig_offset; uint32_t rom_base_address; + uint8_t soc_family = 0; + uint8_t soc_model_min = 0; + uint8_t soc_model_max = 0; + uint8_t efs_spi_readmode = 0xff; + uint8_t efs_spi_speed = 0xff; + uint8_t efs_spi_micron_flag = 0xff; + int multi = 0;
while (1) { @@ -1394,6 +1444,30 @@ register_fw_filename(AMD_FW_PSP_VERSTAGE, sub, optarg); sub = instance = 0; break; + case 'C': + soc_family = strtoull(optarg, NULL, 16); + sub = instance = 0; + break; + case 'D': + soc_model_min = strtoull(optarg, NULL, 16); + sub = instance = 0; + break; + case 'E': + soc_model_max = strtoull(optarg, NULL, 16); + sub = instance = 0; + break; + case LONGOPT_SPI_READ_MODE: + efs_spi_readmode = strtoull(optarg, NULL, 16); + sub = instance = 0; + break; + case LONGOPT_SPI_SPEED: + efs_spi_speed = strtoull(optarg, NULL, 16); + sub = instance = 0; + break; + case LONGOPT_SPI_MICRON_FLAG: + efs_spi_micron_flag = strtoull(optarg, NULL, 16); + sub = instance = 0; + break; case 'o': output = optarg; break; @@ -1455,6 +1529,11 @@ retval = 1; }
+ if (soc_model_min > soc_model_max) { + printf("Error: Invalid SOC model range.\n\n"); + retval = 1; + } + if (retval) { usage(); return retval; @@ -1512,6 +1591,24 @@ amd_romsig->gec_entry = 0; amd_romsig->xhci_entry = 0;
+ if (soc_family == 0x15 && soc_model_min >= 0x60 && soc_model_max <= 0x6f) { + 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; + } + if (soc_family == 0x17 && soc_model_min >= 0x0 && soc_model_max <= 0x2f) { + 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; + amd_romsig->qpr_dummy_cycle_f17_mod_00_2f = efs_spi_micron_flag; + } + if (soc_family == 0x17 && soc_model_min >= 0x30 && soc_model_max <= 0x3f) { + amd_romsig->second_gen_efs = 1; + amd_romsig->spi_readmode_f17_mod_30_3f = efs_spi_readmode; + amd_romsig->spi_fastspeed_f17_mod_30_3f = efs_spi_speed; + amd_romsig->micron_detect_f17_mod_30_3f = efs_spi_micron_flag; + } + integrate_firmwares(&ctx, amd_romsig, amd_fw_table);
ctx.current = ALIGN(ctx.current, 0x10000U); /* TODO: is it necessary? */
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42566/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/1/util/amdfwtool/amdfwtool.c@... PS1, Line 225: printf("--spi-micron-flag <HEX_VAL> SPI Micron override flag to place in EFS Table\n"); line over 96 characters
https://review.coreboot.org/c/coreboot/+/42566/1/util/amdfwtool/amdfwtool.c@... PS1, Line 1078: static const char *optstring = "x:i:g:AMS:p:b:s:r:k:c:n:d:t:u:w:m:T:z:J:B:K:L:Y:N:UW:I:a:Q:V:e:v:j:y:G:O:X:F:H:o:f:l:hZ:qR:P:C:D:E:"; line over 96 characters
Hello Aaron Durbin, Martin Roth, Furquan Shaikh, Marshall Dawson, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42566
to look at the new patch set (#2).
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
util/amdfwtool: Add support for EFS SPI values for F17h and F15h
The Embedded Firmware Structure contains various SPI parameters for the PSP to program. This change adds support to amdfwtool for populating these values as well specifying SOC Family and Model.
BUG=b:158755102 TEST=Read EFS values at appropriate offsets using a hex editor. Boot test on Tremblye and Morphius.
Change-Id: I87c4d44183ca65a5570de5e0c7f9b44aa6dd82f9 Signed-off-by: Matt Papageorge matt.papageorge@amd.corp-partner.google.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 100 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/42566/2
Hello build bot (Jenkins), Martin Roth, Furquan Shaikh, Marshall Dawson, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42566
to look at the new patch set (#3).
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
util/amdfwtool: Add support for EFS SPI values for F17h and F15h
The Embedded Firmware Structure contains various SPI parameters for the PSP to program. This change adds support to amdfwtool for populating these values as well specifying SOC Family and Model.
BUG=b:158755102 TEST=Read EFS values at appropriate offsets using a hex editor. Boot test on Tremblye and Morpheus.
Change-Id: I87c4d44183ca65a5570de5e0c7f9b44aa6dd82f9 Signed-off-by: Matt Papageorge matt.papageorge@amd.corp-partner.google.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 118 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/42566/3
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 3: Code-Review+2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c@... PS3, Line 241: printf(" F17h M 0-2fh: Micron parts use 0x0a, otherwise 0xff\n"); line over 96 characters
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c@... PS3, Line 242: printf(" F17h M 30h-3fh: Use 0x55 to force Micron detection,\n"); line over 96 characters
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c@... PS3, Line 243: printf(" 0xaa for only Micron parts. Other vendors 0xff\n"); line over 96 characters
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c@... PS3, Line 1096: static const char *optstring = "x:i:g:AMS:p:b:s:r:k:c:n:d:t:u:w:m:T:z:J:B:K:L:Y:N:UW:I:a:Q:V:e:v:j:y:G:O:X:F:H:o:f:l:hZ:qR:P:C:D:E:"; line over 96 characters
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c@... PS3, Line 243: 0xaa for only Micron parts. Use the same wording as for F17h M 30h-3fh?
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c@... PS3, Line 1617: if (soc_family == 0x17 && soc_model_min >= 0x0 && soc_model_max <= 0x2f) { : 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; : amd_romsig->qpr_dummy_cycle_f17_mod_00_2f = efs_spi_micron_flag; : } : if (soc_family == 0x17 && soc_model_min >= 0x30 && soc_model_max <= 0x3f) { : amd_romsig->second_gen_efs = 1; : amd_romsig->spi_readmode_f17_mod_30_3f = efs_spi_readmode; : amd_romsig->spi_fastspeed_f17_mod_30_3f = efs_spi_speed; : amd_romsig->micron_detect_f17_mod_30_3f = efs_spi_micron_flag; : } Use `else of` to make sure, these branches cannot be entered twice (by making a mistake in the condition)?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c@... PS3, Line 218: --soc-family Instead of taking family/model-min/model-max as inputs, would it make sense to have family name as input e.g. picasso, stoneyridge or PCO, STR. Then, amdfwtool can apply the translation from family-name to the right EFS fields without really having to sanitize the family/model-min/model-max values. This also relieves the SoC from having to pass in multiple values to identify itself.
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c@... PS3, Line 239: <HEX_VAL> I think this will have to be a boolean flag or a tri-state flag like NO_MICRON_SUPPORT: Micron SPI flash is not supported by this board ONLY_MICRON_SUPPORT: Only Micron SPI flash is supported by this board MIXED_MICRON_SUPPORT: Both Micron and non-Micron SPI flash are supported by this board
Based on the value of this flag, amdfwtool will have to set values differently for `qpr_dummy_cycle_f17_mod_00_2f` and `micron_detect_f17_mod_30_3f`.
For `qpr_dummy_cycle_f17_mod_00_2f`: Set 0x0A for ONLY_MICRON_SUPPORT and 0xFF for NO_MICRON_SUPPORT. I don't think MIXED_MICRON_SUPPORT is available here.
For `micron_detect_f17_mod_30_3f`: Set 0x55 for MIXED_MICRON_SUPPORT, set 0xAA for ONLY_MICRON_SUPPORT and set 0x00/0xff for NO_MICRON_SUPPORT.
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c@... PS3, Line 1088: LONGOPT_FAMILY = 256, : LONGOPT_MODEL = 257, These are not really used.
Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c@... PS3, Line 218: --soc-family
Instead of taking family/model-min/model-max as inputs, would it make sense to have family name as i […]
Sounds good, will see if I can make it work well
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c@... PS3, Line 239: <HEX_VAL>
I think this will have to be a boolean flag or a tri-state flag like […]
This sounds reasonable. The only downside is if other part vendors are added to this exception field in later architectures. I am not sure that is going to ever happen though.
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c@... PS3, Line 1088: LONGOPT_FAMILY = 256, : LONGOPT_MODEL = 257,
These are not really used.
Oops, forgot to remove these from an earlier revision.
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c@... PS3, Line 1617: if (soc_family == 0x17 && soc_model_min >= 0x0 && soc_model_max <= 0x2f) { : 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; : amd_romsig->qpr_dummy_cycle_f17_mod_00_2f = efs_spi_micron_flag; : } : if (soc_family == 0x17 && soc_model_min >= 0x30 && soc_model_max <= 0x3f) { : amd_romsig->second_gen_efs = 1; : amd_romsig->spi_readmode_f17_mod_30_3f = efs_spi_readmode; : amd_romsig->spi_fastspeed_f17_mod_30_3f = efs_spi_speed; : amd_romsig->micron_detect_f17_mod_30_3f = efs_spi_micron_flag; : }
Use `else of` to make sure, these branches cannot be entered twice (by making a mistake in the condi […]
I cannot think of any combinations of these 3 numbers where any of these conditionals would be executed more than once. Let me know if I am missing any
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Martin Roth, Marshall Dawson, Nikolai Vyssotski, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42566
to look at the new patch set (#4).
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
util/amdfwtool: Add support for EFS SPI values for F17h and F15h
The Embedded Firmware Structure contains various SPI parameters for the PSP to program. This change adds support to amdfwtool for populating these values as well specifying SOC Family and Model.
BUG=b:158755102 TEST=Read EFS values at appropriate offsets using a hex editor. Boot test on Tremblye and Morphius.
Change-Id: I87c4d44183ca65a5570de5e0c7f9b44aa6dd82f9 Signed-off-by: Matt Papageorge matt.papageorge@amd.corp-partner.google.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 136 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/42566/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c@... PS4, Line 241: printf(" 0x2 Micron parts optional, this option is only\n"); line over 96 characters
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c@... PS4, Line 1093: static const char *optstring = "x:i:g:AMS:p:b:s:r:k:c:n:d:t:u:w:m:T:z:J:B:K:L:Y:N:UW:I:a:Q:V:e:v:j:y:G:O:X:F:H:o:f:l:hZ:qR:P:C:"; line over 96 characters
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c@... PS4, Line 1603: : if else if
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c@... PS4, Line 1642: if (!soc_name_set) else
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c@... PS4, Line 219: are ST, RV, PCO, RN, or LCN\ nit: It might be helpful to add full names as well so that it is clear what they mean:
ST = Stoneyridge PCO = Picasso
etc.
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c@... PS4, Line 1593: if (soc_name != NULL) { nit: This block can be moved into its own function. Something like `set_spi_params()`.
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c@... PS4, Line 1603: : if
else if
+1. Then you can also drop the `soc_name_set` variable.
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c@... PS4, Line 1625: amd_romsig->micron_detect_f17_mod_30_3f = efs_spi_micron_flag; This is not correct. `micron_detect_f17_mod_30_3f` should be set in the switch case below instead of `qpr_dummy_cycle_f17_mod_00_2f`.
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c@... PS4, Line 1629: qpr_dummy_cycle_f17_mod_00_2f This is not used by Fam17H Mod30-3fH. Instead it should be `micron_detect_f17_mod_30_3f`
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Martin Roth, Marshall Dawson, Nikolai Vyssotski, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42566
to look at the new patch set (#5).
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
util/amdfwtool: Add support for EFS SPI values for F17h and F15h
The Embedded Firmware Structure contains various SPI parameters for the PSP to program. This change adds support to amdfwtool for populating these values as well specifying SOC Family and Model.
BUG=b:158755102 TEST=Read EFS values at appropriate offsets using a hex editor. Boot test on Tremblye and Morphius.
Change-Id: I87c4d44183ca65a5570de5e0c7f9b44aa6dd82f9 Signed-off-by: Matt Papageorge matt.papageorge@amd.corp-partner.google.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 139 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/42566/5
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42566/5/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/5/util/amdfwtool/amdfwtool.c@... PS5, Line 430: typedef ugh, I hate typedefs.
https://review.coreboot.org/c/coreboot/+/42566/5/util/amdfwtool/amdfwtool.c@... PS5, Line 1238: Can you clang-format?
https://review.coreboot.org/c/coreboot/+/42566/5/util/amdfwtool/amdfwtool.c@... PS5, Line 1645: set_efs_table Don't we want to returns some kind of error so the build fails?
Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 6:
(6 comments)
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c@... PS4, Line 219: are ST, RV, PCO, RN, or LCN\
nit: It might be helpful to add full names as well so that it is clear what they mean: […]
Done
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c@... PS4, Line 1593: if (soc_name != NULL) {
nit: This block can be moved into its own function. Something like `set_spi_params()`.
Done
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c@... PS4, Line 1603: : if
+1. Then you can also drop the `soc_name_set` variable.
Done
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c@... PS4, Line 1625: amd_romsig->micron_detect_f17_mod_30_3f = efs_spi_micron_flag;
This is not correct. […]
You are right, my bad
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c@... PS4, Line 1629: qpr_dummy_cycle_f17_mod_00_2f
This is not used by Fam17H Mod30-3fH. […]
Ditto
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c@... PS4, Line 1642: if (!soc_name_set)
else
Done
Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 6:
(5 comments)
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c@... PS3, Line 218: --soc-family
Sounds good, will see if I can make it work well
Done
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c@... PS3, Line 239: <HEX_VAL>
This sounds reasonable. […]
Ack
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c@... PS3, Line 243: 0xaa for only Micron parts.
Use the same wording as for F17h M 30h-3fh?
No longer applicable to the new patch
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c@... PS3, Line 1088: LONGOPT_FAMILY = 256, : LONGOPT_MODEL = 257,
Oops, forgot to remove these from an earlier revision.
Done
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c@... PS3, Line 1617: if (soc_family == 0x17 && soc_model_min >= 0x0 && soc_model_max <= 0x2f) { : 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; : amd_romsig->qpr_dummy_cycle_f17_mod_00_2f = efs_spi_micron_flag; : } : if (soc_family == 0x17 && soc_model_min >= 0x30 && soc_model_max <= 0x3f) { : amd_romsig->second_gen_efs = 1; : amd_romsig->spi_readmode_f17_mod_30_3f = efs_spi_readmode; : amd_romsig->spi_fastspeed_f17_mod_30_3f = efs_spi_speed; : amd_romsig->micron_detect_f17_mod_30_3f = efs_spi_micron_flag; : }
I cannot think of any combinations of these 3 numbers where any of these conditionals would be execu […]
Done
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42566/6/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/6/util/amdfwtool/amdfwtool.c@... PS6, Line 219: printf(" Stoneyridge, Raven, Picasso, Renoir or Lucienne"); line over 96 characters
https://review.coreboot.org/c/coreboot/+/42566/6/util/amdfwtool/amdfwtool.c@... PS6, Line 241: printf(" 0x2 Micron parts optional, this option is only\n"); line over 96 characters
https://review.coreboot.org/c/coreboot/+/42566/6/util/amdfwtool/amdfwtool.c@... PS6, Line 1093: static const char *optstring = "x:i:g:AMS:p:b:s:r:k:c:n:d:t:u:w:m:T:z:J:B:K:L:Y:N:UW:I:a:Q:V:e:v:j:y:G:O:X:F:H:o:f:l:hZ:qR:P:C:"; line over 96 characters
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42566/5/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/5/util/amdfwtool/amdfwtool.c@... PS5, Line 222: printf("--spi-speed <HEX_VAL> SPI speed to place in EFS Table\n"); Seems like --spi-speed and --spi-read-mode below should change places?
Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42566/5/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/5/util/amdfwtool/amdfwtool.c@... PS5, Line 222: printf("--spi-speed <HEX_VAL> SPI speed to place in EFS Table\n");
Seems like --spi-speed and --spi-read-mode below should change places?
Thanks, you are right, I have reworked this change many times now I must have confused myself
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Martin Roth, Marshall Dawson, Nikolai Vyssotski, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42566
to look at the new patch set (#7).
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
util/amdfwtool: Add support for EFS SPI values for F17h and F15h
The Embedded Firmware Structure contains various SPI parameters for the PSP to program. This change adds support to amdfwtool for populating these values as well specifying SOC Family and Model.
BUG=b:158755102 TEST=Read EFS values at appropriate offsets using a hex editor. Boot test on Tremblye and Morphius.
Change-Id: I87c4d44183ca65a5570de5e0c7f9b44aa6dd82f9 Signed-off-by: Matt Papageorge matt.papageorge@amd.corp-partner.google.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 143 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/42566/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42566/7/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/7/util/amdfwtool/amdfwtool.c@... PS7, Line 219: printf(" Stoneyridge, Raven, Picasso, Renoir or Lucienne"); line over 96 characters
https://review.coreboot.org/c/coreboot/+/42566/7/util/amdfwtool/amdfwtool.c@... PS7, Line 241: printf(" 0x2 Micron parts optional, this option is only\n"); line over 96 characters
https://review.coreboot.org/c/coreboot/+/42566/7/util/amdfwtool/amdfwtool.c@... PS7, Line 1093: static const char *optstring = "x:i:g:AMS:p:b:s:r:k:c:n:d:t:u:w:m:T:z:J:B:K:L:Y:N:UW:I:a:Q:V:e:v:j:y:G:O:X:F:H:o:f:l:hZ:qR:P:C:"; line over 96 characters
Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 7:
(3 comments)
Updated to address feedback
https://review.coreboot.org/c/coreboot/+/42566/5/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/5/util/amdfwtool/amdfwtool.c@... PS5, Line 222: printf("--spi-speed <HEX_VAL> SPI speed to place in EFS Table\n");
Thanks, you are right, I have reworked this change many times now I must have confused myself
Done
https://review.coreboot.org/c/coreboot/+/42566/5/util/amdfwtool/amdfwtool.c@... PS5, Line 1238:
Can you clang-format?
Done
https://review.coreboot.org/c/coreboot/+/42566/5/util/amdfwtool/amdfwtool.c@... PS5, Line 1645: set_efs_table
Don't we want to returns some kind of error so the build fails?
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 7: Code-Review+1
(4 comments)
Some minor nits.
https://review.coreboot.org/c/coreboot/+/42566/7/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/7/util/amdfwtool/amdfwtool.c@... PS7, Line 222: speed nit: fast speed
https://review.coreboot.org/c/coreboot/+/42566/7/util/amdfwtool/amdfwtool.c@... PS7, Line 1245: strcmp nit: Use strcasecmp here and below?
https://review.coreboot.org/c/coreboot/+/42566/7/util/amdfwtool/amdfwtool.c@... PS7, Line 1254: 0 It would be good to have enums for these values. Just easier to read.
https://review.coreboot.org/c/coreboot/+/42566/7/util/amdfwtool/amdfwtool.c@... PS7, Line 1511: soc_name = optarg; Just a thought. Probably, we should do strcasecmp right away and set platform_id to some enum
enum platform { PLATFORM_STONEYRIDGE, PLATFORM_RAVEN, PLATFORM_PICASSO, PLATFORM_RENOIR, PLATFORM_LUCIENNE, };
so that rest of the code just needs to check enum and not perform string comparison again. This will allow the tool to use platform/SoC comparison in other places as well.
It's okay if you don't want to do it right now. We can address that later as well.
Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/c/coreboot/+/42566/7/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/7/util/amdfwtool/amdfwtool.c@... PS7, Line 222: speed
nit: fast speed
Done
https://review.coreboot.org/c/coreboot/+/42566/7/util/amdfwtool/amdfwtool.c@... PS7, Line 1245: strcmp
nit: Use strcasecmp here and below?
Done
https://review.coreboot.org/c/coreboot/+/42566/7/util/amdfwtool/amdfwtool.c@... PS7, Line 1254: 0
It would be good to have enums for these values. Just easier to read.
Done
https://review.coreboot.org/c/coreboot/+/42566/7/util/amdfwtool/amdfwtool.c@... PS7, Line 1511: soc_name = optarg;
Just a thought. Probably, we should do strcasecmp right away and set platform_id to some enum […]
Yea this would be good thing to do so future changes do not require a refactoring, should not be too hard.
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Martin Roth, Marshall Dawson, Nikolai Vyssotski, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42566
to look at the new patch set (#8).
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
util/amdfwtool: Add support for EFS SPI values for F17h and F15h
The Embedded Firmware Structure contains various SPI parameters for the PSP to program. This change adds support to amdfwtool for populating these values as well specifying SOC Family and Model.
BUG=b:158755102 TEST=Read EFS values at appropriate offsets using a hex editor. Boot test on Tremblye and Morphius.
Change-Id: I87c4d44183ca65a5570de5e0c7f9b44aa6dd82f9 Signed-off-by: Matt Papageorge matt.papageorge@amd.corp-partner.google.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 181 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/42566/8
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 8:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42566/8/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/8/util/amdfwtool/amdfwtool.c@... PS8, Line 219: printf(" Stoneyridge, Raven, Picasso, Renoir or Lucienne"); line over 96 characters
https://review.coreboot.org/c/coreboot/+/42566/8/util/amdfwtool/amdfwtool.c@... PS8, Line 241: printf(" 0x2 Micron parts optional, this option is only\n"); line over 96 characters
https://review.coreboot.org/c/coreboot/+/42566/8/util/amdfwtool/amdfwtool.c@... PS8, Line 1093: static const char *optstring = "x:i:g:AMS:p:b:s:r:k:c:n:d:t:u:w:m:T:z:J:B:K:L:Y:N:UW:I:a:Q:V:e:v:j:y:G:O:X:F:H:o:f:l:hZ:qR:P:C:"; line over 96 characters
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 8: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/42566/8/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/8/util/amdfwtool/amdfwtool.c@... PS8, Line 1307: if (!strcasecmp(soc_name, "Stoneyridge")) { Can remove the {} here. The whole block only has one statement per condition.
https://review.coreboot.org/c/coreboot/+/42566/8/util/amdfwtool/amdfwtool.c@... PS8, Line 1689: else { }
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Martin Roth, Marshall Dawson, Nikolai Vyssotski, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42566
to look at the new patch set (#9).
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
util/amdfwtool: Add support for EFS SPI values for F17h and F15h
The Embedded Firmware Structure contains various SPI parameters for the PSP to program. This change adds support to amdfwtool for populating these values as well specifying SOC Family and Model.
BUG=b:158755102 TEST=Read EFS values at appropriate offsets using a hex editor. Boot test on Tremblye and Morphius.
Change-Id: I87c4d44183ca65a5570de5e0c7f9b44aa6dd82f9 Signed-off-by: Matt Papageorge matt.papageorge@amd.corp-partner.google.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 180 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/42566/9
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 9:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42566/9/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/9/util/amdfwtool/amdfwtool.c@... PS9, Line 219: printf(" Stoneyridge, Raven, Picasso, Renoir or Lucienne"); line over 96 characters
https://review.coreboot.org/c/coreboot/+/42566/9/util/amdfwtool/amdfwtool.c@... PS9, Line 241: printf(" 0x2 Micron parts optional, this option is only\n"); line over 96 characters
https://review.coreboot.org/c/coreboot/+/42566/9/util/amdfwtool/amdfwtool.c@... PS9, Line 1093: static const char *optstring = "x:i:g:AMS:p:b:s:r:k:c:n:d:t:u:w:m:T:z:J:B:K:L:Y:N:UW:I:a:Q:V:e:v:j:y:G:O:X:F:H:o:f:l:hZ:qR:P:C:"; line over 96 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 10:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42566/10/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/10/util/amdfwtool/amdfwtool.c... PS10, Line 219: printf(" Stoneyridge, Raven, Picasso, Renoir or Lucienne"); line over 96 characters
https://review.coreboot.org/c/coreboot/+/42566/10/util/amdfwtool/amdfwtool.c... PS10, Line 241: printf(" 0x2 Micron parts optional, this option is only\n"); line over 96 characters
https://review.coreboot.org/c/coreboot/+/42566/10/util/amdfwtool/amdfwtool.c... PS10, Line 1093: static const char *optstring = "x:i:g:AMS:p:b:s:r:k:c:n:d:t:u:w:m:T:z:J:B:K:L:Y:N:UW:I:a:Q:V:e:v:j:y:G:O:X:F:H:o:f:l:hZ:qR:P:C:"; line over 96 characters
Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42566/8/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/8/util/amdfwtool/amdfwtool.c@... PS8, Line 1307: if (!strcasecmp(soc_name, "Stoneyridge")) {
Can remove the {} here. The whole block only has one statement per condition.
Done
https://review.coreboot.org/c/coreboot/+/42566/8/util/amdfwtool/amdfwtool.c@... PS8, Line 1689: else
{ }
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 10: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 10:
I assume there is only NDA documentation for EFS?
In the case of a Micron part, will PSP send 0xb1/0xb5 commands to the SPI flash part such that QPR modes on both sides agree about the number of dummy cycles? Or is it necessary to use some proprieatary tool to configure non-volatile registers in the Micron part instead?
In case of a non-Micron part, what happens? One cannot change to QPR modes if power-on defaults about dummy-cycle counts disagree. I think it was a Winbond part that could not use 100MHz with 1-1-2 or 1-2-2 so there is additional dependencies.
I understand if you cannot publish relevant parts of existing BOMs, but documenting which SPI parts are expected to work with the PSP firmwares you release would have some value. And perhaps a general statement in the commit message that SPI parts are no longer as interchangeable as they used to be.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
util/amdfwtool: Add support for EFS SPI values for F17h and F15h
The Embedded Firmware Structure contains various SPI parameters for the PSP to program. This change adds support to amdfwtool for populating these values as well specifying SOC Family and Model.
BUG=b:158755102 TEST=Read EFS values at appropriate offsets using a hex editor. Boot test on Tremblye and Morphius.
Change-Id: I87c4d44183ca65a5570de5e0c7f9b44aa6dd82f9 Signed-off-by: Matt Papageorge matt.papageorge@amd.corp-partner.google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42566 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M util/amdfwtool/amdfwtool.c 1 file changed, 180 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index a5e5110..14ffdb3 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -215,7 +215,31 @@ printf("-q | --anywhere Use any 64-byte aligned addr for Directory\n"); printf("-R | --sharedmem Location of PSP/FW shared memory\n"); printf("-P | --sharedmem-size Maximum size of the PSP/FW shared memory area\n"); + printf("-C | --soc-name <socname> Specify SOC name. Supported names are\n"); + printf(" Stoneyridge, Raven, Picasso, Renoir or Lucienne"); printf("-h | --help show this help\n"); + printf("\nEmbedded Firmware Structure options used by the PSP:\n"); + printf("--spi-speed <HEX_VAL> SPI fast speed to place in EFS Table\n"); + printf(" 0x0 66.66Mhz\n"); + printf(" 0x1 33.33MHz\n"); + printf(" 0x2 22.22MHz\n"); + printf(" 0x3 16.66MHz\n"); + printf(" 0x4 100MHz\n"); + printf(" 0x5 800KHz\n"); + printf("--spi-read-mode <HEX_VAL> SPI read mode to place in EFS Table\n"); + printf(" 0x0 Normal Read (up to 33M)\n"); + printf(" 0x1 Reserved\n"); + printf(" 0x2 Dual IO (1-1-2)\n"); + printf(" 0x3 Quad IO (1-1-4)\n"); + printf(" 0x4 Dual IO (1-2-2)\n"); + printf(" 0x5 Quad IO (1-4-4)\n"); + printf(" 0x6 Normal Read (up to 66M)\n"); + printf(" 0x7 Fast Read\n"); + printf("--spi-micron-flag <HEX_VAL> Micron SPI part support for RV and later SOC\n"); + printf(" 0x0 Micron parts are not used\n"); + printf(" 0x1 Micron parts are always used\n"); + printf(" 0x2 Micron parts optional, this option is only\n"); + printf(" supported with RN/LCN SOC\n"); }
typedef enum _amd_bios_type { @@ -413,7 +437,26 @@ uint32_t bios0_entry; /* todo: add way to select correct entry */ uint32_t bios1_entry; uint32_t bios2_entry; - uint32_t reserved[0x2c]; /* 0x24 - 0x4f */ + uint32_t second_gen_efs; + uint32_t bios3_entry; + uint32_t reserved_2Ch; + uint32_t promontory_fw_ptr; + uint32_t lp_promontory_fw_ptr; + uint32_t reserved_38h; + uint32_t reserved_3Ch; + uint8_t spi_readmode_f15_mod_60_6f; + uint8_t fast_speed_new_f15_mod_60_6f; + uint8_t reserved_42h; + uint8_t spi_readmode_f17_mod_00_2f; + uint8_t spi_fastspeed_f17_mod_00_2f; + uint8_t qpr_dummy_cycle_f17_mod_00_2f; + uint8_t reserved_46h; + uint8_t spi_readmode_f17_mod_30_3f; + uint8_t spi_fastspeed_f17_mod_30_3f; + uint8_t micron_detect_f17_mod_30_3f; + uint8_t reserved_4Ah; + uint8_t reserved_4Bh; + uint32_t reserved_4Ch; } __attribute__((packed, aligned(16))) embedded_firmware;
typedef struct _psp_directory_header { @@ -1038,8 +1081,16 @@
fill_dir_header(biosdir, count, cookie); } -// Unused values: CDE -static const char *optstring = "x:i:g:AMS:p:b:s:r:k:c:n:d:t:u:w:m:T:z:J:B:K:L:Y:N:UW:I:a:Q:V:e:v:j:y:G:O:X:F:H:o:f:l:hZ:qR:P:"; + +enum { + /* begin after ASCII characters */ + LONGOPT_SPI_READ_MODE = 256, + LONGOPT_SPI_SPEED = 257, + LONGOPT_SPI_MICRON_FLAG = 258, +}; + +// Unused values: DE +static const char *optstring = "x:i:g:AMS:p:b:s:r:k:c:n:d:t:u:w:m:T:z:J:B:K:L:Y:N:UW:I:a:Q:V:e:v:j:y:G:O:X:F:H:o:f:l:hZ:qR:P:C:";
static struct option long_options[] = { {"xhci", required_argument, 0, 'x' }, @@ -1086,6 +1137,10 @@ {"mp2-config", required_argument, 0, 'X' }, {"apob-nv-base", required_argument, 0, 'F' }, {"apob-nv-size", required_argument, 0, 'H' }, + /* Embedded Firmware Structure items*/ + {"spi-read-mode", required_argument, 0, LONGOPT_SPI_READ_MODE }, + {"spi-speed", required_argument, 0, LONGOPT_SPI_SPEED }, + {"spi-micron-flag", required_argument, 0, LONGOPT_SPI_MICRON_FLAG }, /* other */ {"output", required_argument, 0, 'o' }, {"flashsize", required_argument, 0, 'f' }, @@ -1093,6 +1148,7 @@ {"anywhere", no_argument, 0, 'q' }, {"sharedmem", required_argument, 0, 'R' }, {"sharedmem-size", required_argument, 0, 'P' }, + {"soc-name", required_argument, 0, 'C' }, {"help", no_argument, 0, 'h' }, {NULL, 0, 0, 0 } }; @@ -1178,6 +1234,91 @@ } }
+enum platform { + PLATFORM_UNKNOWN, + PLATFORM_STONEYRIDGE, + PLATFORM_RAVEN, + PLATFORM_PICASSO, + PLATFORM_RENOIR, + PLATFORM_LUCIENNE, +}; + +static int set_efs_table(uint8_t soc_id, embedded_firmware *amd_romsig, + uint8_t efs_spi_readmode, uint8_t efs_spi_speed, + uint8_t efs_spi_micron_flag) +{ + if ((efs_spi_readmode == 0xFF) || (efs_spi_speed == 0xFF)) { + printf("Error: EFS read mode and SPI speed must be set\n"); + return 1; + } + 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) { + case 0: + amd_romsig->qpr_dummy_cycle_f17_mod_00_2f = 0xff; + break; + case 1: + amd_romsig->qpr_dummy_cycle_f17_mod_00_2f = 0xa; + break; + default: + printf("Error: EFS Micron flag must be correctly set.\n\n"); + return 1; + } + break; + case PLATFORM_RENOIR: + case PLATFORM_LUCIENNE: + amd_romsig->second_gen_efs = 1; + 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) { + case 0: + amd_romsig->micron_detect_f17_mod_30_3f = 0xff; + break; + case 1: + amd_romsig->micron_detect_f17_mod_30_3f = 0xaa; + break; + case 2: + amd_romsig->micron_detect_f17_mod_30_3f = 0x55; + break; + default: + printf("Error: EFS Micron flag must be correctly set.\n\n"); + return 1; + } + break; + case PLATFORM_UNKNOWN: + default: + printf("Error: Invalid SOC name.\n\n"); + return 1; + } + return 0; +} + +static int identify_platform(char *soc_name) +{ + if (!strcasecmp(soc_name, "Stoneyridge")) + return PLATFORM_STONEYRIDGE; + else if (!strcasecmp(soc_name, "Raven")) + return PLATFORM_RAVEN; + else if (!strcasecmp(soc_name, "Picasso")) + return PLATFORM_PICASSO; + else if (!strcasecmp(soc_name, "Renoir")) + return PLATFORM_RENOIR; + else if (!strcasecmp(soc_name, "Lucienne")) + return PLATFORM_LUCIENNE; + else + return PLATFORM_UNKNOWN; + +} + int main(int argc, char **argv) { int c; @@ -1200,6 +1341,11 @@ bool any_location = 0; uint32_t romsig_offset; uint32_t rom_base_address; + uint8_t soc_id = PLATFORM_UNKNOWN; + uint8_t efs_spi_readmode = 0xff; + uint8_t efs_spi_speed = 0xff; + uint8_t efs_spi_micron_flag = 0xff; + int multi = 0;
while (1) { @@ -1394,6 +1540,26 @@ register_fw_filename(AMD_FW_PSP_VERSTAGE, sub, optarg); sub = instance = 0; break; + case 'C': + soc_id = identify_platform(optarg); + if (soc_id == PLATFORM_UNKNOWN) { + printf("Error: Invalid SOC name specified\n\n"); + retval = 1; + } + sub = instance = 0; + break; + case LONGOPT_SPI_READ_MODE: + efs_spi_readmode = strtoull(optarg, NULL, 16); + sub = instance = 0; + break; + case LONGOPT_SPI_SPEED: + efs_spi_speed = strtoull(optarg, NULL, 16); + sub = instance = 0; + break; + case LONGOPT_SPI_MICRON_FLAG: + efs_spi_micron_flag = strtoull(optarg, NULL, 16); + sub = instance = 0; + break; case 'o': output = optarg; break; @@ -1512,6 +1678,17 @@ amd_romsig->gec_entry = 0; amd_romsig->xhci_entry = 0;
+ if (soc_id != PLATFORM_UNKNOWN) { + retval = set_efs_table(soc_id, amd_romsig, efs_spi_readmode, + efs_spi_speed, efs_spi_micron_flag); + if (retval) { + printf("ERROR: Failed to initialize EFS table!\n"); + return retval; + } + } else { + printf("WARNING: No SOC name specified.\n"); + } + integrate_firmwares(&ctx, amd_romsig, amd_fw_table);
ctx.current = ALIGN(ctx.current, 0x10000U); /* TODO: is it necessary? */
-
build bot (Jenkins) (Code Review) -
Furquan Shaikh (Code Review)
-
Kyösti Mälkki (Code Review)
-
Matt Papageorge (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Raul Rangel (Code Review)