diff mbox

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

Message ID 1308911742-27394-3-git-send-email-premi@ti.com (mailing list archive)
State Changes Requested
Delegated to: Kevin Hilman
Headers show

Commit Message

Sanjeev Premi June 24, 2011, 10:35 a.m. UTC
Currently, loops_per_jiffy is being calculated before calling
cpufreq_notify_transition().

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

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

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

Comments

Russell King - ARM Linux June 24, 2011, 10:43 a.m. UTC | #1
On Fri, Jun 24, 2011 at 04:05:42PM +0530, Sanjeev Premi wrote:
> Currently, loops_per_jiffy is being calculated before calling
> cpufreq_notify_transition().
> 
> However, generic cpufreq driver adjusts the jiffies as well.
> Double adjustment leads to incorrect value being assigned to
> loops_per_jiffy.

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

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

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

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

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

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

So, if we want to do this then we need to store the boot-time lpj and
frequency as the baseline reference, and scale according to that,
much like the core cpufreq code does for loops_per_jiffy for the !SMP
case.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sanjeev Premi June 24, 2011, 10:48 a.m. UTC | #2
> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk] 
> Sent: Friday, June 24, 2011 4:14 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix 
> loops_per_jiffy calculation
> 
> On Fri, Jun 24, 2011 at 04:05:42PM +0530, Sanjeev Premi wrote:
> > Currently, loops_per_jiffy is being calculated before calling
> > cpufreq_notify_transition().
> > 
> > However, generic cpufreq driver adjusts the jiffies as well.
> > Double adjustment leads to incorrect value being assigned to
> > loops_per_jiffy.
> 
> Are you sure the generic cpufreq driver adjusts the per-cpu 
> loops_per_jiffy
> values?  I don't believe it does.

Russell,

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

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

	case CPUFREQ_PRECHANGE:

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

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

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


~sanjeev

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

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

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

Notice how if CONFIG_SMP is set, adjust_jiffies becomes a no-op.  So if
CONFIG_SMP=y, loops_per_jiffy will _not_ be scaled by core code.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sanjeev Premi June 24, 2011, 11:11 a.m. UTC | #4
> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk] 
> Sent: Friday, June 24, 2011 4:22 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix 
> loops_per_jiffy calculation
> 
> On Fri, Jun 24, 2011 at 04:18:31PM +0530, Premi, Sanjeev wrote:
> > > -----Original Message-----
> > > From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk] 
> > > Sent: Friday, June 24, 2011 4:14 PM
> > > To: Premi, Sanjeev
> > > Cc: linux-omap@vger.kernel.org; 
> linux-arm-kernel@lists.infradead.org
> > > Subject: Re: [PATCH 2/2] omap2+: pm: cpufreq: Fix 
> > > loops_per_jiffy calculation
> > > 
> > > On Fri, Jun 24, 2011 at 04:05:42PM +0530, Sanjeev Premi wrote:
> > > > Currently, loops_per_jiffy is being calculated before calling
> > > > cpufreq_notify_transition().
> > > > 
> > > > However, generic cpufreq driver adjusts the jiffies as well.
> > > > Double adjustment leads to incorrect value being assigned to
> > > > loops_per_jiffy.
> > > 
> > > Are you sure the generic cpufreq driver adjusts the per-cpu 
> > > loops_per_jiffy
> > > values?  I don't believe it does.
> > 
> > Russell,
> > 
> > I am quoting the function from drivers/cpufreq/cpufreq.h
> > Follow the arrows:
> 
> Let's go to war with quotes then, because clearly you haven't 
> read the code
> properly:

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

~sanjeev

> 
> | #ifndef CONFIG_SMP
> ^^^^^^^^^^^^^^^^^^^^^
> | static unsigned long l_p_j_ref;
> | static unsigned int  l_p_j_ref_freq;
> | 
> | static void adjust_jiffies(unsigned long val, struct 
> cpufreq_freqs *ci)
> | {
> | 	... code to adjust jiffies ...
> | }
> | #else
> | static inline void adjust_jiffies(unsigned long val, struct 
> cpufreq_freqs *ci)
> | {
> |         return;
> | }
> | #endif
> 
> Notice how if CONFIG_SMP is set, adjust_jiffies becomes a 
> no-op.  So if
> CONFIG_SMP=y, loops_per_jiffy will _not_ be scaled by core code.
> --
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Santosh Shilimkar June 24, 2011, 12:45 p.m. UTC | #5
Sanjeev,

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

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

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

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

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

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

~sanjeev

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

Regards
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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