Attention is currently required from: Angel Pons, Dinesh Gehlot, Eric Lai, Kapil Porwal, Matt DeVillier, Nick Vaccaro, Paul Menzel, Sean Rhodes, Subrata Banik.
Nico Huber has posted comments on this change by Sean Rhodes. ( https://review.coreboot.org/c/coreboot/+/81638?usp=email )
Change subject: intel/alderlake: Add helper functions for Power Management ......................................................................
Patch Set 22:
(9 comments)
Patchset:
PS22: Thanks for keeping up the good work! and sorry for the long silence on my end.
Commit Message:
https://review.coreboot.org/c/coreboot/+/81638/comment/0c243ac4_f0295f9f?usp... : PS22, Line 14: Skylake, this is not the : case for Alderlake, Raptorlake and Meteorlake. I think what Paul tried to tell you but less verbose: These started to be named after actual sites. Hence there is no consistency in the spelling: It's Alder Lake, Raptor Lake, Meteor Lake but the exception: Skylake.
File src/soc/intel/alderlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/81638/comment/868eb2c9_48365d3c?usp... : PS22, Line 494: adl_get_aspm_control Could put something like `to_upd` or `to_fsp` in the helper names.
https://review.coreboot.org/c/coreboot/+/81638/comment/4f9522e1_ee6493e2?usp... : PS22, Line 494: unsigned int `enum ASPM_control` to make it clear we're converting from that to something else. (Actually `enum aspm_control` if we follow coreboot conventions.)
https://review.coreboot.org/c/coreboot/+/81638/comment/4bde1391_10d1e962?usp... : PS22, Line 507: unsigned int `enum ...`
https://review.coreboot.org/c/coreboot/+/81638/comment/e3cfc261_5b369c53?usp... : PS22, Line 530: uint32_t Just (`unsigned`)`int` for an index? Unless I miss something
File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/81638/comment/451a79d6_6c737fad?usp... : PS22, Line 40: are zero-indexed To me this doesn't explain much, our enums are also zero-indexed. I'd rather comment that our enums should be (left) compatible but off-by-1.
https://review.coreboot.org/c/coreboot/+/81638/comment/7f3771ea_c3e1e059?usp... : PS22, Line 43: /* This enum is for passing into an FSP UPD, typically PcieRpL1Substates */ Don't want to push to fix things that were already wrong before, but this is not true: The enum is for the devicetree, the converted value is passed to FSP. I guess the comment was always wrong.
https://review.coreboot.org/c/coreboot/+/81638/comment/405856f4_16a97795?usp... : PS22, Line 45: FSP_DEFAULT Probably for another patch: With our own rules like `/* Use auto unless overwritten */` this doesn't match the meaning anymore. How about `L1_SS_UNSET`? Then we could also make the code more explicit, e.g. ``` if (dt_l1ss == L1_SS_UNSET) ``` Should make it self-documenting.