[U-Boot] [PATCH 2/2] ep93xx: Refactoring of timer code

[U-Boot] [PATCH 2/2] ep93xx: Refactoring of timer code

[Matthias Kaehlcke]

ep93xx: Refactoring of the timer code, including the following changes

 * use a free running timer instead of a periodical one
 * use unsigned long long for total number of ticks
 * hold the timer state in a structure instead of separate variables
 * increment the timer counter instead of decrementing it
 * remove unused function udelay_masked()
 * remove unused function set_timer()

Signed-off-by: Matthias Kaehlcke <matthias@kaehlcke.net>
---
 cpu/arm920t/ep93xx/timer.c |   71 ++++++++++++++++---------------------------
 1 files changed, 27 insertions(+), 44 deletions(-)

diff --git a/cpu/arm920t/ep93xx/timer.c b/cpu/arm920t/ep93xx/timer.c
index 98c759c..84bdb12 100644
--- a/cpu/arm920t/ep93xx/timer.c
+++ b/cpu/arm920t/ep93xx/timer.c
@@ -34,16 +34,19 @@
 #include <div64.h>
 
 #define TIMER_CLKSEL	(1 << 3)
-#define TIMER_MODE	(1 << 6)
 #define TIMER_ENABLE	(1 << 7)
 
-#define TIMER_FREQ	508469
-#define TIMER_LOAD_VAL	(TIMER_FREQ / CONFIG_SYS_HZ)
+#define TIMER_FREQ			508469
+#define CLK_TICKS_PER_SYS_TICK		(TIMER_FREQ / CONFIG_SYS_HZ)
+#define TIMER_MAX_VAL			0xFFFFFFFF
 
-static ulong timestamp;
-static ulong lastdec;
+static struct ep93xx_timer
+{
+	unsigned long long ticks;
+	unsigned long last_update;
+} timer;
 
-static inline unsigned long clk_to_systicks(unsigned long clk_ticks)
+static inline unsigned long clk_to_systicks(unsigned long long clk_ticks)
 {
 	unsigned long long sys_ticks = (clk_ticks * CONFIG_SYS_HZ);
 	do_div(sys_ticks, TIMER_FREQ);
@@ -57,10 +60,10 @@ static inline unsigned long usecs_to_ticks(unsigned long usecs)
 
 	if (usecs >= 1000) {
 		ticks = usecs / 1000;
-		ticks *= (TIMER_LOAD_VAL * CONFIG_SYS_HZ);
+		ticks *= (CLK_TICKS_PER_SYS_TICK * CONFIG_SYS_HZ);
 		ticks /= 1000;
 	} else {
-		ticks = usecs * TIMER_LOAD_VAL * CONFIG_SYS_HZ;
+		ticks = usecs * CLK_TICKS_PER_SYS_TICK * CONFIG_SYS_HZ;
 		ticks /= (1000 * 1000);
 	}
 
@@ -71,7 +74,7 @@ static inline unsigned long read_timer(void)
 {
 	struct timer_regs *timer = (struct timer_regs *)TIMER_BASE;
 
-	return readl(&timer->timer3.value);
+	return TIMER_MAX_VAL - readl(&timer->timer3.value);
 }
 
 /*
@@ -81,17 +84,15 @@ unsigned long long get_ticks(void)
 {
 	const unsigned long now = read_timer();
 
-	if (lastdec >= now) {
-		/* normal mode */
-		timestamp += lastdec - now;
-	} else {
-		/* we have an overflow ... */
-		timestamp += lastdec + TIMER_LOAD_VAL - now;
-	}
+	if (now >= timer.last_update)
+		timer.ticks += now - timer.last_update;
+	else
+		/* an overflow occurred */
+		timer.ticks += TIMER_MAX_VAL - timer.last_update + now;
 
-	lastdec = now;
+	timer.last_update = now;
 
-	return timestamp;
+	return timer.ticks;
 }
 
 unsigned long get_timer_masked(void)
@@ -106,8 +107,8 @@ unsigned long get_timer(unsigned long base)
 
 void reset_timer_masked(void)
 {
-	lastdec = read_timer();
-	timestamp = 0;
+	timer.last_update = read_timer();
+	timer.ticks = 0;
 }
 
 void reset_timer(void)
@@ -115,28 +116,11 @@ void reset_timer(void)
 	reset_timer_masked();
 }
 
-void set_timer(unsigned long t)
-{
-	timestamp = t;
-}
-
 void __udelay(unsigned long usec)
 {
-	const unsigned long ticks = usecs_to_ticks(usec);
-	const unsigned long target = clk_to_systicks(ticks) + get_timer(0);
-
-	while (get_timer_masked() < target)
-		/* noop */;
-}
-
-void udelay_masked(unsigned long usec)
-{
-	const unsigned long ticks = usecs_to_ticks(usec);
-	const unsigned long target = clk_to_systicks(ticks) + get_timer(0);
-
-	reset_timer_masked();
+	const unsigned long target = get_ticks() + usecs_to_ticks(usec);
 
-	while (get_timer_masked() < target)
+	while (get_ticks() < target)
 		/* noop */;
 }
 
@@ -147,12 +131,11 @@ int timer_init(void)
 	/* use timer 3 with 508KHz and free running */
 	writel(TIMER_CLKSEL, &timer->timer3.control);
 
