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

[U-Boot] [PATCH 2/2 rev2] 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>
---
Changes with regard to rev1:

 * use TIMER_FREQ directly instead of recalculating it with rounding errors


 cpu/arm920t/ep93xx/timer.c |   70 ++++++++++++++++---------------------------
 1 files changed, 26 insertions(+), 44 deletions(-)

diff --git a/cpu/arm920t/ep93xx/timer.c b/cpu/arm920t/ep93xx/timer.c
index 98c759c..b1a01a0 100644
--- a/cpu/arm920t/ep93xx/timer.c
+++ b/cpu/arm920t/ep93xx/timer.c
@@ -34,16 +34,18 @@
 #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 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 +59,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 *= TIMER_FREQ;
 		ticks /= 1000;
 	} else {
-		ticks = usecs * TIMER_LOAD_VAL * CONFIG_SYS_HZ;
+		ticks = usecs * TIMER_FREQ;
 		ticks /= (1000 * 1000);
 	}
 
@@ -71,7 +73,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 +83,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 +106,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 +115,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 +130,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 rev2] ep93xx: Refactoring of timer code

[Tom]

Matthias Kaehlcke wrote:
> 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>

This set has been applied to ARM.
Thanks
Tom

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

[Matthias Kaehlcke]

Hi Tom,

El Thu, Feb 25, 2010 at 09:54:59AM -0600 Tom ha dit:

> Matthias Kaehlcke wrote:
> >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>
> 
> This set has been applied to ARM.

i was/am working on a new version of the patch, taking into account
your remarks about the unit of TIMER_FREQ and fixing some issues
discussed with Alessandro Rubini off-list, who worked on a similar
patch.

what do you prefer, a patch based on the ones you just applied, or
revert them and get one with all fixes (only one patch, cause
the function clk_to_systicks() fixed in the first patch of the set is
removed)

