diff mbox series

clk: qcom: smd-rpm: conditionally enable scaling before doing handover

Message ID 20230506-rpmcc-scaling-handover-v1-1-374338a8dfd9@z3ntu.xyz
State New
Headers
Series clk: qcom: smd-rpm: conditionally enable scaling before doing handover |

Commit Message

Luca Weiss May 6, 2023, 8:10 p.m. UTC 
  On older platforms like msm8226, msm8974 and msm8916 the driver in the
downstream kernel enables scaling first before doing the handover of the
clocks.

While this normally doesn't seem to cause noticeable problems, on
apq8026-asus-sparrow this causes the device to immediately reboot,
perhaps due to older rpm firmware that becomes unhappy.

On newer platforms the order has swapped and enabling scaling is done
after the handover, so let's introduce this behavior only conditionally
for msm8226 and msm8974 for now.

Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
 drivers/clk/qcom/clk-smd-rpm.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)


---
base-commit: dd9e11d6477a52ede9ebe575c83285e79e823889
change-id: 20230506-rpmcc-scaling-handover-a63029ed9d13

Best regards,

Comments

Konrad Dybcio May 16, 2023, 1:27 a.m. UTC | #1 
On 6.05.2023 22:10, Luca Weiss wrote:
> On older platforms like msm8226, msm8974 and msm8916 the driver in the
> downstream kernel enables scaling first before doing the handover of the
> clocks.
> 
> While this normally doesn't seem to cause noticeable problems, on
> apq8026-asus-sparrow this causes the device to immediately reboot,
> perhaps due to older rpm firmware that becomes unhappy.
> 
> On newer platforms the order has swapped and enabling scaling is done
> after the handover, so let's introduce this behavior only conditionally
> for msm8226 and msm8974 for now.
> 
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
Did you give this a spin on some 8974? I think hammerhead had
issues around rpmcc in the past..

