Hello Zheng Bao,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/46867
to review the following change.
Change subject: amdfwtool: Add an option to show debug message ......................................................................
amdfwtool: Add an option to show debug message
Change-Id: I3e3bcc2c9e1b3edfed1ce845c1603b2a9a2bb044 Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 35 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/46867/1
diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index 561b511..a58cdb2 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -216,6 +216,7 @@ printf(" 0x2 Micron parts optional, this option is only\n"); printf(" supported with RN/LCN SOC\n"); printf("-c | --config <config file> Config file\n"); + printf("-d | --debug Print debug message\n"); printf("-D | --depend List out the firmware files\n"); }
@@ -596,6 +597,27 @@ } }
+/* For debugging */ +static void dump_psp_firmwares(amd_fw_entry *fw_table) +{ + amd_fw_entry *index; + + for (index = fw_table; index->type != AMD_FW_INVALID; index++) { + if (index->filename) + printf(" filename=%s\n", index->filename); + } +} + +static void dump_bdt_firmwares(amd_bios_entry *fw_table) +{ + amd_bios_entry *index; + + for (index = fw_table; index->type != AMD_BIOS_INVALID; index++) { + if (index->filename) + printf(" filename=%s\n", index->filename); + } +} + static void free_psp_firmware_filenames(amd_fw_entry *fw_table) { amd_fw_entry *index; @@ -1007,9 +1029,9 @@ LONGOPT_SPI_MICRON_FLAG = 258, };
-/* Unused values: BGJKNXYbdkmprstuwyz*/ +/* Unused values: BGJKNXYbkmprstuwyz*/ static const char *optstring = "x:i:g:AMn:T:SPLUW:I:a:Q:V:e:v:j:O:F:" - "H:o:f:l:hZ:qR:C:c:E:D"; + "H:o:f:l:hZ:qR:C:c:E:dD";
static struct option long_options[] = { {"xhci", required_argument, 0, 'x' }, @@ -1052,6 +1074,7 @@ {"soc-name", required_argument, 0, 'C' },
{"config", required_argument, 0, 'c' }, + {"debug", no_argument, 0, 'd' }, {"help", no_argument, 0, 'h' }, {"depend", no_argument, 0, 'D' }, {NULL, 0, 0, 0 } @@ -1250,6 +1273,7 @@
int multi = 0; amd_cb_config cb_config; + int debug = 0; int list_deps = 0;
cb_config.have_whitelist = 0; @@ -1425,6 +1449,9 @@ case 'c': config = optarg; break; + case 'd': + debug = 1; + break; case 'h': usage(); return 0; @@ -1450,6 +1477,12 @@ } fclose(config_handle); } + /* For debug. */ + if (debug == 1) { + dump_psp_firmwares(amd_psp_fw_table); + dump_bdt_firmwares(amd_bios_table); + } + if (!fuse_defined) register_fw_fuse(DEFAULT_SOFT_FUSE_CHAIN);
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46867 )
Change subject: amdfwtool: Add an option to show debug message ......................................................................
Patch Set 2: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/46867/2/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/46867/2/util/amdfwtool/amdfwtool.c@... PS2, Line 604: it would be good to print something like "PSP firmware components:" before printing the filenames to provide some context. same for the dump_bdt_firmwares function below
https://review.coreboot.org/c/coreboot/+/46867/2/util/amdfwtool/amdfwtool.c@... PS2, Line 1481: == 1 I'd drop the == 1 in there
Hello build bot (Jenkins), Zheng Bao, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46867
to look at the new patch set (#4).
Change subject: amdfwtool: Add an option to show debug message ......................................................................
amdfwtool: Add an option to show debug message
Change-Id: I3e3bcc2c9e1b3edfed1ce845c1603b2a9a2bb044 Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 37 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/46867/4
Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46867 )
Change subject: amdfwtool: Add an option to show debug message ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46867/2/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/46867/2/util/amdfwtool/amdfwtool.c@... PS2, Line 604:
it would be good to print something like "PSP firmware components:" before printing the filenames to […]
Done
https://review.coreboot.org/c/coreboot/+/46867/2/util/amdfwtool/amdfwtool.c@... PS2, Line 1481: == 1
I'd drop the == 1 in there
Done
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46867 )
Change subject: amdfwtool: Add an option to show debug message ......................................................................
Patch Set 4: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46867 )
Change subject: amdfwtool: Add an option to show debug message ......................................................................
amdfwtool: Add an option to show debug message
Change-Id: I3e3bcc2c9e1b3edfed1ce845c1603b2a9a2bb044 Signed-off-by: Zheng Bao fishbaozi@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/46867 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, 37 insertions(+), 2 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 561b511..0c4153e 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -216,6 +216,7 @@ printf(" 0x2 Micron parts optional, this option is only\n"); printf(" supported with RN/LCN SOC\n"); printf("-c | --config <config file> Config file\n"); + printf("-d | --debug Print debug message\n"); printf("-D | --depend List out the firmware files\n"); }
@@ -596,6 +597,29 @@ } }
+/* For debugging */ +static void dump_psp_firmwares(amd_fw_entry *fw_table) +{ + amd_fw_entry *index; + + printf("PSP firmware components:"); + for (index = fw_table; index->type != AMD_FW_INVALID; index++) { + if (index->filename) + printf(" filename=%s\n", index->filename); + } +} + +static void dump_bdt_firmwares(amd_bios_entry *fw_table) +{ + amd_bios_entry *index; + + printf("BIOS Directory Table (BDT) components:"); + for (index = fw_table; index->type != AMD_BIOS_INVALID; index++) { + if (index->filename) + printf(" filename=%s\n", index->filename); + } +} + static void free_psp_firmware_filenames(amd_fw_entry *fw_table) { amd_fw_entry *index; @@ -1007,9 +1031,9 @@ LONGOPT_SPI_MICRON_FLAG = 258, };
-/* Unused values: BGJKNXYbdkmprstuwyz*/ +/* Unused values: BGJKNXYbkmprstuwyz*/ static const char *optstring = "x:i:g:AMn:T:SPLUW:I:a:Q:V:e:v:j:O:F:" - "H:o:f:l:hZ:qR:C:c:E:D"; + "H:o:f:l:hZ:qR:C:c:E:dD";
static struct option long_options[] = { {"xhci", required_argument, 0, 'x' }, @@ -1052,6 +1076,7 @@ {"soc-name", required_argument, 0, 'C' },
{"config", required_argument, 0, 'c' }, + {"debug", no_argument, 0, 'd' }, {"help", no_argument, 0, 'h' }, {"depend", no_argument, 0, 'D' }, {NULL, 0, 0, 0 } @@ -1250,6 +1275,7 @@
int multi = 0; amd_cb_config cb_config; + int debug = 0; int list_deps = 0;
cb_config.have_whitelist = 0; @@ -1425,6 +1451,9 @@ case 'c': config = optarg; break; + case 'd': + debug = 1; + break; case 'h': usage(); return 0; @@ -1450,6 +1479,12 @@ } fclose(config_handle); } + /* For debug. */ + if (debug) { + dump_psp_firmwares(amd_psp_fw_table); + dump_bdt_firmwares(amd_bios_table); + } + if (!fuse_defined) register_fw_fuse(DEFAULT_SOFT_FUSE_CHAIN);
-
Bao Zheng (Code Review) -
Felix Held (Code Review)
-
Patrick Georgi (Code Review)