[U-Boot] Try to fix Board eb_cpux9k2
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
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(-)
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
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,
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
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,
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
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ßmannandreas.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,
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ßmannandreas.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
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,
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 */
Le 30/11/2010 08:06, Andreas Bießmann a écrit :
Signed-off-by: Andreas Bießmannandreas.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,
Le 30/11/2010 08:06, Andreas Bießmann a écrit :
Signed-off-by: Andreas Bießmannandreas.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
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ßmannandreas.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
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ßmannandreas.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 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ßmannandreas.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.
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ßmannandreas.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
"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ßmannandreas.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
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,
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
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,
(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,
participants (5)
-
Albert ARIBAUD -
Albert ARIBAUD -
Andreas Bießmann -
Jens Scharsig -
Joakim Tjernlund