[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Sanjeev Premi]

Currently, loops_per_jiffy is being calculated twice for
non-SMP processors.
 - Before calling cpufreq_notify_transition()
 - From within cpufreq_notify_transition()

Double adjustment leads to incorrect value being assigned to
loops_per_jiffy. This manifests as incorrect BogoMIPS in
"cat /proc/cpuinfo".

The value of loops_per_jiffy needs to be calculated only
when CONFIG_SMP is true. It is the core change included
in this patch.

The patch also leverages the definition of for_each_cpu()
with and without CONFIG_SMP to consolidate the mechanism
to call cpufreq_notify_transition().

Signed-off-by: Sanjeev Premi <premi@ti.com>
---
 Changes since v1:
   * loops_per_jiffy are updated when CONFIG_SMP is true.
   * leverage definition of for_each_cpu()

 Tested on OMAP3EVM with and without CONFIG_SMP.
 Since the log is rather long, will be posting the log in
 a follow-up mail.

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

diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
index 346519e..0263cae 100644
--- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
+++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
@@ -97,12 +97,8 @@ static int omap_target(struct cpufreq_policy *policy,
 		return ret;

 	/* Notify transitions */
-	if (is_smp()) {
-		for_each_cpu(i, policy->cpus) {
-			freqs.cpu = i;
-			cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
-		}
-	} else {
+	for_each_cpu(i, policy->cpus) {
+		freqs.cpu = i;
 		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
 	}

@@ -114,13 +110,20 @@ static int omap_target(struct cpufreq_policy *policy,

 	freqs.new = omap_getspeed(policy->cpu);

+#ifdef CONFIG_SMP
+	/* Adjust jiffies before transition */
+	for_each_cpu(i, policy->cpus) {
+		unsigned long lpj = per_cpu(cpu_data, i).loops_per_jiffy;
+
+		per_cpu(cpu_data, i).loops_per_jiffy = cpufreq_scale(lpj,
+							freqs.old,
+							freqs.new);
+	}
+#endif
+
 	/* Notify transitions */
-	if (is_smp()) {
-		for_each_cpu(i, policy->cpus) {
-			freqs.cpu = i;
-			cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
-		}
-	} else {
+	for_each_cpu(i, policy->cpus) {
+		freqs.cpu = i;
 		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
 	}

--
1.7.2.2

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Premi, Sanjeev]

> -----Original Message-----
> From: Premi, Sanjeev 
> Sent: Friday, June 24, 2011 7:24 PM
> To: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Cc: Premi, Sanjeev
> Subject: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy 
> calculation
> 
> Currently, loops_per_jiffy is being calculated twice for
> non-SMP processors.
>  - Before calling cpufreq_notify_transition()
>  - From within cpufreq_notify_transition()
> 
> Double adjustment leads to incorrect value being assigned to
> loops_per_jiffy. This manifests as incorrect BogoMIPS in
> "cat /proc/cpuinfo".
> 
> The value of loops_per_jiffy needs to be calculated only
> when CONFIG_SMP is true. It is the core change included
> in this patch.
> 
> The patch also leverages the definition of for_each_cpu()
> with and without CONFIG_SMP to consolidate the mechanism
> to call cpufreq_notify_transition().
> 
> Signed-off-by: Sanjeev Premi <premi@ti.com>
> ---
>  Changes since v1:
>    * loops_per_jiffy are updated when CONFIG_SMP is true.
>    * leverage definition of for_each_cpu()
> 
>  Tested on OMAP3EVM with and without CONFIG_SMP.
>  Since the log is rather long, will be posting the log in
>  a follow-up mail.
> 
[snip]...[snip]

Snapshot of test log - with and without SMP included here:

To ensure compile-time and run-time checks for SMP are visible,
the patch was tested with this code inserted at enty of function
omap_target().

#ifdef CONFIG_SMP
	printk (KERN_ERR "I am defined SMP!\n");
#else
	printk (KERN_ERR "I am not defined SMP!\n");
#endif
	if (is_smp())
		printk (KERN_ERR "Runtime evaluating to SMP!\n");
	else
		printk (KERN_ERR "Runtime evaluating to no SMP!\n");


With CONFIG_SMP disabled
========================

[root at OMAP3EVM /]# cd /sys/devices/system/cpu/cpu0/cpufreq/
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]# cat scaling_available_frequencies
300000 600000 800000 1000000
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]# echo 300000 > scaling_setspeed
[   56.278137] I am not defined SMP!
[   56.281768] Runtime evaluating to no SMP!
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]# cat cpuinfo_cur_freq
300000
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]#
[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
[   96.938049] I am not defined SMP!
[   96.941833] Runtime evaluating to no SMP!
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]#
[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]#
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]# echo 100000 > scaling_setspeed
[  110.865631] I am not defined SMP!
[  110.870025] Runtime evaluating to no SMP!
[root at OMAP3EVM cpufreq]# echo 1000000 > scaling_setspeed
[  116.258941] I am not defined SMP!
[  116.262725] Runtime evaluating to no SMP!
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]# cat cpuinfo_cur_freq
1000000
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]# cat /proc/cpuinfo
Processor       : ARMv7 Processor rev 2 (v7l)
BogoMIPS        : 996.74
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]#

With CONFIG_SMP enabled
========================

[root at OMAP3EVM /]# cd /sys/devices/system/cpu/cpu0/cpufreq/
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]# cat scaling_available_frequencies
300000 600000 800000 1000000
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]# echo 300000 > scaling_setspeed
[   25.040496] I am defined SMP!
[   25.043884] Runtime evaluating to no SMP!
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]# cat /proc/cpuinfo
Processor       : ARMv7 Processor rev 2 (v7l)
processor       : 0
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]# cat cpuinfo_cur_freq
300000
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]# echo 800000 > scaling_setspeed
[   46.333618] I am defined SMP!
[   46.336822] Runtime evaluating to no SMP!
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]# cat cpuinfo_cur_freq
800000
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]# cat /proc/cpuinfo
Processor       : ARMv7 Processor rev 2 (v7l)
processor       : 0
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]#
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]# echo 600000 > scaling_setspeed
[ 1344.705413] I am defined SMP!
[ 1344.709259] Runtime evaluating to no SMP!
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]#
[root at OMAP3EVM cpufreq]# cat /proc/cpuinfo
Processor       : ARMv7 Processor rev 2 (v7l)
processor       : 0
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

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Russell King - ARM Linux]

On Fri, Jun 24, 2011 at 07:23:38PM +0530, Sanjeev Premi wrote:
> @@ -97,12 +97,8 @@ static int omap_target(struct cpufreq_policy *policy,
>  		return ret;
> 
>  	/* Notify transitions */
> -	if (is_smp()) {
> -		for_each_cpu(i, policy->cpus) {
> -			freqs.cpu = i;
> -			cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> -		}
> -	} else {
> +	for_each_cpu(i, policy->cpus) {
> +		freqs.cpu = i;
>  		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
>  	}
> 

Ok.

> @@ -114,13 +110,20 @@ static int omap_target(struct cpufreq_policy *policy,
> 
>  	freqs.new = omap_getspeed(policy->cpu);
> 
> +#ifdef CONFIG_SMP
> +	/* Adjust jiffies before transition */
> +	for_each_cpu(i, policy->cpus) {
> +		unsigned long lpj = per_cpu(cpu_data, i).loops_per_jiffy;
> +
> +		per_cpu(cpu_data, i).loops_per_jiffy = cpufreq_scale(lpj,
> +							freqs.old,
> +							freqs.new);
> +	}
> +#endif
> +

You didn't listen to what I told you.  It'll be quicker for me to write
this patch myself if you send me the *original* file.

>  	/* Notify transitions */
> -	if (is_smp()) {
> -		for_each_cpu(i, policy->cpus) {
> -			freqs.cpu = i;
> -			cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> -		}
> -	} else {
> +	for_each_cpu(i, policy->cpus) {
> +		freqs.cpu = i;
>  		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
>  	}

Ok.

[PATCHv2] 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 7:32 PM
> To: Premi, Sanjeev
> Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCHv2] omap2+: pm: cpufreq: Fix 
> loops_per_jiffy calculation

[snip]...[snip]

> > 
> > +#ifdef CONFIG_SMP
> > +	/* Adjust jiffies before transition */
> > +	for_each_cpu(i, policy->cpus) {
> > +		unsigned long lpj = per_cpu(cpu_data, 
> i).loops_per_jiffy;
> > +
> > +		per_cpu(cpu_data, i).loops_per_jiffy = 
> cpufreq_scale(lpj,
> > +							freqs.old,
> > +							freqs.new);
> > +	}
> > +#endif
> > +
> 
> You didn't listen to what I told you.  It'll be quicker for 
> me to write
> this patch myself if you send me the *original* file.
> 

Russell,

The function adjust_jiffies() is no-op for CONFIG_SMP - but it
is not the case with cpufreq_scale().

So, when CONFIG_SMP is defined, then loops_per_jiffy would be
calculated for both UP and SMP processors.

Did I miss something else?

~sanjeev


[snip]...[snip]

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Russell King - ARM Linux]

On Fri, Jun 24, 2011 at 07:39:41PM +0530, Premi, Sanjeev wrote:
> > -----Original Message-----
> > From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk] 
> > Sent: Friday, June 24, 2011 7:32 PM
> > To: Premi, Sanjeev
> > Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> > Subject: Re: [PATCHv2] omap2+: pm: cpufreq: Fix 
> > loops_per_jiffy calculation
> 
> [snip]...[snip]
> 
> > > 
> > > +#ifdef CONFIG_SMP
> > > +	/* Adjust jiffies before transition */
> > > +	for_each_cpu(i, policy->cpus) {
> > > +		unsigned long lpj = per_cpu(cpu_data, 
> > i).loops_per_jiffy;
> > > +
> > > +		per_cpu(cpu_data, i).loops_per_jiffy = 
> > cpufreq_scale(lpj,
> > > +							freqs.old,
> > > +							freqs.new);
> > > +	}
> > > +#endif
> > > +
> > 
> > You didn't listen to what I told you.  It'll be quicker for 
> > me to write
> > this patch myself if you send me the *original* file.
> > 
> 
> Russell,
> 
> The function adjust_jiffies() is no-op for CONFIG_SMP - but it
> is not the case with cpufreq_scale().
> 
> So, when CONFIG_SMP is defined, then loops_per_jiffy would be
> calculated for both UP and SMP processors.
> 
> Did I miss something else?

Yes you have, and you've not sent me the file, so I can't show you where
you've gone wrong.

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Santosh Shilimkar]

On 6/24/2011 7:23 PM, Sanjeev Premi wrote:
> Currently, loops_per_jiffy is being calculated twice for
> non-SMP processors.
>   - Before calling cpufreq_notify_transition()
>   - From within cpufreq_notify_transition()
>
> Double adjustment leads to incorrect value being assigned to
> loops_per_jiffy. This manifests as incorrect BogoMIPS in
> "cat /proc/cpuinfo".
>
> The value of loops_per_jiffy needs to be calculated only
> when CONFIG_SMP is true. It is the core change included
> in this patch.
>
> The patch also leverages the definition of for_each_cpu()
> with and without CONFIG_SMP to consolidate the mechanism
> to call cpufreq_notify_transition().
>
> Signed-off-by: Sanjeev Premi<premi@ti.com>

