From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jens Scharsig <js_at_ng@scharsoft.de>
Date: Sun, 08 Aug 2010 12:52:15 +0200
Subject: [U-Boot] [PATCH V3] AT91 Fix: return value of get_tbclk
In-Reply-To: <4C5DA0FD.5020709@emk-elektronik.de>
References: <4C5D9CB6.8090501@scharsoft.de>
	<4C5DA0FD.5020709@emk-elektronik.de>
Message-ID: <4C5E8C5F.6000001@scharsoft.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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