[PATCH V3 0/4] cpufreq: add support for intermediate (stable)

[PATCH V3 0/4] cpufreq: add support for intermediate (stable)

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

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

NOTE: Once set to intermediate frequency, driver isn't expected to fail for the
following ->target_index() call, if it fails core will issue a WARN().

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 (4):
  cpufreq: handle calls to ->target_index() in separate routine
  cpufreq: add support for intermediate (stable) frequencies
  cpufreq: Tegra: drop wrapper around tegra_update_cpu_speed()
  cpufreq: Tegra: implement intermediate frequency callbacks

 Documentation/cpu-freq/cpu-drivers.txt | 19 +++++++-
 drivers/cpufreq/cpufreq.c              | 82 ++++++++++++++++++++++++----------
 drivers/cpufreq/tegra-cpufreq.c        | 80 ++++++++++++++++-----------------
 include/linux/cpufreq.h                | 15 +++++++
 4 files changed, 129 insertions(+), 67 deletions(-)

-- 
2.0.0.rc2

[PATCH V3 1/4] 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>
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..9bf12a2 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 V3 2/4] 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: Once set to intermediate frequency, driver isn't expected to fail for the
following ->target_index() call, if it fails core will issue a WARN().

Tested-by: Stephen Warren <swarren@nvidia.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 Documentation/cpu-freq/cpu-drivers.txt | 19 ++++++++++++++--
 drivers/cpufreq/cpufreq.c              | 40 +++++++++++++++++++++++++++-------
 include/linux/cpufreq.h                | 15 +++++++++++++
 3 files changed, 64 insertions(+), 10 deletions(-)

diff --git a/Documentation/cpu-freq/cpu-drivers.txt b/Documentation/cpu-freq/cpu-drivers.txt
index b045fe5..b1bdb8a 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,
@@ -179,7 +184,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 +195,16 @@ 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().
 
 
 2. Frequency Table Helpers
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 9bf12a2..f38f2f2 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1819,27 +1819,50 @@ EXPORT_SYMBOL(cpufreq_unregister_notifier);
 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};
 	int retval = -EINVAL;
 	bool notify;
 
 	notify = !(cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION);
+	if (!notify)
+		goto skip_notify;
 
-	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) {
+		freqs.new = cpufreq_driver->get_intermediate(policy, index);
 
-		pr_debug("%s: cpu: %d, oldfreq: %u, new freq: %u\n",
+		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);
+		retval = cpufreq_driver->target_intermediate(policy, freqs.new);
+		cpufreq_freq_transition_end(policy, &freqs, retval);
+
+		if (retval) {
+			pr_err("%s: Failed to change to intermediate frequency: %d\n",
+				__func__, retval);
+			return retval;
+		}
+
+		/* Set intermediate as old freq */
+		freqs.old = 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);
+
+	cpufreq_freq_transition_begin(policy, &freqs);
+
+skip_notify:
 	retval = cpufreq_driver->target_index(policy, index);
-	if (retval)
+	if (retval) {
+		/* We shouldn't fail after setting intermediate freq */
+		WARN_ON(cpufreq_driver->get_intermediate);
 		pr_err("%s: Failed to change cpu frequency: %d\n",
 				__func__, retval);
+	}
 
 	if (notify)
 		cpufreq_freq_transition_end(policy, &freqs, retval);
@@ -2361,7 +2384,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..bfcba11 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -226,6 +226,21 @@ struct cpufreq_driver {
 				 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().
+	 */
+	unsigned int (*get_intermediate)(struct cpufreq_policy *policy,
+					 unsigned int index);
+	int	(*target_intermediate)(struct cpufreq_policy *policy,
+				       unsigned int frequency);
 
 	/* should be defined, if possible */
 	unsigned int	(*get)	(unsigned int cpu);
-- 
2.0.0.rc2

[PATCH V3 3/4] cpufreq: Tegra: drop wrapper around tegra_update_cpu_speed()

[Viresh Kumar]

Tegra has implemented an unnecessary wrapper over tegra_update_cpu_speed(), i.e.
tegra_target(), which wasn't doing anything apart of calling
tegra_update_cpu_speed(). Get rid of that and use tegra_target() directly.

Tested-by: Stephen Warren <swarren@nvidia.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/tegra-cpufreq.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
index 63f0059..6e774c6 100644
--- a/drivers/cpufreq/tegra-cpufreq.c
+++ b/drivers/cpufreq/tegra-cpufreq.c
@@ -82,9 +82,9 @@ out:
 	return ret;
 }
 
