Felix Held 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 2:
(9 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 4: 2015
Is that correct? Maybe use SPDX headers right away.
Done
https://review.coreboot.org/c/coreboot/+/38698/1/src/vendorcode/amd/fsp/pica... PS1, Line 37: GEN_MAX, // Maximum supported
It sometimes seems to depend on who's reviewing. […]
Done
https://review.coreboot.org/c/coreboot/+/38698/1/src/vendorcode/amd/fsp/pica... PS1, Line 46: SATA_CHANNEL_OTHER, // Default Channel Type
same here
Done
https://review.coreboot.org/c/coreboot/+/38698/1/src/vendorcode/amd/fsp/pica... PS1, Line 68: ASPM_DISABLED, // Disabled
same here
Done
https://review.coreboot.org/c/coreboot/+/38698/1/src/vendorcode/amd/fsp/pica... PS1, Line 77: AUX1, // Aux1
Are these comments needed?
Done
https://review.coreboot.org/c/coreboot/+/38698/1/src/vendorcode/amd/fsp/pica... PS1, Line 77: AUX1, // Aux1
same here
Done
https://review.coreboot.org/c/coreboot/+/38698/1/src/vendorcode/amd/fsp/pica... PS1, Line 88: HDP1, // Hdp1
same here
Done
https://review.coreboot.org/c/coreboot/+/38698/1/src/vendorcode/amd/fsp/pica... PS1, Line 99: DP, // DP
same here
Done
https://review.coreboot.org/c/coreboot/+/38698/1/src/vendorcode/amd/fsp/pica... PS1, Line 131: unsigned int
i think unsigend int was used here, since this are bitfields and not full uint32_t. […]
added a comment about this. there is probably no better option to do this in c that is similarly convenient to use and this code will never be built for big endian machines