cpufreq: add support for intermediate (stable) frequencies

Handling calls to ->target_index() has got complex over time and might become more complex. So, its better to take target_index() bits out in another routine __target_index() for better code readability. Shouldn't have any functional impact.

Tested-by Stephen Warren <[email protected]>

Reviewed-by Doug Anderson <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/cpufreq/cpufreq.c | 56 ++++++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 23 deletions(-)

drivers/cpufreq/cpufreq.c +33 -23

index a05c921..ae11dd5 100644
@@ -1816,12 +1816,43 @@ EXPORT_SYMBOL(cpufreq_unregister_notifier);
  *                              GOVERNORS                            *
  *********************************************************************/
 
+static int __target_index(struct cpufreq_policy *policy,
+			  struct cpufreq_frequency_table *freq_table, int index)
+{
+	struct cpufreq_freqs freqs;
+	int retval = -EINVAL;
+	bool notify;
+
+	notify = !(cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION);
+
+	if (notify) {
+		freqs.old = policy->cur;
+		freqs.new = freq_table[index].frequency;
+		freqs.flags = 0;
+
+		pr_debug("%s: cpu: %d, oldfreq: %u, new freq: %u\n",
+			 __func__, policy->cpu, freqs.old, freqs.new);
+
+		cpufreq_freq_transition_begin(policy, &freqs);
+	}
+
+	retval = cpufreq_driver->target_index(policy, index);
+	if (retval)
+		pr_err("%s: Failed to change cpu frequency: %d\n", __func__,
+		       retval);
+
+	if (notify)
+		cpufreq_freq_transition_end(policy, &freqs, retval);
+
+	return retval;
+}
+
 int __cpufreq_driver_target(struct cpufreq_policy *policy,
 			    unsigned int target_freq,
 			    unsigned int relation)
 {
-	int retval = -EINVAL;
 	unsigned int old_target_freq = target_freq;
+	int retval = -EINVAL;
 
 	if (cpufreq_disabled())
 		return -ENODEV;

@@ -1848,8 +1879,6 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 		retval = cpufreq_driver->target(policy, target_freq, relation);
 	else if (cpufreq_driver->target_index) {
 		struct cpufreq_frequency_table *freq_table;
-		struct cpufreq_freqs freqs;
-		bool notify;
 		int index;
 
 		freq_table = cpufreq_frequency_get_table(policy->cpu);

@@ -1870,26 +1899,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 			goto out;
 		}
 
-		notify = !(cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION);
-
-		if (notify) {
-			freqs.old = policy->cur;
-			freqs.new = freq_table[index].frequency;
-			freqs.flags = 0;
-
-			pr_debug("%s: cpu: %d, oldfreq: %u, new freq: %u\n",
-				 __func__, policy->cpu, freqs.old, freqs.new);
-
-			cpufreq_freq_transition_begin(policy, &freqs);
-		}
-
-		retval = cpufreq_driver->target_index(policy, index);
-		if (retval)
-			pr_err("%s: Failed to change cpu frequency: %d\n",
-			       __func__, retval);
-
-		if (notify)
-			cpufreq_freq_transition_end(policy, &freqs, retval);
+		retval = __target_index(policy, freq_table, index);
 	}
 
 out:

I guess I can take this one without the rest of the series?

Douglas Anderson, recently pointed out an interesting problem due to which udelay() was expiring earlier than it should.

While transitioning between frequencies few platforms may temporarily switch to a stable frequency, waiting for the main PLL to stabilize.

For example: When we transition between very low frequencies on exynos, like between 200MHz and 300MHz, we may temporarily switch to a PLL running at 800MHz. No CPUFREQ notification is sent for that. That means there's a period of time when we're running at 800MHz but loops_per_jiffy is calibrated at between 200MHz and 300MHz. And so udelay behaves badly.

To get this fixed in a generic way, lets introduce another set of callbacks get_intermediate() and target_intermediate(), only for drivers with target_index() and CPUFREQ_ASYNC_NOTIFICATION unset.

