[PATCH 1/3] clk: qcom: gdsc: Fix the handling of PWRSTS_RET support

[PATCH 1/3] clk: qcom: gdsc: Fix the handling of PWRSTS_RET support

[Rajendra Nayak]

GDSCs cannot be transitioned into a Retention state in SW.
When either the RETAIN_MEM bit, or both the RETAIN_MEM and
RETAIN_PERIPH bits are set, and the GDSC is left ON, the HW
takes care of retaining the memory/logic for the domain when
the parent domain transitions to low power state.
The existing logic handling the PWRSTS_RET seems to set the
RETAIN_MEM/RETAIN_PERIPH bits but then explicitly turns the
GDSC OFF as part of _gdsc_disable(). Fix that by leaving the
GDSC in ON state.

Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
---
There are a few existing users of PWRSTS_RET and I am not
sure if they would be impacted with this change

1. mdss_gdsc in mmcc-msm8974.c, I am expecting that the
gdsc is actually transitioning to OFF and might be left
ON as part of this change, atleast till we hit system wide
low power state.
If we really leak more power because of this
change, the right thing to do would be to update .pwrsts for
mdss_gdsc to PWRSTS_OFF_ON instead of PWRSTS_RET_ON
I dont have a msm8974 hardware, so if anyone who has can report
any issues I can take a look further on how to fix it.

2. gpu_gx_gdsc in gpucc-msm8998.c and
   gpu_gx_gdsc in gpucc-sdm660.c
Both of these seem to add support for 3 power state
OFF, RET and ON, however I dont see any logic in gdsc
driver to handle 3 different power states.
So I am expecting that these are infact just transitioning
between ON and OFF and RET state is never really used.
The ideal fix for them would be to just update their resp.
.pwrsts to PWRSTS_OFF_ON only. 

 drivers/clk/qcom/gdsc.c | 10 ++++++++++
 drivers/clk/qcom/gdsc.h |  5 +++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index d3244006c661..ccf63771e852 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -368,6 +368,16 @@ static int _gdsc_disable(struct gdsc *sc)
 	if (sc->pwrsts & PWRSTS_OFF)
 		gdsc_clear_mem_on(sc);
 
+	/*
+	 * If the GDSC supports only a Retention state, apart from ON,
+	 * leave it in ON state.
+	 * There is no SW control to transition the GDSC into
+	 * Retention state. This happens in HW when the parent
+	 * domain goes down to a Low power state
+	 */
+	if (sc->pwrsts == PWRSTS_RET_ON)
+		return 0;
+
 	ret = gdsc_toggle_logic(sc, GDSC_OFF);
 	if (ret)
 		return ret;
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index 5de48c9439b2..981a12c8502d 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -49,6 +49,11 @@ struct gdsc {
 	const u8			pwrsts;
 /* Powerdomain allowable state bitfields */
 #define PWRSTS_OFF		BIT(0)
+/*
+ * There is no SW control to transition a GDSC into
+ * PWRSTS_RET. This happens in HW when the parent
+ * domain goes down to a low power state
+ */
 #define PWRSTS_RET		BIT(1)
 #define PWRSTS_ON		BIT(2)
 #define PWRSTS_OFF_ON		(PWRSTS_OFF | PWRSTS_ON)
-- 
2.17.1

[PATCH 2/3] clk: qcom: gcc-sc7180: Update the .pwrsts for usb gdsc

[Rajendra Nayak]

USB on sc7180 cannot support wakeups from low power states
if the GDSC is turned OFF. Update the .pwrsts for usb GDSC so it
only transitions to RET in low power.

Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
---
 drivers/clk/qcom/gcc-sc7180.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/gcc-sc7180.c b/drivers/clk/qcom/gcc-sc7180.c
index c2ea09945c47..2d3980251e78 100644
--- a/drivers/clk/qcom/gcc-sc7180.c
+++ b/drivers/clk/qcom/gcc-sc7180.c
@@ -2224,7 +2224,7 @@ static struct gdsc usb30_prim_gdsc = {
 	.pd = {
 		.name = "usb30_prim_gdsc",
 	},
-	.pwrsts = PWRSTS_OFF_ON,
+	.pwrsts = PWRSTS_RET_ON,
 };
 
 static struct gdsc hlos1_vote_mmnoc_mmu_tbu_hf0_gdsc = {
-- 
2.17.1

[PATCH 3/3] clk: qcom: gcc-sc7280: Update the .pwrsts for usb gdsc

[Rajendra Nayak]

USB on sc7280 cannot support wakeups from low power states
if the GDSC is turned OFF. Update the .pwrsts for usb GDSC so it
only transitions to RET in low power.

Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
---
 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..de29a034e725 100644
--- a/drivers/clk/qcom/gcc-sc7280.c
+++ b/drivers/clk/qcom/gcc-sc7280.c
@@ -3126,7 +3126,7 @@ static struct gdsc gcc_usb30_prim_gdsc = {
 	.pd = {
 		.name = "gcc_usb30_prim_gdsc",
 	},
-	.pwrsts = PWRSTS_OFF_ON,
+	.pwrsts = PWRSTS_RET_ON,
 	.flags = VOTABLE,
 };
 
-- 
2.17.1

Re: [PATCH 2/3] clk: qcom: gcc-sc7180: Update the .pwrsts for usb gdsc

[Matthias Kaehlcke]

On Thu, Sep 01, 2022 at 03:47:55PM +0530, Rajendra Nayak wrote:
> USB on sc7180 cannot support wakeups from low power states
> if the GDSC is turned OFF. Update the .pwrsts for usb GDSC so it
> only transitions to RET in low power.
> 
> Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Tested-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH 3/3] clk: qcom: gcc-sc7280: Update the .pwrsts for usb gdsc

[Matthias Kaehlcke]

On Thu, Sep 01, 2022 at 03:47:56PM +0530, Rajendra Nayak wrote:
> USB on sc7280 cannot support wakeups from low power states
> if the GDSC is turned OFF. Update the .pwrsts for usb GDSC so it
> only transitions to RET in low power.
> 
> Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Tested-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH 1/3] clk: qcom: gdsc: Fix the handling of PWRSTS_RET support

