Re: [PATCH v2 1/2] clk: qcom: gcc-sc7180: Keep the USB GDSC always on

[Rajendra Nayak <quic_rjendra@quicinc.com>]

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@chromium.org>
> 
> 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.
Does that sounds like a reasonable solution?

Thanks,
Rajendra

[1] https://patchwork.kernel.org/project/linux-arm-msm/patch/1630346073-7099-2-git-send-email-sanm@codeaurora.org/

> Thanks,
> Bjorn
> 
>> ---
>> I'm not entirely sure that this is the correct solution. What makes
>> me doubt is that only msm8953 sets ALWAYS_ON for the USB GDSC. Is USB
>> broken after suspend on all the other QC platforms?
>>
>> Changes in v2:
>> - set the flags of the GDSC not of the GDSC power domain
>> - updated commit message
>>
>>   drivers/clk/qcom/gcc-sc7180.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/clk/qcom/gcc-sc7180.c b/drivers/clk/qcom/gcc-sc7180.c
>> index c2ea09945c47..c0d7509a782e 100644
>> --- a/drivers/clk/qcom/gcc-sc7180.c
>> +++ b/drivers/clk/qcom/gcc-sc7180.c
>> @@ -2225,6 +2225,7 @@ static struct gdsc usb30_prim_gdsc = {
>>   		.name = "usb30_prim_gdsc",
>>   	},
>>   	.pwrsts = PWRSTS_OFF_ON,
>> +	.flags = ALWAYS_ON,
>>   };
>>   
>>   static struct gdsc hlos1_vote_mmnoc_mmu_tbu_hf0_gdsc = {
>> -- 
>> 2.37.2.672.g94769d06f0-goog
>>