-	/* auto load, manual update of Timer 3 */
-	lastdec = TIMER_LOAD_VAL;
-	writel(TIMER_LOAD_VAL, &timer->timer3.load);
+	/* set initial timer value 3 */
+	writel(TIMER_MAX_VAL, &timer->timer3.load);
 
-	/* Enable the timer and periodic mode */
-	writel(TIMER_ENABLE | TIMER_MODE | TIMER_CLKSEL,
+	/* Enable the timer */
+	writel(TIMER_ENABLE | TIMER_CLKSEL,
 		&timer->timer3.control);
 
 	reset_timer_masked();
-- 
1.6.5

[U-Boot] [PATCH 2/2] ep93xx: Refactoring of timer code

[Wolfgang Denk]

Dear Matthias Kaehlcke,

In message <20100223220421.GK20201@darwin> you wrote:
> ep93xx: Refactoring of the timer code, including the following changes
...
> +#define TIMER_FREQ			508469
> +#define CLK_TICKS_PER_SYS_TICK		(TIMER_FREQ / CONFIG_SYS_HZ)
...
> +		ticks *= (CLK_TICKS_PER_SYS_TICK * CONFIG_SYS_HZ);
...
> +		ticks = usecs * CLK_TICKS_PER_SYS_TICK * CONFIG_SYS_HZ;

Why don't you use

	ticks *= TIMER_FREQ;
resp.
	ticks = usecs * TIMER_FREQ;

The combination of " / CONFIG_SYS_HZ * CONFIG_SYS_HZ" is just a bad
NO-OP (with rounding errors).

Hm... re-reading the optimized code makes me wonder if the variable
really should be called "ticks" - looks more as a frequency to me?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If it went on at this rate, in several billion  years  he'd  be  rich
beyond his wildest dreams!            - Terry Pratchett, _Soul Music_

[U-Boot] [PATCH 2/2] ep93xx: Refactoring of timer code

[Matthias Kaehlcke]

Hi Wolfgang,

El Tue, Feb 23, 2010 at 11:23:42PM +0100 Wolfgang Denk ha dit:

> In message <20100223220421.GK20201@darwin> you wrote:
> > ep93xx: Refactoring of the timer code, including the following changes
> ...
> > +#define TIMER_FREQ			508469
> > +#define CLK_TICKS_PER_SYS_TICK		(TIMER_FREQ / CONFIG_SYS_HZ)
> ...
> > +		ticks *= (CLK_TICKS_PER_SYS_TICK * CONFIG_SYS_HZ);
> ...
> > +		ticks = usecs * CLK_TICKS_PER_SYS_TICK * CONFIG_SYS_HZ;
> 
> Why don't you use
> 
> 	ticks *= TIMER_FREQ;
> resp.
> 	ticks = usecs * TIMER_FREQ;
> 
> The combination of " / CONFIG_SYS_HZ * CONFIG_SYS_HZ" is just a bad
> NO-OP (with rounding errors).

you certainly have a point, i'm going to change this as you proposed

> Hm... re-reading the optimized code makes me wonder if the variable
> really should be called "ticks" - looks more as a frequency to me?

here i disagree, the function returns the number of ticks that pass in
a certain number of microseconds, so i think 'ticks' is an appropiate
name

thanks for your review!

-- 
Matthias Kaehlcke
Embedded Linux Developer
Barcelona

        I cannot say whether things will get better if we change,
      what I can say is they must change if they are to get better
                   (Georg Christoph Lichtenberg)
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-

[U-Boot] [PATCH 2/2] ep93xx: Refactoring of timer code

[Tom]

Matthias Kaehlcke wrote:
> Hi Wolfgang,
> 
> El Tue, Feb 23, 2010 at 11:23:42PM +0100 Wolfgang Denk ha dit:
> 
>> In message <20100223220421.GK20201@darwin> you wrote:
>>> ep93xx: Refactoring of the timer code, including the following changes
>> ...
>>> +#define TIMER_FREQ			508469
>>> +#define CLK_TICKS_PER_SYS_TICK		(TIMER_FREQ / CONFIG_SYS_HZ)
>> ...
>>> +		ticks *= (CLK_TICKS_PER_SYS_TICK * CONFIG_SYS_HZ);
>> ...
>>> +		ticks = usecs * CLK_TICKS_PER_SYS_TICK * CONFIG_SYS_HZ;
>> Why don't you use
>>
>> 	ticks *= TIMER_FREQ;
>> resp.
>> 	ticks = usecs * TIMER_FREQ;
>>
>> The combination of " / CONFIG_SYS_HZ * CONFIG_SYS_HZ" is just a bad
>> NO-OP (with rounding errors).
> 
> you certainly have a point, i'm going to change this as you proposed
> 
>> Hm... re-reading the optimized code makes me wonder if the variable
>> really should be called "ticks" - looks more as a frequency to me?
> 
> here i disagree, the function returns the number of ticks that pass in
> a certain number of microseconds, so i think 'ticks' is an appropiate
> name
> 
Perhaps a comment on the units of TIMER_FREQ ?
This is minor point.
Otherwise looks fine.
Tom


> thanks for your review!
>