Marshall Dawson 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 5:
(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. […]
Thanks. The result of many interactive rebases, I'm sure.
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? […]
I'm not yet sold that it should be removed. I'm thinking of a scenario where AMD gives you two files (e.g. as w/Nick). Is it better to let them put the files where they want and change the default path/names, or say they must be copied to these path/names (and still go into menuconfig to enable using them?
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?
Done
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?
Removed. Let's not add it without a recovery mechanism.
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?
No, we need this one to boot. Odd because of its name, though.
https://review.coreboot.org/c/coreboot/+/33764/4/src/soc/amd/picasso/Makefil... PS4, Line 152: "0x0000000010000001" I'm good with the comment.
Do we really want this hardcoded ...
It doesn't bother me too much here. It used to be hardcoded in amdfwtool instead (and still is unless --soft-fuse overrides it). I just don't see much in the spec worth adding a Kconfig option for. And I assume you noticed the override to 30000001 for MP2 FW at line 177 of PS4.
Kyosti and I have talked about removing b0 in production systems, however all AMD images, including the machine I'm typing on now, seem to set it. And given the nature of what it does, I'm not too worried about it.
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?
Nope, this one's required too.