All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] OMAP2+: CPUfreq: Allow the CPU scaling when secondary CPUs are offline.
@ 2011-06-02 14:51 Santosh Shilimkar
  2011-06-02 23:10 ` Kevin Hilman
  2011-06-03  2:44 ` Menon, Nishanth
  0 siblings, 2 replies; 10+ messages in thread
From: Santosh Shilimkar @ 2011-06-02 14:51 UTC (permalink / raw)
  To: linux-omap; +Cc: Santosh Shilimkar

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 <nm@ti.com> for reporting it.

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Reported-by: Nishanth Menon <nm@ti.com>
Tested-by: Vishwanath BS <vishwanath.bs@ti.com>
---
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(-)

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;
 }
 
-- 
1.6.0.4


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] OMAP2+: CPUfreq: Allow the CPU scaling when secondary CPUs are offline.
  2011-06-02 14:51 [PATCH] OMAP2+: CPUfreq: Allow the CPU scaling when secondary CPUs are offline Santosh Shilimkar
@ 2011-06-02 23:10 ` Kevin Hilman
  2011-06-03  6:26   ` Santosh Shilimkar
  2011-06-03  2:44 ` Menon, Nishanth
  1 sibling, 1 reply; 10+ messages in thread
From: Kevin Hilman @ 2011-06-02 23:10 UTC (permalink / raw)
  To: Santosh Shilimkar; +Cc: linux-omap

Santosh Shilimkar <santosh.shilimkar@ti.com> 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 <nm@ti.com> for reporting it.
>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Reported-by: Nishanth Menon <nm@ti.com>
> Tested-by: Vishwanath BS <vishwanath.bs@ti.com>
> ---
> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] OMAP2+: CPUfreq: Allow the CPU scaling when secondary CPUs are offline.
  2011-06-02 14:51 [PATCH] OMAP2+: CPUfreq: Allow the CPU scaling when secondary CPUs are offline Santosh Shilimkar
  2011-06-02 23:10 ` Kevin Hilman
@ 2011-06-03  2:44 ` Menon, Nishanth
  2011-06-03  6:39   ` Santosh Shilimkar
  1 sibling, 1 reply; 10+ messages in thread
From: Menon, Nishanth @ 2011-06-03  2:44 UTC (permalink / raw)
  To: Santosh Shilimkar; +Cc: linux-omap

On Thu, Jun 2, 2011 at 09:51, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
> 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.
is it this requirement a boot requirement or a necessity for
cpufreq_driver.init being called for online cpus? Since we maintain
just a single freq_table... why do we care about multiple cpu_inits?

Anyways, tried testing this and .config with CONFIG_SMP_ON_UP and
USERSPACE. it works with one cpu and does not scale 2 cpus :(

After applying this patch on kevin's cpufreq branch, I added some
prints for logging:
diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c
b/arch/arm/mach-omap2/omap2plus-cpufreq.c
index 909bfcb..89856d5 100644
--- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
+++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
@@ -83,8 +83,13 @@ static int omap_target(struct cpufreq_policy *policy,
 	struct cpufreq_freqs freqs;

 	/* Changes not allowed until all CPUs are online */
-	if (is_smp() && (cpus_initialized < num_online_cpus()))
+	if (is_smp() && (cpus_initialized < num_online_cpus())) {
+		pr_err("%s: cpu %d not ready to go to %d (inits=%d vs
online=%d)\n", __func__,
+                               policy->cpu, target_freq,
cpus_initialized, num_online_cpus());
 		return ret;
+	}
+	pr_err("%s: cpu %d ready to go to %d (inits=%d vs online=%d)\n", __func__,
+		policy->cpu, target_freq, cpus_initialized, num_online_cpus());

 	/* Ensure desired rate is within allowed range.  Some govenors
 	 * (ondemand) will just pass target_freq=0 to get the minimum. */
@@ -197,6 +202,9 @@ static int __cpuinit omap_cpu_init(struct
cpufreq_policy *policy)
 		cpumask_copy(policy->cpus, cpumask);
 		cpus_initialized++;
 		smp_wmb();
+		pr_err("%s: cpu %d cpus_initialized = %d online=%d\n", __func__,
+			policy->cpu, cpus_initialized, num_online_cpus());
+
 	}

 	/* FIXME: what's the actual transition time? */
@@ -212,6 +220,8 @@ static int omap_cpu_exit(struct cpufreq_policy *policy)
 	if (is_smp()) {
 		cpus_initialized--;
 		smp_wmb();
+		pr_err("%s: cpu %d cpus_initialized = %d online=%d\n", __func__,
+			policy->cpu, cpus_initialized, num_online_cpus());
 	}
 	return 0;
 }