-static int tegra_update_cpu_speed(struct cpufreq_policy *policy,
-		unsigned long rate)
+static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
 {
+	unsigned long rate = freq_table[index].frequency;
 	int ret = 0;
 
 	/*
@@ -106,11 +106,6 @@ static int tegra_update_cpu_speed(struct cpufreq_policy *policy,
 	return ret;
 }
 
-static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
-{
-	return tegra_update_cpu_speed(policy, freq_table[index].frequency);
-}
-
 static int tegra_cpu_init(struct cpufreq_policy *policy)
 {
 	int ret;
-- 
2.0.0.rc2

[PATCH V3 4/4] 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.

Tested-by: Stephen Warren <swarren@nvidia.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/tegra-cpufreq.c | 71 +++++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 34 deletions(-)

diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
index 6e774c6..fc66442 100644
--- a/drivers/cpufreq/tegra-cpufreq.c
+++ b/drivers/cpufreq/tegra-cpufreq.c
@@ -46,10 +46,19 @@ 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)
+{
+	return clk_get_rate(pll_p_clk) / 1000; /* kHz */
+}
+
+static int
+tegra_target_intermediate(struct cpufreq_policy *policy, unsigned int frequency)
 {
 	int ret;
 
+	WARN_ON(frequency != clk_get_rate(pll_p_clk) / 1000);
+
 	/*
 	 * Take an extra reference to the main pll so it doesn't turn
 	 * off when we move the cpu off of it
@@ -57,28 +66,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 +88,21 @@ 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);
+	if (rate == clk_get_rate(pll_p_clk))
+		goto out;
+
+	ret = clk_set_rate(pll_x_clk, rate * 1000);
+	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("cpu-tegra: Failed to set cpu frequency to %lu kHz\n",
-			rate);
+		pr_err("Failed to switch cpu to clock pll_x\n");
+
+out:
+	clk_disable_unprepare(pll_x_clk);
 
 	return ret;
 }
@@ -137,16 +138,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 V3 1/4] cpufreq: handle calls to ->target_index() in separate routine

[Doug Anderson]

Viresh,

On Fri, May 16, 2014 at 9:51 PM, Viresh Kumar <viresh.kumar@linaro.org> 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>
> 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..9bf12a2 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;

I'm not sure that this shuffling was really necessary, but I guess it
doesn't hurt.  ...and I guess a CL that's shuffling code anyway is the
place to put it...

Reviewed-by: Doug Anderson <dianders@chromium.org>

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

[Doug Anderson]

Viresh,

On Fri, May 16, 2014 at 9:51 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:

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

Is it worth documenting that if we implement target_intermediate()
that target_index() must not fail?  That means that any failure-prone
things (like setting a regulator) should happen in target_index().


>  2. Frequency Table Helpers
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 9bf12a2..f38f2f2 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1819,27 +1819,50 @@ EXPORT_SYMBOL(cpufreq_unregister_notifier);
>  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};
>         int retval = -EINVAL;
>         bool notify;
>
>         notify = !(cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION);
> +       if (!notify)
> +               goto skip_notify;

I'm personally not a fan of using goto here.  All you're trying to do
is avoiding a level of indentation?  If it really matters that much
then create a sub function.  IMHO goto should generally be reserved
for error handling.


>
> -       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) {
> +               freqs.new = cpufreq_driver->get_intermediate(policy, index);

Would it be worth it to change this to?

intermediate = 0
if (cpufreq_driver->get_intermediate)
  intermediate = cpufreq_driver->get_intermediate();
if (intermediate)

...the idea being that a driver may use an intermediate frequency for
some transitions but not for all.  For instance: on tegra if you
happen to change to the exact clock frequency of the intermediate PLL
it just stays there.  There's no need for two notifications in that
case.  There may be other systems that can optimize some transitions
to totally skip the intermediate stage (maybe you've got an
non-glitching divider somewhere so you can optimize a transition from
1.4GHz to 700MHz to go w/ no intermediate jump).


> -               pr_debug("%s: cpu: %d, oldfreq: %u, new freq: %u\n",
> +               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);
> +               retval = cpufreq_driver->target_intermediate(policy, freqs.new);

It feels like you want to pass in "index" here too, just in case.  A
driver may need to make decisions about other clocks based on the
eventual final frequency.  They could cache it themselves from the
get_intermediate() call, but that seems ugly.


> @@ -2361,7 +2384,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))

I'm OK with the !! trick, but using ^ here seems more confusing.  Why
not use "!="?
  (!!driver_data->get_intermediate != !!driver_data->target_intermediate))

-Doug

Re: [PATCH V3 3/4] cpufreq: Tegra: drop wrapper around tegra_update_cpu_speed()

[Doug Anderson]

Viresh,

On Fri, May 16, 2014 at 9:51 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Tegra has implemented an unnecessary wrapper over tegra_update_cpu_speed(), i.e.
> tegra_target(), which wasn't doing anything apart of calling
> tegra_update_cpu_speed(). Get rid of that and use tegra_target() directly.
>
> Tested-by: Stephen Warren <swarren@nvidia.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/tegra-cpufreq.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)

Reviewed-by: Doug Anderson <dianders@chromium.org>

Re: [PATCH V3 4/4] cpufreq: Tegra: implement intermediate frequency callbacks

[Doug Anderson]

Viresh,

On Fri, May 16, 2014 at 9:51 PM, Viresh Kumar <viresh.kumar@linaro.org> 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.
>
> Tested-by: Stephen Warren <swarren@nvidia.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/tegra-cpufreq.c | 71 +++++++++++++++++++++--------------------
>  1 file changed, 37 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
> index 6e774c6..fc66442 100644
> --- a/drivers/cpufreq/tegra-cpufreq.c
> +++ b/drivers/cpufreq/tegra-cpufreq.c
> @@ -46,10 +46,19 @@ 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)
> +{
> +       return clk_get_rate(pll_p_clk) / 1000; /* kHz */
> +}
> +
> +static int
> +tegra_target_intermediate(struct cpufreq_policy *policy, unsigned int frequency)

