[U-Boot] [PATCH 0/4] get at91rm9200ek working with ARM relocation

[U-Boot] [PATCH 0/4] get at91rm9200ek working with ARM relocation

[Andreas Bießmann]

This patch series enables at91rm9200ek board to work with new ARM relocation.
Unfortunately the board does _not_ fully work, there is one issue left I can't
figure out currently. When booting from pre-initialised clock and RAM (e.g.
by JTAG dongle or preloader) the relocation works.
But when booting from NOR flash the board hangs just after
'relocation Offset is: 11f7f000'. I think it is the same issue with using bss
before relocation but couldn't find the loacation til now. I don't know if
I get it done til release of v2010.12 but like to have the current changes in.

The series includes two patches already in Reinhard's u-boot-atmel/at91-next
branch but the base of at91-next is outdated and they will not apply cleanly.
So feel free to include them or not. Reinhard, any comments?
The mentioned patches are:

  at91rm9200ek: add configure target for RAM boot
  MAKEALL: fix AT91

Beside that the 'add configure target for RAM boot' is needed for working
configuration as ram-boot since nor-boot is currently broken ;)

Andreas Bie?mann (4):
  at91rm9200ek: add configure target for RAM boot
  MAKEALL: fix AT91
  arm920t: fix linker skript for -pie linking
  arm920t/at91/timer: replace bss variables by gd

 MAKEALL                            |   18 +++---------------
 arch/arm/cpu/arm920t/at91/timer.c  |   29 ++++++++++++++---------------
 arch/arm/cpu/arm920t/u-boot.lds    |    3 +++
 board/atmel/at91rm9200ek/config.mk |    2 --
 boards.cfg                         |    3 ++-
 include/configs/at91rm9200ek.h     |   16 ++++++++++++++++
 6 files changed, 38 insertions(+), 33 deletions(-)
 delete mode 100644 board/atmel/at91rm9200ek/config.mk

-- 
1.7.3.2

[U-Boot] [PATCH 1/4] at91rm9200ek: add configure target for RAM boot

[Andreas Bießmann]

This patch also removes now unnecessary config.mk in board directory and
make usage of new features in boards.cfg.

Signed-off-by: Andreas Bie?mann <andreas.devel@googlemail.com>
---
 board/atmel/at91rm9200ek/config.mk |    2 --
 boards.cfg                         |    3 ++-
 include/configs/at91rm9200ek.h     |   14 ++++++++++++++
 3 files changed, 16 insertions(+), 3 deletions(-)
 delete mode 100644 board/atmel/at91rm9200ek/config.mk

diff --git a/board/atmel/at91rm9200ek/config.mk b/board/atmel/at91rm9200ek/config.mk
deleted file mode 100644
index c7323fe..0000000
--- a/board/atmel/at91rm9200ek/config.mk
+++ /dev/null
@@ -1,2 +0,0 @@
-# currently only NOR flash booting is supported
-CONFIG_SYS_TEXT_BASE = 0x10000000
diff --git a/boards.cfg b/boards.cfg
index 2209676..19ff9c5 100644
--- a/boards.cfg
+++ b/boards.cfg
@@ -48,7 +48,8 @@ lpc2292sodimm                arm         arm720t     -                   -
 SMN42                        arm         arm720t     -                   siemens        lpc2292
 evb4510                      arm         arm720t     -                   -              s3c4510b
 a320evb                      arm         arm920t     -                   faraday        a320
-at91rm9200ek                 arm         arm920t     -                   atmel          at91
+at91rm9200ek                 arm         arm920t     at91rm9200ek        atmel          at91        at91rm9200ek
+at91rm9200ek_ram             arm         arm920t     at91rm9200ek        atmel          at91        at91rm9200ek:RAMBOOT
 eb_cpux9k2                   arm         arm920t     -                   BuS            at91
 cmc_pu2                      arm         arm920t     -                   -              at91rm9200
 csb637                       arm         arm920t     -                   -              at91rm9200
