[M] Change in coreboot[main]: intel/alderlake: Add helper functions for Power Management

[Nico Huber (Code Review)]

Attention is currently required from: Dinesh Gehlot, 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 4:

(2 comments)

File src/soc/intel/alderlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/81638/comment/b4d4fc6d_f30da9c7 :
PS4, Line 504: default "auto"
> Right, so if someone (google) uses a modified version of FSP, shouldn't they add a quirk?

Sorry, my bad, should have explained better. To coreboot, default (e.g.
ASPM_DEFAULT) means the value we get when we don't override. That the FSP
UPD has a default value itself and that the integrator could have changed
this in the binary already is another layer.

When I read this comment, I understood "use auto unless coreboot overwrites
it". But now I see it could also mean "use the default, which is auto unless
already overwritten in the binary". So it seems ambiguous.

I wouldn't mention the default value. Maybe just say, use pre-set FSP value
if nothing is set in the dt. If somebody doesn't want to set the value in
the devicetree, they should know what they are doing, i.e. know what value
their binary has.

https://review.coreboot.org/c/coreboot/+/81638/comment/2d8d75c6_a9247b68 :
PS4, Line 506: 		s_cfg->PcieRpAspm[index] = rp_cfg->pcie_rp_aspm;
> `> 0`?

I don't follow. Because of the if, the value we write would always be `> 0`,
is that what you mean?

`0` is a valid value to write, though, should mean disable (AIUI, the header
comments seem incomplete). In coreboot, ASPM_DISABLE is `1`, though. So we'd
have to `- 1` (assuming all the values of `enum ASPM_control` are exactly off
by 1, compared to FSP).