[PATCH V2] [ARM] Add ARCH_PROVIDES_UDELAY config option

[PATCH V2] [ARM] Add ARCH_PROVIDES_UDELAY config option

[Colin Cross]

On SMP kernels, the loops_per_jiffy value is not scaled due to the
possibility of one CPU affecting the speed of another CPU while the
second CPU is in a udelay loop.  Since loops_per_jiffy is calculated
once on boot for SMP kernels, udelays are too long if the CPU
frequency is scaled down from the boot frequency, or too short if the
frequency is scaled up.  Some SOCs have a timer with a constant tick
rate that can be used to time udelays, similar to the TSC on x86.
Provide a config flag to allow these SOCs to override the default
ARM udelay implementation.

Signed-off-by: Colin Cross <ccross@android.com>
---
 arch/arm/Kconfig             |    3 +++
 arch/arm/include/asm/delay.h |    4 ++++
 arch/arm/lib/Makefile        |    6 +++++-
 3 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 33d2825..d9923b0 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -175,6 +175,9 @@ config ARM_L1_CACHE_SHIFT_6
 	help
 	  Setting ARM L1 cache line size to 64 Bytes.
 
+config ARCH_PROVIDES_UDELAY
+  bool
+
 if OPROFILE
 
 config OPROFILE_ARMV6
diff --git a/arch/arm/include/asm/delay.h b/arch/arm/include/asm/delay.h
index b2deda1..57f1fa0 100644
--- a/arch/arm/include/asm/delay.h
+++ b/arch/arm/include/asm/delay.h
@@ -8,6 +8,9 @@
 
 #include <asm/param.h>	/* HZ */
 
+#ifdef CONFIG_ARCH_PROVIDES_UDELAY
+#include <mach/delay.h>
+#else
 extern void __delay(int loops);
 
 /*
@@ -40,5 +43,6 @@ extern void __const_udelay(unsigned long);
 			__const_udelay((n) * ((2199023U*HZ)>>11))) :	\
 	  __udelay(n))
 
+#endif /* defined(ARCH_PROVIDES_UDELAY) */
 #endif /* defined(_ARM_DELAY_H) */
 
diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
index 030ba72..aa449e3 100644
--- a/arch/arm/lib/Makefile
+++ b/arch/arm/lib/Makefile
@@ -6,7 +6,7 @@
 
 lib-y		:= backtrace.o changebit.o csumipv6.o csumpartial.o   \
 		   csumpartialcopy.o csumpartialcopyuser.o clearbit.o \
-		   delay.o findbit.o memchr.o memcpy.o		      \
+		   findbit.o memchr.o memcpy.o                        \
 		   memmove.o memset.o memzero.o setbit.o              \
 		   strncpy_from_user.o strnlen_user.o                 \
 		   strchr.o strrchr.o                                 \
@@ -17,6 +17,10 @@ lib-y		:= backtrace.o changebit.o csumipv6.o csumpartial.o   \
 
 mmu-y	:= clear_user.o copy_page.o getuser.o putuser.o
 
+ifneq ($(CONFIG_ARCH_PROVIDES_UDELAY),y)
+  lib-y += delay.o
+endif
+
 # the code in uaccess.S is not preemption safe and
 # probably faster on ARMv3 only
 ifeq ($(CONFIG_PREEMPT),y)
-- 
1.5.4.3

[PATCH V2] [ARM] Add ARCH_PROVIDES_UDELAY config option

[Colin Cross]

An alternative to this patch would be to add a config option to use
sched_clock() to provide the counter instead of the cycle loop.  The
same loops_per_jiffy calibration could be done to  determine the
sched_clock frequency.  Any machine with an available constant tick
rate counter, which is likely to be used for sched_clock() already,
can enable CONFIG_UDELAY_USES_SCHED_CLOCK.