Konrad
>  drivers/clk/qcom/clk-smd-rpm.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
> index 887b945a6fb7..6d5476afc4d1 100644
> --- a/drivers/clk/qcom/clk-smd-rpm.c
> +++ b/drivers/clk/qcom/clk-smd-rpm.c
> @@ -178,6 +178,7 @@ struct clk_smd_rpm_req {
>  struct rpm_smd_clk_desc {
>  	struct clk_smd_rpm **clks;
>  	size_t num_clks;
> +	bool scaling_before_handover;
>  };
>  
>  static DEFINE_MUTEX(rpm_smd_clk_lock);
> @@ -693,6 +694,7 @@ static struct clk_smd_rpm *msm8974_clks[] = {
>  static const struct rpm_smd_clk_desc rpm_clk_msm8974 = {
>  	.clks = msm8974_clks,
>  	.num_clks = ARRAY_SIZE(msm8974_clks),
> +	.scaling_before_handover = true,
>  };
>  
>  static struct clk_smd_rpm *msm8976_clks[] = {
> @@ -1318,6 +1320,12 @@ static int rpm_smd_clk_probe(struct platform_device *pdev)
>  	rpm_smd_clks = desc->clks;
>  	num_clks = desc->num_clks;
>  
> +	if (desc->scaling_before_handover) {
> +		ret = clk_smd_rpm_enable_scaling(rpm);
> +		if (ret)
> +			goto err;
> +	}
> +
>  	for (i = 0; i < num_clks; i++) {
>  		if (!rpm_smd_clks[i])
>  			continue;
> @@ -1329,9 +1337,11 @@ static int rpm_smd_clk_probe(struct platform_device *pdev)
>  			goto err;
>  	}
>  
> -	ret = clk_smd_rpm_enable_scaling(rpm);
> -	if (ret)
> -		goto err;
> +	if (!desc->scaling_before_handover) {
> +		ret = clk_smd_rpm_enable_scaling(rpm);
> +		if (ret)
> +			goto err;
> +	}
>  
>  	for (i = 0; i < num_clks; i++) {
>  		if (!rpm_smd_clks[i])
> 
> ---
> base-commit: dd9e11d6477a52ede9ebe575c83285e79e823889
> change-id: 20230506-rpmcc-scaling-handover-a63029ed9d13
> 
> Best regards,
Luca Weiss May 17, 2023, 2:59 p.m. UTC | #2 
On Dienstag, 16. Mai 2023 03:27:46 CEST Konrad Dybcio wrote:
> On 6.05.2023 22:10, Luca Weiss wrote:
> > On older platforms like msm8226, msm8974 and msm8916 the driver in the
> > downstream kernel enables scaling first before doing the handover of the
> > clocks.
> > 
> > While this normally doesn't seem to cause noticeable problems, on
> > apq8026-asus-sparrow this causes the device to immediately reboot,
> > perhaps due to older rpm firmware that becomes unhappy.
> > 
> > On newer platforms the order has swapped and enabling scaling is done
> > after the handover, so let's introduce this behavior only conditionally
> > for msm8226 and msm8974 for now.
> > 
> > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> > ---
> 
> Did you give this a spin on some 8974? I think hammerhead had
> issues around rpmcc in the past..

Yes, appears to be fine on msm8974 also.

I tried to reproduce the hammerhead ocmem hang we had in the past but even 
with v6.3 rpmcc it seems to be fine. But iirc it was happening more or less at 
random in the past so wouldn't be surprised if my tests just didn't show 
anything on accident.

Regards
Luca

> 
> Konrad
> 
> >  drivers/clk/qcom/clk-smd-rpm.c | 16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/clk/qcom/clk-smd-rpm.c
> > b/drivers/clk/qcom/clk-smd-rpm.c index 887b945a6fb7..6d5476afc4d1 100644
> > --- a/drivers/clk/qcom/clk-smd-rpm.c
> > +++ b/drivers/clk/qcom/clk-smd-rpm.c
> > @@ -178,6 +178,7 @@ struct clk_smd_rpm_req {
> > 
> >  struct rpm_smd_clk_desc {
> >  
> >  	struct clk_smd_rpm **clks;
> >  	size_t num_clks;
> > 
> > +	bool scaling_before_handover;
> > 
> >  };
> >  
> >  static DEFINE_MUTEX(rpm_smd_clk_lock);
> > 
> > @@ -693,6 +694,7 @@ static struct clk_smd_rpm *msm8974_clks[] = {
> > 
> >  static const struct rpm_smd_clk_desc rpm_clk_msm8974 = {
> >  
> >  	.clks = msm8974_clks,
> >  	.num_clks = ARRAY_SIZE(msm8974_clks),
> > 
> > +	.scaling_before_handover = true,
> > 
> >  };
> >  
> >  static struct clk_smd_rpm *msm8976_clks[] = {
> > 
> > @@ -1318,6 +1320,12 @@ static int rpm_smd_clk_probe(struct platform_device
> > *pdev)> 
> >  	rpm_smd_clks = desc->clks;
> >  	num_clks = desc->num_clks;
> > 
> > +	if (desc->scaling_before_handover) {
> > +		ret = clk_smd_rpm_enable_scaling(rpm);
> > +		if (ret)
> > +			goto err;
> > +	}
> > +
> > 
> >  	for (i = 0; i < num_clks; i++) {
> >  	
> >  		if (!rpm_smd_clks[i])
> >  		
> >  			continue;
> > 
> > @@ -1329,9 +1337,11 @@ static int rpm_smd_clk_probe(struct platform_device
> > *pdev)> 
> >  			goto err;
> >  	
> >  	}
> > 
> > -	ret = clk_smd_rpm_enable_scaling(rpm);
> > -	if (ret)
> > -		goto err;
> > +	if (!desc->scaling_before_handover) {
> > +		ret = clk_smd_rpm_enable_scaling(rpm);
> > +		if (ret)
> > +			goto err;
> > +	}
> > 
> >  	for (i = 0; i < num_clks; i++) {
> >  	
> >  		if (!rpm_smd_clks[i])
> > 
> > ---
> > base-commit: dd9e11d6477a52ede9ebe575c83285e79e823889
> > change-id: 20230506-rpmcc-scaling-handover-a63029ed9d13
> > 
> > Best regards,
Konrad Dybcio May 17, 2023, 7:11 p.m. UTC | #3 
On 17.05.2023 16:59, Luca Weiss wrote:
> On Dienstag, 16. Mai 2023 03:27:46 CEST Konrad Dybcio wrote:
>> On 6.05.2023 22:10, Luca Weiss wrote:
>>> On older platforms like msm8226, msm8974 and msm8916 the driver in the
>>> downstream kernel enables scaling first before doing the handover of the
>>> clocks.
>>>
>>> While this normally doesn't seem to cause noticeable problems, on
>>> apq8026-asus-sparrow this causes the device to immediately reboot,
>>> perhaps due to older rpm firmware that becomes unhappy.
>>>
>>> On newer platforms the order has swapped and enabling scaling is done
>>> after the handover, so let's introduce this behavior only conditionally
>>> for msm8226 and msm8974 for now.
>>>
>>> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
>>> ---
>>
>> Did you give this a spin on some 8974? I think hammerhead had
>> issues around rpmcc in the past..
> 
> Yes, appears to be fine on msm8974 also.
> 
> I tried to reproduce the hammerhead ocmem hang we had in the past but even 
> with v6.3 rpmcc it seems to be fine. But iirc it was happening more or less at 
> random in the past so wouldn't be surprised if my tests just didn't show 
> anything on accident.
> 
> Regards
> Luca
OK sg

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
> 
>>
>> Konrad
>>
>>>  drivers/clk/qcom/clk-smd-rpm.c | 16 +++++++++++++---
>>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/clk/qcom/clk-smd-rpm.c
>>> b/drivers/clk/qcom/clk-smd-rpm.c index 887b945a6fb7..6d5476afc4d1 100644
>>> --- a/drivers/clk/qcom/clk-smd-rpm.c
>>> +++ b/drivers/clk/qcom/clk-smd-rpm.c
>>> @@ -178,6 +178,7 @@ struct clk_smd_rpm_req {
>>>
>>>  struct rpm_smd_clk_desc {
>>>  
>>>  	struct clk_smd_rpm **clks;
>>>  	size_t num_clks;
>>>
>>> +	bool scaling_before_handover;
>>>
>>>  };
>>>  
>>>  static DEFINE_MUTEX(rpm_smd_clk_lock);
>>>
>>> @@ -693,6 +694,7 @@ static struct clk_smd_rpm *msm8974_clks[] = {
>>>
>>>  static const struct rpm_smd_clk_desc rpm_clk_msm8974 = {
>>>  
>>>  	.clks = msm8974_clks,
>>>  	.num_clks = ARRAY_SIZE(msm8974_clks),
>>>
>>> +	.scaling_before_handover = true,
>>>
>>>  };
>>>  
>>>  static struct clk_smd_rpm *msm8976_clks[] = {
>>>
>>> @@ -1318,6 +1320,12 @@ static int rpm_smd_clk_probe(struct platform_device
>>> *pdev)> 
>>>  	rpm_smd_clks = desc->clks;
>>>  	num_clks = desc->num_clks;
>>>
>>> +	if (desc->scaling_before_handover) {
>>> +		ret = clk_smd_rpm_enable_scaling(rpm);
>>> +		if (ret)
>>> +			goto err;
>>> +	}
>>> +
>>>
>>>  	for (i = 0; i < num_clks; i++) {
>>>  	
>>>  		if (!rpm_smd_clks[i])
>>>  		
>>>  			continue;
>>>
>>> @@ -1329,9 +1337,11 @@ static int rpm_smd_clk_probe(struct platform_device
>>> *pdev)> 
>>>  			goto err;
>>>  	
>>>  	}
>>>
>>> -	ret = clk_smd_rpm_enable_scaling(rpm);
>>> -	if (ret)
>>> -		goto err;
>>> +	if (!desc->scaling_before_handover) {
>>> +		ret = clk_smd_rpm_enable_scaling(rpm);
>>> +		if (ret)
>>> +			goto err;
>>> +	}
>>>
>>>  	for (i = 0; i < num_clks; i++) {
>>>  	
>>>  		if (!rpm_smd_clks[i])
>>>
>>> ---
>>> base-commit: dd9e11d6477a52ede9ebe575c83285e79e823889
>>> change-id: 20230506-rpmcc-scaling-handover-a63029ed9d13
>>>
>>> Best regards,
> 
> 
> 
>
Bjorn Andersson May 25, 2023, 4:54 a.m. UTC | #4 
On Sat, 06 May 2023 22:10:32 +0200, Luca Weiss wrote:
> On older platforms like msm8226, msm8974 and msm8916 the driver in the
> downstream kernel enables scaling first before doing the handover of the
> clocks.
> 
> While this normally doesn't seem to cause noticeable problems, on
> apq8026-asus-sparrow this causes the device to immediately reboot,
> perhaps due to older rpm firmware that becomes unhappy.
> 
> [...]

Applied, thanks!

[1/1] clk: qcom: smd-rpm: conditionally enable scaling before doing handover
      commit: e9a2db5caf9f219d9cf570cb0965710344c85519

Best regards,
diff mbox series

Patch

diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index 887b945a6fb7..6d5476afc4d1 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -178,6 +178,7 @@  struct clk_smd_rpm_req {
 struct rpm_smd_clk_desc {
 	struct clk_smd_rpm **clks;
 	size_t num_clks;
+	bool scaling_before_handover;
 };
 
 static DEFINE_MUTEX(rpm_smd_clk_lock);
@@ -693,6 +694,7 @@  static struct clk_smd_rpm *msm8974_clks[] = {
 static const struct rpm_smd_clk_desc rpm_clk_msm8974 = {
 	.clks = msm8974_clks,
 	.num_clks = ARRAY_SIZE(msm8974_clks),
+	.scaling_before_handover = true,
 };
 
 static struct clk_smd_rpm *msm8976_clks[] = {
@@ -1318,6 +1320,12 @@  static int rpm_smd_clk_probe(struct platform_device *pdev)
 	rpm_smd_clks = desc->clks;
 	num_clks = desc->num_clks;
 
+	if (desc->scaling_before_handover) {
+		ret = clk_smd_rpm_enable_scaling(rpm);
+		if (ret)
+			goto err;
+	}
+
 	for (i = 0; i < num_clks; i++) {
 		if (!rpm_smd_clks[i])
 			continue;
@@ -1329,9 +1337,11 @@  static int rpm_smd_clk_probe(struct platform_device *pdev)
 			goto err;
 	}
 
-	ret = clk_smd_rpm_enable_scaling(rpm);
-	if (ret)
-		goto err;
+	if (!desc->scaling_before_handover) {
+		ret = clk_smd_rpm_enable_scaling(rpm);
+		if (ret)
+			goto err;
+	}
 
 	for (i = 0; i < num_clks; i++) {
 		if (!rpm_smd_clks[i])