[Matthias Kaehlcke]

On Thu, Sep 01, 2022 at 03:47:54PM +0530, Rajendra Nayak wrote:
> GDSCs cannot be transitioned into a Retention state in SW.
> When either the RETAIN_MEM bit, or both the RETAIN_MEM and
> RETAIN_PERIPH bits are set, and the GDSC is left ON, the HW
> takes care of retaining the memory/logic for the domain when
> the parent domain transitions to low power state.
> The existing logic handling the PWRSTS_RET seems to set the
> RETAIN_MEM/RETAIN_PERIPH bits but then explicitly turns the
> GDSC OFF as part of _gdsc_disable(). Fix that by leaving the
> GDSC in ON state.
> 
> Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
> Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> ---
> There are a few existing users of PWRSTS_RET and I am not
> sure if they would be impacted with this change
> 
> 1. mdss_gdsc in mmcc-msm8974.c, I am expecting that the
> gdsc is actually transitioning to OFF and might be left
> ON as part of this change, atleast till we hit system wide
> low power state.
> If we really leak more power because of this
> change, the right thing to do would be to update .pwrsts for
> mdss_gdsc to PWRSTS_OFF_ON instead of PWRSTS_RET_ON
> I dont have a msm8974 hardware, so if anyone who has can report
> any issues I can take a look further on how to fix it.
> 
> 2. gpu_gx_gdsc in gpucc-msm8998.c and
>    gpu_gx_gdsc in gpucc-sdm660.c
> Both of these seem to add support for 3 power state
> OFF, RET and ON, however I dont see any logic in gdsc
> driver to handle 3 different power states.
> So I am expecting that these are infact just transitioning
> between ON and OFF and RET state is never really used.
> The ideal fix for them would be to just update their resp.
> .pwrsts to PWRSTS_OFF_ON only.

So far nobody has reported back on this. What are the next steps?

If we want to confirm the actual behavior on those platforms
before landing this maybe you or Bjorn could try to locate
someone within QC with access to such systems.

