[PATCH V4 0/3] cpufreq: add support for intermediate (stable) frequencies

[PATCH V4 0/3] cpufreq: add support for intermediate (stable) frequencies

[Viresh Kumar]

Doug raised many important concerns on V3:
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg645134.html

and so here is another iteration with improvements.

@Stephen: I have dropped you Tested-by's as patches are updated and requires a
retest, sorry for that :(

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

This patchset also update Tegra to use this new infrastructure and is already
tested by Stephen.

V3->V4:
- Allow get_intermediate() to return zero when we don't need to switch to
  intermediate first
- Get rid of 'goto' and create another routine for handling intermediate freqs
- Allow target_index() to fail, its not a crime :)
- Fix tegra driver to return zero from get_intermediate() for few situations
  (refer to patch 3/4)
- Fix issues with tegra's patch, like s/rate/rate * 1000
- Overall there are more modifications that what Doug requested as I felt we
  need better support from core.
- Looks much better now, thanks Doug :)

V2-V3:
- Fix spelling error: s/Uset/Used
- Update tegra with the changes Stephen suggested
- Include a dependency patch sent separately earlier (3/4)

V1-V2: Almost changed completely, V1 was here: https://lkml.org/lkml/2014/5/15/40

Viresh Kumar (3):
  cpufreq: handle calls to ->target_index() in separate routine
  cpufreq: add support for intermediate (stable) frequencies
  cpufreq: Tegra: implement intermediate frequency callbacks

 Documentation/cpu-freq/cpu-drivers.txt |  29 ++++++++-
 drivers/cpufreq/cpufreq.c              | 111 ++++++++++++++++++++++++++-------
 drivers/cpufreq/tegra-cpufreq.c        |  81 ++++++++++++++----------
 include/linux/cpufreq.h                |  25 ++++++++
 4 files changed, 186 insertions(+), 60 deletions(-)

-- 
2.0.0.rc2

[PATCH V4 1/3] cpufreq: handle calls to ->target_index() in separate routine

[Viresh Kumar]

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 <swarren@nvidia.com>
Reviewed-by: Doug Anderson <dianders@chromium.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 56 ++++++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 23 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index a05c921..ae11dd5 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -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:
-- 
2.0.0.rc2

[PATCH V4 2/3] cpufreq: add support for intermediate (stable) frequencies

[Viresh Kumar]

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.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 Documentation/cpu-freq/cpu-drivers.txt | 29 ++++++++++++++-
 drivers/cpufreq/cpufreq.c              | 67 ++++++++++++++++++++++++++++++----
 include/linux/cpufreq.h                | 25 +++++++++++++
 3 files changed, 112 insertions(+), 9 deletions(-)

diff --git a/Documentation/cpu-freq/cpu-drivers.txt b/Documentation/cpu-freq/cpu-drivers.txt
index b045fe5..14f4e63 100644
--- a/Documentation/cpu-freq/cpu-drivers.txt
+++ b/Documentation/cpu-freq/cpu-drivers.txt
@@ -26,6 +26,7 @@ Contents:
 1.4  target/target_index or setpolicy?
 1.5  target/target_index
 1.6  setpolicy
+1.7  get_intermediate and target_intermediate
 2.   Frequency Table Helpers
 
 
@@ -79,6 +80,10 @@ cpufreq_driver.attr -		A pointer to a NULL-terminated list of
 				"struct freq_attr" which allow to
 				export values to sysfs.
 
+cpufreq_driver.get_intermediate
+and target_intermediate		Used to switch to stable frequency while
+				changing CPU frequency.
+
 
 1.2 Per-CPU Initialization
 --------------------------
@@ -151,7 +156,7 @@ Some cpufreq-capable processors switch the frequency between certain
 limits on their own. These shall use the ->setpolicy call
 
 
-1.4. target/target_index
+1.5. target/target_index
 -------------
 
 The target_index call has two arguments: struct cpufreq_policy *policy,
@@ -160,6 +165,9 @@ and unsigned int index (into the exposed frequency table).
 The CPUfreq driver must set the new frequency when called here. The
 actual frequency must be determined by freq_table[index].frequency.
 
