Attention is currently required from: Arthur Heymans, Kapil Porwal, Nick Vaccaro, Subrata Banik, sridhar siricilla.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80362?usp=email )
Change subject: soc/intel/cmn/sa: Add APIs into System Agent (SA) common code ......................................................................
Patch Set 6: Code-Review+1
(6 comments)
Patchset:
PS6: Looks good. I have a few suggestions.
File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/80362/comment/47cfb8d4_e251c4cc : PS6, Line 327: const struct device *dev You know which device the sa. Why not use that directly use _pci_0_00_0? Then you have compiletime checks vs runtime null check + your sure to use the right device.
Some for all the ones below.
https://review.coreboot.org/c/coreboot/+/80362/comment/4b546c18_8db06c03 : PS6, Line 340: BIOS_DEBUG Probably should be BIOS_ERR if CONFIG_ECAM_MMCONF_SUPPORT (that's always the case on modern HW)?
https://review.coreboot.org/c/coreboot/+/80362/comment/41ecc884_5b541956 : PS6, Line 346: 4 * ((uint64_t)GiB); '4ull * GiB' looks better IMO.
https://review.coreboot.org/c/coreboot/+/80362/comment/3ddd741e_36ef56a0 : PS6, Line 367: BIOS_DEBUG BIOS_ERR
https://review.coreboot.org/c/coreboot/+/80362/comment/26513c81_0503c092 : PS6, Line 432: sa_get_dpr_size txt_get_chipset_dpr() in systemagent_early.c does the same.