Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42859 )
Change subject: amdfwtool: Take a config file instead of command line parameters ......................................................................
Patch Set 63:
(7 comments)
https://review.coreboot.org/c/coreboot/+/42859/60/src/soc/amd/picasso/Makefi... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42859/60/src/soc/amd/picasso/Makefi... PS60, Line 288: --whitelist
It looks like we lost ability to add whitelist option. […]
Done
https://review.coreboot.org/c/coreboot/+/42859/60/src/soc/amd/picasso/Makefi... PS60, Line 227: OPT_TOKEN_UNLOCK=$(call add_opt_prefix, $(PSP_TOKEN_UNLOCK), "") : OPT_PSP_USE_PSPSECUREOS=$(call add_opt_prefix, $(PSP_USE_PSPSECUREOS), "") : OPT_PSP_LOAD_MP2_FW=$(call add_opt_prefix, $(PSP_LOAD_MP2_FW), "") : OPT_PSP_LOAD_S0I3_FW=$(call add_opt_prefix, $(PSP_LOAD_S0I3_FW), "")
Do we really need these anymore? We are not really trying to concatenate two strings as we used to d […]
Done
https://review.coreboot.org/c/coreboot/+/42859/60/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42859/60/util/amdfwtool/amdfwtool.c... PS60, Line 1464: process_config(config_handle, &cb_config);
We should be checking the return status here if the config file is parsed correctly and failing the […]
Done
https://review.coreboot.org/c/coreboot/+/42859/60/util/amdfwtool/data_parse.... File util/amdfwtool/data_parse.c:
https://review.coreboot.org/c/coreboot/+/42859/60/util/amdfwtool/data_parse.... PS60, Line 48: void
We really should be returning status and failing the build if we can't parse a non-blank/non-comment […]
Done
https://review.coreboot.org/c/coreboot/+/42859/60/util/amdfwtool/data_parse.... PS60, Line 61: fw_type = AMD_FW_INVALID;
It may make sense to init fw_type = AMD_FW_INVALID at the top and then set fw_type to a new type her […]
Done
https://review.coreboot.org/c/coreboot/+/42859/60/util/amdfwtool/data_parse.... PS60, Line 262: fw_type = AMD_BIOS_INVALID;
It may make sense to split find_register_fw_filename() into two functions e.g. […]
Done
https://review.coreboot.org/c/coreboot/+/42859/60/util/amdfwtool/data_parse.... PS60, Line 306: if (!regexec(&blank_or_comment_expr, oneline, 0, NULL, 0)) { : /* skip comment and blank */ : retval = 1; : }
This does not seem to do much. […]
Done