on boot, this is what I see:
[    0.421020] omap_cpu_init: cpu 0 cpus_initialized = 1 online=2
[    0.421264] omap_target: cpu 0 not ready to go to 1008000 (inits=1
vs online=2)
[    0.421630] omap_cpu_init: cpu 1 cpus_initialized = 2 online=2
[    0.421691] omap_cpu_exit: cpu 1 cpus_initialized = 1 online=2
...
snip
...
[    2.044128] omap_target: cpu 0 not ready to go to 1008000 (inits=1
vs online=2)
[    2.051849] omap_target: cpu 0 not ready to go to 1008000 (inits=1
vs online=2)
... snip..
...boots up to busybox shell..
/ # head /sys/devices/system/cpu/cpu1/online /sys/devices/system/cpu/cpu0/online

==> /sys/devices/system/cpu/cpu1/online <==
1

==> /sys/devices/system/cpu/cpu0/online <==
1
/ # cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_available_frequencies
300000 600000 800000 1008000
/ # cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq
1008000
/ # echo -n "300000">/sys/devices/system/cpu/cpu0/cpufreq/scaling_setspeed
[  130.257385] omap_target: cpu 0 not ready to go to 300000 (inits=1
vs online=2)
/ # echo -n "0" > /sys/devices/system/cpu/cpu1/online
[  144.749877] CPU1: shutdown
/ # head /sys/devices/system/cpu/cpu1/online /sys/devices/system/cpu/cpu0/online

==> /sys/devices/system/cpu/cpu1/online <==
0

==> /sys/devices/system/cpu/cpu0/online <==
1
/ # echo -n "350000" > /sys/devices/system/cpu/cpu0/cpufreq/scaling_setspeed
[  165.881927] omap_target: cpu 0 ready to go to 350000 (inits=1 vs online=1)
[  165.889526] cpufreq-omap: transition: 1008000 --> 0
/ #
/ # echo -n "1" > /sys/devices/system/cpu/cpu1/online
[  176.469360] CPU1: Booted secondary processor
[  176.469421] CPU1: Unknown IPI message 0x1
[  176.475280] Switched to NOHz mode on CPU #1
[  176.600891] omap_cpu_init: cpu 1 cpus_initialized = 2 online=2
[  176.620178] omap_cpu_exit: cpu 1 cpus_initialized = 1 online=2
[  176.626373] omap_target: cpu 0 not ready to go to 350000 (inits=1
vs online=2)

Regards,
Nishanth Menon

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] OMAP2+: CPUfreq: Allow the CPU scaling when secondary CPUs are offline.
  2011-06-02 23:10 ` Kevin Hilman
@ 2011-06-03  6:26   ` Santosh Shilimkar
  2011-06-03  8:31     ` Santosh Shilimkar
  0 siblings, 1 reply; 10+ messages in thread
From: Santosh Shilimkar @ 2011-06-03  6:26 UTC (permalink / raw)
  To: Kevin Hilman; +Cc: linux-omap

On 6/3/2011 4:40 AM, Kevin Hilman wrote:
> Santosh Shilimkar<santosh.shilimkar@ti.com>  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<nm@ti.com>  for reporting it.
>>
>> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com>
>> Reported-by: Nishanth Menon<nm@ti.com>
>> Tested-by: Vishwanath BS<vishwanath.bs@ti.com>
>> ---
>> 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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] OMAP2+: CPUfreq: Allow the CPU scaling when secondary CPUs are offline.
  2011-06-03  2:44 ` Menon, Nishanth
@ 2011-06-03  6:39   ` Santosh Shilimkar
  2011-06-03 12:04     ` Santosh Shilimkar
  0 siblings, 1 reply; 10+ messages in thread
From: Santosh Shilimkar @ 2011-06-03  6:39 UTC (permalink / raw)
  To: Menon, Nishanth; +Cc: linux-omap

On 6/3/2011 8:14 AM, Menon, Nishanth wrote:
> On Thu, Jun 2, 2011 at 09:51, Santosh Shilimkar
> <santosh.shilimkar@ti.com>  wrote:
>> 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.
> is it this requirement a boot requirement or a necessity for
> cpufreq_driver.init being called for online cpus? Since we maintain
> just a single freq_table... why do we care about multiple cpu_inits?
>
I put a comment after ---
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.
And we need inits on all CPUs to ensure the CPU relation is
set. It's not all about _one_ table.


