Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48340 )
Change subject: soc/intel/alderlake: Revise PCIE port config ......................................................................
Patch Set 17:
(6 comments)
https://review.coreboot.org/c/coreboot/+/48340/17/src/soc/intel/alderlake/ch... File src/soc/intel/alderlake/chip.h:
https://review.coreboot.org/c/coreboot/+/48340/17/src/soc/intel/alderlake/ch... PS17, Line 23: RP_1 Probably add PCH_ as a prefix to indicate these are PCH PCIE RP numbers? And we would also need: CPU_RP_1 = 0, CPU_RP_2, ...
BTW, if this enum seems to be growing too long, you can also just use a macro: #define PCIE_RP(x) ((x) - 1) #define PCH_RP(x) PCIE_RP(x) #define CPU_RP(x) PCIE_RP(x)
https://review.coreboot.org/c/coreboot/+/48340/17/src/soc/intel/alderlake/ch... PS17, Line 39: SRC_1 = (1 << 0), This is not required. You can default initialize all clock sources as disabled and then use pch_pcie_rp and cpu_pcie_rp to initialize the used clocks. See my comment on fsp_params.c
https://review.coreboot.org/c/coreboot/+/48340/17/src/soc/intel/alderlake/ch... PS17, Line 54: enum pcie_rp_type { This can be moved to fsp_params.c since it is only used in that file.
https://review.coreboot.org/c/coreboot/+/48340/17/src/soc/intel/alderlake/ro... File src/soc/intel/alderlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/48340/17/src/soc/intel/alderlake/ro... PS17, Line 30: static int pcie_is_flag_enabled(const struct pcie_rp_config pcie_rp, : enum pcie_rp_flags flag_mask) : { : return pcie_rp.flags & flag_mask ? 1 : 0; : } This can be added as inline to chip.h so that it can be used by both romstage and ramstage files.
https://review.coreboot.org/c/coreboot/+/48340/17/src/soc/intel/alderlake/ro... PS17, Line 48: PCH_PCIE_RP Instead of checking PCH_PCIE_RP, should we add a flag, PCIE_RP_NO_CLKREQ and check that? That will help both PCH and CPU root ports.
https://review.coreboot.org/c/coreboot/+/48340/17/src/soc/intel/alderlake/ro... PS17, Line 160: disable_unused_clk(m_cfg, config); You can do this before the pcie_rp_init() calls and then you don't need the special PcieClkSrcDisabled config.
i.e.
/* Disable all PCIe clock sources by default. */ for (i = 0; i < CONFIG_MAX_PCIE_CLOCKS; i++) m_cfg->PcieClkSrcUsage[i] = PCIE_CLK_NOTUSED;
m_cfg->PcieRpEnableMask = pcie_rp_init(..., PCH_PCIE_RP, ...); m_cfg->CpuPcieRpEnableMask = pcie_rp_init(..., CPU_PCIE_RP, ...);