diff --git a/include/configs/at91rm9200ek.h b/include/configs/at91rm9200ek.h
index 14559f5..ba2e9d3 100644
--- a/include/configs/at91rm9200ek.h
+++ b/include/configs/at91rm9200ek.h
@@ -33,6 +33,20 @@
 #include <asm/sizes.h>
 
 /*
+ * set some initial configurations depending on configure target
+ *
+ * at91rm9200ek_config     -> boot from 0x0 in NOR Flash at CS0
+ * at91rm9200ek_ram_config -> continue booting from 0x20100000 in RAM; lowlevel
+ *                            initialisation was done by some preloader
+ */
+#ifdef CONFIG_RAMBOOT
+#define CONFIG_SKIP_LOWLEVEL_INIT
+#define CONFIG_SYS_TEXT_BASE 0x20100000
+#else
+#define CONFIG_SYS_TEXT_BASE 0x10000000
+#endif
+
+/*
  * AT91C_XTAL_CLOCK is the frequency of external xtal in hertz
  * AT91C_MAIN_CLOCK is the frequency of PLLA output
  * AT91C_MASTER_CLOCK is the peripherial clock
-- 
1.7.3.2

[U-Boot] [PATCH 2/4] MAKEALL: fix AT91

[Andreas Bießmann]

 * add boards_by_soc()
 * remove boards already in boards.cfg from LIST_AT91

Signed-off-by: Andreas Bie?mann <andreas.devel@googlemail.com>
---
 MAKEALL |   18 +++---------------
 1 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/MAKEALL b/MAKEALL
index 767d561..4254565 100755
--- a/MAKEALL
+++ b/MAKEALL
@@ -187,6 +187,7 @@ boards_by_field()
 }
 boards_by_arch() { boards_by_field 2 "$@" ; }
 boards_by_cpu()  { boards_by_field 3 "$@" ; }
+boards_by_soc()  { boards_by_field 6 "$@" ; }
 
 #########################################################################
 ## MPC5xx Systems
@@ -439,11 +440,8 @@ LIST_ARMV7="		\
 ## AT91 Systems
 #########################################################################
 
-LIST_at91="			\
-	afeb9260		\
-	at91cap9adk		\
-	at91rm9200dk		\
-	at91rm9200ek		\
+LIST_at91="$(boards_by_soc at91)\
+	$(boards_by_soc at91rm9200)\
 	at91sam9260ek		\
 	at91sam9261ek		\
 	at91sam9263ek		\
@@ -451,19 +449,9 @@ LIST_at91="			\
 	at91sam9g20ek		\
 	at91sam9m10g45ek	\
 	at91sam9rlek		\
-	cmc_pu2			\
 	CPUAT91			\
 	CPU9260			\
 	CPU9G20			\
-	csb637			\
-	eb_cpux9k2		\
-	kb9202			\
-	meesc			\
-	mp2usb			\
-	m501sk			\
-	otc570			\
-	pm9261			\
-	pm9263			\
 	pm9g45			\
 	SBC35_A9G20		\
 	TNY_A9260		\
-- 
1.7.3.2

[U-Boot] [PATCH 3/4] arm920t: fix linker skript for -pie linking

[Andreas Bießmann]

Without this patch the linker will SEGFAULT on some undefined weak
symbols.

Suggested-by: Sebastien Carlier <sebastien.carlier@gmail.com>
Signed-off-by: Andreas Bie?mann <andreas.devel@googlemail.com>
---
 arch/arm/cpu/arm920t/u-boot.lds |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/arm/cpu/arm920t/u-boot.lds b/arch/arm/cpu/arm920t/u-boot.lds
index a6f8b56..d8b77df 100644
--- a/arch/arm/cpu/arm920t/u-boot.lds
+++ b/arch/arm/cpu/arm920t/u-boot.lds
@@ -43,6 +43,9 @@ SECTIONS
 		*(.text)
 	}
 
+	.plt : { *(.plt) }
+	.rel.plt : { *(.rel.plt) }
+
 	. = ALIGN(4);
 	.rodata : { *(SORT_BY_ALIGNMENT(SORT_BY_NAME(.rodata*))) }
 
-- 
1.7.3.2

[U-Boot] [PATCH 3/4] arm920t: fix linker skript for -pie linking

[Wolfgang Denk]

Dear =?UTF-8?q?Andreas=20Bie=C3=9Fmann?=,

In message <1291099039-49672-4-git-send-email-andreas.devel@googlemail.com> you wrote:
> Without this patch the linker will SEGFAULT on some undefined weak
> symbols.
> 
> Suggested-by: Sebastien Carlier <sebastien.carlier@gmail.com>
> Signed-off-by: Andreas Bie?mann <andreas.devel@googlemail.com>
> ---
>  arch/arm/cpu/arm920t/u-boot.lds |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
They say a little knowledge is a dangerous thing,  but it is not  one
half so bad as a lot of ignorance.   - Terry Pratchett, _Equal Rites_

[U-Boot] [PATCH 3/4] arm920t: fix linker skript for -pie linking

[Andreas Bießmann]

Dear Wolfgang Denk,

Am 08.12.2010 um 23:52 schrieb Wolfgang Denk:

> Dear =?UTF-8?q?Andreas=20Bie=C3=9Fmann?=,
> 
> In message <1291099039-49672-4-git-send-email-andreas.devel@googlemail.com> you wrote:
>> Without this patch the linker will SEGFAULT on some undefined weak
>> symbols.
>> 
>> Suggested-by: Sebastien Carlier <sebastien.carlier@gmail.com>
>> Signed-off-by: Andreas Bie?mann <andreas.devel@googlemail.com>
>> ---
>> arch/arm/cpu/arm920t/u-boot.lds |    3 +++
>> 1 files changed, 3 insertions(+), 0 deletions(-)
> 
> Applied, thanks.

Please do not apply that patch! I'm sorry I have split up the thread cause it lead to a generic timer discussion. Therefore I posted v2 of this patch series here:
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/89974

This patch is not needed, if no undefined weak symbol is available.
This patch adds .plt/.rel.plt section which has R_ARM_JUMP_SLOT which is not handled in code. Therefore we do not really know what this patch does to the code, it was mostly a patch to handle the linker segfault described several times.

regards

Andreas Bie?mann

[U-Boot] [PATCH 3/4] arm920t: fix linker skript for -pie linking

[Albert ARIBAUD]

Le 09/12/2010 08:24, Andreas Bie?mann a ?crit :
> Dear Wolfgang Denk,
>
> Am 08.12.2010 um 23:52 schrieb Wolfgang Denk:
>
>> Dear =?UTF-8?q?Andreas=20Bie=C3=9Fmann?=,
>>
>> In message<1291099039-49672-4-git-send-email-andreas.devel@googlemail.com>  you wrote:
>>> Without this patch the linker will SEGFAULT on some undefined weak
>>> symbols.
>>>
>>> Suggested-by: Sebastien Carlier<sebastien.carlier@gmail.com>
>>> Signed-off-by: Andreas Bie?mann<andreas.devel@googlemail.com>
>>> ---
>>> arch/arm/cpu/arm920t/u-boot.lds |    3 +++
>>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> Applied, thanks.
>
> Please do not apply that patch! I'm sorry I have split up the thread cause it lead to a generic timer discussion. Therefore I posted v2 of this patch series here:
> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/89974
>
> This patch is not needed, if no undefined weak symbol is available.
> This patch adds .plt/.rel.plt section which has R_ARM_JUMP_SLOT which is not handled in code. Therefore we do not really know what this patch does to the code, it was mostly a patch to handle the linker segfault described several times.

Thanks for pointing this out. My bad: I did not relate the two series 
when going through patchwork.

Wolfgang, I was about to send out a revert but after pulling u-boot.git 
and u-boot-arm.git I don't see Andreas' patch in there. If you haven't 
committed it yet, maybe you can fix this yourself?

> regards
>
> Andreas Bie?mann

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 3/4] arm920t: fix linker skript for -pie linking

[Wolfgang Denk]

Dear Albert ARIBAUD,

In message <4D008638.4060004@aribaud.net> you wrote:
>
> > This patch is not needed, if no undefined weak symbol is available.
> > This patch adds .plt/.rel.plt section which has R_ARM_JUMP_SLOT which is not handled in code. Therefore we do not really know what this patch does to the code, it was mostly a patch to handle the linker segfault described several times.
> 
> Thanks for pointing this out. My bad: I did not relate the two series 
> when going through patchwork.
> 
> Wolfgang, I was about to send out a revert but after pulling u-boot.git 
> and u-boot-arm.git I don't see Andreas' patch in there. If you haven't 
> committed it yet, maybe you can fix this yourself?

Hm... strange...

I am absolutely sure that I did apply these patches last night, and
then I even pushed them out... ... but now I see no trace of this
either.

Don't know what happened (I should learn this lesson - this is what I
get when _not_ having a beer ;-)

Ummm... now I have a problem, because I have marked all these patches
as "applied", both in patchwork and in my mail reader. This actually
means that I have completely lost track of what is missing and which
patches should be applied :-(

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
You can do this in a number of ways.     IBM chose to do all of them.
Why do you find that funny?        -- D. Taylor, Computer Science 350

[U-Boot] [PATCH 3/4] arm920t: fix linker skript for -pie linking

[Wolfgang Denk]

Dear Wolfgang Denk,

In message <20101209094538.7B9BBD08A93@gemini.denx.de> you wrote:
>
> Hm... strange...
> 
> I am absolutely sure that I did apply these patches last night, and
> then I even pushed them out... ... but now I see no trace of this
> either.
> 
> Don't know what happened (I should learn this lesson - this is what I
> get when _not_ having a beer ;-)
> 
> Ummm... now I have a problem, because I have marked all these patches
> as "applied", both in patchwork and in my mail reader. This actually
> means that I have completely lost track of what is missing and which
> patches should be applied :-(

OK, I found the stuff. I have applied to my working copy of the master
branch instesd of ARM.

So only the "-pie" commit needs to be removed?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"We don't care.  We don't have to.  We're the Phone Company."

[U-Boot] [PATCH 3/4] arm920t: fix linker skript for -pie linking

[Andreas Bießmann]

Dear Wolfgang Denk,

Am 09.12.2010 11:32, schrieb Wolfgang Denk:
> Dear Wolfgang Denk,
> 
> In message <20101209094538.7B9BBD08A93@gemini.denx.de> you wrote:

> 
> OK, I found the stuff. I have applied to my working copy of the master
> branch instesd of ARM.
> 
> So only the "-pie" commit needs to be removed?

All of this series should not be applied. Two of them already upstream
in u-boot-arm/master.
Namely:
 - 0a41edaabb74f2ccfde62a232165088098763e3e MAKEALL: fix AT91
 - 3a4ff8b3cd719372cb3b3a8432e68015d84f1fc2 at91rm9200ek: add configure
target for RAM boot

One of them is modified already in upstream u-boot-arm/master:
 - a429db7e3ce6136f80f22584588247926ba60b05 arm920t/at91/timer: replace
bss variables by gd

And last one of them (-pie) is superseeded by another one also already
in u-boot-arm/master:
 - 305bf489d1e7dd70f45720720ae0066fcce3acb1
arm920t/at91/reset: board_reset: define weak symbol


BTW: All the mentioned hashes miss Signed-off of custodians. Is that
correct?

regards

Andreas Bie?mann

[U-Boot] [PATCH 4/4] arm920t/at91/timer: replace bss variables by gd

[Andreas Bießmann]

Reuse the gd->tbl/tbu values for timestamp/lastinc bss values in
arm920t/at91/timer driver.
The usage of bss values in driver before initialisation of bss is
forbidden. In that special case some data in .rel.dyn gets corrupted by
the arm920t/at91/timer driver.

Signed-off-by: Andreas Bie?mann <andreas.devel@googlemail.com>
---
 arch/arm/cpu/arm920t/at91/timer.c |   29 ++++++++++++++---------------
 include/configs/at91rm9200ek.h    |    2 ++
 2 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/arch/arm/cpu/arm920t/at91/timer.c b/arch/arm/cpu/arm920t/at91/timer.c
index 91377d4..2a7cd9b 100644
--- a/arch/arm/cpu/arm920t/at91/timer.c
+++ b/arch/arm/cpu/arm920t/at91/timer.c
@@ -32,17 +32,16 @@
 
 #include <common.h>
 
-#include <asm/io.h>
-#include <asm/hardware.h>
+#include <asm/arch/io.h>
+#include <asm/arch/hardware.h>
 #include <asm/arch/at91_tc.h>
 #include <asm/arch/at91_pmc.h>
 
+DECLARE_GLOBAL_DATA_PTR;
+
 /* the number of clocks per CONFIG_SYS_HZ */
 #define TIMER_LOAD_VAL (CONFIG_SYS_HZ_CLOCK/CONFIG_SYS_HZ)
 
