Skip to contentSkip to conversation

omap2+: pm: Fix code syntax and BogoMIPS

PATCH New Sanjeev Premi 3 participants 14 messages Jun 24, 2011

Subsystems (3)
Arm (32-bit) Arm sub-architectures OMAP2+ support
lore
Share & tools
lore

Sanjeev Premi

author
#0 lore raw

cover omap2+: pm: Fix code syntax and BogoMIPS

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

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

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

The BogoMIPS calculation has been tested on OMAP3EVM.

Before
======

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

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

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

After
=====

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

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

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

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

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

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

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

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

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

 arch/arm/mach-omap2/omap2plus-cpufreq.c |   51 ++++++++----------------------
 1 files changed, 14 insertions(+), 37 deletions(-)
Signature · 1 line
1.7.2.2
author
#1 lore raw

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

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

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

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

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

Quoted 9 lines · Sanjeev Premi

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

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

Signed-off-by Sanjeev Premi<[email protected]>

---

Don't see need of this patch as well considering your second patch. So NAK.

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

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

Quoted 11 lines

-----Original Message-----
From: Shilimkar, Santosh
Sent: Friday, June 24, 2011 6:18 PM
To: Premi, Sanjeev
Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH 1/2] omap2+: pm: Use if...else instead of goto

Quoted 3 lines · Sanjeev Premi

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

Quoted 9 lines
Quoted 6 lines

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

Signed-off-by Sanjeev Premi<[email protected]>

---

Don't see need of this patch as well considering your second patch. So NAK.

[sp] May be it isn't needed - but it surely highlights some
bad code that does find way upstream.

~sanjeev

#4 lore raw

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

Quoted 15 lines · Sanjeev
Quoted 15 lines

-----Original Message-----
From: Shilimkar, Santosh
Sent: Friday, June 24, 2011 6:18 PM
To: Premi, Sanjeev
Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH 1/2] omap2+: pm: Use if...else instead of goto

Quoted 7 lines · Sanjeev Premi

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

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

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Signed-off-by Sanjeev Premi<[email protected]>

---

Don't see need of this patch as well considering your second patch. So NAK.

[sp] May be it isn't needed - but it surely highlights some
bad code that does find way upstream.

If you think of using if else makes code better than use of goto, then I just ignore your comment. Because that means core scheduler code is also bad as per your remark which makes use of goto.

Regards
Santosh

author
#5 lore raw

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

Quoted 29 lines

-----Original Message-----
From: Shilimkar, Santosh
Sent: Friday, June 24, 2011 6:29 PM
To: Premi, Sanjeev
Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH 1/2] omap2+: pm: Use if...else instead of goto

Quoted 5 lines · Sanjeev
Quoted 5 lines

-----Original Message-----
From: Shilimkar, Santosh
Sent: Friday, June 24, 2011 6:18 PM
To: Premi, Sanjeev
Cc: linux-omap at vger.kernel.org;

linux-arm-kernel at lists.infradead.org

Quoted 15 lines
Quoted 15 lines

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

Quoted 9 lines · Sanjeev Premi

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

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

Signed-off-by Sanjeev Premi<[email protected]>

---

Don't see need of this patch as well considering your second patch. So NAK.

Quoted 7 lines
Quoted 3 lines

[sp] May be it isn't needed - but it surely highlights some
bad code that does find way upstream.

If you think of using if else makes code better than use of goto, then I just ignore your comment. Because that means core scheduler code is also bad as per your remark which makes use of goto.

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

Quoted 4 lines

Regards
Santosh

author
#6 lore raw

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

In reply to Sanjeev Premi

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

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

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

Signed-off-by Sanjeev Premi <[email protected]>
---
 arch/arm/mach-omap2/omap2plus-cpufreq.c |   18 +-----------------
 1 files changed, 1 insertions(+), 17 deletions(-)
