Attention is currently required from: Subrata Banik, Michael Niewöhner, EricR Lai. Nico Huber 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:
(7 comments)
File src/mainboard/intel/adlrvp/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/48340/comment/b3f1e757_44dabe91 PS47, Line 47: }" NB. This can also be set inside the respective `device pci` node, now.
File src/soc/intel/alderlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/48340/comment/fe8c4f94_339aa69f PS47, Line 34: die("Unsupported pcie_rp_type!"); Nit, it's dead code. Maybe an assert() would be better.
https://review.coreboot.org/c/coreboot/+/48340/comment/2afc9045_43f99002 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 special meaning?
File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/48340/comment/aa223e4d_d5c442a0 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 something else, i.e. not an RP? if not, what's the exact difference to CLK_REQ_UNUSED?
https://review.coreboot.org/c/coreboot/+/48340/comment/3e517882_cae95a99 PS47, Line 25: Nit, needs a line break here to comply to our coding style. Or the alternative block style without dangling asterisks.
https://review.coreboot.org/c/coreboot/+/48340/comment/cd69782b_34b7d4b5 PS47, Line 104: uint32_t flags; Nit, `enum pcie_rp_flags` I assume? That would also make the comment obsolete.
https://review.coreboot.org/c/coreboot/+/48340/comment/316411d4_a1220352 PS47, Line 107: }; Why not keep it together with the flags declaration?