[U-Boot] Try to fix Board eb_cpux9k2

[U-Boot] Try to fix Board eb_cpux9k2

[Jens Scharsig]

Dear Albert ARIBAUD,

I have tried to start update the eb_cpux9k2 board. I can compile 
board without errors from current tree. But the board hangs on
NAND detection. If I disable NAND support, the board starts and
I can also start Linux.
A second problem, the board does not restart (reset command).
I find out that a NULL pointer used by reset code, was also
relocated.

I have currently no access to a BDI2000. But I think, this problems
are not board specific. It is possible, there are problems with 
vector tables or memory protection

Any suggestions?

The board use 920t on at91rm9200 soc

regard 
Jens Scharsig

[U-Boot] [PATCH RFC 0/3] chenages to arm relocation

[Andreas Bießmann]

Dear Jens Scharsig and Albert ARIBAUD,

please see following RFC for the mentioned issues with NULL pointer
relocation and some other points on current relocation.

Andreas Bie?mann (3):
  arm920t: do not set register useless
  arm920t: do not use r8 for relocation
  arm920t: do not relocate NULL pointer

 arch/arm/cpu/arm920t/start.S |   32 ++++++++++++++++++--------------
 1 files changed, 18 insertions(+), 14 deletions(-)

-- 
1.7.3.2

[U-Boot] [PATCH RFC 1/3] arm920t: do not set register useless

[Andreas Bießmann]

in case we are still at relocation target address before relocation we
do not need to load the registers needed for relocation. We should
instead skip the whole relocation part and jump over to clear_bss.

Also prepare to not use target address twice. When we use a scratch
register here r6 is unchanged and can be used later on.

Signed-off-by: Andreas Bie?mann <andreas.devel@googlemail.com>
---
 arch/arm/cpu/arm920t/start.S |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm/cpu/arm920t/start.S b/arch/arm/cpu/arm920t/start.S
index 01edb9b..71de373 100644
--- a/arch/arm/cpu/arm920t/start.S
+++ b/arch/arm/cpu/arm920t/start.S
@@ -208,15 +208,16 @@ stack_setup:
 	mov	sp, r4
 
 	adr	r0, _start
+	cmp	r0, r6
+	beq	clear_bss		/* skip relocation */
+	mov	r1, r6
 	ldr	r2, _TEXT_BASE
 	ldr	r3, _bss_start_ofs
 	add	r2, r0, r3		/* r2 <- source end address	    */
-	cmp	r0, r6
-	beq	clear_bss
 
 copy_loop:
 	ldmia	r0!, {r9-r10}		/* copy from source address [r0]    */
-	stmia	r6!, {r9-r10}		/* copy to   target address [r1]    */
+	stmia	r1!, {r9-r10}		/* copy to   target address [r1]    */
 	cmp	r0, r2			/* until source end address [r2]    */
 	blo	copy_loop
 
-- 
1.7.3.2

[U-Boot] [PATCH RFC 1/3] arm920t: do not set register useless

[Albert ARIBAUD]

Hi Andreas,

Le 30/11/2010 08:06, Andreas Bie?mann a ?crit :

> diff --git a/arch/arm/cpu/arm920t/start.S b/arch/arm/cpu/arm920t/start.S
> index 01edb9b..71de373 100644
> --- a/arch/arm/cpu/arm920t/start.S
> +++ b/arch/arm/cpu/arm920t/start.S
> @@ -208,15 +208,16 @@ stack_setup:
>   	mov	sp, r4
>
>   	adr	r0, _start
> +	cmp	r0, r6
> +	beq	clear_bss		/* skip relocation */
> +	mov	r1, r6

Why use r1?

>   	ldr	r2, _TEXT_BASE
>   	ldr	r3, _bss_start_ofs
>   	add	r2, r0, r3		/* r2<- source end address	    */
> -	cmp	r0, r6
> -	beq	clear_bss
>
>   copy_loop:
>   	ldmia	r0!, {r9-r10}		/* copy from source address [r0]    */
> -	stmia	r6!, {r9-r10}		/* copy to   target address [r1]    */
> +	stmia	r1!, {r9-r10}		/* copy to   target address [r1]    */

Ditto.

>   	cmp	r0, r2			/* until source end address [r2]    */
>   	blo	copy_loop

Amicalement,
-- 
Albert.

[U-Boot] [PATCH RFC 1/3] arm920t: do not set register useless

[Andreas Bießmann]

Dear Albert ARIBAUD,

Am 30.11.2010 09:07, schrieb Albert ARIBAUD:
> Hi Andreas,
> 
> Le 30/11/2010 08:06, Andreas Bie?mann a ?crit :
> 
>> diff --git a/arch/arm/cpu/arm920t/start.S b/arch/arm/cpu/arm920t/start.S
>> index 01edb9b..71de373 100644
>> --- a/arch/arm/cpu/arm920t/start.S
>> +++ b/arch/arm/cpu/arm920t/start.S
>> @@ -208,15 +208,16 @@ stack_setup:
>>       mov    sp, r4
>>
>>       adr    r0, _start
>> +    cmp    r0, r6
>> +    beq    clear_bss        /* skip relocation */
>> +    mov    r1, r6
> 
> Why use r1?