NAK. This patch again doesn't make sense considering your issue.
Also jiffies should not be undated before changing the freq.
If the set_rate failed for some reason then you will have wrong
jiffies value.

I understand your issue now. The code for global lpj updation
should have been checking smp_on_up() instead of is_smp().
That one line change is enough. I will post a patch on
the same once I reach to office.

Regards
Santosh

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Premi, Sanjeev]

> -----Original Message-----
> From: Shilimkar, Santosh 
> Sent: Friday, June 24, 2011 8:05 PM
> To: Premi, Sanjeev
> Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCHv2] omap2+: pm: cpufreq: Fix 
> loops_per_jiffy calculation
> 
> On 6/24/2011 7:23 PM, Sanjeev Premi wrote:
> > Currently, loops_per_jiffy is being calculated twice for
> > non-SMP processors.
> >   - Before calling cpufreq_notify_transition()
> >   - From within cpufreq_notify_transition()
> >
> > Double adjustment leads to incorrect value being assigned to
> > loops_per_jiffy. This manifests as incorrect BogoMIPS in
> > "cat /proc/cpuinfo".
> >
> > The value of loops_per_jiffy needs to be calculated only
> > when CONFIG_SMP is true. It is the core change included
> > in this patch.
> >
> > The patch also leverages the definition of for_each_cpu()
> > with and without CONFIG_SMP to consolidate the mechanism
> > to call cpufreq_notify_transition().
> >
> > Signed-off-by: Sanjeev Premi<premi@ti.com>
> 
> NAK. This patch again doesn't make sense considering your issue.
> Also jiffies should not be undated before changing the freq.
> If the set_rate failed for some reason then you will have wrong
> jiffies value.
> 
> I understand your issue now. The code for global lpj updation
> should have been checking smp_on_up() instead of is_smp().
> That one line change is enough. I will post a patch on
> the same once I reach to office.

[sp] Suggest reading the patch before NAKing.
     The patch is updating the loops_per_jiffy value "AFTER"
     the frequency change has happenned and "BEFORE" post
     change notification is sent.

~sanjeev


> 
> Regards
> Santosh
> 

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Santosh Shilimkar]

On 6/24/2011 7:40 AM, Premi, Sanjeev wrote:
>> -----Original Message-----
>> From: Shilimkar, Santosh
>> Sent: Friday, June 24, 2011 8:05 PM
>> To: Premi, Sanjeev
>> Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
>> Subject: Re: [PATCHv2] omap2+: pm: cpufreq: Fix
>> loops_per_jiffy calculation
>>
>> On 6/24/2011 7:23 PM, Sanjeev Premi wrote:
>>> Currently, loops_per_jiffy is being calculated twice for
>>> non-SMP processors.
>>>    - Before calling cpufreq_notify_transition()
>>>    - From within cpufreq_notify_transition()
>>>
>>> Double adjustment leads to incorrect value being assigned to
>>> loops_per_jiffy. This manifests as incorrect BogoMIPS in
>>> "cat /proc/cpuinfo".
>>>
>>> The value of loops_per_jiffy needs to be calculated only
>>> when CONFIG_SMP is true. It is the core change included
>>> in this patch.
>>>
>>> The patch also leverages the definition of for_each_cpu()
>>> with and without CONFIG_SMP to consolidate the mechanism
>>> to call cpufreq_notify_transition().
>>>
>>> Signed-off-by: Sanjeev Premi<premi@ti.com>
>>
>> NAK. This patch again doesn't make sense considering your issue.
>> Also jiffies should not be undated before changing the freq.
>> If the set_rate failed for some reason then you will have wrong
>> jiffies value.
>>
>> I understand your issue now. The code for global lpj updation
>> should have been checking smp_on_up() instead of is_smp().
>> That one line change is enough. I will post a patch on
>> the same once I reach to office.
>
> [sp] Suggest reading the patch before NAKing.
>       The patch is updating the loops_per_jiffy value "AFTER"
>       the frequency change has happenned and "BEFORE" post
>       change notification is sent.
>
Sure. Let me respond to you with the patch instead of making
more noise on the list.

Regard
Santosh

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Russell King - ARM Linux]

Right, thanks for the file.  Here's the patch.

--- omap2plus-cpufreq.c~	2011-06-24 15:50:32.000000000 +0100
+++ omap2plus-cpufreq.c	2011-06-24 16:00:08.000000000 +0100
@@ -44,6 +44,16 @@
 static char *mpu_clk_name;
 static struct device *mpu_dev;
 
