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

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

[Jens Scharsig]

 * 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;
 }
-- 
1.7.1

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

[Reinhard Meyer]

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

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

[Jens Scharsig]

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

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

[Alexander Stein]

Hello,

Am Samstag, 7. August 2010, 19:49:42 schrieb Jens Scharsig:
>  * Fix: return value of get_tbclk
>  * this fixes issue with prematurely restart/retry, if BOOT_RETRY_TIMEOUT
> is used

This issue also arises, if you use CONFIG_AUTOBOOT_KEYED & friends

> 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;
>  }

Best regards,
Alexander

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

[Xu, Hong]

Hi Alex,

> -----Original Message-----
> From: Alexander Stein [mailto:alexander.stein at systec-electronic.com] 
> Sent: Monday, August 09, 2010 2:35 PM
> To: u-boot at lists.denx.de
> Cc: Jens Scharsig; Xu, Hong
> Subject: Re: [U-Boot] [PATCH V3] AT91 Fix: return value of get_tbclk
> 
> Hello,
> 
> Am Samstag, 7. August 2010, 19:49:42 schrieb Jens Scharsig:
> >  * Fix: return value of get_tbclk
> >  * this fixes issue with prematurely restart/retry, if 
> > BOOT_RETRY_TIMEOUT is used
> 
> This issue also arises, if you use CONFIG_AUTOBOOT_KEYED & friends
> 

I tried both BOOT_RETRY_TIMEOUT and CONFIG_AUTOBOOT_KEYED & CONFIG_AUTOBOOT_STOP_STR,
Both work well on AT91SAM9260EK

Which board did you tried?

BR,
Eric

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

[Alexander Stein]

Hello Eric,

On Wednesday 18 August 2010, 09:17:26 you wrote:
> > Hello,
> > 
> > Am Samstag, 7. August 2010, 19:49:42 schrieb Jens Scharsig:
> > >  * Fix: return value of get_tbclk
> > >  * this fixes issue with prematurely restart/retry, if
> > > 
> > > BOOT_RETRY_TIMEOUT is used
> > 
> > This issue also arises, if you use CONFIG_AUTOBOOT_KEYED & friends
> 
> I tried both BOOT_RETRY_TIMEOUT and CONFIG_AUTOBOOT_KEYED &
> CONFIG_AUTOBOOT_STOP_STR, Both work well on AT91SAM9260EK
> 
> Which board did you tried?

That's some time ago and I don't remember which u-boot version that was. I 
think i came across that problem on the AT91SAM9G20EK and our own SAM9G20 
board.

Best regards,
Alexander

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

[Reinhard Meyer]

Hi,

this patch also fixed BOOTDELAY on AT91:

* 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>
---
 cpu/arm926ejs/at91/timer.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/cpu/arm926ejs/at91/timer.c b/cpu/arm926ejs/at91/timer.c
index d21eebf..8efc34b 100644
--- a/cpu/arm926ejs/at91/timer.c
+++ b/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;
 }
-- 1.6.0.2 

Applied to u-boot-atmel/next
Thanks,

Reinhard

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

[Jens Scharsig]

Am 2010-08-31 09:36, schrieb Reinhard Meyer:
> Hi,
>  }
> -- 1.6.0.2 
> 
> Applied to u-boot-atmel/next
> Thanks,
> 
> Reinhard
> 
What is the reason for the new branch/Custodian Tree u-boot-atmel.

Thanks for more details

Jens

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

[Reinhard Meyer]

Jens Scharsig schrieb:
> Am 2010-08-31 09:36, schrieb Reinhard Meyer:
>> Hi,
>>  }
>> -- 1.6.0.2 
>>
>> Applied to u-boot-atmel/next
>> Thanks,
>>
>> Reinhard
>>
> What is the reason for the new branch/Custodian Tree u-boot-atmel.
Hello,

one reason is that AVR32 and AT91 share alot in the peripheral
building blocks, so many drivers are both for AVR32 and AT91.

