Martin Roth 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 65:
(7 comments)
Some minor changes requested. It looks good other than these that are mostly nits.
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?
# Add all the files listed in the config file
This applies to the other makefiles as well.
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 doesn't need to be user-editable at this level. If a mainboard wants to use a different config, it can override it in the mainboard kconfig.
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?
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?
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.
https://review.coreboot.org/c/coreboot/+/42859/65/util/amdfwtool/amdfwtool.c... PS65, Line 599: /* For debugging */ nit: consider making this a separate commit?
https://review.coreboot.org/c/coreboot/+/42859/65/util/amdfwtool/amdfwtool.c... PS65, Line 1031: D remove D from the unused list?