[v2,2/2] ARM: delay: allow timer-based delay implementation to be selected
diff mbox

Message ID 4FFE7DB2.4040702@renesas.com
State New, archived
Headers show

Commit Message

Shinya Kuribayashi July 12, 2012, 7:33 a.m. UTC
Hi Will, Stephen,

On 6/30/2012 2:33 AM, Will Deacon wrote:
> +void __init init_current_timer_delay(unsigned long freq)
> +{
> +	pr_info("Switching to timer-based delay loop\n");
> +	lpj_fine			= freq / HZ;
> +	arm_delay_ops.delay		= __timer_delay;
> +	arm_delay_ops.const_udelay	= __timer_const_udelay;
> +	arm_delay_ops.udelay		= __timer_udelay;
> +}

Once this function is processed, the udelay() behavior changes
_immediately_ from loop-based delay to timer-based one, without waiting
for 'loops_per_jiffy' itself being corrected in calibrate_delay().

As a result, actual udelay()s may be toooo long than expected, in
particular udelay()s used between init_current_timer_delay() and
calibrate_delay().  It's unlikely be short, as the frequency of a
counter for read_current_timer is typically slower than CPU frequency.

I'm not sure udelay()s used in this period are legitimate, but if it's
there we'll in trouble somehow.  This is not so great.

If this assumption is correct, it might be better to adjust / correct
loops_per_jiffy itself along with lpj_fine, prior to calibrate_delay().



What do you think?  Am I missing something?

  Shinya

Comments

Will Deacon July 12, 2012, 8:44 a.m. UTC | #1
On Thu, Jul 12, 2012 at 08:33:06AM +0100, Shinya Kuribayashi wrote:
> Hi Will, Stephen,

Hi Shinya,

> On 6/30/2012 2:33 AM, Will Deacon wrote:
> > +void __init init_current_timer_delay(unsigned long freq)
> > +{
> > +	pr_info("Switching to timer-based delay loop\n");
> > +	lpj_fine			= freq / HZ;
> > +	arm_delay_ops.delay		= __timer_delay;
> > +	arm_delay_ops.const_udelay	= __timer_const_udelay;
> > +	arm_delay_ops.udelay		= __timer_udelay;
> > +}
> 
> Once this function is processed, the udelay() behavior changes
> _immediately_ from loop-based delay to timer-based one, without waiting
> for 'loops_per_jiffy' itself being corrected in calibrate_delay().
> 
> As a result, actual udelay()s may be toooo long than expected, in
> particular udelay()s used between init_current_timer_delay() and
> calibrate_delay().  It's unlikely be short, as the frequency of a
> counter for read_current_timer is typically slower than CPU frequency.

Surely using udelay before calibrate_delay_loop has been called is a
fundamental error?

Will
Shinya Kuribayashi July 12, 2012, 9:35 a.m. UTC | #2
On 7/12/2012 5:44 PM, Will Deacon wrote:
>> On 6/30/2012 2:33 AM, Will Deacon wrote:
>>> +void __init init_current_timer_delay(unsigned long freq)
>>> +{
>>> +	pr_info("Switching to timer-based delay loop\n");
>>> +	lpj_fine			= freq / HZ;
>>> +	arm_delay_ops.delay		= __timer_delay;
>>> +	arm_delay_ops.const_udelay	= __timer_const_udelay;
>>> +	arm_delay_ops.udelay		= __timer_udelay;
>>> +}
>>
>> Once this function is processed, the udelay() behavior changes
>> _immediately_ from loop-based delay to timer-based one, without waiting
>> for 'loops_per_jiffy' itself being corrected in calibrate_delay().
>>
>> As a result, actual udelay()s may be toooo long than expected, in
>> particular udelay()s used between init_current_timer_delay() and
>> calibrate_delay().  It's unlikely be short, as the frequency of a
>> counter for read_current_timer is typically slower than CPU frequency.
> 
> Surely using udelay before calibrate_delay_loop has been called is a
> fundamental error?

Got it.  I'm just not confident about disallowing early use of udelay().

  Shinya
Stephen Boyd July 12, 2012, 4:40 p.m. UTC | #3
On 07/12/12 02:35, Shinya Kuribayashi wrote:
> On 7/12/2012 5:44 PM, Will Deacon wrote:
>>> On 6/30/2012 2:33 AM, Will Deacon wrote:
>>>> +void __init init_current_timer_delay(unsigned long freq)
>>>> +{
>>>> +	pr_info("Switching to timer-based delay loop\n");
>>>> +	lpj_fine			= freq / HZ;
>>>> +	arm_delay_ops.delay		= __timer_delay;
>>>> +	arm_delay_ops.const_udelay	= __timer_const_udelay;
>>>> +	arm_delay_ops.udelay		= __timer_udelay;
>>>> +}
>>> Once this function is processed, the udelay() behavior changes
>>> _immediately_ from loop-based delay to timer-based one, without waiting
>>> for 'loops_per_jiffy' itself being corrected in calibrate_delay().
>>>
>>> As a result, actual udelay()s may be toooo long than expected, in
>>> particular udelay()s used between init_current_timer_delay() and
>>> calibrate_delay().  It's unlikely be short, as the frequency of a
>>> counter for read_current_timer is typically slower than CPU frequency.
>> Surely using udelay before calibrate_delay_loop has been called is a
>> fundamental error?
> Got it.  I'm just not confident about disallowing early use of udelay().
>
>

