Use architected timers for delay loop

This patch implements read_current_timer using the architected timers when they are selected via CONFIG_ARM_ARCH_TIMER. If they are detected not to be usable at runtime, we return -ENXIO to the caller.

Furthermore, if read_current_timer is exported then we can implement get_cycles in terms of it for use as both an entropy source and for implementing __udelay and friends.

arch/arm/include/asm/timex.h +5 -4

 #include <mach/timex.h>
 
 typedef unsigned long cycles_t;
 
-static inline cycles_t get_cycles (void)
-{
-	return 0;
-}
+#ifdef ARCH_HAS_READ_CURRENT_TIMER
+#define get_cycles()	({ cycles_t c; read_current_timer(&c) ? 0 : c; })
+#else
+#define get_cycles()	(0)
+#endif
 
 #endif

arch/arm/kernel/arch_timer.c +0 -0

index dd58035..dbbeec4 100644

arch/arm/include/asm/timex.h line 15

Quoted 9 lines · Will Deacon

arch/arm/include/asm/timex.h +1 -0

index 3be8de3..ce11944 100644
@@ -12,13 +12,15 @@
 #ifndef _ASMARM_TIMEX_H
 #define _ASMARM_TIMEX_H
 
+#include <asm/arch_timer.h>

This seems to cause compilation failures if you aren't building with architected timers enabled.

In file included from arch/arm/include/asm/timex.h:15,

                 from include/linux/timex.h:174,
                 from include/linux/sched.h:57,
                 from arch/arm/kernel/asm-offsets.c:13:

arch/arm/include/asm/arch_timer.h: In function 'arch_timer_of_register':
arch/arm/include/asm/arch_timer.h:11: error: 'ENXIO' undeclared (first use in this function)

Quoted 24 lines

arch/arm/include/asm/timex.h +5 -4

 #include <mach/timex.h>
 
 typedef unsigned long cycles_t;
 
-static inline cycles_t get_cycles (void)
-{
-	return 0;
-}
+#ifdef ARCH_HAS_READ_CURRENT_TIMER
+#define get_cycles()	({ cycles_t c; read_current_timer(&c) ? 0 : c; })
+#else
+#define get_cycles()	(0)
+#endif
 
 #endif

arch/arm/kernel/arch_timer.c +0 -0

index dd58035..dbbeec4 100644
@@ -223,6 +223,14 @@ static cycle_t arch_counter_read(struct clocksource *cs)
 	return arch_counter_get_cntpct();
 }
 

arch/arm/kernel/arch_timer.c static cycle_t arch_counter_read(struct clocksource *cs) · line 229

Quoted 4 lines

arch/arm/kernel/arch_timer.c +4 -0

