[U-Boot] [PATCH V3] AT91 Fix: return value of get_tbclk
Jens Scharsig
js_at_ng at scharsoft.de
Sun Aug 8 12:52:15 CEST 2010
Dear Reinhard
> I think the timer.c is more broken that just that return value:
>
You are right, but the patch fixes only the single small problem.
> example 1:
> /*
> * timer without interrupts
> */
> unsigned long long get_ticks(void)
> {
> at91_pit_t *pit = (at91_pit_t *) AT91_PIT_BASE;
>
> ulong now = readl(&pit->piir);
>
> if (now >= lastinc) /* normal mode (non roll) */
> /* move stamp forward with absolut diff ticks */
> timestamp += (now - lastinc);
> else /* we have rollover of incrementer */
> timestamp += (0xFFFFFFFF - lastinc) + now;
> lastinc = now;
> return timestamp;
> }
>
> observation: timestamp, lastinc, now are all ulong.
> timestamp += (0xFFFFFFFF - lastinc) + now;
> is the same as
> timestamp += (now - lastinc) - 1;
> so in case of a "rollover" timestamp is incremented just by one less.
> I think the if is superfluous. ulong will handle the rollover automatically
But this is only true, if we are using ulong variables. I think the idea behind
this code is use unsigned long long for variables. (especially timestamp)
>
> example 2:
> void __udelay(unsigned long usec)
> {
> unsigned long long tmp;
> ulong tmo;
>
> tmo = usec_to_tick(usec);
> tmp = get_ticks() + tmo; /* get current timestamp */
>
> while (get_ticks() < tmp) /* loop till event */
> /*NOP*/;
> }
>
> observation: tmp being unsigned long long, get_ticks returning
> the unsigned long timestamp, tmp being a sum of two ulongs,
> the while might NEVER end. In practice that is very unlikely,
> however the code should be corrected.
Right, but this isn't only a problem of AT91 arch. Is should be fixed global.
The code is correct, if get_ticks returns real long long values.
>
> I was going to rework that timer sooner or later to address all
> those issues, but you are welcome to go ahead, too. Just we
> should avoid double work :)
I have no time enough at the moment to do that.
>
> Greetings, Reinhard
Best regards
Jens Scharsig
More information about the U-Boot
mailing list