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

[PATCH V2 0/3] 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.

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

@Stephen: Can you please test this? I haven't as I didn't had a board. :)

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 | 19 +++++++-
 drivers/cpufreq/cpufreq.c              | 82 ++++++++++++++++++++++++----------
 drivers/cpufreq/tegra-cpufreq.c        | 81 ++++++++++++++++-----------------
 include/linux/cpufreq.h                | 15 +++++++
 4 files changed, 128 insertions(+), 69 deletions(-)

-- 
2.0.0.rc2

[PATCH V2 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.

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

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..dd64602 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		Uset 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 V2 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.

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

diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
index 6e774c6..10b29ec 100644
--- a/drivers/cpufreq/tegra-cpufreq.c
+++ b/drivers/cpufreq/tegra-cpufreq.c
@@ -46,9 +46,33 @@ 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)
 {
-	int ret;
+	return clk_get_rate(pll_p_clk);
+}
+
+static int
+tegra_target_intermediate(struct cpufreq_policy *policy, unsigned int frequency)
+{
+	return clk_set_parent(cpu_clk, pll_p_clk);
+}
+
+static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
+{
+	unsigned long rate = freq_table[index].frequency;
+	int ret = 0;
+
+	/*
+	 * Vote on memory bus frequency based on cpu frequency
+	 * This sets the minimum frequency, display or avp may request higher
+	 */
+	if (rate >= 816000)
+		clk_set_rate(emc_clk, 600000000); /* cpu 816 MHz, emc max */
+	else if (rate >= 456000)
+		clk_set_rate(emc_clk, 300000000); /* cpu 456 MHz, emc 150Mhz */
+	else
+		clk_set_rate(emc_clk, 100000000);  /* emc 50Mhz */
 
 	/*
 	 * Take an extra reference to the main pll so it doesn't turn
@@ -56,12 +80,6 @@ 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;
 
@@ -72,36 +90,11 @@ static int tegra_cpu_clk_set_rate(unsigned long rate)
 	}
 
 	ret = clk_set_parent(cpu_clk, pll_x_clk);
-	if (ret) {
+	if (ret)
 		pr_err("Failed to switch cpu to clock pll_x\n");
-		goto out;
-	}
 
 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;
-	int ret = 0;
-
-	/*
-	 * Vote on memory bus frequency based on cpu frequency
-	 * This sets the minimum frequency, display or avp may request higher
-	 */
-	if (rate >= 816000)
-		clk_set_rate(emc_clk, 600000000); /* cpu 816 MHz, emc max */
-	else if (rate >= 456000)
-		clk_set_rate(emc_clk, 300000000); /* cpu 456 MHz, emc 150Mhz */
-	else
-		clk_set_rate(emc_clk, 100000000);  /* emc 50Mhz */
-
-	ret = tegra_cpu_clk_set_rate(rate * 1000);
-	if (ret)
-		pr_err("cpu-tegra: Failed to set cpu frequency to %lu kHz\n",
-			rate);
 
 	return ret;
 }
@@ -137,16 +130,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 V2 0/3] cpufreq: add support for intermediate (stable)

[Thomas Abraham]

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

Hi Viresh,

In the given example above, the reworked implementation of cpufreq for
exynos maintains the transition frequency at 800MHz / 4 = 200MHz by
using a clock divider. So the transition frequency is ensured to be
less than or equal to the pre-transition cpu clock frequency.

Thanks,
Thomas.

>
> 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.
>
> 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().
>
> @Stephen: Can you please test this? I haven't as I didn't had a board. :)
>
> 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 | 19 +++++++-
>  drivers/cpufreq/cpufreq.c              | 82 ++++++++++++++++++++++++----------
>  drivers/cpufreq/tegra-cpufreq.c        | 81 ++++++++++++++++-----------------
>  include/linux/cpufreq.h                | 15 +++++++
>  4 files changed, 128 insertions(+), 69 deletions(-)
>
> --
> 2.0.0.rc2
>
>
> _______________________________________________
> linaro-kernel mailing list
> linaro-kernel@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-kernel

Re: [PATCH V2 0/3] cpufreq: add support for intermediate (stable)

