* [U-Boot] [PATCH 2/2] ep93xx: Refactoring of timer code [not found] <7ee75976e78e2f82b4163fe1ff4233a850d4393c.1266962256.git.matthias@kaehlcke.net> @ 2010-02-23 22:04 ` Matthias Kaehlcke 2010-02-23 22:23 ` Wolfgang Denk 0 siblings, 1 reply; 4+ messages in thread From: Matthias Kaehlcke @ 2010-02-23 22:04 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH 2/2] ep93xx: Refactoring of timer code 2010-02-23 22:04 ` [U-Boot] [PATCH 2/2] ep93xx: Refactoring of timer code Matthias Kaehlcke @ 2010-02-23 22:23 ` Wolfgang Denk 2010-02-23 22:44 ` Matthias Kaehlcke 0 siblings, 1 reply; 4+ messages in thread From: Wolfgang Denk @ 2010-02-23 22:23 UTC (permalink / raw) To: u-boot 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_ ^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH 2/2] ep93xx: Refactoring of timer code 2010-02-23 22:23 ` Wolfgang Denk @ 2010-02-23 22:44 ` Matthias Kaehlcke 2010-02-24 2:04 ` Tom 0 siblings, 1 reply; 4+ messages in thread From: Matthias Kaehlcke @ 2010-02-23 22:44 UTC (permalink / raw) To: u-boot 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 `- ^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH 2/2] ep93xx: Refactoring of timer code 2010-02-23 22:44 ` Matthias Kaehlcke @ 2010-02-24 2:04 ` Tom 0 siblings, 0 replies; 4+ messages in thread From: Tom @ 2010-02-24 2:04 UTC (permalink / raw) To: u-boot 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! > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-02-24 2:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <7ee75976e78e2f82b4163fe1ff4233a850d4393c.1266962256.git.matthias@kaehlcke.net>
2010-02-23 22:04 ` [U-Boot] [PATCH 2/2] ep93xx: Refactoring of timer code Matthias Kaehlcke
2010-02-23 22:23 ` Wolfgang Denk
2010-02-23 22:44 ` Matthias Kaehlcke
2010-02-24 2:04 ` Tom
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox