[U-Boot] [PATCH] arm925t: Fix CONFIG_SYS_HZ to 1000

[U-Boot] [PATCH] arm925t: Fix CONFIG_SYS_HZ to 1000

[Dirk Behme]

Dear Ladis,

Ladislav Michl wrote:
> Let CONFIG_SYS_HZ to have value of 1000 effectively fixing all users of
> get_timer.

In the other mail you mentioned OMAP1, which would include the ARM926 
based devices (e.g. OSK), too. Any special reason why in here you deal 
only with the 925 based ones? The ARM926 based OMAP1s seem to have the 
same issue?

Or do you already prepare a 926 patch, too, and I'm just too fast with 
asking? ;)

Best regards

Dirk

> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> 
> diff --git a/cpu/arm925t/interrupts.c b/cpu/arm925t/interrupts.c
> index e5c77f7..a22be66 100644
> --- a/cpu/arm925t/interrupts.c
> +++ b/cpu/arm925t/interrupts.c
> @@ -1,4 +1,7 @@
>  /*
> + * (C) Copyright 2009
> + * 2N Telekomunikace, <www.2n.cz>
> + *
>   * (C) Copyright 2003
>   * Texas Instruments, <www.ti.com>
>   *
> @@ -37,7 +40,8 @@
>  #include <configs/omap1510.h>
>  #include <asm/io.h>
>  
> -#define TIMER_LOAD_VAL 0xffffffff
> +#define TIMER_LOAD_VAL	0xffffffff
> +#define TIMER_CLOCK	(CONFIG_SYS_CLK_FREQ / (2 << CONFIG_SYS_PTV))
>  
>  static uint32_t timestamp;
>  static uint32_t lastdec;
> @@ -76,88 +80,44 @@ void set_timer (ulong t)
>  	timestamp = t;
>  }
>  
> -/* delay x useconds AND preserve advance timestamp value */
> +/* delay usec microseconds preserving timestamp value */
>  void udelay (unsigned long usec)
>  {
> -	ulong tmo, tmp;
> -
> -	if (usec >= 1000) {		/* if "big" number, spread normalization to seconds */
> -		tmo = usec / 1000;	/* start to normalize for usec to ticks per sec */
> -		tmo *= CONFIG_SYS_HZ;	/* find number of "ticks" to wait to achieve target */
> -		tmo /= 1000;		/* finish normalize. */
> -	} else {			/* else small number, don't kill it prior to HZ multiply */
> -		tmo = usec * CONFIG_SYS_HZ;
> -		tmo /= (1000*1000);
> +	int32_t tmo = usec * (TIMER_CLOCK / 1000) / 1000;
> +	uint32_t now, last = __raw_readl(CONFIG_SYS_TIMERBASE + READ_TIM);
> +
> +	while (tmo > 0) {
> +		now = __raw_readl(CONFIG_SYS_TIMERBASE + READ_TIM);
> +		if (last < now) /* count down timer underflow */
> +			tmo -= TIMER_LOAD_VAL - now + last;
> +		else
> +			tmo -= last - now;
> +		last = now;
>  	}
> -
> -	tmp = get_timer (0);		/* get current timestamp */
> -	if ((tmo + tmp + 1) < tmp)	/* if setting this fordward will roll time stamp */
> -		reset_timer_masked ();	/* reset "advancing" timestamp to 0, set lastdec value */
> -	else
> -		tmo += tmp;		/* else, set advancing stamp wake up time */
> -
> -	while (get_timer_masked () < tmo) /* loop till event */
> -		/*NOP*/;
>  }
>  
>  void reset_timer_masked (void)
>  {
>  	/* reset time */
> -	lastdec = __raw_readl(CONFIG_SYS_TIMERBASE + READ_TIM);
> +	lastdec = __raw_readl(CONFIG_SYS_TIMERBASE + READ_TIM) /
> +			(TIMER_CLOCK / CONFIG_SYS_HZ);
>  	timestamp = 0;	       /* start "advancing" time stamp from 0 */
>  }
>  
>  ulong get_timer_masked (void)
>  {
> -	uint32_t now = __raw_readl(CONFIG_SYS_TIMERBASE + READ_TIM);
> -
> -	if (lastdec >= now) {		/* normal mode (non roll) */
> -		/* normal mode */
> -		timestamp += lastdec - now; /* move stamp fordward with absoulte diff ticks */
> -	} else {			/* we have overflow of the count down timer */
> -		/* nts = ts + ld + (TLV - now)
> -		 * ts=old stamp, ld=time that passed before passing through -1
> -		 * (TLV-now) amount of time after passing though -1
> -		 * nts = new "advancing time stamp"...it could also roll and cause problems.
> -		 */
> -		timestamp += lastdec + TIMER_LOAD_VAL - now;
> -	}
> +	uint32_t now = __raw_readl(CONFIG_SYS_TIMERBASE + READ_TIM) /
> +			(TIMER_CLOCK / CONFIG_SYS_HZ);
> +	if (lastdec < now)	/* count down timer underflow */
> +		timestamp += TIMER_LOAD_VAL / (TIMER_CLOCK / CONFIG_SYS_HZ) -
> +				now + lastdec;
> +	else
> +		timestamp += lastdec - now;
>  	lastdec = now;
>  
>  	return timestamp;
>  }
>  
> -/* waits specified delay value and resets timestamp */
> -void udelay_masked (unsigned long usec)
> -{
> -#ifdef CONFIG_INNOVATOROMAP1510
> -	#define LOOPS_PER_MSEC 60 /* tuned on omap1510 */
> -	volatile int i, time_remaining = LOOPS_PER_MSEC*usec;
> -	for (i=time_remaining; i>0; i--) { }
> -#else
> -
> -	ulong tmo;
> -	ulong endtime;
> -	signed long diff;
> -
> -	if (usec >= 1000) {		/* if "big" number, spread normalization to seconds */
> -		tmo = usec / 1000;	/* start to normalize for usec to ticks per sec */
> -		tmo *= CONFIG_SYS_HZ;	/* find number of "ticks" to wait to achieve target */
> -		tmo /= 1000;		/* finish normalize. */
> -	} else {			/* else small number, don't kill it prior to HZ multiply */
> -		tmo = usec * CONFIG_SYS_HZ;
> -		tmo /= (1000*1000);
> -	}
> -
> -	endtime = get_timer_masked () + tmo;
> -
> -	do {
> -		ulong now = get_timer_masked ();
> -		diff = endtime - now;
> -	} while (diff >= 0);
> -#endif
> -}
> -
>  /*
>   * This function is derived from PowerPC code (read timebase as long long).
>   * On ARM it just returns the timer value.
> diff --git a/include/configs/SX1.h b/include/configs/SX1.h
> index caa6592..9313c7b 100644
> --- a/include/configs/SX1.h
> +++ b/include/configs/SX1.h
> @@ -135,12 +135,12 @@
>  
>  #define CONFIG_SYS_LOAD_ADDR		0x10000000	/* default load address */
>  
> -/* The 1510 has 3 timers, they can be driven by the RefClk (12Mhz) or by DPLL1.
> +/* The 1510 has 3 timers, they can be driven by the RefClk (12MHz) or by DPLL1.
>   * This time is further subdivided by a local divisor.
>   */
>  #define CONFIG_SYS_TIMERBASE	OMAP1510_TIMER1_BASE	/* use timer 1 */
>  #define CONFIG_SYS_PTV		7	/* 2^(PTV+1), divide by 256 */
> -#define CONFIG_SYS_HZ		((CONFIG_SYS_CLK_FREQ)/(2 << CONFIG_SYS_PTV))
> +#define CONFIG_SYS_HZ		1000
>  
>  /*-----------------------------------------------------------------------
>   * Stack sizes
> diff --git a/include/configs/netstar.h b/include/configs/netstar.h
> index 39560de..5cfee66 100644
> --- a/include/configs/netstar.h
> +++ b/include/configs/netstar.h
> @@ -222,12 +222,12 @@
>  
>  #define CONFIG_SYS_LOAD_ADDR		PHYS_SDRAM_1 + 0x400000	/* default load address */
>  
> -/* The 1510 has 3 timers, they can be driven by the RefClk (12Mhz) or by DPLL1.
> +/* The 1510 has 3 timers, they can be driven by the RefClk (12MHz) or by DPLL1.
>   * This time is further subdivided by a local divisor.
>   */
>  #define CONFIG_SYS_TIMERBASE		OMAP1510_TIMER1_BASE
> -#define CONFIG_SYS_PTV			7		/* 2^(pvt+1), divide by 256 */
> -#define CONFIG_SYS_HZ			((CONFIG_SYS_CLK_FREQ)/(2 << CONFIG_SYS_PTV))
> +#define CONFIG_SYS_PTV			7
> +#define CONFIG_SYS_HZ			1000
>  
>  #define OMAP5910_DPLL_DIV	1
>  #define OMAP5910_DPLL_MUL	((CONFIG_SYS_CLK_FREQ * \
> diff --git a/include/configs/omap1510inn.h b/include/configs/omap1510inn.h
> index 6c1c5ec..def85c1 100644
> --- a/include/configs/omap1510inn.h
> +++ b/include/configs/omap1510inn.h
> @@ -132,12 +132,12 @@
>  
>  #define CONFIG_SYS_LOAD_ADDR		0x10000000	/* default load address */
>  
> -/* The 1510 has 3 timers, they can be driven by the RefClk (12Mhz) or by DPLL1.
> +/* The 1510 has 3 timers, they can be driven by the RefClk (12MHz) or by DPLL1.
>   * This time is further subdivided by a local divisor.
>   */
>  #define CONFIG_SYS_TIMERBASE	OMAP1510_TIMER1_BASE	/* use timer 1 */
>  #define CONFIG_SYS_PTV		7	/* 2^(PTV+1), divide by 256 */
> -#define CONFIG_SYS_HZ		((CONFIG_SYS_CLK_FREQ)/(2 << CONFIG_SYS_PTV))
> +#define CONFIG_SYS_HZ		1000
>  
>  /*-----------------------------------------------------------------------
>   * Stack sizes
> diff --git a/include/configs/voiceblue.h b/include/configs/voiceblue.h
> index 3f97843..aa8efaa 100644
> --- a/include/configs/voiceblue.h
> +++ b/include/configs/voiceblue.h
> @@ -210,12 +210,12 @@
>  #define CONFIG_SYS_MEMTEST_START	PHYS_SDRAM_1
>  #define CONFIG_SYS_MEMTEST_END		PHYS_SDRAM_1 + PHYS_SDRAM_1_SIZE - PHYS_SDRAM_1_RESERVED
>  
> -/* The 1510 has 3 timers, they can be driven by the RefClk (12Mhz) or by DPLL1.
> +/* The 1510 has 3 timers, they can be driven by the RefClk (12MHz) or by DPLL1.
>   * This time is further subdivided by a local divisor.
>   */
>  #define CONFIG_SYS_TIMERBASE	OMAP1510_TIMER1_BASE
>  #define CONFIG_SYS_PTV		7	/* 2^(PTV+1), divide by 256 */
> -#define CONFIG_SYS_HZ		((CONFIG_SYS_CLK_FREQ)/(2 << CONFIG_SYS_PTV))
> +#define CONFIG_SYS_HZ		1000
>  
>  #define OMAP5910_DPLL_DIV	1
>  #define OMAP5910_DPLL_MUL	((CONFIG_SYS_CLK_FREQ * \
> 

[U-Boot] [PATCH] arm925t: Fix CONFIG_SYS_HZ to 1000

[Dirk Behme]

Dear Ladis,

ah, and some remarks on the patch itself ;)