+It should always restore to earlier frequency (i.e. policy->restore_freq) in
+case of errors, even if we switched to intermediate frequency earlier.
+
 Deprecated:
 ----------
 The target call has three arguments: struct cpufreq_policy *policy,
@@ -179,7 +187,7 @@ Here again the frequency table helper might assist you - see section 2
 for details.
 
 
-1.5 setpolicy
+1.6 setpolicy
 ---------------
 
 The setpolicy call only takes a struct cpufreq_policy *policy as
@@ -190,6 +198,23 @@ setting when policy->policy is CPUFREQ_POLICY_PERFORMANCE, and a
 powersaving-oriented setting when CPUFREQ_POLICY_POWERSAVE. Also check
 the reference implementation in drivers/cpufreq/longrun.c
 
+1.7 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().
+
+Drivers can return '0' from get_intermediate() in case they don't wish to switch
+to intermediate frequency for some target frequency. In that case core will
+directly call ->target_index().
+
+NOTE: ->target_index() should restore to policy->restore_freq in case of
+failures as core would send notifications for that.
 
 
 2. Frequency Table Helpers
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index ae11dd5..a72c4d5 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1816,20 +1816,55 @@ EXPORT_SYMBOL(cpufreq_unregister_notifier);
  *                              GOVERNORS                            *
  *********************************************************************/
 
+/* 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);
+
+	if (ret)
+		pr_err("%s: Failed to change to intermediate frequency: %d\n",
+		       __func__, ret);
+	else
+		/* Set old freq to intermediate */
+		freqs->old = freqs->new;
+
+	return ret;
+}
+
 static int __target_index(struct cpufreq_policy *policy,
 			  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);
-
 	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;
 
+			intermediate_freq = freqs.new;
+		}
+
+		freqs.new = freq_table[index].frequency;
 		pr_debug("%s: cpu: %d, oldfreq: %u, new freq: %u\n",
 			 __func__, policy->cpu, freqs.old, freqs.new);
 
@@ -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);
 
+		/*
+		 * Failed after setting to intermediate freq? Driver should have
+		 * reverted back to initial frequency and so should we. Check
+		 * here for intermediate_freq instead of get_intermediate, in
+		 * case we have't switched to intermediate freq at all.
+		 */
+		if (unlikely(retval && intermediate_freq)) {
+			freqs.old = intermediate_freq;
+			freqs.new = policy->restore_freq;
+			cpufreq_freq_transition_begin(policy, &freqs);
+			cpufreq_freq_transition_end(policy, &freqs, retval);
+		}
+	}
+
 	return retval;
 }
 
@@ -1875,6 +1924,9 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 	if (target_freq == policy->cur)
 		return 0;
 
