[U-Boot] [PATCH v2 4/4] arm: factorize relocate_code routine

["Benoît Thébaudeau" <benoit.thebaudeau@advansee.com>]

Hi Albert,

On Tuesday, May 14, 2013 11:50:30 AM, Albert ARIBAUD wrote:
> Replace all relocate_code routines from ARM start.S files
> with a single instance in file arch/arm/lib/relocate.S.
> For PXA, this requires moving the dcache unlocking code
> from within relocate_code into c_runtime_cpu_setup.
> 
> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>

[...]

> diff --git a/arch/arm/cpu/pxa/start.S b/arch/arm/cpu/pxa/start.S
> index 595778a..f9947de 100644
> --- a/arch/arm/cpu/pxa/start.S
> +++ b/arch/arm/cpu/pxa/start.S
> @@ -167,93 +167,19 @@ reset:
>  	bl	_main
>  
>  /*------------------------------------------------------------------------------*/
> -#ifndef CONFIG_SPL_BUILD
> -/*
> - * void relocate_code(addr_moni)
> - *
> - * This function relocates the monitor code.
> - */
> -	.globl	relocate_code
> -relocate_code:
> -	mov	r6, r0	/* save addr of destination */
> -
> -/* Disable the Dcache RAM lock for stack now */
> -#ifdef	CONFIG_CPU_PXA25X
> -	mov	r12, lr
> -	bl	cpu_init_crit
> -	mov	lr, r12
> -#endif
> -
> -	adr	r0, _start
> -	subs	r9, r6, r0		/* r9 <- relocation offset */
> -	beq	relocate_done		/* skip relocation */
> -	mov	r1, r6			/* r1 <- scratch for copy_loop */
> -	ldr	r3, _image_copy_end_ofs
> -	add	r2, r0, r3		/* r2 <- source end address	    */
> -
> -copy_loop:
> -	ldmia	r0!, {r10-r11}		/* copy from source address [r0]    */
> -	stmia	r1!, {r10-r11}		/* copy to   target address [r1]    */
> -	cmp	r0, r2			/* until source end address [r2]    */
> -	blo	copy_loop
> -
> -	/*
> -	 * fix .rel.dyn relocations
> -	 */
> -	ldr	r0, _TEXT_BASE		/* r0 <- Text base */
> -	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 */
> -	add	r2, r2, r0		/* r2 <- rel dyn start in FLASH */
> -	ldr	r3, _rel_dyn_end_ofs	/* r3 <- rel dyn end ofs */
> -	add	r3, r3, r0		/* r3 <- rel dyn end in FLASH */
> -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	r7, r1, #0xff
> -	cmp	r7, #23			/* relative fixup? */
> -	beq	fixrel
> -	cmp	r7, #2			/* absolute fixup? */
> -	beq	fixabs
> -	/* ignore unknown type of fixup */
> -	b	fixnext
> -fixabs:
> -	/* absolute fix: set location to (offset) symbol value */
> -	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 */
> -	add	r1, r1, r9		/* r1 <- relocated sym addr */
> -	b	fixnext
> -fixrel:
> -	/* relative fix: increase location by offset */
> -	ldr	r1, [r0]
> -	add	r1, r1, r9
> -fixnext:
> -	str	r1, [r0]
> -	add	r2, r2, #8		/* each rel.dyn entry is 8 bytes */
> -	cmp	r2, r3
> -	blo	fixloop
> -
> -relocate_done:
> -
> -	bx	lr
> -
> -_image_copy_end_ofs:
> -	.word __image_copy_end - _start
> -_rel_dyn_start_ofs:
> -	.word __rel_dyn_start - _start
> -_rel_dyn_end_ofs:
> -	.word __rel_dyn_end - _start
> -_dynsym_start_ofs:
> -	.word __dynsym_start - _start
> -
> -#endif
>  
>  	.globl	c_runtime_cpu_setup
>  c_runtime_cpu_setup:
>  
> -	bx	lr
> +	/*
> +	 * Unlock (actually, disable) the cache now that board_init_f
> +	 * is done. We could do this earlier but we would need to add
> +	 * a new C runtime hook, whereas c_runtime_cpu_setup already
> +	 * exists.
> +	 * As this routine is just a call to cpu_init_crit, let us
> +	 * tail-optimize and do a simple branch here.
> +	 */
> +	b	cpu_init_crit

