13 comments:
File src/mainboard/google/brya/variants/baseboard/devicetree.cb:
I think it would be good to push a separate change for brya since it is adding new code that wasn't present before.
nit: one space should be sufficient. same for clk_req below.
File src/soc/intel/alderlake/chip.h:
Patch Set #14, 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.
Patch Set #14, Line 140: clk_req
same here: SRCCLKREQ# used by this root port as per schematics.
/* 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.
Patch Set #14, 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;
Patch Set #14, Line 158: uint8_t PcieClkSrcUsage[CONFIG_MAX_PCIE_CLOCKS];
This is not required anymore.
/* 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.
File src/soc/intel/alderlake/fsp_params.c:
Patch Set #14, 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.
File src/soc/intel/alderlake/romstage/fsp_params.c:
memcpy(m_cfg->PcieClkSrcUsage, config->PcieClkSrcUsage,
sizeof(config->PcieClkSrcUsage));
This is not required.
Patch Set #14, 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.
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;
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.
To view, visit change 48340. To unsubscribe, or for help writing mail filters, visit settings.