From: Reinhard Meyer <reinhard.meyer@emk-elektronik.de>
Date: Sat, 07 Aug 2010 20:07:57 +0200
Subject: [U-Boot] [PATCH V3] AT91 Fix: return value of get_tbclk
In-Reply-To: <4C5D9CB6.8090501@scharsoft.de>
References: <4C5D9CB6.8090501@scharsoft.de>
Message-ID: <4C5DA0FD.5020709@emk-elektronik.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

Jens Scharsig wrote:
>  * Fix: return value of get_tbclk
>  * this fixes issue with prematurely restart/retry, if BOOT_RETRY_TIMEOUT is used
>
>
> Signed-off-by: Jens Scharsig <js_at_ng@scharsoft.de>
> ---
>  the V3 supports the actually file structure.
>  the original patch http://lists.denx.de/pipermail/u-boot/2010-April/069415.html
>  use the old directory /cpu/arch structure.
>
>  arch/arm/cpu/arm926ejs/at91/timer.c |    5 +----
>  1 files changed, 1 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/cpu/arm926ejs/at91/timer.c b/arch/arm/cpu/arm926ejs/at91/timer.c
> index d21eebf..8efc34b 100644
> --- a/arch/arm/cpu/arm926ejs/at91/timer.c
> +++ b/arch/arm/cpu/arm926ejs/at91/timer.c
> @@ -138,8 +138,5 @@ ulong get_timer(ulong base)
>   */
>  ulong get_tbclk(void)
>  {
> -	ulong tbclk;
> -
> -	tbclk = CONFIG_SYS_HZ;
> -	return tbclk;
> +	return timer_freq;
>  }
>   
Dear Jens,

I think the timer.c is more broken that just that return value:

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

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.

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 :)

Greetings, Reinhard