Message ID | 1291100800-61850-3-git-send-email-andreas.devel@googlemail.com |
---|---|
State | RFC |
Headers | show |
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,
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
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,
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
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(-)