[coreboot-gerrit] 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)
+       }
+}