Change in coreboot[master]: soc/intel/{skl,cnl,icl,jsl,elh,tgl}: make XTAL S0ix qualification opt...
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:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45723/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45723/2//COMMIT_MSG@7 PS2, Line 7: soc/intel/{skl,cnl,icl,jsl,elh,tgl}: make XTAL in s0ix conditional Please mention your incentive. What's the benefit of being able to clear the bit?
https://review.coreboot.org/c/coreboot/+/45723/2//COMMIT_MSG@16 PS2, Line 16: This patch adds a new devicetree option for XTAL (dis)qualification and
Huh? It's not (dis)qualifying of shutting down but (dis)qualifying for s0ix. […]
IMHO, it's meant the other way around, XTAL shutdown qualifies SLP_S0# assertion.
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 60: /* Enable XTAL oscillator qualification for S0ix */
IMO it is. This is *not* qualification for shutting down but qualification for s0ix. […]
Doesn't make it more accurate. s/S0ix/SLP_S0# assertion/ would be a start, but still.
My current interpretation:
What is qualified is the SLP_S0# assertion. With XTALSDQDIS=0, the XTAL being shut down qualifies SLP_S0# assertion. With XTALSDQDIS=1, nothing, the XTAL state is just not taken into account for the SLP_S0#-assertion qualification. `allow_slp_s0_even_with_xtal_on` would be the name for that.
https://review.coreboot.org/c/coreboot/+/45723/2/src/soc/intel/cannonlake/ch... PS2, Line 61: uint8_t s0ix_allow_xtal_on;
... 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?
Please ignore. I've taken CB:22237 out of the equation again and things start to make sense.
-
Nico Huber (Code Review)