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

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

[Rajendra Nayak]

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.

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)
-- 
2.17.1

[PATCH v3 2/3] clk: qcom: gcc-sc7180: Update the .pwrsts for usb gdsc

[Rajendra Nayak]

The USB controller on sc7180 does not retain the state when
the system goes into low power state and the GDSC is
turned off. This results in the controller reinitializing and
re-enumerating all the connected devices (resulting in additional
delay while coming out of suspend)
Fix this by updating the .pwrsts for the USB GDSC so it only
transitions to retention state in low power.
Since sc7180 only supports cx (parent of usb gdsc) Retention, there
are no cxcs offsets mentioned in order to support the Retention
state.

Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
---
v3:
Updated the changelog

 drivers/clk/qcom/gcc-sc7180.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/gcc-sc7180.c b/drivers/clk/qcom/gcc-sc7180.c
index c2ea09945c47..2d3980251e78 100644
--- a/drivers/clk/qcom/gcc-sc7180.c
+++ b/drivers/clk/qcom/gcc-sc7180.c
@@ -2224,7 +2224,7 @@ static struct gdsc usb30_prim_gdsc = {
 	.pd = {
 		.name = "usb30_prim_gdsc",
 	},
-	.pwrsts = PWRSTS_OFF_ON,
+	.pwrsts = PWRSTS_RET_ON,
 };
 
 static struct gdsc hlos1_vote_mmnoc_mmu_tbu_hf0_gdsc = {
-- 
2.17.1

[PATCH v3 3/3] clk: qcom: gcc-sc7280: Update the .pwrsts for usb gdscs

[Rajendra Nayak]

The USB controllers on sc7280 do not retain the state when
the system goes into low power state and the GDSCs are
turned off. This results in the controllers reinitializing and
re-enumerating all the connected devices (resulting in additional
delay while coming out of suspend)
Fix this by updating the .pwrsts for the USB GDSCs so they only
transition to retention state in low power.
Since sc7280 only supports cx (parent of usb gdscs) Retention, there
are no cxcs offsets mentioned in order to support the Retention
state.

Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Tested-by: Matthias Kaehlcke <mka@chromium.org>
---
v3:
Updated the changelog

 drivers/clk/qcom/gcc-sc7280.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/gcc-sc7280.c b/drivers/clk/qcom/gcc-sc7280.c
index 7ff64d4d5920..7b6e5a86c11f 100644
--- a/drivers/clk/qcom/gcc-sc7280.c
+++ b/drivers/clk/qcom/gcc-sc7280.c
@@ -3126,7 +3126,7 @@ static struct gdsc gcc_usb30_prim_gdsc = {
 	.pd = {
 		.name = "gcc_usb30_prim_gdsc",
 	},
-	.pwrsts = PWRSTS_OFF_ON,
+	.pwrsts = PWRSTS_RET_ON,
 	.flags = VOTABLE,
 };
 
@@ -3135,7 +3135,7 @@ static struct gdsc gcc_usb30_sec_gdsc = {
 	.pd = {
 		.name = "gcc_usb30_sec_gdsc",
 	},
-	.pwrsts = PWRSTS_OFF_ON,
+	.pwrsts = PWRSTS_RET_ON,
 	.flags = VOTABLE,
 };
 
-- 
2.17.1

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

[AngeloGioacchino Del Regno]

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.

Regards,
Angelo

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

[Rajendra Nayak]

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@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.

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?
How did we come up with this trick to keep the hypervisor happy, was it picked
up from some downstream reference code?

> 
> Regards,
> Angelo
> 

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

[AngeloGioacchino Del Regno]

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@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.
> 
> 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.

> 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
>>

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

[Rajendra Nayak]

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@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.
>>
>> 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?

> 
>> 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
>>>
> 

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

[AngeloGioacchino Del Regno]

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@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.
>>>
>>> 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
>>>>
>>

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

[Bjorn Andersson]

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

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

[AngeloGioacchino Del Regno]

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@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?
> 

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.

Cheers,
Angelo

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

[Bjorn Andersson]

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@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?
> > 
> 
> 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

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

[Bjorn Andersson]

On Tue, 20 Sep 2022 16:45:15 +0530, 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.
> 
> [...]

Applied, thanks!

[1/3] clk: qcom: gdsc: Fix the handling of PWRSTS_RET support
      commit: d399723950c45cd9507aef848771826afc3f69b0
[2/3] clk: qcom: gcc-sc7180: Update the .pwrsts for usb gdsc
      commit: d9fe9f3fefe74d15e280fce628bff1b6fc6d9675
[3/3] clk: qcom: gcc-sc7280: Update the .pwrsts for usb gdscs
      commit: e3ae3e899aa0322ff685fd7cf1322c6670da7db7

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>

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

[AngeloGioacchino Del Regno]

Il 27/09/22 19:05, Bjorn Andersson ha scritto:
> 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@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?
>>>
>>
>> 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?
> 

Correct.

> 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).
> 

Yes I know and agree.

Regards,
Angelo

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

[Luca Weiss]

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

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)

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

[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)
> 
> 
> 
> 

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

[Luca Weiss]

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@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?

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.

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)

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

[Luca Weiss]

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@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?
> 
> 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.

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)

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

[Rajendra Nayak]

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@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?
>>
>> 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)
> 
> 
> 
> 

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

[Manivannan Sadhasivam]

On Tue, Apr 11, 2023 at 10:20:47AM +0530, Rajendra Nayak wrote:
> 
> 
> 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@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?
> > > 
> > > 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.

I fail to understand how enabling retention state affects a peripheral during
boot. It could've some effect during suspend but an issue during boot fuzzies
me.

- Mani

> 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)
> > 
> > 
> > 
> > 

-- 
மணிவண்ணன் சதாசிவம்