Change in coreboot[master]: soc/intel/{skl,cnl,icl,jsl,elh,tgl}: make XTAL S0ix qualification opt...

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45723 )

Change subject: soc/intel/{skl,cnl,icl,jsl,elh,tgl}: make XTAL S0ix qualification optional
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45723/2/src/soc/intel/cannonlake/ch... 
File src/soc/intel/cannonlake/chip.h:

https://review.coreboot.org/c/coreboot/+/45723/2/src/soc/intel/cannonlake/ch... 
PS2, Line 61: 	uint8_t s0ix_allow_xtal_on;
> > I'm afraid I don't follow. Intel documents both the XTAL shutdown and the SLP_S0# assertion (what is what we want to achieve with S0ix, AIUI) as effects of low-power state 3 (LP3). Not as requirements of one another.
>
> This is my understanding: On C10 entry, PMC checks clock requests for XTAL to determine whether it is okay to enter S0i3 (LP3) i.e. XTAL is one of the qualification factors for S0i3 entry. However, it can be ignored by setting the XTAL disqualification bit. Thus, even if XTAL remains enabled on C10 entry, PMC ignores its state when making the determination for S0i3 entry.

Well, I have to repeat it: There is no XTAL disqualification bit. The
exact name is: 24MHz Crystal Shutdown Qualification Disable (XTALSDQDIS).
It's not about the XTAL, but about shutting the XTAL down. This confusion
makes all prior discussions hard to assess. But it's ambiguous anyway:
Either LP3 is qualified by the shutdown or the shutdown is qualified by
LP3 *shrug*

So far I'm still convinced of my original interpretation:

* XTALSDQDIS == 0, XTAL will be shut down in LP3.
* XTALSDQDIS == 1, XTAL won't be shut down.

That would mean it doesn't block other parts from powering down.

Maybe we should leave all this "qualification" reasoning aside for a
moment and first discuss which setting improves power savings?
> > Where is their relation documented? IIRC, we can't have S0ix with that timer anyway.
>
> I never found actual documentation on this. Mostly during discussions with Intel. There was a patch series at one point that Intel had: https://review.coreboot.org/c/coreboot/+/22299 which basically indicated that:

Thanks I read through the comments of these patches. Most interesting
statement: "XTAL is not required for WoV since FRO/Audio PLL is a clock
source to DSP." (CB:22237). This says the exact opposite of the patch
that introduced this (CB:19442). If it was an accident, I guess we won't
need an option. Neither of the commit messages states clearly if the
new setting is supposed to save power or not, btw.
> 8254 clock gating is required for XTAL shutdown
> XTAL shutdown is necessary for S0ix unless disqualification bit is set.

I will only start trusting the latter statement when somebody measured
SLP_S0# assertion. It just doesn't make much sense. If it is optional
hardware-wise, when would one want to block low-power states because
the XTAL is still running? Also, CB:22237 wanted to *clear* the bit, and it
seems from the comments that they wanted to save power. Which would
imply the opposite of this statement.

And all the power-saving questions aside: Did CB:19442 fix anything that
Chromebooks need?