diff mbox series

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

Message ID 20220916102417.24549-2-quic_rjendra@quicinc.com (mailing list archive)
State Superseded
Headers show
Series [v2,1/3] clk: qcom: gdsc: Fix the handling of PWRSTS_RET support | expand

Commit Message

Rajendra Nayak Sept. 16, 2022, 10:24 a.m. UTC 
The USB controller on sc7180 does not retain the state when
the system goes into low power state and the GDSC is
turned off. This results in the controller reinitializing and
re-enumerating all the connected devices (resulting in additional
delay while coming out of suspend)
Fix this by updating the .pwrsts for the USB GDSC so it only
transitions to retention state 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>
---
v2:
Updated the changelog

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

Comments

Bjorn Andersson Sept. 16, 2022, 7:06 p.m. UTC | #1 
On Fri, Sep 16, 2022 at 03:54:16PM +0530, Rajendra Nayak wrote:
> The USB controller on sc7180 does not retain the state when
> the system goes into low power state and the GDSC is
> turned off. This results in the controller reinitializing and
> re-enumerating all the connected devices (resulting in additional
> delay while coming out of suspend)
> Fix this by updating the .pwrsts for the USB GDSC so it only
> transitions to retention state 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>
> ---
> v2:
> Updated the changelog
> 
>  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,

I presume the same should be applied to gcc_usb30_sec_gdsc?

Regards,
Bjorn

