Messages in this thread | | | Date | Wed, 21 Sep 2022 11:18:10 +0200 | Subject | Re: [PATCH v3 1/3] clk: qcom: gdsc: Fix the handling of PWRSTS_RET support | From | AngeloGioacchino Del Regno <> |
| |
Il 21/09/22 11:05, Rajendra Nayak ha scritto: > > On 9/21/2022 1:21 PM, AngeloGioacchino Del Regno wrote: >> Il 20/09/22 15:39, Rajendra Nayak ha scritto: >>> >>> On 9/20/2022 6:09 PM, AngeloGioacchino Del Regno wrote: >>>> Il 20/09/22 13:15, Rajendra Nayak ha scritto: >>>>> 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 power collapse/power off state. >>>>> >>>>> On some platforms where the parent domains lowest power state >>>>> itself is Retention, just leaving the GDSC in ON (without any >>>>> RETAIN_MEM/RETAIN_PERIPH bits being set) will also transition >>>>> it to Retention. >>>>> >>>>> The existing logic handling the PWRSTS_RET seems to set the >>>>> RETAIN_MEM/RETAIN_PERIPH bits if the cxcs offsets are specified >>>>> 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@collabora.com> >>>>> --- >>>>> v3: >>>>> Updated changelog >>>>> >>>>> 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. >>>> >>>> I think that the safest option is to add a PWRSTS_RET_HW_CTRL flag (or similar), >>>> used for the specific cases of SC7180 and SC7280 (and possibly others) where the >>>> GDSC is automatically transitioned to a Retention state by HW control, with no >>>> required software (kernel driver) intervention. >>> >>> Having a PWRSTS_RET_HW_CTRL flag would make sense if there was also a >>> PWRSTS_RET_SW_CTRL way of achieving Retention state, but FWIK there isn't. >>> I am sure that's the way it is on 8974 as well, I just don't have hardware to >>> confirm. >>> >>>> >>>>> >>>>> 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. >>>> >>>> static int gdsc_init(struct gdsc *sc) >>>> { >>>> >>>> ... >>>> >>>> if (on || (sc->pwrsts & PWRSTS_RET)) >>>> gdsc_force_mem_on(sc); >>>> else >>>> gdsc_clear_mem_on(sc); >>>> >>>> ... >>>> } >>>> >>>> On MSM8998 and SDM630/636/660, we're reaching that point with a GDSC that is >>>> left OFF from the bootloader, but we want (at least for 630/660) memretain >>>> without periph-retain: this is required to make the hypervisor happy. >>> >>> Ideally setting the memretain bits while the GDSC is OFF should have no affect >>> at all. Is this for the gpu_gx_gdsc on 630/660? Is this needed only at the init >>> time (when the bootloader has left it OFF) or is it needed everytime the kernel >>> turns it OFF too? >> >> Even though I don't remember the flow in a clear way (this entire thing was done >> years ago), I'm sure that for PWRSTS_OFF memretain can be cleared, so, the current >> flow that we have in gdsc.c does work correctly. >> >> Ideally, I agree with you that the memretain bits should have no effect at all >> while the GDSC is OFF, but that's the situation on these platforms. > > Would you be able to test this patch on these platforms to see if we end up > with regressions? >
Not in a timely manner.
Konrad, Marijn, Jami, can any of you perform a "fast" test?
Thanks.
>> >>> How did we come up with this trick to keep the hypervisor happy, was it picked >>> up from some downstream reference code? >> >> Yes, it was found in various releases of the downstream kernel for 8998/630/660. >> >>> >>>> >>>> Regards, >>>> Angelo >>>> >>
| |