Ladislav Michl wrote:
> Let CONFIG_SYS_HZ to have value of 1000 effectively fixing all users of
> get_timer.
> 
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> 
> diff --git a/cpu/arm925t/interrupts.c b/cpu/arm925t/interrupts.c
> index e5c77f7..a22be66 100644
> --- a/cpu/arm925t/interrupts.c
> +++ b/cpu/arm925t/interrupts.c
...
> -#define TIMER_LOAD_VAL 0xffffffff
> +#define TIMER_LOAD_VAL	0xffffffff
> +#define TIMER_CLOCK	(CONFIG_SYS_CLK_FREQ / (2 << CONFIG_SYS_PTV))

Just to get an idea of the math:

CONFIG_SYS_CLK_FREQ is 12000000 (12MHz)? This is divided by 256, so 
TIMER_CLOCK is 46875Hz? A free running 32-bit count down timer is used 
starting@0xffffffff? Underflow (0) is reached after ~91626s == 
~25hours with this?

Please correct if something is wrong ;)

> -/* delay x useconds AND preserve advance timestamp value */
> +/* delay usec microseconds preserving timestamp value */

Hmm, 'usec microseconds' sounds somehow confusing?

>  void udelay (unsigned long usec)
>  {
...
> +	int32_t tmo = usec * (TIMER_CLOCK / 1000) / 1000;
> +	uint32_t now, last = __raw_readl(CONFIG_SYS_TIMERBASE + READ_TIM);

The first '1000' should be CONFIG_SYS_HZ? I.e.

(TIMER_CLOCK / CONFIG_SYS_HZ) / 1000;

?

In my udelay patch, I use

+	tmo = usec * (TIMER_CLOCK / CONFIG_SYS_HZ);
+	tmo /= 1000;

 From some printf debugging, for OMAP3 there was a difference doing it 
in one or two lines. If I remember correctly due to integer vs 
floating point math and rounding. What do you think?

Running OMAP3 counter with 1.625MHz, max udelay resolution is ~1.62us. 
If you run with 46875Hz, you have max udelay resolution of ~22us?

> +	while (tmo > 0) {
> +		now = __raw_readl(CONFIG_SYS_TIMERBASE + READ_TIM);
> +		if (last < now) /* count down timer underflow */
> +			tmo -= TIMER_LOAD_VAL - now + last;
> +		else
> +			tmo -= last - now;
> +		last = now;

I will think about this, I always need some time for this clock math ;)

In contrast to OMAP3 your timer here counts down, right? So while 
OMAP1 has to deal with underflow, OMAP3 will need overflow handling, 
right?

Best regards

Dirk

[U-Boot] [PATCH] arm925t: Fix CONFIG_SYS_HZ to 1000

[Ladislav Michl]

Let CONFIG_SYS_HZ to have value of 1000 effectively fixing all users of
get_timer.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>

diff --git a/cpu/arm925t/interrupts.c b/cpu/arm925t/interrupts.c
index e5c77f7..a22be66 100644
--- a/cpu/arm925t/interrupts.c
+++ b/cpu/arm925t/interrupts.c
@@ -1,4 +1,7 @@
 /*
+ * (C) Copyright 2009
+ * 2N Telekomunikace, <www.2n.cz>
+ *
  * (C) Copyright 2003
  * Texas Instruments, <www.ti.com>
  *
@@ -37,7 +40,8 @@
 #include <configs/omap1510.h>
 #include <asm/io.h>
 
-#define TIMER_LOAD_VAL 0xffffffff
+#define TIMER_LOAD_VAL	0xffffffff
+#define TIMER_CLOCK	(CONFIG_SYS_CLK_FREQ / (2 << CONFIG_SYS_PTV))
 
 static uint32_t timestamp;
 static uint32_t lastdec;
@@ -76,88 +80,44 @@ void set_timer (ulong t)
 	timestamp = t;
 }
 
-/* delay x useconds AND preserve advance timestamp value */
+/* delay usec microseconds preserving timestamp value */
 void udelay (unsigned long usec)
 {
-	ulong tmo, tmp;
-
-	if (usec >= 1000) {		/* if "big" number, spread normalization to seconds */
-		tmo = usec / 1000;	/* start to normalize for usec to ticks per sec */
-		tmo *= CONFIG_SYS_HZ;	/* find number of "ticks" to wait to achieve target */
-		tmo /= 1000;		/* finish normalize. */
-	} else {			/* else small number, don't kill it prior to HZ multiply */
-		tmo = usec * CONFIG_SYS_HZ;
-		tmo /= (1000*1000);
+	int32_t tmo = usec * (TIMER_CLOCK / 1000) / 1000;
+	uint32_t now, last = __raw_readl(CONFIG_SYS_TIMERBASE + READ_TIM);
+
+	while (tmo > 0) {
+		now = __raw_readl(CONFIG_SYS_TIMERBASE + READ_TIM);
+		if (last < now) /* count down timer underflow */
+			tmo -= TIMER_LOAD_VAL - now + last;
+		else
+			tmo -= last - now;
+		last = now;
 	}
-
-	tmp = get_timer (0);		/* get current timestamp */
-	if ((tmo + tmp + 1) < tmp)	/* if setting this fordward will roll time stamp */
-		reset_timer_masked ();	/* reset "advancing" timestamp to 0, set lastdec value */
-	else
-		tmo += tmp;		/* else, set advancing stamp wake up time */
-
-	while (get_timer_masked () < tmo) /* loop till event */
-		/*NOP*/;
 }
 
 void reset_timer_masked (void)
 {
 	/* reset time */
-	lastdec = __raw_readl(CONFIG_SYS_TIMERBASE + READ_TIM);
+	lastdec = __raw_readl(CONFIG_SYS_TIMERBASE + READ_TIM) /
+			(TIMER_CLOCK / CONFIG_SYS_HZ);
 	timestamp = 0;	       /* start "advancing" time stamp from 0 */
 }
 
 ulong get_timer_masked (void)
 {
-	uint32_t now = __raw_readl(CONFIG_SYS_TIMERBASE + READ_TIM);
-
-	if (lastdec >= now) {		/* normal mode (non roll) */
-		/* normal mode */
-		timestamp += lastdec - now; /* move stamp fordward with absoulte diff ticks */
-	} else {			/* we have overflow of the count down timer */
-		/* nts = ts + ld + (TLV - now)
-		 * ts=old stamp, ld=time that passed before passing through -1
-		 * (TLV-now) amount of time after passing though -1
-		 * nts = new "advancing time stamp"...it could also roll and cause problems.
-		 */
-		timestamp += lastdec + TIMER_LOAD_VAL - now;
-	}
+	uint32_t now = __raw_readl(CONFIG_SYS_TIMERBASE + READ_TIM) /
+			(TIMER_CLOCK / CONFIG_SYS_HZ);
+	if (lastdec < now)	/* count down timer underflow */
+		timestamp += TIMER_LOAD_VAL / (TIMER_CLOCK / CONFIG_SYS_HZ) -
+				now + lastdec;
+	else
+		timestamp += lastdec - now;
 	lastdec = now;
 
 	return timestamp;
 }
 
-/* waits specified delay value and resets timestamp */
-void udelay_masked (unsigned long usec)
-{
-#ifdef CONFIG_INNOVATOROMAP1510
-	#define LOOPS_PER_MSEC 60 /* tuned on omap1510 */
-	volatile int i, time_remaining = LOOPS_PER_MSEC*usec;
-	for (i=time_remaining; i>0; i--) { }
-#else
-
-	ulong tmo;
-	ulong endtime;
-	signed long diff;
-
-	if (usec >= 1000) {		/* if "big" number, spread normalization to seconds */
-		tmo = usec / 1000;	/* start to normalize for usec to ticks per sec */
-		tmo *= CONFIG_SYS_HZ;	/* find number of "ticks" to wait to achieve target */
-		tmo /= 1000;		/* finish normalize. */
-	} else {			/* else small number, don't kill it prior to HZ multiply */
-		tmo = usec * CONFIG_SYS_HZ;
-		tmo /= (1000*1000);
-	}
-
-	endtime = get_timer_masked () + tmo;
-
-	do {
-		ulong now = get_timer_masked ();
-		diff = endtime - now;
-	} while (diff >= 0);
-#endif
-}
-
 /*
  * This function is derived from PowerPC code (read timebase as long long).
  * On ARM it just returns the timer value.
diff --git a/include/configs/SX1.h b/include/configs/SX1.h
index caa6592..9313c7b 100644
--- a/include/configs/SX1.h
+++ b/include/configs/SX1.h
@@ -135,12 +135,12 @@
 
 #define CONFIG_SYS_LOAD_ADDR		0x10000000	/* default load address */
 
-/* The 1510 has 3 timers, they can be driven by the RefClk (12Mhz) or by DPLL1.
+/* The 1510 has 3 timers, they can be driven by the RefClk (12MHz) or by DPLL1.
  * This time is further subdivided by a local divisor.
  */
 #define CONFIG_SYS_TIMERBASE	OMAP1510_TIMER1_BASE	/* use timer 1 */
 #define CONFIG_SYS_PTV		7	/* 2^(PTV+1), divide by 256 */
-#define CONFIG_SYS_HZ		((CONFIG_SYS_CLK_FREQ)/(2 << CONFIG_SYS_PTV))
+#define CONFIG_SYS_HZ		1000
 
 /*-----------------------------------------------------------------------
  * Stack sizes
diff --git a/include/configs/netstar.h b/include/configs/netstar.h
index 39560de..5cfee66 100644
--- a/include/configs/netstar.h
+++ b/include/configs/netstar.h
@@ -222,12 +222,12 @@
 
 #define CONFIG_SYS_LOAD_ADDR		PHYS_SDRAM_1 + 0x400000	/* default load address */
 
-/* The 1510 has 3 timers, they can be driven by the RefClk (12Mhz) or by DPLL1.
+/* The 1510 has 3 timers, they can be driven by the RefClk (12MHz) or by DPLL1.
  * This time is further subdivided by a local divisor.
  */
 #define CONFIG_SYS_TIMERBASE		OMAP1510_TIMER1_BASE
-#define CONFIG_SYS_PTV			7		/* 2^(pvt+1), divide by 256 */
-#define CONFIG_SYS_HZ			((CONFIG_SYS_CLK_FREQ)/(2 << CONFIG_SYS_PTV))
+#define CONFIG_SYS_PTV			7
+#define CONFIG_SYS_HZ			1000
 
 #define OMAP5910_DPLL_DIV	1
 #define OMAP5910_DPLL_MUL	((CONFIG_SYS_CLK_FREQ * \
diff --git a/include/configs/omap1510inn.h b/include/configs/omap1510inn.h
index 6c1c5ec..def85c1 100644
--- a/include/configs/omap1510inn.h
+++ b/include/configs/omap1510inn.h
@@ -132,12 +132,12 @@
 
 #define CONFIG_SYS_LOAD_ADDR		0x10000000	/* default load address */
 
-/* The 1510 has 3 timers, they can be driven by the RefClk (12Mhz) or by DPLL1.
+/* The 1510 has 3 timers, they can be driven by the RefClk (12MHz) or by DPLL1.
  * This time is further subdivided by a local divisor.
  */
 #define CONFIG_SYS_TIMERBASE	OMAP1510_TIMER1_BASE	/* use timer 1 */
 #define CONFIG_SYS_PTV		7	/* 2^(PTV+1), divide by 256 */
-#define CONFIG_SYS_HZ		((CONFIG_SYS_CLK_FREQ)/(2 << CONFIG_SYS_PTV))
+#define CONFIG_SYS_HZ		1000
 
 /*-----------------------------------------------------------------------
  * Stack sizes
diff --git a/include/configs/voiceblue.h b/include/configs/voiceblue.h
index 3f97843..aa8efaa 100644
--- a/include/configs/voiceblue.h
+++ b/include/configs/voiceblue.h
@@ -210,12 +210,12 @@
 #define CONFIG_SYS_MEMTEST_START	PHYS_SDRAM_1
 #define CONFIG_SYS_MEMTEST_END		PHYS_SDRAM_1 + PHYS_SDRAM_1_SIZE - PHYS_SDRAM_1_RESERVED
 
-/* The 1510 has 3 timers, they can be driven by the RefClk (12Mhz) or by DPLL1.
+/* The 1510 has 3 timers, they can be driven by the RefClk (12MHz) or by DPLL1.
  * This time is further subdivided by a local divisor.
  */
 #define CONFIG_SYS_TIMERBASE	OMAP1510_TIMER1_BASE
 #define CONFIG_SYS_PTV		7	/* 2^(PTV+1), divide by 256 */
-#define CONFIG_SYS_HZ		((CONFIG_SYS_CLK_FREQ)/(2 << CONFIG_SYS_PTV))
+#define CONFIG_SYS_HZ		1000
 
 #define OMAP5910_DPLL_DIV	1
 #define OMAP5910_DPLL_MUL	((CONFIG_SYS_CLK_FREQ * \

[U-Boot] [PATCH] arm925t: Fix CONFIG_SYS_HZ to 1000

[Ladislav Michl]

On Mon, Apr 20, 2009 at 08:27:34PM +0200, Dirk Behme wrote:
> Dear Ladis,
>
> ah, and some remarks on the patch itself ;)

Thanks, I'm glad someone still cares about ancient stuff.

> Ladislav Michl wrote:
>> Let CONFIG_SYS_HZ to have value of 1000 effectively fixing all users of
>> get_timer.
>>
>> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
>>
>> diff --git a/cpu/arm925t/interrupts.c b/cpu/arm925t/interrupts.c
>> index e5c77f7..a22be66 100644
>> --- a/cpu/arm925t/interrupts.c
>> +++ b/cpu/arm925t/interrupts.c
> ...
>> -#define TIMER_LOAD_VAL 0xffffffff
>> +#define TIMER_LOAD_VAL	0xffffffff
>> +#define TIMER_CLOCK	(CONFIG_SYS_CLK_FREQ / (2 << CONFIG_SYS_PTV))
>
> Just to get an idea of the math:
>
> CONFIG_SYS_CLK_FREQ is 12000000 (12MHz)? This is divided by 256, so  
> TIMER_CLOCK is 46875Hz? A free running 32-bit count down timer is used  
> starting at 0xffffffff? Underflow (0) is reached after ~91626s ==  
> ~25hours with this?
>
> Please correct if something is wrong ;)