-static u32 timestamp;
-static u32 lastinc;
-
 int timer_init(void)
 {
 	at91_tc_t *tc = (at91_tc_t *) AT91_TC_BASE;
@@ -64,8 +63,8 @@ int timer_init(void)
 	writel(TIMER_LOAD_VAL, &tc->tc[0].rc);
 
 	writel(AT91_TC_CCR_SWTRG | AT91_TC_CCR_CLKEN, &tc->tc[0].ccr);
-	lastinc = 0;
-	timestamp = 0;
+	gd->tbu = 0;
+	gd->tbl = 0;
 
 	return 0;
 }
@@ -86,7 +85,7 @@ ulong get_timer(ulong base)
 
 void set_timer(ulong t)
 {
-	timestamp = t;
+	gd->tbu = t;
 }
 
 void __udelay(unsigned long usec)
@@ -98,8 +97,8 @@ void reset_timer_masked(void)
 {
 	/* reset time */
 	at91_tc_t *tc = (at91_tc_t *) AT91_TC_BASE;
-	lastinc = readl(&tc->tc[0].cv) & 0x0000ffff;
-	timestamp = 0;
+	gd->tbl = readl(&tc->tc[0].cv) & 0x0000ffff;
+	gd->tbu = 0;
 }
 
 ulong get_timer_raw(void)