>  };
>  
>  static struct gdsc hlos1_vote_mmnoc_mmu_tbu_hf0_gdsc = {
> -- 
> 2.17.1
>
Bjorn Andersson Sept. 16, 2022, 7:41 p.m. UTC | #2 
On Fri, Sep 16, 2022 at 02:06:58PM -0500, Bjorn Andersson wrote:
> On Fri, Sep 16, 2022 at 03:54:16PM +0530, Rajendra Nayak wrote:
> > The USB controller on sc7180 does not retain the state when
> > the system goes into low power state and the GDSC is
> > turned off. This results in the controller reinitializing and
> > re-enumerating all the connected devices (resulting in additional
> > delay while coming out of suspend)
> > Fix this by updating the .pwrsts for the USB GDSC so it only
> > transitions to retention state 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>
> > ---
> > v2:
> > Updated the changelog
> > 
> >  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,
> 
> I presume the same should be applied to gcc_usb30_sec_gdsc?
> 

Please ignore that, missed the '1' in the filename.

Reviewed-by: Bjorn Andersson <andersson@kernel.org>

Regards,
Bjorn
Johan Hovold Sept. 19, 2022, 3:45 p.m. UTC | #3 
On Fri, Sep 16, 2022 at 03:54:16PM +0530, Rajendra Nayak wrote:
> The USB controller on sc7180 does not retain the state when
> the system goes into low power state and the GDSC is
> turned off. This results in the controller reinitializing and
> re-enumerating all the connected devices (resulting in additional
> delay while coming out of suspend)
> Fix this by updating the .pwrsts for the USB GDSC so it only
> transitions to retention state 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>
> ---
> v2:
> Updated the changelog
> 
>  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 = {

It seems like the above will not work unless you also provide the
registers offsets that gdsc_force_mem_on() expects.

Specifically, unless you set cxc_count for the GDSC, the above call is a
no-op and the retention setting (i.e. the RETAIN_MEM and RETAIN_PERIPH
bits) will not be updated when registering the GDSC.

Johan
Rajendra Nayak Sept. 20, 2022, 3:36 a.m. UTC | #4 
On 9/19/2022 9:15 PM, Johan Hovold wrote:
> On Fri, Sep 16, 2022 at 03:54:16PM +0530, Rajendra Nayak wrote:
>> The USB controller on sc7180 does not retain the state when
>> the system goes into low power state and the GDSC is
>> turned off. This results in the controller reinitializing and
>> re-enumerating all the connected devices (resulting in additional
>> delay while coming out of suspend)
>> Fix this by updating the .pwrsts for the USB GDSC so it only
>> transitions to retention state 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>
>> ---
>> v2:
>> Updated the changelog
>>
>>   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 = {
> 
> It seems like the above will not work unless you also provide the
> registers offsets that gdsc_force_mem_on() expects.

That's true, but its needed only on platforms that support complete
CX domain power collapse. sc7280 (and sc7180) does not support
that and hence we can achieve GDSC RET without any of the RETAIN_MEM/
RETAIN_PERIPH bits programmed.
I explained some of that in detail on another related thread a
while back [1]

[1] https://lore.kernel.org/all/5ff21b1e-3af9-36ef-e13e-fa33f526d0e3@quicinc.com/

> 
> Specifically, unless you set cxc_count for the GDSC, the above call is a
> no-op and the retention setting (i.e. the RETAIN_MEM and RETAIN_PERIPH
> bits) will not be updated when registering the GDSC.
> 
> Johan
Johan Hovold Sept. 20, 2022, 5:51 a.m. UTC | #5 
On Tue, Sep 20, 2022 at 09:06:22AM +0530, Rajendra Nayak wrote:
> 
> 
> On 9/19/2022 9:15 PM, Johan Hovold wrote:
> > On Fri, Sep 16, 2022 at 03:54:16PM +0530, Rajendra Nayak wrote:
> >> The USB controller on sc7180 does not retain the state when
> >> the system goes into low power state and the GDSC is
> >> turned off. This results in the controller reinitializing and
> >> re-enumerating all the connected devices (resulting in additional
> >> delay while coming out of suspend)
> >> Fix this by updating the .pwrsts for the USB GDSC so it only
> >> transitions to retention state 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>
> >> ---
> >> v2:
> >> Updated the changelog
> >>
> >>   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 = {
> > 
> > It seems like the above will not work unless you also provide the
> > registers offsets that gdsc_force_mem_on() expects.
> 
> That's true, but its needed only on platforms that support complete
> CX domain power collapse. sc7280 (and sc7180) does not support
> that and hence we can achieve GDSC RET without any of the RETAIN_MEM/
> RETAIN_PERIPH bits programmed.
> I explained some of that in detail on another related thread a
> while back [1]
> 
> [1] https://lore.kernel.org/all/5ff21b1e-3af9-36ef-e13e-fa33f526d0e3@quicinc.com/

Thanks for the link, that was was very informative. 

Could you please update the commit message of patch 1/3 so that it also
covers these systems and explains why you don't set the RETAIN_MEM and
RETAIN_PERIPH bits for them?

As that commit message stands now, it seems that those bits must be set
for retention to work.

> > Specifically, unless you set cxc_count for the GDSC, the above call is a
> > no-op and the retention setting (i.e. the RETAIN_MEM and RETAIN_PERIPH
> > bits) will not be updated when registering the GDSC.

Johan
Rajendra Nayak Sept. 20, 2022, 6:01 a.m. UTC | #6 
On 9/20/2022 11:21 AM, Johan Hovold wrote:
> On Tue, Sep 20, 2022 at 09:06:22AM +0530, Rajendra Nayak wrote:
>>
>>
>> On 9/19/2022 9:15 PM, Johan Hovold wrote:
>>> On Fri, Sep 16, 2022 at 03:54:16PM +0530, Rajendra Nayak wrote:
>>>> The USB controller on sc7180 does not retain the state when
>>>> the system goes into low power state and the GDSC is
>>>> turned off. This results in the controller reinitializing and
>>>> re-enumerating all the connected devices (resulting in additional
>>>> delay while coming out of suspend)
>>>> Fix this by updating the .pwrsts for the USB GDSC so it only
>>>> transitions to retention state 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>
>>>> ---
>>>> v2:
>>>> Updated the changelog
>>>>
>>>>    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 = {
>>>
>>> It seems like the above will not work unless you also provide the
>>> registers offsets that gdsc_force_mem_on() expects.
>>
>> That's true, but its needed only on platforms that support complete
>> CX domain power collapse. sc7280 (and sc7180) does not support
>> that and hence we can achieve GDSC RET without any of the RETAIN_MEM/
>> RETAIN_PERIPH bits programmed.
>> I explained some of that in detail on another related thread a
>> while back [1]
>>
>> [1] https://lore.kernel.org/all/5ff21b1e-3af9-36ef-e13e-fa33f526d0e3@quicinc.com/
> 
> Thanks for the link, that was was very informative.
> 
> Could you please update the commit message of patch 1/3 so that it also
> covers these systems and explains why you don't set the RETAIN_MEM and
> RETAIN_PERIPH bits for them?
> 
> As that commit message stands now, it seems that those bits must be set
> for retention to work.

Agree, I will update the commit message and re-spin, thanks.

> 
>>> Specifically, unless you set cxc_count for the GDSC, the above call is a
>>> no-op and the retention setting (i.e. the RETAIN_MEM and RETAIN_PERIPH
>>> bits) will not be updated when registering the GDSC.
> 
> Johan
diff mbox series

Patch

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