Math is perfectly correct, except in my case CONFIG_SYS_CLK_FREQ is 150MHz,
so resolution is actually 12.5 times better. Perhaps I should modify those
boards wich uses 12MHz clock to use smaller divisor, but let's wait for more
comments first.

>> -/* delay x useconds AND preserve advance timestamp value */
>> +/* delay usec microseconds preserving timestamp value */
>
> Hmm, 'usec microseconds' sounds somehow confusing?

It depends. 'usec' is obviously variable name and 'microsecond' is a time
unit, while 'x' is unreferenced variable and 'usec' is abbreviation.
And I prefer former (or deleting that part of comment entirely).

>>  void udelay (unsigned long usec)
>>  {
> ...
>> +	int32_t tmo = usec * (TIMER_CLOCK / 1000) / 1000;
>> +	uint32_t now, last = __raw_readl(CONFIG_SYS_TIMERBASE + READ_TIM);
>
> The first '1000' should be CONFIG_SYS_HZ? I.e.

No. Actually it should read 'usec * (TIMER_CLOCK / (1000 * 1000))', where
one '1000' is to get miliseconds and other brings you to microseconds
digit place. But integer math needs former writing.

> (TIMER_CLOCK / CONFIG_SYS_HZ) / 1000;
>
> ?
>
> In my udelay patch, I use
>
> +	tmo = usec * (TIMER_CLOCK / CONFIG_SYS_HZ);
> +	tmo /= 1000;
>
> From some printf debugging, for OMAP3 there was a difference doing it in 
> one or two lines. If I remember correctly due to integer vs floating 
> point math and rounding. What do you think?

I think all that udelay code is pointless once CONFIG_SYS_HZ always
_have_ to be 1000 and can be simplyfied.

> Running OMAP3 counter with 1.625MHz, max udelay resolution is ~1.62us.  
> If you run with 46875Hz, you have max udelay resolution of ~22us?

See above, it is ~1.7us.

>> +	while (tmo > 0) {
>> +		now = __raw_readl(CONFIG_SYS_TIMERBASE + READ_TIM);
>> +		if (last < now) /* count down timer underflow */
>> +			tmo -= TIMER_LOAD_VAL - now + last;
>> +		else
>> +			tmo -= last - now;
>> +		last = now;
>
> I will think about this, I always need some time for this clock math ;)
>
> In contrast to OMAP3 your timer here counts down, right? So while OMAP1 
> has to deal with underflow, OMAP3 will need overflow handling, right?

Right, but the key point here is to unbind udelay from get_timer as now
get_timer works with miliseconds resolution.

	ladis

[U-Boot] [PATCH] arm925t: Fix CONFIG_SYS_HZ to 1000

[Ladislav Michl]

On Mon, Apr 20, 2009 at 07:38:21PM +0200, Dirk Behme wrote:
> Dear Ladis,
>
> Ladislav Michl wrote:
>> Let CONFIG_SYS_HZ to have value of 1000 effectively fixing all users of
>> get_timer.
>
> In the other mail you mentioned OMAP1, which would include the ARM926  
> based devices (e.g. OSK), too. Any special reason why in here you deal  
> only with the 925 based ones? The ARM926 based OMAP1s seem to have the  
> same issue?

Yes, the very same, but I do not have hardware to test with, but see bellow.

> Or do you already prepare a 926 patch, too, and I'm just too fast with  
> asking? ;)