@@ -109,16 +108,16 @@ ulong get_timer_raw(void)
 
 	now = readl(&tc->tc[0].cv) & 0x0000ffff;
 
-	if (now >= lastinc) {
+	if (now >= gd->tbl) {
 		/* normal mode */
-		timestamp += now - lastinc;
+		gd->tbu += now - gd->tbl;
 	} else {
 		/* we have an overflow ... */
-		timestamp += now + TIMER_LOAD_VAL - lastinc;
+		gd->tbu += now + TIMER_LOAD_VAL - gd->tbl;
 	}
-	lastinc = now;
+	gd->tbl = now;
 
-	return timestamp;
+	return gd->tbu;
 }
 
 ulong get_timer_masked(void)
diff --git a/include/configs/at91rm9200ek.h b/include/configs/at91rm9200ek.h
index ba2e9d3..57e17e9 100644
--- a/include/configs/at91rm9200ek.h
+++ b/include/configs/at91rm9200ek.h
@@ -71,6 +71,8 @@
 #define CONFIG_SETUP_MEMORY_TAGS
 #define CONFIG_INITRD_TAG
 
+#define CONFIG_AT91FAMILY
+
 /*
  * Memory Configuration
  */
-- 
1.7.3.2

[U-Boot] [PATCH 4/4] arm920t/at91/timer: replace bss variables by gd

[Reinhard Meyer]

Dear Andreas Bie?mann,
> Reuse the gd->tbl/tbu values for timestamp/lastinc bss values in
> arm920t/at91/timer driver.
> The usage of bss values in driver before initialisation of bss is
> forbidden. In that special case some data in .rel.dyn gets corrupted by
> the arm920t/at91/timer driver.
> 
> Signed-off-by: Andreas Bie?mann <andreas.devel@googlemail.com>
> ---
>  arch/arm/cpu/arm920t/at91/timer.c |   29 ++++++++++++++---------------
>  {
>  	/* reset time */
>  	at91_tc_t *tc = (at91_tc_t *) AT91_TC_BASE;
> -	lastinc = readl(&tc->tc[0].cv) & 0x0000ffff;
> -	timestamp = 0;
> +	gd->tbl = readl(&tc->tc[0].cv) & 0x0000ffff;
> +	gd->tbu = 0;
>  }
>  
>  ulong get_timer_raw(void)
> @@ -109,16 +108,16 @@ ulong get_timer_raw(void)
>  
>  	now = readl(&tc->tc[0].cv) & 0x0000ffff;
>  
> -	if (now >= lastinc) {
> +	if (now >= gd->tbl) {
>  		/* normal mode */
> -		timestamp += now - lastinc;
> +		gd->tbu += now - gd->tbl;
>  	} else {
>  		/* we have an overflow ... */
> -		timestamp += now + TIMER_LOAD_VAL - lastinc;
> +		gd->tbu += now + TIMER_LOAD_VAL - gd->tbl;
>  	}
> -	lastinc = now;
> +	gd->tbl = now;
>  
> -	return timestamp;
> +	return gd->tbu;
>  }

I see your dilemma here.

tbu/tbl were introduced by me to form a true 64 bit monotonous incrementing value
(like on most powerPC).
You use tbl as the last (16 bit) value of the 16 bit hardware timer and
tbu as the actual, only 32 bits worth time value.
If the rest of the timer functions handle this correctly (I doubt that, but I cannot look at
that right now), that "abuse" might be OK.
But I rather have a field, say "u32 last_hw_val" (or a better name) added to the GD inside the
AT91FAMILY define and have tbu/tbl really be a functional 64 bit value.

That would also ease a later unification/factoring of all AT91 timer sources:
A small SoC dependant part that just makes sure tbu/tbl increment with a high frequency,
and a common parts that derives udelay() and get_timer() from that. We should not need any
other functions like *_masked or reset_timer() in the future.

I know there is an endless but not leading anywhere discussion about timers ongoing, but no clean
solution exists right now because of the messed up ways the many different timer functions are used
throughout u-boot.

I would like to reduce it to as few functions as possible...

I am working on a proposal for that, but since that untimately requires to fix *ALL* existing timer
uses I see little change that we will ever get to clean up the mess.

Best Regards,
Reinhard

[U-Boot] [PATCH 4/4] arm920t/at91/timer: replace bss variables by gd

[Andreas Bießmann]

Dear Reinhard Meyer,

Am 30.11.2010 08:17, schrieb Reinhard Meyer:
> Dear Andreas Bie?mann,
>> Reuse the gd->tbl/tbu values for timestamp/lastinc bss values in
>> arm920t/at91/timer driver.

> I see your dilemma here.
> 
> tbu/tbl were introduced by me to form a true 64 bit monotonous incrementing value
> (like on most powerPC).
> You use tbl as the last (16 bit) value of the 16 bit hardware timer and
> tbu as the actual, only 32 bits worth time value.
> If the rest of the timer functions handle this correctly (I doubt that, but I cannot look at
> that right now), that "abuse" might be OK.
> But I rather have a field, say "u32 last_hw_val" (or a better name) added to the GD inside the
> AT91FAMILY define and have tbu/tbl really be a functional 64 bit value.

[snip]

I think get your point.

To get this bss issue fixed for v2010.12 I'd like to add another value
to GD to hold the last hw timer value. My current usage of tbu should
therefore go to tbl, to have a virutal 64 bit value just counting 32
bit, is that right?