Shouldn't the "#ifdef CONFIG_CPU_PXA25X" from the original code be kept here?

>  
>  /*
>   *************************************************************************

[...]

> diff --git a/arch/arm/lib/relocate.S b/arch/arm/lib/relocate.S
> new file mode 100644
> index 0000000..9e91fae
> --- /dev/null
> +++ b/arch/arm/lib/relocate.S
> @@ -0,0 +1,111 @@
> +/*
> + *  relocate - common relocation function for ARM U-Boot
> + *
> + *  Copyright (c) 2013  Albert ARIBAUD <albert.u.boot@aribaud.net>
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <linux/linkage.h>
> +
> +/*
> + * These are defined in the board-specific linker script.
> + * Subtracting _start from them lets the linker put their
> + * relative position in the executable instead of leaving
> + * them null.
> + */

This comment is obsolete. It should either be removed or updated.

> +
> +/*
> + * void relocate_code(addr_moni)
> + *
> + * This function relocates the monitor code.
> + */
> +ENTRY(relocate_code)
> +	mov	r6, r0	/* save addr of destination */
> +
> +	ldr	r0, =_start

If you are avoiding the "ldr =" construct below, why do you use it here? You
could "ldr r0, _TEXT_BASE".

> +	subs	r9, r6, r0		/* r9 <- relocation offset */
> +	beq	relocate_done		/* skip relocation */
> +	mov	r1, r6			/* r1 <- scratch for copy loop */
> +	ldr	r3, _image_copy_end_ofs
> +	add	r2, r0, r3		/* r2 <- source end address	    */

Adding _start to a relocate_code-relative _image_copy_end_ofs?!

> +
> +copy_loop:
> +	ldmia	r0!, {r10-r11}		/* copy from source address [r0]    */
> +	stmia	r1!, {r10-r11}		/* copy to   target address [r1]    */
> +	cmp	r0, r2			/* until source end address [r2]    */
> +	blo	copy_loop
> +
> +	/*
> +	 * fix .rel.dyn relocations
> +	 */
> +	ldr	r10, _dynsym_start_ofs	/* r10 <- sym table ofs */
> +	add	r10, r10, r9		/* r10 <- sym table in FLASH */
> +	ldr	r2, _rel_dyn_start_ofs	/* r2 <- rel dyn start ofs */
> +	add	r2, r2, r9		/* r2 <- rel dyn start in FLASH */
> +	ldr	r3, _rel_dyn_end_ofs	/* r3 <- rel dyn end ofs */
> +	add	r3, r3, r9		/* r3 <- rel dyn end in FLASH */

This is mixing relocate_code-relative offsets with the relocation offset!

> +fixloop:
> +	ldr	r0, [r2]		/* r0 <- SRC location to fix up */
> +	add	r0, r0, r9		/* r0 <- DST location to fix up */
> +	ldr	r1, [r2, #4]
> +	and	r7, r1, #0xff
> +	cmp	r7, #23			/* relative fixup? */
> +	beq	fixrel
> +	cmp	r7, #2			/* absolute fixup? */
> +	beq	fixabs
> +	/* ignore unknown type of fixup */
> +	b	fixnext
> +fixabs:
> +	/* absolute fix: set location to (offset) symbol value */
> +	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 */
> +	add	r1, r1, r9		/* r1 <- relocated sym addr */
> +	b	fixnext
> +fixrel:
> +	/* relative fix: increase location by offset */
> +	ldr	r1, [r0]
> +	add	r1, r1, r9
> +fixnext:
> +	str	r1, [r0]
> +	add	r2, r2, #8		/* each rel.dyn entry is 8 bytes */
> +	cmp	r2, r3
> +	blo	fixloop
> +
> +relocate_done:
> +
> +	/* ARMv4- don't know bx lr but the assembler fails to see that */
> +
> +#ifdef __ARM_ARCH_4__
> +        mov        pc, lr
> +#else
> +        bx        lr
> +#endif
> +
> +_image_copy_end_ofs:
> +	.word __image_copy_end - relocate_code
> +_rel_dyn_start_ofs:
> +	.word __rel_dyn_start - relocate_code
> +_rel_dyn_end_ofs:
> +	.word __rel_dyn_end - relocate_code
> +_dynsym_start_ofs:
> +	.word __dynsym_start - relocate_code
> +
> +ENDPROC(relocate_code)
> --
> 1.7.10.4

Best regards,
Beno?t