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 S0ix qualification optional
......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45723/2/src/soc/intel/cannonlake/c…
File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/45723/2/src/soc/intel/cannonlake/c…
PS2, Line 60: /* Enable XTAL oscillator qualification for S0ix */
> 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.
Sorry, I was mixing things up. You are right, that this is about Slp_S0#, and `allow_slp_s0_even_with_xtal_on` would indeed be way more accurate.
I'm a bit unhappy about such long names, however, as Angel pointed out, that is what cb coding guidelines suggest (descriptive naming).
Regarding that comment; if we remove this comment, the comment for cppmvric2_adsposcdis should be removed, too.
https://review.coreboot.org/c/coreboot/+/45723/2/src/soc/intel/cannonlake/c…
PS2, Line 61: uint8_t s0ix_allow_xtal_on;
First, let's stop using the term "qualification", since it seems to cause great confusion and we're constantly talking past each other and I'm not sure anymore if I really got what it means... Let's use "allow" or whatever 😄
> Found a hint that might explain the bit, just a theory: Normally, when SLP_S0# gets asserted the VR might shut further down. I guess this can be problematic when the XTAL is still running. The bit, then should only be set if one is sure that the VR can take it (the additional load of the XTAL).
> But if this is about the VRs capabilities, I wonder a) why was that never mentioned before, and b) why was the bit set unconditionally.
Well, I have nowhere seen VR caps being mentioned.
I've read "25.4.2 PCH S0 Low Power" (doc# 615170-001) again:
"""
24 MHz Crystal Shutdown
When the CPU and system are in a power management state that can tolerate gating
the 24 MHz crystal clock, this circuit can be powered down. This occurs when the
processor enters C10 state, the PCH is in LP3 and all other consumers of the 24 MHz
XTAL de-assert their clock request.
"""
The crucial point here is "all other consumers of the 24 MHz XTAL de-assert their clock request".
Could it be, that XTAL is also disabled by the PCH itself, when there is no clock request from any user, during runtime, regardless of S0ix/C10/whatever? That would mean I was wrong about the OS controlling XTAL (I checked the ASL again, which does not directly control XTAL, but the XTALSDQDIS bit).
If that is right, then XTALSDQDIS is indeed disabling one of the precondition checks for SLP_S0# assertion, because the XTAL clock never gets de-asserted when WoV (and other stuff) is used.
--
To view, visit https://review.coreboot.org/c/coreboot/+/45723
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I17bac9b06e5291b1548704744e872b22b2435c9c
Gerrit-Change-Number: 45723
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Naresh Solanki <naresh.solanki(a)intel.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Sun, 27 Sep 2020 11:31:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com>
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment
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/c…
File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/45723/2/src/soc/intel/cannonlake/c…
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/c…
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.
--
To view, visit https://review.coreboot.org/c/coreboot/+/45723
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I17bac9b06e5291b1548704744e872b22b2435c9c
Gerrit-Change-Number: 45723
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Naresh Solanki <naresh.solanki(a)intel.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Sun, 27 Sep 2020 00:12:31 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com>
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment
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/c…
File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/45723/2/src/soc/intel/cannonlake/c…
PS2, Line 61: uint8_t s0ix_allow_xtal_on;
> > > 8254 clock gating is required for XTAL shutdown […]
Found a hint that might explain the bit, just a theory: Normally, when
SLP_S0# gets asserted the VR might shut further down. I guess this can
be problematic when the XTAL is still running. The bit, then should only
be set if one is sure that the VR can take it (the additional load of the
XTAL).
But if this is about the VRs capabilities, I wonder a) why was that never
mentioned before, and b) why was the bit set unconditionally.
--
To view, visit https://review.coreboot.org/c/coreboot/+/45723
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I17bac9b06e5291b1548704744e872b22b2435c9c
Gerrit-Change-Number: 45723
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Naresh Solanki <naresh.solanki(a)intel.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Sat, 26 Sep 2020 23:42:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com>
Gerrit-MessageType: comment
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/c…
File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/45723/2/src/soc/intel/cannonlake/c…
PS2, Line 61: uint8_t s0ix_allow_xtal_on;
> > 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.
I was there too, then saw CB:22237. Why did they want to clear the bit?
And most of all, why do you want to clear the bit then?
--
To view, visit https://review.coreboot.org/c/coreboot/+/45723
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I17bac9b06e5291b1548704744e872b22b2435c9c
Gerrit-Change-Number: 45723
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Naresh Solanki <naresh.solanki(a)intel.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Sat, 26 Sep 2020 23:33:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com>
Gerrit-MessageType: comment