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 47:
(11 comments)
I think the patch train probably needs to be rebased on
https://review.coreboot.org/c/coreboot/+/42859/47//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42859/47//COMMIT_MSG@9 PS47, Line 9: Internal Cleanup: shouldn't this be just "b:"?
https://review.coreboot.org/c/coreboot/+/42859/47//COMMIT_MSG@10 PS47, Line 10: mandolin Nit: Maybe change to "Build & boot on mandolin"
https://review.coreboot.org/c/coreboot/+/42859/47/src/soc/amd/picasso/Makefi... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42859/47/src/soc/amd/picasso/Makefi... PS47, Line 125: "3rdparty/amd_blobs/picasso/PSP" I mention this in the other locations as well. Could this be moved back into a Kconfig symbol so it can be updated by a platform if desired?
https://review.coreboot.org/c/coreboot/+/42859/47/src/soc/amd/picasso/fw.cfg File src/soc/amd/picasso/fw.cfg:
https://review.coreboot.org/c/coreboot/+/42859/47/src/soc/amd/picasso/fw.cfg... PS47, Line 3: # Should be first line Why does the location need to be the first line? Doesn't the parser look through the entire file for this?
https://review.coreboot.org/c/coreboot/+/42859/47/src/soc/amd/stoneyridge/Ma... File src/soc/amd/stoneyridge/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42859/47/src/soc/amd/stoneyridge/Ma... PS47, Line 105: ifeq ($(CONFIG_AMD_APU_STONEYRIDGE),y) Can we put the directory values into a Kconfig option instead of hardcoding them into the makefile?
https://review.coreboot.org/c/coreboot/+/42859/47/src/soc/amd/stoneyridge/Ma... PS47, Line 106: FIRMWARE_LOCATE Change to FIRMWARE_LOCATION?
https://review.coreboot.org/c/coreboot/+/42859/47/src/southbridge/amd/pi/hud... File src/southbridge/amd/pi/hudson/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42859/47/src/southbridge/amd/pi/hud... PS47, Line 81: "3rdparty/blobs/southbridge/amd/avalon/PSP" Again, use a Kconfig option please.
https://review.coreboot.org/c/coreboot/+/42859/47/util/amdfwtool/amdfwtool.h File util/amdfwtool/amdfwtool.h:
https://review.coreboot.org/c/coreboot/+/42859/47/util/amdfwtool/amdfwtool.h... PS47, Line 117: endif #endif /* _AMD_FW_TOOL_H_ */
https://review.coreboot.org/c/coreboot/+/42859/39/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42859/39/util/amdfwtool/amdfwtool.c... PS39, Line 156: printf("-x | --xhci <FILE> Add XHCI blob\n"); : printf("-i | --imc <FILE> Add IMC blob\n"); : printf("-g | --gec <FILE> Add GEC blob\n");
Many old platforms only has these 3 binary to integrate. […]
Ack
https://review.coreboot.org/c/coreboot/+/42859/39/util/amdfwtool/amdfwtool.c... PS39, Line 1456: exit(1);
I dont quite understand. […]
That's what I was thinking, yes. Generate a sample config file with just the possible values. Maybe it's not worth the trouble. I'll let you decide. If you decide to do it, that's for a future commit, so marking this as resolved.
https://review.coreboot.org/c/coreboot/+/42859/47/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42859/47/util/amdfwtool/amdfwtool.c... PS47, Line 169: printf("-p | --load-mp2-fw Set if load MP2 firmare\n");
'firmare' may be misspelled - perhaps 'firmware'?
Good job, build bot.