get_intermediate() should return a stable intermediate frequency platform wants to switch to, and target_intermediate() should set CPU to to that frequency, before jumping to the frequency corresponding to 'index'. Core will take care of sending notifications and driver doesn't have to handle them in target_intermediate() or target_index().

NOTE: ->target_index() should restore to policy->restore_freq in case of failures as core would send notifications for that.

drivers/cpufreq/cpufreq.c +18 -0

 
+/* Must set freqs->new to intermediate frequency */
+static int __target_intermediate(struct cpufreq_policy *policy,
+				 struct cpufreq_freqs *freqs, int index)
+{
+	int ret;
+
+	freqs->new = cpufreq_driver->get_intermediate(policy, index);
+
+	/* We don't need to switch to intermediate freq */
+	if (!freqs->new)
+		return 0;
+
+	pr_debug("%s: cpu: %d, switching to intermediate freq: oldfreq: %u, intermediate freq: %u\n",
+		 __func__, policy->cpu, freqs->old, freqs->new);
+
+	cpufreq_freq_transition_begin(policy, freqs);
+	ret = cpufreq_driver->target_intermediate(policy, index);
+	cpufreq_freq_transition_end(policy, freqs, ret);

static int __target_index(struct cpufreq_policy *policy,

drivers/cpufreq/cpufreq.c +2 -1

 			  struct cpufreq_frequency_table *freq_table, int index)
 {
-	struct cpufreq_freqs freqs;
+	struct cpufreq_freqs freqs = {.old = policy->cur, .flags = 0};
+	unsigned int intermediate_freq = 0;
 	int retval = -EINVAL;
 	bool notify;
 
 	notify = !(cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION);

It seems rather odd to set both freqs->old and freqs->new to the intermediate frequency upon success. It feels like it would make more sense to remove that final else clause above, and do the following where this function is called:

intermediate_freq = freqs.new;
if (intermediate_freq)
freqs.old = intermediate_freq
freqs.new = freq_table[index].frequency

(or even return the intermediate frequency from the function)

?

But I suppose isolating all the inside __target_intermediate() isn't so bad.

Quoted 10 lines

static int __target_index(struct cpufreq_policy *policy,
struct cpufreq_frequency_table *freq_table, int index)
{

drivers/cpufreq/cpufreq.c +2 -1

-     struct cpufreq_freqs freqs;
+     struct cpufreq_freqs freqs = {.old = policy->cur, .flags = 0};
+     unsigned int intermediate_freq = 0;
      int retval = -EINVAL;
      bool notify;
 
      notify = !(cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION);

drivers/cpufreq/cpufreq.c EXPORT_SYMBOL(cpufreq_unregister_notifier); · line 1862

Quoted 10 lines

drivers/cpufreq/cpufreq.c +5 -4

-
      if (notify) {
-             freqs.old = policy->cur;
-             freqs.new = freq_table[index].frequency;
-             freqs.flags = 0;
+             /* Handle switching to intermediate frequency */
+             if (cpufreq_driver->get_intermediate) {
+                     retval = __target_intermediate(policy, &freqs, index);
+                     if (retval)
+                             return retval;

Shouldn't this all be outside the if (notify) block, so that __target_intermediate is always called, and it's just the notify callbacks that gets skipped if (!notify)?

Quoted 26 lines · Stephen Warren

It seems rather odd to set both freqs->old and freqs->new to the intermediate frequency upon success. It feels like it would make more sense to remove that final else clause above, and do the following where this function is called:

Quoted 10 lines

static int __target_index(struct cpufreq_policy *policy,
struct cpufreq_frequency_table *freq_table, int index)
{

drivers/cpufreq/cpufreq.c +2 -1

-     struct cpufreq_freqs freqs;
+     struct cpufreq_freqs freqs = {.old = policy->cur, .flags = 0};
+     unsigned int intermediate_freq = 0;
      int retval = -EINVAL;
      bool notify;
 
      notify = !(cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION);

drivers/cpufreq/cpufreq.c EXPORT_SYMBOL(cpufreq_unregister_notifier); · line 1862

Quoted 10 lines

drivers/cpufreq/cpufreq.c +5 -4

-
      if (notify) {
-             freqs.old = policy->cur;
-             freqs.new = freq_table[index].frequency;
-             freqs.flags = 0;
+             /* Handle switching to intermediate frequency */
+             if (cpufreq_driver->get_intermediate) {
+                     retval = __target_intermediate(policy, &freqs, index);
+                     if (retval)
+                             return retval;

Quoted 3 lines

Shouldn't this all be outside the if (notify) block, so that __target_intermediate is always called, and it's just the notify callbacks that gets skipped if (!notify)?

So, this is logic I had:

Should we support 'intermediate freq' infrastructure when driver wants to handle notifications themselves?

Probably not.

The whole point of implementing this 'intermediate freq' infrastructure is to get rid of code redundancy while sending notifications. If drivers want to handle notifications then they better handle intermediate freqs as well in their target_index() callback. They don't need to implement another routine for intermediate stuff..

Oh OK, I guess the "notify" value is static then, and driver defined, so this is fine.

drivers/cpufreq/cpufreq.c +2 -1

@@ -1841,9 +1876,23 @@ static int __target_index(struct cpufreq_policy *policy,
                pr_err("%s: Failed to change cpu frequency: %d\n", __func__,
                       retval);
 
-       if (notify)
+       if (notify) {
                cpufreq_freq_transition_end(policy, &freqs, retval);
 
+               /*

As far as I can tell this notification says "I tried to switch from "intermediate_freq" to "policy->restore_freq" but I failed, so I'm still at "intermediate_freq". I think you probably want to pass 0 as the last argument to cpufreq_freq_transition_end() to fix...

Other than that this looks good to me. I'll do a final review when you spin the next version (since I think you need to fix stuff for Stephen too). I'll probably wait and re-review the Tegra version when you and Stephen come to a consensus on it.

Tegra had always been switching to intermediate frequency (pll_p_clk) since ever. CPUFreq core has better support for handling notifications for these frequencies and so we can adapt Tegra's driver to it.

Also do a WARN() if clk_set_parent() fails while moving back to pll_x as we should have atleast restored to earlier frequency on error.

I think the call to tegra_target_intermediate() is wrong here; shouldn't the cpufreq core guarantee that tegra_target_intermediate() has always been called before tegra_target(), so there's no need to repeat that call here?

Also, tegra_target() doesn't seem to follow the rule documented by patch 2/3 that states ->target() should restore the orignal frequency on error. I'm not even sure if that's possible in general.

Quoted 8 lines · Stephen Warren

I think the call to tegra_target_intermediate() is wrong here; shouldn't the cpufreq core guarantee that tegra_target_intermediate() has always been called before tegra_target(), so there's no need to repeat that call here?

Also, tegra_target() doesn't seem to follow the rule documented by patch 2/3 that states ->target() should restore the orignal frequency on error. I'm not even sure if that's possible in general.

I thought I took care of that. Can you please give some example when we aren't restoring original frequency on failure ?

About the rule, that has to be the expectation from core as there is no way out that for core to know what happened at end of target_index()..

It can call get_rate() but that would be over engineering it looks ..

	ret = clk_set_rate(pll_x_clk, rate * 1000);
	/* Restore to earlier frequency on error, i.e. pll_x */
	if (ret)
		pr_err("Failed to change pll_x to %lu\n", rate);

	ret = clk_set_parent(cpu_clk, pll_x_clk);

E.g. in the following code after the series is applied:

Quoted 5 lines

      ret = clk_set_rate(pll_x_clk, rate * 1000);
      /* Restore to earlier frequency on error, i.e. pll_x */
      if (ret)
              pr_err("Failed to change pll_x to %lu\n", rate);

Quoted 1 line

      ret = clk_set_parent(cpu_clk, pll_x_clk);

If the clk_set_rate() failed, we have no idea what the pll_x rate is; I don't think the clock API guarantees that zero HW changes have been made if the function fails. Yet the code switches the CPU clock back to pll_x without attempting to fix the pll_x rate to be the old rate. Perhaps there's not much that can be done here though, since if one clk_set_rate() failed, perhaps the one to fix it will too.

I suspect there are other issues, like switching between pll_p/pllx might not be restored on error

and the EMC frequency isn't switched back, etc.

Tegra had always been switching to intermediate frequency (pll_p_clk) since ever. CPUFreq core has better support for handling notifications for these frequencies and so we can adapt Tegra's driver to it.

Also do a WARN() if clk_set_parent() fails while moving back to pll_x as we should have atleast restored to earlier frequency on error.

drivers/cpufreq/tegra-cpufreq.c +2 -0

+static int
+tegra_target_intermediate(struct cpufreq_policy *policy, unsigned int index)
 {
 	int ret;

 
	/*
	 * Take an extra reference to the main pll so it doesn't turn
	 * off when we move the cpu off of it
	 */
	clk_prepare_enable(pll_x_clk);

drivers/cpufreq/tegra-cpufreq.c +3 -0

+
+	ret = clk_set_rate(pll_x_clk, rate * 1000);
+	/* Restore to earlier frequency on error, i.e. pll_x */
 	if (ret)

This patch breaks Tegra. The reason is below.

drivers/cpufreq/tegra-cpufreq.c static struct clk *pll_x_clk; · line 50

Quoted 5 lines

drivers/cpufreq/tegra-cpufreq.c +2 -1

 
-static int tegra_cpu_clk_set_rate(unsigned long rate)
+static unsigned int
+tegra_get_intermediate(struct cpufreq_policy *policy, unsigned int index)

(BTW, can we please not put the return type on a separate line; it's inconsistent with the rest of the code in this file)

drivers/cpufreq/tegra-cpufreq.c static struct clk *pll_x_clk; · line 60

Quoted 10 lines

drivers/cpufreq/tegra-cpufreq.c +10 -0

+{
+     unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000;
+
+     /*
+      * Don't switch to intermediate freq if:
+      * - we are already at it, i.e. policy->cur == ifreq
+      * - index corresponds to ifreq
+      */
+     if ((freq_table[index].frequency == ifreq) || (policy->cur == ifreq))
+             return 0;

If policy->cur == ifreq here, then tegra_target_intermediate() isn't called by the cpufreq core, so ...

drivers/cpufreq/tegra-cpufreq.c static struct clk *pll_x_clk;

Quoted 10 lines

drivers/cpufreq/tegra-cpufreq.c +2 -0

+static int
+tegra_target_intermediate(struct cpufreq_policy *policy, unsigned int index)
 {
      int ret;
 
      /*
       * Take an extra reference to the main pll so it doesn't turn
       * off when we move the cpu off of it
       */
      clk_prepare_enable(pll_x_clk);

... that reference isn't added...

drivers/cpufreq/tegra-cpufreq.c static int tegra_target(struct cpufreq_policy *policy, unsigned int index) · line 103

Quoted 10 lines

drivers/cpufreq/tegra-cpufreq.c +5 -1

@@ -98,10 +96,23 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
      else
              clk_set_rate(emc_clk, 100000000);  /* emc 50Mhz */
 
-     ret = tegra_cpu_clk_set_rate(rate * 1000);
+     /* target freq == pll_p */
+     if (rate * 1000 == clk_get_rate(pll_p_clk)) {
+             ret = tegra_target_intermediate(policy, index);
+             goto disable_pll_x;
+     }

... and this code doesn't call it either, since we could be switching from the pll_p rate to something faster ...

Quoted 4 lines

drivers/cpufreq/tegra-cpufreq.c +3 -0

+
+     ret = clk_set_rate(pll_x_clk, rate * 1000);
+     /* Restore to earlier frequency on error, i.e. pll_x */
      if (ret)

drivers/cpufreq/tegra-cpufreq.c static int tegra_target(struct cpufreq_policy *policy, unsigned int index) · line 115

Quoted 10 lines

drivers/cpufreq/tegra-cpufreq.c +8 -2

-             pr_err("cpu-tegra: Failed to set cpu frequency to %lu kHz\n",
-                     rate);
+             pr_err("Failed to change pll_x to %lu\n", rate);
+
+     ret = clk_set_parent(cpu_clk, pll_x_clk);
+     /* This shouldn't fail while changing or restoring */
+     WARN_ON(ret);
+
+disable_pll_x:
+     clk_disable_unprepare(pll_x_clk);

... so this turns off pll_x even though we're running from it.

It would be simpler if Tegra *always* used an intermediate frequency, and hence the core *always* called tegra_target_intermediate(). Admittedly, this would result in tegra_target() sometimes (when switching CPU clock rate to the pll_p rate) doing nothing other than removing the extra reference on pll_x, but I think that the code would be simpler to follow and more robust.

Quoted 1 line · Stephen Warren

This patch breaks Tegra. The reason is below.

Quoted 4 lines

drivers/cpufreq/tegra-cpufreq.c static int tegra_target(struct cpufreq_policy *policy, unsigned int index) · line 115

Quoted 2 lines

drivers/cpufreq/tegra-cpufreq.c +2 -0

+disable_pll_x:
+     clk_disable_unprepare(pll_x_clk);

... so this turns off pll_x even though we're running from it.

Can you describe the role of the enable/disable of this pll_x_clk please? Which all clocks depend on it, etc? So that I understand why its important to enable it and for which clocks. And also if we need to disable it after changing to any freq..

I believe the issue is this:

We can't change the rate of pll_x while it's being used as a source for something. I'm not 100% sure why. I assume the PLL output simply isn't stable enough while changing rates; perhaps it can go out-of-range, or generate glitches.

This means we must switch the CPU clock source to something else (we use pll_p) while changing the pll_x rate.

Since the CPU is the only thing that uses pll_x, if we switch the CPU to some other clock source, pll_x will become unused, so it will be automatically disabled.

Disabling the PLL, and then re-enabling it later when switching the CPU back to it, presumably takes some extra time (e.g. waiting for PLL lock when it starts back up), so the code takes an extra reference to the clock (prepare_enable) before switching the CPU away from it, and drops that (disable_unprepare) after switching the CPU back to it.

The only case pll_x is disabled is when we use a cpufreq of 216MHz; that frequency is provided by pll_p itself, so we never switch back to pll_x in this case, and do want to shut down pll_x to save some power.

Now, in your patch when switching from 216MHz to pll_x, the initial prepare_enable(pll_x) never happens, then the CPU is switched back to pll_x which turns it on, then the unpaired disable_unprepare(pll_x) happens which turns off pll_x even though the CPU is using it as clock source. Arguably the clock API has a bug in that it shouldn't allow these unpaired calls to break the reference counting, but that's the way the API is right now.

Quoted 2 lines · Stephen Warren

... [This patch causes issues on Tegra20] ...
I believe the issue is this:

Okay, that was very helpful..

What about this ? (Attached for testing) :

Author: Viresh Kumar <[email protected]>
Date:   Fri May 16 14:22:40 2014 +0530

cpufreq: Tegra: implement intermediate frequency callbacks

Tegra had always been switching to intermediate frequency (pll_p_clk) since ever. CPUFreq core has better support for handling notifications for these frequencies and so we can adapt Tegra's driver to it.

Also do a WARN() if clk_set_parent() fails while moving back to pll_x as we should have atleast restored to earlier frequency on error.