Later on I also see some work in that arm920t/at91 SoC 'driver' section.
E.g. we really do need something like arm926ejs/at91/clock to have the
usart driver merged. And I might have some other changes to the timer
part of arm920t/at91. I found out the at91rm9200 do also have a PIT ...
but that is for later discussion.

regards

Andreas Bie?mann

[U-Boot] [PATCH 4/4] arm920t/at91/timer: replace bss variables by gd

[Wolfgang Denk]

Dear "=?UTF-8?B?QW5kcmVhcyBCaWXDn21hbm4=?=",

In message <4CF4AFED.1010609@gmail.com> you wrote:
> 
> To get this bss issue fixed for v2010.12 I'd like to add another value
> to GD to hold the last hw timer value. My current usage of tbu should
> therefore go to tbl, to have a virutal 64 bit value just counting 32
> bit, is that right?

That sounds like a terrible mess to me, please do not do that. Either
we have a 64 bit counter, hen it should cound the full 64 bit range.
Or use a plain uint32_t if 32 bit are sfficient. Don't play any
tricks like misusing an "unused" part of one variable for other,
independent purposes.

I think we should provide a "uint64_t timebase" which represents a
real 64 bit counter. And if you need a separate "uint32_t timelast"
to store the previous timer value then please make this a separate
variable.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Of all possible committee reactions to any  given  agenda  item,  the
reaction  that will occur is the one which will liberate the greatest
amount of hot air.                                -- Thomas L. Martin

[U-Boot] [PATCH 4/4] arm920t/at91/timer: replace bss variables by gd

[Andreas Bießmann]

Dear Wolfgang Denk,

Am 30.11.2010 09:16, schrieb Wolfgang Denk:
> Dear "=?UTF-8?B?QW5kcmVhcyBCaWXDn21hbm4=?=",
> 
> In message <4CF4AFED.1010609@gmail.com> you wrote:
>>
>> To get this bss issue fixed for v2010.12 I'd like to add another value
>> to GD to hold the last hw timer value. My current usage of tbu should
>> therefore go to tbl, to have a virutal 64 bit value just counting 32
>> bit, is that right?
> 
> That sounds like a terrible mess to me, please do not do that. Either
> we have a 64 bit counter, hen it should cound the full 64 bit range.
> Or use a plain uint32_t if 32 bit are sfficient. Don't play any
> tricks like misusing an "unused" part of one variable for other,
> independent purposes.

If I got Reinhard correct the uint32 values tbl and tbu do build the
Upper and Lower part of an virtual 64 bit counter. His statement is to
not use these values in another context like I did (32 bit timestamp and
32 bit 'lastinc' which is really 16 bit). Therefore I additionally
suggested to have the idea with virtual 64 bit reproduced for at91rm9200
and use the lower part for timestamp.

> I think we should provide a "uint64_t timebase" which represents a
> real 64 bit counter. And if you need a separate "uint32_t timelast"
> to store the previous timer value then please make this a separate
> variable.

As you suggested in another thread those changes will go into next
branch. So for v2010.12 I'd like to have a 'short time' solution to get
the at91rm9200 boards working. Is it OK to use the tbu/tbl values as
mentioned above and then migrate to a uint64_t timebase later on?

regards

Andreas Bie?mann

[U-Boot] TIMER cleanup RFC, was: [PATCH 4/4] arm920t/at91/timer: replace bss variables by gd

[Reinhard Meyer]

Dear Wolfgang Denk,

what we really need is only a 32 bit monotonous free running tick that increments
at a rate of at least 1 MHz. As someone pointed out a while ago, even at 1GHz that would
last for four seconds before it rolls over. But a 1HGz counter could be 64 bit internally
and always be returned as 32 bits when it is shifted right to bring it into the "few" MHz
range.

Any architecture should be able to provide such a 32 bit value. On powerpc that would
simply be tbu|tbl shifted right a few bits.

An architecture and SoC specific timer should export 3 functions:

int timer_init(void);
u32 get_tick(void); /* return the current value of the internal free running counter */
u32 get_tbclk(void); /* return the rate at which that counter increments (per second) */

A generic timer function common to *most* architectures and SoCs would use those two
functions to provice udelay() and reset_timer() and get_timer().
Any other timer functions should not be required in u-boot anymore.

However get_timer() and reset_timer() are a bit of a functional problem:

currently reset_timer() does either actually RESET the free running timer (BAD!) or
remember its current value in another (gd-)static variable which later is subtracted
when get_timer() is called. That precludes the use of several timers concurrently.

Also, since the 1000Hz base for that timer is usually derived from get_tick() by
dividing it by some value, the resulting 1000Hz base is not exhausting the 32 bits
before it wraps to zero.

Therefore I propose two new functions that are to replace reset_timer() and get_timer():

u32 init_timeout(u32 timeout_in_ms); /* return the 32 bit tick value when the timeout will be */
bool is_timeout(u32 reference); /* return true if reference is in the past */

A timeout loop would therefore be like:

u32 t_ref = timeout_init(3000);	/* init a 3 second timeout */

do ... loop ... while (!is_timeout(t_ref));

coarse sketches of those functions:

u32 init_timeout(u32 ms)
{
	return get_ticks() + ((u64)get_tbclk() * (u64)ms) / (u64)1000;
}

bool is_timeout(u32 reference)
{
	return ((int)get_ticks() - (int)reference) > 0;
}

Unfortunately this requires to "fix" all uses of get_timer() and friends, but I see no other
long term solution to the current incoherencies.

Comments welcome (and I would provide patches)... 

Reinhard

[U-Boot] TIMER cleanup RFC, was: [PATCH 4/4] arm920t/at91/timer: replace bss variables by gd

[J. William Campbell]