On Fri, Apr 30, 2010 at 12:12 PM, Colin Cross <ccross@android.com> wrote:
> On SMP kernels, the loops_per_jiffy value is not scaled due to the
> possibility of one CPU affecting the speed of another CPU while the
> second CPU is in a udelay loop. ?Since loops_per_jiffy is calculated
> once on boot for SMP kernels, udelays are too long if the CPU
> frequency is scaled down from the boot frequency, or too short if the
> frequency is scaled up. ?Some SOCs have a timer with a constant tick
> rate that can be used to time udelays, similar to the TSC on x86.
> Provide a config flag to allow these SOCs to override the default
> ARM udelay implementation.
>
> Signed-off-by: Colin Cross <ccross@android.com>
> ---
> ?arch/arm/Kconfig ? ? ? ? ? ? | ? ?3 +++
> ?arch/arm/include/asm/delay.h | ? ?4 ++++
> ?arch/arm/lib/Makefile ? ? ? ?| ? ?6 +++++-
> ?3 files changed, 12 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 33d2825..d9923b0 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -175,6 +175,9 @@ config ARM_L1_CACHE_SHIFT_6
> ? ? ? ?help
> ? ? ? ? ?Setting ARM L1 cache line size to 64 Bytes.
>
> +config ARCH_PROVIDES_UDELAY
> + ?bool
> +
> ?if OPROFILE
>
> ?config OPROFILE_ARMV6
> diff --git a/arch/arm/include/asm/delay.h b/arch/arm/include/asm/delay.h
> index b2deda1..57f1fa0 100644
> --- a/arch/arm/include/asm/delay.h
> +++ b/arch/arm/include/asm/delay.h
> @@ -8,6 +8,9 @@
>
> ?#include <asm/param.h> /* HZ */
>
> +#ifdef CONFIG_ARCH_PROVIDES_UDELAY
> +#include <mach/delay.h>
> +#else
> ?extern void __delay(int loops);
>
> ?/*
> @@ -40,5 +43,6 @@ extern void __const_udelay(unsigned long);
> ? ? ? ? ? ? ? ? ? ? ? ?__const_udelay((n) * ((2199023U*HZ)>>11))) : ? ?\
> ? ? ? ? ?__udelay(n))
>
> +#endif /* defined(ARCH_PROVIDES_UDELAY) */
> ?#endif /* defined(_ARM_DELAY_H) */
>
> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
> index 030ba72..aa449e3 100644
> --- a/arch/arm/lib/Makefile
> +++ b/arch/arm/lib/Makefile
> @@ -6,7 +6,7 @@
>
> ?lib-y ? ? ? ? ?:= backtrace.o changebit.o csumipv6.o csumpartial.o ? \
> ? ? ? ? ? ? ? ? ? csumpartialcopy.o csumpartialcopyuser.o clearbit.o \
> - ? ? ? ? ? ? ? ? ?delay.o findbit.o memchr.o memcpy.o ? ? ? ? ? ? ? ?\
> + ? ? ? ? ? ? ? ? ?findbit.o memchr.o memcpy.o ? ? ? ? ? ? ? ? ? ? ? ?\
> ? ? ? ? ? ? ? ? ? memmove.o memset.o memzero.o setbit.o ? ? ? ? ? ? ?\
> ? ? ? ? ? ? ? ? ? strncpy_from_user.o strnlen_user.o ? ? ? ? ? ? ? ? \
> ? ? ? ? ? ? ? ? ? strchr.o strrchr.o ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> @@ -17,6 +17,10 @@ lib-y ? ? ? ? ? ? ? ?:= backtrace.o changebit.o csumipv6.o csumpartial.o ? \
>
> ?mmu-y ?:= clear_user.o copy_page.o getuser.o putuser.o
>
> +ifneq ($(CONFIG_ARCH_PROVIDES_UDELAY),y)
> + ?lib-y += delay.o
> +endif
> +
> ?# the code in uaccess.S is not preemption safe and
> ?# probably faster on ARMv3 only
> ?ifeq ($(CONFIG_PREEMPT),y)
> --
> 1.5.4.3
>
>

[PATCH V2] [ARM] Add ARCH_PROVIDES_UDELAY config option

[Kevin Hilman]

Colin Cross <ccross@android.com> writes:

> An alternative to this patch would be to add a config option to use
> sched_clock() to provide the counter instead of the cycle loop.  The
> same loops_per_jiffy calibration could be done to  determine the
> sched_clock frequency.  Any machine with an available constant tick
> rate counter, which is likely to be used for sched_clock() already,
> can enable CONFIG_UDELAY_USES_SCHED_CLOCK.

Or even better, why not have an option to use the clocksource which is
most likely using the constant tick timer as well.

Kevin

> On Fri, Apr 30, 2010 at 12:12 PM, Colin Cross <ccross@android.com> wrote:
>> On SMP kernels, the loops_per_jiffy value is not scaled due to the
>> possibility of one CPU affecting the speed of another CPU while the
>> second CPU is in a udelay loop. ?Since loops_per_jiffy is calculated
>> once on boot for SMP kernels, udelays are too long if the CPU
>> frequency is scaled down from the boot frequency, or too short if the
>> frequency is scaled up. ?Some SOCs have a timer with a constant tick
>> rate that can be used to time udelays, similar to the TSC on x86.
>> Provide a config flag to allow these SOCs to override the default
>> ARM udelay implementation.
>>
>> Signed-off-by: Colin Cross <ccross@android.com>

[PATCH V2] [ARM] Add ARCH_PROVIDES_UDELAY config option

[Saravana Kannan]

On some machines/boards, the timer used for the sched clock does not 
have usec accuracy. So, we should not assume that a sched clock is 
sufficient.

It won't be wrong, but if we can't ever give usec level accuracy, we 
shouldn't act as if we can.

Cross,