[Viresh Kumar]

On 16 May 2014 15:39, Thomas Abraham <ta.omasab@gmail.com> wrote:
> In the given example above, the reworked implementation of cpufreq for
> exynos maintains the transition frequency at 800MHz / 4 = 200MHz by
> using a clock divider. So the transition frequency is ensured to be
> less than or equal to the pre-transition cpu clock frequency.

Thanks for the information. But I think this patchset is still required for
many platforms. Anyway these extra notifications must be sent for exynos
as well.

Re: [PATCH V2 0/3] cpufreq: add support for intermediate (stable)

[Doug Anderson]

On Fri, May 16, 2014 at 3:10 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 16 May 2014 15:39, Thomas Abraham <ta.omasab@gmail.com> wrote:
>> In the given example above, the reworked implementation of cpufreq for
>> exynos maintains the transition frequency at 800MHz / 4 = 200MHz by
>> using a clock divider. So the transition frequency is ensured to be
>> less than or equal to the pre-transition cpu clock frequency.
>
> Thanks for the information. But I think this patchset is still required for
> many platforms. Anyway these extra notifications must be sent for exynos
> as well.

Right, so I think on exynos no functionality will be broken once
Thomas's cpufreq-cpu0 change lands (udelay will only run long, never
short).  ...but from the purist standpoint we will be transitioning
from 1.6 GHz => 800 MHz => 1.7 GHz without any notification about the
800 MHz.   You could imagine someone registering for cpufreq
notifications that would care about the 800MHz transition.

...so it seems like we could wait for Thomas's patches to land as-is
(since they make things better) and then atop that see about adding
support for intermediate frequencies to cpufreq-cpu0.

-Doug

Re: [PATCH V2 0/3] cpufreq: add support for intermediate (stable)

[Viresh Kumar]

On 16 May 2014 20:50, Doug Anderson <dianders@chromium.org> wrote:
> Right, so I think on exynos no functionality will be broken once
> Thomas's cpufreq-cpu0 change lands (udelay will only run long, never
> short).  ...but from the purist standpoint we will be transitioning
> from 1.6 GHz => 800 MHz => 1.7 GHz without any notification about the
> 800 MHz.   You could imagine someone registering for cpufreq
> notifications that would care about the 800MHz transition.
>
> ...so it seems like we could wait for Thomas's patches to land as-is
> (since they make things better) and then atop that see about adding
> support for intermediate frequencies to cpufreq-cpu0.

Hmm, don't know. I think these patches aren't aimed at solving exynos's
problem but rather a general solution which must have already been there.

If some platform can work without it then its fine, but otherwise they should
use it, even if udelay does work for them..

So, I would propose to go ahead with these patches in linux-next and lets
see who all would use it.

Re: [PATCH V2 0/3] cpufreq: add support for intermediate (stable)

[Doug Anderson]

Viresh,

On Fri, May 16, 2014 at 8:26 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 16 May 2014 20:50, Doug Anderson <dianders@chromium.org> wrote:
>> Right, so I think on exynos no functionality will be broken once
>> Thomas's cpufreq-cpu0 change lands (udelay will only run long, never
>> short).  ...but from the purist standpoint we will be transitioning
>> from 1.6 GHz => 800 MHz => 1.7 GHz without any notification about the
>> 800 MHz.   You could imagine someone registering for cpufreq
>> notifications that would care about the 800MHz transition.
>>
>> ...so it seems like we could wait for Thomas's patches to land as-is
>> (since they make things better) and then atop that see about adding
>> support for intermediate frequencies to cpufreq-cpu0.
>
> Hmm, don't know. I think these patches aren't aimed at solving exynos's
> problem but rather a general solution which must have already been there.
>
> If some platform can work without it then its fine, but otherwise they should
> use it, even if udelay does work for them..
>
> So, I would propose to go ahead with these patches in linux-next and lets
> see who all would use it.

Ah.  I wasn't suggesting to wait on your patches.  I think it's fine
to get your patches landed and to get Thomas's patches landed (without
actually intermediate frequencies).  ...and then both sets have landed
then we can modify cpufreq-cpu0 / exynos to actually use the
intermediate freq.