+#ifdef CONFIG_SMP
+struct lpj_info {
+	unsigned long	ref;
+	unsigned int	freq;
+};
+
+static DEFINE_PER_CPU(struct lpj_info, lpj_ref);
+static struct lpj_info global_lpj_ref;
+#endif
+
 static int omap_verify_speed(struct cpufreq_policy *policy)
 {
 	if (!freq_table)
@@ -109,14 +119,25 @@
 	freqs.new = omap_getspeed(policy->cpu);
 
 #ifdef CONFIG_SMP
-	/* Adjust jiffies before transition */
+	/* Adjust per-cpu loops_per_jiffy before transition */
 	for_each_cpu(i, policy->cpus) {
-		unsigned long lpj = per_cpu(cpu_data, i).loops_per_jiffy;
-
-		per_cpu(cpu_data, i).loops_per_jiffy = cpufreq_scale(lpj,
-							freqs.old,
-							freqs.new);
+		struct lpj_info *lpj = &per_cpu(lpj_ref, i);
+		if (!lpj->freq) {
+			lpj->ref = per_cpu(cpu_data, i).loops_per_jiffy;
+			lpj->freq = freqs.old;
+		}
+
+		per_cpu(cpu_data, i).loops_per_jiffy =
+			cpufreq_scale(lpj->ref, lpj->freq, freqs.new);
+	}
+
+	/* And don't forget to adjust the global one */
+	if (!global_lpj_ref.freq) {
+		global_lpj_ref.ref = loops_per_jiffy;
+		global_lpj_ref.freq = freqs.old;
 	}
+	loops_per_jiffy = cpufreq_scale(global_lpj_ref.ref, global_lpj_ref.freq,
+					freqs.new);
 #endif
 
 	/* Notify transitions */


Notice how we adjust _both_ the per-cpu loops_per_jiffy, and that we
adjust them with reference to the initial values.

If you adjust lpj with reference to the last, then you _will_ build up
a progressively bigger and bigger error in the value over time.

[PATCHv2] 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 8:42 PM
> To: Premi, Sanjeev
> Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCHv2] omap2+: pm: cpufreq: Fix 
> loops_per_jiffy calculation
> 
> Right, thanks for the file.  Here's the patch.
> 
> --- omap2plus-cpufreq.c~	2011-06-24 15:50:32.000000000 +0100
> +++ omap2plus-cpufreq.c	2011-06-24 16:00:08.000000000 +0100
> @@ -44,6 +44,16 @@
>  static char *mpu_clk_name;
>  static struct device *mpu_dev;
>  
> +#ifdef CONFIG_SMP
> +struct lpj_info {
> +	unsigned long	ref;
> +	unsigned int	freq;
> +};
> +
> +static DEFINE_PER_CPU(struct lpj_info, lpj_ref);
> +static struct lpj_info global_lpj_ref;
> +#endif
> +
>  static int omap_verify_speed(struct cpufreq_policy *policy)
>  {
>  	if (!freq_table)
> @@ -109,14 +119,25 @@
>  	freqs.new = omap_getspeed(policy->cpu);
>  
>  #ifdef CONFIG_SMP
> -	/* Adjust jiffies before transition */
> +	/* Adjust per-cpu loops_per_jiffy before transition */
>  	for_each_cpu(i, policy->cpus) {
> -		unsigned long lpj = per_cpu(cpu_data, 
> i).loops_per_jiffy;
> -
> -		per_cpu(cpu_data, i).loops_per_jiffy = 
> cpufreq_scale(lpj,
> -							freqs.old,
> -							freqs.new);
> +		struct lpj_info *lpj = &per_cpu(lpj_ref, i);
> +		if (!lpj->freq) {
> +			lpj->ref = per_cpu(cpu_data, i).loops_per_jiffy;
> +			lpj->freq = freqs.old;
> +		}
> +
> +		per_cpu(cpu_data, i).loops_per_jiffy =
> +			cpufreq_scale(lpj->ref, lpj->freq, freqs.new);
> +	}
> +
> +	/* And don't forget to adjust the global one */
> +	if (!global_lpj_ref.freq) {
> +		global_lpj_ref.ref = loops_per_jiffy;
> +		global_lpj_ref.freq = freqs.old;
>  	}
> +	loops_per_jiffy = cpufreq_scale(global_lpj_ref.ref, 
> global_lpj_ref.freq,
> +					freqs.new);
>  #endif
>  
>  	/* Notify transitions */
> 
> 
> Notice how we adjust _both_ the per-cpu loops_per_jiffy, and that we
> adjust them with reference to the initial values.
> 
> If you adjust lpj with reference to the last, then you _will_ build up
> a progressively bigger and bigger error in the value over time.

Russell,

I definitely didn't see so many things through your comments. But
that may just be reflection of my naivety with SMP!

I am currently testing another patch for beagle - will apply and
test on OMAP3EVM (just to be sure).

Can I include it in my next patch rev?

~sanjeev

[PATCHv2] 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 8:42 PM
> To: Premi, Sanjeev
> Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCHv2] omap2+: pm: cpufreq: Fix 
> loops_per_jiffy calculation
> 
> Right, thanks for the file.  Here's the patch.
> 
> --- omap2plus-cpufreq.c~	2011-06-24 15:50:32.000000000 +0100
> +++ omap2plus-cpufreq.c	2011-06-24 16:00:08.000000000 +0100
> @@ -44,6 +44,16 @@
>  static char *mpu_clk_name;
>  static struct device *mpu_dev;
>  
> +#ifdef CONFIG_SMP
> +struct lpj_info {
> +	unsigned long	ref;
> +	unsigned int	freq;
> +};
> +
> +static DEFINE_PER_CPU(struct lpj_info, lpj_ref);
> +static struct lpj_info global_lpj_ref;
> +#endif
> +
>  static int omap_verify_speed(struct cpufreq_policy *policy)
>  {
>  	if (!freq_table)
> @@ -109,14 +119,25 @@
>  	freqs.new = omap_getspeed(policy->cpu);
>  
>  #ifdef CONFIG_SMP
> -	/* Adjust jiffies before transition */
> +	/* Adjust per-cpu loops_per_jiffy before transition */
>  	for_each_cpu(i, policy->cpus) {
> -		unsigned long lpj = per_cpu(cpu_data, 
> i).loops_per_jiffy;
> -
> -		per_cpu(cpu_data, i).loops_per_jiffy = 
> cpufreq_scale(lpj,
> -							freqs.old,
> -							freqs.new);
> +		struct lpj_info *lpj = &per_cpu(lpj_ref, i);
> +		if (!lpj->freq) {
> +			lpj->ref = per_cpu(cpu_data, i).loops_per_jiffy;
> +			lpj->freq = freqs.old;
> +		}
> +
> +		per_cpu(cpu_data, i).loops_per_jiffy =
> +			cpufreq_scale(lpj->ref, lpj->freq, freqs.new);
> +	}
> +
> +	/* And don't forget to adjust the global one */
> +	if (!global_lpj_ref.freq) {
> +		global_lpj_ref.ref = loops_per_jiffy;
> +		global_lpj_ref.freq = freqs.old;
>  	}
> +	loops_per_jiffy = cpufreq_scale(global_lpj_ref.ref, 
> global_lpj_ref.freq,
> +					freqs.new);
>  #endif
>  
>  	/* Notify transitions */
> 
> 
> Notice how we adjust _both_ the per-cpu loops_per_jiffy, and that we
> adjust them with reference to the initial values.
> 
> If you adjust lpj with reference to the last, then you _will_ build up
> a progressively bigger and bigger error in the value over time.
> 

Russell,

I was able to test BogoMIPS calculations via /proc/cpuinfo for
both with & without CONFIG_SMP selected.

For most part things work fine - but I do notice occassional Oops
and segmentation faults while doing "cat /proc/cpuinfo"

With CONFIG_SMP enabled, system doesn't recover from the Oops;
but without SMP - I noticed segmentation faults/ BUG but system
does recover.

They could be unrelated - but i didn't see any of these earlier
today. I will continue debug on MON.

Here are details:

[1] This log comes corresponds to CONFIG_SMP enabled.

[2] This corresponds to running "cat /proc/cpuinfo" in a tight
    loop. (CONFIG_SMP disabled).
    
[3] Saw only once today - but had seen it few days ago. None
    of my local changes included. (CONFIG_SMP disabled).
    [http://marc.info/?l=linux-omap&m=130884641524123&w=2]

~sanjeev

=== [1]

[root at OMAP3EVM cpufreq]# cat /proc/cpuinfo
[   73.832366] Internal error: Oops - undefined instruction: 0 [#1] SMP
[   73.839019] Modules linked in:
[   73.842193] CPU: 0    Not tainted  (3.0.0-rc3-14002-g40b6752-dirty #21)
[   73.849121] PC is at __do_fault+0x1c0/0x450
[   73.853485] LR is at __do_fault+0x2b0/0x450
[   73.857879] pc : [<c010fa18>]    lr : [<c010fb08>]    psr: 00000113
[   73.857879] sp : c7907d48  ip : 00000000  fp : c5d518c0
[   73.869873] r10: 00000200  r9 : 40214000  r8 : 00000000
[   73.875335] r7 : c2692f98  r6 : c0ad7600  r5 : 87fb018f  r4 : 00000000
[   73.882141] r3 : 87fb0a3e  r2 : 00000800  r1 : 87fb01cf  r0 : c5d518c0
[   73.888977] Flags: nzcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
[   73.896423] Control: 10c5387d  Table: 8795c019  DAC: 00000015
[   73.902435] Process cat (pid: 449, stack limit = 0xc79062f8)
[   73.908355] Stack: (0xc7907d48 to 0xc7908000)
[   73.912902] 7d40:                   00000008 c795d008 00000000 00000000 c5d4d050 c5d518f4
[   73.921447] 7d60: 00000000 0000000c 40214000 c0ad7600 00000002 0000000c c795d008 00000000
[   73.929992] 7d80: 40214000 c5d518c0 40214000 c2692f98 be91579c c0110760 0000000c 00000000
[   73.938537] 7da0: 00000000 c00bb088 00000002 c5d518c0 00000000 c03111a0 00000000 c795c000
[   73.947113] 7dc0: c795d008 00000201 40214000 c5d518c0 00000000 c2692f98 be91579c c0111184
[   73.955657] 7de0: c795d008 00000000 c2692f98 c7907fb0 00000017 402140ea c5d518c0 402140ea
[   73.964202] 7e00: c5d51914 c031138c c78c6180 00000000 00000002 0000002f 00000017 c78c6180
[   73.972747] 7e20: c08da030 c00b9184 c08da030 c00b9184 c5d518c4 c00b7860 c78c6180 c78c6180
[   73.981292] 7e40: c04787dc 00000000 c04af0e0 0000013e 00000000 00000000 00000002 c00b9184
[   73.989868] 7e60: 00000000 00000000 c78c6180 00000001 c78c6180 00000000 a2217a4d 3caba335
[   73.998413] 7e80: 89831c46 a201fb19 4c802a1d c78c65e0 0000002f 00000000 0895e1ee 8cafea22
[   74.006958] 7ea0: 2f5807fc 70396d1c 752baa3e 00000001 c78c6180 00000007 00000006 000080d0
[   74.015502] 7ec0: c2692b78 00000000 00000000 00000000 00000000 c5d51928 00000002 60000093
[   74.024047] 7ee0: 00000000 c00bb088 00000002 c044d374 0001b900 00000017 c7907fb0 402140ea
[   74.032592] 7f00: 402140ea 4032d068 be91579c c003e480 60000013 00001000 c5d51914 00000003
[   74.041168] 7f20: be915a14 c030e788 00000002 00000000 c021e1b4 00000000 c048cb1c c7906000
[   74.049713] 7f40: 4008e000 00000000 c78c6180 c030ecf4 00000000 00000000 00001000 c00b7ba0
[   74.058258] 7f60: c5d51918 60000013 00000000 c030ecf4 c78c6180 c030f1d8 000004b0 4021051c
[   74.066802] 7f80: 4021c998 c00b7ba0 ffffffff 0001b900 000004b0 c030f1d8 ffffffff 0001b900
[   74.075347] 7fa0: 000004b0 4021051c 4021c998 c030f50c 402140ea 4005c950 be91587c 4005cb08
[   74.083892] 7fc0: 00000015 0001b900 000004b0 4021051c 4021c998 402140ea 4032d068 be91579c
[   74.092468] 7fe0: 00000000 be9156d0 400403f8 4003e6f4 20000010 ffffffff ffffffff ffffffff
[   74.101013] [<c010fa18>] (__do_fault+0x1c0/0x450) from [<c0110760>] (handle_pte_fault+0x74/0x674)
[   74.110290] [<c0110760>] (handle_pte_fault+0x74/0x674) from [<c0111184>] (handle_mm_fault+0x94/0xc4)
[   74.119842] [<c0111184>] (handle_mm_fault+0x94/0xc4) from [<c031138c>] (do_page_fault+0x2b8/0x358)
[   74.129211] [<c031138c>] (do_page_fault+0x2b8/0x358) from [<c003e480>] (do_DataAbort+0x38/0x98)
[   74.138336] [<c003e480>] (do_DataAbort+0x38/0x98) from [<c030f50c>] (ret_from_exception+0x0/0x10)
[   74.147613] Exception stack(0xc7907fb0 to 0xc7907ff8)
[   74.152862] 7fa0:                                     402140ea 4005c950 be91587c 4005cb08
[   74.161437] 7fc0: 00000015 0001b900 000004b0 4021051c 4021c998 402140ea 4032d068 be91579c
[   74.169982] 7fe0: 00000000 be9156d0 400403f8 4003e6f4 20000010 ffffffff
[   74.176879] Code: e1a01005 e3a02000 ebfd1694 e59d0014 (eb07fcba)
[   74.183380] ---[ end trace 9fe8ca36c9812c43 ]---
[   86.665679] BUG: spinlock lockup on CPU#0, cat/449, c5d518f4
[   86.671661] [<c00509c4>] (unwind_backtrace+0x0/0xf8) from [<c0227320>] (do_raw_spin_lock+0xec/0x178)
[   86.681213] [<c0227320>] (do_raw_spin_lock+0xec/0x178) from [<c0112578>] (unmap_vmas+0x188/0x620)
[   86.690521] [<c0112578>] (unmap_vmas+0x188/0x620) from [<c0114fd4>] (exit_mmap+0x10c/0x23c)
[   86.699279] [<c0114fd4>] (exit_mmap+0x10c/0x23c) from [<c007f4bc>] (mmput+0x54/0x118)
[   86.707489] [<c007f4bc>] (mmput+0x54/0x118) from [<c00838a0>] (exit_mm+0x13c/0x194)
[   86.715545] [<c00838a0>] (exit_mm+0x13c/0x194) from [<c0085864>] (do_exit+0x660/0x714)
[   86.723846] [<c0085864>] (do_exit+0x660/0x714) from [<c004dafc>] (die+0x2d8/0x2f4)
[   86.731781] [<c004dafc>] (die+0x2d8/0x2f4) from [<c003e204>] (do_undefinstr+0x14c/0x150)
[   86.740264] [<c003e204>] (do_undefinstr+0x14c/0x150) from [<c030f0e4>] (__und_svc+0x44/0x60)
[   86.749114] Exception stack(0xc7907cc0 to 0xc7907d08)
[   86.754394] 7cc0: c5d518c0 87fb01cf 00000800 87fb0a3e 00000000 87fb018f c0ad7600 c2692f98
[   86.762969] 7ce0: 00000000 40214000 00000200 c5d518c0 00000000 c7907d48 c010fb08 c010fa18
[   86.771514] 7d00: 00000113 ffffffff
[   86.775177] [<c030f0e4>] (__und_svc+0x44/0x60) from [<c010fa18>] (__do_fault+0x1c0/0x450)
[   86.783752] [<c010fa18>] (__do_fault+0x1c0/0x450) from [<c0110760>] (handle_pte_fault+0x74/0x674)
[   86.793060] [<c0110760>] (handle_pte_fault+0x74/0x674) from [<c0111184>] (handle_mm_fault+0x94/0xc4)
[   86.802612] [<c0111184>] (handle_mm_fault+0x94/0xc4) from [<c031138c>] (do_page_fault+0x2b8/0x358)
[   86.812011] [<c031138c>] (do_page_fault+0x2b8/0x358) from [<c003e480>] (do_DataAbort+0x38/0x98)
[   86.821136] [<c003e480>] (do_DataAbort+0x38/0x98) from [<c030f50c>] (ret_from_exception+0x0/0x10)
[   86.830444] Exception stack(0xc7907fb0 to 0xc7907ff8)
[   86.835723] 7fa0:                                     402140ea 4005c950 be91587c 4005cb08
[   86.844299] 7fc0: 00000015 0001b900 000004b0 4021051c 4021c998 402140ea 4032d068 be91579c
[   86.852874] 7fe0: 00000000 be9156d0 400403f8 4003e6f4 20000010 ffffffff


=== [2]

[root at OMAP3EVM cpufreq]# cat /proc/cpuinfo
[   99.083923] Unable to handle kernel paging request at virtual address 85d44831
[   99.091583] pgd = c5d30000
[   99.094390] [85d44831] *pgd=00000000
[   99.098175] Internal error: Oops: 5 [#1]
[   99.102264] Modules linked in:
[   99.105468] CPU: 0    Not tainted  (3.0.0-rc3-14002-g40b6752-dirty #22)
[   99.112365] PC is at find_get_pages+0xd8/0x170
[   99.117004] LR is at 0x2
[   99.119659] pc : [<c00d3cc4>]    lr : [<00000002>]    psr: 60000113
[   99.119659] sp : c7945d08  ip : 00000001  fp : c7945d58
[   99.131652] r10: c759b89c  r9 : c5d3e860  r8 : c7945d58
[   99.137115] r7 : 00000013  r6 : 00000000  r5 : c5d37900  r4 : c5d31000
[   99.143951] r3 : c759b758  r2 : 85d44831  r1 : 00000001  r0 : c759b89c
[   99.150756] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
[   99.158203] Control: 10c5387d  Table: 85d30019  DAC: 00000015
[   99.164215] Process sh (pid: 446, stack limit = 0xc79442f0)
[   99.170043] Stack: (0xc7945d08 to 0xc7946000)
[   99.174591] 5d00:                   c78cc280 c78cc6c0 00000001 00000001 c759b758 00000001
[   99.183135] 5d20: c5d472b4 c5d31000 00000000 00000000 c5d3e860 c7945d58 400d3000 c5d3e860
[   99.191680] 5d40: c5d47280 c00edf20 00000129 00000000 00000000 c5d472e8 00000000 00000013
[   99.200256] 5d60: 400d3000 00000000 00000002 00000013 c5d31000 00000000 400d3000 c5d47280
[   99.208801] 5d80: 400d3000 c5d3e860 00000000 c00eedf8 00000013 00000000 00000000 c00a1114
[   99.217346] 5da0: 00000002 c5d47280 00000000 c02e4760 00000000 c5d30000 c5d31000 00000200
[   99.225891] 5dc0: 400d3000 c5d47280 00000000 c5d3e860 00000000 c00ef89c c5d31000 00000000
[   99.234436] 5de0: c5d3e860 c7945fb0 00000007 400d3c6c c5d47280 400584b0 c5d472d4 c02e4948
[   99.242980] 5e00: 00000000 c02e47d0 c0413fc0 c043f468 80000007 c78cc280 0000081f c78cc280
[   99.251525] 5e20: 00000002 60000093 00000000 c00a1114 00000000 c042c204 00000000 00000001
[   99.260070] 5e40: c045d510 00000044 00000006 c5e3c000 00000000 00000000 c78cc280 00000001
[   99.268646] 5e60: c78cc280 c02e258c 00000000 c781e8c0 c5e3c000 c009dd48 c5e3c0ac 60000013
[   99.277191] 5e80: 00000000 c78cc6c0 00000044 00000000 beacd7ec c0229940 00000001 00000000
[   99.285736] 5ea0: c0229894 0000000b 20000113 00000000 00000002 00000000 c78cc280 00000000
[   99.294281] 5ec0: 00000000 00000000 00000000 c5e32694 00000002 60000093 00000000 c00a1114
[   99.302825] 5ee0: 00000002 c03fff0c 000d72cc 00000007 c03fff7c c7945fb0 400584b0 400d3c6c
[   99.311370] 5f00: 00000000 c0034208 000d7008 c00342a4 c78cc280 c02e2554 c7945f84 c78cc280
[   99.319915] 5f20: c5e321ac c009dd48 c5e32684 00000003 c7945f84 c02e2554 0000000a c007b0a8
[   99.328491] 5f40: c7944000 00000000 000d7008 00000000 c7945f84 00000003 00000000 c7945f84
[   99.337036] 5f60: 000000ae c003f8a8 00000000 c007b26c c78cc280 c02e2c7c 00000000 000d727c
[   99.345581] 5f80: 400584b0 c009dd48 ffffffff 000d72cc 00000000 ffffffff 000d72cc 00000000
[   99.354125] 5fa0: 000d727c 400584b0 000d727c c02e2c8c 000d72cc 000d726c 000d727c 000d6dfc
[   99.362670] 5fc0: 000d726c 000d72cc 00000000 000d727c 400584b0 400584b0 000d727c 00000000
[   99.371215] 5fe0: 000d450c beacd804 000d4104 400d3c6c 60000010 ffffffff 00000000 00000000
[   99.379791] [<c00d3cc4>] (find_get_pages+0xd8/0x170) from [<c5d3e860>] (0xc5d3e860)
[   99.387786] Code: e1a0c001 eafffff2 e5942000 e5903000 (e5923000)
[   99.394195] ---[ end trace a04771993a740cbe ]---
Segmentation fault
[root at OMAP3EVM cpufreq]#

=== [3]

[root at OMAP3EVM cpufreq]# cat /proc/cpuinfo
[   90.313537] BUG: looking up invalid subclass: 3348054496
[   90.319122] turning off the locking correctness validator.
[   90.324859] [<c00509c4>] (unwind_backtrace+0x0/0xf8) from [<c00ba6b4>] (__lock_acquire+0x1984/0x1d90)
[   90.334503] [<c00ba6b4>] (__lock_acquire+0x1984/0x1d90) from [<c00bb088>] (lock_acquire+0xd4/0xf8)
[   90.343902] [<c00bb088>] (lock_acquire+0xd4/0xf8) from [<c030e780>] (_raw_spin_lock_irqsave+0x44/0x58)
[   90.353637] [<c030e780>] (_raw_spin_lock_irqsave+0x44/0x58) from [<c021e44c>] (__down_read_trylock+0x14/0x54)
[   90.364013] [<c021e44c>] (__down_read_trylock+0x14/0x54) from [<c00a69a4>] (down_read_trylock+0x14/0x54)
[   90.373931] [<c00a69a4>] (down_read_trylock+0x14/0x54) from [<c03111a0>] (do_page_fault+0xcc/0x358)
[   90.383392] [<c03111a0>] (do_page_fault+0xcc/0x358) from [<c003e480>] (do_DataAbort+0x38/0x98)
[   90.392395] [<c003e480>] (do_DataAbort+0x38/0x98) from [<c030f50c>] (ret_from_exception+0x0/0x10)
[   90.401672] Exception stack(0xc796ffb0 to 0xc796fff8)

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Santosh Shilimkar]

Russell,

On 6/24/2011 8:12 AM, Russell King - ARM Linux wrote:
> Right, thanks for the file.  Here's the patch.
>

[.....]

> Notice how we adjust _both_ the per-cpu loops_per_jiffy, and that we
> adjust them with reference to the initial values.
>
> If you adjust lpj with reference to the last, then you _will_ build up
> a progressively bigger and bigger error in the value over time.
>
Thanks Russell for the change. This change should fix the global
lpj for UP machine as well when build with SMP_ON_UP.

Can you have a look at below complete change which should
make the BOGOMIPS happy on all OMAP2PLUS machines. Generated
against Kevin's cpufreq branch.

url = 
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git 
pm-wip/cpufreq.

Just compile tested with UP and SMP OMAP builds. After your
review, I can give a test.

Regards
Santosh

 From 9a6154c0f68e39c4d1fbc4ef3fef5ce577ba87d4 Mon Sep 17 00:00:00 2001
From: Russell King <rmk+kernel@arm.linux.org.uk>
Date: Fri, 24 Jun 2011 10:51:17 -0700
Subject: [PATCH] OMAP2+: CPUfreq: update lpj with refernce value to 
avoid progressive error.

Adjust _both_ the per-cpu loops_per_jiffy and global lpj. Calibrate them
with with reference to the initial values to avoid a progressively
bigger and bigger error in the value over time.

While at this also re-use the notifiers for UP/SMP since on
UP machine or UP_ON_SMP policy->cpus mask would contain only
the one CPU.

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
[santosh.shilimkar at ti.com: rebased against omap cpufreq upstream branch]
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Kevin Hilman <khilman@ti.com>
---
  arch/arm/mach-omap2/omap2plus-cpufreq.c |   48 
+++++++++++++++++-------------
  1 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c 
b/arch/arm/mach-omap2/omap2plus-cpufreq.c
index 1f3b2e1..434698e 100644
--- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
+++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
@@ -38,6 +38,16 @@

  #include <mach/hardware.h>

+#ifdef CONFIG_SMP
+struct lpj_info {
+	unsigned long	ref;
+	unsigned int	freq;
+};
+
+static DEFINE_PER_CPU(struct lpj_info, lpj_ref);
+static struct lpj_info global_lpj_ref;
+#endif
+
  static struct cpufreq_frequency_table *freq_table;
  static atomic_t freq_table_users = ATOMIC_INIT(0);
  static struct clk *mpu_clk;
@@ -96,11 +106,6 @@ 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;
@@ -114,19 +119,7 @@ set_freq:

  	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
  	/*
@@ -134,10 +127,24 @@ set_freq:
  	 * cpufreq driver. So, update the per-CPU loops_per_jiffy value
  	 * on frequency transition. We need to update all dependent CPUs.
  	 */
-	for_each_cpu(i, policy->cpus)
+	for_each_cpu(i, policy->cpus) {
+		struct lpj_info *lpj = &per_cpu(lpj_ref, i);
+		if (!lpj->freq) {
+			lpj->ref = per_cpu(cpu_data, i).loops_per_jiffy;
+			lpj->freq = freqs.old;
+		}
+
  		per_cpu(cpu_data, i).loops_per_jiffy =
-			cpufreq_scale(per_cpu(cpu_data, i).loops_per_jiffy,
-					freqs.old, freqs.new);
+			cpufreq_scale(lpj->ref, lpj->freq, freqs.new);
+	}
+
+	/* And don't forget to adjust the global one */
+	if (!global_lpj_ref.freq) {
+		global_lpj_ref.ref = loops_per_jiffy;
+		global_lpj_ref.freq = freqs.old;
+	}
+	loops_per_jiffy = cpufreq_scale(global_lpj_ref.ref, global_lpj_ref.freq,
+					freqs.new);
  #endif

  	/* notifiers */
@@ -146,7 +153,6 @@ set_freq:
  		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
  	}

-skip_lpj:
  	return ret;
  }

-- 
1.7.4.1

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Russell King - ARM Linux]

On Fri, Jun 24, 2011 at 11:20:44PM +0530, Premi, Sanjeev wrote:
> I was able to test BogoMIPS calculations via /proc/cpuinfo for
> both with & without CONFIG_SMP selected.
> 
> For most part things work fine - but I do notice occassional Oops
> and segmentation faults while doing "cat /proc/cpuinfo"
> 
> With CONFIG_SMP enabled, system doesn't recover from the Oops;
> but without SMP - I noticed segmentation faults/ BUG but system
> does recover.
> 
> They could be unrelated - but i didn't see any of these earlier
> today. I will continue debug on MON.

I don't think these are related to the patch - I think there's something
up with your hardware.

Let's take the first.

> [root at OMAP3EVM cpufreq]# cat /proc/cpuinfo
> [   73.832366] Internal error: Oops - undefined instruction: 0 [#1] SMP

Ok an undefined instruction.  So...

> [   73.839019] Modules linked in:
> [   73.842193] CPU: 0    Not tainted  (3.0.0-rc3-14002-g40b6752-dirty #21)
> [   73.849121] PC is at __do_fault+0x1c0/0x450
> [   73.853485] LR is at __do_fault+0x2b0/0x450
> [   73.857879] pc : [<c010fa18>]    lr : [<c010fb08>]    psr: 00000113
> [   73.857879] sp : c7907d48  ip : 00000000  fp : c5d518c0
> [   73.869873] r10: 00000200  r9 : 40214000  r8 : 00000000
> [   73.875335] r7 : c2692f98  r6 : c0ad7600  r5 : 87fb018f  r4 : 00000000
> [   73.882141] r3 : 87fb0a3e  r2 : 00000800  r1 : 87fb01cf  r0 : c5d518c0
> [   73.888977] Flags: nzcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
> [   73.896423] Control: 10c5387d  Table: 8795c019  DAC: 00000015
> [   73.902435] Process cat (pid: 449, stack limit = 0xc79062f8)

... lets look at the code line:

> [   74.176879] Code: e1a01005 e3a02000 ebfd1694 e59d0014 (eb07fcba)

and disassemble it:

   0:   e1a01005        mov     r1, r5
   4:   e3a02000        mov     r2, #0  ; 0x0
   8:   ebfd1694        bl      0xfff45a60
   c:   e59d0014        ldr     r0, [sp, #20]
  10:   eb07fcba        bl      0x1ff300

There is no way that 0xeb07fcba should ever cause an undefined ARM
instruction on a properly functioning system.

It points at a hardware problem - are you using a socketed SoC?  Is
it properly socketed?  Is the socket dirty?  And all other questions
related to hardware integrity...

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Kevin Hilman]

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

> On Fri, Jun 24, 2011 at 11:20:44PM +0530, Premi, Sanjeev wrote:
>> I was able to test BogoMIPS calculations via /proc/cpuinfo for
>> both with & without CONFIG_SMP selected.
>> 
>> For most part things work fine - but I do notice occassional Oops
>> and segmentation faults while doing "cat /proc/cpuinfo"
>> 
>> With CONFIG_SMP enabled, system doesn't recover from the Oops;
>> but without SMP - I noticed segmentation faults/ BUG but system
>> does recover.
>> 
>> They could be unrelated - but i didn't see any of these earlier
>> today. I will continue debug on MON.
>
> I don't think these are related to the patch - I think there's something
> up with your hardware.
>
> Let's take the first.
>
>> [root at OMAP3EVM cpufreq]# cat /proc/cpuinfo
>> [   73.832366] Internal error: Oops - undefined instruction: 0 [#1] SMP
>
> Ok an undefined instruction.  So...
>
>> [   73.839019] Modules linked in:
>> [   73.842193] CPU: 0    Not tainted  (3.0.0-rc3-14002-g40b6752-dirty #21)
>> [   73.849121] PC is at __do_fault+0x1c0/0x450
>> [   73.853485] LR is at __do_fault+0x2b0/0x450
>> [   73.857879] pc : [<c010fa18>]    lr : [<c010fb08>]    psr: 00000113
>> [   73.857879] sp : c7907d48  ip : 00000000  fp : c5d518c0
>> [   73.869873] r10: 00000200  r9 : 40214000  r8 : 00000000
>> [   73.875335] r7 : c2692f98  r6 : c0ad7600  r5 : 87fb018f  r4 : 00000000
>> [   73.882141] r3 : 87fb0a3e  r2 : 00000800  r1 : 87fb01cf  r0 : c5d518c0
>> [   73.888977] Flags: nzcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
>> [   73.896423] Control: 10c5387d  Table: 8795c019  DAC: 00000015
>> [   73.902435] Process cat (pid: 449, stack limit = 0xc79062f8)
>
> ... lets look at the code line:
>
>> [   74.176879] Code: e1a01005 e3a02000 ebfd1694 e59d0014 (eb07fcba)
>
> and disassemble it:
>
>    0:   e1a01005        mov     r1, r5
>    4:   e3a02000        mov     r2, #0  ; 0x0
>    8:   ebfd1694        bl      0xfff45a60
>    c:   e59d0014        ldr     r0, [sp, #20]
>   10:   eb07fcba        bl      0x1ff300
>
> There is no way that 0xeb07fcba should ever cause an undefined ARM
> instruction on a properly functioning system.
>
> It points at a hardware problem - are you using a socketed SoC?  Is
> it properly socketed?  Is the socket dirty?  And all other questions
> related to hardware integrity...

And in particular, since we're talking CPUfreq, are you running at a
frequency that the SoC and especially the memory support?

Kevin

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Premi, Sanjeev]

> -----Original Message-----
> From: Hilman, Kevin 
> Sent: Saturday, June 25, 2011 1:44 AM
> To: Russell King - ARM Linux
> Cc: Premi, Sanjeev; linux-omap at vger.kernel.org; 
> linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCHv2] omap2+: pm: cpufreq: Fix 
> loops_per_jiffy calculation
> 
> Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
> 
> > On Fri, Jun 24, 2011 at 11:20:44PM +0530, Premi, Sanjeev wrote:
> >> I was able to test BogoMIPS calculations via /proc/cpuinfo for
> >> both with & without CONFIG_SMP selected.
> >> 
> >> For most part things work fine - but I do notice occassional Oops
> >> and segmentation faults while doing "cat /proc/cpuinfo"
> >> 
> >> With CONFIG_SMP enabled, system doesn't recover from the Oops;
> >> but without SMP - I noticed segmentation faults/ BUG but system
> >> does recover.
> >> 
> >> They could be unrelated - but i didn't see any of these earlier
> >> today. I will continue debug on MON.
> >
> > I don't think these are related to the patch - I think 
> there's something
> > up with your hardware.
> >
> > Let's take the first.
> >
> >> [root at OMAP3EVM cpufreq]# cat /proc/cpuinfo
> >> [   73.832366] Internal error: Oops - undefined 
> instruction: 0 [#1] SMP
> >
> > Ok an undefined instruction.  So...
> >
> >> [   73.839019] Modules linked in:
> >> [   73.842193] CPU: 0    Not tainted  
> (3.0.0-rc3-14002-g40b6752-dirty #21)
> >> [   73.849121] PC is at __do_fault+0x1c0/0x450
> >> [   73.853485] LR is at __do_fault+0x2b0/0x450
> >> [   73.857879] pc : [<c010fa18>]    lr : [<c010fb08>]    
> psr: 00000113
> >> [   73.857879] sp : c7907d48  ip : 00000000  fp : c5d518c0
> >> [   73.869873] r10: 00000200  r9 : 40214000  r8 : 00000000
> >> [   73.875335] r7 : c2692f98  r6 : c0ad7600  r5 : 87fb018f 
>  r4 : 00000000
> >> [   73.882141] r3 : 87fb0a3e  r2 : 00000800  r1 : 87fb01cf 
>  r0 : c5d518c0
> >> [   73.888977] Flags: nzcv  IRQs on  FIQs on  Mode SVC_32  
> ISA ARM  Segment user
> >> [   73.896423] Control: 10c5387d  Table: 8795c019  DAC: 00000015
> >> [   73.902435] Process cat (pid: 449, stack limit = 0xc79062f8)
> >
> > ... lets look at the code line:
> >
> >> [   74.176879] Code: e1a01005 e3a02000 ebfd1694 e59d0014 (eb07fcba)
> >
> > and disassemble it:
> >
> >    0:   e1a01005        mov     r1, r5
> >    4:   e3a02000        mov     r2, #0  ; 0x0
> >    8:   ebfd1694        bl      0xfff45a60
> >    c:   e59d0014        ldr     r0, [sp, #20]
> >   10:   eb07fcba        bl      0x1ff300
> >
> > There is no way that 0xeb07fcba should ever cause an undefined ARM
> > instruction on a properly functioning system.
> >
> > It points at a hardware problem - are you using a socketed SoC?  Is
> > it properly socketed?  Is the socket dirty?  And all other questions
> > related to hardware integrity...
> 
> And in particular, since we're talking CPUfreq, are you running at a
> frequency that the SoC and especially the memory support?

Yes. the frequencies are 300 - 800MHz range. The same board is also quite
stable for 1GHz operations (tested ARM only) - with sources hosted at:
http://arago-project.org/git/projects/?p=linux-omap3.git;a=summary

For testing, I was changing frequencies in a tight 'forever' loop. But, as
I mentioned earlier that issues could be unrelated. And the loop could be
exposing something else.

~sanjeev

> 
> Kevin
> 
> 
> 

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Premi, Sanjeev]

> -----Original Message-----
> From: Shilimkar, Santosh 
> Sent: Saturday, June 25, 2011 12:18 AM
> To: Russell King - ARM Linux
> Cc: Premi, Sanjeev; linux-omap at vger.kernel.org; 
> linux-arm-kernel at lists.infradead.org; Hilman, Kevin
> Subject: Re: [PATCHv2] omap2+: pm: cpufreq: Fix 
> loops_per_jiffy calculation
> 
> Russell,
> 
> On 6/24/2011 8:12 AM, Russell King - ARM Linux wrote:
> > Right, thanks for the file.  Here's the patch.
> >
> 
> [.....]
> 
> > Notice how we adjust _both_ the per-cpu loops_per_jiffy, and that we
> > adjust them with reference to the initial values.
> >
> > If you adjust lpj with reference to the last, then you 
> _will_ build up
> > a progressively bigger and bigger error in the value over time.
> >
> Thanks Russell for the change. This change should fix the global
> lpj for UP machine as well when build with SMP_ON_UP.
> 
> Can you have a look at below complete change which should
> make the BOGOMIPS happy on all OMAP2PLUS machines. Generated
> against Kevin's cpufreq branch.
> 
> url = 
> git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-om
> ap-pm.git 
> pm-wip/cpufreq.
> 
> Just compile tested with UP and SMP OMAP builds. After your
> review, I can give a test.
> 
> Regards
> Santosh
> 
>  From 9a6154c0f68e39c4d1fbc4ef3fef5ce577ba87d4 Mon Sep 17 
> 00:00:00 2001
> From: Russell King <rmk+kernel@arm.linux.org.uk>
> Date: Fri, 24 Jun 2011 10:51:17 -0700
> Subject: [PATCH] OMAP2+: CPUfreq: update lpj with refernce value to 
> avoid progressive error.
> 
> Adjust _both_ the per-cpu loops_per_jiffy and global lpj. 
> Calibrate them
> with with reference to the initial values to avoid a progressively
> bigger and bigger error in the value over time.
> 
> While at this also re-use the notifiers for UP/SMP since on
> UP machine or UP_ON_SMP policy->cpus mask would contain only
> the one CPU.
> 
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> [santosh.shilimkar at ti.com: rebased against omap cpufreq 
> upstream branch]
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Kevin Hilman <khilman@ti.com>

[sp] I thought we were solving a problem - but this makes it
     look like race for addding sign-offs - which I am not
     interested in.

[snip]...[snip]

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Russell King - ARM Linux]

On Sun, Jun 26, 2011 at 12:23:31AM +0530, Premi, Sanjeev wrote:
> [sp] I thought we were solving a problem - but this makes it
>      look like race for addding sign-offs - which I am not
>      interested in.

No, it's called packaging the patch up and getting it ready, putting it
out on the list for people to test and provide Tested-by's, acked-by's
etc.

Would you rather people sat on fixes doing nothing with them for a
month instead, watching broken -rc after broken -rc going by?

[PATCHv2] 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: Sunday, June 26, 2011 12:39 AM
> To: Premi, Sanjeev
> Cc: Shilimkar, Santosh; linux-omap at vger.kernel.org; 
> linux-arm-kernel at lists.infradead.org; Hilman, Kevin
> Subject: Re: [PATCHv2] omap2+: pm: cpufreq: Fix 
> loops_per_jiffy calculation
> 
> On Sun, Jun 26, 2011 at 12:23:31AM +0530, Premi, Sanjeev wrote:
> > [sp] I thought we were solving a problem - but this makes it
> >      look like race for addding sign-offs - which I am not
> >      interested in.
> 
> No, it's called packaging the patch up and getting it ready, 
> putting it
> out on the list for people to test and provide Tested-by's, acked-by's
> etc.

[sp] Agree.

> 
> Would you rather people sat on fixes doing nothing with them for a
> month instead, watching broken -rc after broken -rc going by?
>
[sp] The original patch was just few hours ago... not month(s).

~sanjeev

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Russell King - ARM Linux]

On Mon, Jun 27, 2011 at 10:24:43AM +0530, Premi, Sanjeev wrote:
> > -----Original Message-----
> > From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk] 
> > Sent: Sunday, June 26, 2011 12:39 AM
> > To: Premi, Sanjeev
> > Cc: Shilimkar, Santosh; linux-omap at vger.kernel.org; 
> > linux-arm-kernel at lists.infradead.org; Hilman, Kevin
> > Subject: Re: [PATCHv2] omap2+: pm: cpufreq: Fix 
> > loops_per_jiffy calculation
> > 
> > On Sun, Jun 26, 2011 at 12:23:31AM +0530, Premi, Sanjeev wrote:
> > > [sp] I thought we were solving a problem - but this makes it
> > >      look like race for addding sign-offs - which I am not
> > >      interested in.
> > 
> > No, it's called packaging the patch up and getting it ready, 
> > putting it
> > out on the list for people to test and provide Tested-by's, acked-by's
> > etc.
> 
> [sp] Agree.
> 
> > 
> > Would you rather people sat on fixes doing nothing with them for a
> > month instead, watching broken -rc after broken -rc going by?
> >
> [sp] The original patch was just few hours ago... not month(s).

I fail to see what the problem is you're referring to.

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Colin Cross]

<resending as plain text>

On Fri, Jun 24, 2011 at 6:53 AM, Sanjeev Premi <premi@ti.com> wrote:
>
> Currently, loops_per_jiffy is being calculated twice for
> non-SMP processors.
> ?- Before calling cpufreq_notify_transition()
> ?- From within cpufreq_notify_transition()
>
> Double adjustment leads to incorrect value being assigned to
> loops_per_jiffy. This manifests as incorrect BogoMIPS in
> "cat /proc/cpuinfo".
>
> The value of loops_per_jiffy needs to be calculated only
> when CONFIG_SMP is true. It is the core change included
> in this patch.
>
> The patch also leverages the definition of for_each_cpu()
> with and without CONFIG_SMP to consolidate the mechanism
> to call cpufreq_notify_transition().
>
> Signed-off-by: Sanjeev Premi <premi@ti.com>
> ---
> ?Changes since v1:
> ? * loops_per_jiffy are updated when CONFIG_SMP is true.
> ? * leverage definition of for_each_cpu()
>
> ?Tested on OMAP3EVM with and without CONFIG_SMP.
> ?Since the log is rather long, will be posting the log in
> ?a follow-up mail.
>
> ?arch/arm/mach-omap2/omap2plus-cpufreq.c | ? 27 +++++++++++++++------------
> ?1 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
> index 346519e..0263cae 100644
> --- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
> +++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
> @@ -97,12 +97,8 @@ static int omap_target(struct cpufreq_policy *policy,
> ? ? ? ? ? ? ? ?return ret;
>
> ? ? ? ?/* Notify transitions */
> - ? ? ? if (is_smp()) {
> - ? ? ? ? ? ? ? for_each_cpu(i, policy->cpus) {
> - ? ? ? ? ? ? ? ? ? ? ? freqs.cpu = i;
> - ? ? ? ? ? ? ? ? ? ? ? cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> - ? ? ? ? ? ? ? }
> - ? ? ? } else {
> + ? ? ? for_each_cpu(i, policy->cpus) {
> + ? ? ? ? ? ? ? freqs.cpu = i;
> ? ? ? ? ? ? ? ?cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> ? ? ? ?}
>
> @@ -114,13 +110,20 @@ static int omap_target(struct cpufreq_policy *policy,
>
> ? ? ? ?freqs.new = omap_getspeed(policy->cpu);
>
> +#ifdef CONFIG_SMP
> + ? ? ? /* Adjust jiffies before transition */
> + ? ? ? for_each_cpu(i, policy->cpus) {
> + ? ? ? ? ? ? ? unsigned long lpj = per_cpu(cpu_data, i).loops_per_jiffy;
> +
> + ? ? ? ? ? ? ? per_cpu(cpu_data, i).loops_per_jiffy = cpufreq_scale(lpj,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? freqs.old,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? freqs.new);
Can't this rewrite the loops_per_jiffy for the other CPU while it is
in a udelay?  If it has already calculated the number of loops
necessary, and the CPU frequency increases, it could end up returning
too early from udelay.

There were previous discussions about polling a fixed-frequency timer
for udelay on SMP systems.

> + ? ? ? }
> +#endif
> +
> ? ? ? ?/* Notify transitions */
> - ? ? ? if (is_smp()) {
> - ? ? ? ? ? ? ? for_each_cpu(i, policy->cpus) {
> - ? ? ? ? ? ? ? ? ? ? ? freqs.cpu = i;
> - ? ? ? ? ? ? ? ? ? ? ? cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> - ? ? ? ? ? ? ? }
> - ? ? ? } else {
> + ? ? ? for_each_cpu(i, policy->cpus) {
> + ? ? ? ? ? ? ? freqs.cpu = i;
> ? ? ? ? ? ? ? ?cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> ? ? ? ?}
>
> --
> 1.7.2.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Santosh Shilimkar]

On 6/28/2011 3:29 PM, Colin Cross wrote:
> <resending as plain text>
>
> On Fri, Jun 24, 2011 at 6:53 AM, Sanjeev Premi<premi@ti.com>  wrote:
>>

>> +#ifdef CONFIG_SMP
>> +       /* Adjust jiffies before transition */
>> +       for_each_cpu(i, policy->cpus) {
>> +               unsigned long lpj = per_cpu(cpu_data, i).loops_per_jiffy;
>> +
>> +               per_cpu(cpu_data, i).loops_per_jiffy = cpufreq_scale(lpj,
>> +                                                       freqs.old,
>> +                                                       freqs.new);

This isn't the right patch since it does suffer from the progressive
error. There was an updated patch on this thread from Russell which
I re-based.
> Can't this rewrite the loops_per_jiffy for the other CPU while it is
> in a udelay?  If it has already calculated the number of loops
> necessary, and the CPU frequency increases, it could end up returning
> too early from udelay.
>
> There were previous discussions about polling a fixed-frequency timer
> for udelay on SMP systems.
>
The udelay code doesn't use the per-cpu lpj variable. It uses the global
lpj. Secondly the calibration of no. of loops to be done is
precalculateed so overwrite shouldn't impact the scenario you mentioned.

Though it has an issue where, pre-calculated loops can become short/long
based on new clock change which impacts both CPU's on OMAP, when the 
other CPU is in in the middle of u-delay routine..

When CPU can scale independently, then we have bigger problem since
global lpj based udelay becomes always error prone in all scenario's.

So for the OMAP, where the whole CPU cluster is scaled together,
I don't see this as a bigger problem.

Regards
Santosh

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Russell King - ARM Linux]

On Tue, Jun 28, 2011 at 03:29:57PM -0700, Colin Cross wrote:
> Can't this rewrite the loops_per_jiffy for the other CPU while it is
> in a udelay?  If it has already calculated the number of loops
> necessary, and the CPU frequency increases, it could end up returning
> too early from udelay.

udelay uses the global loops_per_jiffy.

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Colin Cross]

<resending as plain text again, sorry, mailer troubles>

On Tue, Jun 28, 2011 at 3:45 PM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
>
> On 6/28/2011 3:29 PM, Colin Cross wrote:
>>
>> <resending as plain text>
>>
>> On Fri, Jun 24, 2011 at 6:53 AM, Sanjeev Premi<premi@ti.com> ?wrote:
>>>
>
>>> +#ifdef CONFIG_SMP
>>> + ? ? ? /* Adjust jiffies before transition */
>>> + ? ? ? for_each_cpu(i, policy->cpus) {
>>> + ? ? ? ? ? ? ? unsigned long lpj = per_cpu(cpu_data, i).loops_per_jiffy;
>>> +
>>> + ? ? ? ? ? ? ? per_cpu(cpu_data, i).loops_per_jiffy = cpufreq_scale(lpj,
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? freqs.old,
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? freqs.new);
>
> This isn't the right patch since it does suffer from the progressive
> error. There was an updated patch on this thread from Russell which
> I re-based.

Both patches have the same issue - they update loops_per_jiffy, which
is unsafe on an SMP system.

>>
>> Can't this rewrite the loops_per_jiffy for the other CPU while it is
>> in a udelay? ?If it has already calculated the number of loops
>> necessary, and the CPU frequency increases, it could end up returning
>> too early from udelay.
>>
>> There were previous discussions about polling a fixed-frequency timer
>> for udelay on SMP systems.
>>
> The udelay code doesn't use the per-cpu lpj variable. It uses the global
> lpj. Secondly the calibration of no. of loops to be done is
> precalculateed so overwrite shouldn't impact the scenario you mentioned.
>
> Though it has an issue where, pre-calculated loops can become short/long
> based on new clock change which impacts both CPU's on OMAP, when the other CPU is in in the middle of u-delay routine..

The precalculated loops is exactly the problem I described.
udelay(100) can return in 50 microseconds if the cpu speed is doubled.
 On OMAP4, frequencies can range from 350Mhz to 1.5GHz, so udelay can
be more than 4 times too short.  That breaks the guarantees of udelay.

> When CPU can scale independently, then we have bigger problem since
> global lpj based udelay becomes always error prone in all scenario's.
>
> So for the OMAP, where the whole CPU cluster is scaled together,
> I don't see this as a bigger problem.
>
> Regards
> Santosh
>
>
>

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Colin Cross]

On Tue, Jun 28, 2011 at 3:55 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Jun 28, 2011 at 03:29:57PM -0700, Colin Cross wrote:
>> Can't this rewrite the loops_per_jiffy for the other CPU while it is
>> in a udelay? ?If it has already calculated the number of loops
>> necessary, and the CPU frequency increases, it could end up returning
>> too early from udelay.
>
> udelay uses the global loops_per_jiffy.
>

The problem is still the same - loops_per_jiffy applies to both CPUs,
and the frequency of the other CPU cannot be changed if it is in a
udelay.

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Santosh Shilimkar]

On 6/28/2011 3:53 PM, Colin Cross wrote:
> On Tue, Jun 28, 2011 at 3:45 PM, Santosh Shilimkar
> <santosh.shilimkar at ti.com <mailto:santosh.shilimkar@ti.com>> wrote:

[....]

>         Can't this rewrite the loops_per_jiffy for the other CPU while it is
>         in a udelay?  If it has already calculated the number of loops
>         necessary, and the CPU frequency increases, it could end up
>         returning
>         too early from udelay.
>
>         There were previous discussions about polling a fixed-frequency
>         timer
>         for udelay on SMP systems.
>
>     The udelay code doesn't use the per-cpu lpj variable. It uses the global
>     lpj. Secondly the calibration of no. of loops to be done is
>     precalculateed so overwrite shouldn't impact the scenario you mentioned.
>
>     Though it has an issue where, pre-calculated loops can become short/long
>     based on new clock change which impacts both CPU's on OMAP, when the
>     other CPU is in in the middle of u-delay routine..
>
>
> The precalculated loops is exactly the problem I described.  udelay(100)
> can return in 50 microseconds if the cpu speed is doubled.  On OMAP4,
> frequencies can range from 350Mhz to 1.5GHz, so udelay can be more than
> 4 times too short.  That breaks the guarantees of udelay.
>
You have a point and I agree with you on above.
And to fix that scenrio, the only option is to use hardware
timer based u-delay() which can remain constant across the
CPU freq change.

Regards,
Santosh

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Russell King - ARM Linux]

On Tue, Jun 28, 2011 at 03:45:22PM -0700, Santosh Shilimkar wrote:
> The udelay code doesn't use the per-cpu lpj variable. It uses the global
> lpj. Secondly the calibration of no. of loops to be done is
> precalculateed so overwrite shouldn't impact the scenario you mentioned.
>
> Though it has an issue where, pre-calculated loops can become short/long
> based on new clock change which impacts both CPU's on OMAP, when the  
> other CPU is in in the middle of u-delay routine..

And there's also the issue where you can start a udelay loop on one CPU,
be pre-empted and end up running it on a different CPU running at a
different speed.

The thing to bear in mind is that udelays are approximate at best - I did
some investigation into the accuracy of the loops_per_jiffy calculation,
and it _will_ produce shorter delays than expected by the fact that
what is being calibrated is the udelay() loop _plus_ the timer interrupt
overhead.

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Santosh Shilimkar]

On 6/28/2011 4:00 PM, Santosh Shilimkar wrote:
> On 6/28/2011 3:53 PM, Colin Cross wrote:
>> On Tue, Jun 28, 2011 at 3:45 PM, Santosh Shilimkar
>> <santosh.shilimkar at ti.com <mailto:santosh.shilimkar@ti.com>> wrote:
>
> [....]
>
>> Can't this rewrite the loops_per_jiffy for the other CPU while it is
>> in a udelay? If it has already calculated the number of loops
>> necessary, and the CPU frequency increases, it could end up
>> returning
>> too early from udelay.
>>
>> There were previous discussions about polling a fixed-frequency
>> timer
>> for udelay on SMP systems.
>>
>> The udelay code doesn't use the per-cpu lpj variable. It uses the global
>> lpj. Secondly the calibration of no. of loops to be done is
>> precalculateed so overwrite shouldn't impact the scenario you mentioned.
>>
>> Though it has an issue where, pre-calculated loops can become short/long
>> based on new clock change which impacts both CPU's on OMAP, when the
>> other CPU is in in the middle of u-delay routine..
>>
>>
>> The precalculated loops is exactly the problem I described. udelay(100)
>> can return in 50 microseconds if the cpu speed is doubled. On OMAP4,
>> frequencies can range from 350Mhz to 1.5GHz, so udelay can be more than
>> 4 times too short. That breaks the guarantees of udelay.
>>
> You have a point and I agree with you on above.
> And to fix that scenrio, the only option is to use hardware
> timer based u-delay() which can remain constant across the
> CPU freq change.
>
Or block CPUFREQ change when any CPU which is in udelay() is done with
it, which will be stupid to do :-)

Regards
Santosh

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Santosh Shilimkar]