Note that in the old code you used to set the "emc" clock before the
transition to the intermediate clock.  Now you don't.  Are you sure
it's OK to change this order?


> @@ -98,10 +88,21 @@ 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);
> +       if (rate == clk_get_rate(pll_p_clk))

Shouldn't this be "rate * 1000 =="?

> +               goto out;
> +
> +       ret = clk_set_rate(pll_x_clk, rate * 1000);
> +       if (ret) {
> +               pr_err("Failed to change pll_x to %lu\n", rate);
> +               goto out;
> +       }

I feel like this should be in tegra_target_intermediate(), since it
could fail (right?).  Essentially we want to be as sure as possible
that tegra_target() doesn't return an error code.


> +
> +       ret = clk_set_parent(cpu_clk, pll_x_clk);

Presumably this won't actually ever fail, since that violates the
rules that target() shouldn't fail if you used an intermediate
frequency.

-Doug

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

[Viresh Kumar]

Doug,

On 20 May 2014 22:18, Doug Anderson <dianders@chromium.org> wrote:

> Is it worth documenting that if we implement target_intermediate()
> that target_index() must not fail?  That means that any failure-prone
> things (like setting a regulator) should happen in target_index().

You meant target_intermediate() is the last line, right ?

Yeah, we can add that..

>>  2. Frequency Table Helpers
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 9bf12a2..f38f2f2 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1819,27 +1819,50 @@ EXPORT_SYMBOL(cpufreq_unregister_notifier);
>>  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};
>>         int retval = -EINVAL;
>>         bool notify;
>>
>>         notify = !(cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION);
>> +       if (!notify)
>> +               goto skip_notify;
>
> I'm personally not a fan of using goto here.  All you're trying to do
> is avoiding a level of indentation?  If it really matters that much
> then create a sub function.  IMHO goto should generally be reserved
> for error handling.

