ARM: delay: set loops_per_jiffy when moving to timer-based loop

PATCH New Will Deacon 2 participants 9 messages Aug 29, 2012 – Sep 7, 2012

Subsystems (2)

Arm (32-bit) Arm architected timer driver

lore

Share & tools

lore

Will Deacon

author

2012-08-29 21:25 UTC #0 lore raw

PATCH v2 1/2 ARM: delay: set loops_per_jiffy when moving to timer-based loop

The delay functions may be called by some platforms between switching to the timer-based delay loop but before calibration. In this case, the initial loops_per_jiffy may not be suitable for the timer (although a compromise may be achievable) and delay times may be considered too inaccurate.

This patch updates loops_per_jiffy when switching to the timer-based delay loop so that delays are consistent prior to calibration.

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm/lib/delay.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

arch/arm/lib/delay.c +1 -0

index d6dacc6..395d5fb 100644
@@ -59,6 +59,7 @@ void __init init_current_timer_delay(unsigned long freq)
 {
 	pr_info("Switching to timer-based delay loop\n");
 	lpj_fine			= freq / HZ;
+	loops_per_jiffy			= lpj_fine;
 	arm_delay_ops.delay		= __timer_delay;
 	arm_delay_ops.const_udelay	= __timer_const_udelay;
 	arm_delay_ops.udelay		= __timer_udelay;

Signature · 1 line

1.7.4.1

Will Deacon

author

2012-08-29 21:25 UTC #1 lore raw

PATCH v2 2/2 ARM: delay: add registration mechanism for delay timer sources

From: Jonathan Austin <[email protected]>

The current timer-based delay loop relies on the architected timer to initiate the switch away from the polling-based implementation. This is unfortunate for platforms without the architected timers but with a suitable delay source (that is, constant frequency, always powered-up and ticking as long as the CPUs are online).

This patch introduces a registration mechanism for the delay timer (which provides an unconditional read_current_timer implementation) and updates the architected timer code to use the new interface.

Signed-off-by Jonathan Austin <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---

v2: Changed prototype for low-level read_current_timer to return the
timer value directly.

 arch/arm/include/asm/arch_timer.h |    1 -
 arch/arm/include/asm/delay.h      |    9 +++++++++
 arch/arm/include/asm/timex.h      |    6 ------
 arch/arm/kernel/arch_timer.c      |   17 ++++++++---------
 arch/arm/lib/delay.c              |   31 ++++++++++++++++++++++---------
 5 files changed, 39 insertions(+), 25 deletions(-)

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

index 62e7547..88401c2 100644
@@ -4,7 +4,6 @@
 #include <asm/errno.h>
 
 #ifdef CONFIG_ARM_ARCH_TIMER
-#define ARCH_HAS_READ_CURRENT_TIMER
 int arch_timer_of_register(void);
 int arch_timer_sched_clock_init(void);
 #else

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

index dc61451..50928e8 100644
@@ -15,6 +15,11 @@
 
 #ifndef __ASSEMBLY__
 
+struct delay_timer {
+	unsigned long (*read_current_timer)(void);
+	unsigned long freq;
+};
+
 extern struct arm_delay_ops {
 	void (*delay)(unsigned long);
 	void (*const_udelay)(unsigned long);

@@ -56,6 +61,10 @@ extern void __loop_delay(unsigned long loops);
 extern void __loop_udelay(unsigned long usecs);
 extern void __loop_const_udelay(unsigned long);
 
+/* Delay-loop timer registration. */
+#define ARCH_HAS_READ_CURRENT_TIMER
+extern void register_current_timer_delay(struct delay_timer *timer);
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* defined(_ARM_DELAY_H) */

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

index ce11944..9acc135 100644
@@ -12,15 +12,9 @@
 #ifndef _ASMARM_TIMEX_H
 #define _ASMARM_TIMEX_H
 
-#include <asm/arch_timer.h>
 #include <mach/timex.h>
 
 typedef unsigned long cycles_t;
-
-#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 +8 -9

index cf25880..700917a 100644
@@ -21,6 +21,7 @@
 #include <linux/io.h>
 
 #include <asm/cputype.h>
+#include <asm/delay.h>
 #include <asm/localtimer.h>
 #include <asm/arch_timer.h>
 #include <asm/system_info.h>

@@ -31,8 +32,7 @@ static int arch_timer_ppi;
 static int arch_timer_ppi2;
 
 static struct clock_event_device __percpu **arch_timer_evt;
-
-extern void init_current_timer_delay(unsigned long freq);
+static struct delay_timer arch_delay_timer;
 
 /*
  * Architected system timer support.

@@ -225,12 +225,9 @@ static cycle_t arch_counter_read(struct clocksource *cs)
 	return arch_counter_get_cntpct();
 }
 
-int read_current_timer(unsigned long *timer_val)
+static unsigned long arch_timer_read_current_timer(void)
 {
-	if (!arch_timer_rate)
-		return -ENXIO;
-	*timer_val = arch_counter_get_cntpct();
-	return 0;
+	return arch_counter_get_cntpct();
 }
 
 static struct clocksource clocksource_counter = {

@@ -302,11 +299,13 @@ static int __init arch_timer_register(void)
 		arch_timer_global_evt.cpumask = cpumask_of(0);
 		err = arch_timer_setup(&arch_timer_global_evt);
 	}
-
 	if (err)
 		goto out_free_irq;
 
-	init_current_timer_delay(arch_timer_rate);
+	/* Use the architected timer for the delay loop. */
+	arch_delay_timer.read_current_timer = &arch_timer_read_current_timer;
+	arch_delay_timer.freq = arch_timer_rate;
+	register_current_timer_delay(&arch_delay_timer);
 	return 0;
 
 out_free_irq:

arch/arm/lib/delay.c +22 -9

index 395d5fb..fe6f4fb 100644
@@ -34,7 +34,14 @@ struct arm_delay_ops arm_delay_ops = {
 	.udelay		= __loop_udelay,
 };
 
-#ifdef ARCH_HAS_READ_CURRENT_TIMER
+static struct delay_timer *delay_timer;
+static bool delay_calibrated;
+
+int read_current_timer(unsigned long *timer_val)
+{
+	return delay_timer ? delay_timer->read_current_timer() : -ENXIO;
+}
+
 static void __timer_delay(unsigned long cycles)
 {
 	cycles_t start = get_cycles();

@@ -55,18 +62,24 @@ static void __timer_udelay(unsigned long usecs)
 	__timer_const_udelay(usecs * UDELAY_MULT);
 }
 
-void __init init_current_timer_delay(unsigned long freq)
+void __init register_current_timer_delay(struct delay_timer *timer)
 {
-	pr_info("Switching to timer-based delay loop\n");
-	lpj_fine			= freq / HZ;
-	loops_per_jiffy			= lpj_fine;
-	arm_delay_ops.delay		= __timer_delay;
-	arm_delay_ops.const_udelay	= __timer_const_udelay;
-	arm_delay_ops.udelay		= __timer_udelay;
+	if (!delay_calibrated) {
+		pr_info("Switching to timer-based delay loop\n");
+		delay_timer			= timer;
+		lpj_fine			= timer->freq / HZ;
+		loops_per_jiffy			= lpj_fine;
+		arm_delay_ops.delay		= __timer_delay;
+		arm_delay_ops.const_udelay	= __timer_const_udelay;
+		arm_delay_ops.udelay		= __timer_udelay;
+		delay_calibrated		= true;
+	} else {
+		pr_info("Ignoring duplicate/late registration of read_current_timer delay\n");
+	}
 }
 
 unsigned long __cpuinit calibrate_delay_is_known(void)
 {
+	delay_calibrated = true;
 	return lpj_fine;
 }
-#endif

Signature · 1 line

1.7.4.1

Stephen Boyd

2012-08-30 23:51 UTC #2 lore raw

PATCH v2 2/2 ARM: delay: add registration mechanism for delay timer sources

Quoted 8 lines · Will Deacon

From: Jonathan Austin <[email protected]>
The current timer-based delay loop relies on the architected timer to initiate the switch away from the polling-based implementation. This is unfortunate for platforms without the architected timers but with a suitable delay source (that is, constant frequency, always powered-up and ticking as long as the CPUs are online).

Quoted 6 lines

This patch introduces a registration mechanism for the delay timer (which provides an unconditional read_current_timer implementation) and updates the architected timer code to use the new interface.
Signed-off-by Jonathan Austin <[email protected]>
Signed-off-by Will Deacon <[email protected]>

Reviewed-by Stephen Boyd <[email protected]>

Quoted 17 lines

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

index 62e7547..88401c2 100644
@@ -4,7 +4,6 @@
 #include <asm/errno.h>
 
 #ifdef CONFIG_ARM_ARCH_TIMER
-#define ARCH_HAS_READ_CURRENT_TIMER
 int arch_timer_of_register(void);
 int arch_timer_sched_clock_init(void);
 #else

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

index dc61451..50928e8 100644

arch/arm/include/asm/delay.h line 20

Quoted 6 lines

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

@@ -15,6 +15,11 @@
 
 #ifndef __ASSEMBLY__
 
+struct delay_timer {
+	unsigned long (*read_current_timer)(void);
+	unsigned long freq;

I wonder if we should print a warning and not actually switch to the timer based udelay if the frequency is not fast enough (< 1Mhz). Or people just wouldn't do that?

arch/arm/include/asm/delay.h static void __timer_udelay(unsigned long usecs) · line 65

Quoted 6 lines

arch/arm/include/asm/delay.h +1 -1

@@ -55,18 +62,24 @@ static void __timer_udelay(unsigned long usecs)
 	__timer_const_udelay(usecs * UDELAY_MULT);
 }
 
-void __init init_current_timer_delay(unsigned long freq)
+void __init register_current_timer_delay(struct delay_timer *timer)

const?

Quoted 8 lines

arch/arm/include/asm/delay.h +1 -6

 {
-	pr_info("Switching to timer-based delay loop\n");
-	lpj_fine			= freq / HZ;
-	loops_per_jiffy			= lpj_fine;
-	arm_delay_ops.delay		= __timer_delay;
-	arm_delay_ops.const_udelay	= __timer_const_udelay;
-	arm_delay_ops.udelay		= __timer_udelay;
+	if (!delay_calibrated) {

arch/arm/include/asm/delay.h static void __timer_udelay(unsigned long usecs) · line 77

Quoted 10 lines

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

+		pr_info("Switching to timer-based delay loop\n");
+		delay_timer			= timer;
+		lpj_fine			= timer->freq / HZ;
+		loops_per_jiffy			= lpj_fine;
+		arm_delay_ops.delay		= __timer_delay;
+		arm_delay_ops.const_udelay	= __timer_const_udelay;
+		arm_delay_ops.udelay		= __timer_udelay;
+		delay_calibrated		= true;
+	} else {
+		pr_info("Ignoring duplicate/late registration of read_current_timer delay\n");

warn?

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.

Will Deacon

author

2012-09-04 10:07 UTC #3 lore raw

PATCH v2 2/2 ARM: delay: add registration mechanism for delay timer sources

Quoted 9 lines · Stephen Boyd

Quoted 8 lines · Will Deacon
From: Jonathan Austin <[email protected]>
The current timer-based delay loop relies on the architected timer to initiate the switch away from the polling-based implementation. This is unfortunate for platforms without the architected timers but with a suitable delay source (that is, constant frequency, always powered-up and ticking as long as the CPUs are online).

Quoted 8 lines

Quoted 6 lines
This patch introduces a registration mechanism for the delay timer (which provides an unconditional read_current_timer implementation) and updates the architected timer code to use the new interface.
Signed-off-by Jonathan Austin <[email protected]>
Signed-off-by Will Deacon <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>

Thanks Stephen.

Quoted 3 lines

I wonder if we should print a warning and not actually switch to the timer based udelay if the frequency is not fast enough (< 1Mhz). Or people just wouldn't do that?

I don't think we need to worry about that -- we should let people decide whether they deem their timer to be appropriate or not.

Quoted 8 lines

arch/arm/lib/delay.c static void __timer_udelay(unsigned long usecs) · line 65

Quoted 6 lines

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

@@ -55,18 +62,24 @@ static void __timer_udelay(unsigned long usecs)
 	__timer_const_udelay(usecs * UDELAY_MULT);
 }
 
-void __init init_current_timer_delay(unsigned long freq)
+void __init register_current_timer_delay(struct delay_timer *timer)

const?

Sure. I'll send an updated patch.

Quoted 3 lines

arch/arm/lib/delay.c static void __timer_udelay(unsigned long usecs) · line 77
Quoted 1 line
arch/arm/lib/delay.c +1 -0
+		pr_info("Ignoring duplicate/late registration of read_current_timer delay\n");
warn?

Nah, it's not the end of the world if this happens. We'll just fall back to the CPU-based calibration, which isn't a problem.

Will

Will Deacon

author

2012-09-04 10:44 UTC #4 lore raw

PATCH v2 2/2 ARM: delay: add registration mechanism for delay timer sources

Quoted 6 lines · Will Deacon

Quoted 3 lines · Stephen Boyd
Quoted 1 line
+void __init register_current_timer_delay(struct delay_timer *timer)
const?
Sure. I'll send an updated patch.

Gah, spoke too soon. This isn't going to work, as the frequency may not be known until probe time (for example, it could be obtained by the device tree) so const data isn't going to cut it.

Will

Stephen Boyd

2012-09-04 17:11 UTC #5 lore raw

PATCH v2 2/2 ARM: delay: add registration mechanism for delay timer sources

Quoted 9 lines · Will Deacon

Quoted 4 lines · Will Deacon
Quoted 2 lines · Stephen Boyd
Quoted 1 line
+void __init register_current_timer_delay(struct delay_timer *timer)
const?
Sure. I'll send an updated patch.
Gah, spoke too soon. This isn't going to work, as the frequency may not be known until probe time (for example, it could be obtained by the device tree) so const data isn't going to cut it.

That's fine. We can still do const can't we? I was suggesting you make the argument const and not the data in arch_timer.c const. This way we can be sure that delay.c isn't going to muck with the struct, but the ones who register the struct can fill it in at runtime.

Squash this in and test compile?

----8<------

arch/arm/include/asm/delay.h +1 -1

index 50928e8..ab98fdd 100644
@@ -63,7 +63,7 @@ extern void __loop_const_udelay(unsigned long);
 
 /* Delay-loop timer registration. */
 #define ARCH_HAS_READ_CURRENT_TIMER
-extern void register_current_timer_delay(struct delay_timer *timer);
+extern void register_current_timer_delay(const struct delay_timer *timer);
 
 #endif /* __ASSEMBLY__ */

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

index fe6f4fb..e868ef0 100644
@@ -34,7 +34,7 @@ struct arm_delay_ops arm_delay_ops = {
        .udelay         = __loop_udelay,
 };
 
-static struct delay_timer *delay_timer;
+static const struct delay_timer *delay_timer;
 static bool delay_calibrated;
 
 int read_current_timer(unsigned long *timer_val)

@@ -62,7 +62,7 @@ static void __timer_udelay(unsigned long usecs)
        __timer_const_udelay(usecs * UDELAY_MULT);
 }
 
-void __init register_current_timer_delay(struct delay_timer *timer)
+void __init register_current_timer_delay(const struct delay_timer *timer)
 {
        if (!delay_calibrated) {
                pr_info("Switching to timer-based delay loop\n");

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.

Will Deacon

author

2012-09-04 17:38 UTC #6 lore raw

PATCH v2 2/2 ARM: delay: add registration mechanism for delay timer sources

Quoted 11 lines · Stephen Boyd

Quoted 9 lines · Will Deacon
Quoted 4 lines · Will Deacon
Quoted 2 lines · Stephen Boyd
Quoted 1 line
+void __init register_current_timer_delay(struct delay_timer *timer)
const?
Sure. I'll send an updated patch.
Gah, spoke too soon. This isn't going to work, as the frequency may not be known until probe time (for example, it could be obtained by the device tree) so const data isn't going to cut it.

Quoted 4 lines

That's fine. We can still do const can't we? I was suggesting you make the argument const and not the data in arch_timer.c const. This way we can be sure that delay.c isn't going to muck with the struct, but the ones who register the struct can fill it in at runtime.

Aha, gotcha. I thought you wanted to enforce const-ness on the provider for some reason.

Quoted 1 line

Squash this in and test compile?

Works a treat.

Will

Will Deacon

author

2012-09-07 17:07 UTC #7 lore raw

PATCH v2 2/2 ARM: delay: add registration mechanism for delay timer sources

In reply to Will Deacon

Quoted 11 lines · Will Deacon

From: Jonathan Austin <[email protected]>
The current timer-based delay loop relies on the architected timer to initiate the switch away from the polling-based implementation. This is unfortunate for platforms without the architected timers but with a suitable delay source (that is, constant frequency, always powered-up and ticking as long as the CPUs are online).
This patch introduces a registration mechanism for the delay timer (which provides an unconditional read_current_timer implementation) and updates the architected timer code to use the new interface.

[...]

arch/arm/lib/delay.c struct arm_delay_ops arm_delay_ops = { · line 43

Quoted 4 lines

arch/arm/lib/delay.c +4 -0

+int read_current_timer(unsigned long *timer_val)
+{
+	return delay_timer ? delay_timer->read_current_timer() : -ENXIO;
+}

So this is clearly wrong with the new function prototype -- I've fixed it on my delay branch (when the mirrors sync).

Will

Stephen Boyd

2012-08-30 23:51 UTC #8 lore raw

PATCH v2 1/2 ARM: delay: set loops_per_jiffy when moving to timer-based loop

In reply to Will Deacon

Quoted 10 lines · Will Deacon

The delay functions may be called by some platforms between switching to the timer-based delay loop but before calibration. In this case, the initial loops_per_jiffy may not be suitable for the timer (although a compromise may be achievable) and delay times may be considered too inaccurate.
This patch updates loops_per_jiffy when switching to the timer-based delay loop so that delays are consistent prior to calibration.
Signed-off-by Will Deacon <[email protected]>

Reviewed-by Stephen Boyd <[email protected]>

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.