[U-Boot] [PATCH V4] Nomadik: fix reset_timer()

[U-Boot] [PATCH V4] Nomadik: fix reset_timer()

[Alessandro Rubini]

From: Alessandro Rubini <rubini@unipv.it>

Previous code was failing when reading back the timer less than
400us after resetting it. This lead nand operations to incorrectly
timeout any now and then.  Moreover, writing the load register isn't
immediately reflected in the value register. We must wait for a clock
edge, so read_timer now waits for the value to change at least once,
otherwise nand operation would timeout anyways (though less frequently).


Signed-off-by: Alessandro Rubini <rubini@unipv.it>
Acked-by: Andrea Gallo <andrea.gallo@stericsson.com>
---

I finally managed to test on hardware my V2 and V3, posted on
2009-11-06 after a compile check. V2 was the denk-suggested way, while
V3 was a less invasive proposal, not as bad as my original V1.

Unfortunately, during my tests another bug appeared: it turns out
writing to the load register has not really an immediate effect, so
all of V1 V2 and V3 were all fixing the bigger bug but not very reliable on
the field.

This is a respin of V3 that also deals with the unexpected hardware latency.


 cpu/arm926ejs/nomadik/timer.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/cpu/arm926ejs/nomadik/timer.c b/cpu/arm926ejs/nomadik/timer.c
index 16067c9..544d438 100644
--- a/cpu/arm926ejs/nomadik/timer.c
+++ b/cpu/arm926ejs/nomadik/timer.c
@@ -34,8 +34,8 @@
 #define TICKS_PER_HZ		(TIMER_CLOCK / CONFIG_SYS_HZ)
 #define TICKS_TO_HZ(x)		((x) / TICKS_PER_HZ)
 
-/* macro to read the 32 bit timer: since it decrements, we invert read value */
-#define READ_TIMER() (~readl(CONFIG_SYS_TIMERBASE + MTU_VAL(0)))
+/* macro to read the decrementing 32 bit timer as an increasing count */
+#define READ_TIMER() (0 - readl(CONFIG_SYS_TIMERBASE + MTU_VAL(0)))
 
 /* Configure a free-running, auto-wrap counter with no prescaler */
 int timer_init(void)
@@ -49,7 +49,16 @@ int timer_init(void)
 /* Restart counting from 0 */
 void reset_timer(void)
 {
-	writel(0, CONFIG_SYS_TIMERBASE + MTU_LR(0)); /* Immediate effect */
+	ulong val;
+	writel(0, CONFIG_SYS_TIMERBASE + MTU_LR(0));
+	/*
+	 * The load-register isn't really immediate: it changes on clock
+	 * edges, so we must wait for our newly-written value to appear.
+	 * Since we might miss reading 0, wait for any change in value.
+	 */
+	val = READ_TIMER();
+	while (READ_TIMER() == val)
+		;
 }
 
 /* Return how many HZ passed since "base" */
-- 
1.6.0.2

[U-Boot] [PATCH V4] Nomadik: fix reset_timer()

[Tom Rix]

Alessandro Rubini wrote:
> From: Alessandro Rubini <rubini@unipv.it>
> 
> Previous code was failing when reading back the timer less than
> 400us after resetting it. This lead nand operations to incorrectly
> timeout any now and then.  Moreover, writing the load register isn't
> immediately reflected in the value register. We must wait for a clock
> edge, so read_timer now waits for the value to change at least once,
> otherwise nand operation would timeout anyways (though less frequently).
> 
> 
> Signed-off-by: Alessandro Rubini <rubini@unipv.it>
> Acked-by: Andrea Gallo <andrea.gallo@stericsson.com>
> ---
> 
> I finally managed to test on hardware my V2 and V3, posted on
> 2009-11-06 after a compile check. V2 was the denk-suggested way, while
> V3 was a less invasive proposal, not as bad as my original V1.
> 
> Unfortunately, during my tests another bug appeared: it turns out
> writing to the load register has not really an immediate effect, so
> all of V1 V2 and V3 were all fixing the bigger bug but not very reliable on
> the field.
> 
> This is a respin of V3 that also deals with the unexpected hardware latency.
> 
> 
>  cpu/arm926ejs/nomadik/timer.c |   15 ++++++++++++---
>  1 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/cpu/arm926ejs/nomadik/timer.c b/cpu/arm926ejs/nomadik/timer.c
> index 16067c9..544d438 100644
> --- a/cpu/arm926ejs/nomadik/timer.c
> +++ b/cpu/arm926ejs/nomadik/timer.c
> @@ -34,8 +34,8 @@
>  #define TICKS_PER_HZ		(TIMER_CLOCK / CONFIG_SYS_HZ)
>  #define TICKS_TO_HZ(x)		((x) / TICKS_PER_HZ)
>  
> -/* macro to read the 32 bit timer: since it decrements, we invert read value */
> -#define READ_TIMER() (~readl(CONFIG_SYS_TIMERBASE + MTU_VAL(0)))
> +/* macro to read the decrementing 32 bit timer as an increasing count */
> +#define READ_TIMER() (0 - readl(CONFIG_SYS_TIMERBASE + MTU_VAL(0)))
>  
>  /* Configure a free-running, auto-wrap counter with no prescaler */
>  int timer_init(void)
> @@ -49,7 +49,16 @@ int timer_init(void)
>  /* Restart counting from 0 */
>  void reset_timer(void)
>  {
> -	writel(0, CONFIG_SYS_TIMERBASE + MTU_LR(0)); /* Immediate effect */
> +	ulong val;
> +	writel(0, CONFIG_SYS_TIMERBASE + MTU_LR(0));
> +	/*
> +	 * The load-register isn't really immediate: it changes on clock
> +	 * edges, so we must wait for our newly-written value to appear.
> +	 * Since we might miss reading 0, wait for any change in value.
> +	 */
> +	val = READ_TIMER();
> +	while (READ_TIMER() == val)
> +		;
>  }
>  
>  /* Return how many HZ passed since "base" */
Applied to arm/next
Sorry for the delay.
Tom