Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31729
Change subject: util/amdfwtool: Introduce combo-capable option ......................................................................
util/amdfwtool: Introduce combo-capable option
There are effectively two unique sets of arguments for the utility, causing one of two tables to be constructed. Both tables are identical, however, and therefore the only practical difference is the offset in the Embedded Firmware Structure which holds the pointer to the table.
This patch is part 1 of 2 to reduce the number of command-line options to amdfwtool. Create a new option that is used as an indicator for which Embedded Firmware offset to use. Part 2 will be added once makefiles no longer use the duplicated options.
This patch also adds two new options for fanless SMU firmware to be used instead of the ones that will be removed in part 2.
TEST=Confirm amdfw.rom file is unchanged before/after when building google/grunt BUG=b:126691068
Change-Id: I249700c6addad1c0ecb495a406ffe7a022dd920b Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 28 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/31729/1
diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index e1c59ae..7076829 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -164,9 +164,11 @@ printf("-g | --gec <FILE> Add GEC blob\n");
printf("\nPSP options:\n"); + printf("-A | --combo-capable Use PSP dir. offset able to support combo\n"); printf("-p | --pubkey <FILE> Add pubkey\n"); printf("-b | --bootloader <FILE> Add bootloader\n"); printf("-s | --smufirmware <FILE> Add smufirmware\n"); + printf("-j | --smufnfirmware <FILE> Add fanless smufirmware\n"); printf("-r | --recovery <FILE> Add recovery\n"); printf("-k | --rtmpubkey <FILE> Add rtmpubkey\n"); printf("-c | --secureos <FILE> Add secureos\n"); @@ -175,6 +177,7 @@ printf("-t | --trustlets <FILE> Add trustlets\n"); printf("-u | --trustletkey <FILE> Add trustletkey\n"); printf("-w | --smufirmware2 <FILE> Add smufirmware2\n"); + printf("-e | --smufnfirmware2 <FILE> Add fanless smufirmware2\n"); printf("-m | --smuscs <FILE> Add smuscs\n");
#if PSP2 @@ -474,9 +477,9 @@
#if PSP2 static const char *optstring = - "x:i:g:p:b:s:r:k:c:n:d:t:u:w:m:P:B:S:L:R:K:C:N:D:T:U:W:E:M:o:f:l:h"; + "x:i:g:Ap:b:s:r:k:c:n:d:t:u:w:e:j:m:P:B:S:L:R:K:C:N:D:T:U:W:E:M:o:f:l:h"; #else -static const char *optstring = "x:i:g:p:b:s:r:k:c:n:d:t:u:w:m:o:f:l:h"; +static const char *optstring = "x:i:g:Ap:b:s:r:k:c:n:d:t:u:w:e:j:m:o:f:l:h"; #endif
static struct option long_options[] = { @@ -484,9 +487,11 @@ {"imc", required_argument, 0, 'i' }, {"gec", required_argument, 0, 'g' }, /* PSP */ + {"combo-capable", no_argument, 0, 'A' }, {"pubkey", required_argument, 0, 'p' }, {"bootloader", required_argument, 0, 'b' }, {"smufirmware", required_argument, 0, 's' }, + {"smufnfirmware", required_argument, 0, 'j' }, {"recovery", required_argument, 0, 'r' }, {"rtmpubkey", required_argument, 0, 'k' }, {"secureos", required_argument, 0, 'c' }, @@ -495,6 +500,7 @@ {"trustlets", required_argument, 0, 't' }, {"trustletkey", required_argument, 0, 'u' }, {"smufirmware2", required_argument, 0, 'w' }, + {"smufnfirmware2", required_argument, 0, 'e' }, {"smuscs", required_argument, 0, 'm' },
/* TODO: PSP2 */ @@ -574,6 +580,7 @@ uint32_t current; embedded_firmware *amd_romsig; psp_directory_table *pspdir; + int comboable = 0;
int targetfd; char *output = NULL; @@ -600,6 +607,9 @@ case 'g': register_fw_filename(AMD_FW_GEC, optarg, 0); break; + case 'A': + comboable = 1; + break; case 'p': register_fw_filename(AMD_FW_PSP_PUBKEY, optarg, 1); pspflag = 1; @@ -613,6 +623,11 @@ optarg, 1); pspflag = 1; break; + case 'j': + register_fw_filename(AMD_FW_PSP_SMU_FN_FIRMWARE, + optarg, 1); + pspflag = 1; + break; case 'r': register_fw_filename(AMD_FW_PSP_RECOVERY, optarg, 1); pspflag = 1; @@ -647,6 +662,11 @@ optarg, 1); pspflag = 1; break; + case 'e': + register_fw_filename(AMD_FW_PSP_SMU_FN_FIRMWARE2, + optarg, 1); + pspflag = 1; + break; case 'm': register_fw_filename(AMD_FW_PSP_SMUSCS, optarg, 1); pspflag = 1; @@ -816,11 +836,14 @@ current = integrate_firmwares(rom, current, amd_romsig, amd_fw_table, rom_size);
- if (pspflag == 1) { - current = ALIGN(current, 0x10000U); - pspdir = (void *)(rom + current); + current = ALIGN(current, 0x10000U); + if (psp2flag || comboable) + amd_romsig->comboable = current + rom_base_address; + else amd_romsig->psp_entry = current + rom_base_address;
+ if (pspflag == 1) { + pspdir = (void *)(rom + current); current += 0x200; /* Conservative size of pspdir */ current = integrate_psp_firmwares(rom, current, pspdir, amd_psp_fw_table, rom_size); @@ -828,9 +851,7 @@
#if PSP2 if (psp2flag == 1) { - current = ALIGN(current, 0x10000U); /* PSP2 dir */ psp2dir = (void *)(rom + current); - amd_romsig->comboable = current + rom_base_address; current += 0x200; /* Add conservative size of psp2dir. */
#if PSP_COMBO
Marshall Dawson has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/31729 )
Change subject: util/amdfwtool: Introduce combo-capable option ......................................................................
util/amdfwtool: Introduce combo-capable option
There are effectively two unique sets of arguments for the utility, causing one of two tables to be constructed. Both tables are identical, however, and therefore the only practical difference is the offset in the Embedded Firmware Structure which holds the pointer to the table.
This patch is part 1 of 2 to reduce the number of command-line options to amdfwtool. Create a new option that is used as an indicator for which Embedded Firmware offset to use. Part 2 will be added once makefiles no longer use the duplicated options.
This patch also adds two new options for fanless SMU firmware to be used instead of the ones that will be removed in part 2.
TEST=Confirm amdfw.rom file is unchanged before/after when building google/grunt BUG=b:126691068
Change-Id: I249700c6addad1c0ecb495a406ffe7a022dd920b Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 28 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/31729/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31729 )
Change subject: util/amdfwtool: Introduce combo-capable option ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31729/2/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/#/c/31729/2/util/amdfwtool/amdfwtool.c@167 PS2, Line 167: dir directory?
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31729
to look at the new patch set (#3).
Change subject: util/amdfwtool: Introduce combo-capable option ......................................................................
util/amdfwtool: Introduce combo-capable option
There are effectively two unique sets of arguments for the utility, causing one of two tables to be constructed. Both tables are identical, however, and therefore the only practical difference is the offset in the Embedded Firmware Structure which holds the pointer to the table.
This patch is part 1 of 2 to reduce the number of command-line options to amdfwtool. Create a new option that is used as an indicator for which Embedded Firmware offset to use. Part 2 will be added once makefiles no longer use the duplicated options.
This patch also adds two new options for fanless SMU firmware to be used instead of the ones that will be removed in part 2.
TEST=Verify no difference in amdfw.rom for google/grunt before and after the patch is applied BUG=b:126691068
Change-Id: I249700c6addad1c0ecb495a406ffe7a022dd920b Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 29 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/31729/3
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31729 )
Change subject: util/amdfwtool: Introduce combo-capable option ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31729/2/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/#/c/31729/2/util/amdfwtool/amdfwtool.c@167 PS2, Line 167: dir
directory?
Done
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31729 )
Change subject: util/amdfwtool: Introduce combo-capable option ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31729/3/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/#/c/31729/3/util/amdfwtool/amdfwtool.c@172 PS3, Line 172: n Nit: I guess this matches AMD_FW_PSP_SMU_FN_FIRMWARE, but for fanless, smufLfirmware would make more sense to me. FN makes it seem like it's the fan version.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31729 )
Change subject: util/amdfwtool: Introduce combo-capable option ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/31729/3/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/#/c/31729/3/util/amdfwtool/amdfwtool.c@172 PS3, Line 172: n
Nit: I guess this matches AMD_FW_PSP_SMU_FN_FIRMWARE, but for fanless, smufLfirmware would make more […]
And it's remove later.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31729 )
Change subject: util/amdfwtool: Introduce combo-capable option ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31729/3/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/#/c/31729/3/util/amdfwtool/amdfwtool.c@172 PS3, Line 172: n
Nit: I guess this matches AMD_FW_PSP_SMU_FN_FIRMWARE, but for fanless, smufLfirmware would make more […]
Side benefit, maybe we'll be able to drop the "fanless" vernacular. Seems many people confuse it as having something to do with fan control, however it's only referring to SKUs within a certain power envelope.
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31729 )
Change subject: util/amdfwtool: Introduce combo-capable option ......................................................................
util/amdfwtool: Introduce combo-capable option
There are effectively two unique sets of arguments for the utility, causing one of two tables to be constructed. Both tables are identical, however, and therefore the only practical difference is the offset in the Embedded Firmware Structure which holds the pointer to the table.
This patch is part 1 of 2 to reduce the number of command-line options to amdfwtool. Create a new option that is used as an indicator for which Embedded Firmware offset to use. Part 2 will be added once makefiles no longer use the duplicated options.
This patch also adds two new options for fanless SMU firmware to be used instead of the ones that will be removed in part 2.
TEST=Verify no difference in amdfw.rom for google/grunt before and after the patch is applied BUG=b:126691068
Change-Id: I249700c6addad1c0ecb495a406ffe7a022dd920b Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/31729 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Martin Roth martinroth@google.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 29 insertions(+), 7 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved
diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index e1c59ae..673f31d 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -164,9 +164,12 @@ printf("-g | --gec <FILE> Add GEC blob\n");
printf("\nPSP options:\n"); + printf("-A | --combo-capable Place PSP directory pointer at Embedded Firmware\n"); + printf(" offset able to support combo directory\n"); printf("-p | --pubkey <FILE> Add pubkey\n"); printf("-b | --bootloader <FILE> Add bootloader\n"); printf("-s | --smufirmware <FILE> Add smufirmware\n"); + printf("-j | --smufnfirmware <FILE> Add fanless smufirmware\n"); printf("-r | --recovery <FILE> Add recovery\n"); printf("-k | --rtmpubkey <FILE> Add rtmpubkey\n"); printf("-c | --secureos <FILE> Add secureos\n"); @@ -175,6 +178,7 @@ printf("-t | --trustlets <FILE> Add trustlets\n"); printf("-u | --trustletkey <FILE> Add trustletkey\n"); printf("-w | --smufirmware2 <FILE> Add smufirmware2\n"); + printf("-e | --smufnfirmware2 <FILE> Add fanless smufirmware2\n"); printf("-m | --smuscs <FILE> Add smuscs\n");
#if PSP2 @@ -474,9 +478,9 @@
#if PSP2 static const char *optstring = - "x:i:g:p:b:s:r:k:c:n:d:t:u:w:m:P:B:S:L:R:K:C:N:D:T:U:W:E:M:o:f:l:h"; + "x:i:g:Ap:b:s:r:k:c:n:d:t:u:w:e:j:m:P:B:S:L:R:K:C:N:D:T:U:W:E:M:o:f:l:h"; #else -static const char *optstring = "x:i:g:p:b:s:r:k:c:n:d:t:u:w:m:o:f:l:h"; +static const char *optstring = "x:i:g:Ap:b:s:r:k:c:n:d:t:u:w:e:j:m:o:f:l:h"; #endif
static struct option long_options[] = { @@ -484,9 +488,11 @@ {"imc", required_argument, 0, 'i' }, {"gec", required_argument, 0, 'g' }, /* PSP */ + {"combo-capable", no_argument, 0, 'A' }, {"pubkey", required_argument, 0, 'p' }, {"bootloader", required_argument, 0, 'b' }, {"smufirmware", required_argument, 0, 's' }, + {"smufnfirmware", required_argument, 0, 'j' }, {"recovery", required_argument, 0, 'r' }, {"rtmpubkey", required_argument, 0, 'k' }, {"secureos", required_argument, 0, 'c' }, @@ -495,6 +501,7 @@ {"trustlets", required_argument, 0, 't' }, {"trustletkey", required_argument, 0, 'u' }, {"smufirmware2", required_argument, 0, 'w' }, + {"smufnfirmware2", required_argument, 0, 'e' }, {"smuscs", required_argument, 0, 'm' },
/* TODO: PSP2 */ @@ -574,6 +581,7 @@ uint32_t current; embedded_firmware *amd_romsig; psp_directory_table *pspdir; + int comboable = 0;
int targetfd; char *output = NULL; @@ -600,6 +608,9 @@ case 'g': register_fw_filename(AMD_FW_GEC, optarg, 0); break; + case 'A': + comboable = 1; + break; case 'p': register_fw_filename(AMD_FW_PSP_PUBKEY, optarg, 1); pspflag = 1; @@ -613,6 +624,11 @@ optarg, 1); pspflag = 1; break; + case 'j': + register_fw_filename(AMD_FW_PSP_SMU_FN_FIRMWARE, + optarg, 1); + pspflag = 1; + break; case 'r': register_fw_filename(AMD_FW_PSP_RECOVERY, optarg, 1); pspflag = 1; @@ -647,6 +663,11 @@ optarg, 1); pspflag = 1; break; + case 'e': + register_fw_filename(AMD_FW_PSP_SMU_FN_FIRMWARE2, + optarg, 1); + pspflag = 1; + break; case 'm': register_fw_filename(AMD_FW_PSP_SMUSCS, optarg, 1); pspflag = 1; @@ -816,11 +837,14 @@ current = integrate_firmwares(rom, current, amd_romsig, amd_fw_table, rom_size);
- if (pspflag == 1) { - current = ALIGN(current, 0x10000U); - pspdir = (void *)(rom + current); + current = ALIGN(current, 0x10000U); + if (psp2flag || comboable) + amd_romsig->comboable = current + rom_base_address; + else amd_romsig->psp_entry = current + rom_base_address;
+ if (pspflag == 1) { + pspdir = (void *)(rom + current); current += 0x200; /* Conservative size of pspdir */ current = integrate_psp_firmwares(rom, current, pspdir, amd_psp_fw_table, rom_size); @@ -828,9 +852,7 @@
#if PSP2 if (psp2flag == 1) { - current = ALIGN(current, 0x10000U); /* PSP2 dir */ psp2dir = (void *)(rom + current); - amd_romsig->comboable = current + rom_base_address; current += 0x200; /* Add conservative size of psp2dir. */
#if PSP_COMBO
-
Marshall Dawson (Code Review) -
Martin Roth (Code Review)
-
Paul Menzel (Code Review)