My comment about CPU freq switching while we are delay looping is 
present even for non-SMP kernels. Yes, it's bad, but we don't have a 
scalable solution yet. So, you might want to reword it yet again :-)

Thanks,
Saravana


Kevin Hilman wrote:
> Colin Cross <ccross@android.com> writes:
> 
>> An alternative to this patch would be to add a config option to use
>> sched_clock() to provide the counter instead of the cycle loop.  The
>> same loops_per_jiffy calibration could be done to  determine the
>> sched_clock frequency.  Any machine with an available constant tick
>> rate counter, which is likely to be used for sched_clock() already,
>> can enable CONFIG_UDELAY_USES_SCHED_CLOCK.
> 
> Or even better, why not have an option to use the clocksource which is
> most likely using the constant tick timer as well.
> 
> Kevin
> 
>> On Fri, Apr 30, 2010 at 12:12 PM, Colin Cross <ccross@android.com> wrote:
>>> On SMP kernels, the loops_per_jiffy value is not scaled due to the
>>> possibility of one CPU affecting the speed of another CPU while the
>>> second CPU is in a udelay loop.  Since loops_per_jiffy is calculated
>>> once on boot for SMP kernels, udelays are too long if the CPU
>>> frequency is scaled down from the boot frequency, or too short if the
>>> frequency is scaled up.  Some SOCs have a timer with a constant tick
>>> rate that can be used to time udelays, similar to the TSC on x86.
>>> Provide a config flag to allow these SOCs to override the default
>>> ARM udelay implementation.
>>>
>>> Signed-off-by: Colin Cross <ccross@android.com>

[PATCH V2] [ARM] Add ARCH_PROVIDES_UDELAY config option

[Russell King - ARM Linux]

On Fri, Apr 30, 2010 at 03:11:20PM -0700, Kevin Hilman wrote:
> Colin Cross <ccross@android.com> writes:
> 
> > An alternative to this patch would be to add a config option to use
> > sched_clock() to provide the counter instead of the cycle loop.  The
> > same loops_per_jiffy calibration could be done to  determine the
> > sched_clock frequency.  Any machine with an available constant tick
> > rate counter, which is likely to be used for sched_clock() already,
> > can enable CONFIG_UDELAY_USES_SCHED_CLOCK.
> 
> Or even better, why not have an option to use the clocksource which is
> most likely using the constant tick timer as well.

We may be running into the same problem which we did with the printk
clock - that is using a machine provided sched_clock() or clocksource
requires MMIO accesses, which can only be done after the IO mappings
have been initialized.

Let's hope no one ever uses udelay() before the necessary IO mappings
are present.

[PATCH V2] [ARM] Add ARCH_PROVIDES_UDELAY config option

[Saravana Kannan]

Russell King - ARM Linux wrote:
> On Fri, Apr 30, 2010 at 03:11:20PM -0700, Kevin Hilman wrote:
>> Colin Cross <ccross@android.com> writes:
>>
>>> An alternative to this patch would be to add a config option to use
>>> sched_clock() to provide the counter instead of the cycle loop.  The
>>> same loops_per_jiffy calibration could be done to  determine the
>>> sched_clock frequency.  Any machine with an available constant tick
>>> rate counter, which is likely to be used for sched_clock() already,
>>> can enable CONFIG_UDELAY_USES_SCHED_CLOCK.
>> Or even better, why not have an option to use the clocksource which is
>> most likely using the constant tick timer as well.
> 
> We may be running into the same problem which we did with the printk
> clock - that is using a machine provided sched_clock() or clocksource
> requires MMIO accesses, which can only be done after the IO mappings
> have been initialized.
> 
> Let's hope no one ever uses udelay() before the necessary IO mappings
> are present.

Is the patch that uses CONFIG_ARCH_PROVIDES_UDELAY acceptable? I don't 
care much for how each arch decides to implement it, but I think we 
should have this config to let each arch decide how they want to handle 
udelay.

I personally prefer not to use the sched clock source due to the 
unnecessary complexities. If you have a some kind of constant counter, 
it sounds much simpler to just use it instead of adding dependencies 
between udelay and sched clock.

-Saravana

[PATCH V2] [ARM] Add ARCH_PROVIDES_UDELAY config option

[Russell King - ARM Linux]

On Fri, May 21, 2010 at 03:01:48PM -0700, Saravana Kannan wrote:
> Russell King - ARM Linux wrote:
>> We may be running into the same problem which we did with the printk
>> clock - that is using a machine provided sched_clock() or clocksource
>> requires MMIO accesses, which can only be done after the IO mappings
>> have been initialized.
>>
>> Let's hope no one ever uses udelay() before the necessary IO mappings
>> are present.
>
> Is the patch that uses CONFIG_ARCH_PROVIDES_UDELAY acceptable? I don't  
> care much for how each arch decides to implement it, but I think we  
> should have this config to let each arch decide how they want to handle  
> udelay.
>
> I personally prefer not to use the sched clock source due to the  
> unnecessary complexities. If you have a some kind of constant counter,  
> it sounds much simpler to just use it instead of adding dependencies  
> between udelay and sched clock.

