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 66:
(8 comments)
https://review.coreboot.org/c/coreboot/+/42859/65/src/soc/amd/picasso/Makefi... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42859/65/src/soc/amd/picasso/Makefi... PS65, Line 238:
Could you put a comment on DEP_FILES explaining what it's doing? […]
Done
https://review.coreboot.org/c/coreboot/+/42859/65/src/soc/amd/picasso/Makefi... PS65, Line 259: $(DEP_FILES) \
This does not make sense to me. Why would we want to add dependent files to arguments of AMDFWTOOL. […]
Done
https://review.coreboot.org/c/coreboot/+/42859/65/src/southbridge/amd/pi/hud... File src/southbridge/amd/pi/hudson/Kconfig:
https://review.coreboot.org/c/coreboot/+/42859/65/src/southbridge/amd/pi/hud... PS65, Line 68: string "AMD PSP Firmware config file"
Remove this line and move the text to a help block? You don't need two 'string' entries and this do […]
Done
https://review.coreboot.org/c/coreboot/+/42859/65/src/southbridge/amd/pi/hud... PS65, Line 69: if CPU_AMD_PI_00730F01
change this to a "depends on" statement so the option doesn't appear for other processors?
Only avalon is active now.
https://review.coreboot.org/c/coreboot/+/42859/65/src/southbridge/amd/pi/hud... File src/southbridge/amd/pi/hudson/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42859/65/src/southbridge/amd/pi/hud... PS65, Line 100: #SMUFWM_FILE=$(top)/$(FIRMWARE_LOCATE)/SmuFirmware$(FIRMWARE_TYPE).sbin : #SMUFWM_FN_FILE=$(top)/$(FIRMWARE_LOCATE)/SmuFirmware$(FIRMWARE_TYPE)_FN.sbin : #SMUSCS_FILE=$(top)/$(FIRMWARE_LOCATE)/SmuScs$(FIRMWARE_TYPE).bin
Why not remove these instead of commenting them out?
Done
https://review.coreboot.org/c/coreboot/+/42859/65/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42859/65/util/amdfwtool/amdfwtool.c... PS65, Line 222: /* static */
Is the comment supposed to be on a different line? This seems odd.
Done
https://review.coreboot.org/c/coreboot/+/42859/65/util/amdfwtool/amdfwtool.c... PS65, Line 1031: D
remove D from the unused list?
Done
https://review.coreboot.org/c/coreboot/+/42859/65/util/amdfwtool/data_parse.... File util/amdfwtool/data_parse.c:
https://review.coreboot.org/c/coreboot/+/42859/65/util/amdfwtool/data_parse.... PS65, Line 401: fprintf(stderr, "Modules name "%s" is not valid\n", oneline);
Nicely done. […]
Done