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 9:
(5 comments)
https://review.coreboot.org/c/coreboot/+/48340/9/src/soc/intel/alderlake/chi... File src/soc/intel/alderlake/chip.h:
https://review.coreboot.org/c/coreboot/+/48340/9/src/soc/intel/alderlake/chi... PS9, Line 122: struct { In my opinion, all the PCIe related configs below should be combined into a single structure. Probably something like:
enum pcie_rp_flags { PCIE_RP_ENABLED = (1 << 0), PCIE_RP_HOTPLUG_ENABLED = (1 << 1), PCIE_RP_LTR_ENABLED = (1 << 2), PCIE_RP_AER_ENABLED = (1 << 3), PCIE_RP_ALWAYS_ON_CLK = (1 << 4), /* This can be used to set 0x80 for clksrcusage. */ PCIE_RP_LAN_PORT = (1 << 5), /* This can be used to set 0x70 for clksrcusage */ PCIE_RP_CLK_REQ_DETECT = (1 << 6), };
struct pcie_rp_config { uint8_t clk_src; uint8_t clk_req; uint32_t flags; } pcie_rp[CONFIG_MAX_ROOT_PORTS];
and then mainboard can set something like this in devicetree:
pcie_rp_config[0] = { .clk_src = 3, .clk_req = 4, .flags = PCIE_RP_ENABLED | PCIE_RP_LTR_ENABLED | PCIE_RP_AER_ENABLED, };
https://review.coreboot.org/c/coreboot/+/48340/9/src/soc/intel/alderlake/rom... File src/soc/intel/alderlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/48340/9/src/soc/intel/alderlake/rom... PS9, Line 31: /* Back up special usage */ This is not required.
https://review.coreboot.org/c/coreboot/+/48340/9/src/soc/intel/alderlake/rom... PS9, Line 40: PcieClkSrcClkReq You don't need to set this in a local array. It can directly be set in m_cfg->PcieClkSrcClkReq. Same for ClkSrcUsage below.
https://review.coreboot.org/c/coreboot/+/48340/9/src/soc/intel/alderlake/rom... PS9, Line 40: config->PcieRp[i].clkreq I think this will have to be config->pcie_rp[i].clk_req - 1. Same for clk_src below.
https://review.coreboot.org/c/coreboot/+/48340/9/src/soc/intel/alderlake/rom... PS9, Line 43: : i Let's not do this. Instead let's ensure that mainboard sets it correctly.