>  drivers/clk/qcom/gdsc.c | 10 ++++++++++
>  drivers/clk/qcom/gdsc.h |  5 +++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index d3244006c661..ccf63771e852 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -368,6 +368,16 @@ static int _gdsc_disable(struct gdsc *sc)
>  	if (sc->pwrsts & PWRSTS_OFF)
>  		gdsc_clear_mem_on(sc);
>  
> +	/*
> +	 * If the GDSC supports only a Retention state, apart from ON,
> +	 * leave it in ON state.
> +	 * There is no SW control to transition the GDSC into
> +	 * Retention state. This happens in HW when the parent
> +	 * domain goes down to a Low power state
> +	 */
> +	if (sc->pwrsts == PWRSTS_RET_ON)
> +		return 0;
> +
>  	ret = gdsc_toggle_logic(sc, GDSC_OFF);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> index 5de48c9439b2..981a12c8502d 100644
> --- a/drivers/clk/qcom/gdsc.h
> +++ b/drivers/clk/qcom/gdsc.h
> @@ -49,6 +49,11 @@ struct gdsc {
>  	const u8			pwrsts;
>  /* Powerdomain allowable state bitfields */
>  #define PWRSTS_OFF		BIT(0)
> +/*
> + * There is no SW control to transition a GDSC into
> + * PWRSTS_RET. This happens in HW when the parent
> + * domain goes down to a low power state
> + */
>  #define PWRSTS_RET		BIT(1)
>  #define PWRSTS_ON		BIT(2)
>  #define PWRSTS_OFF_ON		(PWRSTS_OFF | PWRSTS_ON)
> -- 
> 2.17.1
> 

Re: [PATCH 1/3] clk: qcom: gdsc: Fix the handling of PWRSTS_RET support

[Rajendra Nayak]

On 9/1/2022 3:47 PM, Rajendra Nayak wrote:
> GDSCs cannot be transitioned into a Retention state in SW.
> When either the RETAIN_MEM bit, or both the RETAIN_MEM and
> RETAIN_PERIPH bits are set, and the GDSC is left ON, the HW
> takes care of retaining the memory/logic for the domain when
> the parent domain transitions to low power state.
> The existing logic handling the PWRSTS_RET seems to set the
> RETAIN_MEM/RETAIN_PERIPH bits but then explicitly turns the
> GDSC OFF as part of _gdsc_disable(). Fix that by leaving the
> GDSC in ON state.

Any thoughts on this patch? We now have at-least one more user,
PCIe [1] that wants to follow in the footsteps of USB and use RET
state to support wake-ups from low power state.

[1] https://lore.kernel.org/lkml/20220913164233.GF25849@workstation/

> 
> Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
> Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> ---
> There are a few existing users of PWRSTS_RET and I am not
> sure if they would be impacted with this change
> 
> 1. mdss_gdsc in mmcc-msm8974.c, I am expecting that the
> gdsc is actually transitioning to OFF and might be left
> ON as part of this change, atleast till we hit system wide
> low power state.
> If we really leak more power because of this
> change, the right thing to do would be to update .pwrsts for
> mdss_gdsc to PWRSTS_OFF_ON instead of PWRSTS_RET_ON
> I dont have a msm8974 hardware, so if anyone who has can report
> any issues I can take a look further on how to fix it.
> 
> 2. gpu_gx_gdsc in gpucc-msm8998.c and
>     gpu_gx_gdsc in gpucc-sdm660.c
> Both of these seem to add support for 3 power state
> OFF, RET and ON, however I dont see any logic in gdsc
> driver to handle 3 different power states.
> So I am expecting that these are infact just transitioning
> between ON and OFF and RET state is never really used.
> The ideal fix for them would be to just update their resp.
> .pwrsts to PWRSTS_OFF_ON only.
> 
>   drivers/clk/qcom/gdsc.c | 10 ++++++++++
>   drivers/clk/qcom/gdsc.h |  5 +++++
>   2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index d3244006c661..ccf63771e852 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -368,6 +368,16 @@ static int _gdsc_disable(struct gdsc *sc)
>   	if (sc->pwrsts & PWRSTS_OFF)
>   		gdsc_clear_mem_on(sc);
>   
> +	/*
> +	 * If the GDSC supports only a Retention state, apart from ON,
> +	 * leave it in ON state.
> +	 * There is no SW control to transition the GDSC into
> +	 * Retention state. This happens in HW when the parent
> +	 * domain goes down to a Low power state
> +	 */
> +	if (sc->pwrsts == PWRSTS_RET_ON)
> +		return 0;
> +
>   	ret = gdsc_toggle_logic(sc, GDSC_OFF);
>   	if (ret)
>   		return ret;
> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> index 5de48c9439b2..981a12c8502d 100644
> --- a/drivers/clk/qcom/gdsc.h
> +++ b/drivers/clk/qcom/gdsc.h
> @@ -49,6 +49,11 @@ struct gdsc {
>   	const u8			pwrsts;
>   /* Powerdomain allowable state bitfields */
>   #define PWRSTS_OFF		BIT(0)
> +/*
> + * There is no SW control to transition a GDSC into
> + * PWRSTS_RET. This happens in HW when the parent
> + * domain goes down to a low power state
> + */
>   #define PWRSTS_RET		BIT(1)
>   #define PWRSTS_ON		BIT(2)
>   #define PWRSTS_OFF_ON		(PWRSTS_OFF | PWRSTS_ON)

Re: [PATCH 3/3] clk: qcom: gcc-sc7280: Update the .pwrsts for usb gdsc

[Johan Hovold]

On Thu, Sep 01, 2022 at 03:47:56PM +0530, Rajendra Nayak wrote:
> USB on sc7280 cannot support wakeups from low power states
> if the GDSC is turned OFF. Update the .pwrsts for usb GDSC so it
> only transitions to RET in low power.

It seems this isn't just needed for wakeup to work. On both sc7280 and
sc8280xp the controller doesn't resume properly if the domain has been
powered off (i.e. regardless of whether wakeup is enabled or not).

Are you sure there's no state that needs to be retained regardless of
the wakeup setting?

> Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
> ---
>  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..de29a034e725 100644
> --- a/drivers/clk/qcom/gcc-sc7280.c
> +++ b/drivers/clk/qcom/gcc-sc7280.c
> @@ -3126,7 +3126,7 @@ static struct gdsc gcc_usb30_prim_gdsc = {
>  	.pd = {
>  		.name = "gcc_usb30_prim_gdsc",
>  	},
> -	.pwrsts = PWRSTS_OFF_ON,
> +	.pwrsts = PWRSTS_RET_ON,
>  	.flags = VOTABLE,
>  };

And what about gcc_usb30_sec_gdsc?

Johan

Re: [PATCH 3/3] clk: qcom: gcc-sc7280: Update the .pwrsts for usb gdsc

[Johan Hovold]

On Thu, Sep 01, 2022 at 09:04:55AM -0700, Matthias Kaehlcke wrote:
> On Thu, Sep 01, 2022 at 03:47:56PM +0530, Rajendra Nayak wrote:
> > USB on sc7280 cannot support wakeups from low power states
> > if the GDSC is turned OFF. Update the .pwrsts for usb GDSC so it
> > only transitions to RET in low power.
> > 
> > Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
> 
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> Tested-by: Matthias Kaehlcke <mka@chromium.org>

Did you confirm that you actually hit the retention state?

IIUC, this series is equivalent to using ALWAYS_ON unless CX is actually
powered off during suspend.

Johan

Re: [PATCH 3/3] clk: qcom: gcc-sc7280: Update the .pwrsts for usb gdsc

[Krishna Kurapati PSSNV]

On 9/14/2022 12:39 PM, Johan Hovold wrote:
> On Thu, Sep 01, 2022 at 03:47:56PM +0530, Rajendra Nayak wrote:
>> USB on sc7280 cannot support wakeups from low power states
>> if the GDSC is turned OFF. Update the .pwrsts for usb GDSC so it
>> only transitions to RET in low power.
> 
> It seems this isn't just needed for wakeup to work. On both sc7280 and
> sc8280xp the controller doesn't resume properly if the domain has been
> powered off (i.e. regardless of whether wakeup is enabled or not).
> 
Hi Johan,

   I believe you are referring to the reinit that happens in xhci resume 
path after wakeup happens:

[   48.675839] xhci-hcd xhci-hcd.14.auto: xHC error in resume, USBSTS 
0x411, Reinit

I see that when USB GDSC is not in retention, we don't retain controller 
state and go for reinit and re-enum of connected devices. We are seeing 
an additional delay of around ~0.7 sec (in chromebooks running on 
SC7280) in the wakeup path for re-enumeration of connected USB devices. 
To avoid this, we wanted to put GDSC in retention during PM suspend.

> Are you sure there's no state that needs to be retained regardless of
> the wakeup setting?
> 
>> Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
>> ---
>>   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..de29a034e725 100644
>> --- a/drivers/clk/qcom/gcc-sc7280.c
>> +++ b/drivers/clk/qcom/gcc-sc7280.c
>> @@ -3126,7 +3126,7 @@ static struct gdsc gcc_usb30_prim_gdsc = {
>>   	.pd = {
>>   		.name = "gcc_usb30_prim_gdsc",
>>   	},
>> -	.pwrsts = PWRSTS_OFF_ON,
>> +	.pwrsts = PWRSTS_RET_ON,
>>   	.flags = VOTABLE,
>>   };
> 
> And what about gcc_usb30_sec_gdsc?

Currently wakeup is not enabled on secondary controller as its not 
required for end product platform (herobrine variant). So leaving the 
usb30_sec_gdsc as it is for now.

Regards,
Krishna,

Re: [PATCH 3/3] clk: qcom: gcc-sc7280: Update the .pwrsts for usb gdsc

[Johan Hovold]

On Wed, Sep 14, 2022 at 02:07:06PM +0530, Krishna Kurapati PSSNV wrote:
> On 9/14/2022 12:39 PM, Johan Hovold wrote:
> > On Thu, Sep 01, 2022 at 03:47:56PM +0530, Rajendra Nayak wrote:
> >> USB on sc7280 cannot support wakeups from low power states
> >> if the GDSC is turned OFF. Update the .pwrsts for usb GDSC so it
> >> only transitions to RET in low power.
> > 
> > It seems this isn't just needed for wakeup to work. On both sc7280 and
> > sc8280xp the controller doesn't resume properly if the domain has been
> > powered off (i.e. regardless of whether wakeup is enabled or not).

>    I believe you are referring to the reinit that happens in xhci resume 
> path after wakeup happens:
> 
> [   48.675839] xhci-hcd xhci-hcd.14.auto: xHC error in resume, USBSTS 
> 0x411, Reinit

Right, and on sc8280xp the controller doesn't survive resume at all.
 
> I see that when USB GDSC is not in retention, we don't retain controller 
> state and go for reinit and re-enum of connected devices. We are seeing 
> an additional delay of around ~0.7 sec (in chromebooks running on 
> SC7280) in the wakeup path for re-enumeration of connected USB devices. 
> To avoid this, we wanted to put GDSC in retention during PM suspend.

Thanks for confirming. This needs to be described in the commit message
of this patches, which currently only says that this change is needed
for wakeup.

> > Are you sure there's no state that needs to be retained regardless of
> > the wakeup setting?
> > 
> >> Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
> >> ---
> >>   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..de29a034e725 100644
> >> --- a/drivers/clk/qcom/gcc-sc7280.c
> >> +++ b/drivers/clk/qcom/gcc-sc7280.c
> >> @@ -3126,7 +3126,7 @@ static struct gdsc gcc_usb30_prim_gdsc = {
> >>   	.pd = {
> >>   		.name = "gcc_usb30_prim_gdsc",
> >>   	},
> >> -	.pwrsts = PWRSTS_OFF_ON,
> >> +	.pwrsts = PWRSTS_RET_ON,
> >>   	.flags = VOTABLE,
> >>   };
> > 
> > And what about gcc_usb30_sec_gdsc?
> 
> Currently wakeup is not enabled on secondary controller as its not 
> required for end product platform (herobrine variant). So leaving the 
> usb30_sec_gdsc as it is for now.

This is generic code that can be used in different products and if this
is needed for the first controller it is also needed for the second
(i.e. even if wakeup isn't wired up or used on the products you are
working on).

Johan

Re: [PATCH 3/3] clk: qcom: gcc-sc7280: Update the .pwrsts for usb gdsc

[Rajendra Nayak]

On 9/14/2022 2:07 PM, Krishna Kurapati PSSNV wrote:
> 
> 
> On 9/14/2022 12:39 PM, Johan Hovold wrote:
>> On Thu, Sep 01, 2022 at 03:47:56PM +0530, Rajendra Nayak wrote:
>>> USB on sc7280 cannot support wakeups from low power states
>>> if the GDSC is turned OFF. Update the .pwrsts for usb GDSC so it
>>> only transitions to RET in low power.
>>
>> It seems this isn't just needed for wakeup to work. On both sc7280 and
>> sc8280xp the controller doesn't resume properly if the domain has been
>> powered off (i.e. regardless of whether wakeup is enabled or not).
>>
> Hi Johan,
> 
>    I believe you are referring to the reinit that happens in xhci resume path after wakeup happens:
> 
> [   48.675839] xhci-hcd xhci-hcd.14.auto: xHC error in resume, USBSTS 0x411, Reinit
> 
> I see that when USB GDSC is not in retention, we don't retain controller state and go for reinit and re-enum of connected devices. We are seeing an additional delay of around ~0.7 sec (in chromebooks running on SC7280) in the wakeup path for re-enumeration of connected USB devices. To avoid this, we wanted to put GDSC in retention during PM suspend.

ok, so perhaps the commit msg should be updated to something like

'USB on sc7280 needs to prevent the GDSC from being turned OFF for a couple of reasons
1. To prevent re-init and re-enumeration of all connected devices resulting in additional delay coming out of low power states
2. To support wakeups from connected devices from low power states'

> 
>> Are you sure there's no state that needs to be retained regardless of
>> the wakeup setting?
>>
>>> Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
>>> ---
>>>   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..de29a034e725 100644
>>> --- a/drivers/clk/qcom/gcc-sc7280.c
>>> +++ b/drivers/clk/qcom/gcc-sc7280.c
>>> @@ -3126,7 +3126,7 @@ static struct gdsc gcc_usb30_prim_gdsc = {
>>>       .pd = {
>>>           .name = "gcc_usb30_prim_gdsc",
>>>       },
>>> -    .pwrsts = PWRSTS_OFF_ON,
>>> +    .pwrsts = PWRSTS_RET_ON,
>>>       .flags = VOTABLE,
>>>   };
>>
>> And what about gcc_usb30_sec_gdsc?
> 
> Currently wakeup is not enabled on secondary controller as its not required for end product platform (herobrine variant). So leaving the usb30_sec_gdsc as it is for now.

It perhaps makes sense to update that as well, and given this is a compute specific chipset and we dont have to worry about
USB in device mode, its safe to assume if and when that controller is used (in future designs) it would only support host mode?

Re: [PATCH 3/3] clk: qcom: gcc-sc7280: Update the .pwrsts for usb gdsc

[Johan Hovold]

[ Please try to wrap your replies at 72 columns or so. ]

On Wed, Sep 14, 2022 at 02:28:31PM +0530, Rajendra Nayak wrote:
> 
> On 9/14/2022 2:07 PM, Krishna Kurapati PSSNV wrote:
> > 
> > 
> > On 9/14/2022 12:39 PM, Johan Hovold wrote:
> >> On Thu, Sep 01, 2022 at 03:47:56PM +0530, Rajendra Nayak wrote:
> >>> USB on sc7280 cannot support wakeups from low power states
> >>> if the GDSC is turned OFF. Update the .pwrsts for usb GDSC so it
> >>> only transitions to RET in low power.
> >>
> >> It seems this isn't just needed for wakeup to work. On both sc7280 and
> >> sc8280xp the controller doesn't resume properly if the domain has been
> >> powered off (i.e. regardless of whether wakeup is enabled or not).
> >>
> > Hi Johan,
> > 
> >    I believe you are referring to the reinit that happens in xhci resume path after wakeup happens:
> > 
> > [   48.675839] xhci-hcd xhci-hcd.14.auto: xHC error in resume, USBSTS 0x411, Reinit
> > 
> > I see that when USB GDSC is not in retention, we don't retain controller state and go for reinit and re-enum of connected devices. We are seeing an additional delay of around ~0.7 sec (in chromebooks running on SC7280) in the wakeup path for re-enumeration of connected USB devices. To avoid this, we wanted to put GDSC in retention during PM suspend.
> 
> ok, so perhaps the commit msg should be updated to something like
> 
> 'USB on sc7280 needs to prevent the GDSC from being turned OFF for a couple of reasons
> 1. To prevent re-init and re-enumeration of all connected devices resulting in additional delay coming out of low power states
> 2. To support wakeups from connected devices from low power states'

The fundamental issue here is that state is lost during suspend which
the driver doesn't currently (or can not) restore. Doesn't hurt to
mention the specific consequences you list above as well.

> >> Are you sure there's no state that needs to be retained regardless of
> >> the wakeup setting?
> >>
> >>> Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
> >>> ---
> >>>   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..de29a034e725 100644
> >>> --- a/drivers/clk/qcom/gcc-sc7280.c
> >>> +++ b/drivers/clk/qcom/gcc-sc7280.c
> >>> @@ -3126,7 +3126,7 @@ static struct gdsc gcc_usb30_prim_gdsc = {
> >>>       .pd = {
> >>>           .name = "gcc_usb30_prim_gdsc",
> >>>       },
> >>> -    .pwrsts = PWRSTS_OFF_ON,
> >>> +    .pwrsts = PWRSTS_RET_ON,
> >>>       .flags = VOTABLE,
> >>>   };
> >>
> >> And what about gcc_usb30_sec_gdsc?
> > 
> > Currently wakeup is not enabled on secondary controller as its not required for end product platform (herobrine variant). So leaving the usb30_sec_gdsc as it is for now.
> 
> It perhaps makes sense to update that as well, and given this is a compute specific chipset and we dont have to worry about
> USB in device mode, its safe to assume if and when that controller is used (in future designs) it would only support host mode?

What would be the problem when using the controller in peripheral mode?
Don't you want to retain the controller state when suspended also in
that case?

Johan

Re: [PATCH 3/3] clk: qcom: gcc-sc7280: Update the .pwrsts for usb gdsc

[Krishna Kurapati PSSNV]

On 9/14/2022 2:37 PM, Johan Hovold wrote:
> [ Please try to wrap your replies at 72 columns or so. ]
> 
> On Wed, Sep 14, 2022 at 02:28:31PM +0530, Rajendra Nayak wrote:
>>
>> On 9/14/2022 2:07 PM, Krishna Kurapati PSSNV wrote:
>>>
>>>
>>> On 9/14/2022 12:39 PM, Johan Hovold wrote:
>>>> On Thu, Sep 01, 2022 at 03:47:56PM +0530, Rajendra Nayak wrote:
>>>>> USB on sc7280 cannot support wakeups from low power states
>>>>> if the GDSC is turned OFF. Update the .pwrsts for usb GDSC so it
>>>>> only transitions to RET in low power.
>>>>
>>>> It seems this isn't just needed for wakeup to work. On both sc7280 and
>>>> sc8280xp the controller doesn't resume properly if the domain has been
>>>> powered off (i.e. regardless of whether wakeup is enabled or not).
>>>>
>>> Hi Johan,
>>>
>>>     I believe you are referring to the reinit that happens in xhci resume path after wakeup happens:
>>>
>>> [   48.675839] xhci-hcd xhci-hcd.14.auto: xHC error in resume, USBSTS 0x411, Reinit
>>>
>>> I see that when USB GDSC is not in retention, we don't retain controller state and go for reinit and re-enum of connected devices. We are seeing an additional delay of around ~0.7 sec (in chromebooks running on SC7280) in the wakeup path for re-enumeration of connected USB devices. To avoid this, we wanted to put GDSC in retention during PM suspend.
>>
>> ok, so perhaps the commit msg should be updated to something like
>>
>> 'USB on sc7280 needs to prevent the GDSC from being turned OFF for a couple of reasons
>> 1. To prevent re-init and re-enumeration of all connected devices resulting in additional delay coming out of low power states
>> 2. To support wakeups from connected devices from low power states'
> 
> The fundamental issue here is that state is lost during suspend which
> the driver doesn't currently (or can not) restore. Doesn't hurt to
> mention the specific consequences you list above as well.
> 
>>>> Are you sure there's no state that needs to be retained regardless of
>>>> the wakeup setting?
>>>>
>>>>> Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
>>>>> ---
>>>>>    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..de29a034e725 100644
>>>>> --- a/drivers/clk/qcom/gcc-sc7280.c
>>>>> +++ b/drivers/clk/qcom/gcc-sc7280.c
>>>>> @@ -3126,7 +3126,7 @@ static struct gdsc gcc_usb30_prim_gdsc = {
>>>>>        .pd = {
>>>>>            .name = "gcc_usb30_prim_gdsc",
>>>>>        },
>>>>> -    .pwrsts = PWRSTS_OFF_ON,
>>>>> +    .pwrsts = PWRSTS_RET_ON,
>>>>>        .flags = VOTABLE,
>>>>>    };
>>>>
>>>> And what about gcc_usb30_sec_gdsc?
>>>
>>> Currently wakeup is not enabled on secondary controller as its not required for end product platform (herobrine variant). So leaving the usb30_sec_gdsc as it is for now.
>>
>> It perhaps makes sense to update that as well, and given this is a compute specific chipset and we dont have to worry about
>> USB in device mode, its safe to assume if and when that controller is used (in future designs) it would only support host mode?
> 
> What would be the problem when using the controller in peripheral mode?
> Don't you want to retain the controller state when suspended also in
> that case?
> 
Hi Johan. You're right. When peripheral bus suspend lands in, we might 
need to keep GDSC ON in device mode as well.

Regards,
Krishna,

Re: [PATCH 3/3] clk: qcom: gcc-sc7280: Update the .pwrsts for usb gdsc

[Matthias Kaehlcke]

On Wed, Sep 14, 2022 at 09:12:36AM +0200, Johan Hovold wrote:
> On Thu, Sep 01, 2022 at 09:04:55AM -0700, Matthias Kaehlcke wrote:
> > On Thu, Sep 01, 2022 at 03:47:56PM +0530, Rajendra Nayak wrote:
> > > USB on sc7280 cannot support wakeups from low power states
> > > if the GDSC is turned OFF. Update the .pwrsts for usb GDSC so it
> > > only transitions to RET in low power.
> > > 
> > > Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
> > 
> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> > Tested-by: Matthias Kaehlcke <mka@chromium.org>
> 
> Did you confirm that you actually hit the retention state?

No, how would I do that?

> IIUC, this series is equivalent to using ALWAYS_ON unless CX is actually
> powered off during suspend.

The count in /sys/kernel/debug/qcom_stats/cxsd increses with each suspend,
however it also does that with the GDSC configured as ALWAYS_ON and with
Rajendra's "arm64: dts: qcom: sc7280: Add required-opps for USB" [1], so
I guess that isn't the correct signal.

https://patchwork.kernel.org/project/linux-arm-msm/patch/20220914053017.23617-1-quic_rjendra@quicinc.com/

Re: [PATCH 3/3] clk: qcom: gcc-sc7280: Update the .pwrsts for usb gdsc

[Rajendra Nayak]

On 9/15/2022 2:53 AM, Matthias Kaehlcke wrote:
> On Wed, Sep 14, 2022 at 09:12:36AM +0200, Johan Hovold wrote:
>> On Thu, Sep 01, 2022 at 09:04:55AM -0700, Matthias Kaehlcke wrote:
>>> On Thu, Sep 01, 2022 at 03:47:56PM +0530, Rajendra Nayak wrote:
>>>> USB on sc7280 cannot support wakeups from low power states
>>>> if the GDSC is turned OFF. Update the .pwrsts for usb GDSC so it
>>>> only transitions to RET in low power.
>>>>
>>>> Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
>>>
>>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>>> Tested-by: Matthias Kaehlcke <mka@chromium.org>
>>
>> Did you confirm that you actually hit the retention state?
> 
> No, how would I do that?
> 
>> IIUC, this series is equivalent to using ALWAYS_ON unless CX is actually
>> powered off during suspend.
> 
> The count in /sys/kernel/debug/qcom_stats/cxsd increses with each suspend,
> however it also does that with the GDSC configured as ALWAYS_ON and with
> Rajendra's "arm64: dts: qcom: sc7280: Add required-opps for USB" [1], so

hmm, that's really not expected. With my above patch and with the GDSC as
ALWAYS_ON there is a cx vote in suspend preventing cxsd so the counter
should not go up.

> I guess that isn't the correct signal.
> 
> https://patchwork.kernel.org/project/linux-arm-msm/patch/20220914053017.23617-1-quic_rjendra@quicinc.com/

Re: [PATCH 3/3] clk: qcom: gcc-sc7280: Update the .pwrsts for usb gdsc

[Rajendra Nayak]

On 9/15/2022 12:55 PM, Rajendra Nayak wrote:
> 
> 
> On 9/15/2022 2:53 AM, Matthias Kaehlcke wrote:
>> On Wed, Sep 14, 2022 at 09:12:36AM +0200, Johan Hovold wrote:
>>> On Thu, Sep 01, 2022 at 09:04:55AM -0700, Matthias Kaehlcke wrote:
>>>> On Thu, Sep 01, 2022 at 03:47:56PM +0530, Rajendra Nayak wrote:
>>>>> USB on sc7280 cannot support wakeups from low power states
>>>>> if the GDSC is turned OFF. Update the .pwrsts for usb GDSC so it
>>>>> only transitions to RET in low power.
>>>>>
>>>>> Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
>>>>
>>>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>>>> Tested-by: Matthias Kaehlcke <mka@chromium.org>
>>>
>>> Did you confirm that you actually hit the retention state?
>>
>> No, how would I do that?
>>
>>> IIUC, this series is equivalent to using ALWAYS_ON unless CX is actually
>>> powered off during suspend.
>>
>> The count in /sys/kernel/debug/qcom_stats/cxsd increses with each suspend,
>> however it also does that with the GDSC configured as ALWAYS_ON and with
>> Rajendra's "arm64: dts: qcom: sc7280: Add required-opps for USB" [1], so
> 
> hmm, that's really not expected. With my above patch and with the GDSC as
> ALWAYS_ON there is a cx vote in suspend preventing cxsd so the counter
> should not go up.

Perhaps you are missing '1b771839: clk: qcom: gdsc: enable optional power domain support'
in your tree?

> 
>> I guess that isn't the correct signal.
>>
>> https://patchwork.kernel.org/project/linux-arm-msm/patch/20220914053017.23617-1-quic_rjendra@quicinc.com/

Re: [PATCH 3/3] clk: qcom: gcc-sc7280: Update the .pwrsts for usb gdsc

[Matthias Kaehlcke]

On Thu, Sep 15, 2022 at 06:59:36PM +0530, Rajendra Nayak wrote:
> 
> On 9/15/2022 12:55 PM, Rajendra Nayak wrote:
> > 
> > 
> > On 9/15/2022 2:53 AM, Matthias Kaehlcke wrote:
> > > On Wed, Sep 14, 2022 at 09:12:36AM +0200, Johan Hovold wrote:
> > > > On Thu, Sep 01, 2022 at 09:04:55AM -0700, Matthias Kaehlcke wrote:
> > > > > On Thu, Sep 01, 2022 at 03:47:56PM +0530, Rajendra Nayak wrote:
> > > > > > USB on sc7280 cannot support wakeups from low power states
> > > > > > if the GDSC is turned OFF. Update the .pwrsts for usb GDSC so it
> > > > > > only transitions to RET in low power.
> > > > > > 
> > > > > > Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
> > > > > 
> > > > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> > > > > Tested-by: Matthias Kaehlcke <mka@chromium.org>
> > > > 
> > > > Did you confirm that you actually hit the retention state?
> > > 
> > > No, how would I do that?
> > > 
> > > > IIUC, this series is equivalent to using ALWAYS_ON unless CX is actually
> > > > powered off during suspend.
> > > 
> > > The count in /sys/kernel/debug/qcom_stats/cxsd increses with each suspend,
> > > however it also does that with the GDSC configured as ALWAYS_ON and with
> > > Rajendra's "arm64: dts: qcom: sc7280: Add required-opps for USB" [1], so
> > 
> > hmm, that's really not expected. With my above patch and with the GDSC as
> > ALWAYS_ON there is a cx vote in suspend preventing cxsd so the counter
> > should not go up.
> 
> Perhaps you are missing '1b771839: clk: qcom: gdsc: enable optional power domain support'
> in your tree?

Indeed, that patch was missing in our tree, thanks for the pointer!

With it the CXSD count does not increase when the GDSC is configured as
ALWAYS_ON.