Change in coreboot[master]: [under test]mb/google/hatch: Program gpio clk power gating settings i...

[Kane Chen (Code Review)]

Kane Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: [under test]mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

[under test]mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

With PchPmSlpS0Vm075VSupport FSP UPD set, SoC requires gpio clk to be
power gated.
But when gpio clk is power gated, it requires longer interrupt
assertion from device.

This commit provides a way to set gpio clk power gating settings in
SPIO PS0/PS3 so that cr50 doesn't need longer interrupt assertion
and SoC can still enter runtime s0ix with PchPmSlpS0Vm075VSupport set.

BUG=:141831197

Change-Id: I33a3d5897ec40afee29759160963363c322d5ad0
Signed-off-by: Kane Chen 
---
M src/mainboard/google/hatch/mainboard.asl
1 file changed, 14 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/37865/1

diff --git a/src/mainboard/google/hatch/mainboard.asl b/src/mainboard/google/hatch/mainboard.asl
index dff1a75..db3c7ff 100644
--- a/src/mainboard/google/hatch/mainboard.asl
+++ b/src/mainboard/google/hatch/mainboard.asl
@@ -55,3 +55,17 @@
    	LOCL (0)
    }
 }
+
+Scope (_SB.PCI0.SPI0)
+{
+	Method (_PS0, 0)
+	{
+		LOCL (0)
+	}
+
+	Method(_PS3, 0)
+	{
+		LOCL (MISCCFG_ENABLE_GPIO_PM_CONFIG)
+	}
+
+}