-Doug

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

[Stephen Warren]

On 05/16/2014 03:07 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: 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().

> diff --git a/Documentation/cpu-freq/cpu-drivers.txt b/Documentation/cpu-freq/cpu-drivers.txt

> +cpufreq_driver.get_intermediate
> +and target_intermediate		Uset to switch to stable frequency while
> +				changing CPU frequency.

s/Uset/Used.

> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h

> @@ -226,6 +226,21 @@ struct cpufreq_driver {

> +	unsigned int (*get_intermediate)(struct cpufreq_policy *policy,
> +					 unsigned int index);

Should get_intermediate be passed a struct cpufreq_freqs freqs rather
than just the target index? That way, if the intermediate frequency
varies depending on old/new frequencies, then the driver won't have to
go look up the current frequency in order to implement that logic.

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

[Stephen Warren]

On 05/16/2014 03:07 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.

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

> +static int
> +tegra_target_intermediate(struct cpufreq_policy *policy, unsigned int frequency)
> +{
> +	return clk_set_parent(cpu_clk, pll_p_clk);
> +}

I think you also need to move the following code from
tegra_cpu_clk_set_rate() to the start of tegra_target_intermediate().
Otherwise, pll_x will turn off, which judging by the comment in
tegra_cpu_clk_set_rate(), shouldn't be allowed to happen:

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

I'll go try this version anyway in a minute...

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

[Stephen Warren]

On 05/16/2014 03:07 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.

You need to squash in the patch below in order for this series to work.
Once that's done,

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

>     Signed-off-by: Stephen Warren <swarren@nvidia.com>
> 
> diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
> index 10b29ec99bdc..c04fec02ac6a 100644
> --- a/drivers/cpufreq/tegra-cpufreq.c
> +++ b/drivers/cpufreq/tegra-cpufreq.c
> @@ -49,13 +49,22 @@ static struct clk *emc_clk;
>  static unsigned int
>  tegra_get_intermediate(struct cpufreq_policy *policy, unsigned int index)
>  {
> -       return clk_get_rate(pll_p_clk);
> +       return clk_get_rate(pll_p_clk) / 1000; /* kHz */
>  }
>  
>  static int
>  tegra_target_intermediate(struct cpufreq_policy *policy, unsigned int frequency)
>  {
> +       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
> +        */
> +       clk_prepare_enable(pll_x_clk);
> +
>         return clk_set_parent(cpu_clk, pll_p_clk);
> +       /* FIXME: if error, remove pll_x reference */
>  }
>  
>  static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
> @@ -74,16 +83,10 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
>         else
>                 clk_set_rate(emc_clk, 100000000);  /* emc 50Mhz */
>  
> -       /*
> -        * 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);
> -
>         if (rate == clk_get_rate(pll_p_clk))
>                 goto out;
>  
> -       ret = clk_set_rate(pll_x_clk, rate);
> +       ret = clk_set_rate(pll_x_clk, rate * 1000);
>         if (ret) {
>                 pr_err("Failed to change pll_x to %lu\n", rate);
>                 goto out;

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

[Viresh Kumar]

On 17 May 2014 00:20, Stephen Warren <swarren@wwwdotorg.org> wrote:
> s/Uset/Used.

:(

>> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
>> +     unsigned int (*get_intermediate)(struct cpufreq_policy *policy,
>> +                                      unsigned int index);
>
> Should get_intermediate be passed a struct cpufreq_freqs freqs rather
> than just the target index? That way, if the intermediate frequency
> varies depending on old/new frequencies, then the driver won't have to
> go look up the current frequency in order to implement that logic.

That can be done by simply doing policy->cur and we don't require to
get it from hardware again. So, probably should stay as is.

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

[Viresh Kumar]

On 17 May 2014 01:09, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>  static int
>>  tegra_target_intermediate(struct cpufreq_policy *policy, unsigned int frequency)
>>  {
>> +       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
>> +        */
>> +       clk_prepare_enable(pll_x_clk);
>> +
>>         return clk_set_parent(cpu_clk, pll_p_clk);
>> +       /* FIXME: if error, remove pll_x reference */

Fixed this as well