[PATCH 0/2] omap2+: pm: Fix code syntax and BogoMIPS

[PATCH 0/2] omap2+: pm: Fix code syntax and BogoMIPS

[Sanjeev Premi]

Current implementation was using "goto" to get to a basic
if..else structure - but wasn't too obvious on first few
reads.

Once the code was restructured, the relation between the
calls to cpufreq_scale() and cpufreq_notify_transition()
became obvious. And small trip to drivers/cpufreq/cpufreq.c
revealed double adjustment of the jiffies.

Though the patches may seem unrelated, but as described here
first led to second. Hence, submitting them together.

The BogoMIPS calculation has been tested on OMAP3EVM.

Before
======

[root at OMAP3EVM cpufreq]# cat scaling_available_frequencies
300000 600000 800000 1000000
[root at OMAP3EVM cpufreq]# cat /proc/cpuinfo
Processor       : ARMv7 Processor rev 2 (v7l)
BogoMIPS        : 298.32
Features        : swp half thumb fastmult vfp edsp thumbee neon vfpv3
CPU implementer : 0x41
CPU architecture: 7
CPU variant     : 0x3
CPU part        : 0xc08
CPU revision    : 2

Hardware        : OMAP3 EVM
Revision        : 0020
Serial          : 0000000000000000
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]# echo 800000 > scaling_setspeed
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]# cat /proc/cpuinfo
Processor       : ARMv7 Processor rev 2 (v7l)
BogoMIPS        : 2125.51
Features        : swp half thumb fastmult vfp edsp thumbee neon vfpv3
CPU implementer : 0x41
CPU architecture: 7
CPU variant     : 0x3
CPU part        : 0xc08
CPU revision    : 2

Hardware        : OMAP3 EVM
Revision        : 0020
Serial          : 0000000000000000
[root at OMAP3EVM cpufreq]#

After
=====

[root at OMAP3EVM cpufreq]# cat scaling_available_frequencies
300000 600000 800000 1000000
[root at OMAP3EVM cpufreq]# cat /proc/cpuinfo
Processor       : ARMv7 Processor rev 2 (v7l)
BogoMIPS        : 597.64
Features        : swp half thumb fastmult vfp edsp thumbee neon vfpv3
CPU implementer : 0x41
CPU architecture: 7
CPU variant     : 0x3
CPU part        : 0xc08
CPU revision    : 2

Hardware        : OMAP3 EVM
Revision        : 0020
Serial          : 0000000000000000
[root at OMAP3EVM cpufreq]# echo 300000 > scaling_setspeed
[root at OMAP3EVM cpufreq]# cat /proc/cpuinfo
Processor       : ARMv7 Processor rev 2 (v7l)
BogoMIPS        : 298.32
Features        : swp half thumb fastmult vfp edsp thumbee neon vfpv3
CPU implementer : 0x41
CPU architecture: 7
CPU variant     : 0x3
CPU part        : 0xc08
CPU revision    : 2

Hardware        : OMAP3 EVM
Revision        : 0020
Serial          : 0000000000000000
[root at OMAP3EVM cpufreq]# echo 600000 > scaling_setspeed
[root at OMAP3EVM cpufreq]# cat /proc/cpuinfo
Processor       : ARMv7 Processor rev 2 (v7l)
BogoMIPS        : 597.64
Features        : swp half thumb fastmult vfp edsp thumbee neon vfpv3
CPU implementer : 0x41
CPU architecture: 7
CPU variant     : 0x3
CPU part        : 0xc08
CPU revision    : 2

Hardware        : OMAP3 EVM
Revision        : 0020
Serial          : 0000000000000000
[root at OMAP3EVM cpufreq]# echo 800000 > scaling_setspeed
[root at OMAP3EVM cpufreq]# cat /proc/cpuinfo
Processor       : ARMv7 Processor rev 2 (v7l)
BogoMIPS        : 796.19
Features        : swp half thumb fastmult vfp edsp thumbee neon vfpv3
CPU implementer : 0x41
CPU architecture: 7
CPU variant     : 0x3
CPU part        : 0xc08
CPU revision    : 2

Hardware        : OMAP3 EVM
Revision        : 0020
Serial          : 0000000000000000
[root at OMAP3EVM cpufreq]#

PS: 800MHz and 1GHZ were enabled via out of tree patch - to be submitted soon.


I don't have an OMAP4 board handy. Confirmation that BogoMIPS
calculations are right on the platform will help.

