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

[Bjorn Andersson <andersson@kernel.org>]

On Tue, Sep 20, 2022 at 02:39:21PM +0200, 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.
> 
> > 
> > 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.
> 

Forgive me Angelo, but can you please help me understand your concern
here?

Are yous saying that the valid states for 8998/660 are PWRSTS_OFF_ON,
but you also want gdsc_force_mem_on() - with NO_RET_PERIPH?


It seems to me that as Rajendra's patch is written, the gpu_gx_gdsc
won't be affected, because pwrsts != PWRSTS_RET. So this is a question
about the validity of fixing the pwrsts in gpucc-msm8998, rather than
about this patch in itself?

Regards,
Bjorn