cpufreq: arm_big_little: fix frequency check when bL switcher is active

PATCH In review Jon Medhurst (Tixy) 4 participants 6 messages Oct 21, 2015 – Nov 5, 2015 CPU frequency scaling framework

lore

Share & tools

lore

Jon Medhurst (Tixy)

author

2015-10-21 09:55 UTC #0 lore raw

PATCH v2 1/1 cpufreq: arm_big_little: fix frequency check when bL switcher is active

The check for correct frequency being set in bL_cpufreq_set_rate is broken when the big.LITTLE switcher is active, for two reasons.

1. The 'new_rate' variable gets overwritten before the test by the code calculating the frequency of the old cluster.

2. The frequency returned by bL_cpufreq_get_rate will be the virtual frequency, not the actual one the intended version of new_rate contains.

This means the function always returns an error causing an endless stream of: "cpufreq: __target_index: Failed to change cpu frequency: -5"

As the intent is to check for errors that clk_set_rate doesn't report lets move the check to immediately after that and directly use clk_get_rate, rather than the arm_big_little helpers which only confuse matters. Also, update the comment to be hopefully clearer about the purpose of the code.

Fixes 0a95e630b49a ("cpufreq: arm_big_little: check if the frequency is set correctly")

Signed-off-by Jon Medhurst <[email protected]>

Acked-by Sudeep Holla <[email protected]>

---

Changes since V1:
- Check rate using clk_get_rate rather than disabling check when bL switcher active

Sudeep, I added your Ack from the last comment on the previous patch. This final patch differs from what was discussed only in the commit message and in source comment which is hopefully more clear and is also satisfactory.

I've also added Michael Turquette's correct email to the CC this time, rather than his old Linaro address which was bouncing.

 drivers/cpufreq/arm_big_little.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

drivers/cpufreq/arm_big_little.c +13 -9

index f1e42f8..c5d256c 100644
@@ -149,6 +149,19 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
 			__func__, cpu, old_cluster, new_cluster, new_rate);
 
 	ret = clk_set_rate(clk[new_cluster], new_rate * 1000);
+	if (!ret) {
+		/*
+		 * FIXME: clk_set_rate hasn't returned an error here however it
+		 * may be that clk_change_rate failed due to hardware or
+		 * firmware issues and wasn't able to report that due to the
+		 * current design of the clk core layer. To work around this
+		 * problem we will read back the clock rate and check it is
+		 * correct. This needs to be removed once clk core is fixed.
+		 */
+		if (clk_get_rate(clk[new_cluster]) != new_rate * 1000)
+			ret = -EIO;
+	}
+
 	if (WARN_ON(ret)) {
 		pr_err("clk_set_rate failed: %d, new cluster: %d\n", ret,
 				new_cluster);

@@ -189,15 +202,6 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
 		mutex_unlock(&cluster_lock[old_cluster]);
 	}
 
-	/*
-	 * FIXME: clk_set_rate has to handle the case where clk_change_rate
-	 * can fail due to hardware or firmware issues. Until the clk core
-	 * layer is fixed, we can check here. In most of the cases we will
-	 * be reading only the cached value anyway. This needs to  be removed
-	 * once clk core is fixed.
-	 */
-	if (bL_cpufreq_get_rate(cpu) != new_rate)
-		return -EIO;
 	return 0;
 }

Signature · 1 line

2.1.4

Viresh Kumar

maintainer Acked-by on v2 1/1

2015-10-21 09:57 UTC #1 lore raw

Quoted 33 lines · Jon Medhurst (Tixy)