[Kane Chen (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/37865

to look at the new patch set (#2).

Change subject: [under test]mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

[under test]mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

With PchPmSlpS0Vm075VSupport FSP UPD set, SoC requires gpio clk to be
power gated.
But when gpio clk is power gated, it requires longer interrupt
assertion from device.

This commit provides a way to set gpio clk power gating settings in
SPIO PS0/PS3 so that cr50 doesn't need longer interrupt assertion
and SoC can still enter runtime s0ix with PchPmSlpS0Vm075VSupport set.

BUG=b:141831197

Change-Id: I33a3d5897ec40afee29759160963363c322d5ad0
Signed-off-by: Kane Chen 
---
M src/mainboard/google/hatch/mainboard.asl
1 file changed, 14 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/37865/2

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Kane Chen (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/37865

to look at the new patch set (#3).

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

With PchPmSlpS0Vm075VSupport FSP UPD set, SoC requires gpio clk to be
power gated.
But when gpio clk is power gated, it requires longer interrupt
assertion from device.

This commit provides a way to set gpio clk power gating settings in
SPIO PS0/PS3 so that cr50 doesn't need longer interrupt assertion
and SoC can still enter runtime s0ix with PchPmSlpS0Vm075VSupport set.

BUG=:141831197
TEST=run suspend/stress, cold reset tests 50 cycles and
     check no irq missed in kernel msg

Change-Id: I33a3d5897ec40afee29759160963363c322d5ad0
Signed-off-by: Kane Chen 
---
M src/mainboard/google/hatch/mainboard.asl
1 file changed, 14 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/37865/3

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/37865/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/37865/3//COMMIT_MSG@21 
PS3, Line 21: 
I guess you can highlight the original problem statement 
1. Runtime S0ix was not working 
2. Audio bypass bit was not set

and this CL able to fix both issues?

https://review.coreboot.org/c/coreboot/+/37865/3/src/mainboard/google/hatch/... 
File src/mainboard/google/hatch/mainboard.asl:

https://review.coreboot.org/c/coreboot/+/37865/3/src/mainboard/google/hatch/... 
PS3, Line 70: 
@Kane, can we move this into serialio.asl directly ?

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Kane Chen (Code Review)]

Hello Subrata Banik, Aamir Bohra, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/37865

to look at the new patch set (#4).

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

With PchPmSlpS0Vm075VSupport FSP UPD set, SoC requires gpio clk to be
power gated, but when gpio clk is power gated, it requires longer interrupt
assertion from device.

This change fixes runtime s0ix with PchPmSlpS0Vm075VSupport set.

And it sets gpio clk power gating settings in SPIO PS0/PS3 so that
cr50 doesn't need longer interrupt assertion.

BUG=:141831197
TEST=run suspend/stress, cold reset tests 2500 cycles and
     check no irq missed in kernel msg

Change-Id: I33a3d5897ec40afee29759160963363c322d5ad0
Signed-off-by: Kane Chen 
---
M src/mainboard/google/hatch/mainboard.asl
1 file changed, 14 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/37865/4

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Kane Chen (Code Review)]

Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/37865/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/37865/3//COMMIT_MSG@21 
PS3, Line 21:
> I guess you can highlight the original problem statement  […]

This change only fix runtime s0ix.
The Audio bypass bit is clear and covered by 
 PchPmSlpS0Vm075VSupport UPD in another change

https://review.coreboot.org/c/coreboot/+/37865/3/src/mainboard/google/hatch/... 
File src/mainboard/google/hatch/mainboard.asl:

https://review.coreboot.org/c/coreboot/+/37865/3/src/mainboard/google/hatch/... 
PS3, Line 70:
> @Kane, can we move this into serialio. […]

Hi Subrata, 
I think it's hatch specific WA, that's why i put here.

Other projects like Sarien are setting gpio pm in dev tree directly.

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Aamir Bohra (Code Review)]

Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 4: Code-Review+1

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/37865/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/37865/4//COMMIT_MSG@9 
PS4, Line 9: With PchPmSlpS0Vm075VSupport FSP UPD set, SoC requires gpio clk to be
           : power gated
Where is this requirement captured?

https://review.coreboot.org/c/coreboot/+/37865/4//COMMIT_MSG@14 
PS4, Line 14: 
            : And it sets gpio clk power gating settings in SPIO PS0/PS3 so that
            : cr50 doesn't need longer interrupt assertion.
            : 
This is not correct. Setting GPIO PM config bits for all communities when GSPI for TPM is idle does not really guarantee that the other subsystems going to other peripherals are also idle. Thus, this could have a side-effect where GSPI0 is in idle and hence another peripheral which uses shorter pulses would start missing out on the interrupts.

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Kane Chen (Code Review)]

Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 4:
> Patch Set 4:
>
> (2 comments)

Hi Furquan, 
I replied in the issue tracker.
thanks

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 4: Code-Review-1

There's no guarantee that SPI0 is the last device to enter S0ix and the first device to wake up from S0ix, hence this could cause races where other IRQs are missed.

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 4:
> Patch Set 4: Code-Review-1
>
> There's no guarantee that SPI0 is the last device to enter S0ix and the first device to wake up from S0ix, hence this could cause races where other IRQs are missed.

As per Kane, this is must for runtime S0ix, can i suggest to do the same in GFX0 _PS0/_PS3 as GFX would be last device to enter into low power mode in runtime S0ix. I have enable ACPI debug to verify the same.

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Kane Chen (Code Review)]

Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 4:
> Patch Set 4:
> > Patch Set 4: Code-Review-1
> >
> > There's no guarantee that SPI0 is the last device to enter S0ix and the first device to wake up from S0ix, hence this could cause races where other IRQs are missed.
>
> As per Kane, this is must for runtime S0ix, can i suggest to do the same in GFX0 _PS0/_PS3 as GFX would be last device to enter into low power mode in runtime S0ix. I have enable ACPI debug to verify the same.

hi Subrata, 
to be more accurate.
runtime s0ix is still working on tot now since i reverted PchPmSlpS0Vm075VSupport on tot .

This change is needed for runtime s0ix only when PchPmSlpS0Vm075VSupport is set
thanks

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 4:
> Patch Set 4:
> > Patch Set 4:
> > > Patch Set 4: Code-Review-1
> > >
> > > There's no guarantee that SPI0 is the last device to enter S0ix and the first device to wake up from S0ix, hence this could cause races where other IRQs are missed.
> >
> > As per Kane, this is must for runtime S0ix, can i suggest to do the same in GFX0 _PS0/_PS3 as GFX would be last device to enter into low power mode in runtime S0ix. I have enable ACPI debug to verify the same.
>
> hi Subrata, 
> to be more accurate.
> runtime s0ix is still working on tot now since i reverted PchPmSlpS0Vm075VSupport on tot .
>
> This change is needed for runtime s0ix only when PchPmSlpS0Vm075VSupport is set
> thanks

Yes Kane, agree.

Can you please move this W/A into GFX _PS0/_PS3 and run few cycle ?

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Kane Chen (Code Review)]

Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 4:
> Patch Set 4:
> > Patch Set 4:
> > > Patch Set 4:
> > > > Patch Set 4: Code-Review-1
> > > >
> > > > There's no guarantee that SPI0 is the last device to enter S0ix and the first device to wake up from S0ix, hence this could cause races where other IRQs are missed.
> > >
> > > As per Kane, this is must for runtime S0ix, can i suggest to do the same in GFX0 _PS0/_PS3 as GFX would be last device to enter into low power mode in runtime S0ix. I have enable ACPI debug to verify the same.
> >
> > hi Subrata, 
> > to be more accurate.
> > runtime s0ix is still working on tot now since i reverted PchPmSlpS0Vm075VSupport on tot .
> >
> > This change is needed for runtime s0ix only when PchPmSlpS0Vm075VSupport is set
> > thanks
>
> Yes Kane, agree.
>
> Can you please move this W/A into GFX _PS0/_PS3 and run few cycle ?