The patchset applied on the 
40d1020 : rebuild PM from branches

Sanjeev Premi (2):
  omap2+: pm: Use if...else instead of goto
  omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

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

-- 
1.7.2.2

[PATCH 1/2] omap2+: pm: Use if...else instead of goto

[Sanjeev Premi]

This patch replaces the use of goto with simple
if...else syntax. No change in functionality.

This also means that the comment describing the
dependency between CONFIG_SMP and calculation
of loops_per_jiffy can be unified.

Signed-off-by: Sanjeev Premi <premi@ti.com>
---
 arch/arm/mach-omap2/omap2plus-cpufreq.c |   59 +++++++++++++-----------------
 1 files changed, 26 insertions(+), 33 deletions(-)

diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
index 1f3b2e1..ce9d534 100644
--- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
+++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
@@ -96,57 +96,50 @@ static int omap_target(struct cpufreq_policy *policy,
 	if (freqs.old == freqs.new && policy->cur == freqs.new)
 		return ret;
 
-	if (!is_smp()) {
-		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
-		goto set_freq;
-	}
-
-	/* notifiers */
-	for_each_cpu(i, policy->cpus) {
-		freqs.cpu = i;
+	/* Notify transitions */
+	if (is_smp()) {
+		for_each_cpu(i, policy->cpus) {
+			freqs.cpu = i;
+			cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+		}
+	} else {
 		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
 	}
 
-set_freq:
 #ifdef CONFIG_CPU_FREQ_DEBUG
 	pr_info("cpufreq-omap: transition: %u --> %u\n", freqs.old, freqs.new);
 #endif
 
 	ret = clk_set_rate(mpu_clk, freqs.new * 1000);
 
-	/*
-	 * Generic CPUFREQ driver jiffy update is under !SMP. So jiffies
-	 * won't get updated when UP machine cpufreq build with
-	 * CONFIG_SMP enabled. Below code is added only to manage that
-	 * scenario
-	 */
 	freqs.new = omap_getspeed(policy->cpu);
-	if (!is_smp()) {
-		loops_per_jiffy =
-			 cpufreq_scale(loops_per_jiffy, freqs.old, freqs.new);
-		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
-		goto skip_lpj;
-	}
 
-#ifdef CONFIG_SMP
 	/*
-	 * Note that loops_per_jiffy is not updated on SMP systems in
-	 * cpufreq driver. So, update the per-CPU loops_per_jiffy value
-	 * on frequency transition. We need to update all dependent CPUs.
+	 * In the generic cpufreq driver jiffies are updated only for
+	 * non-SMP cases. Ensure that jiffies are bing updated for both
+	 * SMP systems and UP systems built with CONFIG_SMP enabled.
 	 */
-	for_each_cpu(i, policy->cpus)
-		per_cpu(cpu_data, i).loops_per_jiffy =
-			cpufreq_scale(per_cpu(cpu_data, i).loops_per_jiffy,
-					freqs.old, freqs.new);
+	if (is_smp()) {
+#ifdef CONFIG_SMP
+		for_each_cpu(i, policy->cpus)
+			per_cpu(cpu_data, i).loops_per_jiffy =
+				cpufreq_scale(per_cpu(cpu_data, i).loops_per_jiffy,
+						freqs.old,
+						freqs.new);
 #endif
+		/* Notify transitions */
+		for_each_cpu(i, policy->cpus) {
+			freqs.cpu = i;
+			cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+		}
+	} else {
+		loops_per_jiffy = cpufreq_scale(loops_per_jiffy,
+					freqs.old, freqs.new);
 
-	/* notifiers */
-	for_each_cpu(i, policy->cpus) {
-		freqs.cpu = i;
+		/* Notify transitions */
 		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
 	}
 
-skip_lpj:
 	return ret;
 }
 
-- 
1.7.2.2

[PATCH 1/2] omap2+: pm: Use if...else instead of goto

[Santosh Shilimkar]

On 6/24/2011 4:05 PM, Sanjeev Premi wrote:
> This patch replaces the use of goto with simple
> if...else syntax. No change in functionality.
>
> This also means that the comment describing the
> dependency between CONFIG_SMP and calculation
> of loops_per_jiffy can be unified.
>
> Signed-off-by: Sanjeev Premi<premi@ti.com>
> ---
Don't see need of this patch as well considering your second patch.
So NAK.

>   arch/arm/mach-omap2/omap2plus-cpufreq.c |   59 +++++++++++++-----------------
>   1 files changed, 26 insertions(+), 33 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
> index 1f3b2e1..ce9d534 100644
> --- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
> +++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
> @@ -96,57 +96,50 @@ static int omap_target(struct cpufreq_policy *policy,
>   	if (freqs.old == freqs.new&&  policy->cur == freqs.new)
>   		return ret;
>
> -	if (!is_smp()) {
> -		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> -		goto set_freq;
> -	}
> -
> -	/* notifiers */
> -	for_each_cpu(i, policy->cpus) {
> -		freqs.cpu = i;
> +	/* Notify transitions */
> +	if (is_smp()) {
> +		for_each_cpu(i, policy->cpus) {
> +			freqs.cpu = i;
> +			cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> +		}
> +	} else {
>   		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
>   	}
>
> -set_freq:
>   #ifdef CONFIG_CPU_FREQ_DEBUG
>   	pr_info("cpufreq-omap: transition: %u -->  %u\n", freqs.old, freqs.new);
>   #endif
>
>   	ret = clk_set_rate(mpu_clk, freqs.new * 1000);
>
> -	/*
> -	 * Generic CPUFREQ driver jiffy update is under !SMP. So jiffies
> -	 * won't get updated when UP machine cpufreq build with
> -	 * CONFIG_SMP enabled. Below code is added only to manage that
> -	 * scenario
> -	 */
>   	freqs.new = omap_getspeed(policy->cpu);
> -	if (!is_smp()) {
> -		loops_per_jiffy =
> -			 cpufreq_scale(loops_per_jiffy, freqs.old, freqs.new);
> -		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> -		goto skip_lpj;
> -	}
>
> -#ifdef CONFIG_SMP
>   	/*
> -	 * Note that loops_per_jiffy is not updated on SMP systems in
> -	 * cpufreq driver. So, update the per-CPU loops_per_jiffy value
> -	 * on frequency transition. We need to update all dependent CPUs.
> +	 * In the generic cpufreq driver jiffies are updated only for
> +	 * non-SMP cases. Ensure that jiffies are bing updated for both
> +	 * SMP systems and UP systems built with CONFIG_SMP enabled.
>   	 */
> -	for_each_cpu(i, policy->cpus)
> -		per_cpu(cpu_data, i).loops_per_jiffy =
> -			cpufreq_scale(per_cpu(cpu_data, i).loops_per_jiffy,
> -					freqs.old, freqs.new);
> +	if (is_smp()) {
> +#ifdef CONFIG_SMP
> +		for_each_cpu(i, policy->cpus)
> +			per_cpu(cpu_data, i).loops_per_jiffy =
> +				cpufreq_scale(per_cpu(cpu_data, i).loops_per_jiffy,
> +						freqs.old,
> +						freqs.new);
>   #endif
> +		/* Notify transitions */
> +		for_each_cpu(i, policy->cpus) {
> +			freqs.cpu = i;
> +			cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> +		}
> +	} else {
> +		loops_per_jiffy = cpufreq_scale(loops_per_jiffy,
> +					freqs.old, freqs.new);
>
> -	/* notifiers */
> -	for_each_cpu(i, policy->cpus) {
> -		freqs.cpu = i;
> +		/* Notify transitions */
>   		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
>   	}
>
> -skip_lpj:
>   	return ret;
>   }
>

[PATCH 1/2] omap2+: pm: Use if...else instead of goto

[Premi, Sanjeev]

> -----Original Message-----
> From: Shilimkar, Santosh 
> Sent: Friday, June 24, 2011 6:18 PM
> To: Premi, Sanjeev
> Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH 1/2] omap2+: pm: Use if...else instead of goto
> 
> On 6/24/2011 4:05 PM, Sanjeev Premi wrote:
> > This patch replaces the use of goto with simple
> > if...else syntax. No change in functionality.
> >
> > This also means that the comment describing the
> > dependency between CONFIG_SMP and calculation
> > of loops_per_jiffy can be unified.
> >
> > Signed-off-by: Sanjeev Premi<premi@ti.com>
> > ---
> Don't see need of this patch as well considering your second patch.
> So NAK.
> 
[sp] May be it isn't needed - but it surely highlights some
     bad code that does find way upstream.

~sanjeev

[PATCH 1/2] omap2+: pm: Use if...else instead of goto

[Santosh Shilimkar]

On 6/24/2011 6:24 PM, Premi, Sanjeev wrote:
>> -----Original Message-----
>> From: Shilimkar, Santosh
>> Sent: Friday, June 24, 2011 6:18 PM
>> To: Premi, Sanjeev
>> Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
>> Subject: Re: [PATCH 1/2] omap2+: pm: Use if...else instead of goto
>>
>> On 6/24/2011 4:05 PM, Sanjeev Premi wrote:
>>> This patch replaces the use of goto with simple
>>> if...else syntax. No change in functionality.
>>>
>>> This also means that the comment describing the
>>> dependency between CONFIG_SMP and calculation
>>> of loops_per_jiffy can be unified.
>>>
>>> Signed-off-by: Sanjeev Premi<premi@ti.com>
>>> ---
>> Don't see need of this patch as well considering your second patch.
>> So NAK.
>>
> [sp] May be it isn't needed - but it surely highlights some
>       bad code that does find way upstream.
>
If you think of using if else makes code better than use
of goto, then I just ignore your comment. Because that means
core scheduler code is also bad as per your remark which makes
use of goto.

Regards
Santosh

[PATCH 1/2] omap2+: pm: Use if...else instead of goto

[Premi, Sanjeev]

> -----Original Message-----
> From: Shilimkar, Santosh 
> Sent: Friday, June 24, 2011 6:29 PM
> To: Premi, Sanjeev
> Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH 1/2] omap2+: pm: Use if...else instead of goto
> 
> On 6/24/2011 6:24 PM, Premi, Sanjeev wrote:
> >> -----Original Message-----
> >> From: Shilimkar, Santosh
> >> Sent: Friday, June 24, 2011 6:18 PM
> >> To: Premi, Sanjeev
> >> Cc: linux-omap at vger.kernel.org; 
> linux-arm-kernel at lists.infradead.org
> >> Subject: Re: [PATCH 1/2] omap2+: pm: Use if...else instead of goto
> >>
> >> On 6/24/2011 4:05 PM, Sanjeev Premi wrote:
> >>> This patch replaces the use of goto with simple
> >>> if...else syntax. No change in functionality.
> >>>
> >>> This also means that the comment describing the
> >>> dependency between CONFIG_SMP and calculation
> >>> of loops_per_jiffy can be unified.
> >>>
> >>> Signed-off-by: Sanjeev Premi<premi@ti.com>
> >>> ---
> >> Don't see need of this patch as well considering your second patch.
> >> So NAK.
> >>
> > [sp] May be it isn't needed - but it surely highlights some
> >       bad code that does find way upstream.
> >
> If you think of using if else makes code better than use
> of goto, then I just ignore your comment. Because that means
> core scheduler code is also bad as per your remark which makes
> use of goto.

[sp] No. It is context based - and in this context goto isn't
     any good... because code it evaluates to simple if..else
     as we "goto" over just another instruction.

> 
> Regards
> Santosh
> 

[PATCH 2/2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Sanjeev Premi]

Currently, loops_per_jiffy is being calculated before calling
cpufreq_notify_transition().

However, generic cpufreq driver adjusts the jiffies as well.
Double adjustment leads to incorrect value being assigned to
loops_per_jiffy.

This manifests as incorrect BogoMIPS in "cat /proc/cpuinfo'

Signed-off-by: Sanjeev Premi <premi@ti.com>
---
 arch/arm/mach-omap2/omap2plus-cpufreq.c |   18 +-----------------
 1 files changed, 1 insertions(+), 17 deletions(-)

diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
index ce9d534..346519e 100644
--- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
+++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
@@ -114,29 +114,13 @@ static int omap_target(struct cpufreq_policy *policy,
 
 	freqs.new = omap_getspeed(policy->cpu);
 
-	/*
-	 * In the generic cpufreq driver jiffies are updated only for
-	 * non-SMP cases. Ensure that jiffies are bing updated for both
-	 * SMP systems and UP systems built with CONFIG_SMP enabled.
-	 */
+	/* Notify transitions */
 	if (is_smp()) {
-#ifdef CONFIG_SMP
-		for_each_cpu(i, policy->cpus)
-			per_cpu(cpu_data, i).loops_per_jiffy =
-				cpufreq_scale(per_cpu(cpu_data, i).loops_per_jiffy,
-						freqs.old,
-						freqs.new);
-#endif
-		/* Notify transitions */
 		for_each_cpu(i, policy->cpus) {
 			freqs.cpu = i;
 			cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
 		}
 	} else {
-		loops_per_jiffy = cpufreq_scale(loops_per_jiffy,
-					freqs.old, freqs.new);
-
-		/* Notify transitions */
 		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
 	}
 
-- 
1.7.2.2

[PATCH 2/2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Russell King - ARM Linux]

On Fri, Jun 24, 2011 at 04:05:42PM +0530, Sanjeev Premi wrote:
> Currently, loops_per_jiffy is being calculated before calling
> cpufreq_notify_transition().
> 
> However, generic cpufreq driver adjusts the jiffies as well.
> Double adjustment leads to incorrect value being assigned to
> loops_per_jiffy.

Are you sure the generic cpufreq driver adjusts the per-cpu loops_per_jiffy
values?  I don't believe it does.

> diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
> index ce9d534..346519e 100644
> --- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
> +++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
> @@ -114,29 +114,13 @@ static int omap_target(struct cpufreq_policy *policy,
>  
>  	freqs.new = omap_getspeed(policy->cpu);
>  
> -	/*
> -	 * In the generic cpufreq driver jiffies are updated only for
> -	 * non-SMP cases. Ensure that jiffies are bing updated for both
> -	 * SMP systems and UP systems built with CONFIG_SMP enabled.
> -	 */
> +	/* Notify transitions */
>  	if (is_smp()) {
> -#ifdef CONFIG_SMP
> -		for_each_cpu(i, policy->cpus)
> -			per_cpu(cpu_data, i).loops_per_jiffy =
> -				cpufreq_scale(per_cpu(cpu_data, i).loops_per_jiffy,
> -						freqs.old,
> -						freqs.new);
> -#endif
> -		/* Notify transitions */

So this is a NAK.  What's also missing is a scaling of loops_per_jiffy
itself here, because with SMP=y the core won't do this for you.

>  		for_each_cpu(i, policy->cpus) {
>  			freqs.cpu = i;
>  			cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
>  		}
>  	} else {
> -		loops_per_jiffy = cpufreq_scale(loops_per_jiffy,
> -					freqs.old, freqs.new);
> -
> -		/* Notify transitions */

This is almost right - the core cpufreq code looks after this when
CONFIG_SMP is not selected.  However, if you're running a kernel built
for SMP on UP, then loops_per_jiffy won't be scaled, so something
needs to be done to cover that case.

Note also that you should scale the loops_per_jiffy against a reference
value, otherwise you'll get an increasing error each time you scale.

So, if we want to do this then we need to store the boot-time lpj and
frequency as the baseline reference, and scale according to that,
much like the core cpufreq code does for loops_per_jiffy for the !SMP
case.

[PATCH 2/2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Premi, Sanjeev]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk] 
> Sent: Friday, June 24, 2011 4:14 PM
> To: Premi, Sanjeev
> Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix 
> loops_per_jiffy calculation
> 
> On Fri, Jun 24, 2011 at 04:05:42PM +0530, Sanjeev Premi wrote:
> > Currently, loops_per_jiffy is being calculated before calling
> > cpufreq_notify_transition().
> > 
> > However, generic cpufreq driver adjusts the jiffies as well.
> > Double adjustment leads to incorrect value being assigned to
> > loops_per_jiffy.
> 
> Are you sure the generic cpufreq driver adjusts the per-cpu 
> loops_per_jiffy
> values?  I don't believe it does.

Russell,

I am quoting the function from drivers/cpufreq/cpufreq.h
Follow the arrows:

[quote]
void cpufreq_notify_transition(struct cpufreq_freqs *freqs, unsigned int state)
{
      [snip]...[snip]

	case CPUFREQ_PRECHANGE:

	      [snip]...[snip]
		srcu_notifier_call_chain(&cpufreq_transition_notifier_list,
				CPUFREQ_PRECHANGE, freqs);
---->		adjust_jiffies(CPUFREQ_PRECHANGE, freqs);
		break;

	case CPUFREQ_POSTCHANGE:
---->		adjust_jiffies(CPUFREQ_POSTCHANGE, freqs);
	      [snip]...[snip]
	}
}
[/quote]

Same can be viewed here:
http://lxr.free-electrons.com/source/drivers/cpufreq/cpufreq.c?a=arm#L300


~sanjeev

[snip]...[snip]

[PATCH 2/2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Russell King - ARM Linux]

On Fri, Jun 24, 2011 at 04:18:31PM +0530, Premi, Sanjeev wrote:
> > -----Original Message-----
> > From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk] 
> > Sent: Friday, June 24, 2011 4:14 PM
> > To: Premi, Sanjeev
> > Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> > Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix 
> > loops_per_jiffy calculation
> > 
> > On Fri, Jun 24, 2011 at 04:05:42PM +0530, Sanjeev Premi wrote:
> > > Currently, loops_per_jiffy is being calculated before calling
> > > cpufreq_notify_transition().
> > > 
> > > However, generic cpufreq driver adjusts the jiffies as well.
> > > Double adjustment leads to incorrect value being assigned to
> > > loops_per_jiffy.
> > 
> > Are you sure the generic cpufreq driver adjusts the per-cpu 
> > loops_per_jiffy
> > values?  I don't believe it does.
> 
> Russell,
> 
> I am quoting the function from drivers/cpufreq/cpufreq.h
> Follow the arrows:

Let's go to war with quotes then, because clearly you haven't read the code
properly:

| #ifndef CONFIG_SMP
^^^^^^^^^^^^^^^^^^^^^
| static unsigned long l_p_j_ref;
| static unsigned int  l_p_j_ref_freq;
| 
| static void adjust_jiffies(unsigned long val, struct cpufreq_freqs *ci)
| {
| 	... code to adjust jiffies ...
| }
| #else
| static inline void adjust_jiffies(unsigned long val, struct cpufreq_freqs *ci)
| {
|         return;
| }
| #endif

Notice how if CONFIG_SMP is set, adjust_jiffies becomes a no-op.  So if
CONFIG_SMP=y, loops_per_jiffy will _not_ be scaled by core code.

[PATCH 2/2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Premi, Sanjeev]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk] 
> Sent: Friday, June 24, 2011 4:22 PM
> To: Premi, Sanjeev
> Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix 
> loops_per_jiffy calculation
> 
> On Fri, Jun 24, 2011 at 04:18:31PM +0530, Premi, Sanjeev wrote:
> > > -----Original Message-----
> > > From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk] 
> > > Sent: Friday, June 24, 2011 4:14 PM
> > > To: Premi, Sanjeev
> > > Cc: linux-omap at vger.kernel.org; 
> linux-arm-kernel at lists.infradead.org
> > > Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix 
> > > loops_per_jiffy calculation
> > > 
> > > On Fri, Jun 24, 2011 at 04:05:42PM +0530, Sanjeev Premi wrote:
> > > > Currently, loops_per_jiffy is being calculated before calling
> > > > cpufreq_notify_transition().
> > > > 
> > > > However, generic cpufreq driver adjusts the jiffies as well.
> > > > Double adjustment leads to incorrect value being assigned to
> > > > loops_per_jiffy.
> > > 
> > > Are you sure the generic cpufreq driver adjusts the per-cpu 
> > > loops_per_jiffy
> > > values?  I don't believe it does.
> > 
> > Russell,
> > 
> > I am quoting the function from drivers/cpufreq/cpufreq.h
> > Follow the arrows:
> 
> Let's go to war with quotes then, because clearly you haven't 
> read the code
> properly:

