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

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

[Matthias Kaehlcke]

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

[PATCH v2 2/2] clk: qcom: gcc-sc7280: Keep the USB GDSCs always on

[Matthias Kaehlcke]

When the GDSC is disabled during system suspend USB is broken on
sc7280 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>
---

Changes in v2:
- set the flags of the GDSC not of the GDSC power domain
- updated commit message

 drivers/clk/qcom/gcc-sc7280.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/gcc-sc7280.c b/drivers/clk/qcom/gcc-sc7280.c
index 7ff64d4d5920..adef68d2cb0b 100644
--- a/drivers/clk/qcom/gcc-sc7280.c
+++ b/drivers/clk/qcom/gcc-sc7280.c
@@ -3127,7 +3127,7 @@ static struct gdsc gcc_usb30_prim_gdsc = {
 		.name = "gcc_usb30_prim_gdsc",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
-	.flags = VOTABLE,
+	.flags = VOTABLE | ALWAYS_ON,
 };
 
 static struct gdsc gcc_usb30_sec_gdsc = {
-- 
2.37.2.672.g94769d06f0-goog

Re: [PATCH v2 2/2] clk: qcom: gcc-sc7280: Keep the USB GDSCs always on

[Johan Hovold]

On Thu, Aug 25, 2022 at 06:21:59PM -0700, Matthias Kaehlcke wrote:
> When the GDSC is disabled during system suspend USB is broken on
> sc7280 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>
> ---
> 
> Changes in v2:
> - set the flags of the GDSC not of the GDSC power domain
> - updated commit message
> 
>  drivers/clk/qcom/gcc-sc7280.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/gcc-sc7280.c b/drivers/clk/qcom/gcc-sc7280.c
> index 7ff64d4d5920..adef68d2cb0b 100644
> --- a/drivers/clk/qcom/gcc-sc7280.c
> +++ b/drivers/clk/qcom/gcc-sc7280.c

Perhaps you can add a comment here about why this is needed similar to
what I did for sc8280xp:

	https://lore.kernel.org/all/20220805121250.10347-3-johan+linaro@kernel.org/

> @@ -3127,7 +3127,7 @@ static struct gdsc gcc_usb30_prim_gdsc = {
>  		.name = "gcc_usb30_prim_gdsc",
>  	},
>  	.pwrsts = PWRSTS_OFF_ON,
> -	.flags = VOTABLE,
> +	.flags = VOTABLE | ALWAYS_ON,
>  };
>  
>  static struct gdsc gcc_usb30_sec_gdsc = {

Look good otherwise. For both patches:

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>

Johan

Re: [PATCH v2 2/2] clk: qcom: gcc-sc7280: Keep the USB GDSCs always on

[Matthias Kaehlcke]

On Fri, Aug 26, 2022 at 09:16:38AM +0200, Johan Hovold wrote:
> On Thu, Aug 25, 2022 at 06:21:59PM -0700, Matthias Kaehlcke wrote:
> > When the GDSC is disabled during system suspend USB is broken on
> > sc7280 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>
> > ---
> > 
> > Changes in v2:
> > - set the flags of the GDSC not of the GDSC power domain
> > - updated commit message
> > 
> >  drivers/clk/qcom/gcc-sc7280.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/qcom/gcc-sc7280.c b/drivers/clk/qcom/gcc-sc7280.c
> > index 7ff64d4d5920..adef68d2cb0b 100644
> > --- a/drivers/clk/qcom/gcc-sc7280.c
> > +++ b/drivers/clk/qcom/gcc-sc7280.c
> 
> Perhaps you can add a comment here about why this is needed similar to
> what I did for sc8280xp:
> 
> 	https://lore.kernel.org/all/20220805121250.10347-3-johan+linaro@kernel.org/

From Bjorn's comment on the sc7180 patch it seems it's not an dwc3
implementation issue. IIUC the GDSCs should have a retention state
that would be used during suspend.

> > @@ -3127,7 +3127,7 @@ static struct gdsc gcc_usb30_prim_gdsc = {
> >  		.name = "gcc_usb30_prim_gdsc",
> >  	},
> >  	.pwrsts = PWRSTS_OFF_ON,
> > -	.flags = VOTABLE,
> > +	.flags = VOTABLE | ALWAYS_ON,
> >  };
> >  
> >  static struct gdsc gcc_usb30_sec_gdsc = {
> 
> Look good otherwise. For both patches:
> 
> Reviewed-by: Johan Hovold <johan+linaro@kernel.org>

Thanks!

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

[Bjorn Andersson]

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?

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
> 

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

[Rajendra Nayak]

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

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

[Matthias Kaehlcke]

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@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.

Krishna and I confirmed that this works from the USB side for sc7180 and sc7280.

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

[Stephen Boyd]

Quoting Rajendra Nayak (2022-08-29 01:12:02)
> 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.

To clarify, usb_gdsc is not setup as a subdomain of CX so far, right?
Just that eventually we'll make usb_gdsc a subdomain of CX so that
dev_pm_opp_set_rate() can target the CX domain instead of the usb one?

> Does that sounds like a reasonable solution?

It sounds good to me. What about the existing users of PWRSTS_RET
though? If I understand correctly this flag means the domain will never
be turned off, instead it will hit retention during low power modes.

While you're crafting this patch can you also document the PWRSTS_*
defines so that we know what they mean? I can guess that PWRSTS_RET
means "retention" but I don't know what it really means. I guess it
means "Deepest power off state is retention of memory cells".

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

[Rajendra Nayak]

On 8/31/2022 3:05 AM, Stephen Boyd wrote:
> Quoting Rajendra Nayak (2022-08-29 01:12:02)
>> 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.
> 
> To clarify, usb_gdsc is not setup as a subdomain of CX so far, right?
> Just that eventually we'll make usb_gdsc a subdomain of CX so that
> dev_pm_opp_set_rate() can target the CX domain instead of the usb one?

Thats right,

> 
>> Does that sounds like a reasonable solution?
> 
> It sounds good to me. What about the existing users of PWRSTS_RET

hmm, I thought no ones been actually using PWRSTS_RET but looks like
there is *one* user on msm8974, I'll need to dig up how this change would
affect it,

> though? If I understand correctly this flag means the domain will never
> be turned off, instead it will hit retention during low power modes.

right,

> 
> While you're crafting this patch can you also document the PWRSTS_*
> defines so that we know what they mean? I can guess that PWRSTS_RET
> means "retention" but I don't know what it really means. I guess it
> means "Deepest power off state is retention of memory cells".

Sure I will update the PWRSTS_RET description. The definition of 'retention'
here I think remains the same, it means memory is retained (if RETAIN_MEM is set)
or memory *and* some part of logic is retained (if both RETAIN_MEM and RETAIN_PERIPH
are set) in Deepest power off state, however there is a disconnected in terms of
'how to enter the retention state' for a given domain.
There is no SW control to enter the retention state and this happens in HW when the
parent domain/rail transitions to low power. Either the parents lowest power state
is also retention (like is the case with sc7180/sc7280 for usb with cx as parent) or the
parent makes sure it hands the domain over to another parent (CX to MX in some cases)
to support retention.