hmmm, I think putting this WA under any device's _PS0/_PS3 still can't make it solid.
I guess it would be better to put the WA under device whose interrupt is missed.
And for sure, there will be new devices in the future and it might be impacted.

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 4:

So I guess I'm misunderstanding something.  Does the kernel treat "opportunistic" S0ix differently from other entries into S0ix?  And so that's why this is needed?

The LPIT document https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.... doesn't say anything about this, and just says that the OSPM will call the _DSM function with 5 or 6 as the argument upon entering/exiting the S0 Idle state.

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 4:
> Patch Set 4:
>
> So I guess I'm misunderstanding something.  Does the kernel treat "opportunistic" S0ix differently from other entries into S0ix?  And so that's why this is needed?
>
> The LPIT document https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.... doesn't say anything about this, and just says that the OSPM will call the _DSM function with 5 or 6 as the argument upon entering/exiting the S0 Idle state.

I guess that is true for regular S0ix entry and exit but not for runtme S0ix where you just left system idle and it enters into S0ix. In Runtime S0ix, those _DSM won't get called.

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Kane Chen (Code Review)]

Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 4:
> Patch Set 4:
> > Patch Set 4:
> >
> > So I guess I'm misunderstanding something.  Does the kernel treat "opportunistic" S0ix differently from other entries into S0ix?  And so that's why this is needed?
> >
> > The LPIT document https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.... doesn't say anything about this, and just says that the OSPM will call the _DSM function with 5 or 6 as the argument upon entering/exiting the S0 Idle state.
>
> I guess that is true for regular S0ix entry and exit but not for runtme S0ix where you just left system idle and it enters into S0ix. In Runtime S0ix, those _DSM won't get called.

Subrata is correct.
for opportunistic/runtime s0ix, it won't call the _DSM and that's why we are having issue w/ PchPmSlpS0Vm075VSupport set
thanks

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 4:
> Patch Set 4:
> > Patch Set 4:
> > > Patch Set 4:
> > >
> > > So I guess I'm misunderstanding something.  Does the kernel treat "opportunistic" S0ix differently from other entries into S0ix?  And so that's why this is needed?
> > >
> > > The LPIT document https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.... doesn't say anything about this, and just says that the OSPM will call the _DSM function with 5 or 6 as the argument upon entering/exiting the S0 Idle state.
> >
> > I guess that is true for regular S0ix entry and exit but not for runtme S0ix where you just left system idle and it enters into S0ix. In Runtime S0ix, those _DSM won't get called.
>
> Subrata is correct.
> for opportunistic/runtime s0ix, it won't call the _DSM and that's why we are having issue w/ PchPmSlpS0Vm075VSupport set
> thanks

Gotcha.