Use a scratch register here cause stmia Rn! does increment Rn.

> 
>>       ldr    r2, _TEXT_BASE
>>       ldr    r3, _bss_start_ofs
>>       add    r2, r0, r3        /* r2<- source end address        */
>> -    cmp    r0, r6
>> -    beq    clear_bss
>>
>>   copy_loop:
>>       ldmia    r0!, {r9-r10}        /* copy from source address
>> [r0]    */
>> -    stmia    r6!, {r9-r10}        /* copy to   target address [r1]    */
>> +    stmia    r1!, {r9-r10}        /* copy to   target address [r1]    */

Therefore usage of r6 here would destroy the saved 'addr of
destination'. Cause that fact we have saved the 'addr of destination' @
beginning of relocate_code twice, once to r6 and once to r7. This is
obviously not needed iv we change the register in copy_loop (which was
already used in comment .. if you saw that on the end of the line).

I doubt if this a 'speed up' but we can have a cleaner interface. We
have r4, r5, r6 as storage of input values to relocate_code. We do only
use scratch registers for copy_loop, fixloop for relocation, clear_bss
later on. We do decide if we need fixloop for relocation or if we can
skip that and then setup the relevant scratch registers.

Well this is only an RFC. I found that plus the NULL pointer stuff worth
to mention. It is not that important to get this special patch in cause
the current implementation do also work.

regards

Andreas Bie?mann

[U-Boot] [PATCH RFC 1/3] arm920t: do not set register useless

[Albert ARIBAUD]

Le 30/11/2010 09:28, Andreas Bie?mann a ?crit :

>>> +    beq    clear_bss        /* skip relocation */
>>> +    mov    r1, r6
>>
>> Why use r1?
>
> Use a scratch register here cause stmia Rn! does increment Rn.

> Therefore usage of r6 here would destroy the saved 'addr of
> destination'. Cause that fact we have saved the 'addr of destination' @
> beginning of relocate_code twice, once to r6 and once to r7. This is
> obviously not needed iv we change the register in copy_loop (which was
> already used in comment .. if you saw that on the end of the line).

Understood.

> I doubt if this a 'speed up' but we can have a cleaner interface. We
> have r4, r5, r6 as storage of input values to relocate_code. We do only
> use scratch registers for copy_loop, fixloop for relocation, clear_bss
> later on. We do decide if we need fixloop for relocation or if we can
> skip that and then setup the relevant scratch registers.
>
> Well this is only an RFC. I found that plus the NULL pointer stuff worth
> to mention. It is not that important to get this special patch in cause
> the current implementation do also work.

The r8 issue and the zero-filled reloc entry issue are important to pull 
in as they can cause all sorts of time-wasting issues.

> regards
>
> Andreas Bie?mann

Amicalement,
-- 
Albert.

[U-Boot] [PATCH RFC 2/3] arm920t: do not use r8 for relocation

[Andreas Bießmann]

r8 is used for gd and should therefore be left alone

Signed-off-by: Andreas Bie?mann <andreas.devel@googlemail.com>
---

I don't know if this is really needed, but we use --fixed-r8 compiler
flag for all arm boards. Albert, can you shed some light on that?

 arch/arm/cpu/arm920t/start.S |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/arch/arm/cpu/arm920t/start.S b/arch/arm/cpu/arm920t/start.S
index 71de373..629be3f 100644
--- a/arch/arm/cpu/arm920t/start.S
+++ b/arch/arm/cpu/arm920t/start.S
@@ -201,7 +201,6 @@ relocate_code:
 	mov	r4, r0	/* save addr_sp */
 	mov	r5, r1	/* save addr of gd */
 	mov	r6, r2	/* save addr of destination */
-	mov	r7, r2	/* save addr of destination */
 
 	/* Set up the stack						    */
 stack_setup:
@@ -226,7 +225,7 @@ copy_loop:
 	 * fix .rel.dyn relocations
 	 */
 	ldr	r0, _TEXT_BASE		/* r0 <- Text base */
-	sub	r9, r7, r0		/* r9 <- relocation offset */
+	sub	r9, r6, r0		/* r9 <- relocation offset */
 	ldr	r10, _dynsym_start_ofs	/* r10 <- sym table ofs */
 	add	r10, r10, r0		/* r10 <- sym table in FLASH */
 	ldr	r2, _rel_dyn_start_ofs	/* r2 <- rel dyn start ofs */