I don't think it's an error. Instead you get a very large delay, similar
to what would happen if you called udelay() before calibrate_delay()
anyway (see the comment in init/main.c above loops_per_jiffy).
Shinya Kuribayashi July 13, 2012, 2:16 a.m. UTC | #4
On 7/13/2012 1:40 AM, Stephen Boyd wrote:
>>>> As a result, actual udelay()s may be toooo long than expected, in
>>>> particular udelay()s used between init_current_timer_delay() and
>>>> calibrate_delay().  It's unlikely be short, as the frequency of a
>>>> counter for read_current_timer is typically slower than CPU frequency.
>>> Surely using udelay before calibrate_delay_loop has been called is a
>>> fundamental error?
>> Got it.  I'm just not confident about disallowing early use of udelay().
>>
> 
> I don't think it's an error. Instead you get a very large delay, similar
> to what would happen if you called udelay() before calibrate_delay()
> anyway (see the comment in init/main.c above loops_per_jiffy).

Thanks, so I'd set up loops_per_jiffy early, along with lpj_fine in
init_current_timer_delay().

As a matter of fact, I do have early use of udelay()s in my tree :-)
Will give it a try for some time.
Will Deacon July 13, 2012, 8:57 a.m. UTC | #5
On Fri, Jul 13, 2012 at 03:16:41AM +0100, Shinya Kuribayashi wrote:
> On 7/13/2012 1:40 AM, Stephen Boyd wrote:
> >>>> As a result, actual udelay()s may be toooo long than expected, in
> >>>> particular udelay()s used between init_current_timer_delay() and
> >>>> calibrate_delay().  It's unlikely be short, as the frequency of a
> >>>> counter for read_current_timer is typically slower than CPU frequency.
> >>> Surely using udelay before calibrate_delay_loop has been called is a
> >>> fundamental error?
> >> Got it.  I'm just not confident about disallowing early use of udelay().
> >>
> > 
> > I don't think it's an error. Instead you get a very large delay, similar
> > to what would happen if you called udelay() before calibrate_delay()
> > anyway (see the comment in init/main.c above loops_per_jiffy).

Interesting, I didn't notice it was initialised to 4k, so yes I suppose you
could make use of some sort of delay. I don't think it's necessarily `very
large' though -- anything ticking at over ~400KHz with HZ=100 would give you
a smaller delay.

> Thanks, so I'd set up loops_per_jiffy early, along with lpj_fine in
> init_current_timer_delay().

That should work, providing you can get a sensible initial estimate for
loops_per_jiffy.

Will
Santosh Shilimkar July 13, 2012, 10:48 a.m. UTC | #6
Wiil,

On Fri, Jul 13, 2012 at 2:27 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Fri, Jul 13, 2012 at 03:16:41AM +0100, Shinya Kuribayashi wrote:
>> On 7/13/2012 1:40 AM, Stephen Boyd wrote:
>> >>>> As a result, actual udelay()s may be toooo long than expected, in
>> >>>> particular udelay()s used between init_current_timer_delay() and
>> >>>> calibrate_delay().  It's unlikely be short, as the frequency of a
>> >>>> counter for read_current_timer is typically slower than CPU frequency.
>> >>> Surely using udelay before calibrate_delay_loop has been called is a
>> >>> fundamental error?
>> >> Got it.  I'm just not confident about disallowing early use of udelay().
>> >>
>> >
>> > I don't think it's an error. Instead you get a very large delay, similar
>> > to what would happen if you called udelay() before calibrate_delay()
>> > anyway (see the comment in init/main.c above loops_per_jiffy).
>
> Interesting, I didn't notice it was initialised to 4k, so yes I suppose you
> could make use of some sort of delay. I don't think it's necessarily `very
> large' though -- anything ticking at over ~400KHz with HZ=100 would give you
> a smaller delay.
>
>> Thanks, so I'd set up loops_per_jiffy early, along with lpj_fine in
>> init_current_timer_delay().
>
> That should work, providing you can get a sensible initial estimate for
> loops_per_jiffy.
>
Do you have an updated version of the patch ?

Regards,
Santosh

Patch
diff mbox

diff --git a/arch/arm/lib/delay.c b/arch/arm/lib/delay.c
index cb881c3..9065d30 100644
--- a/arch/arm/lib/delay.c
+++ b/arch/arm/lib/delay.c
@@ -57,7 +57,7 @@  static void __timer_udelay(unsigned long usecs)
 
 void __init init_current_timer_delay(unsigned long freq)
 {
-	lpj_fine			= (freq + HZ/2) / HZ;
+	loops_per_jiffy = lpj_fine	= (freq + HZ/2) / HZ;
 	arm_delay_ops.delay		= __timer_delay;
 	arm_delay_ops.const_udelay	= __timer_const_udelay;
 	arm_delay_ops.udelay		= __timer_udelay;