Anyone have any better ideas?  I'm not coming up with much.

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 4:
> Patch Set 4:
> > Patch Set 4:
> > > Patch Set 4:
> > > > Patch Set 4:
> > > >
> > > > So I guess I'm misunderstanding something.  Does the kernel treat "opportunistic" S0ix differently from other entries into S0ix?  And so that's why this is needed?
> > > >
> > > > The LPIT document https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.... doesn't say anything about this, and just says that the OSPM will call the _DSM function with 5 or 6 as the argument upon entering/exiting the S0 Idle state.
> > >
> > > I guess that is true for regular S0ix entry and exit but not for runtme S0ix where you just left system idle and it enters into S0ix. In Runtime S0ix, those _DSM won't get called.
> >
> > Subrata is correct.
> > for opportunistic/runtime s0ix, it won't call the _DSM and that's why we are having issue w/ PchPmSlpS0Vm075VSupport set
> > thanks
>
> Gotcha.
>
> Anyone have any better ideas?  I'm not coming up with much.

my best bet would be inside GFX _PS0/_PS3 because of last device entering into D3 in runtime idle and other cases. I do remember about many W/A place inside my windows days in ASL specifically either storage or GFX side _PS0/_PS3

Also we have noted the power saving is also huge with and without this CL, this might be useful for devices with lower battery capacity

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Kane Chen (Code Review)]

Hello Subrata Banik, Aamir Bohra, Tim Wawrzynczak, Tim Wawrzynczak, Marx Wang, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/37865

to look at the new patch set (#5).

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

With PchPmSlpS0Vm075VSupport FSP UPD set, SoC requires gpio clk to be
power gated, but when gpio clk is power gated, it requires longer interrupt
assertion from device.

This change fixes runtime s0ix with PchPmSlpS0Vm075VSupport set.

And it sets gpio clk power gating settings in SPIO PS0/PS3 so that
cr50 doesn't need longer interrupt assertion.

BUG=:141831197
TEST=run suspend/stress, cold reset tests 2500 cycles and
     check no irq missed in kernel msg

Change-Id: I33a3d5897ec40afee29759160963363c322d5ad0
Signed-off-by: Kane Chen 
---
M src/mainboard/google/hatch/dsdt.asl
A src/mainboard/google/hatch/mainboard.asl
2 files changed, 41 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/37865/5

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Kane Chen (Code Review)]

Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 5:
> Patch Set 4:
> > Patch Set 4:
> > > Patch Set 4:
> > > > Patch Set 4:
> > > > > Patch Set 4:
> > > > >
> > > > > So I guess I'm misunderstanding something.  Does the kernel treat "opportunistic" S0ix differently from other entries into S0ix?  And so that's why this is needed?
> > > > >
> > > > > The LPIT document https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.... doesn't say anything about this, and just says that the OSPM will call the _DSM function with 5 or 6 as the argument upon entering/exiting the S0 Idle state.
> > > >
> > > > I guess that is true for regular S0ix entry and exit but not for runtme S0ix where you just left system idle and it enters into S0ix. In Runtime S0ix, those _DSM won't get called.
> > >
> > > Subrata is correct.
> > > for opportunistic/runtime s0ix, it won't call the _DSM and that's why we are having issue w/ PchPmSlpS0Vm075VSupport set
> > > thanks
> >
> > Gotcha.
> >
> > Anyone have any better ideas?  I'm not coming up with much.
>
> my best bet would be inside GFX _PS0/_PS3 because of last device entering into D3 in runtime idle and other cases. I do remember about many W/A place inside my windows days in ASL specifically either storage or GFX side _PS0/_PS3
>
> Also we have noted the power saving is also huge with and without this CL, this might be useful for devices with lower battery capacity

I don't see any PS3, PS0 in current GFX acpi device.
I will try put some log in GFX, SPI PS0/PS3 and compare in btwn.

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Kane Chen (Code Review)]

Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 5:
> Patch Set 5:
> > Patch Set 4:
> > > Patch Set 4:
> > > > Patch Set 4:
> > > > > Patch Set 4:
> > > > > > Patch Set 4:
> > > > > >
> > > > > > So I guess I'm misunderstanding something.  Does the kernel treat "opportunistic" S0ix differently from other entries into S0ix?  And so that's why this is needed?
> > > > > >
> > > > > > The LPIT document https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.... doesn't say anything about this, and just says that the OSPM will call the _DSM function with 5 or 6 as the argument upon entering/exiting the S0 Idle state.
> > > > >
> > > > > I guess that is true for regular S0ix entry and exit but not for runtme S0ix where you just left system idle and it enters into S0ix. In Runtime S0ix, those _DSM won't get called.
> > > >
> > > > Subrata is correct.
> > > > for opportunistic/runtime s0ix, it won't call the _DSM and that's why we are having issue w/ PchPmSlpS0Vm075VSupport set
> > > > thanks
> > >
> > > Gotcha.
> > >
> > > Anyone have any better ideas?  I'm not coming up with much.
> >
> > my best bet would be inside GFX _PS0/_PS3 because of last device entering into D3 in runtime idle and other cases. I do remember about many W/A place inside my windows days in ASL specifically either storage or GFX side _PS0/_PS3
> >
> > Also we have noted the power saving is also huge with and without this CL, this might be useful for devices with lower battery capacity
>
> I don't see any PS3, PS0 in current GFX acpi device.
> I will try put some log in GFX, SPI PS0/PS3 and compare in btwn.

