On Mon, Aug 29, 2022 at 01:42:02PM +0530, Rajendra Nayak wrote: > > On 8/26/2022 8:10 AM, Bjorn Andersson wrote: > > On Thu, Aug 25, 2022 at 06:21:58PM -0700, Matthias Kaehlcke wrote: > > > When the GDSC is disabled during system suspend USB is broken on > > > sc7180 when the system resumes. Mark the GDSC as always on to > > > make sure USB still works after system suspend. > > > > > > Signed-off-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx> > > > > Rajendra, where you able to find some time to look into take the GDSC > > into retention state? Do you suggest that I merge these two patches for > > now? > > > > Hi Bjorn, based on my experiments to support retention on sc7280 these are > some of my findings > > On Platforms which support CX retention (for example sc7180/sc7280) instead of > CX PowerCollapse (PC), We can leave the GDSC turned ON. When CX transitions to RET state > the GDSC goes into retention too (some controller state is retained) and USB wakeups work. > > On platforms which support CX PC, just leaving the GDSC > turned ON will not help since the GDSC will also transition to OFF state > when we enter CX PC, hence wake-ups from USB won't work. > For such platforms we need to make sure gdsc_force_mem_on() is called > and cxcs (* @cxcs: offsets of branch registers to toggle mem/periph bits in) > are populated correctly, while leaving the GDSC turned ON. > This will make sure usb gdsc transitions from being powered by CX to MX > when CX hits PC and we still get USB wakeups to work. > So in short we could do the same thing that this patch does on those > platforms too with additionally populating the right cxcs entries and it > should just work fine. > > Now the problem that I see with this approach is not with getting USB wakeups > to work in suspend, but with supporting performance state voting when > USB is active. > The last conclusion we had on that [1] was to model usb_gdsc as a subdomain of CX, > so if we do that and we model usb_gdsc as something that supports ALWAYS_ON, > we would _never_ drop the CX vote and prevent CX from going down (either to ret > or pc) > > The only way I think we can solve both the USB wakeups and performance state > needs (with usb_gdsc as a subdomain of CX) is if we can model a RET state for gdsc > which sets the mem/periph bits while leaving the GDSC ON (Today the RET state sets > the mem/periph bits but turns the GDSC OFF) > > That would mean a change in gdsc.c like this > --- > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > index d3244006c661..0fe017ba901b 100644 > --- a/drivers/clk/qcom/gdsc.c > +++ b/drivers/clk/qcom/gdsc.c > @@ -368,6 +368,10 @@ static int _gdsc_disable(struct gdsc *sc) > if (sc->pwrsts & PWRSTS_OFF) > gdsc_clear_mem_on(sc); > > + /* If the GDSC supports RET, do not explicitly power it off */ > + if (sc->pwrsts & PWRSTS_RET) > + return 0; > + > ret = gdsc_toggle_logic(sc, GDSC_OFF); > if (ret) > return ret; > > > So with that change, we would then not need the ALWAYS_ON flag set for usb gdsc, > instead we would update the .pwrsts to PWRSTS_RET_ON instead of PWRSTS_OFF_ON, > and that should make both usb wake-ups to work and we can still have the usb_gdsc as > a subdomain of CX for performance state voting. Krishna and I confirmed that this works from the USB side for sc7180 and sc7280.