> Anyways, tried testing this and .config with CONFIG_SMP_ON_UP and
> USERSPACE. it works with one cpu and does not scale 2 cpus :(
>
You mean userspace governor? I don't think why that can happen.
Vishwa tested this and it did worked. We will test this again.

> After applying this patch on kevin's cpufreq branch, I added some
> prints for logging:

[..]

>
>   	/* FIXME: what's the actual transition time? */
> @@ -212,6 +220,8 @@ static int omap_cpu_exit(struct cpufreq_policy *policy)
>   	if (is_smp()) {
>   		cpus_initialized--;
>   		smp_wmb();
> +		pr_err("%s: cpu %d cpus_initialized = %d online=%d\n", __func__,
> +			policy->cpu, cpus_initialized, num_online_cpus());
>   	}
>   	return 0;
>   }
>
> on boot, this is what I see:
> [    0.421020] omap_cpu_init: cpu 0 cpus_initialized = 1 online=2
> [    0.421264] omap_target: cpu 0 not ready to go to 1008000 (inits=1
> vs online=2)
> [    0.421630] omap_cpu_init: cpu 1 cpus_initialized = 2 online=2
> [    0.421691] omap_cpu_exit: cpu 1 cpus_initialized = 1 online=2
This must be becasue hot-plug governor is kicking in here which
is taking CPU1 in and OUT.

> ...
> snip
> ...
> [    2.044128] omap_target: cpu 0 not ready to go to 1008000 (inits=1
> vs online=2)
> [    2.051849] omap_target: cpu 0 not ready to go to 1008000 (inits=1
> vs online=2)

This is expected as well because CPU1 has not really offline yet. And
the code is doing right thing.

> ... snip..
> ...boots up to busybox shell..
> / # head /sys/devices/system/cpu/cpu1/online /sys/devices/system/cpu/cpu0/online
>
> ==>  /sys/devices/system/cpu/cpu1/online<==
> 1
>
> ==>  /sys/devices/system/cpu/cpu0/online<==
> 1
> / # cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_available_frequencies
> 300000 600000 800000 1008000
> / # cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq
> 1008000
> / # echo -n "300000">/sys/devices/system/cpu/cpu0/cpufreq/scaling_setspeed
> [  130.257385] omap_target: cpu 0 not ready to go to 300000 (inits=1
> vs online=2)
> / # echo -n "0">  /sys/devices/system/cpu/cpu1/online
> [  144.749877] CPU1: shutdown
> / # head /sys/devices/system/cpu/cpu1/online /sys/devices/system/cpu/cpu0/online
>
> ==>  /sys/devices/system/cpu/cpu1/online<==
> 0
>
> ==>  /sys/devices/system/cpu/cpu0/online<==
> 1
> / # echo -n "350000">  /sys/devices/system/cpu/cpu0/cpufreq/scaling_setspeed
> [  165.881927] omap_target: cpu 0 ready to go to 350000 (inits=1 vs online=1)
> [  165.889526] cpufreq-omap: transition: 1008000 -->  0
> / #
> / # echo -n "1">  /sys/devices/system/cpu/cpu1/online
> [  176.469360] CPU1: Booted secondary processor
> [  176.469421] CPU1: Unknown IPI message 0x1
> [  176.475280] Switched to NOHz mode on CPU #1
> [  176.600891] omap_cpu_init: cpu 1 cpus_initialized = 2 online=2
> [  176.620178] omap_cpu_exit: cpu 1 cpus_initialized = 1 online=2
> [  176.626373] omap_target: cpu 0 not ready to go to 350000 (inits=1
> vs online=2)
>
All the logs look normal to me. There is nothing wrong here.

regards
Santosh

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] OMAP2+: CPUfreq: Allow the CPU scaling when secondary CPUs are offline.
  2011-06-03  6:26   ` Santosh Shilimkar
@ 2011-06-03  8:31     ` Santosh Shilimkar
  2011-06-03 12:05       ` Santosh Shilimkar
  0 siblings, 1 reply; 10+ messages in thread
From: Santosh Shilimkar @ 2011-06-03  8:31 UTC (permalink / raw)
  To: Kevin Hilman; +Cc: linux-omap

On 6/3/2011 11:56 AM, Santosh Shilimkar wrote:
> On 6/3/2011 4:40 AM, Kevin Hilman wrote:
>> Santosh Shilimkar<santosh.shilimkar@ti.com> 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<nm@ti.com> for reporting it.
>>>
>>> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com>
>>> Reported-by: Nishanth Menon<nm@ti.com>
>>> Tested-by: Vishwanath BS<vishwanath.bs@ti.com>
>>> ---
>>> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] OMAP2+: CPUfreq: Allow the CPU scaling when secondary CPUs are offline.
  2011-06-03  6:39   ` Santosh Shilimkar