Unfortunately, GFX PS3/PS0 are not getting called even when the display is off after idle for a while.
I'm sure the code is added properly and i can see the below code is in acpi.
        Scope (PCI0.GFX0)
        {
            Method (_PS0, 0, NotSerialized)  // _PS0: Power State 0
            {
                Debug = "kaedbg GFX0 PS3"
            }

Method (_PS3, 0, NotSerialized)  // _PS3: Power State 3
            {
                Debug = "kaedbg GFX0 PS0"
            }
        }

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 5:
> Patch Set 5:
> > Patch Set 5:
> > > Patch Set 4:
> > > > Patch Set 4:
> > > > > Patch Set 4:
> > > > > > Patch Set 4:
> > > > > >
> > > > > > > Patch Set 4:
> > > > > > > 
> > > > > > > So I guess I'm misunderstanding something.  Does the kernel treat "opportunistic" S0ix differently from other entries into S0ix?  And so that's why this is needed?
> > > > > > > 
> > > > > > > The LPIT document https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.... doesn't say anything about this, and just says that the OSPM will call the _DSM function with 5 or 6 as the argument upon entering/exiting the S0 Idle state.
> > > > > >
> > > > > > I guess that is true for regular S0ix entry and exit but not for runtme S0ix where you just left system idle and it enters into S0ix. In Runtime S0ix, those _DSM won't get called.
> > > > >
> > > > > Subrata is correct.
> > > > > for opportunistic/runtime s0ix, it won't call the _DSM and that's why we are having issue w/ PchPmSlpS0Vm075VSupport set
> > > > > thanks
> > > >
> > > > Gotcha.
> > > >
> > > > Anyone have any better ideas?  I'm not coming up with much.
> > >
> > > my best bet would be inside GFX _PS0/_PS3 because of last device entering into D3 in runtime idle and other cases. I do remember about many W/A place inside my windows days in ASL specifically either storage or GFX side _PS0/_PS3
> > >
> > > Also we have noted the power saving is also huge with and without this CL, this might be useful for devices with lower battery capacity
> >
> > I don't see any PS3, PS0 in current GFX acpi device.
> > I will try put some log in GFX, SPI PS0/PS3 and compare in btwn.
>
> Unfortunately, GFX PS3/PS0 are not getting called even when the display is off after idle for a while.
> I'm sure the code is added properly and i can see the below code is in acpi.
>         Scope (PCI0.GFX0)
>         {
>             Method (_PS0, 0, NotSerialized)  // _PS0: Power State 0
>             {
>                 Debug = "kaedbg GFX0 PS3"
>             }
>         Method (_PS3, 0, NotSerialized)  // _PS3: Power State 3
>         {
>             Debug = "kaedbg GFX0 PS0"
>         }
>     }