The check for correct frequency being set in bL_cpufreq_set_rate is broken when the big.LITTLE switcher is active, for two reasons.
1. The 'new_rate' variable gets overwritten before the test by the code calculating the frequency of the old cluster.
2. The frequency returned by bL_cpufreq_get_rate will be the virtual frequency, not the actual one the intended version of new_rate contains.
This means the function always returns an error causing an endless stream of: "cpufreq: __target_index: Failed to change cpu frequency: -5"
As the intent is to check for errors that clk_set_rate doesn't report lets move the check to immediately after that and directly use clk_get_rate, rather than the arm_big_little helpers which only confuse matters. Also, update the comment to be hopefully clearer about the purpose of the code.
Fixes 0a95e630b49a ("cpufreq: arm_big_little: check if the frequency is set correctly")
Signed-off-by Jon Medhurst <[email protected]>
Acked-by Sudeep Holla <[email protected]>
---
Changes since V1:
- Check rate using clk_get_rate rather than disabling check when bL switcher active
Sudeep, I added your Ack from the last comment on the previous patch. This final patch differs from what was discussed only in the commit message and in source comment which is hopefully more clear and is also satisfactory.

Quoted 6 lines

I've also added Michael Turquette's correct email to the CC this time, rather than his old Linaro address which was bouncing.
 drivers/cpufreq/arm_big_little.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

Acked-by Viresh Kumar <[email protected]>

Signature · 1 line

viresh

Jon Medhurst (Tixy)

author

2015-10-30 10:48 UTC #2 lore raw

Quoted 25 lines · Viresh Kumar

Quoted 24 lines · Jon Medhurst (Tixy)
The check for correct frequency being set in bL_cpufreq_set_rate is broken when the big.LITTLE switcher is active, for two reasons.
1. The 'new_rate' variable gets overwritten before the test by the code calculating the frequency of the old cluster.
2. The frequency returned by bL_cpufreq_get_rate will be the virtual frequency, not the actual one the intended version of new_rate contains.
This means the function always returns an error causing an endless stream of: "cpufreq: __target_index: Failed to change cpu frequency: -5"
As the intent is to check for errors that clk_set_rate doesn't report lets move the check to immediately after that and directly use clk_get_rate, rather than the arm_big_little helpers which only confuse matters. Also, update the comment to be hopefully clearer about the purpose of the code.
Fixes 0a95e630b49a ("cpufreq: arm_big_little: check if the frequency is set correctly")
Signed-off-by Jon Medhurst <[email protected]>
Acked-by Sudeep Holla <[email protected]>
---

[...]

Quoted 3 lines

Acked-by Viresh Kumar <[email protected]>

Do I need to send this again with that Ack and Michael's Reviewed-by added?

Signature · 1 line

Tixy

Rafael J. Wysocki

2015-10-31 02:05 UTC #3 lore raw

Quoted 26 lines · Jon Medhurst

Quoted 19 lines · Viresh Kumar
Quoted 18 lines · Jon Medhurst (Tixy)
The check for correct frequency being set in bL_cpufreq_set_rate is broken when the big.LITTLE switcher is active, for two reasons.
1. The 'new_rate' variable gets overwritten before the test by the code calculating the frequency of the old cluster.
2. The frequency returned by bL_cpufreq_get_rate will be the virtual frequency, not the actual one the intended version of new_rate contains.
This means the function always returns an error causing an endless stream of: "cpufreq: __target_index: Failed to change cpu frequency: -5"
As the intent is to check for errors that clk_set_rate doesn't report lets move the check to immediately after that and directly use clk_get_rate, rather than the arm_big_little helpers which only confuse matters. Also, update the comment to be hopefully clearer about the purpose of the code.
Quoted 6 lines
Quoted 6 lines
Fixes 0a95e630b49a ("cpufreq: arm_big_little: check if the frequency is set correctly")
Signed-off-by Jon Medhurst <[email protected]>
Acked-by Sudeep Holla <[email protected]>
---

Quoted 7 lines

[...]
Quoted 3 lines
Acked-by Viresh Kumar <[email protected]>
Do I need to send this again with that Ack and Michael's Reviewed-by added?

Nope, it's on my to-do list.

Thanks,
Rafael

Rafael J. Wysocki

2015-11-05 22:17 UTC #4 lore raw

Quoted 29 lines · Rafael J. Wysocki