[sp] Peace. No war!
     Looks like I missed the ifndef in systax highlighting on my
     editor. As mentioned, I couldn't test on OMAP4.
     The patch obvioulsy works for A8... SMP needs to be fixed.
     v2 coming soon.

~sanjeev

> 
> | #ifndef CONFIG_SMP
> ^^^^^^^^^^^^^^^^^^^^^
> | static unsigned long l_p_j_ref;
> | static unsigned int  l_p_j_ref_freq;
> | 
> | static void adjust_jiffies(unsigned long val, struct 
> cpufreq_freqs *ci)
> | {
> | 	... code to adjust jiffies ...
> | }
> | #else
> | static inline void adjust_jiffies(unsigned long val, struct 
> cpufreq_freqs *ci)
> | {
> |         return;
> | }
> | #endif
> 
> Notice how if CONFIG_SMP is set, adjust_jiffies becomes a 
> no-op.  So if
> CONFIG_SMP=y, loops_per_jiffy will _not_ be scaled by core code.
> 

[PATCH 2/2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Santosh Shilimkar]

Sanjeev,

On 6/24/2011 4:21 PM, Russell King - ARM Linux wrote:
> On Fri, Jun 24, 2011 at 04:18:31PM +0530, Premi, Sanjeev wrote:
>>> -----Original Message-----
>>> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
>>> Sent: Friday, June 24, 2011 4:14 PM
>>> To: Premi, Sanjeev
>>> Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
>>> Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix
>>> loops_per_jiffy calculation
>>>
>>> On Fri, Jun 24, 2011 at 04:05:42PM +0530, Sanjeev Premi wrote:
>>>> Currently, loops_per_jiffy is being calculated before calling
>>>> cpufreq_notify_transition().
>>>>
>>>> However, generic cpufreq driver adjusts the jiffies as well.
>>>> Double adjustment leads to incorrect value being assigned to
>>>> loops_per_jiffy.
>>>
>>> Are you sure the generic cpufreq driver adjusts the per-cpu
>>> loops_per_jiffy
>>> values?  I don't believe it does.
>>
>> Russell,
>>
>> I am quoting the function from drivers/cpufreq/cpufreq.h
>> Follow the arrows:
>
> Let's go to war with quotes then, because clearly you haven't read the code
> properly:
>
> | #ifndef CONFIG_SMP
> ^^^^^^^^^^^^^^^^^^^^^
> | static unsigned long l_p_j_ref;
> | static unsigned int  l_p_j_ref_freq;
> |
> | static void adjust_jiffies(unsigned long val, struct cpufreq_freqs *ci)
> | {
> | 	... code to adjust jiffies ...
> | }
> | #else
> | static inline void adjust_jiffies(unsigned long val, struct cpufreq_freqs *ci)
> | {
> |         return;
> | }
> | #endif
>
> Notice how if CONFIG_SMP is set, adjust_jiffies becomes a no-op.  So if
> CONFIG_SMP=y, loops_per_jiffy will _not_ be scaled by core code.
>
As Russell rightly pointed out, you need to take care of UP/SMP and UP 
OVER SMP.

