Change in coreboot[master]: soc/intel/{skl,cnl,icl,jsl,elh,tgl}: make XTAL in s0ix conditional
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 in s0ix conditional ......................................................................
Patch Set 2:
(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 don't think this bit is really deciding whether to shut down XTAL. It basically controls whether PMC takes its state into consideration when making a decision about entering S0ix. Probably should be: xtal_blocks_s0ix?
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. Also the patch that introduced this quirk seems to treat it like that: c261c4b (soc/intel/skylake: Set xtal bypass on low power idle).
Also, we might have to take USE_LEGACY_8254_TIMER into consideration i.e. if CONFIG_USE_LEGACY_8254_TIMER is enabled, then xtal_blocks_s0ix must be false.
Where is their relation documented? IIRC, we can't have S0ix with that timer anyway.
-
Nico Huber (Code Review)