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 14:
(13 comments)
https://review.coreboot.org/c/coreboot/+/48340/14/src/mainboard/google/brya/... File src/mainboard/google/brya/variants/baseboard/devicetree.cb:
PS14: I think it would be good to push a separate change for brya since it is adding new code that wasn't present before.
https://review.coreboot.org/c/coreboot/+/48340/14/src/mainboard/google/brya/... PS14, Line 6: nit: one space should be sufficient. same for clk_req below.
https://review.coreboot.org/c/coreboot/+/48340/14/src/soc/intel/alderlake/ch... File src/soc/intel/alderlake/chip.h:
https://review.coreboot.org/c/coreboot/+/48340/14/src/soc/intel/alderlake/ch... PS14, Line 139: uint8_t clk_src; A one-line comment would be helpful here. CLKOUT_PCIE_P/N# used by this root port as per schematics.
https://review.coreboot.org/c/coreboot/+/48340/14/src/soc/intel/alderlake/ch... PS14, Line 140: clk_req same here: SRCCLKREQ# used by this root port as per schematics.
https://review.coreboot.org/c/coreboot/+/48340/14/src/soc/intel/alderlake/ch... PS14, Line 148: /* This can be used to set 0x70 for clksrcusage */ : PCIE_RP_LAN_PORT = (1 << 5), I just looked through the UPD description again. This is actually not correct. A PCIE clock source can be assigned to one of the following: 1. PCIe RP 2. LAN port 3. Free running (not used by PCIe or LAN)
For #2, currently, I don't see any ADL board using it. So, we can drop this flag. When we have to support it, we can add a new config: `uint8_t lan_clk;` and that can be used to set 0x70.
About #3, I know Subrata recently added 0x80 for RP8 on adlrvp. I am still not completely convinced it is correct since UPD description says that 0x80 is used for something other than the options stated i.e. other than PCIe RP, LAN, etc. So, I don't understand why it is associated with a RP. Anyways, we can support that case with the PCIE_RP_ALWAYS_ON_CLK for now. But, it would be good if Subrata can confirm if this is really the right configuration. +Subrata.
https://review.coreboot.org/c/coreboot/+/48340/14/src/soc/intel/alderlake/ch... PS14, Line 152: flags Data type for this should not be enum pcie_rp_flags. flags allows multiple of the enums to be set. I think this should be uint32_t flags;
https://review.coreboot.org/c/coreboot/+/48340/14/src/soc/intel/alderlake/ch... PS14, Line 158: uint8_t PcieClkSrcUsage[CONFIG_MAX_PCIE_CLOCKS]; This is not required anymore.
https://review.coreboot.org/c/coreboot/+/48340/14/src/soc/intel/alderlake/ch... PS14, Line 160: /* PCIe RP L1 substate */ : enum L1_substates_control { : L1_SS_FSP_DEFAULT, : L1_SS_DISABLED, : L1_SS_L1_1, : L1_SS_L1_2, : } PcieRpL1Substates[CONFIG_MAX_ROOT_PORTS]; This should be moved into the above structure.
https://review.coreboot.org/c/coreboot/+/48340/14/src/soc/intel/alderlake/fs... File src/soc/intel/alderlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/48340/14/src/soc/intel/alderlake/fs... PS14, Line 266: config->pcie_rp[i].flags & PCIE_RP_LTR_ENABLED ? 1 : 0; I think it would be good to have a helper for the flags:
static bool pcie_is_flag_enabled(const struct pcie_rp_config *pcie_rp, enum pcie_rp_flags flag_mask) { return pcie_rp->flags & flag_mask; }
params->PcieRpLtrEnable[i] = pcie_is_flag_enabled(config->pcie_rp[i], PCIE_RP_LTR_ENABLED); params->PcieRpAdvancedErrorReporting[i] = pcie_is_flag_enabled(config->pcie_rp[i], PCIE_RP_AER_ENABLED); params->PcieRpHotPlug[i] = pcie_is_flag_enabled(config->pcie_rp[i], PCIE_RP_HOTPLUG_ENABLED); ...
struct pcie_rp_config will have to be a named structure in chip.h.
https://review.coreboot.org/c/coreboot/+/48340/14/src/soc/intel/alderlake/ro... File src/soc/intel/alderlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/48340/14/src/soc/intel/alderlake/ro... PS14, Line 24: memcpy(m_cfg->PcieClkSrcUsage, config->PcieClkSrcUsage, : sizeof(config->PcieClkSrcUsage)); This is not required.
https://review.coreboot.org/c/coreboot/+/48340/14/src/soc/intel/alderlake/ro... PS14, Line 28: if ((config->pcie_rp[i].flags & PCIE_RP_ENABLED) == 0) Same helper as mentioned in the other file can be used here:
if (!pcie_is_flag_enabled(config->pcie_rp[i], PCIE_RP_ENABLED)) continue;
and so on.
https://review.coreboot.org/c/coreboot/+/48340/14/src/soc/intel/alderlake/ro... PS14, Line 33: m_cfg->PcieClkSrcClkReq[config->pcie_rp[i].clk_req] = : config->pcie_rp[i].clk_src; I think the logic is inverted here. UPD description says "Number of ClkReq signal assigned to ClkSrc" which means that the entry provides the ClkReq# and the index is ClkSrc#.
m_cfg->PcieClkSrcClkReq[config->pcie_rp[i].clk_src] = config->pcie_rp[i].clk_req;
https://review.coreboot.org/c/coreboot/+/48340/14/src/soc/intel/alderlake/ro... PS14, Line 35: if (config->pcie_rp[i].flags & PCIE_RP_ALWAYS_ON_CLK) : m_cfg->PcieClkSrcUsage[config->pcie_rp[i].clk_src] = 0x80; : else if (config->pcie_rp[i].flags & PCIE_RP_LAN_PORT) : m_cfg->PcieClkSrcUsage[config->pcie_rp[i].clk_src] = 0x70; : else : m_cfg->PcieClkSrcUsage[config->pcie_rp[i].clk_src] = i; Convert to a helper function:
m_cfg->PcieClkSrcUsage[config->pcie_rp[i].clk_src]] = pcie_get_clksrc_usage(config->pcie_rp[i]);
It would be easier to look through the logic behind the selection for clk src usage in a separate helper function of its own.