arch/arm/mach-omap2/omap2plus-cpufreq.c +1 -17
index ce9d534..346519e 100644
@@ -114,29 +114,13 @@ static int omap_target(struct cpufreq_policy *policy,
 
 	freqs.new = omap_getspeed(policy->cpu);
 
-	/*
-	 * In the generic cpufreq driver jiffies are updated only for
-	 * non-SMP cases. Ensure that jiffies are bing updated for both
-	 * SMP systems and UP systems built with CONFIG_SMP enabled.
-	 */
+	/* Notify transitions */
 	if (is_smp()) {
-#ifdef CONFIG_SMP
-		for_each_cpu(i, policy->cpus)
-			per_cpu(cpu_data, i).loops_per_jiffy =
-				cpufreq_scale(per_cpu(cpu_data, i).loops_per_jiffy,
-						freqs.old,
-						freqs.new);
-#endif
-		/* Notify transitions */
 		for_each_cpu(i, policy->cpus) {
 			freqs.cpu = i;
 			cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
 		}
 	} else {
-		loops_per_jiffy = cpufreq_scale(loops_per_jiffy,
-					freqs.old, freqs.new);
-
-		/* Notify transitions */
 		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
 	}
Signature · 1 line
1.7.2.2
#7 lore raw

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

Quoted 6 lines · Sanjeev Premi

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

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

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

Quoted 13 lines
arch/arm/mach-omap2/omap2plus-cpufreq.c +0 -5
index ce9d534..346519e 100644
@@ -114,29 +114,13 @@ static int omap_target(struct cpufreq_policy *policy,
 
 	freqs.new = omap_getspeed(policy->cpu);
 
-	/*
-	 * In the generic cpufreq driver jiffies are updated only for
-	 * non-SMP cases. Ensure that jiffies are bing updated for both
-	 * SMP systems and UP systems built with CONFIG_SMP enabled.
-	 */

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

Quoted 10 lines
arch/arm/mach-omap2/omap2plus-cpufreq.c +1 -8
+	/* Notify transitions */
 	if (is_smp()) {
-#ifdef CONFIG_SMP
-		for_each_cpu(i, policy->cpus)
-			per_cpu(cpu_data, i).loops_per_jiffy =
-				cpufreq_scale(per_cpu(cpu_data, i).loops_per_jiffy,
-						freqs.old,
-						freqs.new);
-#endif
-		/* Notify transitions */

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

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

Quoted 9 lines
arch/arm/mach-omap2/omap2plus-cpufreq.c +0 -4
 		for_each_cpu(i, policy->cpus) {
 			freqs.cpu = i;
 			cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
 		}
 	} else {
-		loops_per_jiffy = cpufreq_scale(loops_per_jiffy,
-					freqs.old, freqs.new);
-
-		/* Notify transitions */

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

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

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

author
#8 lore raw

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

Quoted 12 lines

-----Original Message-----
From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
Sent: Friday, June 24, 2011 4:14 PM
To: Premi, Sanjeev
Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 3 lines · Sanjeev Premi

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

Quoted 7 lines
Quoted 3 lines

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

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

Russell,

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

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

{
      [snip]...[snip]

	case CPUFREQ_PRECHANGE:

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

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

[/quote]

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

~sanjeev

[snip]...[snip]

#9 lore raw

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

Quoted 16 lines · Sanjeev
Quoted 16 lines

-----Original Message-----
From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
Sent: Friday, June 24, 2011 4:14 PM
To: Premi, Sanjeev
Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 6 lines · Sanjeev Premi

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

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

Quoted 8 lines
Quoted 3 lines

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

Russell,

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

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

| #ifndef CONFIG_SMP
^^^^^^^^^^^^^^^^^^^^^

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

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

author
#10 lore raw

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

Quoted 30 lines

-----Original Message-----
From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
Sent: Friday, June 24, 2011 4:22 PM
To: Premi, Sanjeev
Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 5 lines · Sanjeev
Quoted 5 lines

-----Original Message-----
From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
Sent: Friday, June 24, 2011 4:14 PM
To: Premi, Sanjeev
Cc: linux-omap at vger.kernel.org;

linux-arm-kernel at lists.infradead.org

Quoted 15 lines
Quoted 14 lines

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

Quoted 6 lines · Sanjeev Premi

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

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

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

Quoted 8 lines
Quoted 4 lines

Russell,

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

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

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

~sanjeev

Quoted 23 lines

| #ifndef CONFIG_SMP
^^^^^^^^^^^^^^^^^^^^^

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

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

#11 lore raw

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

In reply to Russell King - ARM Linux

Sanjeev,

Quoted 45 lines · Russell King - ARM Linux
Quoted 16 lines · Sanjeev
Quoted 16 lines

-----Original Message-----
From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
Sent: Friday, June 24, 2011 4:14 PM
To: Premi, Sanjeev
Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Quoted 6 lines · Sanjeev Premi

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

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

Quoted 8 lines
Quoted 3 lines

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

Russell,

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

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

| #ifndef CONFIG_SMP
^^^^^^^^^^^^^^^^^^^^^

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

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

As Russell rightly pointed out, you need to take care of UP/SMP and UP OVER SMP.

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

Regards
Santosh

author
#12 lore raw

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

Quoted 63 lines

-----Original Message-----
From: Shilimkar, Santosh
Sent: Friday, June 24, 2011 6:16 PM
To: Russell King - ARM Linux
Cc: Premi, Sanjeev; linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Sanjeev,

Quoted 6 lines · Russell King - ARM Linux
Quoted 5 lines · Sanjeev
Quoted 5 lines

-----Original Message-----
From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
Sent: Friday, June 24, 2011 4:14 PM
To: Premi, Sanjeev
Cc: linux-omap at vger.kernel.org;

linux-arm-kernel at lists.infradead.org

Quoted 15 lines
Quoted 15 lines
Quoted 14 lines

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

Quoted 6 lines · Sanjeev Premi

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

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

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

Quoted 6 lines
Quoted 4 lines

Russell,

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

Let's go to war with quotes then, because clearly you

haven't read the code

Quoted 8 lines

properly:

| #ifndef CONFIG_SMP
^^^^^^^^^^^^^^^^^^^^^

| static unsigned long l_p_j_ref;
| static unsigned int  l_p_j_ref_freq;
|
| static void adjust_jiffies(unsigned long val, struct

cpufreq_freqs *ci)

Quoted 5 lines
| {
| 	... code to adjust jiffies ...
| }
| #else
| static inline void adjust_jiffies(unsigned long val,

struct cpufreq_freqs *ci)

Quoted 6 lines
| {
|         return;
| }
| #endif

Notice how if CONFIG_SMP is set, adjust_jiffies becomes a

no-op. So if

Quoted 8 lines
Quoted 2 lines

CONFIG_SMP=y, loops_per_jiffy will _not_ be scaled by core code.

As Russell rightly pointed out, you need to take care of UP/SMP and UP
OVER SMP.

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

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

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

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

~sanjeev

Quoted 4 lines

Regards
Santosh

#13 lore raw

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

Quoted 75 lines · Sanjeev
Quoted 63 lines

-----Original Message-----
From: Shilimkar, Santosh
Sent: Friday, June 24, 2011 6:16 PM
To: Russell King - ARM Linux
Cc: Premi, Sanjeev; linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation

Sanjeev,

Quoted 6 lines · Russell King - ARM Linux
Quoted 5 lines · Sanjeev
Quoted 5 lines

-----Original Message-----
From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
Sent: Friday, June 24, 2011 4:14 PM
To: Premi, Sanjeev
Cc: linux-omap at vger.kernel.org;

linux-arm-kernel at lists.infradead.org

Quoted 15 lines
Quoted 15 lines
Quoted 14 lines

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

Quoted 6 lines · Sanjeev Premi

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

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

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

Quoted 6 lines
Quoted 4 lines

Russell,

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

Let's go to war with quotes then, because clearly you

haven't read the code

Quoted 8 lines

properly:

| #ifndef CONFIG_SMP
^^^^^^^^^^^^^^^^^^^^^

| static unsigned long l_p_j_ref;
| static unsigned int  l_p_j_ref_freq;
|
| static void adjust_jiffies(unsigned long val, struct

cpufreq_freqs *ci)

Quoted 5 lines
| {
| 	... code to adjust jiffies ...
| }
| #else
| static inline void adjust_jiffies(unsigned long val,

struct cpufreq_freqs *ci)

Quoted 6 lines
| {
|         return;
| }
| #endif

Notice how if CONFIG_SMP is set, adjust_jiffies becomes a

no-op. So if

Quoted 8 lines
Quoted 2 lines

CONFIG_SMP=y, loops_per_jiffy will _not_ be scaled by core code.

As Russell rightly pointed out, you need to take care of UP/SMP and UP
OVER SMP.

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

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

Quoted 7 lines

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

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

I can imagine what you are gonna post based on Russell comment, but will wait for your patch before commenting.

Regards
Santosh