Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38698 )
Change subject: vc/amd/fsp/picasso: Add PCIe and DDI helpers ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38698/1/src/vendorcode/amd/fsp/pica... File src/vendorcode/amd/fsp/picasso/platform_descriptors.h:
https://review.coreboot.org/c/coreboot/+/38698/1/src/vendorcode/amd/fsp/pica... PS1, Line 37: GEN_MAX, // Maximum supported
I'd add a = 0 here to make t clearer that the absolute values matter here and not only the symbolic […]
It sometimes seems to depend on who's reviewing. I believe I've seen that requested to be omitted. OTOH, maybe there's a better case for clarity here since the info is shared between coreboot and FSP.
Hmm, the values mixed hex and decimal numbers above. I don't recall now if this was originally copy/pasted out of AGESA.
https://review.coreboot.org/c/coreboot/+/38698/1/src/vendorcode/amd/fsp/pica... PS1, Line 131: unsigned int
i'd strongly prefer uint32_t here, since it makes the sizes more obvious. […]
Ew, yeah I agree.