Attention is currently required from: 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 7:
(2 comments)
File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/80362/comment/0aa3ecba_b2db6240 : PS6, Line 327: const struct device *dev
SA is nothing but PCI_0_00_0 aka host bridge.
__pci_0_00_0' is a real reference in the static.c code. You can do pci_read_config32(__pci_0_00_0, PCIEXBAR);
are you suggesting to use rather passing the `dev` argument to the function?
struct device *dev = pcidev_path_on_root(SA_DEVFN_ROOT);
You can get a reference from the devicetree struct device directly, rather than using runtime functions (see build/mainboard/../../static.c) . Using the alias is indeed a better idea than the reference via pci numbers: _dev_system_agent_ptr would be it.
The advantage of this over the runtime, is that you get a direct reference (no searching for it in tree), and the if the device is not there (it's in chipset.cb so that won't ever happen) the linker will complain, which is a buildtime error, which are better than runtime ones.
https://review.coreboot.org/c/coreboot/+/80362/comment/6e520818_49d31f7c : PS6, Line 432: sa_get_dpr_size
txt_get_chipset_dpr() in systemagent_early.c does the same.
good candidate for the convergence at some point. At least it covers the raw read from the register (like what line#435 does below) and additionally, this function returning the size in MiB (which may not be needed for early code piece).
I will pick that later for convergence. hoping u r good with that plan ?
sounds good.