+	/* Save last value to restore later on errors */
+	policy->restore_freq = policy->cur;
+
 	if (cpufreq_driver->target)
 		retval = cpufreq_driver->target(policy, target_freq, relation);
 	else if (cpufreq_driver->target_index) {
@@ -2361,7 +2413,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
 	    !(driver_data->setpolicy || driver_data->target_index ||
 		    driver_data->target) ||
 	     (driver_data->setpolicy && (driver_data->target_index ||
-		    driver_data->target)))
+		    driver_data->target)) ||
+	     (!!driver_data->get_intermediate != !!driver_data->target_intermediate))
 		return -EINVAL;
 
 	pr_debug("trying to register driver %s\n", driver_data->name);
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 3f45889..ec4112d 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -75,6 +75,7 @@ struct cpufreq_policy {
 	unsigned int		max;    /* in kHz */
 	unsigned int		cur;    /* in kHz, only needed if cpufreq
 					 * governors are used */
+	unsigned int		restore_freq; /* = policy->cur before transition */
 	unsigned int		suspend_freq; /* freq to set during suspend */
 
 	unsigned int		policy; /* see above */
@@ -221,11 +222,35 @@ struct cpufreq_driver {
 
 	/* define one out of two */
 	int	(*setpolicy)	(struct cpufreq_policy *policy);
+
+	/*
+	 * On failure, should always restore frequency to policy->restore_freq
+	 * (i.e. old freq).
+	 */
 	int	(*target)	(struct cpufreq_policy *policy,	/* Deprecated */
 				 unsigned int target_freq,
 				 unsigned int relation);
 	int	(*target_index)	(struct cpufreq_policy *policy,
 				 unsigned int index);
+	/*
+	 * 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().
+	 *
+	 * Drivers can return '0' from get_intermediate() in case they don't
+	 * wish to switch to intermediate frequency for some target frequency.
+	 * In that case core will directly call ->target_index().
+	 */
+	unsigned int (*get_intermediate)(struct cpufreq_policy *policy,
+					 unsigned int index);
+	int	(*target_intermediate)(struct cpufreq_policy *policy,
+				       unsigned int index);
 
 	/* should be defined, if possible */
 	unsigned int	(*get)	(unsigned int cpu);
-- 
2.0.0.rc2

[PATCH V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks

[Viresh Kumar]

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.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/tegra-cpufreq.c | 81 ++++++++++++++++++++++++-----------------
 1 file changed, 47 insertions(+), 34 deletions(-)

diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
index 6e774c6..a64b970 100644
--- a/drivers/cpufreq/tegra-cpufreq.c
+++ b/drivers/cpufreq/tegra-cpufreq.c
@@ -46,7 +46,24 @@ static struct clk *pll_x_clk;
 static struct clk *pll_p_clk;
 static struct clk *emc_clk;
 
-static int tegra_cpu_clk_set_rate(unsigned long rate)
+static unsigned int
+tegra_get_intermediate(struct cpufreq_policy *policy, unsigned int index)
+{
+	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;
+
+	return ifreq;
+}
+
+static int
+tegra_target_intermediate(struct cpufreq_policy *policy, unsigned int index)
 {
 	int ret;
 
@@ -57,28 +74,9 @@ static int tegra_cpu_clk_set_rate(unsigned long rate)
 	clk_prepare_enable(pll_x_clk);
 
 	ret = clk_set_parent(cpu_clk, pll_p_clk);
-	if (ret) {
-		pr_err("Failed to switch cpu to clock pll_p\n");
-		goto out;
-	}
-
-	if (rate == clk_get_rate(pll_p_clk))
-		goto out;
-
-	ret = clk_set_rate(pll_x_clk, rate);
-	if (ret) {
-		pr_err("Failed to change pll_x to %lu\n", rate);
-		goto out;
-	}
-
-	ret = clk_set_parent(cpu_clk, pll_x_clk);
-	if (ret) {
-		pr_err("Failed to switch cpu to clock pll_x\n");
-		goto out;
-	}
+	if (ret)
+		clk_disable_unprepare(pll_x_clk);
 
-out:
-	clk_disable_unprepare(pll_x_clk);
 	return ret;
 }
 
@@ -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;
+	}
+
+	ret = clk_set_rate(pll_x_clk, rate * 1000);
+	/* Restore to earlier frequency on error, i.e. pll_x */
 	if (ret)
-		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);
 
 	return ret;
 }
@@ -137,16 +148,18 @@ static int tegra_cpu_exit(struct cpufreq_policy *policy)
 }
 
 static struct cpufreq_driver tegra_cpufreq_driver = {
-	.flags		= CPUFREQ_NEED_INITIAL_FREQ_CHECK,
-	.verify		= cpufreq_generic_frequency_table_verify,
-	.target_index	= tegra_target,
-	.get		= cpufreq_generic_get,
-	.init		= tegra_cpu_init,
-	.exit		= tegra_cpu_exit,
-	.name		= "tegra",
-	.attr		= cpufreq_generic_attr,
+	.flags			= CPUFREQ_NEED_INITIAL_FREQ_CHECK,
+	.verify			= cpufreq_generic_frequency_table_verify,
+	.get_intermediate	= tegra_get_intermediate,
+	.target_intermediate	= tegra_target_intermediate,
+	.target_index		= tegra_target,
+	.get			= cpufreq_generic_get,
+	.init			= tegra_cpu_init,
+	.exit			= tegra_cpu_exit,
+	.name			= "tegra",
+	.attr			= cpufreq_generic_attr,
 #ifdef CONFIG_PM
-	.suspend	= cpufreq_generic_suspend,
+	.suspend		= cpufreq_generic_suspend,
 #endif
 };
 
