Change in coreboot[master]: soc/intel/{skl,cnl,icl,jsl,elh,tgl}: make XTAL in s0ix conditional
Michael Niewöhner 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:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45723/2//COMMIT_MSG Commit Message:
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
Disqualificaton of shutting something down is not exactly the same […]
Huh? It's not (dis)qualifying of shutting down but (dis)qualifying for s0ix. AIUI When XTAL is qualified for s0ix, it is allowed to be on. When it's disqualified for s0ix, it's not allowed.
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 */
The comment is neither accurate nor necessary. If you want to avoid to […]
IMO it is. This is *not* qualification for shutting down but qualification for s0ix. See below
https://review.coreboot.org/c/coreboot/+/45723/2/src/soc/intel/cannonlake/ch... PS2, Line 61: uint8_t s0ix_allow_xtal_on; Nico: Furquan is right here. That bit does not control shutting down but if XTAL is qualified (=is allowed) to be kept running. XTAL gets disabled via asl code / the OS, not the PMC.
8254 clock gating is required for XTAL shutdown XTAL shutdown is necessary for S0ix unless disqualification bit is set.
Exactly. That's what I understood, too.
-
Michael Niewöhner (Code Review)