Yeah, I was trying that :) .. Another routine wouldn't be right here
as the POST_NOTIFICATION will be handled in this routine only.

I will try again to see if I can write some better code here, but wouldn't
promise that :)

>> -       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) {
>> +               freqs.new = cpufreq_driver->get_intermediate(policy, index);
>
> Would it be worth it to change this to?
>
> intermediate = 0
> if (cpufreq_driver->get_intermediate)
>   intermediate = cpufreq_driver->get_intermediate();
> if (intermediate)
>
> ...the idea being that a driver may use an intermediate frequency for
> some transitions but not for all.  For instance: on tegra if you
> happen to change to the exact clock frequency of the intermediate PLL
> it just stays there.  There's no need for two notifications in that
> case.  There may be other systems that can optimize some transitions
> to totally skip the intermediate stage (maybe you've got an
> non-glitching divider somewhere so you can optimize a transition from
> 1.4GHz to 700MHz to go w/ no intermediate jump).

Hmm, will try to fix that as well. Looks like a valid point.

>> -               pr_debug("%s: cpu: %d, oldfreq: %u, new freq: %u\n",
>> +               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);
>> +               retval = cpufreq_driver->target_intermediate(policy, freqs.new);
>
> It feels like you want to pass in "index" here too, just in case.  A
> driver may need to make decisions about other clocks based on the
> eventual final frequency.  They could cache it themselves from the
> get_intermediate() call, but that seems ugly.

I had index here initially, but then it looked like they may need to perform
get_intermediate() again from this routine and so sending the intermediate
freq is probably better. So, probably just wait for some drivers which may
need index here ?

>> @@ -2361,7 +2384,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))
>
> I'm OK with the !! trick, but using ^ here seems more confusing.  Why
> not use "!="?
>   (!!driver_data->get_intermediate != !!driver_data->target_intermediate))

Will work as well :)

Re: [PATCH V3 4/4] cpufreq: Tegra: implement intermediate frequency callbacks

[Viresh Kumar]

On 20 May 2014 22:19, Doug Anderson <dianders@chromium.org> wrote:
> Note that in the old code you used to set the "emc" clock before the
> transition to the intermediate clock.  Now you don't.  Are you sure
> it's OK to change this order?

Yeah, I have seen that and as Stephen didn't had any objection to the
change I thought it is probably fine.

>> @@ -98,10 +88,21 @@ 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);
>> +       if (rate == clk_get_rate(pll_p_clk))
>
> Shouldn't this be "rate * 1000 =="?

Yes.

>> +               goto out;
>> +
>> +       ret = clk_set_rate(pll_x_clk, rate * 1000);
>> +       if (ret) {
>> +               pr_err("Failed to change pll_x to %lu\n", rate);
>> +               goto out;
>> +       }
>
> I feel like this should be in tegra_target_intermediate(), since it
> could fail (right?).  Essentially we want to be as sure as possible
> that tegra_target() doesn't return an error code.
>
>
>> +
>> +       ret = clk_set_parent(cpu_clk, pll_x_clk);
>
> Presumably this won't actually ever fail, since that violates the
> rules that target() shouldn't fail if you used an intermediate
> frequency.

Don't know, it looks this rule isn't going to last long.. Let me see
if I can improve it a bit.