Attention is currently required from: Dinesh Gehlot, Eric Lai, Kapil Porwal, Lean Sheng Tan, Matt DeVillier, Nick Vaccaro, Sean Rhodes, Subrata Banik.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81638?usp=email )
Change subject: intel/alderlake: Add helper functions for Power Management ......................................................................
Patch Set 7:
(3 comments)
File src/soc/intel/alderlake/chip.h:
https://review.coreboot.org/c/coreboot/+/81638/comment/d174cff8_572f46d2 : PS4, Line 799: };
Why not update src/soc/intel/common/block/include/intelblocks/pcie_rp.h?
Do you mean to remove the `DEFAULT = 0` entries?
IIRC, the whole idea to have a special 0 value (different from the meaning of a 0 in the UPD) was to avoid having to re-test dozens of mainboards when a new option was introduced (and there was no way to derive from the code what value already ported boards used; because the value was in a downstream blob).
The current ADL code shows that we can do better and hardcoded a default in coreboot. IMO, that's a good idea (I would even hardcode all UPD defaults in coreboot).
I wonder now if we could indeed get rid of the special 0. We have chipset devicetrees now. Maybe we could set defaults there? If that works, we could do it for older platforms, too (after a survey if they work with the upstream, unaltered blobs?).
File src/soc/intel/alderlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/81638/comment/6a09ee8e_f4515ad3 : PS4, Line 504: default "auto" And sorry again. Mixed something up.
To coreboot, default (e.g. ASPM_DEFAULT) means the value we get when we don't override.
This used to be the case. It isn't for the ADL code, though. Maybe we should try to find a single pattern for all generations.
File src/soc/intel/alderlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/81638/comment/b73b52d6_4897dc74 : PS7, Line 504: ctl = ASPM_AUTO; So before this change unset in the devicetree meant `auto`. After this change, it uses the value from the binary. While that's what we did in past, it seems like a step backwards. We offert the "leave the default" in the past because we didn't know what values to set for all the boards already in the tree. This time we know they used `auto` if unset.