Given the fact that cpu/arm926ejs/omap/timer.c was once upon a time a copy
of cpu/arm925t/interrupts.c (or vice versa, I'm lazy to check), it should
be pretty easy to repeat this copy operation one more time, to get at least
some solution, before we get shinny new timer code. I didn't do it myself
in a hope that someone with hardware will give it a try...

	ladis

[U-Boot] [PATCH] arm925t: Fix CONFIG_SYS_HZ to 1000

[Dirk Behme]

Dear Ladis,

Ladislav Michl wrote:
> On Mon, Apr 20, 2009 at 08:27:34PM +0200, Dirk Behme wrote:
>> Dear Ladis,
>>
>> ah, and some remarks on the patch itself ;)
> 
> Thanks, I'm glad someone still cares about ancient stuff.
> 
>> Ladislav Michl wrote:
>>> Let CONFIG_SYS_HZ to have value of 1000 effectively fixing all users of
>>> get_timer.
>>>
>>> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
>>>
>>> diff --git a/cpu/arm925t/interrupts.c b/cpu/arm925t/interrupts.c
>>> index e5c77f7..a22be66 100644
>>> --- a/cpu/arm925t/interrupts.c
>>> +++ b/cpu/arm925t/interrupts.c
>> ...
>>> -#define TIMER_LOAD_VAL 0xffffffff
>>> +#define TIMER_LOAD_VAL	0xffffffff
>>> +#define TIMER_CLOCK	(CONFIG_SYS_CLK_FREQ / (2 << CONFIG_SYS_PTV))
>> Just to get an idea of the math:
>>
>> CONFIG_SYS_CLK_FREQ is 12000000 (12MHz)? This is divided by 256, so  
>> TIMER_CLOCK is 46875Hz? A free running 32-bit count down timer is used  
>> starting at 0xffffffff? Underflow (0) is reached after ~91626s ==  
>> ~25hours with this?
>>
>> Please correct if something is wrong ;)
> 
> Math is perfectly correct, except in my case CONFIG_SYS_CLK_FREQ is 150MHz,
> so resolution is actually 12.5 times better. 