Then it might be chrome GFX driver doesn't bother to call those methods

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 5:
> Patch Set 5:
> > Patch Set 5:
> > > Patch Set 5:
> > > > Patch Set 4:
> > > > > Patch Set 4:
> > > > > > Patch Set 4:
> > > > > >
> > > > > > > Patch Set 4:
> > > > > > > 
> > > > > > > > Patch Set 4:
> > > > > > > > 
> > > > > > > > So I guess I'm misunderstanding something.  Does the kernel treat "opportunistic" S0ix differently from other entries into S0ix?  And so that's why this is needed?
> > > > > > > > 
> > > > > > > > The LPIT document https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.... doesn't say anything about this, and just says that the OSPM will call the _DSM function with 5 or 6 as the argument upon entering/exiting the S0 Idle state.
> > > > > > > 
> > > > > > > I guess that is true for regular S0ix entry and exit but not for runtme S0ix where you just left system idle and it enters into S0ix. In Runtime S0ix, those _DSM won't get called.
> > > > > >
> > > > > > Subrata is correct.
> > > > > > for opportunistic/runtime s0ix, it won't call the _DSM and that's why we are having issue w/ PchPmSlpS0Vm075VSupport set
> > > > > > thanks
> > > > >
> > > > > Gotcha.
> > > > >
> > > > > Anyone have any better ideas?  I'm not coming up with much.
> > > >
> > > > my best bet would be inside GFX _PS0/_PS3 because of last device entering into D3 in runtime idle and other cases. I do remember about many W/A place inside my windows days in ASL specifically either storage or GFX side _PS0/_PS3
> > > >
> > > > Also we have noted the power saving is also huge with and without this CL, this might be useful for devices with lower battery capacity
> > >
> > > I don't see any PS3, PS0 in current GFX acpi device.
> > > I will try put some log in GFX, SPI PS0/PS3 and compare in btwn.
> >
> > Unfortunately, GFX PS3/PS0 are not getting called even when the display is off after idle for a while.
> > I'm sure the code is added properly and i can see the below code is in acpi.
> >         Scope (PCI0.GFX0)
> >         {
> >             Method (_PS0, 0, NotSerialized)  // _PS0: Power State 0
> >             {
> >                 Debug = "kaedbg GFX0 PS3"
> >             }
> >         Method (_PS3, 0, NotSerialized)  // _PS3: Power State 3
> >         {
> >             Debug = "kaedbg GFX0 PS0"
> >         }
> >     }
>
> Then it might be chrome GFX driver doesn't bother to call those methods

Shouldn't that be _SB.PCI0.GFX0 for the Scope?

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Kane Chen (Code Review)]

Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 5:
> Patch Set 5:
> > Patch Set 5:
> > > Patch Set 5:
> > > > Patch Set 5:
> > > > > Patch Set 4:
> > > > > > Patch Set 4:
> > > > > >
> > > > > > > Patch Set 4:
> > > > > > > 
> > > > > > > > Patch Set 4:
> > > > > > > > 
> > > > > > > > > Patch Set 4:
> > > > > > > > > 
> > > > > > > > > So I guess I'm misunderstanding something.  Does the kernel treat "opportunistic" S0ix differently from other entries into S0ix?  And so that's why this is needed?
> > > > > > > > > 
> > > > > > > > > The LPIT document https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.... doesn't say anything about this, and just says that the OSPM will call the _DSM function with 5 or 6 as the argument upon entering/exiting the S0 Idle state.
> > > > > > > > 
> > > > > > > > I guess that is true for regular S0ix entry and exit but not for runtme S0ix where you just left system idle and it enters into S0ix. In Runtime S0ix, those _DSM won't get called.
> > > > > > > 
> > > > > > > Subrata is correct.
> > > > > > > for opportunistic/runtime s0ix, it won't call the _DSM and that's why we are having issue w/ PchPmSlpS0Vm075VSupport set
> > > > > > > thanks
> > > > > >
> > > > > > Gotcha.
> > > > > >
> > > > > > Anyone have any better ideas?  I'm not coming up with much.
> > > > >
> > > > > my best bet would be inside GFX _PS0/_PS3 because of last device entering into D3 in runtime idle and other cases. I do remember about many W/A place inside my windows days in ASL specifically either storage or GFX side _PS0/_PS3
> > > > >
> > > > > Also we have noted the power saving is also huge with and without this CL, this might be useful for devices with lower battery capacity
> > > >
> > > > I don't see any PS3, PS0 in current GFX acpi device.
> > > > I will try put some log in GFX, SPI PS0/PS3 and compare in btwn.
> > >
> > > Unfortunately, GFX PS3/PS0 are not getting called even when the display is off after idle for a while.
> > > I'm sure the code is added properly and i can see the below code is in acpi.
> > >         Scope (PCI0.GFX0)
> > >         {
> > >             Method (_PS0, 0, NotSerialized)  // _PS0: Power State 0
> > >             {
> > >                 Debug = "kaedbg GFX0 PS3"
> > >             }
> > >         Method (_PS3, 0, NotSerialized)  // _PS3: Power State 3
> > >         {
> > >             Debug = "kaedbg GFX0 PS0"
> > >         }
> > >     }
> >
> > Then it might be chrome GFX driver doesn't bother to call those methods
>
> Shouldn't that be _SB.PCI0.GFX0 for the Scope?