On 11/30/2010 1:14 AM, Reinhard Meyer wrote:
> Dear Wolfgang Denk,
>
> what we really need is only a 32 bit monotonous free running tick that increments
> at a rate of at least 1 MHz. As someone pointed out a while ago, even at 1GHz that would
> last for four seconds before it rolls over. But a 1HGz counter could be 64 bit internally
> and always be returned as 32 bits when it is shifted right to bring it into the "few" MHz
> range.
>
> Any architecture should be able to provide such a 32 bit value. On powerpc that would
> simply be tbu|tbl shifted right a few bits.
>
> An architecture and SoC specific timer should export 3 functions:
>
> int timer_init(void);
> u32 get_tick(void); /* return the current value of the internal free running counter */
> u32 get_tbclk(void); /* return the rate at which that counter increments (per second) */
>
> A generic timer function common to *most* architectures and SoCs would use those two
> functions to provice udelay() and reset_timer() and get_timer().
> Any other timer functions should not be required in u-boot anymore.
>
> However get_timer() and reset_timer() are a bit of a functional problem:
>
> currently reset_timer() does either actually RESET the free running timer (BAD!) or
> remember its current value in another (gd-)static variable which later is subtracted
> when get_timer() is called. That precludes the use of several timers concurrently.
>
> Also, since the 1000Hz base for that timer is usually derived from get_tick() by
> dividing it by some value, the resulting 1000Hz base is not exhausting the 32 bits
> before it wraps to zero.
>
> Therefore I propose two new functions that are to replace reset_timer() and get_timer():
>
> u32 init_timeout(u32 timeout_in_ms); /* return the 32 bit tick value when the timeout will be */
> bool is_timeout(u32 reference); /* return true if reference is in the past */
>
> A timeout loop would therefore be like:
>
> u32 t_ref = timeout_init(3000);	/* init a 3 second timeout */
>
> do ... loop ... while (!is_timeout(t_ref));
>
> coarse sketches of those functions:
>
> u32 init_timeout(u32 ms)
> {
> 	return get_ticks() + ((u64)get_tbclk() * (u64)ms) / (u64)1000;
> }
>
> bool is_timeout(u32 reference)
> {
> 	return ((int)get_ticks() - (int)reference)>  0;
> }
>
> Unfortunately this requires to "fix" all uses of get_timer() and friends, but I see no other
> long term solution to the current incoherencies.
>
> Comments welcome (and I would provide patches)...
Hi All,
       The idea of changing the get_timer interface to the 
init_timeout/is_timeout pair has the advantage that it is only necessary 
to change the delay time in ms to an internal timebase once, and after 
that, only a 32-bit subtraction is required. I do not however like the 
idea of using 64 bit math to do so, as on many systems this is quite 
expensive. However, this is a feature that can be optimized for 
particular CPUs. I also REALLY don't like the idea of having a get_ticks 
function, because for sure people will use this instead of the desired 
interface to the timer because it is "better". Then we get back into a 
mess. Since in most cases get_ticks is one or two instructions, please, 
let us hide them in init_timeout/is_timeout.
        An alternate approach, which has the merit of being more like 
the originally intended interface, simply disallows reset_timer since it 
is  totally unnecessary. The only dis-advantage of the original approach 
using just get_timer is that the conversion to ms must be considered at 
each call to get_timer, and will require at a minimum one 32 bit integer 
to remember the hardware timer value the last time get_timer was called 
(unless the hardware time can be trivially converted to a 32 bit value 
in ms, which is quite uncommon). This is not a high price to pay, and 
matches the current usage. This is probably for Mr. Denk to decide. If 
we were just starting now, the init_timeout/is_timeout is simpler, but 
since we are not, perhaps keeping the current approach has value.
        I would really like to help by providing some patches, but I am 
just way too busy at present.

Best Regards,
Bill Campbell
> Reinhard
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
>

[U-Boot] TIMER cleanup RFC, was: [PATCH 4/4] arm920t/at91/timer: replace bss variables by gd

[Reinhard Meyer]

