ARM: 7653/2: do not scale loops_per_jiffy when using a constant delay clock

ARM: 7653/2: do not scale loops_per_jiffy when using a constant delay clock

[Will Deacon]

Hi Nico,

I notice that commit 70264367a243 ("ARM: 7653/2: do not scale loops_per_jiffy
when using a constant delay clock") is in mainline, but I'm not sure whether
it's the right fix. Unfortunately, I failed to find it in the list archives,
so I couldn't repy to the original patch.

It seems like a better fix would be to pass the CPUFREQ_CONST_LOOPS flag from
the cpufreq driver doing the frequency scaling, which would prevent
loops_per_jiffy from being scaled at all. It also means that UP platforms
would work correctly, while I think they're still broken with your patch.

What do you reckon? I'm trying to do some cleanup in this area with the aim
of making the core cpufreq driver SMP-aware.

Cheers,

Will

ARM: 7653/2: do not scale loops_per_jiffy when using a constant delay clock

[Russell King - ARM Linux]

On Wed, Mar 06, 2013 at 02:23:08AM +0000, Will Deacon wrote:
> I notice that commit 70264367a243 ("ARM: 7653/2: do not scale loops_per_jiffy
> when using a constant delay clock") is in mainline, but I'm not sure whether
> it's the right fix. Unfortunately, I failed to find it in the list archives,
> so I couldn't repy to the original patch.

Sigh.  Here we go again.  I've said this many times.  Patches need to be
sent to the mailing list *before* they're sent to the patch system.  Not
just the complex ones.  ALL PATCHES including those which look like simple
fixes.

The reason being is that if someone wants to comment on the patch, they
can.  For exactly the kind of reason that Will brings up above.  You may
think your fix is obvious and the right solution, but someone else in this
complex ecosystem may have a case where your otherwise perfect solution
doesn't work.

ARM: 7653/2: do not scale loops_per_jiffy when using a constant delay clock

[Will Deacon]

Hi guys,

On Wed, Mar 06, 2013 at 06:37:51PM +0000, Russell King - ARM Linux wrote:
> On Wed, Mar 06, 2013 at 02:23:08AM +0000, Will Deacon wrote:
> > I notice that commit 70264367a243 ("ARM: 7653/2: do not scale loops_per_jiffy
> > when using a constant delay clock") is in mainline, but I'm not sure whether
> > it's the right fix. Unfortunately, I failed to find it in the list archives,
> > so I couldn't repy to the original patch.
> 
> Sigh.  Here we go again.  I've said this many times.  Patches need to be
> sent to the mailing list *before* they're sent to the patch system.  Not
> just the complex ones.  ALL PATCHES including those which look like simple
> fixes.
> 
> The reason being is that if someone wants to comment on the patch, they
> can.  For exactly the kind of reason that Will brings up above.  You may
> think your fix is obvious and the right solution, but someone else in this
> complex ecosystem may have a case where your otherwise perfect solution
> doesn't work.

I've just spoken to Nico in person about this, so it's probably worthwhile
mentioning something here in an attempt to clear things up.

It turns out that the problem which the patch in question tries to solve was
originally fixed by somebody in ARM and discussed off-list (which explains
the acks on the final patch). However, this got stuck in code review for a
disproportionally large amount of time, until Nico admittedly lost his rag;
writing his own patch and putting it straight into the patch system.

Of course, this still isn't the right way to get patches into mainline and
the points Russell makes above are completely correct. I wonder if we could
extend the patch system to reject patches automatically if they don't appear
in the linux-arm-kernel archives?

On the topic of this patch: I still think that we should revert it and
require cpufreq drivers to pass CPUFREQ_CONST_LOOPS in their flags (I guess
the cpu0 platform data may need extending to take some flags). Longer
term, we might want to assess the binding between timer-based delays and
loops_per_jiffy, but that's an entirely new problem.

Will

ARM: 7653/2: do not scale loops_per_jiffy when using a constant delay clock

[Nicolas Pitre]

On Thu, 7 Mar 2013, Will Deacon wrote:

> On the topic of this patch: I still think that we should revert it and
> require cpufreq drivers to pass CPUFREQ_CONST_LOOPS in their flags (I guess
> the cpu0 platform data may need extending to take some flags).

I must disagree.  The constness here is a property of the timer source 
used to implement one of the udelay() providers.  Constness has no 
relation with cpufreq which may or may not impact a given udelay 
implementation.

> Longer term, we might want to assess the binding between timer-based 
> delays and loops_per_jiffy, but that's an entirely new problem.

Actually I do think that the lpj should always be scaled regardless, as 
this has meaning only for a CPU loop.  This is even more important if 
there is the possibility for switching between different 
implementations. The local timer based udelay implementation should 
simply never factor in lpj into its delay evaluation.

So the current patch is a stop gap to fix the problem today.  When 
something better is proposed we can remove this fix.


Nicolas

ARM: 7653/2: do not scale loops_per_jiffy when using a constant delay clock

[Russell King - ARM Linux]

On Thu, Mar 07, 2013 at 03:32:21AM +0000, Will Deacon wrote:
> Of course, this still isn't the right way to get patches into mainline and
> the points Russell makes above are completely correct. I wonder if we could
> extend the patch system to reject patches automatically if they don't appear
> in the linux-arm-kernel archives?

Ah, so what you're saying is that people can't be trusted to behave in a
responsible and professional manner. :)

How can that be done?  Parse the patch escaping all the special characters
as appropriate, passing it across to the ARM920T based list server, and
having that take a while to grep 600MB of linux-arm-kernel archive -
and have it fail because it got turned into quoted-printable or base64
encoded because of the UTF-8 used in the email message?

What you suggest sounds simple, but in practise it is a very hairy problem.

ARM: 7653/2: do not scale loops_per_jiffy when using a constant delay clock

[Will Deacon]

On Thu, Mar 07, 2013 at 06:37:30AM +0000, Nicolas Pitre wrote:
> On Thu, 7 Mar 2013, Will Deacon wrote:
> 
> > On the topic of this patch: I still think that we should revert it and
> > require cpufreq drivers to pass CPUFREQ_CONST_LOOPS in their flags (I guess
> > the cpu0 platform data may need extending to take some flags).
> 
> I must disagree.  The constness here is a property of the timer source 
> used to implement one of the udelay() providers.  Constness has no 
> relation with cpufreq which may or may not impact a given udelay 
> implementation.
> 
> > Longer term, we might want to assess the binding between timer-based 
> > delays and loops_per_jiffy, but that's an entirely new problem.
> 
> Actually I do think that the lpj should always be scaled regardless, as 
> this has meaning only for a CPU loop.  This is even more important if 
> there is the possibility for switching between different 
> implementations. The local timer based udelay implementation should 
> simply never factor in lpj into its delay evaluation.
> 
> So the current patch is a stop gap to fix the problem today.  When 
> something better is proposed we can remove this fix.

Ok, how about a simple change like the one below?

Will

--->8

diff --git a/arch/arm/include/asm/delay.h b/arch/arm/include/asm/delay.h
index 720799f..06777b9 100644
--- a/arch/arm/include/asm/delay.h
+++ b/arch/arm/include/asm/delay.h
@@ -24,7 +24,7 @@ extern struct arm_delay_ops {
        void (*delay)(unsigned long);
        void (*const_udelay)(unsigned long);
        void (*udelay)(unsigned long);
-       bool const_clock;
+       unsigned long lpj;
 } arm_delay_ops;
 
 #define __delay(n)             arm_delay_ops.delay(n)
diff --git a/arch/arm/lib/delay.c b/arch/arm/lib/delay.c
index 6b93f6a..0d90ed8 100644
--- a/arch/arm/lib/delay.c
+++ b/arch/arm/lib/delay.c
@@ -58,7 +58,7 @@ static void __timer_delay(unsigned long cycles)
 static void __timer_const_udelay(unsigned long xloops)
 {
        unsigned long long loops = xloops;
-       loops *= loops_per_jiffy;
+       loops *= arm_delay_ops.lpj;
        __timer_delay(loops >> UDELAY_SHIFT);
 }
 
@@ -73,11 +73,13 @@ void __init register_current_timer_delay(const struct delay_timer *timer)
                pr_info("Switching to timer-based delay loop\n");
                delay_timer                     = timer;
                lpj_fine                        = timer->freq / HZ;
-               loops_per_jiffy                 = lpj_fine;
+
+               /* cpufreq may scale loops_per_jiffy, so keep a private copy */
+               arm_delay_ops.lpj               = lpj_fine;
                arm_delay_ops.delay             = __timer_delay;
                arm_delay_ops.const_udelay      = __timer_const_udelay;
                arm_delay_ops.udelay            = __timer_udelay;
-               arm_delay_ops.const_clock       = true;
+
                delay_calibrated                = true;
        } else {
                pr_info("Ignoring duplicate/late registration of read_current_timer delay\n");

ARM: 7653/2: do not scale loops_per_jiffy when using a constant delay clock

[Nicolas Pitre]

On Tue, 19 Mar 2013, Will Deacon wrote:

> On Thu, Mar 07, 2013 at 06:37:30AM +0000, Nicolas Pitre wrote:
> > On Thu, 7 Mar 2013, Will Deacon wrote:
> > 
> > > On the topic of this patch: I still think that we should revert it and
> > > require cpufreq drivers to pass CPUFREQ_CONST_LOOPS in their flags (I guess
> > > the cpu0 platform data may need extending to take some flags).
> > 
> > I must disagree.  The constness here is a property of the timer source 
> > used to implement one of the udelay() providers.  Constness has no 
> > relation with cpufreq which may or may not impact a given udelay 
> > implementation.
> > 
> > > Longer term, we might want to assess the binding between timer-based 
> > > delays and loops_per_jiffy, but that's an entirely new problem.
> > 
> > Actually I do think that the lpj should always be scaled regardless, as 
> > this has meaning only for a CPU loop.  This is even more important if 
> > there is the possibility for switching between different 
> > implementations. The local timer based udelay implementation should 
> > simply never factor in lpj into its delay evaluation.
> > 
> > So the current patch is a stop gap to fix the problem today.  When 
> > something better is proposed we can remove this fix.
> 
> Ok, how about a simple change like the one below?
> 
> Will
> 
> --->8
> 
> diff --git a/arch/arm/include/asm/delay.h b/arch/arm/include/asm/delay.h
> index 720799f..06777b9 100644
> --- a/arch/arm/include/asm/delay.h
> +++ b/arch/arm/include/asm/delay.h
> @@ -24,7 +24,7 @@ extern struct arm_delay_ops {
>         void (*delay)(unsigned long);
>         void (*const_udelay)(unsigned long);
>         void (*udelay)(unsigned long);
> -       bool const_clock;
> +       unsigned long lpj;

What about calling this ticks_per_jiffy so to make sure this is not 
confused with the other lpj?



>  } arm_delay_ops;
>  
>  #define __delay(n)             arm_delay_ops.delay(n)
> diff --git a/arch/arm/lib/delay.c b/arch/arm/lib/delay.c
> index 6b93f6a..0d90ed8 100644
> --- a/arch/arm/lib/delay.c
> +++ b/arch/arm/lib/delay.c
> @@ -58,7 +58,7 @@ static void __timer_delay(unsigned long cycles)
>  static void __timer_const_udelay(unsigned long xloops)
>  {
>         unsigned long long loops = xloops;
> -       loops *= loops_per_jiffy;
> +       loops *= arm_delay_ops.lpj;

... and here that could be ticks *= arm_delay_ops.ticks_per_jiffy to 
make it clearer as well.

Otherwise this is fine with me.


Nicolas