the code i added is _SB.PCI0.GFX0, it's just the iasl dissemble in this way.
i also used same way for SPI0 and they are called properly

+
+Scope (_SB.PCI0.GFX0)
+{
+       Method (_PS0, 0)
+       {
+               Store("kaedbg GFX0 PS3", Debug)
+       }
+
+       Method(_PS3, 0)
+       {
+               Store("kaedbg GFX0 PS0", Debug)
+       }
+}

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 5:
> Patch Set 5:
> > Patch Set 5:
> > > Patch Set 5:
> > > > Patch Set 5:
> > > > > Patch Set 5:
> > > > > > Patch Set 4:
> > > > > >
> > > > > > > Patch Set 4:
> > > > > > > 
> > > > > > > > Patch Set 4:
> > > > > > > > 
> > > > > > > > > Patch Set 4:
> > > > > > > > > 
> > > > > > > > > > Patch Set 4:
> > > > > > > > > > 
> > > > > > > > > > So I guess I'm misunderstanding something.  Does the kernel treat "opportunistic" S0ix differently from other entries into S0ix?  And so that's why this is needed?
> > > > > > > > > > 
> > > > > > > > > > The LPIT document https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.... doesn't say anything about this, and just says that the OSPM will call the _DSM function with 5 or 6 as the argument upon entering/exiting the S0 Idle state.
> > > > > > > > > 
> > > > > > > > > I guess that is true for regular S0ix entry and exit but not for runtme S0ix where you just left system idle and it enters into S0ix. In Runtime S0ix, those _DSM won't get called.
> > > > > > > > 
> > > > > > > > Subrata is correct.
> > > > > > > > for opportunistic/runtime s0ix, it won't call the _DSM and that's why we are having issue w/ PchPmSlpS0Vm075VSupport set
> > > > > > > > thanks
> > > > > > > 
> > > > > > > Gotcha.
> > > > > > > 
> > > > > > > Anyone have any better ideas?  I'm not coming up with much.
> > > > > >
> > > > > > my best bet would be inside GFX _PS0/_PS3 because of last device entering into D3 in runtime idle and other cases. I do remember about many W/A place inside my windows days in ASL specifically either storage or GFX side _PS0/_PS3
> > > > > >
> > > > > > Also we have noted the power saving is also huge with and without this CL, this might be useful for devices with lower battery capacity
> > > > >
> > > > > I don't see any PS3, PS0 in current GFX acpi device.
> > > > > I will try put some log in GFX, SPI PS0/PS3 and compare in btwn.
> > > >
> > > > Unfortunately, GFX PS3/PS0 are not getting called even when the display is off after idle for a while.
> > > > I'm sure the code is added properly and i can see the below code is in acpi.
> > > >         Scope (PCI0.GFX0)
> > > >         {
> > > >             Method (_PS0, 0, NotSerialized)  // _PS0: Power State 0
> > > >             {
> > > >                 Debug = "kaedbg GFX0 PS3"
> > > >             }
> > > >         Method (_PS3, 0, NotSerialized)  // _PS3: Power State 3
> > > >         {
> > > >             Debug = "kaedbg GFX0 PS0"
> > > >         }
> > > >     }
> > >
> > > Then it might be chrome GFX driver doesn't bother to call those methods
> >
> > Shouldn't that be _SB.PCI0.GFX0 for the Scope?
>
> the code i added is _SB.PCI0.GFX0, it's just the iasl dissemble in this way.
> i also used same way for SPI0 and they are called properly
>
> +Scope (_SB.PCI0.GFX0)
> +{
>   Method (_PS0, 0)
>   {
>           Store("kaedbg GFX0 PS3", Debug)
>   }
>   Method(_PS3, 0)
>   {
>           Store("kaedbg GFX0 PS0", Debug)
>   }
>
> +}

Oh sorry, gotcha.