On 6/28/2011 4:03 PM, Russell King - ARM Linux wrote:
> On Tue, Jun 28, 2011 at 03:45:22PM -0700, Santosh Shilimkar wrote:
>> The udelay code doesn't use the per-cpu lpj variable. It uses the global
>> lpj. Secondly the calibration of no. of loops to be done is
>> precalculateed so overwrite shouldn't impact the scenario you mentioned.
>>
>> Though it has an issue where, pre-calculated loops can become short/long
>> based on new clock change which impacts both CPU's on OMAP, when the
>> other CPU is in in the middle of u-delay routine..
>
> And there's also the issue where you can start a udelay loop on one CPU,
> be pre-empted and end up running it on a different CPU running at a
> different speed.
>
Indeed.

> The thing to bear in mind is that udelays are approximate at best - I did
> some investigation into the accuracy of the loops_per_jiffy calculation,
> and it _will_ produce shorter delays than expected by the fact that
> what is being calibrated is the udelay() loop _plus_ the timer interrupt
> overhead.
 >
Sure but as Colin pointed out 4 times shorter delay will be too much.

Regards
Santosh

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Russell King - ARM Linux]

On Tue, Jun 28, 2011 at 03:58:57PM -0700, Colin Cross wrote:
> On Tue, Jun 28, 2011 at 3:55 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Tue, Jun 28, 2011 at 03:29:57PM -0700, Colin Cross wrote:
> >> Can't this rewrite the loops_per_jiffy for the other CPU while it is
> >> in a udelay? ?If it has already calculated the number of loops
> >> necessary, and the CPU frequency increases, it could end up returning
> >> too early from udelay.
> >
> > udelay uses the global loops_per_jiffy.
> >
> 
> The problem is still the same - loops_per_jiffy applies to both CPUs,
> and the frequency of the other CPU cannot be changed if it is in a
> udelay.

