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@xxxxxxxxxxx>
Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
---
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