The generic code updates in only in case of UP build. I thought, the
comment is the code was well explaining that part.

Regards
Santosh

[PATCH 2/2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Premi, Sanjeev]

> -----Original Message-----
> From: Shilimkar, Santosh 
> Sent: Friday, June 24, 2011 6:16 PM
> To: Russell King - ARM Linux
> Cc: Premi, Sanjeev; linux-omap at vger.kernel.org; 
> linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix 
> loops_per_jiffy calculation
> 
> Sanjeev,
> 
> On 6/24/2011 4:21 PM, Russell King - ARM Linux wrote:
> > On Fri, Jun 24, 2011 at 04:18:31PM +0530, Premi, Sanjeev wrote:
> >>> -----Original Message-----
> >>> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> >>> Sent: Friday, June 24, 2011 4:14 PM
> >>> To: Premi, Sanjeev
> >>> Cc: linux-omap at vger.kernel.org; 
> linux-arm-kernel at lists.infradead.org
> >>> Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix
> >>> loops_per_jiffy calculation
> >>>
> >>> On Fri, Jun 24, 2011 at 04:05:42PM +0530, Sanjeev Premi wrote:
> >>>> Currently, loops_per_jiffy is being calculated before calling
> >>>> cpufreq_notify_transition().
> >>>>
> >>>> However, generic cpufreq driver adjusts the jiffies as well.
> >>>> Double adjustment leads to incorrect value being assigned to
> >>>> loops_per_jiffy.
> >>>
> >>> Are you sure the generic cpufreq driver adjusts the per-cpu
> >>> loops_per_jiffy
> >>> values?  I don't believe it does.
> >>
> >> Russell,
> >>
> >> I am quoting the function from drivers/cpufreq/cpufreq.h
> >> Follow the arrows:
> >
> > Let's go to war with quotes then, because clearly you 
> haven't read the code
> > properly:
> >
> > | #ifndef CONFIG_SMP
> > ^^^^^^^^^^^^^^^^^^^^^
> > | static unsigned long l_p_j_ref;
> > | static unsigned int  l_p_j_ref_freq;
> > |
> > | static void adjust_jiffies(unsigned long val, struct 
> cpufreq_freqs *ci)
> > | {
> > | 	... code to adjust jiffies ...
> > | }
> > | #else
> > | static inline void adjust_jiffies(unsigned long val, 
> struct cpufreq_freqs *ci)
> > | {
> > |         return;
> > | }
> > | #endif
> >
> > Notice how if CONFIG_SMP is set, adjust_jiffies becomes a 
> no-op.  So if
> > CONFIG_SMP=y, loops_per_jiffy will _not_ be scaled by core code.
> >
> As Russell rightly pointed out, you need to take care of 
> UP/SMP and UP 
> OVER SMP.
> 
> The generic code updates in only in case of UP build. I thought, the
> comment is the code was well explaining that part.

