Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37490 )
Change subject: soc/amd/picasso: Add bootblock support ......................................................................
Patch Set 9:
(4 comments)
https://review.coreboot.org/c/coreboot/+/37490/9/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/37490/9/src/soc/amd/picasso/Kconfig... PS9, Line 221: menu "PSP Configuration Options" Bootblock parameters are not PSP configuraion options.
https://review.coreboot.org/c/coreboot/+/37490/9/src/soc/amd/picasso/Kconfig... PS9, Line 235: config AMDFW_OUTSIDE_CBFS off-topic
https://ticket.coreboot.org/issues/224
https://review.coreboot.org/c/coreboot/+/37490/4/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/37490/4/src/soc/amd/picasso/Makefil... PS4, Line 213: PSP_BIOSBIN_DEST=$(shell printf "%x" $(call int-subtract, $(call int-add, $(CONFIG_X86_RESET_VECTOR) 0x10) $(PSP_BIOSBIN_SIZE)))
not sure what to do about this right now. […]
"The benefit to using C_ENV_BOOTBLOCK_SIZE and X86_RESET_VECTOR+0x10-n is to have those base/size values later when I need to reserve that RAM."
Can I review that part somewhere? They were not used in CB:38702 that deals with these RAM reservations. I do not understand how parsing PSP table would come to play.
I can see you abandoned CB:33759, but my question there (and below), if it will be allowed for you to have PSP table point outside amdfw.rom is still open and relevant. I am not saying it is forbidden but someone else might since it is sort of crossing two filesystems (CBFS and PSP layout) with some absolute (or relative) addressing.
https://review.coreboot.org/c/coreboot/+/37490/9/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/37490/9/src/soc/amd/picasso/Makefil... PS9, Line 207: 0x10
Why the magic number?
I've raised questions some questions in patchset #4 and the solution seems to be to mention possible future work on this area in the commit message. It is 0x10 because vector ends with 0xfff0, int-align might be more correct.