Date:   Tue, 11 Apr 2023 10:20:47 +0530
From:   Rajendra Nayak <quic_rjendra@...cinc.com>
To:     Luca Weiss <luca@...tu.xyz>, <agross@...nel.org>,
        <andersson@...nel.org>, <konrad.dybcio@...ainline.org>,
        <mturquette@...libre.com>, <sboyd@...nel.org>, <mka@...omium.org>
CC:     <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>,
        <angelogioacchino.delregno@...labora.com>,
        <~postmarketos/upstreaming@...ts.sr.ht>
Subject: Re: [PATCH v3 1/3] clk: qcom: gdsc: Fix the handling of PWRSTS_RET
 support



On 4/11/2023 1:05 AM, Luca Weiss wrote:
> Hi Rajendra,
> 
> On Mittwoch, 1. Februar 2023 19:04:37 CEST Luca Weiss wrote:
>> On Montag, 23. Jänner 2023 05:30:55 CET Rajendra Nayak wrote:
>>> 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@...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.
>>>>
>>>> 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?
>>
>> Hi Rajendra,
>>
>> yes with this change the display init works fine again. Do you think this is
>> the intended solution then? I also haven't tested really more than this
>> simple case.
>>
>> Let me know what you think.
> 
> Any feedback on this? Would be great to get this fixed sometime soon, quite
> annoying to carry a patch for this locally.

Hi Luca, really sorry I seem to have completely missed your previous
email. Yes, moving the gdsc from PWRSTS_RET_ON to PWRSTS_OFF_ON seems to
be the right thing to do. The behavior of the RET state was same as that
of OFF prior to my patch, so the change should ideally make display go
back to having the same behavior as before.
I can certainly ack the change if you send in a patch.
thanks,
Rajendra

> 
> Regards
> Luca
> 
>>
>> Regards
>> Luca
>>
>> diff --git a/drivers/clk/qcom/mmcc-msm8974.c
>> b/drivers/clk/qcom/mmcc-msm8974.c index 26f3f8f06edf..f95e38abde13 100644
>> --- a/drivers/clk/qcom/mmcc-msm8974.c
>> +++ b/drivers/clk/qcom/mmcc-msm8974.c
>> @@ -2389,7 +2389,7 @@ static struct gdsc mdss_gdsc = {
>>   	.pd = {
>>   		.name = "mdss",
>>   	},
>> -	.pwrsts = PWRSTS_RET_ON,
>> +	.pwrsts = PWRSTS_OFF_ON,
>>   };
>>
>>   static struct gdsc camss_jpeg_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)
> 
> 
> 
> 

Powered by blists - more mailing lists