OMAP2+: CPUfreq: Allow the CPU scaling when secondary CPUs are offline.

Commit Message

Santosh Shilimkar June 2, 2011, 2:51 p.m. UTC

Current OMAP2PLUS CPUfreq tagret() functions returns when all
the CPU's are not online. This will break DVFS when secondary
CPUs are offlined.

The intention of that check was just avoid CPU frequency change
during the window when CPU becomes online but it's cpufreq_init is
not done yet.

Fix the check accordingly.

Thanks for Nishant Menon <[e-mail]> for reporting it.

Signed-off-by: Santosh Shilimkar <[e-mail]>
Reported-by: Nishanth Menon <[e-mail]>
Tested-by: Vishwanath BS <[e-mail]>
---
There were some question of making the variable atomic etc
in an internal discussion. After some thinking, I realised
there is no need of that since this is just a counter which
maintains the count for online_cpus = cpufreq_init_cpus. 

 arch/arm/mach-omap2/omap2plus-cpufreq.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

Comments

Kevin Hilman June 2, 2011, 11:10 p.m. UTC | #1

Santosh Shilimkar <[e-mail]> writes:

> Current OMAP2PLUS CPUfreq tagret() functions returns when all
> the CPU's are not online. This will break DVFS when secondary
> CPUs are offlined.
>
> The intention of that check was just avoid CPU frequency change
> during the window when CPU becomes online but it's cpufreq_init is
> not done yet.
>
> Fix the check accordingly.
>
> Thanks for Nishant Menon <[e-mail]> for reporting it.
>
> Signed-off-by: Santosh Shilimkar <[e-mail]>
> Reported-by: Nishanth Menon <[e-mail]>
> Tested-by: Vishwanath BS <[e-mail]>
> ---
> There were some question of making the variable atomic etc
> in an internal discussion. After some thinking, I realised
> there is no need of that since this is just a counter which
> maintains the count for online_cpus = cpufreq_init_cpus. 

Since this is init-time only check, the check for every call to
->target() seems excessive.

How about leaving the ->target callback empty until all the CPUs are
online.

Also, how will this handle an SMP kernel booted with maxcpus=1 on the
cmdline?

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Santosh Shilimkar June 3, 2011, 6:26 a.m. UTC | #2

On 6/3/2011 4:40 AM, Kevin Hilman wrote:
> Santosh Shilimkar<[e-mail]>  writes:
>
>> Current OMAP2PLUS CPUfreq tagret() functions returns when all
>> the CPU's are not online. This will break DVFS when secondary
>> CPUs are offlined.
>>
>> The intention of that check was just avoid CPU frequency change
>> during the window when CPU becomes online but it's cpufreq_init is
>> not done yet.
>>
>> Fix the check accordingly.
>>
>> Thanks for Nishant Menon<[e-mail]>  for reporting it.
>>
>> Signed-off-by: Santosh Shilimkar<[e-mail]>
>> Reported-by: Nishanth Menon<[e-mail]>
>> Tested-by: Vishwanath BS<[e-mail]>
>> ---
>> There were some question of making the variable atomic etc
>> in an internal discussion. After some thinking, I realised
>> there is no need of that since this is just a counter which
>> maintains the count for online_cpus = cpufreq_init_cpus.
>
> Since this is init-time only check, the check for every call to
> ->target() seems excessive.
>
> How about leaving the ->target callback empty until all the CPUs are
> online.
>
Can do that as well.

> Also, how will this handle an SMP kernel booted with maxcpus=1 on the
> cmdline?
>
That works because online CPU will be only 1 in that case.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Santosh Shilimkar June 3, 2011, 8:31 a.m. UTC | #3

On 6/3/2011 11:56 AM, Santosh Shilimkar wrote:
> On 6/3/2011 4:40 AM, Kevin Hilman wrote:
>> Santosh Shilimkar<[e-mail]> writes:
>>
>>> Current OMAP2PLUS CPUfreq tagret() functions returns when all
>>> the CPU's are not online. This will break DVFS when secondary
>>> CPUs are offlined.
>>>
>>> The intention of that check was just avoid CPU frequency change
>>> during the window when CPU becomes online but it's cpufreq_init is
>>> not done yet.
>>>
>>> Fix the check accordingly.
>>>
>>> Thanks for Nishant Menon<[e-mail]> for reporting it.
>>>
>>> Signed-off-by: Santosh Shilimkar<[e-mail]>
>>> Reported-by: Nishanth Menon<[e-mail]>
>>> Tested-by: Vishwanath BS<[e-mail]>
>>> ---
>>> There were some question of making the variable atomic etc
>>> in an internal discussion. After some thinking, I realised
>>> there is no need of that since this is just a counter which
>>> maintains the count for online_cpus = cpufreq_init_cpus.
>>
>> Since this is init-time only check, the check for every call to
>> ->target() seems excessive.
>>
>> How about leaving the ->target callback empty until all the CPUs are
>> online.
>>
> Can do that as well.
>
After re-looking at this, seems not straight forward. This check is
not for cpufreqdriver_init time but per-CPU init functions which is
called after the driver is registered to the governor. We don't do 
registration again except the cases where driver is build as a module.

How do we handle that ?

Regards
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Santosh Shilimkar June 3, 2011, 12:05 p.m. UTC | #4

On 6/3/2011 2:01 PM, Santosh Shilimkar wrote:
> On 6/3/2011 11:56 AM, Santosh Shilimkar wrote:
>> On 6/3/2011 4:40 AM, Kevin Hilman wrote:

[...]

>> Can do that as well.
>>
> After re-looking at this, seems not straight forward. This check is
> not for cpufreqdriver_init time but per-CPU init functions which is
> called after the driver is registered to the governor. We don't do
> registration again except the cases where driver is build as a module.
>
> How do we handle that ?
Ignore this questions since mostly we don't need that anymore.
Will post and updated patch.

regards
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
index 33a91ec..909bfcb 100644
--- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
+++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
@@ -44,6 +44,7 @@  static struct cpufreq_frequency_table *freq_table;
 static struct clk *mpu_clk;
 static char *mpu_clk_name;
 static struct device *mpu_dev;
+static int cpus_initialized;
 
 static int omap_verify_speed(struct cpufreq_policy *policy)
 {
@@ -82,7 +83,7 @@  static int omap_target(struct cpufreq_policy *policy,
 	struct cpufreq_freqs freqs;
 
 	/* Changes not allowed until all CPUs are online */
-	if (is_smp() && (num_online_cpus() < NR_CPUS))
+	if (is_smp() && (cpus_initialized < num_online_cpus()))
 		return ret;
 
 	/* Ensure desired rate is within allowed range.  Some govenors
@@ -194,6 +195,8 @@  static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
 		policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
 		cpumask_or(cpumask, cpumask_of(policy->cpu), cpumask);
 		cpumask_copy(policy->cpus, cpumask);
+		cpus_initialized++;
+		smp_wmb();
 	}
 
 	/* FIXME: what's the actual transition time? */
@@ -206,6 +209,10 @@  static int omap_cpu_exit(struct cpufreq_policy *policy)
 {
 	clk_exit_cpufreq_table(&freq_table);
 	clk_put(mpu_clk);
+	if (is_smp()) {
+		cpus_initialized--;
+		smp_wmb();
+	}
 	return 0;
 }