Re: [U-Boot] [PATCH V3] AT91 Fix: return value of get_tbclk

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