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:
(1 comment)
File src/soc/intel/alderlake/chip.h:
https://review.coreboot.org/c/coreboot/+/81638/comment/e2692088_e220bf98 : PS4, Line 799: };
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).
The current code leaves devices without ASPM and friends enabled, even with the Kconfig option set. In that state, there are serious issues with devices, particularly SSDs - some won't be detected, they can drop off, and it can cause issues with payloads (yes, as expected, edk2 is the worst).
Not saying the state is good. Just the fact that the default is set in coreboot sources. That's a good design. I assume it's undocumented what `auto` means, so that would indeed not be a good default.
Maybe it is just a question of what the default value is, but IMO, it's not better -the "default" should be the most compatible - which is going to be the default in FSP.
Do you mean ASPM? Your code comments and those in FSP headers say the FSP default is `auto`, too? FWIW, the most compatible is always to turn it off.