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

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(-)

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,