i apologize for not having taken the time to notify about this and
causing you overhead :(

-- 
Matthias Kaehlcke
Embedded Linux Developer
Barcelona

 Someone has said that it requires less mental effort to condemn than to think
                             (Emma Goldman)
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-

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

[Alessandro Rubini]

> i was/am working on a new version of the patch, taking into account
> your remarks about the unit of TIMER_FREQ and fixing some issues
> discussed with Alessandro Rubini off-list, who worked on a similar
> patch.

Actually, I checked the point we disagreed about, which is the unit of
get_ticks() and get_tbclk().  You currently return hw-ticks in
get_ticks, and CONFIG_SYS_HZ (i.e. 1000) in get_tbclk.  However, these
two functions are expected to be used together, so they must be
consistent in their return value.

It's true that the functions are little used (they are mostly used in
ppc code, within cpu/*/interrupts), and that's why I didn't even
provide them in cpu/arm926ejs/nomadik/timer.c. All few users assume
they are consistent, but there is no documentation:

     tornado% grep -qr get_tbclk README* doc  || echo not found
     not found
     tornado% grep -qr get_ticks README* doc/* || echo not found
     not found

I've made a quick tour of all definitions in cpu/ and here is the result.
As you see, at91 (which you used as reference, I understand) is wrong,
while all the others use either hwticks or SYS_HZ consistently.


  source-file                    get_ticks()          get_tbclk

arm920t/at91/timer.c                 get_timer()      CONFIG_SYS_HZ
arm920t/a320/timer.c                 get_timer()      CONFIG_SYS_HZ
arm920t/s3c24x0/timer.c              hw-ticks         (I didn't understand)
arm920t/imx/timer.c                  get_timer()      CONFIG_SYS_HZ
arm920t/at91rm9200/timer.c           get_timer()      CONFIG_SYS_HZ
pxa/timer.c                          hw-ticks-32bits  TIMER_FREQ_HZ
arm_cortexa8/omap3/timer.c           get_timer()      CONFIG_SYS_HZ
arm_cortexa8/s5pc1xx/timer.c         get_timer()      CONFIG_SYS_HZ
arm925t/timer.c                      get_timer()      CONFIG_SYS_HZ
ppc4xx/cpu.c                         lib_ppc/ticks.S  freqProcessor
blackfin/interrupts.c                get_timer()      CONFIG_SYS_HZ
sa1100/timer.c                       get_timer()      CONFIG_SYS_HZ
mpc824x/cpu.c                        lib_ppc/ticks.S  bus_freq
mpc8xx/cpu.c                         lib_ppc/ticks.S  timebase
mpc512x/cpu.c                        lib_ppc/ticks.S  timebase
mpc5xxx/cpu.c                        lib_ppc/ticks.S  timebase
mpc8220/cpu.c                        lib_ppc/ticks.S  timebase
at32ap/interrupts.c                  hw-ticks         cpu_hz
mpc85xx/cpu.c                        lib_ppc/ticks.S  timebase
74xx_7xx/cpu.c                       lib_ppc/ticks.S  timebase
arm1136/omap24xx/timer.c             get_timer()      CONFIG_SYS_HZ
mpc86xx/cpu.c                        lib_ppc/ticks.S  timebase
mpc8260/cpu.c                        lib_ppc/ticks.S  timebase
arm926ejs/omap/timer.c               get_timer()      CONFIG_SYS_HZ
arm926ejs/davinci/timer.c            get_timer()      CONFIG_SYS_HZ
arm926ejs/versatile/timer.c          get_timer()      CONFIG_SYS_HZ
arm926ejs/at91/timer.c               hwticks          CONFIG_SYS_HZ
arm926ejs/spear/timer.c              hwticks          CONFIG_SYS_HZ
arm1176/s3c64xx/timer.c              hwrticks         (I didn't understand)
lh7a40x/timer.c                      get_timer        (I didn't understand)
mpc5xx/cpu.c                         lib_ppc/ticks.S  timebase

/alessandro

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

[Matthias Kaehlcke]

Hi,

El Thu, Feb 25, 2010 at 08:22:59PM +0100 Alessandro Rubini ha dit:

> > i was/am working on a new version of the patch, taking into account
> > your remarks about the unit of TIMER_FREQ and fixing some issues
> > discussed with Alessandro Rubini off-list, who worked on a similar
> > patch.
> 
> Actually, I checked the point we disagreed about, which is the unit of
> get_ticks() and get_tbclk().  You currently return hw-ticks in
> get_ticks, and CONFIG_SYS_HZ (i.e. 1000) in get_tbclk.  However, these
> two functions are expected to be used together, so they must be
> consistent in their return value.

actually there is no disagreement between us, i totally agree with you
that the return value of get_ticks() should be in CONFIG_SYS_HZ
resolution and consistent with get_tbclk(). the patch i sent you
yesterday off-list fixes exactly this.

> It's true that the functions are little used (they are mostly used in
> ppc code, within cpu/*/interrupts), and that's why I didn't even
> provide them in cpu/arm926ejs/nomadik/timer.c. All few users assume
> they are consistent, but there is no documentation:
> 
>      tornado% grep -qr get_tbclk README* doc  || echo not found
>      not found
>      tornado% grep -qr get_ticks README* doc/* || echo not found
>      not found
> 
> I've made a quick tour of all definitions in cpu/ and here is the result.
> As you see, at91 (which you used as reference, I understand) is wrong,
> while all the others use either hwticks or SYS_HZ consistently.

yes, i used precisely at91 as reference, i liked it's code structure
and didn't notice that it is wrong in this point.

thanks for your research!

-- 
Matthias Kaehlcke
Embedded Linux Developer
Barcelona

              We build too many walls and not enough bridges
                             (Isaac Newton)
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-

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

[Tom]

Matthias Kaehlcke wrote:
> Hi Tom,
> 
> El Thu, Feb 25, 2010 at 09:54:59AM -0600 Tom ha dit:
> 
>> Matthias Kaehlcke wrote:
>>> 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>
>> This set has been applied to ARM.
> 
> i was/am working on a new version of the patch, taking into account
> your remarks about the unit of TIMER_FREQ and fixing some issues
> discussed with Alessandro Rubini off-list, who worked on a similar
> patch.
> 
> what do you prefer, a patch based on the ones you just applied, or
> revert them and get one with all fixes (only one patch, cause
> the function clk_to_systicks() fixed in the first patch of the set is
> removed)
> 
> i apologize for not having taken the time to notify about this and
> causing you overhead :(
> 
Its ok with me if you just rebase and submit a smaller patch.
Tom