[sp] I did read the code but took long to understand most of it.
     Hence patch 1 of this series.

     I already accepted that I didn't notice definition of adjust_jiffies.
     ...call it perils of leaving job half-done across a good-night-sleep!

     As is, the calculations of UP are incorrect... which I was fixing.
     Anyway, I have an updated patch - currently testing with CONFIG_SMP
     enabled. Will be posting soon.

~sanjeev

> 
> Regards
> Santosh
> 

[PATCH 2/2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Santosh Shilimkar]

On 6/24/2011 6:22 PM, Premi, Sanjeev wrote:
>> -----Original Message-----
>> From: Shilimkar, Santosh
>> Sent: Friday, June 24, 2011 6:16 PM
>> To: Russell King - ARM Linux
>> Cc: Premi, Sanjeev; linux-omap at vger.kernel.org;
>> linux-arm-kernel at lists.infradead.org
>> Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix
>> loops_per_jiffy calculation
>>
>> Sanjeev,
>>
>> On 6/24/2011 4:21 PM, Russell King - ARM Linux wrote:
>>> On Fri, Jun 24, 2011 at 04:18:31PM +0530, Premi, Sanjeev wrote:
>>>>> -----Original Message-----
>>>>> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
>>>>> Sent: Friday, June 24, 2011 4:14 PM
>>>>> To: Premi, Sanjeev
>>>>> Cc: linux-omap at vger.kernel.org;
>> linux-arm-kernel at lists.infradead.org
>>>>> Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix
>>>>> loops_per_jiffy calculation
>>>>>
>>>>> On Fri, Jun 24, 2011 at 04:05:42PM +0530, Sanjeev Premi wrote:
>>>>>> Currently, loops_per_jiffy is being calculated before calling
>>>>>> cpufreq_notify_transition().
>>>>>>
>>>>>> However, generic cpufreq driver adjusts the jiffies as well.
>>>>>> Double adjustment leads to incorrect value being assigned to
>>>>>> loops_per_jiffy.
>>>>>
>>>>> Are you sure the generic cpufreq driver adjusts the per-cpu
>>>>> loops_per_jiffy
>>>>> values?  I don't believe it does.
>>>>
>>>> Russell,
>>>>
>>>> I am quoting the function from drivers/cpufreq/cpufreq.h
>>>> Follow the arrows:
>>>
>>> Let's go to war with quotes then, because clearly you
>> haven't read the code
>>> properly:
>>>
>>> | #ifndef CONFIG_SMP
>>> ^^^^^^^^^^^^^^^^^^^^^
>>> | static unsigned long l_p_j_ref;
>>> | static unsigned int  l_p_j_ref_freq;
>>> |
>>> | static void adjust_jiffies(unsigned long val, struct
>> cpufreq_freqs *ci)
>>> | {
>>> | 	... code to adjust jiffies ...
>>> | }
>>> | #else
>>> | static inline void adjust_jiffies(unsigned long val,
>> struct cpufreq_freqs *ci)
>>> | {
>>> |         return;
>>> | }
>>> | #endif
>>>
>>> Notice how if CONFIG_SMP is set, adjust_jiffies becomes a
>> no-op.  So if
>>> CONFIG_SMP=y, loops_per_jiffy will _not_ be scaled by core code.
>>>
>> As Russell rightly pointed out, you need to take care of
>> UP/SMP and UP
>> OVER SMP.
>>
>> The generic code updates in only in case of UP build. I thought, the
>> comment is the code was well explaining that part.
>
> [sp] I did read the code but took long to understand most of it.
>       Hence patch 1 of this series.
>
>       I already accepted that I didn't notice definition of adjust_jiffies.
>       ...call it perils of leaving job half-done across a good-night-sleep!
>
>       As is, the calculations of UP are incorrect... which I was fixing.
>       Anyway, I have an updated patch - currently testing with CONFIG_SMP
>       enabled. Will be posting soon.
>
I can imagine what you are gonna post based on Russell comment, but will
wait for your patch before commenting.

Regards
Santosh