Nikolai Vyssotski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45278 )
Change subject: WIP: Use SPDs from PSP BIOS table type 0x69 based on GPIO straps ......................................................................
Patch Set 3:
(4 comments)
Zork and Dalboz use different GPIOs for memory configuration. It looks like we will still have to use APCB magic for integrating GPIO information into APCB. Until APCB build is open sourced we will have to do patching of the APCB_magic binary.
https://review.coreboot.org/c/coreboot/+/45278/3/src/mainboard/google/zork/s... File src/mainboard/google/zork/spd/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45278/3/src/mainboard/google/zork/s... PS3, Line 7: SPD_BLOB:=$(obj)/SPD_array.bin
Does this need to be here? It's constructed in the picasso makefile.
Missed that one. I meant to move it from top level make but on second thought it probably should stay there and get removed here.
https://review.coreboot.org/c/coreboot/+/45278/3/src/mainboard/google/zork/s... PS3, Line 10: APCB_SOURCES:=$(FIRMWARE_LOCATE)/APCB_magic.bin
Why is one even needed? Is this just to pass the validation check? If so, let's remove the check or […]
Presence of this variable indicates that this platform supports the new way of packing SPDs into type 0x69 with APCB not having any SPD info. This is how we can differentiate between cases of APCB with SPD(s) (Mandolin) and the the new SPD-agnostic APCB (SPD in type 0x69). We could add Kconfig option as an alternative.
https://review.coreboot.org/c/coreboot/+/45278/3/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45278/3/src/soc/amd/picasso/Makefil... PS3, Line 243: #ifdef SPD_SOURCES
Is this ifdef necessary? PSP_SPD_FILES will just be empty when its not defined.
Yes, unfortunately it is needed. Without it Mandolin build will be broken (no idea how build bot managed to mark it passing). I was actually experimenting with commenting them out prior to committing to confirm that they were indeed necessary. I managed to push my experimental version where they are both commented out. It should be ifdef (without #). It is hard to catch it switching between C and makefiles - I am glad you drew my attention to it. Without it SPD_BLOB gets defined which causes AMDFWTOOL to fail as it is looking for non-existing SPD_array.bin (from OPT_SPD_BLOB).
https://review.coreboot.org/c/coreboot/+/45278/3/src/soc/amd/picasso/Makefil... PS3, Line 247: SPD_BLOB:=$(obj)/SPD_array.bin
Maybe this be SPD_ARRAY_BLOB since it may contain multiple SPDs
Good idea.