+int read_current_timer(unsigned long *timer_val)
+{
+	if (!arch_timer_rate)
+		return -ENXIO;

Shouldn't this be returning 0? Otherwise get_cycles() up there will evaluate to -ENXIO?

Signature · 2 lines

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Quoted 6 lines · Stephen Boyd

arch/arm/kernel/arch_timer.c static cycle_t arch_counter_read(struct clocksource *cs) · line 229

Quoted 4 lines

arch/arm/kernel/arch_timer.c +4 -0

+int read_current_timer(unsigned long *timer_val)
+{
+	if (!arch_timer_rate)
+		return -ENXIO;

Shouldn't this be returning 0? Otherwise get_cycles() up there will evaluate to -ENXIO?

I don't think so. The cycle count is returned via the timer_val parameter and the get_cycles code checks the return value, so will give 0 if read_current_timer returns anything other than 0. The core calibration code (not that we call it) also expects a return value < 0 to indicate failure.

Quoted 6 lines · Will Deacon

arch/arm/kernel/arch_timer.c +0 -0

index dbbeec4..675cee0 100644
@@ -32,6 +32,8 @@ static int arch_timer_ppi2;
 

arch/arm/kernel/arch_timer.c static int arch_timer_ppi2; · line 35

Quoted 3 lines

arch/arm/kernel/arch_timer.c +1 -0

 static struct clock_event_device __percpu **arch_timer_evt;
 
+extern void init_current_timer_delay(unsigned long freq);

Can we find a home for this in some header file?

arch/arm/kernel/arch_timer.c static int arch_timer_ppi2;

Quoted 6 lines

arch/arm/kernel/arch_timer.c +6 -0

+static void __timer_const_udelay(unsigned long xloops)
+{
+	unsigned long long loops = xloops;
+	loops *= loops_per_jiffy;
+	__timer_delay(loops >> 30);
+}

Is it ok to have a 64 bit multiply here? It seems the assembly version tries to keep it all 32 bit math.

arch/arm/kernel/arch_timer.c

Quoted 5 lines

arch/arm/kernel/arch_timer.c +5 -0

+
+static void __timer_udelay(unsigned long usecs)
+{
+	__timer_const_udelay(usecs * ((2199023U * HZ) >> 11));
+}

It's unfortunate that we have to duplicate the same code and constants in both C and assembly. With my approach we convert delay.S into C and avoid the duplication.

arch/arm/kernel/arch_timer.c

Quoted 4 lines

arch/arm/kernel/arch_timer.c +4 -0

+
+void __init init_current_timer_delay(unsigned long freq)
+{
+	pr_info("Switching to timer-based delay loop\n");

Might be worth printing the frequency here too.

Quoted 4 lines · Stephen Boyd

It's unfortunate that we have to duplicate the same code and constants in both C and assembly. With my approach we convert delay.S into C and avoid the duplication.

It's probably easy enough to have a #define for the multiplier, I can do that for v2.

Quoted 8 lines · Will Deacon

Quoted 4 lines · Stephen Boyd

It's unfortunate that we have to duplicate the same code and constants in both C and assembly. With my approach we convert delay.S into C and avoid the duplication.

It's probably easy enough to have a #define for the multiplier, I can do that for v2.

Looks like I spoke too soon :)

The reason this is slightly problematic is due to gas's inability to represent unsigned constants -- namely that the 'U' suffix causes it to barf. I can fix it with the diff below, but please let me know what you think.

arch/arm/include/asm/delay.h +2 -0

+#endif /* __ASSEMBLY__ */
+
 #endif /* defined(_ARM_DELAY_H) */

arch/arm/lib/delay-loop.S +2 -2

index 1428a8f..36b668d 100644
@@ -9,11 +9,11 @@
  */
 #include <linux/linkage.h>
 #include <asm/assembler.h>
-#include <asm/param.h>
+#include <asm/delay.h>
 		.text
 
 .LC0:		.word	loops_per_jiffy
-.LC1:		.word	(2199023*HZ)>>11
+.LC1:		.word	UDELAY_MULT
 
 /*
  * r0  <= 2000

arch/arm/lib/delay.c +2 -2

index 2b590c3..e1030e1 100644
@@ -47,12 +47,12 @@ static void __timer_const_udelay(unsigned long xloops)
 {
 	unsigned long long loops = xloops;
 	loops *= loops_per_jiffy;
-	__timer_delay(loops >> 30);
+	__timer_delay(loops >> UDELAY_SHIFT);
 }
 
 static void __timer_udelay(unsigned long usecs)
 {
-	__timer_const_udelay(usecs * ((2199023U * HZ) >> 11));
+	__timer_const_udelay(usecs * UDELAY_MULT);
 }
 
 void __init init_current_timer_delay(unsigned long freq)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 4 lines · Will Deacon

The reason this is slightly problematic is due to gas's inability to represent unsigned constants -- namely that the 'U' suffix causes it to barf. I can fix it with the diff below, but please let me know what you think.

[snip]

Quoted 2 lines

arch/arm/include/asm/delay.h +2 -0

+#ifndef __ASSEMBLY__
+

arch/arm/include/asm/delay.h extern void __loop_delay(unsigned long loops); · line 60

Quoted 3 lines

arch/arm/include/asm/delay.h +3 -0

+#define UDELAY_MULT_HZ	2199023U
+#else
+#define UDELAY_MULT_HZ	2199023

Doesn't UL() macro do what you need?

Quoted 11 lines · Stephen Boyd

arch/arm/kernel/arch_timer.c static int arch_timer_ppi2; · line 35

Quoted 9 lines · Will Deacon

arch/arm/kernel/arch_timer.c +1 -0

index dbbeec4..675cee0 100644
@@ -32,6 +32,8 @@ static int arch_timer_ppi2;
 
 static struct clock_event_device __percpu **arch_timer_evt;
 
+extern void init_current_timer_delay(unsigned long freq);

Can we find a home for this in some header file?

I wondered about that...

The reason I didn't add it to a header file is that we really don't want it to be called willy-nilly across the kernel. In fact, it must be called exactly once by the clocksource backing read_current_timer when it knows that the timer is live and ticking.

I suppose I could allow the function to fail if it's called after we've calibrated. What do you reckon?

It's actually a 32-bit multiply with a 64-bit result, so it's just a umull:

00000050 <__timer_const_udelay>:
50: e3003000 movw r3, #0
54: e3403000 movt r3, #0
58: e5932000 ldr r2, [r3]
5c: e0832290 umull r2, r3, r0, r2
60: e1a00f22 lsr r0, r2, #30
64: e1800103 orr r0, r0, r3, lsl #2
68: eaffffe4 b 0 <__timer_delay>

Quoted 3 lines

It's unfortunate that we have to duplicate the same code and constants in both C and assembly. With my approach we convert delay.S into C and avoid the duplication.

It's probably easy enough to have a #define for the multiplier, I can do that for v2.

Quoted 6 lines

arch/arm/kernel/arch_timer.c static int arch_timer_ppi2; · line 39

Quoted 5 lines

arch/arm/kernel/arch_timer.c +4 -0

+
+void __init init_current_timer_delay(unsigned long freq)
+{
+	pr_info("Switching to timer-based delay loop\n");

Might be worth printing the frequency here too.

Could do, but the timer itself probably prints that information already (at least it does the arch timer).

Quoted 3 lines · Will Deacon

I suppose I could allow the function to fail if it's called after we've calibrated. What do you reckon?

Fair enough. Would anything actually go wrong if you called it twice? I would think everything would be assigned to what it already is but I haven't thought deeply about it. I don't really care to make the function more complicated for a case that should never happen.

Hello,

This patch series reworks our delay loop implementation to allow the busy-loop to be replaced with the physical counter included as part of the architected timer on recent CPUs. The polling implementation is still maintained to support cores without the timers and we use the same maths and upper delay bound of 2ms.

Russell - I think my maths is correct, but why can't we increase the
precision by using a magic constant of 4398046U and a right shift of 31?

Quoted 3 lines · Will Deacon

Russell - I think my maths is correct, but why can't we increase the
precision by using a magic constant of 4398046U and a right shift of 31?

It's been soo long since I looked at that code that I've forgotten the maths behind it.

What I can say is that the maths in the assembly code version is carefully chosen to avoid overflows during the various stages of computation without loosing too much precision along the way.

The assembly is also designed to round up any lost precision, so we try to produce a longer delay by way of the lost precision in the maths.

I forget what kind of precision we do get, but precision is not really a major concern here. We are only after a "rough" estimate of the delay (hell, we don't even guarantee according to Linus that you will get a minimum of the delay you asked for - you might get a shorter delay!) You _will_ get shorter delays because of the way the loop is calibrated (think: the jiffy code tries to find how many loops fit in a single jiffy, but a jiffy _also_ includes the overhead of running the timer interrupt, which artificially reduces loops_per_jiffy.)

Provided we get a delay "in the ballpark" of what was requested, that's good enough.

Hello,

This patch series reworks our delay loop implementation to allow the busy-loop to be replaced with the physical counter included as part of the architected timer on recent CPUs. The polling implementation is still maintained to support cores without the timers and we use the same maths and upper delay bound of 2ms.

Russell - I think my maths is correct, but why can't we increase the
precision by using a magic constant of 4398046U and a right shift of 31?

As usual, all feedback is welcome,

Quoted 5 lines · Will Deacon

This patch series reworks our delay loop implementation to allow the busy-loop to be replaced with the physical counter included as part of the architected timer on recent CPUs. The polling implementation is still maintained to support cores without the timers and we use the same maths and upper delay bound of 2ms.

I have posted a read_current_timer implementation to the list a couple times but had no success in getting it merged. The patches are still in the patch tracker but I haven't really pushed them to get merged.

    http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6874/1
    http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6873/1
    http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6875/1

So far we've just been carrying them internally. I suppose ARM needs this now because of big little?

Quoted 4 lines · Stephen Boyd

I have posted a read_current_timer implementation to the list a couple times but had no success in getting it merged. The patches are still in the patch tracker but I haven't really pushed them to get merged.

Quoted 3 lines

    http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6874/1
    http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6873/1
    http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6875/1

I took a look at these but I really shy away from using memory-mapped clocksources for delay -- it feels like we're asking for problems if we go down that route. Maybe it could work, but switching from the polling loop to the clocksource would surely require some recalibration?