[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 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