@@ -237,10 +236,10 @@ fixloop:
 	ldr	r0, [r2]		/* r0 <- location to fix up, IN FLASH! */
 	add	r0, r0, r9		/* r0 <- location to fix up in RAM */
 	ldr	r1, [r2, #4]
-	and	r8, r1, #0xff
-	cmp	r8, #23			/* relative fixup? */
+	and	r7, r1, #0xff
+	cmp	r7, #23			/* relative fixup? */
 	beq	fixrel
-	cmp	r8, #2			/* absolute fixup? */
+	cmp	r7, #2			/* absolute fixup? */
 	beq	fixabs
 	/* ignore unknown type of fixup */
 	b	fixnext
@@ -253,10 +252,10 @@ fixabs:
 	b	fixnext
 fixrel:
 	/* relative fix: increase location by offset */
-	ldr	r1, [r0]
-	add	r1, r1, r9
+	ldr	r1, [r0]		/* r1 <- address of symbol */
+	add	r1, r1, r9		/* r1 <- relocated address of symbol */
 fixnext:
-	str	r1, [r0]
+	str	r1, [r0]		/* store back content of r1 */
 	add	r2, r2, #8		/* each rel.dyn entry is 8 bytes */
 	cmp	r2, r3
 	blo	fixloop
@@ -267,7 +266,7 @@ clear_bss:
 	ldr	r0, _bss_start_ofs
 	ldr	r1, _bss_end_ofs
 	ldr	r3, _TEXT_BASE		/* Text base */
-	mov	r4, r7			/* reloc addr */
+	mov	r4, r6			/* reloc addr */
 	add	r0, r0, r4
 	add	r1, r1, r4
 	mov	r2, #0x00000000		/* clear			    */
@@ -295,10 +294,10 @@ _nand_boot_ofs:
 	ldr	r0, _board_init_r_ofs
 	adr	r1, _start
 	add	lr, r0, r1
-	add	lr, lr, r9
+	add	lr, lr, r9	/* position from board_init_r in RAM */
 	/* setup parameters for board_init_r */
 	mov	r0, r5		/* gd_t */
-	mov	r1, r7		/* dest_addr */
+	mov	r1, r6		/* dest_addr */
 	/* jump to it ... */
 	mov	pc, lr
 
-- 
1.7.3.2

[U-Boot] [PATCH RFC 2/3] arm920t: do not use r8 for relocation

[Albert ARIBAUD]

Le 30/11/2010 08:06, Andreas Bie?mann a ?crit :
> r8 is used for gd and should therefore be left alone

I'm surprised that this did not break things so far... Whatever value r8 
ended with was used as the address of GD.

After a quick look I haven't found out where r8 is *set* to GD, 
though... Will have to look this up tonight.

> Signed-off-by: Andreas Bie?mann<andreas.devel@googlemail.com>
> ---
>
> I don't know if this is really needed, but we use --fixed-r8 compiler
> flag for all arm boards. Albert, can you shed some light on that?

ffixed-r8 is for the C compiler. The assembler cannot honor ffixed-r8 
since register usage is decided by the programmer in each instruction.

>   arch/arm/cpu/arm920t/start.S |   21 ++++++++++-----------
>   1 files changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm/cpu/arm920t/start.S b/arch/arm/cpu/arm920t/start.S
> index 71de373..629be3f 100644
> --- a/arch/arm/cpu/arm920t/start.S
> +++ b/arch/arm/cpu/arm920t/start.S
> @@ -201,7 +201,6 @@ relocate_code:
>   	mov	r4, r0	/* save addr_sp */
>   	mov	r5, r1	/* save addr of gd */
>   	mov	r6, r2	/* save addr of destination */
> -	mov	r7, r2	/* save addr of destination */
>
>   	/* Set up the stack						    */
>   stack_setup:
> @@ -226,7 +225,7 @@ copy_loop:
>   	 * fix .rel.dyn relocations
>   	 */
>   	ldr	r0, _TEXT_BASE		/* r0<- Text base */
> -	sub	r9, r7, r0		/* r9<- relocation offset */
> +	sub	r9, r6, r0		/* r9<- relocation offset */
>   	ldr	r10, _dynsym_start_ofs	/* r10<- sym table ofs */
>   	add	r10, r10, r0		/* r10<- sym table in FLASH */
>   	ldr	r2, _rel_dyn_start_ofs	/* r2<- rel dyn start ofs */
> @@ -237,10 +236,10 @@ fixloop:
>   	ldr	r0, [r2]		/* r0<- location to fix up, IN FLASH! */
>   	add	r0, r0, r9		/* r0<- location to fix up in RAM */
>   	ldr	r1, [r2, #4]
> -	and	r8, r1, #0xff
> -	cmp	r8, #23			/* relative fixup? */
> +	and	r7, r1, #0xff
> +	cmp	r7, #23			/* relative fixup? */
>   	beq	fixrel
> -	cmp	r8, #2			/* absolute fixup? */
> +	cmp	r7, #2			/* absolute fixup? */
>   	beq	fixabs
>   	/* ignore unknown type of fixup */
>   	b	fixnext
> @@ -253,10 +252,10 @@ fixabs:
>   	b	fixnext
>   fixrel:
>   	/* relative fix: increase location by offset */
> -	ldr	r1, [r0]
> -	add	r1, r1, r9
> +	ldr	r1, [r0]		/* r1<- address of symbol */
> +	add	r1, r1, r9		/* r1<- relocated address of symbol */

I'd like to see a less ambiguous comment here, but I'm not sure what's 
best. Any suggestions?

>   fixnext:
> -	str	r1, [r0]
> +	str	r1, [r0]		/* store back content of r1 */

Nak. This comment paraphrases the instruction.

>   	add	r2, r2, #8		/* each rel.dyn entry is 8 bytes */
>   	cmp	r2, r3
>   	blo	fixloop
> @@ -267,7 +266,7 @@ clear_bss:
>   	ldr	r0, _bss_start_ofs
>   	ldr	r1, _bss_end_ofs
>   	ldr	r3, _TEXT_BASE		/* Text base */
> -	mov	r4, r7			/* reloc addr */
> +	mov	r4, r6			/* reloc addr */
>   	add	r0, r0, r4
>   	add	r1, r1, r4
>   	mov	r2, #0x00000000		/* clear			    */
> @@ -295,10 +294,10 @@ _nand_boot_ofs:
>   	ldr	r0, _board_init_r_ofs
>   	adr	r1, _start
>   	add	lr, r0, r1
> -	add	lr, lr, r9
> +	add	lr, lr, r9	/* position from board_init_r in RAM */
>   	/* setup parameters for board_init_r */
>   	mov	r0, r5		/* gd_t */
> -	mov	r1, r7		/* dest_addr */
> +	mov	r1, r6		/* dest_addr */
>   	/* jump to it ... */
>   	mov	pc, lr
>

Amicalement,
-- 
Albert.

[U-Boot] [PATCH RFC 2/3] arm920t: do not use r8 for relocation

[Andreas Bießmann]

Dear Albert ARIBAUD,

Am 30.11.2010 09:22, schrieb Albert ARIBAUD:
> Le 30/11/2010 08:06, Andreas Bie?mann a ?crit :
>> r8 is used for gd and should therefore be left alone
> 
> I'm surprised that this did not break things so far... Whatever value r8
> ended with was used as the address of GD.

Well r8 is set in arch/arm/include/asm/global_data.h

---8<---
#define DECLARE_GLOBAL_DATA_PTR     register volatile gd_t *gd asm ("r8")
--->8---

The GD is then later on allocated in board_init_f

---8<---
	/* Pointer is writable since we allocated a register for it */
	gd = (gd_t *) (CONFIG_SYS_INIT_SP_ADDR);
	/* compiler optimization barrier needed for GCC >= 3.4 */
	__asm__ __volatile__("": : :"memory");

	memset ((void*)gd, 0, sizeof (gd_t));
--->8---

Therefore r8 is free, when we use it in relocate_code, isn't it?

> After a quick look I haven't found out where r8 is *set* to GD,
> though... Will have to look this up tonight.

> 
>> Signed-off-by: Andreas Bie?mann<andreas.devel@googlemail.com>
>> ---
>>
>> I don't know if this is really needed, but we use --fixed-r8 compiler
>> flag for all arm boards. Albert, can you shed some light on that?
> 
> ffixed-r8 is for the C compiler. The assembler cannot honor ffixed-r8
> since register usage is decided by the programmer in each instruction.

I do know that. Therefore this patch to do _not_ use r8 in assembler.

>>       /* relative fix: increase location by offset */
>> -    ldr    r1, [r0]
>> -    add    r1, r1, r9
>> +    ldr    r1, [r0]        /* r1<- address of symbol */
>> +    add    r1, r1, r9        /* r1<- relocated address of symbol */
> 
> I'd like to see a less ambiguous comment here, but I'm not sure what's
> best. Any suggestions?

Not currently, this was just slipped in by fast preperation of that patch.

>>   fixnext:
>> -    str    r1, [r0]
>> +    str    r1, [r0]        /* store back content of r1 */
> 
> Nak. This comment paraphrases the instruction.

dito

regards

Andreas Bie?mann

[U-Boot] [PATCH RFC 2/3] arm920t: do not use r8 for relocation

[Albert ARIBAUD]

Le 30/11/2010 09:35, Andreas Bie?mann a ?crit :
> Dear Albert ARIBAUD,
>
> Am 30.11.2010 09:22, schrieb Albert ARIBAUD:
>> Le 30/11/2010 08:06, Andreas Bie?mann a ?crit :
>>> r8 is used for gd and should therefore be left alone
>>
>> I'm surprised that this did not break things so far... Whatever value r8
>> ended with was used as the address of GD.
>
> Well r8 is set in arch/arm/include/asm/global_data.h
>
> ---8<---
> #define DECLARE_GLOBAL_DATA_PTR     register volatile gd_t *gd asm ("r8")
> --->8---
>
> The GD is then later on allocated in board_init_f
>
> ---8<---
> 	/* Pointer is writable since we allocated a register for it */
> 	gd = (gd_t *) (CONFIG_SYS_INIT_SP_ADDR);
> 	/* compiler optimization barrier needed for GCC>= 3.4 */
> 	__asm__ __volatile__("": : :"memory");
>
> 	memset ((void*)gd, 0, sizeof (gd_t));
> --->8---

... which is why I could not find "r8" allocation :) ... Thanks. So yes, 
we need to keep r8, and yes, we were lucky that the start.S code did run 
at all with a corrupted r8 -- fixing that may remove some weird 
behaviors we could see,

Can you do a pass on other ARM archs which must be fixed too?

>> I'd like to see a less ambiguous comment here, but I'm not sure what's
>> best. Any suggestions?
>
> Not currently, this was just slipped in by fast preperation of that patch.
>
>>>    fixnext:
>>> -    str    r1, [r0]
>>> +    str    r1, [r0]        /* store back content of r1 */
>>
>> Nak. This comment paraphrases the instruction.
>
> dito

I'd rather have no comment than a paraphrase of the code, but I'd rather 
have an imperfect yet at least partly helpful comment than no comment at 
all for assembly language code.

> regards
>
> Andreas Bie?mann

Amicalement,
-- 
Albert.

[U-Boot] [PATCH RFC 3/3] arm920t: do not relocate NULL pointer

[Andreas Bießmann]

Signed-off-by: Andreas Bie?mann <andreas.devel@googlemail.com>
---
 arch/arm/cpu/arm920t/start.S |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/arm/cpu/arm920t/start.S b/arch/arm/cpu/arm920t/start.S
index 629be3f..e3f9cdb 100644
--- a/arch/arm/cpu/arm920t/start.S
+++ b/arch/arm/cpu/arm920t/start.S
@@ -248,11 +248,15 @@ fixabs:
 	mov	r1, r1, LSR #4		/* r1 <- symbol index in .dynsym */
 	add	r1, r10, r1		/* r1 <- address of symbol in table */
 	ldr	r1, [r1, #4]		/* r1 <- symbol value */
+	cmp	r1, #0			/* symbol == NULL ? */
+	beq	fixnext
 	add	r1, r9			/* r1 <- relocated sym addr */
 	b	fixnext
 fixrel:
 	/* relative fix: increase location by offset */
 	ldr	r1, [r0]		/* r1 <- address of symbol */
+	cmp	r1, #0			/* symbol == NULL ? */
+	beq	fixnext
 	add	r1, r1, r9		/* r1 <- relocated address of symbol */
 fixnext:
 	str	r1, [r0]		/* store back content of r1 */
-- 
1.7.3.2

[U-Boot] [PATCH RFC 3/3] arm920t: do not relocate NULL pointer

[Albert ARIBAUD]

Le 30/11/2010 08:06, Andreas Bie?mann a ?crit :
> Signed-off-by: Andreas Bie?mann<andreas.devel@googlemail.com>

> +	cmp	r1, #0			/* symbol == NULL ? */
> +	beq	fixnext

Nak. Don't hide a null pointer. NULL pointers are *not* relocated, since 
they are a constant. If a NULL ends up in relocation tables, that is 
because of a corruption *or* because it was to be relocated, and should 
thus never be ignored.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH RFC 3/3] arm920t: do not relocate NULL pointer

[Joakim Tjernlund]

>
> Le 30/11/2010 08:06, Andreas Bie?mann a ?crit :
> > Signed-off-by: Andreas Bie?mann<andreas.devel@googlemail.com>
>
> > +   cmp   r1, #0         /* symbol == NULL ? */
> > +   beq   fixnext
>
> Nak. Don't hide a null pointer. NULL pointers are *not* relocated, since
> they are a constant. If a NULL ends up in relocation tables, that is
> because of a corruption *or* because it was to be relocated, and should
> thus never be ignored.

Depends, if the same routine is used for relocating fixups you need
this test. Undefined weaks will generate a NULL fixup entry.

    Jocke

[U-Boot] [PATCH RFC 3/3] arm920t: do not relocate NULL pointer

[Andreas Bießmann]

Dear All,

Am 30.11.2010 09:47, schrieb Joakim Tjernlund:
>>
>> Le 30/11/2010 08:06, Andreas Bie?mann a ?crit :
>>> Signed-off-by: Andreas Bie?mann<andreas.devel@googlemail.com>
>>
>>> +   cmp   r1, #0         /* symbol == NULL ? */
>>> +   beq   fixnext
>>
>> Nak. Don't hide a null pointer. NULL pointers are *not* relocated, since
>> they are a constant. If a NULL ends up in relocation tables, that is
>> because of a corruption *or* because it was to be relocated, and should
>> thus never be ignored.
> 
> Depends, if the same routine is used for relocating fixups you need
> this test.

> Undefined weaks will generate a NULL fixup entry.

As mentioned by Jens and one other some time ago. Albert, please build
e.g. at91rm9200ek boards and search for board_reset(), defined in
arch/arm/cpu/arm920t/at91/reset.c

regards

Andreas Bie?mann

[U-Boot] [PATCH RFC 3/3] arm920t: do not relocate NULL pointer

[Albert ARIBAUD]

Le 30/11/2010 09:47, Joakim Tjernlund a ?crit :
>>
>> Le 30/11/2010 08:06, Andreas Bie?mann a ?crit :
>>> Signed-off-by: Andreas Bie?mann<andreas.devel@googlemail.com>
>>
>>> +   cmp   r1, #0         /* symbol == NULL ? */
>>> +   beq   fixnext
>>
>> Nak. Don't hide a null pointer. NULL pointers are *not* relocated, since
>> they are a constant. If a NULL ends up in relocation tables, that is
>> because of a corruption *or* because it was to be relocated, and should
>> thus never be ignored.
>
> Depends, if the same routine is used for relocating fixups you need
> this test. Undefined weaks will generate a NULL fixup entry.

Understood.

Weren't there an effort to not use weak symbols any more?

If not, then a comment *must* be added to indicate that weak symbols can 
cause zero-filled reloc entries (I would suggest saying 'zero-filled' 
rather than 'NULL', because obviously readers will think of stdio's NULL).

We could possibly even send out a diagnostic message, but that'll be 
when the relocation code is turned to C language; I don't want to see 
asm code that calls printf.

>      Jocke

Amicalement,
-- 
Albert.

[U-Boot] [PATCH RFC 3/3] arm920t: do not relocate NULL pointer

[Joakim Tjernlund]

Albert ARIBAUD <albert@aribaud.net> wrote on 2010/11/30 10:02:45:
>
> Le 30/11/2010 09:47, Joakim Tjernlund a ?crit :
> >>
> >> Le 30/11/2010 08:06, Andreas Bie?mann a ?crit :
> >>> Signed-off-by: Andreas Bie?mann<andreas.devel@googlemail.com>
> >>
> >>> +   cmp   r1, #0         /* symbol == NULL ? */
> >>> +   beq   fixnext
> >>
> >> Nak. Don't hide a null pointer. NULL pointers are *not* relocated, since
> >> they are a constant. If a NULL ends up in relocation tables, that is
> >> because of a corruption *or* because it was to be relocated, and should
> >> thus never be ignored.
> >
> > Depends, if the same routine is used for relocating fixups you need
> > this test. Undefined weaks will generate a NULL fixup entry.
>
> Understood.

note that I don't know how this routine is used so if just
relocates the GOT you don't need to test for NULL.

>
> Weren't there an effort to not use weak symbols any more?

ehh, not what I am aware of. Just that we should not use(ATM)
undefined weaks.

>
> If not, then a comment *must* be added to indicate that weak symbols can
> cause zero-filled reloc entries (I would suggest saying 'zero-filled'
> rather than 'NULL', because obviously readers will think of stdio's NULL).

zero-filled/NULL fixup entries. The GOT never holds NULL

>
> We could possibly even send out a diagnostic message, but that'll be
> when the relocation code is turned to C language; I don't want to see
> asm code that calls printf.

No need really.

[U-Boot] [PATCH RFC 3/3] arm920t: do not relocate NULL pointer

[Andreas Bießmann]

Dear Joakim Tjernlund,
Dear Albert ARIBAUD,

Am 30.11.2010 10:41, schrieb Joakim Tjernlund:
> Albert ARIBAUD <albert@aribaud.net> wrote on 2010/11/30 10:02:45:
>>
>> Le 30/11/2010 09:47, Joakim Tjernlund a ?crit :
>>>>
>>>> Le 30/11/2010 08:06, Andreas Bie?mann a ?crit :
>>>>> Signed-off-by: Andreas Bie?mann<andreas.devel@googlemail.com>
>>>>
>>>>> +   cmp   r1, #0         /* symbol == NULL ? */
>>>>> +   beq   fixnext
>>>>
>>>> Nak. Don't hide a null pointer. NULL pointers are *not* relocated, since
>>>> they are a constant. If a NULL ends up in relocation tables, that is
>>>> because of a corruption *or* because it was to be relocated, and should
>>>> thus never be ignored.
>>>
>>> Depends, if the same routine is used for relocating fixups you need
>>> this test. Undefined weaks will generate a NULL fixup entry.
>>
>> Understood.
> 
> note that I don't know how this routine is used so if just
> relocates the GOT you don't need to test for NULL.
> 
>>
>> Weren't there an effort to not use weak symbols any more?
> 
> ehh, not what I am aware of. Just that we should not use(ATM)
> undefined weaks.

Ok, I did forget to investigate that as mentioned in
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/88024/focus=88324

Here are the results:

Currently arm920/at91 devices have one undefined weak symbol in .dynsym:

---8<---
Symbol table '.dynsym' contains 11 entries:
   Num:    Value  Size Type    Bind   Vis      Ndx Name
     0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND <corrupt>
     1: 10000000     0 SECTION LOCAL  DEFAULT    1 <corrupt>
     2: 10028a28     0 SECTION LOCAL  DEFAULT    6 <corrupt>
     3: 10029ff8     0 NOTYPE  GLOBAL DEFAULT    9 <corrupt>
     4: 100299f4     0 NOTYPE  GLOBAL DEFAULT  ABS <corrupt>
     5: 00000000     0 NOTYPE  WEAK   DEFAULT  UND <corrupt>
     6: 10029ff8     0 NOTYPE  GLOBAL DEFAULT  ABS <corrupt>
     7: 10029ff8     0 NOTYPE  GLOBAL DEFAULT   11 <corrupt>
     8: 1002edf0     0 NOTYPE  GLOBAL DEFAULT   10 <corrupt>
     9: 100701c0     0 NOTYPE  GLOBAL DEFAULT   11 <corrupt>
    10: 1002edf0     0 NOTYPE  GLOBAL DEFAULT    9 <corrupt>
--->8---

---8<---
Hex dump of section '.dynsym':
  0x1002edf0 00000000 00000000 00000000 00000000 ................
  0x1002ee00 00000000 00000010 00000000 03000100 ................
  0x1002ee10 00000000 288a0210 00000000 03000600 ....(...........
  0x1002ee20 25000000 f89f0210 00000000 10000900 %...............
  0x1002ee30 01000000 f4990210 00000000 1000f1ff ................
  0x1002ee40 5e000000 00000000 00000000 20000000 ^........... ...
  0x1002ee50 14000000 f89f0210 00000000 1000f1ff ................
  0x1002ee60 52000000 f89f0210 00000000 10000b00 R...............
  0x1002ee70 43000000 f0ed0210 00000000 10000a00 C...............
  0x1002ee80 20000000 c0010710 00000000 10000b00  ...............
  0x1002ee90 35000000 f0ed0210 00000000 10000900 5...............
--->8---

So there are two options here.

First is to apply '[PATCH v2 1/2] arm920t/at91/reset.c: fix weak
reset_board()' second (and safer) is to apply (maybe modified) '[PATCH
RFC 3/3] arm920t: do not relocate NULL pointer'.

which one is preferred? Or should we do both?

regards

Andreas Bie?mann

[U-Boot] [PATCH RFC 3/3] arm920t: do not relocate NULL pointer

[Joakim Tjernlund]

"Andreas Bie?mann" <andreas.devel@googlemail.com> wrote on 2010/11/30 12:48:41:
>
> Dear Joakim Tjernlund,
> Dear Albert ARIBAUD,
>
> Am 30.11.2010 10:41, schrieb Joakim Tjernlund:
> > Albert ARIBAUD <albert@aribaud.net> wrote on 2010/11/30 10:02:45:
> >>
> >> Le 30/11/2010 09:47, Joakim Tjernlund a ?crit :
> >>>>
> >>>> Le 30/11/2010 08:06, Andreas Bie?mann a ?crit :
> >>>>> Signed-off-by: Andreas Bie?mann<andreas.devel@googlemail.com>
> >>>>
> >>>>> +   cmp   r1, #0         /* symbol == NULL ? */
> >>>>> +   beq   fixnext
> >>>>
> >>>> Nak. Don't hide a null pointer. NULL pointers are *not* relocated, since
> >>>> they are a constant. If a NULL ends up in relocation tables, that is
> >>>> because of a corruption *or* because it was to be relocated, and should
> >>>> thus never be ignored.
> >>>
> >>> Depends, if the same routine is used for relocating fixups you need
> >>> this test. Undefined weaks will generate a NULL fixup entry.
> >>
> >> Understood.
> >
> > note that I don't know how this routine is used so if just
> > relocates the GOT you don't need to test for NULL.
> >
> >>
> >> Weren't there an effort to not use weak symbols any more?
> >
> > ehh, not what I am aware of. Just that we should not use(ATM)
> > undefined weaks.
>
> Ok, I did forget to investigate that as mentioned in
> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/88024/focus=88324
>
> Here are the results:
>
> Currently arm920/at91 devices have one undefined weak symbol in .dynsym:
>
> ---8<---
> Symbol table '.dynsym' contains 11 entries:
>    Num:    Value  Size Type    Bind   Vis      Ndx Name
>      0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND <corrupt>
>      1: 10000000     0 SECTION LOCAL  DEFAULT    1 <corrupt>
>      2: 10028a28     0 SECTION LOCAL  DEFAULT    6 <corrupt>
>      3: 10029ff8     0 NOTYPE  GLOBAL DEFAULT    9 <corrupt>
>      4: 100299f4     0 NOTYPE  GLOBAL DEFAULT  ABS <corrupt>
>      5: 00000000     0 NOTYPE  WEAK   DEFAULT  UND <corrupt>
>      6: 10029ff8     0 NOTYPE  GLOBAL DEFAULT  ABS <corrupt>
>      7: 10029ff8     0 NOTYPE  GLOBAL DEFAULT   11 <corrupt>
>      8: 1002edf0     0 NOTYPE  GLOBAL DEFAULT   10 <corrupt>
>      9: 100701c0     0 NOTYPE  GLOBAL DEFAULT   11 <corrupt>
>     10: 1002edf0     0 NOTYPE  GLOBAL DEFAULT    9 <corrupt>
> --->8---
>
> ---8<---
> Hex dump of section '.dynsym':
>   0x1002edf0 00000000 00000000 00000000 00000000 ................
>   0x1002ee00 00000000 00000010 00000000 03000100 ................
>   0x1002ee10 00000000 288a0210 00000000 03000600 ....(...........
>   0x1002ee20 25000000 f89f0210 00000000 10000900 %...............
>   0x1002ee30 01000000 f4990210 00000000 1000f1ff ................
>   0x1002ee40 5e000000 00000000 00000000 20000000 ^........... ...
>   0x1002ee50 14000000 f89f0210 00000000 1000f1ff ................
>   0x1002ee60 52000000 f89f0210 00000000 10000b00 R...............
>   0x1002ee70 43000000 f0ed0210 00000000 10000a00 C...............
>   0x1002ee80 20000000 c0010710 00000000 10000b00  ...............
>   0x1002ee90 35000000 f0ed0210 00000000 10000900 5...............
> --->8---
>
> So there are two options here.
>
> First is to apply '[PATCH v2 1/2] arm920t/at91/reset.c: fix weak
> reset_board()' second (and safer) is to apply (maybe modified) '[PATCH
> RFC 3/3] arm920t: do not relocate NULL pointer'.
>
> which one is preferred? Or should we do both?

Both, you don't know what the future will be and perhaps someone
wants to use undef weaks in board code.

   Jocke

[U-Boot] Try to fix Board eb_cpux9k2

[Albert ARIBAUD]

Le 29/11/2010 20:58, Jens Scharsig a ?crit :
> Dear Albert ARIBAUD,
>
> I have tried to start update the eb_cpux9k2 board. I can compile
> board without errors from current tree. But the board hangs on
> NAND detection. If I disable NAND support, the board starts and
> I can also start Linux.

We've had issues in the past with boards that did not detect NAND. Can 
you look up the u-boot archives for this?

> A second problem, the board does not restart (reset command).
> I find out that a NULL pointer used by reset code, was also
> relocated.

How did you come to this conclusion? NULL pointers are constants and 
thus are *not* relocated.

> I have currently no access to a BDI2000. But I think, this problems
> are not board specific. It is possible, there are problems with
> vector tables or memory protection
>
> Any suggestions?

I suggest visually running through all (board or cpu-specific) code that 
runs as part of execution board_init_f() and checking that no global is 
ever used -- BSS or initialized.

> The board use 920t on at91rm9200 soc
>
> regard
> Jens Scharsig

Amicalement,
-- 
Albert.

[U-Boot] Try to fix Board eb_cpux9k2

[Jens Scharsig]

Dear Albert ARIBAUD,

> 
>> A second problem, the board does not restart (reset command).
>> I find out that a NULL pointer used by reset code, was also
>> relocated.
> 
> How did you come to this conclusion? NULL pointers are constants and
> thus are *not* relocated.

This problem will be gone with Andreas Bie?mann patch

arm920t/at91/reset: board_reset: define	weak symbol reset
> 
>> I have currently no access to a BDI2000. But I think, this problems
>> are not board specific. It is possible, there are problems with
>> vector tables or memory protection
>>
>> Any suggestions?
> 
> I suggest visually running through all (board or cpu-specific) code that
> runs as part of execution board_init_f() and checking that no global is
> ever used -- BSS or initialized.

Andreas mirror BSS check says OK, But the board hangs on write access to
nand memory space (0x40000000) without any error message.

> 
>> The board use 920t on at91rm9200 soc
>>


regards Jens

[U-Boot] Try to fix Board eb_cpux9k2

[Albert ARIBAUD]

Le 02/12/2010 21:27, Jens Scharsig a ?crit :

>> I suggest visually running through all (board or cpu-specific) code that
>> runs as part of execution board_init_f() and checking that no global is
>> ever used -- BSS or initialized.
>
> Andreas mirror BSS check says OK, But the board hangs on write access to
> nand memory space (0x40000000) without any error message.

Did you also apply Andreas's fix to not use r8 in start.S? Thats's 
<http://patchwork.ozlabs.org/patch/73685/>.

Amicalement,
-- 
Albert.

[U-Boot] Try to fix Board eb_cpux9k2

[Albert ARIBAUD]

(not sure I've answered you... sorry if I did already)

Le 29/11/2010 20:58, Jens Scharsig a ?crit :
> Dear Albert ARIBAUD,
>
> I have tried to start update the eb_cpux9k2 board. I can compile
> board without errors from current tree. But the board hangs on
> NAND detection. If I disable NAND support, the board starts and
> I can also start Linux.
> A second problem, the board does not restart (reset command).
> I find out that a NULL pointer used by reset code, was also
> relocated.

Does any code executed directly or indirectly by biard_init_f() have 
global (BSS) variables? If so, you should fix this by moving those 
globals to the GD structure: BSS does not exist before relocation.

Amicalement,
-- 
Albert.