Skip to contentSkip to conversation

AT91 Fix: return value of get_tbclk

PATCH Superseded Jens Scharsig 7 participants 12 messages Aug 7, 2010 – Sep 2, 2010

Subsystems (2)
Arm (32-bit) Arm/Microchip (AT91) SoC support
lore
Share & tools
lore

Jens Scharsig

author
#0 lore raw

PATCH v3 1/1 AT91 Fix: return value of get_tbclk

* Fix: return value of get_tbclk
* this fixes issue with prematurely restart/retry, if BOOT_RETRY_TIMEOUT is used

Signed-off-by Jens Scharsig <[email protected]>

---
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(-)
arch/arm/cpu/arm926ejs/at91/timer.c +1 -4
index d21eebf..8efc34b 100644
@@ -138,8 +138,5 @@ ulong get_timer(ulong base)
  */
 ulong get_tbclk(void)
 {
-	ulong tbclk;
-
-	tbclk = CONFIG_SYS_HZ;
-	return tbclk;
+	return timer_freq;
 }
Signature · 1 line
1.7.1
#1 lore raw

PATCH v3 1/1 AT91 Fix: return value of get_tbclk

Quoted 19 lines · 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 <[email protected]>

---
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(-)
arch/arm/cpu/arm926ejs/at91/timer.c +0 -0
index d21eebf..8efc34b 100644

arch/arm/cpu/arm926ejs/at91/timer.c ulong get_timer(ulong base) · line 142

Quoted 9 lines
arch/arm/cpu/arm926ejs/at91/timer.c +1 -4
@@ -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

author
#2 lore raw

PATCH v3 1/1 AT91 Fix: return value of get_tbclk

Dear Reinhard

Quoted 2 lines

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.

Quoted 19 lines

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;
}
Quoted 6 lines

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)

Quoted 14 lines

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*/;
}
Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

Greetings, Reinhard

Best regards

Jens Scharsig

#3 lore raw

PATCH v3 1/1 AT91 Fix: return value of get_tbclk

In reply to Jens Scharsig

Hello,

Am Samstag, 7. August 2010, 19:49:42 schrieb Jens Scharsig:

Quoted 3 lines

* 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

Quoted 15 lines
Signed-off-by Jens Scharsig <[email protected]>

---
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(-)
(quoted diff) +0 -0

b/arch/arm/cpu/arm926ejs/at91/timer.c index d21eebf..8efc34b 100644

arch/arm/cpu/arm926ejs/at91/timer.c ulong get_timer(ulong base) · line 142

Quoted 9 lines
arch/arm/cpu/arm926ejs/at91/timer.c +1 -4
@@ -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

#4 lore raw

PATCH v3 1/1 AT91 Fix: return value of get_tbclk

Hi Alex,

Quoted 7 lines

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

Quoted 9 lines

Hello,

Am Samstag, 7. August 2010, 19:49:42 schrieb Jens Scharsig:

Quoted 3 lines

* 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

#5 lore raw

PATCH v3 1/1 AT91 Fix: return value of get_tbclk

Hello Eric,

Quoted 6 lines · On Wednesday 18 August 2010, 09:17:26 you wrote:
Quoted 6 lines

Hello,

Am Samstag, 7. August 2010, 19:49:42 schrieb Jens Scharsig:

Quoted 3 lines

* Fix: return value of get_tbclk
* this fixes issue with prematurely restart/retry, if

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

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

#6 lore raw

PATCH v3 1/1 AT91 Fix: return value of get_tbclk

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 <[email protected]>
---
 cpu/arm926ejs/at91/timer.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)
cpu/arm926ejs/at91/timer.c +1 -4
index d21eebf..8efc34b 100644
@@ -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

#7 lore raw

PATCH v3 1/1 AT91 Fix: return value of get_tbclk

Am 2010-08-31 09:36, schrieb Reinhard Meyer:

Quoted 9 lines

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

#8 lore raw

PATCH v3 1/1 AT91 Fix: return value of get_tbclk

Quoted 11 lines · Jens Scharsig

Am 2010-08-31 09:36, schrieb Reinhard Meyer:

Quoted 9 lines

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

author
#9 lore raw

PATCH v3 1/1 AT91 Fix: return value of get_tbclk

Am 2010-09-01 09:30, schrieb Reinhard Meyer:

Quoted 5 lines

at91_emac AND macb?

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

Quoted 7 lines

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

#10 lore raw

PATCH v3 1/1 AT91 Fix: return value of get_tbclk

Hi,

Quoted 29 lines

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

Quoted 5 lines

at91_emac AND macb?

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

Quoted 7 lines

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

Quoted 2 lines

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

#11 lore raw

PATCH v3 1/1 AT91 Fix: return value of get_tbclk

In reply to Jens Scharsig

Hi,

Am 01.09.2010 19:37, schrieb Jens Scharsig:

Quoted 6 lines

Am 2010-09-01 09:30, schrieb Reinhard Meyer:

Quoted 5 lines

at91_emac AND macb?

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

Quoted 8 lines
Quoted 4 lines

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.

Quoted 2 lines

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