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

[Rajendra Nayak <quic_rjendra@quicinc.com>]

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