Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33764 )
Change subject: soc/amd/picasso: Update all PSP and amdfw.rom building ......................................................................
Patch Set 4:
(7 comments)
https://review.coreboot.org/c/coreboot/+/33764/4/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/33764/4/src/soc/amd/picasso/Kconfig... PS4, Line 335: config ROMSTAGE_MAX_SIZE : hex "Size of first execution stage" : default 0x10000 : help : The base address where romstage should be linked. This is the : uncompressed size of romstage. This is the second instance of this entry. Maybe delete this or the one above. If you have it here just so it will be inside the PSP menu as well as outside, at least remove the default.
https://review.coreboot.org/c/coreboot/+/33764/4/src/soc/amd/picasso/Kconfig... PS4, Line 403: config PSP_WHITELIST_BL_FILE : string "Whitelist-capable bootloader file name" : depends on HAVE_PSP_WHITELIST_FILE : default "3rdparty/blobs/soc/amd/picasso/PSP/PspBootLoader_WL_RV.sbin" Do we need this entry? Just update the standard bootloader file based on HAVE_PSP_WHITELIST_FILE?
At the very least, this and the below PSP_WHITELIST_FILE entries could be combined, assuming you'll have both in the same directory.
https://review.coreboot.org/c/coreboot/+/33764/4/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33764/4/src/soc/amd/picasso/Makefil... PS4, Line 128: # Maybe add the AMD document number for people with AMD NDA access?
https://review.coreboot.org/c/coreboot/+/33764/4/src/soc/amd/picasso/Makefil... PS4, Line 139: # type = 0x3 : PSPRCVR_FILE=$(top)/$(FIRMWARE_LOCATE)/PspRecoveryBootLoader_prod_RV.sbin Optional?
https://review.coreboot.org/c/coreboot/+/33764/4/src/soc/amd/picasso/Makefil... PS4, Line 148: # type = 0x9 : PSP_SEC_DBG_KEY_FILE=$(top)/$(FIRMWARE_LOCATE)/RavenSecureDebug_PublicKey.bin Should this be optional like psp secure os?
https://review.coreboot.org/c/coreboot/+/33764/4/src/soc/amd/picasso/Makefil... PS4, Line 152: "0x0000000010000001" Mark this as NDA description only? Otherwise I'm sure there will be questions about what each bit means.
Do we really want this hardcoded in the makefile? Maybe make it a Kconfig value?
https://review.coreboot.org/c/coreboot/+/33764/4/src/soc/amd/picasso/Makefil... PS4, Line 160: # type = 0x13 : PSP_SEC_DEBUG_FILE=$(top)/$(FIRMWARE_LOCATE)/secure_unlock_prod_RV.sbin Optional?