EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48340 )
Change subject: soc/intel/alderlake: Revise PCIE port config ......................................................................
Patch Set 15:
(9 comments)
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.
Done
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.
Done
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. […]
Done
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.
I kept this for special assigning :
# Enable CPU PCIE RP 1 using PEG CLK 0 register "PcieClkSrcUsage[0]" = "0x40" # Enable PCU PCIE PEG Slot 1 and 2 register "PcieClkSrcUsage[3]" = "0x41" register "PcieClkSrcUsage[4]" = "0x42" I don't know why they need this, any idea to add this into config?
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];
okay.
Done
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: […]
Done
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.
Need to copy the special usage.
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. […]
Take Brya for example. SD PCIE_RP8 use REQ_3 for CLKSRC_4, so we should fill PcieClkSrcClkReq[3]=4; And PcieClkSrcUsage[4]=8 right?
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: […]
Done