Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48340 )
Change subject: soc/intel/alderlake: Revise PCIE port config ......................................................................
Patch Set 28:
(9 comments)
https://review.coreboot.org/c/coreboot/+/48340/28/src/soc/intel/alderlake/ch... File src/soc/intel/alderlake/chip.h:
https://review.coreboot.org/c/coreboot/+/48340/28/src/soc/intel/alderlake/ch... PS28, Line 22: #define PCIE_RP(x) ((x) - 1) : #define PCH_RP(x) PCIE_RP(x) : #define CPU_RP(x) PCIE_RP(x) : not soc-specific
https://review.coreboot.org/c/coreboot/+/48340/28/src/soc/intel/alderlake/ch... PS28, Line 27: PCIE_RP_HOTPLUG_ENABLED = (1 << 0), : PCIE_RP_LTR_ENABLED = (1 << 1), : /* PCIE RP Advanced Error Report */ : PCIE_RP_AER_ENABLED = (1 << 2), nit: maybe drop _ENABLED? I'd say it's pretty clear what a flag PCIE_RP_HOTPLUG would do
https://review.coreboot.org/c/coreboot/+/48340/28/src/soc/intel/alderlake/ch... File src/soc/intel/alderlake/chip.c:
https://review.coreboot.org/c/coreboot/+/48340/28/src/soc/intel/alderlake/ch... PS28, Line 19: t struct pcie_rp_group pch_lp_rp_groups[] = { : { .slot = PCH_DEV_SLOT_PCIE, .count = 8 }, : { .slot = PCH_DEV_SLOT_PCIE_1, .count = 4 }, : { 0 } : }; moving this and the related parts could be done in a separate change to reduce patch size and make review a bit easier
https://review.coreboot.org/c/coreboot/+/48340/28/src/soc/intel/alderlake/ro... File src/soc/intel/alderlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/48340/28/src/soc/intel/alderlake/ro... PS28, Line 34: nit: newline before
https://review.coreboot.org/c/coreboot/+/48340/28/src/soc/intel/alderlake/ro... PS28, Line 41: return m_cfg->PcieRpEnableMask not sure if that is a good idea because the function only returns valid results after the fsp mask was written below; what about looking it up directly in the devicetree instead?
https://review.coreboot.org/c/coreboot/+/48340/28/src/soc/intel/alderlake/ro... PS28, Line 43: CpuPcieRpEnableMask only one rp?
https://review.coreboot.org/c/coreboot/+/48340/28/src/soc/intel/alderlake/ro... PS28, Line 44: die nit: newline before
https://review.coreboot.org/c/coreboot/+/48340/28/src/soc/intel/alderlake/ro... PS28, Line 57: pcie_is_flag_enabled(cfg[i], PCIE_RP_CLK_REQ_UNUSED)) : m_cfg->PcieClkSrcClkReq[cfg[i].clk_src] = cfg[i].clk_req; : m_cfg->PcieClkSrcUsage[cfg[i].clk_src] = : convert_clk_src_to_fsp(type, cfg[i], i); this won't be executed when the port is enabled (line 53)
https://review.coreboot.org/c/coreboot/+/48340/28/src/soc/intel/alderlake/ro... PS28, Line 157: CONFIG_MAX_ROOT_PORTS why pass that als parameter instead of using it in the function?