@ 2011-06-03 12:04     ` Santosh Shilimkar
  0 siblings, 0 replies; 10+ messages in thread
From: Santosh Shilimkar @ 2011-06-03 12:04 UTC (permalink / raw)
  To: Menon, Nishanth; +Cc: linux-omap

Nishant,

On 6/3/2011 12:09 PM, Santosh Shilimkar wrote:
> On 6/3/2011 8:14 AM, Menon, Nishanth wrote:
>> On Thu, Jun 2, 2011 at 09:51, Santosh Shilimkar
>> <santosh.shilimkar@ti.com> wrote:
>>> 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.
>> is it this requirement a boot requirement or a necessity for
>> cpufreq_driver.init being called for online cpus? Since we maintain
>> just a single freq_table... why do we care about multiple cpu_inits?
>>
> I put a comment after ---
> 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.
> And we need inits on all CPUs to ensure the CPU relation is
> set. It's not all about _one_ table.
>
>
>> Anyways, tried testing this and .config with CONFIG_SMP_ON_UP and
>> USERSPACE. it works with one cpu and does not scale 2 cpus :(
>>
> You mean userspace governor? I don't think why that can happen.
> Vishwa tested this and it did worked. We will test this again.
>
Your observation is right. This patch and earlier tested patch
had one difference. We were not decrementing the counter in the
exit path. I assumed that during boot you have hot-plug
governor selected and later you were switching to user-space.
It wasn't the case because I could reproduce same observation
as you. Sorry for that.

I did some debug on this overall issue. Will send an updated patch.

Regards
Santosh


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] OMAP2+: CPUfreq: Allow the CPU scaling when secondary CPUs are offline.
  2011-06-03  8:31     ` Santosh Shilimkar
@ 2011-06-03 12:05       ` Santosh Shilimkar
  0 siblings, 0 replies; 10+ messages in thread
From: Santosh Shilimkar @ 2011-06-03 12:05 UTC (permalink / raw)
  To: Kevin Hilman; +Cc: linux-omap

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] OMAP2+: CPUfreq: Allow the CPU scaling when secondary CPUs are offline.
  2011-06-02 14:53 Santosh Shilimkar
@ 2011-06-03 10:07 ` Igor Dmitriev
  0 siblings, 0 replies; 10+ messages in thread
From: Igor Dmitriev @ 2011-06-03 10:07 UTC (permalink / raw)
  To: ext Santosh Shilimkar; +Cc: l-o, Kevin Hilman

Hi,

On Thu, 2011-06-02 at 20:23 +0530, ext Santosh Shilimkar wrote:
> Missed Kevin in cc. :(
> Sorry about that.
> 
> -------- Original Message --------
> Subject: [PATCH] OMAP2+: CPUfreq: Allow the CPU scaling when secondary 
> CPUs are offline.
> Date: Thu, 2 Jun 2011 20:21:10 +0530
> From: Santosh Shilimkar <santosh.shilimkar@ti.com>
> To: <linux-omap@vger.kernel.org>
> CC: Santosh Shilimkar <santosh.shilimkar@ti.com>
> 
> 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 <nm@ti.com> for reporting it.
> 
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Reported-by: Nishanth Menon <nm@ti.com>
> Tested-by: Vishwanath BS <vishwanath.bs@ti.com>
> ---
> 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(-)
> 
> 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 */

This comment are no more relevant and should be changed also...

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

Best Regards,
Igor Dmitriev


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] OMAP2+: CPUfreq: Allow the CPU scaling when secondary CPUs are offline.
@ 2011-06-02 14:53 Santosh Shilimkar
  2011-06-03 10:07 ` Igor Dmitriev
  0 siblings, 1 reply; 10+ messages in thread
From: Santosh Shilimkar @ 2011-06-02 14:53 UTC (permalink / raw)
  To: l-o; +Cc: Kevin Hilman

Missed Kevin in cc. :(
Sorry about that.

-------- Original Message --------
Subject: [PATCH] OMAP2+: CPUfreq: Allow the CPU scaling when secondary 
CPUs are offline.
Date: Thu, 2 Jun 2011 20:21:10 +0530
From: Santosh Shilimkar <santosh.shilimkar@ti.com>
To: <linux-omap@vger.kernel.org>
CC: Santosh Shilimkar <santosh.shilimkar@ti.com>

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 <nm@ti.com> for reporting it.

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Reported-by: Nishanth Menon <nm@ti.com>
Tested-by: Vishwanath BS <vishwanath.bs@ti.com>
---
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(-)

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

-- 
1.6.0.4


^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2011-06-03 12:05 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-06-02 14:51 [PATCH] OMAP2+: CPUfreq: Allow the CPU scaling when secondary CPUs are offline Santosh Shilimkar
2011-06-02 23:10 ` Kevin Hilman
2011-06-03  6:26   ` Santosh Shilimkar
2011-06-03  8:31     ` Santosh Shilimkar
2011-06-03 12:05       ` Santosh Shilimkar
2011-06-03  2:44 ` Menon, Nishanth
2011-06-03  6:39   ` Santosh Shilimkar
2011-06-03 12:04     ` Santosh Shilimkar
2011-06-02 14:53 Santosh Shilimkar
2011-06-03 10:07 ` Igor Dmitriev

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.