-- 
2.0.0.rc2

Re: [PATCH V4 2/3] cpufreq: add support for intermediate (stable) frequencies

[Stephen Warren]

On 05/21/2014 02:59 AM, Viresh Kumar wrote:
> 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.

> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c

> +/* 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);
> +
> +	if (ret)
> +		pr_err("%s: Failed to change to intermediate frequency: %d\n",
> +		       __func__, ret);
> +	else
> +		/* Set old freq to intermediate */
> +		freqs->old = freqs->new;
> +
> +	return ret;
> +}

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.

>  static int __target_index(struct cpufreq_policy *policy,
>  			  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);
> -
>  	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)?

Re: [PATCH V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks

[Stephen Warren]

On 05/21/2014 02:59 AM, Viresh Kumar wrote:
> 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.

> diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c

> @@ -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;
> +	}

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.

Re: [PATCH V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks

[Viresh Kumar]

On 22 May 2014 22:09, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 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?

That's what Doug requested in the previous version. get_intermediate()
can return zero in case drivers don't want to switch to intermediate
frequency for some target frequency.

Core should rather guarantee that target_index() is always called, if you
want core to guarantee that target_intermediate() is also always called,
then don't ever return zero from get_intermediate.

I did it that way for tegra as both target_intermediate() & target_index()
would have tried to set the same frequency for this particular case,
i.e. when target freq == intermediate frequency.

And both would have sent notification and the last notification wouldn't
have made any sense, both old-freq & new-freq would have been
intermediate freqs.

So, yes I see the feature suggested by Doug quite useful. Like in your
case.

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

Re: [PATCH V4 2/3] cpufreq: add support for intermediate (stable) frequencies

[Viresh Kumar]

On 22 May 2014 22:07, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 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

Looks better.

> (or even return the intermediate frequency from the function)

It can return errors as well and so I didn't tried to return frequency
from it. Though there are routines that return both error and freq
from such calls :)

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

:)

>>  static int __target_index(struct cpufreq_policy *policy,
>>                         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);
>> -
>>       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)?

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

--
viresh

Re: [PATCH V4 2/3] cpufreq: add support for intermediate (stable) frequencies

[Stephen Warren]

On 05/22/2014 10:24 PM, Viresh Kumar wrote:
> On 22 May 2014 22:07, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> 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:

>>>  static int __target_index(struct cpufreq_policy *policy,
>>>                         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);
>>> -
>>>       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)?
> 
> 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.

Re: [PATCH V4 2/3] cpufreq: add support for intermediate (stable) frequencies

[Viresh Kumar]

On 23 May 2014 21:26, Stephen Warren <swarren@wwwdotorg.org> wrote:
> Oh OK, I guess the "notify" value is static then, and driver defined, so
> this is fine.

Correct!! Can you reply on the tegra patch also? So that we can close this
thread ASAP?

Re: [PATCH V4 1/3] cpufreq: handle calls to ->target_index() in separate routine

[Rafael J. Wysocki]

On Wednesday, May 21, 2014 02:29:29 PM Viresh Kumar wrote:
> 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 <swarren@nvidia.com>
> Reviewed-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

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

> ---
>  drivers/cpufreq/cpufreq.c | 56 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 33 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index a05c921..ae11dd5 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -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 speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V4 1/3] cpufreq: handle calls to ->target_index() in separate routine

[Viresh Kumar]

On 27 May 2014 04:51, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> I guess I can take this one without the rest of the series?

Yes.

Re: [PATCH V4 2/3] cpufreq: add support for intermediate (stable) frequencies

[Doug Anderson]

Viresh,

