[U-Boot] [PATCH V3] 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 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; }
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
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
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
Hi Alex,
-----Original Message----- From: Alexander Stein [mailto:alexander.stein@systec-electronic.com] Sent: Monday, August 09, 2010 2:35 PM To: u-boot@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
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
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
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
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
Hi,
-----Original Message----- From: Jens Scharsig [mailto:js_at_ng@scharsoft.de] Sent: Thursday, September 02, 2010 1:37 AM To: Reinhard Meyer Cc: Xu, Hong; u-boot@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
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
participants (7)
-
Alexander Stein -
Andreas Bießmann -
Jens Scharsig -
Jens Scharsig -
Reinhard Meyer -
Reinhard Meyer -
Xu, Hong