Message-ID: <20220927170516.zrkzn3xl7oedzi4l@builder.lan>
Date:   Tue, 27 Sep 2022 12:05:16 -0500
From:   Bjorn Andersson <andersson@...nel.org>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
Cc:     Rajendra Nayak <quic_rjendra@...cinc.com>, agross@...nel.org,
        konrad.dybcio@...ainline.org, mturquette@...libre.com,
        sboyd@...nel.org, mka@...omium.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, johan+linaro@...nel.org,
        quic_kriskura@...cinc.com, dianders@...omium.org,
        linux-clk@...r.kernel.org
Subject: Re: [PATCH v3 1/3] clk: qcom: gdsc: Fix the handling of PWRSTS_RET
 support

On Tue, Sep 27, 2022 at 01:57:59PM +0200, AngeloGioacchino Del Regno wrote:
> Il 27/09/22 05:02, Bjorn Andersson ha scritto:
> > 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@...cinc.com>
> > > > Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.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?
> > 
> 
> Hello Bjorn,
> 
> my replies were related to this part of the commit description:
> 
> >>> The ideal fix for them would be to just update their resp.
> >>> .pwrsts to PWRSTS_OFF_ON only.
> 
> By updating MSM8998 and SDM660's gpu_gx_gdsc to remove PWRSTS_RET, the gdsc_init()
> flow will change, as in the aforementioned branch, `on` will be false, hence,
> we will clear RETAIN_MEM during the gpu_gx_gdsc initialization, producing side
> effects.
> I agree on the fact that PWRSTS_RET was *not* handled correctly before this commit
> and this alone will not produce any side effects on MSM8998, nor SDM660.
> 
> So yes, this is a discussion about the validity of fixing the pwrsts in
> gpucc-msm8998 and in gpucc-sdm660.c.
> 

Okay, now I understand the context, I will move ahead and merge these
patches then.


And for 8998/660 you're saying that the GDSC is found to be OFF at boot
and in runtime you're going to bounce it between on and off in software,
but you need RETAIN_MEM set?

If that's the case this GDSC can be in all 3 states. But as you can
find in the discussions that lead up to this discussion, we don't have a
way to represent this to the clients (today).

Regards,
Bjorn

Powered by blists - more mailing lists