If you have a SMP system where both CPUs scale together then you will
get both CPUs being impacted, which may result in udelay() terminating
well early or taking much longer than was originally intended.

That's rather unavoidable with software timing loops - we could add a
rw spinlock around udelay, but that would require interrupts to be
disabled and that wouldn't be nice in general to have every udelay
running with IRQs off.

That's why people have proposed hardware-timer based delay loops -
these screw up the bogomips value (it no longer refers to the CPU
but to the timer used for the delays) and the code proposed thus far
probably has a severe negative impact on ARMs running at low clock
rates (the calculation cost of the number of loops to run becomes
significant for CPUs below 100MHz for short delays with the existing
optimized assembler, so moving it into C and introducing function
pointers will only make it worse.)

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Colin Cross]

On Tue, Jun 28, 2011 at 4:17 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Jun 28, 2011 at 03:58:57PM -0700, Colin Cross wrote:
>> On Tue, Jun 28, 2011 at 3:55 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Tue, Jun 28, 2011 at 03:29:57PM -0700, Colin Cross wrote:
>> >> Can't this rewrite the loops_per_jiffy for the other CPU while it is
>> >> in a udelay? ?If it has already calculated the number of loops
>> >> necessary, and the CPU frequency increases, it could end up returning
>> >> too early from udelay.
>> >
>> > udelay uses the global loops_per_jiffy.
>> >
>>
>> The problem is still the same - loops_per_jiffy applies to both CPUs,
>> and the frequency of the other CPU cannot be changed if it is in a
>> udelay.
>
> If you have a SMP system where both CPUs scale together then you will
> get both CPUs being impacted, which may result in udelay() terminating
> well early or taking much longer than was originally intended.
>
> That's rather unavoidable with software timing loops - we could add a
> rw spinlock around udelay, but that would require interrupts to be
> disabled and that wouldn't be nice in general to have every udelay
> running with IRQs off.
>
> That's why people have proposed hardware-timer based delay loops -
> these screw up the bogomips value (it no longer refers to the CPU
> but to the timer used for the delays) and the code proposed thus far
> probably has a severe negative impact on ARMs running at low clock
> rates (the calculation cost of the number of loops to run becomes
> significant for CPUs below 100MHz for short delays with the existing
> optimized assembler, so moving it into C and introducing function
> pointers will only make it worse.)
>