Quoted 28 lines · Jon Medhurst
Quoted 25 lines · Viresh Kumar
Quoted 24 lines · Jon Medhurst (Tixy)
The check for correct frequency being set in bL_cpufreq_set_rate is broken when the big.LITTLE switcher is active, for two reasons.
1. The 'new_rate' variable gets overwritten before the test by the code calculating the frequency of the old cluster.
2. The frequency returned by bL_cpufreq_get_rate will be the virtual frequency, not the actual one the intended version of new_rate contains.
This means the function always returns an error causing an endless stream of: "cpufreq: __target_index: Failed to change cpu frequency: -5"
As the intent is to check for errors that clk_set_rate doesn't report lets move the check to immediately after that and directly use clk_get_rate, rather than the arm_big_little helpers which only confuse matters. Also, update the comment to be hopefully clearer about the purpose of the code.
Fixes 0a95e630b49a ("cpufreq: arm_big_little: check if the frequency is set correctly")
Signed-off-by Jon Medhurst <[email protected]>
Acked-by Sudeep Holla <[email protected]>
---
[...]
Quoted 1 line

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
Acked-by Viresh Kumar <[email protected]>
Do I need to send this again with that Ack and Michael's Reviewed-by added?
Nope, it's on my to-do list.

Applied now, thanks!

Rafael

Michael Turquette

Reviewed-by on v2 1/1

2015-10-26 12:42 UTC #5 lore raw

In reply to Jon Medhurst (Tixy)

Quoting Jon Medhurst (Tixy) (2015-10-21 02:55:33)

Quoted 18 lines

The check for correct frequency being set in bL_cpufreq_set_rate is broken when the big.LITTLE switcher is active, for two reasons.
1. The 'new_rate' variable gets overwritten before the test by the code calculating the frequency of the old cluster.
2. The frequency returned by bL_cpufreq_get_rate will be the virtual frequency, not the actual one the intended version of new_rate contains.
This means the function always returns an error causing an endless stream of: "cpufreq: __target_index: Failed to change cpu frequency: -5"
As the intent is to check for errors that clk_set_rate doesn't report lets move the check to immediately after that and directly use clk_get_rate, rather than the arm_big_little helpers which only confuse matters. Also, update the comment to be hopefully clearer about the purpose of the code.

Quoted 4 lines

Fixes 0a95e630b49a ("cpufreq: arm_big_little: check if the frequency is set correctly")
Signed-off-by Jon Medhurst <[email protected]>
Acked-by Sudeep Holla <[email protected]>

Reviewed-by Michael Turquette <[email protected]>

Fixing exception paths for clk_change_rate is on the TODO list.

Regards,
Mike

Quoted 62 lines

---

Changes since V1:
- Check rate using clk_get_rate rather than disabling check when bL switcher active

I've also added Michael Turquette's correct email to the CC this time, rather than his old Linaro address which was bouncing.

 drivers/cpufreq/arm_big_little.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

drivers/cpufreq/arm_big_little.c +13 -9

index f1e42f8..c5d256c 100644
@@ -149,6 +149,19 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
                        __func__, cpu, old_cluster, new_cluster, new_rate);
 
        ret = clk_set_rate(clk[new_cluster], new_rate * 1000);
+       if (!ret) {
+               /*
+                * FIXME: clk_set_rate hasn't returned an error here however it
+                * may be that clk_change_rate failed due to hardware or
+                * firmware issues and wasn't able to report that due to the
+                * current design of the clk core layer. To work around this
+                * problem we will read back the clock rate and check it is
+                * correct. This needs to be removed once clk core is fixed.
+                */
+               if (clk_get_rate(clk[new_cluster]) != new_rate * 1000)
+                       ret = -EIO;
+       }
+
        if (WARN_ON(ret)) {
                pr_err("clk_set_rate failed: %d, new cluster: %d\n", ret,
                                new_cluster);

@@ -189,15 +202,6 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
                mutex_unlock(&cluster_lock[old_cluster]);
        }
 
-       /*
-        * FIXME: clk_set_rate has to handle the case where clk_change_rate
-        * can fail due to hardware or firmware issues. Until the clk core
-        * layer is fixed, we can check here. In most of the cases we will
-        * be reading only the cached value anyway. This needs to  be removed
-        * once clk core is fixed.
-        */
-       if (bL_cpufreq_get_rate(cpu) != new_rate)
-               return -EIO;
        return 0;
 }

Signature · 1 line

2.1.4