Messages in this thread |  | | Date | Mon, 23 Jan 2023 10:00:55 +0530 | Subject | Re: [PATCH v3 1/3] clk: qcom: gdsc: Fix the handling of PWRSTS_RET support | From | Rajendra Nayak <> |
| |
On 1/22/2023 5:45 AM, Luca Weiss wrote: > Hi Rajendra, > > On Dienstag, 20. September 2022 13:15:15 CET Rajendra Nayak wrote: >> 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. > > Unfortunately indeed this patch makes problems on msm8974, at least on > fairphone-fp2 hardware. > > With this patch in place, the screen doesn't initialize correctly in maybe 80% > of boots and is stuck in weird states, mostly just becomes completely blue. > > Kernel log at least sometimes includes messages like this: > [ 25.847541] dsi_cmds2buf_tx: cmd dma tx failed, type=0x39, data0=0x51, > len=8, ret=-110 > > Do you have anything I can try on msm8974? For now, reverting this patch makes > display work again on v6.1
hmm, I was really expecting this to leak more power than break anything functionally, Did you try moving to PWRSTS_OFF_ON instead of PWRSTS_RET_ON for mdss_gdsc?
> > Regards > Luca > >> >> 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. >> >> drivers/clk/qcom/gdsc.c | 10 ++++++++++ >> drivers/clk/qcom/gdsc.h | 5 +++++ >> 2 files changed, 15 insertions(+) >> >> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c >> index d3244006c661..ccf63771e852 100644 >> --- a/drivers/clk/qcom/gdsc.c >> +++ b/drivers/clk/qcom/gdsc.c >> @@ -368,6 +368,16 @@ static int _gdsc_disable(struct gdsc *sc) >> if (sc->pwrsts & PWRSTS_OFF) >> gdsc_clear_mem_on(sc); >> >> + /* >> + * If the GDSC supports only a Retention state, apart from ON, >> + * leave it in ON state. >> + * There is no SW control to transition the GDSC into >> + * Retention state. This happens in HW when the parent >> + * domain goes down to a Low power state >> + */ >> + if (sc->pwrsts == PWRSTS_RET_ON) >> + return 0; >> + >> ret = gdsc_toggle_logic(sc, GDSC_OFF); >> if (ret) >> return ret; >> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h >> index 5de48c9439b2..981a12c8502d 100644 >> --- a/drivers/clk/qcom/gdsc.h >> +++ b/drivers/clk/qcom/gdsc.h >> @@ -49,6 +49,11 @@ struct gdsc { >> const u8 pwrsts; >> /* Powerdomain allowable state bitfields */ >> #define PWRSTS_OFF BIT(0) >> +/* >> + * There is no SW control to transition a GDSC into >> + * PWRSTS_RET. This happens in HW when the parent >> + * domain goes down to a low power state >> + */ >> #define PWRSTS_RET BIT(1) >> #define PWRSTS_ON BIT(2) >> #define PWRSTS_OFF_ON (PWRSTS_OFF | PWRSTS_ON) > > > >
|  |