[U-Boot] [PATCH v2] at91: change CONFIG_SYS_HZ to 1000
Change at91 CPUs based on arm926ejs to return milliseconds from get_timer and get_ticks. Also changes in the value of CONFIG_SYS_HZ to 1000 in all board configs using these CPUs. This will not compile on boards using these CPUs with a different value for CONFIG_SYS_HZ.
Signed-off-by: Detlef Vollmann dv@vollmann.ch --- cpu/arm926ejs/at91/timer.c | 37 +++++++++++++++++++++++++++++++++---- include/configs/afeb9260.h | 2 +- include/configs/at91cap9adk.h | 2 +- include/configs/at91sam9260ek.h | 2 +- include/configs/at91sam9261ek.h | 2 +- include/configs/at91sam9263ek.h | 2 +- include/configs/at91sam9rlek.h | 2 +- 7 files changed, 39 insertions(+), 10 deletions(-)
diff --git a/cpu/arm926ejs/at91/timer.c b/cpu/arm926ejs/at91/timer.c index fec545b..0a38a4f 100644 --- a/cpu/arm926ejs/at91/timer.c +++ b/cpu/arm926ejs/at91/timer.c @@ -30,6 +30,19 @@ #include <asm/arch/io.h>
/* + * This code essentially ignores the settings of AT91_MASTER_CLOCK + * and CONFIG_SYS_HZ, so we just bail out if they're not set to + * the canonical values. + */ +#if (CONFIG_SYS_HZ != 1000) +#error "CONFIG_SYS_HZ must be set to 1000 for this CPU" +#endif +/* We leave some leeway for the master clock... */ +#if (AT91_MASTER_CLOCK < 85000000) || (AT91_MASTER_CLOCK > 115000000) +#error "AT91_MASTER_CLOCK must be set to 100000000 for this CPU" +#endif + +/* * We're using the AT91CAP9/SAM9 PITC in 32 bit mode, by * setting the 20 bit counter period to its maximum (0xfffff). */ @@ -38,6 +51,7 @@ #define READ_TIMER at91_sys_read(AT91_PIT_PIIR) #define TIMER_FREQ (AT91C_MASTER_CLOCK << 4) #define TICKS_TO_USEC(ticks) ((ticks) / 6) +#define TICKS_TO_MSEC(ticks) ((ticks) / 6250)
ulong get_timer_masked(void); ulong resettime; @@ -73,6 +87,21 @@ static inline ulong get_timer_raw(void) return 0xFFFFFFFFUL - (resettime - now) ; }
+static inline ulong get_timer_masked_usec(void) +{ + return TICKS_TO_USEC(get_timer_raw()); +} + +static inline ulong get_timer_usec(ulong base) +{ + ulong now = get_timer_masked_usec(); + + if (now >= base) + return now - base; + else + return TICKS_TO_USEC(0xFFFFFFFFUL) - (base - now) ; +} + void reset_timer_masked(void) { resettime = READ_TIMER; @@ -80,7 +109,7 @@ void reset_timer_masked(void)
ulong get_timer_masked(void) { - return TICKS_TO_USEC(get_timer_raw()); + return TICKS_TO_MSEC(get_timer_raw());
}
@@ -88,8 +117,8 @@ void udelay_masked(unsigned long usec) { ulong tmp;
- tmp = get_timer(0); - while (get_timer(tmp) < usec) /* our timer works in usecs */ + tmp = get_timer_usec(0); + while (get_timer_usec(tmp) < usec) ; /* NOP */ }
@@ -105,7 +134,7 @@ ulong get_timer(ulong base) if (now >= base) return now - base; else - return TICKS_TO_USEC(0xFFFFFFFFUL) - (base - now) ; + return TICKS_TO_MSEC(0xFFFFFFFFUL) - (base - now) ; }
void udelay(unsigned long usec) diff --git a/include/configs/afeb9260.h b/include/configs/afeb9260.h index 9eed342..7545bd0 100644 --- a/include/configs/afeb9260.h +++ b/include/configs/afeb9260.h @@ -30,7 +30,7 @@ #define AT91_MAIN_CLOCK 18429952 /* from 18.432 MHz crystal */ #define AT91_MASTER_CLOCK 89999598 /* peripheral = main / 2 */ #define CONFIG_SYS_AT91_PLLB 0x107c3e18 /* PLLB settings for USB */ -#define CONFIG_SYS_HZ 1000000 /* 1us resolution */ +#define CONFIG_SYS_HZ 1000 /* 1ms resolution */
#define AT91_SLOW_CLOCK 32768 /* slow clock */
diff --git a/include/configs/at91cap9adk.h b/include/configs/at91cap9adk.h index 01da99b..3b033c5 100644 --- a/include/configs/at91cap9adk.h +++ b/include/configs/at91cap9adk.h @@ -33,7 +33,7 @@ #define AT91_MASTER_CLOCK 100000000 /* peripheral */ #define AT91_CPU_CLOCK 200000000 /* cpu */ #define CONFIG_SYS_AT91_PLLB 0x10073e01 /* PLLB settings for USB */ -#define CONFIG_SYS_HZ 1000000 /* 1us resolution */ +#define CONFIG_SYS_HZ 1000 /* 1ms resolution */
#define AT91_SLOW_CLOCK 32768 /* slow clock */
diff --git a/include/configs/at91sam9260ek.h b/include/configs/at91sam9260ek.h index 2f1a41f..ec2b6f1 100644 --- a/include/configs/at91sam9260ek.h +++ b/include/configs/at91sam9260ek.h @@ -33,7 +33,7 @@ #define AT91_MASTER_CLOCK 100000000 /* peripheral */ #define AT91_CPU_CLOCK 200000000 /* cpu */ #define CONFIG_SYS_AT91_PLLB 0x107c3e18 /* PLLB settings for USB */ -#define CONFIG_SYS_HZ 1000000 /* 1us resolution */ +#define CONFIG_SYS_HZ 1000 /* 1ms resolution */
#define AT91_SLOW_CLOCK 32768 /* slow clock */
diff --git a/include/configs/at91sam9261ek.h b/include/configs/at91sam9261ek.h index ebecfa4..d3209a3 100644 --- a/include/configs/at91sam9261ek.h +++ b/include/configs/at91sam9261ek.h @@ -32,7 +32,7 @@ #define AT91_MAIN_CLOCK 18432000 /* 18.432 MHz crystal */ #define AT91_MASTER_CLOCK 100000000 /* peripheral */ #define AT91_CPU_CLOCK 200000000 /* cpu */ -#define CONFIG_SYS_HZ 1000000 /* 1us resolution */ +#define CONFIG_SYS_HZ 1000 /* 1ms resolution */
#define AT91_SLOW_CLOCK 32768 /* slow clock */
diff --git a/include/configs/at91sam9263ek.h b/include/configs/at91sam9263ek.h index 09b871a..ca60965 100644 --- a/include/configs/at91sam9263ek.h +++ b/include/configs/at91sam9263ek.h @@ -33,7 +33,7 @@ #define AT91_MASTER_CLOCK 100000000 /* peripheral */ #define AT91_CPU_CLOCK 200000000 /* cpu */ #define CONFIG_SYS_AT91_PLLB 0x133a3e8d /* PLLB settings for USB */ -#define CONFIG_SYS_HZ 1000000 /* 1us resolution */ +#define CONFIG_SYS_HZ 1000 /* 1ms resolution */
#define AT91_SLOW_CLOCK 32768 /* slow clock */
diff --git a/include/configs/at91sam9rlek.h b/include/configs/at91sam9rlek.h index 5bef1fe..caf0e22 100644 --- a/include/configs/at91sam9rlek.h +++ b/include/configs/at91sam9rlek.h @@ -32,7 +32,7 @@ #define AT91_MAIN_CLOCK 12000000 /* 12 MHz crystal */ #define AT91_MASTER_CLOCK 100000000 /* peripheral */ #define AT91_CPU_CLOCK 200000000 /* cpu */ -#define CONFIG_SYS_HZ 1000000 /* 1us resolution */ +#define CONFIG_SYS_HZ 1000 /* 1ms resolution */
#define AT91_SLOW_CLOCK 32768 /* slow clock */
On 02:58 Wed 25 Feb , Detlef Vollmann wrote:
Change at91 CPUs based on arm926ejs to return milliseconds from get_timer and get_ticks. Also changes in the value of CONFIG_SYS_HZ to 1000 in all board configs using these CPUs. This will not compile on boards using these CPUs with a different value for CONFIG_SYS_HZ.
Signed-off-by: Detlef Vollmann dv@vollmann.ch
cpu/arm926ejs/at91/timer.c | 37 +++++++++++++++++++++++++++++++++---- include/configs/afeb9260.h | 2 +- include/configs/at91cap9adk.h | 2 +- include/configs/at91sam9260ek.h | 2 +- include/configs/at91sam9261ek.h | 2 +- include/configs/at91sam9263ek.h | 2 +- include/configs/at91sam9rlek.h | 2 +- 7 files changed, 39 insertions(+), 10 deletions(-)
diff --git a/cpu/arm926ejs/at91/timer.c b/cpu/arm926ejs/at91/timer.c index fec545b..0a38a4f 100644 --- a/cpu/arm926ejs/at91/timer.c +++ b/cpu/arm926ejs/at91/timer.c @@ -30,6 +30,19 @@ #include <asm/arch/io.h>
/*
- This code essentially ignores the settings of AT91_MASTER_CLOCK
- and CONFIG_SYS_HZ, so we just bail out if they're not set to
- the canonical values.
- */
+#if (CONFIG_SYS_HZ != 1000) +#error "CONFIG_SYS_HZ must be set to 1000 for this CPU" +#endif
no need please remove
+/* We leave some leeway for the master clock... */ +#if (AT91_MASTER_CLOCK < 85000000) || (AT91_MASTER_CLOCK > 115000000) +#error "AT91_MASTER_CLOCK must be set to 100000000 for this CPU" +#endif
why?
Best Regards, J.
Jean-Christophe PLAGNIOL-VILLARD wrote:
On 02:58 Wed 25 Feb , Detlef Vollmann wrote:
/*
- This code essentially ignores the settings of AT91_MASTER_CLOCK
- and CONFIG_SYS_HZ, so we just bail out if they're not set to
- the canonical values.
- */
+#if (CONFIG_SYS_HZ != 1000) +#error "CONFIG_SYS_HZ must be set to 1000 for this CPU" +#endif
no need please remove
As I already wrote in another message, I don't think so. All current boards that use U-Boot on that chip family need to use a value of 1000000 for CONFIG_SYS_HZ, and most of these config files are not in the public U-Boot tree. If these projects change to a current U-Boot version, they fail. And I prefer a loud compile time fail over a silent runtime fail. And I can't see any reason for not having that check: it doesn't clutter the code, has no measurable effects on compile time, and absolutely no effects for the runtime.
+/* We leave some leeway for the master clock... */ +#if (AT91_MASTER_CLOCK < 85000000) || (AT91_MASTER_CLOCK > 115000000) +#error "AT91_MASTER_CLOCK must be set to 100000000 for this CPU" +#endif
why?
The old implementation simply assumes that AT91_MASTER_CLOCK has a value of 100000000, and I didn't change that. But the config file for afeb9260 sets it to a value of 89999598, and I can't change that as I don't know that board, but it's near enough to 100000000 to accept that. And I noticed that only because I had the check there in the first place, so it didn't compile with MAKEALL when I only checked for the exact value of 100000000.
Best Regards, Detlef
Dear Detlef Vollmann,
In message 49B97998.2080805@vollmann.ch you wrote:
+#if (CONFIG_SYS_HZ != 1000) +#error "CONFIG_SYS_HZ must be set to 1000 for this CPU" +#endif
no need please remove
As I already wrote in another message, I don't think so. All current boards that use U-Boot on that chip family need to use a value of 1000000 for CONFIG_SYS_HZ, and most of these config files are not in the public U-Boot tree.
We do not add dead code or provide other provisions for code that may exist in out-of-tree ports.
If these projects change to a current U-Boot version, they
And I can't see any reason for not having that check: it doesn't clutter the code, has no measurable effects on compile time, and absolutely no effects for the runtime.
Yes, it does clutter the code. It is dead code. We could as well add
#if 1 != 1 #error Armageddon! #endif
or similar to each and every source fine, just in case hell breaks lose.
+/* We leave some leeway for the master clock... */ +#if (AT91_MASTER_CLOCK < 85000000) || (AT91_MASTER_CLOCK > 115000000) +#error "AT91_MASTER_CLOCK must be set to 100000000 for this CPU" +#endif
why?
The old implementation simply assumes that AT91_MASTER_CLOCK has a value of 100000000, and I didn't change that. But the config file for afeb9260 sets it to a value of 89999598, and I can't change that as I don't know that board, but it's near enough to 100000000 to accept that. And I noticed that only because I had the check there in the first place, so it didn't compile with MAKEALL when I only checked for the exact value of 100000000.
That's a very obscure reasoning, me thinks. We don't understand the 89999598, so we accept 85000000...115000000 ? That's true vodoo programming.
Best regards,
Wolfgang Denk
But the config file for afeb9260 sets it to a value of 89999598, and I can't change that as I don't know that board, but it's near enough to 100000000 to accept that. And I noticed that only because I had the check there in the first place, so it didn't compile with MAKEALL when I only checked for the exact value of 100000000.
That's a very obscure reasoning, me thinks. We don't understand the 89999598, so we accept 85000000...115000000 ? That's true vodoo programming.
I can explain this stuff about master clock :)
1. AT91SAM9260 CPU is run from PLLA on this board. PLLA runs at 180MHz there as maximum stable speed (Actually there are boards with smaller value, I have revisions for 166 and 133MHz here and I want to provide a few patches about this stuff). But 180 MHz is not exact value and it is from 18429952 by multiplication and division. And we need to use it as close as possible because ALL peripherals, except USB depend on this value for timings. Also, a problem is that not all combinations and divisions actually work, and that is hardware feature, too. That's because it is not possible to get this 100MHz value, it is just it - you can't get 100MHz by division of 180MHz by any integer value starting from 2.
I don't see any real reason to pretend we have always 100MHz on this CPU. That might harm a lot. And you can't even imagine how nasty this hardware can be.
Hi Detlef,
Detlef Vollmann a écrit :
Change at91 CPUs based on arm926ejs to return milliseconds from get_timer and get_ticks. Also changes in the value of CONFIG_SYS_HZ to 1000 in all board configs using these CPUs. This will not compile on boards using these CPUs with a different value for CONFIG_SYS_HZ.
I confirm that this patch is fixing timeout problems with tftp downloads on a SAM9260. What is blocking from pushing it mainline ?
Eric
On 21:11 Thu 02 Apr , Eric BENARD wrote:
Hi Detlef,
Detlef Vollmann a écrit :
Change at91 CPUs based on arm926ejs to return milliseconds from get_timer and get_ticks. Also changes in the value of CONFIG_SYS_HZ to 1000 in all board configs using these CPUs. This will not compile on boards using these CPUs with a different value for CONFIG_SYS_HZ.
I confirm that this patch is fixing timeout problems with tftp downloads on a SAM9260. What is blocking from pushing it mainline ?
see response e-mail and please note that the current patch is not supposed to work because AT91C_MAIN_CLOCK does not exist and it assusme that the main_clock will be at 100000000 which is not the case everytime as example on 9G20 it's not
I've send a new patch that introduce an improve of the clock on at91 which give you the calculated main_clock based on the main crystal and the lowlevel settings
Best Regards, J.
Hi Jean Christophe,
Jean-Christophe PLAGNIOL-VILLARD a écrit :
On 21:11 Thu 02 Apr , Eric BENARD wrote:
I confirm that this patch is fixing timeout problems with tftp downloads on a SAM9260. What is blocking from pushing it mainline ?
see response e-mail and please note that the current patch is not supposed to work because AT91C_MAIN_CLOCK does not exist and it assusme that the
it's using MASTER_CLOCK and it seems to work, at least on my board, and the improvement is clearly seen when using tftp (no more T and no more retry when downloading a large file).
main_clock will be at 100000000 which is not the case everytime as example on 9G20 it's not
true, I think G20 support was not mainlined when the patch was created which can explain this.
I've send a new patch that introduce an improve of the clock on at91 which give you the calculated main_clock based on the main crystal and the lowlevel settings
I've just seen this patch. I will give a try asap. Is it interesting to loose time calculating PLL values at each boot when these value can be fixed in the board config file ?
Eric
Jean-Christophe PLAGNIOL-VILLARD wrote:
I've send a new patch that introduce an improve of the clock on at91 which give you the calculated main_clock based on the main crystal and the lowlevel settings
I was looking for that patch in git://git.denx.de/u-boot-at91.git, but couldn't find it there (it should add a new file cpu/arm926ejs/at91/clock.c, correct?). Where can I find that patch?
Best Regards, Detlef
Detlef Vollmann a écrit :
Jean-Christophe PLAGNIOL-VILLARD wrote:
I've send a new patch that introduce an improve of the clock on at91 which give you the calculated main_clock based on the main crystal and the lowlevel settings
I was looking for that patch in git://git.denx.de/u-boot-at91.git, but couldn't find it there (it should add a new file cpu/arm926ejs/at91/clock.c, correct?). Where can I find that patch?
http://article.gmane.org/gmane.comp.boot-loaders.u-boot/56837
Is it interesting to loose time calculating PLL values at each boot when these value can be defined in the board config file ?
Eric
On 15:20 Fri 03 Apr , Eric BENARD wrote:
Detlef Vollmann a écrit :
Jean-Christophe PLAGNIOL-VILLARD wrote:
I've send a new patch that introduce an improve of the clock on at91 which give you the calculated main_clock based on the main crystal and the lowlevel settings
I was looking for that patch in git://git.denx.de/u-boot-at91.git, but couldn't find it there (it should add a new file cpu/arm926ejs/at91/clock.c, correct?). Where can I find that patch?
http://article.gmane.org/gmane.comp.boot-loaders.u-boot/56837
Is it interesting to loose time calculating PLL values at each boot when these value can be defined in the board config file ?
yes because you will not loose that much 1ms in the worse case but you will be sure about it evenif you change it in the at91bootstrap change or the lowlevel_init. It's safer
Best Regards, J.
Jean-Christophe PLAGNIOL-VILLARD a écrit :
On 15:20 Fri 03 Apr , Eric BENARD wrote:
Detlef Vollmann a écrit :
Jean-Christophe PLAGNIOL-VILLARD wrote:
I've send a new patch that introduce an improve of the clock on at91 which give you the calculated main_clock based on the main crystal and the lowlevel settings
I was looking for that patch in git://git.denx.de/u-boot-at91.git, but couldn't find it there (it should add a new file cpu/arm926ejs/at91/clock.c, correct?). Where can I find that patch?
http://article.gmane.org/gmane.comp.boot-loaders.u-boot/56837
This patch is not adressing the problem of the network timeout. In fact Detlef's patch is still needed (with some rework for G20) to fix this tiemout problem.
Eric
Jean-Christophe PLAGNIOL-VILLARD wrote:
On 15:20 Fri 03 Apr , Eric BENARD wrote:
Detlef Vollmann a écrit :
Where can I find that patch?
http://article.gmane.org/gmane.comp.boot-loaders.u-boot/56837
Thanks.
Is it interesting to loose time calculating PLL values at each boot when these value can be defined in the board config file ?
yes because you will not loose that much 1ms in the worse case but you will be sure about it evenif you change it in the at91bootstrap change or the lowlevel_init. It's safer
I probably agree. (Well, not completely, as I'd like to get rid of at91bootstrap completely and let U-Boot be the original boot code, but that's future.)
Unfortunately that clock patch doesn't solve the timer issue, and makes it even worse... And I agree that it's not very useful to apply my old patch against that. The point is that I don't know against which git repository I should create any new patch. I'd prefer to use git://git.denx.de/u-boot.git, but I'm willing to use git://git.denx.de/u-boot-at91.git, but neither of those contain the clock patch yet...
Best Regards, Detlef
On 16:46 Fri 03 Apr , Detlef Vollmann wrote:
Jean-Christophe PLAGNIOL-VILLARD wrote:
On 15:20 Fri 03 Apr , Eric BENARD wrote:
Detlef Vollmann a écrit :
Where can I find that patch?
http://article.gmane.org/gmane.comp.boot-loaders.u-boot/56837
Thanks.
Is it interesting to loose time calculating PLL values at each boot when these value can be defined in the board config file ?
yes because you will not loose that much 1ms in the worse case but you will be sure about it evenif you change it in the at91bootstrap change or the lowlevel_init. It's safer
I probably agree. (Well, not completely, as I'd like to get rid of at91bootstrap completely and let U-Boot be the original boot code, but that's future.)
I've a patch that I'll send tomorow that boot the at91sam9263ek from NOR without the AT91bootstrap
Unfortunately that clock patch doesn't solve the timer issue, and makes it even worse... And I agree that it's not very useful to apply my old patch against that. The point is that I don't know against which git repository I should create any new patch. I'd prefer to use git://git.denx.de/u-boot.git, but I'm willing to use git://git.denx.de/u-boot-at91.git, but neither of those contain the clock patch yet...
I'll put in the clock branch
Best Regards, J.
participants (5)
-
Detlef Vollmann -
Eric BENARD -
Jean-Christophe PLAGNIOL-VILLARD -
Sergey Lapin -
Wolfgang Denk