I don't think it affects bogomips - loops_per_jiffy can still be
calibrated and updated by cpufreq, udelay just wont use them.

If the pointer dereference is done at the udelay() call to allow each
platform to override udelay, slow platforms can continue to use the
original optimized assembly with only a few extra instructions
overhead on entry.

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Russell King - ARM Linux]

On Tue, Jun 28, 2011 at 04:37:08PM -0700, Colin Cross wrote:
> I don't think it affects bogomips - loops_per_jiffy can still be
> calibrated and updated by cpufreq, udelay just wont use them.

No, you can't avoid it.  __delay(), udelay(), and the global
loops_per_jiffy are all linked together - for instance this is how
the spinlock code has a 1s timeout:

static void __spin_lock_debug(raw_spinlock_t *lock)
{
        u64 loops = loops_per_jiffy * HZ;
        int print_once = 1;

        for (;;) {
                for (i = 0; i < loops; i++) {
                        if (arch_spin_trylock(&lock->raw_lock))
                                return;
                        __delay(1);
                }

which goes wrong for all the same reasons you're pointing out about
udelay().  So just restricting your argument to udelay() is not
realistic.

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Colin Cross]

On Tue, Jun 28, 2011 at 4:46 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Jun 28, 2011 at 04:37:08PM -0700, Colin Cross wrote:
>> I don't think it affects bogomips - loops_per_jiffy can still be
>> calibrated and updated by cpufreq, udelay just wont use them.
>
> No, you can't avoid it. ?__delay(), udelay(), and the global
> loops_per_jiffy are all linked together - for instance this is how
> the spinlock code has a 1s timeout:
>
> static void __spin_lock_debug(raw_spinlock_t *lock)
> {
> ? ? ? ?u64 loops = loops_per_jiffy * HZ;
> ? ? ? ?int print_once = 1;
>
> ? ? ? ?for (;;) {
> ? ? ? ? ? ? ? ?for (i = 0; i < loops; i++) {
> ? ? ? ? ? ? ? ? ? ? ? ?if (arch_spin_trylock(&lock->raw_lock))
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?return;
> ? ? ? ? ? ? ? ? ? ? ? ?__delay(1);
> ? ? ? ? ? ? ? ?}
>
> which goes wrong for all the same reasons you're pointing out about
> udelay(). ?So just restricting your argument to udelay() is not
> realistic.
>

True, there are a few other users of loops_per_jiffy and __delay, but
it looks like __spin_lock_debug is the only one worth worrying about,
and it's timing is not as critical as udelay - worst case here is that
the warning occurs after 250 ms instead of 1s.  Leaving
loops_per_jiffy and __delay intact, and fixing udelay, would still be
a net gain.

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Russell King - ARM Linux]

On Tue, Jun 28, 2011 at 04:59:57PM -0700, Colin Cross wrote:
> On Tue, Jun 28, 2011 at 4:46 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Tue, Jun 28, 2011 at 04:37:08PM -0700, Colin Cross wrote:
> >> I don't think it affects bogomips - loops_per_jiffy can still be
> >> calibrated and updated by cpufreq, udelay just wont use them.
> >
> > No, you can't avoid it. ?__delay(), udelay(), and the global
> > loops_per_jiffy are all linked together - for instance this is how
> > the spinlock code has a 1s timeout:
> >
> > static void __spin_lock_debug(raw_spinlock_t *lock)
> > {
> > ? ? ? ?u64 loops = loops_per_jiffy * HZ;
> > ? ? ? ?int print_once = 1;
> >
> > ? ? ? ?for (;;) {
> > ? ? ? ? ? ? ? ?for (i = 0; i < loops; i++) {
> > ? ? ? ? ? ? ? ? ? ? ? ?if (arch_spin_trylock(&lock->raw_lock))
> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?return;
> > ? ? ? ? ? ? ? ? ? ? ? ?__delay(1);
> > ? ? ? ? ? ? ? ?}
> >
> > which goes wrong for all the same reasons you're pointing out about
> > udelay(). ?So just restricting your argument to udelay() is not
> > realistic.
> >
> 
> True, there are a few other users of loops_per_jiffy and __delay, but
> it looks like __spin_lock_debug is the only one worth worrying about,
> and it's timing is not as critical as udelay - worst case here is that
> the warning occurs after 250 ms instead of 1s.  Leaving
> loops_per_jiffy and __delay intact, and fixing udelay, would still be
> a net gain.

Other users of loops_per_jiffy are incorrect in any case:

static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host *host,                                                   unsigned long bit)
{
        unsigned long i = 0;
        unsigned long limit = (loops_per_jiffy *
                                msecs_to_jiffies(MMC_TIMEOUT_MS));
...
        if (mmc_slot(host).features & HSMMC_HAS_UPDATED_RESET) {
                while ((!(OMAP_HSMMC_READ(host->base, SYSCTL) & bit))
                                        && (i++ < limit))
                        cpu_relax();
        }

Is rubbish.

static void omap_write_buf_pref(struct mtd_info *mtd,
                                        const u_char *buf, int len)
{
...
                /* wait for data to flushed-out before reset the prefetch */
                tim = 0;
                limit = (loops_per_jiffy *
                                        msecs_to_jiffies(OMAP_NAND_TIMEOUT_MS));                while (gpmc_read_status(GPMC_PREFETCH_COUNT) && (tim++ < limit))                        cpu_relax();

Another load of rubbish.

static int flush(struct pl022 *pl022)
{
        unsigned long limit = loops_per_jiffy << 1;

        dev_dbg(&pl022->adev->dev, "flush\n");
        do {
                while (readw(SSP_SR(pl022->virtbase)) & SSP_SR_MASK_RNE)
                        readw(SSP_DR(pl022->virtbase));
        } while ((readw(SSP_SR(pl022->virtbase)) & SSP_SR_MASK_BSY) && limit--);

Yet more...

static int flush(struct driver_data *drv_data)
{
        unsigned long limit = loops_per_jiffy << 1;

        void __iomem *reg = drv_data->ioaddr;

        do {
                while (read_SSSR(reg) & SSSR_RNE) {
                        read_SSDR(reg);
                }
        } while ((read_SSSR(reg) & SSSR_BSY) && --limit);

and...

sound/soc/samsung/i2s.c:
#define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
        /* Be patient */
        val = msecs_to_loops(1) / 1000; /* 1 usec */
        while (--val)
                cpu_relax();

even worse.

#define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
static int s3c2412_snd_lrsync(struct s3c_i2sv2_info *i2s)
{
        u32 iiscon;
        unsigned long loops = msecs_to_loops(5);

        while (--loops) {
                iiscon = readl(i2s->regs + S3C2412_IISCON);
                if (iiscon & S3C2412_IISCON_LRINDEX)
                        break;

                cpu_relax();
        }

It just goes on...

And strangely the major offender of this stuff are ARM drivers, not other
peoples stuff and not stuff in drivers/staging.

So I don't think its sane to ignore loops_per_jiffy and __delay, and just
concentrate on udelay().

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Colin Cross]

On Wed, Jun 29, 2011 at 7:00 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Jun 28, 2011 at 04:59:57PM -0700, Colin Cross wrote:
>> On Tue, Jun 28, 2011 at 4:46 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Tue, Jun 28, 2011 at 04:37:08PM -0700, Colin Cross wrote:
>> >> I don't think it affects bogomips - loops_per_jiffy can still be
>> >> calibrated and updated by cpufreq, udelay just wont use them.
>> >
>> > No, you can't avoid it. ?__delay(), udelay(), and the global
>> > loops_per_jiffy are all linked together - for instance this is how
>> > the spinlock code has a 1s timeout:
>> >
>> > static void __spin_lock_debug(raw_spinlock_t *lock)
>> > {
>> > ? ? ? ?u64 loops = loops_per_jiffy * HZ;
>> > ? ? ? ?int print_once = 1;
>> >
>> > ? ? ? ?for (;;) {
>> > ? ? ? ? ? ? ? ?for (i = 0; i < loops; i++) {
>> > ? ? ? ? ? ? ? ? ? ? ? ?if (arch_spin_trylock(&lock->raw_lock))
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?return;
>> > ? ? ? ? ? ? ? ? ? ? ? ?__delay(1);
>> > ? ? ? ? ? ? ? ?}
>> >
>> > which goes wrong for all the same reasons you're pointing out about
>> > udelay(). ?So just restricting your argument to udelay() is not
>> > realistic.
>> >
>>
>> True, there are a few other users of loops_per_jiffy and __delay, but
>> it looks like __spin_lock_debug is the only one worth worrying about,
>> and it's timing is not as critical as udelay - worst case here is that
>> the warning occurs after 250 ms instead of 1s. ?Leaving
>> loops_per_jiffy and __delay intact, and fixing udelay, would still be
>> a net gain.
>
> Other users of loops_per_jiffy are incorrect in any case:

The same conclusion I came to on a quick scan of other uses of
loops_per_jiffy...

<snip>

> And strangely the major offender of this stuff are ARM drivers, not other
> peoples stuff and not stuff in drivers/staging.
>
> So I don't think its sane to ignore loops_per_jiffy and __delay, and just
> concentrate on udelay().

But this I don't understand.  Every other use I found of
loops_per_jiffy is incorrect and should be changed.  Fixing udelay now
would not make them any worse - they would stay just as broken as they
were, so there is no need to couple a fix to udelay with fixing other
users of loops_per_jiffy.  Why block a legitimate fix because some
other broken code uses a variable whose behavior would not change?

If you are requesting an alternate change that would allow __delay to
continue to be used to time loops while irqs are off and jiffies is
not being updated, but also allows loops_per_jiffy to change in the
middle of a loop, while not increasing the number of instructions
executed in __delay, I don't think that is possible.  You could
replace __delay with a function that tests against a timer, and
loops_per_jiffy with the frequency of the counter divided by HZ, but
that would limit your spinlock spins to the frequency of the counter -
1MHz on Tegra.

Are there ever other legitimate uses of loops_per_jiffy/__delay?  I
don't think even the spinlock_debug use is correct - the number of
instructions executed in the loop before the __delay call (I count 17)
is going to be much higher than the instructions in the __delay(1)
call (3 instructions).  The spinlock debug timeout is already going to
be much longer than expected.  It looks to me like the only legitimate
use of loops_per_jiffy is to calculate the number of loops and call
__delay(loops) (exactly what udelay does), the overhead of doing
anything else will swamp the __delay call.  spinlock debug can get
away with its incorrect usage, because it doesn't really care how long
the delay is, and must have a minimum overhead.

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Stephen Boyd]

On 06/28/2011 04:17 PM, Russell King - ARM Linux wrote:
>
> That's why people have proposed hardware-timer based delay loops -
> these screw up the bogomips value (it no longer refers to the CPU
> but to the timer used for the delays) and the code proposed thus far
> probably has a severe negative impact on ARMs running at low clock
> rates (the calculation cost of the number of loops to run becomes
> significant for CPUs below 100MHz for short delays with the existing
> optimized assembler, so moving it into C and introducing function
> pointers will only make it worse.)

Am I people? ;-)

The code I've proposed doesn't seem to have a negative impact on our
targets even when the processor is running at 19.2 Mhz. Before and after
the patches I get the same lpj value (this is all described in the
commit text). I've also shown that rewriting delay.S into C doesn't
negatively affect the hand optimized assembly as the before and after
object code is nearly identical modulo register allocation. The only
issue would be the one function pointer which I haven't heard anyone
complain about until now.

Even if the time to get into the __delay() routine increases by a few
instructions I don't see how this harms anything as udelay() is a
minimum time guarantee. We should strive to make it as close as possible
to the time requested by the caller, but we shouldn't balk at the
introduction of a few more cycles along the setup path. Finally, since
the calibration takes into account most of the new instructions I doubt
it will even be noticeable overhead to have the function pointers.

What more can I do to convince you to take this patch?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

[Russell King - ARM Linux]

On Wed, Jun 29, 2011 at 11:29:29AM -0700, Stephen Boyd wrote:
> On 06/28/2011 04:17 PM, Russell King - ARM Linux wrote:
> >
> > That's why people have proposed hardware-timer based delay loops -
> > these screw up the bogomips value (it no longer refers to the CPU
> > but to the timer used for the delays) and the code proposed thus far
> > probably has a severe negative impact on ARMs running at low clock
> > rates (the calculation cost of the number of loops to run becomes
> > significant for CPUs below 100MHz for short delays with the existing
> > optimized assembler, so moving it into C and introducing function
> > pointers will only make it worse.)
> 
> Am I people? ;-)

That depends if you're a multiple personality person!

> The code I've proposed doesn't seem to have a negative impact on our
> targets even when the processor is running at 19.2 Mhz. Before and after
> the patches I get the same lpj value (this is all described in the
> commit text). I've also shown that rewriting delay.S into C doesn't
> negatively affect the hand optimized assembly as the before and after
> object code is nearly identical modulo register allocation. The only
> issue would be the one function pointer which I haven't heard anyone
> complain about until now.
> 
> Even if the time to get into the __delay() routine increases by a few
> instructions I don't see how this harms anything as udelay() is a
> minimum time guarantee. We should strive to make it as close as possible
> to the time requested by the caller, but we shouldn't balk at the
> introduction of a few more cycles along the setup path. Finally, since
> the calibration takes into account most of the new instructions I doubt
> it will even be noticeable overhead to have the function pointers.
> 
> What more can I do to convince you to take this patch?

What I'm aware of is that I did create a kernel-side parport jtag driver,
and the limiting factor in that was udelay(), or rather udelay(1) not
giving a delay of 1us but several us longer - and that was tracked down
to the overhead of the CPU getting into __delay.

So, having experienced that problem I'm over-sensitive towards it...