omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

PATCH Superseded Sanjeev Premi 6 participants 36 messages Jun 24, 2011 – Jun 29, 2011 1 message not in the archive

Subsystems (3)

Arm (32-bit) Arm sub-architectures OMAP2+ support

lore

Share & tools

lore

Premi, Sanjeev

author

2011-06-24 13:59 UTC #1 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 26 lines

-----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 <[email protected]>

Quoted 9 lines

---
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
========================

[   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

Russell King - ARM Linux

2011-06-24 14:01 UTC #2 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

In reply to Sanjeev Premi

Quoted 5 lines · Sanjeev Premi

arch/arm/mach-omap2/omap2plus-cpufreq.c +0 -1

@@ -97,12 +97,8 @@ static int omap_target(struct cpufreq_policy *policy,
 		return ret;
 
 	/* Notify transitions */
-	if (is_smp()) {

arch/arm/mach-omap2/omap2plus-cpufreq.c static int omap_target(struct cpufreq_policy *policy, · line 104

Quoted 10 lines

arch/arm/mach-omap2/omap2plus-cpufreq.c +2 -5

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

Quoted 5 lines

arch/arm/mach-omap2/omap2plus-cpufreq.c +1 -0

@@ -114,13 +110,20 @@ static int omap_target(struct cpufreq_policy *policy,
 
 	freqs.new = omap_getspeed(policy->cpu);
 
+#ifdef CONFIG_SMP

arch/arm/mach-omap2/omap2plus-cpufreq.c static int omap_target(struct cpufreq_policy *policy, · line 123

Quoted 10 lines

arch/arm/mach-omap2/omap2plus-cpufreq.c +10 -0

+	/* 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.

arch/arm/mach-omap2/omap2plus-cpufreq.c static int omap_target(struct cpufreq_policy *policy, · line 128

Quoted 11 lines

arch/arm/mach-omap2/omap2plus-cpufreq.c +2 -6

 	/* 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.

Premi, Sanjeev

author

2011-06-24 14:09 UTC #3 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 7 lines

-----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]

Quoted 9 lines

arch/arm/mach-omap2/omap2plus-cpufreq.c static int omap_target(struct cpufreq_policy *policy,
Quoted 5 lines
arch/arm/mach-omap2/omap2plus-cpufreq.c +4 -0
+#ifdef CONFIG_SMP
+	/* Adjust jiffies before transition */
+	for_each_cpu(i, policy->cpus) {
+		unsigned long lpj = per_cpu(cpu_data, 
i).loops_per_jiffy;
Review
Quoted 2 lines
(quoted diff) +2 -0
+
+		per_cpu(cpu_data, i).loops_per_jiffy = 
cpufreq_scale(lpj,

Quoted 10 lines

arch/arm/mach-omap2/omap2plus-cpufreq.c static int omap_target(struct cpufreq_policy *policy, · line 123
Quoted 5 lines
arch/arm/mach-omap2/omap2plus-cpufreq.c +5 -0
+							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]

Russell King - ARM Linux

2011-06-24 14:14 UTC #4 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 32 lines · Sanjeev

Quoted 7 lines
-----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]
Quoted 9 lines
arch/arm/mach-omap2/omap2plus-cpufreq.c static int omap_target(struct cpufreq_policy *policy,
Quoted 5 lines
arch/arm/mach-omap2/omap2plus-cpufreq.c +4 -0
+#ifdef CONFIG_SMP
+	/* Adjust jiffies before transition */
+	for_each_cpu(i, policy->cpus) {
+		unsigned long lpj = per_cpu(cpu_data, 
i).loops_per_jiffy;
Review
Quoted 2 lines
(quoted diff) +2 -0
+
+		per_cpu(cpu_data, i).loops_per_jiffy = 
cpufreq_scale(lpj,
Quoted 10 lines
arch/arm/mach-omap2/omap2plus-cpufreq.c static int omap_target(struct cpufreq_policy *policy, · line 123
Quoted 5 lines
arch/arm/mach-omap2/omap2plus-cpufreq.c +5 -0
+							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,

Quoted 7 lines

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.

Russell King - ARM Linux

2011-06-24 15:12 UTC #5 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

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

omap2plus-cpufreq.c renamed from omap2plus-cpufreq.c~ +27 -6

@@ -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.

Premi, Sanjeev

author

2011-06-24 15:34 UTC #6 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 63 lines

-----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 renamed from omap2plus-cpufreq.c~ +27 -6

@@ -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

Quoted 8 lines

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

Premi, Sanjeev

author

2011-06-24 17:50 UTC #7 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

In reply to Russell King - ARM Linux

Quoted 63 lines

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

omap2plus-cpufreq.c renamed from omap2plus-cpufreq.c~ +27 -6

@@ -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

Quoted 9 lines

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

Russell King - ARM Linux

2011-06-24 18:51 UTC #8 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 6 lines · Sanjeev

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"

Quoted 6 lines

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.

Quoted 2 lines

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

Ok an undefined instruction. So...

Quoted 4 lines

[   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

Quoted 8 lines

[   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:

Quoted 1 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...

Kevin Hilman

2011-06-24 20:14 UTC #9 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 49 lines · Russell King - ARM Linux

Quoted 6 lines · Sanjeev
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"
Quoted 6 lines
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.
Quoted 2 lines
[root at OMAP3EVM cpufreq]# cat /proc/cpuinfo
[   73.832366] Internal error: Oops - undefined instruction: 0 [#1] SMP
Ok an undefined instruction. So...
Quoted 4 lines
[   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
Quoted 8 lines
[   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:
Quoted 1 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

Quoted 6 lines

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

Premi, Sanjeev

author

2011-06-25 16:20 UTC #10 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 70 lines

-----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
Quoted 7 lines · Russell King - ARM Linux
Quoted 6 lines · Sanjeev
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"
Quoted 8 lines
Quoted 6 lines
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
Quoted 6 lines
up with your hardware.
Let's take the first.
Quoted 2 lines
[root at OMAP3EVM cpufreq]# cat /proc/cpuinfo
[   73.832366] Internal error: Oops - undefined 
instruction: 0 [#1] SMP
Quoted 5 lines
Ok an undefined instruction. So...
Quoted 2 lines
[   73.839019] Modules linked in:
[   73.842193] CPU: 0    Not tainted  
(3.0.0-rc3-14002-g40b6752-dirty #21)
Quoted 3 lines
Quoted 3 lines
[   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
Quoted 3 lines
Quoted 3 lines
[   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
Quoted 1 line
Quoted 1 line
[   73.882141] r3 : 87fb0a3e  r2 : 00000800  r1 : 87fb01cf 
r0 : c5d518c0
Quoted 1 line
Quoted 1 line
[   73.888977] Flags: nzcv  IRQs on  FIQs on  Mode SVC_32  
ISA ARM Segment user
Quoted 18 lines
Quoted 2 lines
[   73.896423] Control: 10c5387d  Table: 8795c019  DAC: 00000015
[   73.902435] Process cat (pid: 449, stack limit = 0xc79062f8)
... lets look at the code line:
Quoted 1 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.

Quoted 6 lines

Quoted 3 lines
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

Quoted 5 lines

Kevin

Santosh Shilimkar

2011-06-24 18:48 UTC #11 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

In reply to Russell King - ARM Linux

Russell,

Quoted 2 lines · Russell King - ARM Linux

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

[.....]

Quoted 6 lines

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 <[email protected]>
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 <[email protected]>

[santosh.shilimkar at ti.com: rebased against omap cpufreq upstream branch]

Signed-off-by Russell King <[email protected]>

Cc Kevin Hilman <[email protected]>

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

(quoted diff) +0 -0

b/arch/arm/mach-omap2/omap2plus-cpufreq.c
index 1f3b2e1..434698e 100644

arch/arm/mach-omap2/omap2plus-cpufreq.c +27 ~~-21~~

80 lines not on this page · raw message

Signature · 1 line

1.7.4.1

Premi, Sanjeev

author

2011-06-25 18:53 UTC #12 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 58 lines

-----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,
Quoted 2 lines · Russell King - ARM Linux
Right, thanks for the file. Here's the patch.
[.....]
Quoted 4 lines
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
Quoted 2 lines
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 <[email protected]>
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.

Quoted 5 lines

Signed-off-by Santosh Shilimkar <[email protected]>
[santosh.shilimkar at ti.com: rebased against omap cpufreq upstream branch]
Signed-off-by Russell King <[email protected]>
Cc Kevin Hilman <[email protected]>

[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]

Russell King - ARM Linux

2011-06-25 19:09 UTC #13 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 3 lines · Sanjeev

[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?

Premi, Sanjeev

author

2011-06-27 04:54 UTC #14 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 10 lines

-----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:

Quoted 8 lines

Quoted 3 lines
[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.

Quoted 4 lines

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

Russell King - ARM Linux

2011-06-27 07:40 UTC #15 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 19 lines · Sanjeev

Quoted 10 lines
-----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:
Quoted 8 lines
Quoted 3 lines
[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.

Quoted 7 lines

[sp] Agree.
Quoted 4 lines
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.

Santosh Shilimkar

2011-06-24 14:35 UTC #16 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

In reply to Sanjeev Premi

Quoted 13 lines · 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.

Quoted 5 lines

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<[email protected]>

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

Premi, Sanjeev

author

2011-06-24 14:40 UTC #17 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 33 lines

-----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
Quoted 13 lines · 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.
Quoted 5 lines
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<[email protected]>
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.

Quoted 4 lines

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

Quoted 4 lines

Regards
Santosh

Santosh Shilimkar

2011-06-24 14:47 UTC #18 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 38 lines · Sanjeev

Quoted 33 lines
-----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
Quoted 13 lines · 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.
Quoted 5 lines
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<[email protected]>
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.
Quoted 4 lines
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.

Quoted 5 lines

[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

Colin Cross

2011-06-28 22:29 UTC #19 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

In reply to Sanjeev Premi

Quoted 52 lines · 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 <[email protected]>
---
?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(-)
arch/arm/mach-omap2/omap2plus-cpufreq.c +0 -0
index 346519e..0263cae 100644
@@ -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); ? ? ? ?}
arch/arm/mach-omap2/omap2plus-cpufreq.c +0 -0
@@ -114,13 +110,20 @@ static int omap_target(struct cpufreq_policy *policy,
 

arch/arm/mach-omap2/omap2plus-cpufreq.c static int omap_target(struct cpufreq_policy *policy, · line 120

Quoted 10 lines

arch/arm/mach-omap2/omap2plus-cpufreq.c +8 -0

 ? ? ? ?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.

Quoted 22 lines

arch/arm/mach-omap2/omap2plus-cpufreq.c +3 -0
+ ? ? ? }
+#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

Santosh Shilimkar

2011-06-28 22:45 UTC #20 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 5 lines · Colin Cross

<resending as plain text>
Quoted 1 line · Sanjeev Premi

Quoted 8 lines

arch/arm/mach-omap2/omap2plus-cpufreq.c static int omap_target(struct cpufreq_policy *policy, · line 120

Quoted 8 lines

arch/arm/mach-omap2/omap2plus-cpufreq.c +8 -0

+#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.

Quoted 8 lines

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

Colin Cross

2011-06-28 22:56 UTC #21 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 8 lines · Santosh Shilimkar

Quoted 5 lines · Colin Cross
<resending as plain text>
Quoted 1 line · On Fri, Jun 24, 2011 at 6:53 AM, Sanjeev Premi<[email protected]> ?wrote:

Quoted 12 lines

Quoted 8 lines
Review
Quoted 8 lines
(quoted diff) +8 -0
+#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.

Quoted 9 lines

Quoted 9 lines
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.

Quoted 6 lines

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.

Quoted 11 lines

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

A message that is not in the archive (sent off-list or not yet received): CAMbhsRRctHC2wSi7cWjO2Fn_rM7=dMtTrt6PbsVehrgx9SKwzw@mail.gmail.com

Santosh Shilimkar

2011-06-28 23:00 UTC #23 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 2 lines · Colin Cross

On Tue, Jun 28, 2011 at 3:45 PM, Santosh Shilimkar <santosh.shilimkar at ti.com <mailto:[email protected]>> wrote:

[....]

Quoted 19 lines

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

Quoted 5 lines

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

Santosh Shilimkar

2011-06-28 23:04 UTC #24 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 30 lines · Santosh Shilimkar

Quoted 2 lines · Colin Cross
On Tue, Jun 28, 2011 at 3:45 PM, Santosh Shilimkar <santosh.shilimkar at ti.com <mailto:[email protected]>> wrote:
[....]
Quoted 19 lines
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..
Quoted 5 lines
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.

Quoted 5 lines

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

Russell King - ARM Linux

2011-06-28 23:03 UTC #25 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

In reply to Santosh Shilimkar

Quoted 7 lines · Santosh Shilimkar

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.

Santosh Shilimkar

2011-06-28 23:07 UTC #26 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 5 lines · Russell King - ARM Linux

Quoted 4 lines · Santosh Shilimkar
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.

Quoted 8 lines

Quoted 3 lines
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.

Quoted 6 lines

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

Russell King - ARM Linux

2011-06-28 22:55 UTC #27 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

In reply to Colin Cross

Quoted 4 lines · Colin Cross

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.

Colin Cross

2011-06-28 22:58 UTC #28 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 8 lines · Russell King - ARM Linux

Quoted 4 lines · Colin Cross
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.

Russell King - ARM Linux

2011-06-28 23:17 UTC #29 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 8 lines · Colin Cross

Quoted 6 lines · Russell King - ARM Linux
Quoted 4 lines · Colin Cross
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.

Quoted 6 lines

Quoted 2 lines
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.)

Colin Cross

2011-06-28 23:37 UTC #30 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 25 lines · Russell King - ARM Linux

Quoted 8 lines · Colin Cross
Quoted 6 lines · Russell King - ARM Linux
Quoted 4 lines · Colin Cross
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.
Quoted 6 lines
Quoted 2 lines
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.

Quoted 9 lines

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.

Russell King - ARM Linux

2011-06-28 23:46 UTC #31 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 2 lines · Colin Cross

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.

Colin Cross

2011-06-28 23:59 UTC #32 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 20 lines · Russell King - ARM Linux

Quoted 2 lines · Colin Cross
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);
? ? ? ? ? ? ? ?}

Quoted 4 lines

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.

Russell King - ARM Linux

2011-06-29 14:00 UTC #33 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 27 lines · Colin Cross

Quoted 20 lines · Russell King - ARM Linux
Quoted 2 lines · Colin Cross
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);
? ? ? ? ? ? ? ?}
Quoted 4 lines
which goes wrong for all the same reasons you're pointing out about udelay(). ?So just restricting your argument to udelay() is not realistic.

Quoted 6 lines

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

Colin Cross

2011-06-29 16:57 UTC #34 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 28 lines · Russell King - ARM Linux

Quoted 27 lines · Colin Cross
Quoted 24 lines · Russell King - ARM Linux
Quoted 2 lines · Colin Cross
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.

Quoted 8 lines

Quoted 6 lines
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>

Quoted 5 lines

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.

Stephen Boyd

2011-06-29 18:29 UTC #35 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

In reply to Russell King - ARM Linux

Quoted 9 lines · Russell King - ARM Linux

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?

Signature · 2 lines

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

Russell King - ARM Linux

2011-06-29 18:43 UTC #36 lore raw

PATCH v2 1/1 omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 11 lines · Stephen Boyd

Quoted 9 lines · Russell King - ARM Linux
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.)

Quoted 1 line

Am I people? ;-)

That depends if you're a multiple personality person!

Quoted 17 lines

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.

Quoted 1 line

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