On Wed, May 21, 2014 at 1:59 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> @@ -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);
>
> +               /*
> +                * Failed after setting to intermediate freq? Driver should have
> +                * reverted back to initial frequency and so should we. Check
> +                * here for intermediate_freq instead of get_intermediate, in
> +                * case we have't switched to intermediate freq at all.
> +                */
> +               if (unlikely(retval && intermediate_freq)) {
> +                       freqs.old = intermediate_freq;
> +                       freqs.new = policy->restore_freq;
> +                       cpufreq_freq_transition_begin(policy, &freqs);
> +                       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.

-Doug

Re: [PATCH V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks

[Stephen Warren]

On 05/21/2014 02:59 AM, Viresh Kumar wrote:
> 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.

This patch breaks Tegra. The reason is below.

> diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c

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

> +{
> +	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 ...

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

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

> +
> +	ret = clk_set_rate(pll_x_clk, rate * 1000);
> +	/* Restore to earlier frequency on error, i.e. pll_x */
>  	if (ret)
> -		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.

Re: [PATCH V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks

[Stephen Warren]

On 05/22/2014 10:05 PM, Viresh Kumar wrote:
> On 22 May 2014 22:09, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> 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 ..

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

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

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.

Re: [PATCH V4 2/3] cpufreq: add support for intermediate (stable) frequencies

[Viresh Kumar]

On 29 May 2014 01:10, Doug Anderson <dianders@chromium.org> wrote:
> 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...

Correct !!

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

I am waiting for Stephen's reply on the other thread to close this issue.

Re: [PATCH V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks

[Viresh Kumar]

On 29 May 2014 23:10, Stephen Warren <swarren@wwwdotorg.org> wrote:
> This patch breaks Tegra. The reason is below.

Lets see what blunder I made :)

>> diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
>
>> -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)

Sure.

>> +{
>> +     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 ...
>
>> +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...
>
>> @@ -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 ...
>
>> +
>> +     ret = clk_set_rate(pll_x_clk, rate * 1000);
>> +     /* Restore to earlier frequency on error, i.e. pll_x */
>>       if (ret)
>> -             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.

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

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

Ok, will check what suits best.

Re: [PATCH V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks

[Stephen Warren]

On 05/29/2014 07:56 PM, Viresh Kumar wrote:
> On 29 May 2014 23:10, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> This patch breaks Tegra. The reason is below.
...
>>> +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.

Re: [PATCH V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks

[Viresh Kumar]

On 29 May 2014 23:12, Stephen Warren <swarren@wwwdotorg.org> wrote:

> E.g. in the following code after the series is applied:
>
>>       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);
>
> 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.

This was the case here with the existing driver as well. CPUFreq core
expects this today as well and so sends reverse notification in case of
failures.

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

Another example would be useful..

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

It wasn't in the existing code as well and so left it as is..

Re: [PATCH V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks

[Viresh Kumar]

[-- Attachment #1: Type: text/plain, Size: 7435 bytes --]

On 30 May 2014 21:56, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 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.

Okay, that was very helpful..

What about this ? (Attached for testing) :

Author: Viresh Kumar <viresh.kumar@linaro.org>
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.

    Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/tegra-cpufreq.c | 97
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------
 1 file changed, 62 insertions(+), 35 deletions(-)

diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
index 6e774c6..25c145a 100644
--- a/drivers/cpufreq/tegra-cpufreq.c
+++ b/drivers/cpufreq/tegra-cpufreq.c
@@ -45,46 +45,51 @@ static struct clk *cpu_clk;
 static struct clk *pll_x_clk;
 static struct clk *pll_p_clk;
 static struct clk *emc_clk;
+static int pll_p_clk_count;

-static int tegra_cpu_clk_set_rate(unsigned long rate)
+static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
+                                          unsigned int index)
+{
+       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;
+
+       return ifreq;
+}
+
+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
+        * off when we move the cpu off of it as enabling it again while we
+        * switch to it from tegra_target() would take additional time. Though
+        * when target-freq is intermediate freq, we don't need to take this
+        * reference.
         */
        clk_prepare_enable(pll_x_clk);

        ret = clk_set_parent(cpu_clk, pll_p_clk);
-       if (ret) {
-               pr_err("Failed to switch cpu to clock pll_p\n");
-               goto out;
-       }
-
-       if (rate == clk_get_rate(pll_p_clk))
-               goto out;
-
-       ret = clk_set_rate(pll_x_clk, rate);
-       if (ret) {
-               pr_err("Failed to change pll_x to %lu\n", rate);
-               goto out;
-       }
-
-       ret = clk_set_parent(cpu_clk, pll_x_clk);
-       if (ret) {
-               pr_err("Failed to switch cpu to clock pll_x\n");
-               goto out;
-       }
+       if (ret)
+               clk_disable_unprepare(pll_x_clk);
+       else
+               pll_p_clk_count++;

-out:
-       clk_disable_unprepare(pll_x_clk);
        return ret;
 }

 static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
 {
        unsigned long rate = freq_table[index].frequency;
+       unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000;
        int ret = 0;

        /*
@@ -98,10 +103,30 @@ 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, don't need to take extra reference to pll_x_clk
+        * as it isn't used anymore.
+        */
+       if (rate == ifreq)
+               return clk_set_parent(cpu_clk, pll_p_clk);
+
+       ret = clk_set_rate(pll_x_clk, rate * 1000);
+       /* Restore to earlier frequency on error, i.e. pll_x */
        if (ret)
-               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);
+
+       /*
+        * Drop count to pll_x clock only if we switched to intermediate freq
+        * earlier while transitioning to a target frequency.
+        */
+       if (pll_p_clk_count) {
+               clk_disable_unprepare(pll_x_clk);
+               pll_p_clk_count--;
+       }

        return ret;
 }
@@ -137,16 +162,18 @@ static int tegra_cpu_exit(struct cpufreq_policy *policy)
 }

 static struct cpufreq_driver tegra_cpufreq_driver = {
-       .flags          = CPUFREQ_NEED_INITIAL_FREQ_CHECK,
-       .verify         = cpufreq_generic_frequency_table_verify,
-       .target_index   = tegra_target,
-       .get            = cpufreq_generic_get,
-       .init           = tegra_cpu_init,
-       .exit           = tegra_cpu_exit,
-       .name           = "tegra",
-       .attr           = cpufreq_generic_attr,
+       .flags                  = CPUFREQ_NEED_INITIAL_FREQ_CHECK,
+       .verify                 = cpufreq_generic_frequency_table_verify,
+       .get_intermediate       = tegra_get_intermediate,
+       .target_intermediate    = tegra_target_intermediate,
+       .target_index           = tegra_target,
+       .get                    = cpufreq_generic_get,
+       .init                   = tegra_cpu_init,
+       .exit                   = tegra_cpu_exit,
+       .name                   = "tegra",
+       .attr                   = cpufreq_generic_attr,
 #ifdef CONFIG_PM
-       .suspend        = cpufreq_generic_suspend,
+       .suspend                = cpufreq_generic_suspend,
 #endif
 };

[-- Attachment #2: 0001-cpufreq-Tegra-implement-intermediate-frequency-callb.patch --]
[-- Type: text/x-patch, Size: 4861 bytes --]

From fb2d82a52d95bbd20e4eb75b3b79f74efa4c1a75 Mon Sep 17 00:00:00 2001
Message-Id: <fb2d82a52d95bbd20e4eb75b3b79f74efa4c1a75.1401703573.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Fri, 16 May 2014 14:22:40 +0530
Subject: [PATCH] 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.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/tegra-cpufreq.c | 97 ++++++++++++++++++++++++++---------------
 1 file changed, 62 insertions(+), 35 deletions(-)

diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
index 6e774c6..25c145a 100644
--- a/drivers/cpufreq/tegra-cpufreq.c
+++ b/drivers/cpufreq/tegra-cpufreq.c
@@ -45,46 +45,51 @@ static struct clk *cpu_clk;
 static struct clk *pll_x_clk;
 static struct clk *pll_p_clk;
 static struct clk *emc_clk;
+static int pll_p_clk_count;
 
-static int tegra_cpu_clk_set_rate(unsigned long rate)
+static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy,
+					   unsigned int index)
+{
+	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;
+
+	return ifreq;
+}
+
+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
+	 * off when we move the cpu off of it as enabling it again while we
+	 * switch to it from tegra_target() would take additional time. Though
+	 * when target-freq is intermediate freq, we don't need to take this
+	 * reference.
 	 */
 	clk_prepare_enable(pll_x_clk);
 
 	ret = clk_set_parent(cpu_clk, pll_p_clk);
-	if (ret) {
-		pr_err("Failed to switch cpu to clock pll_p\n");
-		goto out;
-	}
-
-	if (rate == clk_get_rate(pll_p_clk))
-		goto out;
-
-	ret = clk_set_rate(pll_x_clk, rate);
-	if (ret) {
-		pr_err("Failed to change pll_x to %lu\n", rate);
-		goto out;
-	}
-
-	ret = clk_set_parent(cpu_clk, pll_x_clk);
-	if (ret) {
-		pr_err("Failed to switch cpu to clock pll_x\n");
-		goto out;
-	}
+	if (ret)
+		clk_disable_unprepare(pll_x_clk);
+	else
+		pll_p_clk_count++;
 
-out:
-	clk_disable_unprepare(pll_x_clk);
 	return ret;
 }
 
 static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
 {
 	unsigned long rate = freq_table[index].frequency;
+	unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000;
 	int ret = 0;
 
 	/*
@@ -98,10 +103,30 @@ 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, don't need to take extra reference to pll_x_clk
+	 * as it isn't used anymore.
+	 */
+	if (rate == ifreq)
+		return clk_set_parent(cpu_clk, pll_p_clk);
+
+	ret = clk_set_rate(pll_x_clk, rate * 1000);
+	/* Restore to earlier frequency on error, i.e. pll_x */
 	if (ret)
-		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);
+
+	/*
+	 * Drop count to pll_x clock only if we switched to intermediate freq
+	 * earlier while transitioning to a target frequency.
+	 */
+	if (pll_p_clk_count) {
+		clk_disable_unprepare(pll_x_clk);
+		pll_p_clk_count--;
+	}
 
 	return ret;
 }
@@ -137,16 +162,18 @@ static int tegra_cpu_exit(struct cpufreq_policy *policy)
 }
 
 static struct cpufreq_driver tegra_cpufreq_driver = {
-	.flags		= CPUFREQ_NEED_INITIAL_FREQ_CHECK,
-	.verify		= cpufreq_generic_frequency_table_verify,
-	.target_index	= tegra_target,
-	.get		= cpufreq_generic_get,
-	.init		= tegra_cpu_init,
-	.exit		= tegra_cpu_exit,
-	.name		= "tegra",
-	.attr		= cpufreq_generic_attr,
+	.flags			= CPUFREQ_NEED_INITIAL_FREQ_CHECK,
+	.verify			= cpufreq_generic_frequency_table_verify,
+	.get_intermediate	= tegra_get_intermediate,
+	.target_intermediate	= tegra_target_intermediate,
+	.target_index		= tegra_target,
+	.get			= cpufreq_generic_get,
+	.init			= tegra_cpu_init,
+	.exit			= tegra_cpu_exit,
+	.name			= "tegra",
+	.attr			= cpufreq_generic_attr,
 #ifdef CONFIG_PM
-	.suspend	= cpufreq_generic_suspend,
+	.suspend		= cpufreq_generic_suspend,
 #endif
 };
 
-- 
2.0.0.rc2

Re: [PATCH V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks

[Stephen Warren]

On 06/02/2014 04:06 AM, Viresh Kumar wrote:
> On 30 May 2014 21:56, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> ... [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 <viresh.kumar@linaro.org>
> 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.

Tested-by: Stephen Warren <swarren@nvidia.com>

I'd prefer a couple of changes though:

a) Rename "pll_p_clk_count" to better describe what it represents. It
represents the fact that pll_x has been prepare_enabled, so why not call
it "pll_x_prepared"?

b) I think it should be a Boolean not an integer; there should never be
a case where the value is not 0 or 1. The only way that could happen is
if the cpufreq core called tegra_target_intermediate() out of sequence
too many times.