Dear J. William Campbell,
> On 11/30/2010 1:14 AM, Reinhard Meyer wrote:
>> Dear Wolfgang Denk,
>>
>> what we really need is only a 32 bit monotonous free running tick that
>> increments
>> at a rate of at least 1 MHz. As someone pointed out a while ago, even
>> at 1GHz that would
>> last for four seconds before it rolls over. But a 1HGz counter could
>> be 64 bit internally
>> and always be returned as 32 bits when it is shifted right to bring it
>> into the "few" MHz
>> range.
>>
>> Any architecture should be able to provide such a 32 bit value. On
>> powerpc that would
>> simply be tbu|tbl shifted right a few bits.
>>
>> An architecture and SoC specific timer should export 3 functions:
>>
>> int timer_init(void);
>> u32 get_tick(void); /* return the current value of the internal free
>> running counter */
>> u32 get_tbclk(void); /* return the rate at which that counter
>> increments (per second) */
>>
>> A generic timer function common to *most* architectures and SoCs would
>> use those two
>> functions to provice udelay() and reset_timer() and get_timer().
>> Any other timer functions should not be required in u-boot anymore.
>>
>> However get_timer() and reset_timer() are a bit of a functional problem:
>>
>> currently reset_timer() does either actually RESET the free running
>> timer (BAD!) or
>> remember its current value in another (gd-)static variable which later
>> is subtracted
>> when get_timer() is called. That precludes the use of several timers
>> concurrently.
>>
>> Also, since the 1000Hz base for that timer is usually derived from
>> get_tick() by
>> dividing it by some value, the resulting 1000Hz base is not exhausting
>> the 32 bits
>> before it wraps to zero.
(###)
>>
>> Therefore I propose two new functions that are to replace
>> reset_timer() and get_timer():
>>
>> u32 init_timeout(u32 timeout_in_ms); /* return the 32 bit tick value
>> when the timeout will be */
>> bool is_timeout(u32 reference); /* return true if reference is in the
>> past */
>>
>> A timeout loop would therefore be like:
>>
>> u32 t_ref = timeout_init(3000);    /* init a 3 second timeout */
>>
>> do ... loop ... while (!is_timeout(t_ref));
>>
>> coarse sketches of those functions:
>>
>> u32 init_timeout(u32 ms)
>> {
>>     return get_ticks() + ((u64)get_tbclk() * (u64)ms) / (u64)1000;
>> }
>>
>> bool is_timeout(u32 reference)
>> {
>>     return ((int)get_ticks() - (int)reference)>  0;
>> }
>>
>> Unfortunately this requires to "fix" all uses of get_timer() and
>> friends, but I see no other
>> long term solution to the current incoherencies.
>>
>> Comments welcome (and I would provide patches)...
> Hi All,
>       The idea of changing the get_timer interface to the
> init_timeout/is_timeout pair has the advantage that it is only necessary
> to change the delay time in ms to an internal timebase once, and after
> that, only a 32-bit subtraction is required.
Exactly.
> I do not however like the
> idea of using 64 bit math to do so, as on many systems this is quite
> expensive. However, this is a feature that can be optimized for
> particular CPUs.
64 Bit math is only necessary when get_tbclk() times the maximun anticipated
timeout (in ms) is larger than 32 bits. This happens easyly when tbclk is a
few MHz. Besides the current working(!) implementations use 64 bit math
already. You can optimize into 32 bits by factoring the timeout_in_ms into
whole seconds, and the remainder.
> I also REALLY don't like the idea of having a get_ticks
> function, because for sure people will use this instead of the desired
> interface to the timer because it is "better". Then we get back into a
> mess. Since in most cases get_ticks is one or two instructions, please,
> let us hide them in init_timeout/is_timeout.
Agreed. However I intended to split the timer into two sources (read above):
one hardware dependant part exporting exactly those functions, and one
generic part using them.
>        An alternate approach, which has the merit of being more like the
> originally intended interface, simply disallows reset_timer since it is 
> totally unnecessary. The only dis-advantage of the original approach
> using just get_timer is that the conversion to ms must be considered at
> each call to get_timer, and will require at a minimum one 32 bit integer
> to remember the hardware timer value the last time get_timer was called
"remembering" that inside the timer function is exactly what I want to get
rid of. And that would not work in case of the usage method (2.) below.
> (unless the hardware time can be trivially converted to a 32 bit value
> in ms, which is quite uncommon).
Indeed.
> This is not a high price to pay, and
> matches the current usage. This is probably for Mr. Denk to decide. If
The "current usages" are mainly those two:
1. reset_timer(); ... if (get_timer(0) > TIMEOUT) ...
--> this is an ab-use, of course, but can be found in u-boot.
2. epoch = get_timer(0); ... if (get_timer(epoch) > TIMEOUT) ...
--> this is the correct way to use get_timer(), however it has the problem
of incorrect rollover after 32 bits. See (###) above!
> we were just starting now, the init_timeout/is_timeout is simpler, but
> since we are not, perhaps keeping the current approach has value.
Finding the current uses of get_timer() is a simple grep, fixing the code
should not require more than a few minutes per location. Testing, however
is a problem. But ANY change would require testing.
>        I would really like to help by providing some patches, but I am
> just way too busy at present.
Same here. Nevertheless some xmax holidays are coming up, and maybe some
idle hours in them ;)

Best Regards,
Reinhard

[U-Boot] TIMER cleanup RFC, was: [PATCH 4/4] arm920t/at91/timer: replace bss variables by gd

[J. William Campbell]

On 11/30/2010 7:48 AM, Reinhard Meyer wrote:
> Dear J. William Campbell,
>> On 11/30/2010 1:14 AM, Reinhard Meyer wrote:
>>> Dear Wolfgang Denk,
>>>
>>> what we really need is only a 32 bit monotonous free running tick that
>>> increments
>>> at a rate of at least 1 MHz. As someone pointed out a while ago, even
>>> at 1GHz that would
>>> last for four seconds before it rolls over. But a 1HGz counter could
>>> be 64 bit internally
>>> and always be returned as 32 bits when it is shifted right to bring it
>>> into the "few" MHz
>>> range.
>>>
>>> Any architecture should be able to provide such a 32 bit value. On
>>> powerpc that would
>>> simply be tbu|tbl shifted right a few bits.
>>>
>>> An architecture and SoC specific timer should export 3 functions:
>>>
>>> int timer_init(void);
>>> u32 get_tick(void); /* return the current value of the internal free
>>> running counter */
>>> u32 get_tbclk(void); /* return the rate at which that counter
>>> increments (per second) */
>>>
>>> A generic timer function common to *most* architectures and SoCs would
>>> use those two
>>> functions to provice udelay() and reset_timer() and get_timer().
>>> Any other timer functions should not be required in u-boot anymore.
>>>
>>> However get_timer() and reset_timer() are a bit of a functional problem:
>>>
>>> currently reset_timer() does either actually RESET the free running
>>> timer (BAD!) or
>>> remember its current value in another (gd-)static variable which later
>>> is subtracted
>>> when get_timer() is called. That precludes the use of several timers
>>> concurrently.
>>>
>>> Also, since the 1000Hz base for that timer is usually derived from
>>> get_tick() by
>>> dividing it by some value, the resulting 1000Hz base is not exhausting
>>> the 32 bits
>>> before it wraps to zero.
> (###)
Hi All,
       A correct method to provide a full 32 bits of resolution at a 1 
ms rate is simple. It requires maintaining a software timer in ms. This 
timer is updated by converting the difference in the hardware time base 
to ms and then adding the correct number of ms to the software timer.
As previously discussed, this only requires calling get_timer once a 
second or so. The conversion can be made simply in most cases, 
especially if the clock rate is a "nice" number. Even in the worst case, 
it is not too hard to "adjust" the saved hardware timer value to contain 
any remainder ticks. Yes, if this is done incorrectly, the results are bad.
>>> Therefore I propose two new functions that are to replace
>>> reset_timer() and get_timer():
>>>
>>> u32 init_timeout(u32 timeout_in_ms); /* return the 32 bit tick value
>>> when the timeout will be */
>>> bool is_timeout(u32 reference); /* return true if reference is in the
>>> past */
>>>
>>> A timeout loop would therefore be like:
>>>
>>> u32 t_ref = timeout_init(3000);    /* init a 3 second timeout */
>>>
>>> do ... loop ... while (!is_timeout(t_ref));
>>>
>>> coarse sketches of those functions:
>>>
>>> u32 init_timeout(u32 ms)
>>> {
>>>      return get_ticks() + ((u64)get_tbclk() * (u64)ms) / (u64)1000;
>>> }
>>>
>>> bool is_timeout(u32 reference)
>>> {
>>>      return ((int)get_ticks() - (int)reference)>   0;
>>> }
>>>
>>> Unfortunately this requires to "fix" all uses of get_timer() and
>>> friends, but I see no other
>>> long term solution to the current incoherencies.
>>>
>>> Comments welcome (and I would provide patches)...
>> Hi All,
>>        The idea of changing the get_timer interface to the
>> init_timeout/is_timeout pair has the advantage that it is only necessary
>> to change the delay time in ms to an internal timebase once, and after
>> that, only a 32-bit subtraction is required.
> Exactly.
>> I do not however like the
>> idea of using 64 bit math to do so, as on many systems this is quite
>> expensive. However, this is a feature that can be optimized for
>> particular CPUs.
> 64 Bit math is only necessary when get_tbclk() times the maximun anticipated
> timeout (in ms) is larger than 32 bits. This happens easyly when tbclk is a
> few MHz.
What I think you mean to say here is that the delay time fits easily 
into 32 bits for all reasonable clock rates. For instance, a 1 GHz clock 
rate yields a maximum delay of about 2 seconds in a 32 bit word?  For 
fast clock rates, it is probably advisable to shift the 64 bit counter 
right a few bits to ensure adequate range.
>   Besides the current working(!) implementations use 64 bit math
> already.
In ARM perhaps, but I think there are lots of working 32 bit in other 
processors(?).
>   You can optimize into 32 bits by factoring the timeout_in_ms into
> whole seconds, and the remainder.
True.
>> I also REALLY don't like the idea of having a get_ticks
>> function, because for sure people will use this instead of the desired
>> interface to the timer because it is "better". Then we get back into a
>> mess. Since in most cases get_ticks is one or two instructions, please,
>> let us hide them in init_timeout/is_timeout.
> Agreed. However I intended to split the timer into two sources (read above):
> one hardware dependant part exporting exactly those functions, and one
> generic part using them.
Yes, and that is EXACTLY what I think is a bad idea for the reason I 
mentioned. If the get_ticks interface is public, it will be used, and 
this will lead to problems (as it already has). Yes, we can say "don't 
do that", but people will do it anyway, and eventually it will sneak 
into the code base. Since the conversion to/from hardware resolution to 
1 ms resolution becomes CPU dependent if any optimization is used, this 
operation really needs to be a per CPU thing.
>>         An alternate approach, which has the merit of being more like the
>> originally intended interface, simply disallows reset_timer since it is
>> totally unnecessary. The only dis-advantage of the original approach
>> using just get_timer is that the conversion to ms must be considered at
>> each call to get_timer, and will require at a minimum one 32 bit integer
>> to remember the hardware timer value the last time get_timer was called
> "remembering" that inside the timer function is exactly what I want to get
> rid of. And that would not work in case of the usage method (2.) below.
>> (unless the hardware time can be trivially converted to a 32 bit value
>> in ms, which is quite uncommon).
> Indeed.
>> This is not a high price to pay, and
>> matches the current usage. This is probably for Mr. Denk to decide. If
> The "current usages" are mainly those two:
> 1. reset_timer(); ... if (get_timer(0)>  TIMEOUT) ...
> -->  this is an ab-use, of course, but can be found in u-boot.
Getting rid of reset_reset timer detects all such abuses.
> 2. epoch = get_timer(0); ... if (get_timer(epoch)>  TIMEOUT) ...
> -->  this is the correct way to use get_timer(), however it has the problem
> of incorrect rollover after 32 bits. See (###) above!
I assume by ### you mean that is get_timer(int x) is not based on a 
correct conversion of hardware time to a 1 ms timebase, there is a 
problem. 32 bits at 1 ms resolution is 49.7/2  days, so you can't mean 
this is the problem.
>> we were just starting now, the init_timeout/is_timeout is simpler, but
>> since we are not, perhaps keeping the current approach has value.
> Finding the current uses of get_timer() is a simple grep, fixing the code
> should not require more than a few minutes per location. Testing, however
> is a problem. But ANY change would require testing.
>>         I would really like to help by providing some patches, but I am
>> just way too busy at present.
> Same here. Nevertheless some xmax holidays are coming up, and maybe some
> idle hours in them ;)
I hope so!

Best Regards,
Bill Campbell
> Best Regards,
> Reinhard
>
>
>
>

[U-Boot] [PATCH 0/4] get at91rm9200ek working with ARM relocation

[Andreas Bießmann]

Dear Reinhard Meyer,

Am 30.11.2010 um 07:37 schrieb Andreas Bie?mann:

> The series includes two patches already in Reinhard's u-boot-atmel/at91-next
> branch but the base of at91-next is outdated and they will not apply cleanly.
> So feel free to include them or not. Reinhard, any comments?
> The mentioned patches are:
> 
>  at91rm9200ek: add configure target for RAM boot
>  MAKEALL: fix AT91
> 
> Beside that the 'add configure target for RAM boot' is needed for working
> configuration as ram-boot since nor-boot is currently broken ;)

is it OK for you to get the two patches in with this series? Or will you rebase and send pull request for your u-boot-atmel/at91 branch for v2010.12?

regards

Andreas Bie?mann