Attention is currently required from: Nico Huber, Subrata Banik, Michael Niewöhner, EricR Lai. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48340 )
Change subject: soc/intel/alderlake: Refactor PCIE port config ......................................................................
Patch Set 47: -Code-Review
(4 comments)
File src/mainboard/intel/adlrvp/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/48340/comment/12194e15_29f250df PS47, Line 47: }"
NB. This can also be set inside the respective `device pci` node, now.
+1. I really like that idea. And hoping that is how we organize things on Google reference board. Makes it easier to read and follow.
File src/soc/intel/alderlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/48340/comment/1c2f9831_f5243f25 PS47, Line 42: for (i = 0; i < cfg_count; i++) { nit: This can also be organized to only loop through RPs that are initialized:
while (en_mask) { i = __ffs(en_mask); en_mask &= ~i;
... }
Then you don't need to pass in cfg_count at all. Anyways, not a big deal if we simply loop over the root ports.
https://review.coreboot.org/c/coreboot/+/48340/comment/e2cf0933_f88e0b02 PS47, Line 48: m_cfg->PcieClkSrcClkReq[cfg[i].clk_src] = cfg[i].clk_req;
Why skip the assignment? What is the default value and does it have […]
I think it will have to be initialized to 0xFF (PCIE_CLK_NOT_USED) just like PcieClkSrcUsage is on line 142 below.
File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/48340/comment/aec6c552_34194f7d PS47, Line 22: PCIE_RP_CLK_FREE = (1 << 3),
I'm a bit confused here. Doesn't this usually mean the clock is used for […]
That is what I am confused about too. I assumed "free running clock" meant that the clock is not associated with any RP, but has some unspecified usage. Hence, it is an attribute of the clock and not of the root port. But, looking at the way it is set up for ADLRVP, it looks like a clock source which has a CLKREQ# associated with it is marked as a free running clock. It seems like a bad workaround for some different issue.