I will try to make sure changes to drivers in that area are
not breaking either architecture.

On that behalf I'd like a briefing why there is

at91_emac AND macb?

macb works for AVR32AP700x and (at least) for AT91SAM9260/9G20/9XE.

What is the deal with at91_emac? Is it required for some AT91's
because macb does not work there?

Does it provide better "results" than macb?

Best Regards
Reinhard

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

[Jens Scharsig]

Am 2010-09-01 09:30, schrieb Reinhard Meyer:
> 
> at91_emac AND macb?
> 
> macb works for AVR32AP700x and (at least) for AT91SAM9260/9G20/9XE.
> 
> What is the deal with at91_emac? Is it required for some AT91's
> because macb does not work there?
> 
> Does it provide better "results" than macb?
> 
> Best Regards
> Reinhard
Hello,

I think the separation has historical reasons.
For AT91RM and AT91SAM existed separate drivers and separate arch.
I remeber in older datascheets the ethernet inteface also marked with MACB.
In my opinion, the at91_emac the latest drivers (NET_MULTI api SoC
access via c structurs).

But I dont know the exact differences are not of single use devices.
Please ask Xu, Hong

Best regards

Jens

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

[Xu, Hong]

Hi, 

> -----Original Message-----
> From: Jens Scharsig [mailto:js_at_ng at scharsoft.de] 
> Sent: Thursday, September 02, 2010 1:37 AM
> To: Reinhard Meyer
> Cc: Xu, Hong; u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH V3] AT91 Fix: return value of get_tbclk
> 
> Am 2010-09-01 09:30, schrieb Reinhard Meyer:
> > 
> > at91_emac AND macb?
> > 
> > macb works for AVR32AP700x and (at least) for AT91SAM9260/9G20/9XE.
> > 
> > What is the deal with at91_emac? Is it required for some AT91's 
> > because macb does not work there?
> > 
> > Does it provide better "results" than macb?
> > 
> > Best Regards
> > Reinhard
> Hello,
> 
> I think the separation has historical reasons.
> For AT91RM and AT91SAM existed separate drivers and separate arch.
> I remeber in older datascheets the ethernet inteface also 
> marked with MACB.
> In my opinion, the at91_emac the latest drivers (NET_MULTI 
> api SoC access via c structurs).
> 
> But I dont know the exact differences are not of single use devices.
> Please ask Xu, Hong

I believe there must be historic reasons. I'm not familiar with AT91RM series. But from the mainline code,

$ grep CONFIG_DRIVER_AT91EMAC include/configs/* | wc -l
11
$ grep CONFIG_MACB include/configs/* | wc -l
19

Many boards are dependent on both. And currently AT91SAM always uses CONFIG_MACB, I don't see any
change in the near future.

BR,
Eric

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

[Andreas Bießmann]

Hi,

Am 01.09.2010 19:37, schrieb Jens Scharsig:
> Am 2010-09-01 09:30, schrieb Reinhard Meyer:
>>
>> at91_emac AND macb?
>>
>> macb works for AVR32AP700x and (at least) for AT91SAM9260/9G20/9XE.
>>
>> What is the deal with at91_emac? Is it required for some AT91's
>> because macb does not work there?

>> Does it provide better "results" than macb?

> I think the separation has historical reasons.
> For AT91RM and AT91SAM existed separate drivers and separate arch.
> I remeber in older datascheets the ethernet inteface also marked with
MACB.
> In my opinion, the at91_emac the latest drivers (NET_MULTI api SoC
> access via c structurs).

currently at91_emac is used for AT91RM9200 devices cause this register
footprint differs to the one of MACB used in SAM9/AVR32 devices. Both
subsystems use similar namings but some register flags have different
offsets. MACB additionally seems to have more features than EMAC (namely
WOL register, some more flags in status register, ...).

But I think it is possible to merge both drivers cause except for the
different register footprint they both seems to work the same way. I
already have some improvements for at91_emac on stack and can look for
possibilities merging both.

regards

Andreas Bie?mann