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 23:
(5 comments)
https://review.coreboot.org/c/coreboot/+/48340/23/src/soc/intel/alderlake/ch... File src/soc/intel/alderlake/chip.h:
https://review.coreboot.org/c/coreboot/+/48340/23/src/soc/intel/alderlake/ch... PS23, Line 26: pcie_rp_flags A one-line comment for each of these flags would be very helpful.
https://review.coreboot.org/c/coreboot/+/48340/23/src/soc/intel/alderlake/ch... PS23, Line 32: PCIE_RP_ALWAYS_ON_CLK nit: For consistency I think it would be better to name these -
/* Clock source is free running i.e. always on. Sets 0x80 for ClkSrcUsage. */ PCIE_RP_CLK_SRC_ALWAYS_ON = /* Clock source is not used by the root port. */ PCIE_RP_CLK_SRC_UNUSED = /* Clock request signal requires probing before enabling CLKREQ# based power management. */ PCIE_RP_CLK_REQ_DETECT = /* Clock request signal is not used by the root port. */ PCIE_RP_CLK_REQ_UNUSED =
https://review.coreboot.org/c/coreboot/+/48340/23/src/soc/intel/alderlake/fs... File src/soc/intel/alderlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/48340/23/src/soc/intel/alderlake/fs... PS23, Line 266: pcie_is_flag_enabled nit: If you add one additional tab before pcie_is_flag_enabled(), it would make it easier to figure that there is an assignment being performed here and in the lines below.
https://review.coreboot.org/c/coreboot/+/48340/23/src/soc/intel/alderlake/ro... File src/soc/intel/alderlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/48340/23/src/soc/intel/alderlake/ro... PS23, Line 150: nit: extra blank line not required
https://review.coreboot.org/c/coreboot/+/48340/23/src/soc/intel/alderlake/ro... PS23, Line 154: is_dev_enabled(dev) Actually, we can use is_dev_enabled() for all root ports -- PCH and CPU. This will allow us to drop the PCIE_RP_ENABLED flag completely. What do you think?