My point is not specific to sched_clock, but to counters which on ARM
are 99.9% always memory mapped, and therefore inaccessible during the
very early kernel boot.  sched_clock was merely an illustration of the
problem.

[PATCH V2] [ARM] Add ARCH_PROVIDES_UDELAY config option

[Saravana Kannan]

Russell King - ARM Linux wrote:
> On Fri, May 21, 2010 at 03:01:48PM -0700, Saravana Kannan wrote:
>> Russell King - ARM Linux wrote:
>>> We may be running into the same problem which we did with the printk
>>> clock - that is using a machine provided sched_clock() or clocksource
>>> requires MMIO accesses, which can only be done after the IO mappings
>>> have been initialized.
>>>
>>> Let's hope no one ever uses udelay() before the necessary IO mappings
>>> are present.
>> Is the patch that uses CONFIG_ARCH_PROVIDES_UDELAY acceptable? I don't  
>> care much for how each arch decides to implement it, but I think we  
>> should have this config to let each arch decide how they want to handle  
>> udelay.
>>
>> I personally prefer not to use the sched clock source due to the  
>> unnecessary complexities. If you have a some kind of constant counter,  
>> it sounds much simpler to just use it instead of adding dependencies  
>> between udelay and sched clock.
> 
> My point is not specific to sched_clock, but to counters which on ARM
> are 99.9% always memory mapped, and therefore inaccessible during the
> very early kernel boot.  sched_clock was merely an illustration of the
> problem.

Agree with the point about the counters being memory mapped. But does 
any of the really early init code use udelay? AFAIK, udelay is mostly 
used when talking to devices at which point IO mapping needs to have 
been completed to be able to talk to the device in the first place.

Even otherwise, a given arch might not need udelay during early init 
code. So, we could still give them that option. No?

-Saravana

[PATCH V2] [ARM] Add ARCH_PROVIDES_UDELAY config option

[Saravana Kannan]

Russell King - ARM Linux wrote:
> My point is not specific to sched_clock, but to counters which on ARM
> are 99.9% always memory mapped, and therefore inaccessible during the
> very early kernel boot.  sched_clock was merely an illustration of the
> problem.

Do you know any code which uses udelay that early? I can't imagine any 
non-driver code using udelay. If it's just drivers using udelay, then 
they need IO mapping to have been completed for the device register 
access to work and hence udelay won't have any problems.

-Saravana

[PATCH V2] [ARM] Add ARCH_PROVIDES_UDELAY config option

[Saravana Kannan]

Russel,

Would appreciate your thoughts on this.

Colin,

Are you continuing to use the approach used in your patch in your 
development tree? I just want to make sure we have a solution that 
everyone can agree to.

Thanks,
Saravana

Saravana Kannan wrote:
> Russell King - ARM Linux wrote:
>> My point is not specific to sched_clock, but to counters which on ARM
>> are 99.9% always memory mapped, and therefore inaccessible during the
>> very early kernel boot.  sched_clock was merely an illustration of the
>> problem.
> 
> Do you know any code which uses udelay that early? I can't imagine any 
> non-driver code using udelay. If it's just drivers using udelay, then 
> they need IO mapping to have been completed for the device register 
> access to work and hence udelay won't have any problems.
> 
> -Saravana
> 

[PATCH V2] [ARM] Add ARCH_PROVIDES_UDELAY config option

[Colin Cross]

I am using this patch in our Tegra development tree.  So far, I
haven't come across any need for supporting udelay before iomap, but
if it is necessary, it could be handled in the per-architecture
implementation by using a busy loop with a worst-case count if a
sched_clock initialization flag is not set.

On Mon, Jun 21, 2010 at 6:14 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
> Russel,
>
> Would appreciate your thoughts on this.
>
> Colin,
>
> Are you continuing to use the approach used in your patch in your
> development tree? I just want to make sure we have a solution that everyone
> can agree to.
>
> Thanks,
> Saravana
>
> Saravana Kannan wrote:
>>
>> Russell King - ARM Linux wrote:
>>>
>>> My point is not specific to sched_clock, but to counters which on ARM
>>> are 99.9% always memory mapped, and therefore inaccessible during the
>>> very early kernel boot. ?sched_clock was merely an illustration of the
>>> problem.
>>
>> Do you know any code which uses udelay that early? I can't imagine any
>> non-driver code using udelay. If it's just drivers using udelay, then they
>> need IO mapping to have been completed for the device register access to
>> work and hence udelay won't have any problems.
>>
>> -Saravana
>>
>
>