What guarantees that GFX is the last device to run _PS3 ?

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Kane Chen (Code Review)]

Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 5:
> Patch Set 5:
> > Patch Set 5:
> > > Patch Set 5:
> > > > Patch Set 5:
> > > > > Patch Set 5:
> > > > > > Patch Set 5:
> > > > > >
> > > > > > > Patch Set 4:
> > > > > > > 
> > > > > > > > Patch Set 4:
> > > > > > > > 
> > > > > > > > > Patch Set 4:
> > > > > > > > > 
> > > > > > > > > > Patch Set 4:
> > > > > > > > > > 
> > > > > > > > > > > Patch Set 4:
> > > > > > > > > > > 
> > > > > > > > > > > So I guess I'm misunderstanding something.  Does the kernel treat "opportunistic" S0ix differently from other entries into S0ix?  And so that's why this is needed?
> > > > > > > > > > > 
> > > > > > > > > > > The LPIT document https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.... doesn't say anything about this, and just says that the OSPM will call the _DSM function with 5 or 6 as the argument upon entering/exiting the S0 Idle state.
> > > > > > > > > > 
> > > > > > > > > > I guess that is true for regular S0ix entry and exit but not for runtme S0ix where you just left system idle and it enters into S0ix. In Runtime S0ix, those _DSM won't get called.
> > > > > > > > > 
> > > > > > > > > Subrata is correct.
> > > > > > > > > for opportunistic/runtime s0ix, it won't call the _DSM and that's why we are having issue w/ PchPmSlpS0Vm075VSupport set
> > > > > > > > > thanks
> > > > > > > > 
> > > > > > > > Gotcha.
> > > > > > > > 
> > > > > > > > Anyone have any better ideas?  I'm not coming up with much.
> > > > > > > 
> > > > > > > my best bet would be inside GFX _PS0/_PS3 because of last device entering into D3 in runtime idle and other cases. I do remember about many W/A place inside my windows days in ASL specifically either storage or GFX side _PS0/_PS3
> > > > > > > 
> > > > > > > Also we have noted the power saving is also huge with and without this CL, this might be useful for devices with lower battery capacity
> > > > > >
> > > > > > I don't see any PS3, PS0 in current GFX acpi device.
> > > > > > I will try put some log in GFX, SPI PS0/PS3 and compare in btwn.
> > > > >
> > > > > Unfortunately, GFX PS3/PS0 are not getting called even when the display is off after idle for a while.
> > > > > I'm sure the code is added properly and i can see the below code is in acpi.
> > > > >         Scope (PCI0.GFX0)
> > > > >         {
> > > > >             Method (_PS0, 0, NotSerialized)  // _PS0: Power State 0
> > > > >             {
> > > > >                 Debug = "kaedbg GFX0 PS3"
> > > > >             }
> > > > >         Method (_PS3, 0, NotSerialized)  // _PS3: Power State 3
> > > > >         {
> > > > >             Debug = "kaedbg GFX0 PS0"
> > > > >         }
> > > > >     }
> > > >
> > > > Then it might be chrome GFX driver doesn't bother to call those methods
> > >
> > > Shouldn't that be _SB.PCI0.GFX0 for the Scope?
> >
> > the code i added is _SB.PCI0.GFX0, it's just the iasl dissemble in this way.
> > i also used same way for SPI0 and they are called properly
> >
> > +Scope (_SB.PCI0.GFX0)
> > +{
> >   Method (_PS0, 0)
> >   {
> >           Store("kaedbg GFX0 PS3", Debug)
> >   }
> >   Method(_PS3, 0)
> >   {
> >           Store("kaedbg GFX0 PS0", Debug)
> >   }
> >
> > +}
>
> Oh sorry, gotcha.
>
> What guarantees that GFX is the last device to run _PS3 ?

From my experiments, the GFX PS3/PS0 are never called even when the display is off after long idle.
I guess GFX driver has its way to handle that so it doesn't call ACPI

Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Kane Chen (Code Review)]

Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37865 )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Patch Set 5:

+Duncan 
Hi Duncan, 
Do you have any other idea to WA such issue.
thank you

[S] Change in coreboot[master]: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0

[Stefan Reinauer (Code Review)]

Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37865?usp=email )

Change subject: mb/google/hatch: Program gpio clk power gating settings in SPI0 PS3/PS0
......................................................................

Abandoned