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

[Rajendra Nayak <quic_rjendra@quicinc.com>]

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.