Ok. Is this 150MHz defined in one of the configs you modify with this 
patch or do you use a custom config? Just curious ;)

> Perhaps I should modify those
> boards wich uses 12MHz clock to use smaller divisor, 

Yes, this should be easily doable by changing CONFIG_SYS_PTV.

> but let's wait for more
> comments first.

I hope there will be some ;)

>>> -/* delay x useconds AND preserve advance timestamp value */
>>> +/* delay usec microseconds preserving timestamp value */
>> Hmm, 'usec microseconds' sounds somehow confusing?
> 
> It depends. 'usec' is obviously variable name and 'microsecond' is a time
> unit, while 'x' is unreferenced variable and 'usec' is abbreviation.
> And I prefer former (or deleting that part of comment entirely).
> 
>>>  void udelay (unsigned long usec)
>>>  {
>> ...
>>> +	int32_t tmo = usec * (TIMER_CLOCK / 1000) / 1000;
>>> +	uint32_t now, last = __raw_readl(CONFIG_SYS_TIMERBASE + READ_TIM);
>> The first '1000' should be CONFIG_SYS_HZ? I.e.
> 
> No. Actually it should read 'usec * (TIMER_CLOCK / (1000 * 1000))', where
> one '1000' is to get miliseconds and other brings you to microseconds
> digit place. But integer math needs former writing.

Ok, understood. Thanks for the hint!

>> (TIMER_CLOCK / CONFIG_SYS_HZ) / 1000;
>>
>> ?
>>
>> In my udelay patch, I use
>>
>> +	tmo = usec * (TIMER_CLOCK / CONFIG_SYS_HZ);
>> +	tmo /= 1000;
>>
>> From some printf debugging, for OMAP3 there was a difference doing it in 
>> one or two lines. If I remember correctly due to integer vs floating 
>> point math and rounding. What do you think?
> 
> I think all that udelay code is pointless once CONFIG_SYS_HZ always
> _have_ to be 1000 and can be simplyfied.
> 
>> Running OMAP3 counter with 1.625MHz, max udelay resolution is ~1.62us.  
>> If you run with 46875Hz, you have max udelay resolution of ~22us?
> 
> See above, it is ~1.7us.
> 
>>> +	while (tmo > 0) {
>>> +		now = __raw_readl(CONFIG_SYS_TIMERBASE + READ_TIM);
>>> +		if (last < now) /* count down timer underflow */
>>> +			tmo -= TIMER_LOAD_VAL - now + last;
>>> +		else
>>> +			tmo -= last - now;
>>> +		last = now;
>> I will think about this, I always need some time for this clock math ;)
>>
>> In contrast to OMAP3 your timer here counts down, right? So while OMAP1 
>> has to deal with underflow, OMAP3 will need overflow handling, right?
> 
> Right, but the key point here is to unbind udelay from get_timer as now
> get_timer works with miliseconds resolution.

I hope I got it right with the updated patch sent some minutes ago.

Best regards

Dirk

[U-Boot] [PATCH] arm925t: Fix CONFIG_SYS_HZ to 1000

[Ladislav Michl]

On Tue, Apr 21, 2009 at 05:38:21PM +0200, Dirk Behme wrote:
> Ladislav Michl wrote:
>> On Mon, Apr 20, 2009 at 08:27:34PM +0200, Dirk Behme wrote:
>>> Just to get an idea of the math:
>>>
>>> CONFIG_SYS_CLK_FREQ is 12000000 (12MHz)? This is divided by 256, so   
>>> TIMER_CLOCK is 46875Hz? A free running 32-bit count down timer is 
>>> used  starting at 0xffffffff? Underflow (0) is reached after ~91626s 
>>> ==  ~25hours with this?
>>>
>>> Please correct if something is wrong ;)
>>
>> Math is perfectly correct, except in my case CONFIG_SYS_CLK_FREQ is 150MHz,
>> so resolution is actually 12.5 times better. 
>
> Ok. Is this 150MHz defined in one of the configs you modify with this  
> patch or do you use a custom config? Just curious ;)

All three 32-bit counters receive a dedicated clock from clock generator
module 1 (either CLKIN or DPLL1 output). You will find CONFIG_SYS_CLK_FREQ
defined to be 150000000 in include/configs/netstar.h and code in
board/netstar/setup.S selects DPLL1 as a clock source (and you'll probably
notice that netstar's lowlevel_init code looks different from all others just
because it was not copied, but written from scratch long before its author
even explored u-boot's existence, so take care when copying anything from
netstar's config file as it handles low level init different way).

>> Perhaps I should modify those
>> boards wich uses 12MHz clock to use smaller divisor, 
>
> Yes, this should be easily doable by changing CONFIG_SYS_PTV.

Yes, I'll send updated patch.

[snip]
>> Right, but the key point here is to unbind udelay from get_timer as now
>> get_timer works with miliseconds resolution.
>
> I hope I got it right with the updated patch sent some minutes ago.

Yes, that one looks definitely better (although I have no way to test it).

	ladis