Attention is currently required from: Nico Huber, Tim Wawrzynczak, Subrata Banik, Angel Pons, Michael Niewöhner, EricR Lai. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48340 )
Change subject: soc/intel/alderlake: Refactor PCIE port config ......................................................................
Patch Set 60: Code-Review+1
(3 comments)
Patchset:
PS60: Thanks for your patience Eric. Just one comment about the usage of PCIE_CLK_FREE. I think it is defined as enum and macro. But there are really two different values in the two cases. I think we can just drop pch.h completely and move the definitions to romstage/fsp_params.c since they are now used only in that file. Also, better to add the prefix FSP_ to indicate that they are the values expected by FSP.
File src/soc/intel/alderlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/48340/comment/a492e319_2cd855be PS60, Line 142: PCIE_CLK_FREE Looks like PCIE_CLK_FREE got reused for two different purposes.
1. Mainboard setting in pcie_clk_config_flag --> I think this should be named PCIE_CLK_FREE_RUNNING
2. FSP value for free running clock --> I think it should be named FSP_CLK_FREE_RUNNING just to make it clear that it is something that FSP expects. It will be the one that is assigned to PcieClkSrcUsage.
File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/48340/comment/3943f43e_f7de421b PS60, Line 33: PCIE_CLK_FREE Woops. This name clashes with the definition her required by FSP: https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/alderlake/i...
Probably name this PCIE_CLK_FREE_RUNNING?