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

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

[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 <will.deacon@arm.com>
---
 arch/arm/lib/delay.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/lib/delay.c b/arch/arm/lib/delay.c
index d6dacc6..395d5fb 100644
--- a/arch/arm/lib/delay.c
+++ b/arch/arm/lib/delay.c
@@ -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;
-- 
1.7.4.1

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

[Will Deacon]

From: Jonathan Austin <Jonathan.Austin@arm.com>

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 <jonathan.austin@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---

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(-)

diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
index 62e7547..88401c2 100644
--- a/arch/arm/include/asm/arch_timer.h
+++ b/arch/arm/include/asm/arch_timer.h
@@ -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
diff --git a/arch/arm/include/asm/delay.h b/arch/arm/include/asm/delay.h
index dc61451..50928e8 100644
--- a/arch/arm/include/asm/delay.h
+++ b/arch/arm/include/asm/delay.h
@@ -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) */
diff --git a/arch/arm/include/asm/timex.h b/arch/arm/include/asm/timex.h
index ce11944..9acc135 100644
--- a/arch/arm/include/asm/timex.h
+++ b/arch/arm/include/asm/timex.h
@@ -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
diff --git a/arch/arm/kernel/arch_timer.c b/arch/arm/kernel/arch_timer.c
index cf25880..700917a 100644
--- a/arch/arm/kernel/arch_timer.c
+++ b/arch/arm/kernel/arch_timer.c
@@ -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:
diff --git a/arch/arm/lib/delay.c b/arch/arm/lib/delay.c
index 395d5fb..fe6f4fb 100644
--- a/arch/arm/lib/delay.c
+++ b/arch/arm/lib/delay.c
@@ -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
-- 
1.7.4.1

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

[Stephen Boyd]

On 8/29/2012 2:25 PM, Will Deacon wrote:
> 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 <will.deacon@arm.com>

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

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

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

[Stephen Boyd]

On 8/29/2012 2:25 PM, Will Deacon wrote:
> From: Jonathan Austin <Jonathan.Austin@arm.com>
>
> 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 <jonathan.austin@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

> diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
> index 62e7547..88401c2 100644
> --- a/arch/arm/include/asm/arch_timer.h
> +++ b/arch/arm/include/asm/arch_timer.h
> @@ -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
> diff --git a/arch/arm/include/asm/delay.h b/arch/arm/include/asm/delay.h
> index dc61451..50928e8 100644
> --- a/arch/arm/include/asm/delay.h
> +++ b/arch/arm/include/asm/delay.h
> @@ -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?

> @@ -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?

>  {
> -	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");

warn?

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

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

[Will Deacon]

On Fri, Aug 31, 2012 at 12:51:25AM +0100, Stephen Boyd wrote:
> On 8/29/2012 2:25 PM, Will Deacon wrote:
> > From: Jonathan Austin <Jonathan.Austin@arm.com>
> >
> > 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 <jonathan.austin@arm.com>
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> 
> Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

Thanks Stephen.

> 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.

> > @@ -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.

> > +		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

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

[Will Deacon]

On Tue, Sep 04, 2012 at 11:07:50AM +0100, Will Deacon wrote:
> On Fri, Aug 31, 2012 at 12:51:25AM +0100, Stephen Boyd wrote:
> > > +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

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

[Stephen Boyd]

On 09/04/12 03:44, Will Deacon wrote:
> On Tue, Sep 04, 2012 at 11:07:50AM +0100, Will Deacon wrote:
>> On Fri, Aug 31, 2012 at 12:51:25AM +0100, Stephen Boyd wrote:
>>>> +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<------

diff --git a/arch/arm/include/asm/delay.h b/arch/arm/include/asm/delay.h
index 50928e8..ab98fdd 100644
--- a/arch/arm/include/asm/delay.h
+++ b/arch/arm/include/asm/delay.h
@@ -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__ */
 
diff --git a/arch/arm/lib/delay.c b/arch/arm/lib/delay.c
index fe6f4fb..e868ef0 100644
--- a/arch/arm/lib/delay.c
+++ b/arch/arm/lib/delay.c
@@ -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");

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

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

[Will Deacon]

On Tue, Sep 04, 2012 at 06:11:51PM +0100, Stephen Boyd wrote:
> On 09/04/12 03:44, Will Deacon wrote:
> > On Tue, Sep 04, 2012 at 11:07:50AM +0100, Will Deacon wrote:
> >> On Fri, Aug 31, 2012 at 12:51:25AM +0100, Stephen Boyd wrote:
> >>>> +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.

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

> Squash this in and test compile?

Works a treat.

Will

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

[Will Deacon]

On Wed, Aug 29, 2012 at 10:25:24PM +0100, Will Deacon wrote:
> From: Jonathan Austin <Jonathan.Austin@arm.com>
> 
> 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.

[...]

> +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