diff mbox

[U-Boot,v2,1/4] mx31pdk: copy SPL directly, not using relocate_code.

Message ID 1368525030-5162-2-git-send-email-albert.u.boot@aribaud.net
State Superseded
Delegated to: Albert ARIBAUD
Headers show

Commit Message

Albert ARIBAUD May 14, 2013, 9:50 a.m. UTC
Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
---
Changes in v2:
- dropped relocate_code() call from mx31pdk SPL

 board/freescale/mx31pdk/mx31pdk.c |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Comments

Benoît Thébaudeau May 14, 2013, 3:14 p.m. UTC | #1
Hi Albert,

On Tuesday, May 14, 2013 11:50:27 AM, Albert ARIBAUD wrote:
> Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> ---
> Changes in v2:
> - dropped relocate_code() call from mx31pdk SPL
> 
>  board/freescale/mx31pdk/mx31pdk.c |   16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/board/freescale/mx31pdk/mx31pdk.c
> b/board/freescale/mx31pdk/mx31pdk.c
> index 49158bd..4f6cfee 100644
> --- a/board/freescale/mx31pdk/mx31pdk.c
> +++ b/board/freescale/mx31pdk/mx31pdk.c
> @@ -39,7 +39,21 @@ DECLARE_GLOBAL_DATA_PTR;
>  #ifdef CONFIG_SPL_BUILD
>  void board_init_f(ulong bootflag)
>  {
> -	relocate_code(CONFIG_SPL_TEXT_BASE);
> +	/*
> +	 * copy ourselves from where we are running to where we were
> +	 * linked at. Use ulong pointers as all addresses involved
> +	 * are 4-byte-aligned.
> +	 */
> +	ulong *start_ptr, *end_ptr, *link_ptr, *run_ptr, *dst;
> +	asm volatile ("ldr %0, =_start" : "=r"(start_ptr));
> +	asm volatile ("ldr %0, =_end" : "=r"(end_ptr));

Why not __image_copy_start/end instead? I know that the result will be the same
here, but the naming would be more appropriate. The existing u-boot-spl.lds
still gives access to __image_copy_*.

> +	asm volatile ("ldr %0, =board_init_f" : "=r"(link_ptr));
> +	asm volatile ("adr %0, board_init_f" : "=r"(run_ptr));
> +	for (dst = start_ptr; dst < end_ptr; dst++)
> +		*dst = *(dst+(run_ptr-link_ptr));
> +	/*
> +	 * branch to nand_boot's link-time address.
> +	 */
>  	asm volatile("ldr pc, =nand_boot");
>  }
>  #endif
> --
> 1.7.10.4

Best regards,
Benoît
Albert ARIBAUD May 14, 2013, 4:13 p.m. UTC | #2
Hi Benoît,

On Tue, 14 May 2013 17:14:12 +0200 (CEST), Benoît Thébaudeau
<benoit.thebaudeau@advansee.com> wrote:

> Hi Albert,
> 
> On Tuesday, May 14, 2013 11:50:27 AM, Albert ARIBAUD wrote:
> > Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> > ---
> > Changes in v2:
> > - dropped relocate_code() call from mx31pdk SPL
> > 
> >  board/freescale/mx31pdk/mx31pdk.c |   16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/board/freescale/mx31pdk/mx31pdk.c
> > b/board/freescale/mx31pdk/mx31pdk.c
> > index 49158bd..4f6cfee 100644
> > --- a/board/freescale/mx31pdk/mx31pdk.c
> > +++ b/board/freescale/mx31pdk/mx31pdk.c
> > @@ -39,7 +39,21 @@ DECLARE_GLOBAL_DATA_PTR;
> >  #ifdef CONFIG_SPL_BUILD
> >  void board_init_f(ulong bootflag)
> >  {
> > -	relocate_code(CONFIG_SPL_TEXT_BASE);
> > +	/*
> > +	 * copy ourselves from where we are running to where we were
> > +	 * linked at. Use ulong pointers as all addresses involved
> > +	 * are 4-byte-aligned.
> > +	 */
> > +	ulong *start_ptr, *end_ptr, *link_ptr, *run_ptr, *dst;
> > +	asm volatile ("ldr %0, =_start" : "=r"(start_ptr));
> > +	asm volatile ("ldr %0, =_end" : "=r"(end_ptr));
> 
> Why not __image_copy_start/end instead? I know that the result will be the same
> here, but the naming would be more appropriate. The existing u-boot-spl.lds
> still gives access to __image_copy_*.

Well, yes, the naming seems appropriate, and I thought and said so
myself some time ago. But then, I realize that __image_copy_start and
__image_copy_end are tightly coupled with relocation, and I want to
avoid creating any additional ties between relocation and SPL just when
I am severing them. IOW, I want to keep the option of having a reduced
SPL linker file, distinct from the u-boot one, and where none of
__image_copy_*, __rel_dyn_* or __dynsym_start exist.

Amicalement,
Benoît Thébaudeau May 14, 2013, 5:10 p.m. UTC | #3
Hi Albert,

On Tuesday, May 14, 2013 6:13:55 PM, Albert ARIBAUD wrote:
> Hi Benoît,
> 
> On Tue, 14 May 2013 17:14:12 +0200 (CEST), Benoît Thébaudeau
> <benoit.thebaudeau@advansee.com> wrote:
> 
> > Hi Albert,
> > 
> > On Tuesday, May 14, 2013 11:50:27 AM, Albert ARIBAUD wrote:
> > > Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> > > ---
> > > Changes in v2:
> > > - dropped relocate_code() call from mx31pdk SPL
> > > 
> > >  board/freescale/mx31pdk/mx31pdk.c |   16 +++++++++++++++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/board/freescale/mx31pdk/mx31pdk.c
> > > b/board/freescale/mx31pdk/mx31pdk.c
> > > index 49158bd..4f6cfee 100644
> > > --- a/board/freescale/mx31pdk/mx31pdk.c
> > > +++ b/board/freescale/mx31pdk/mx31pdk.c
> > > @@ -39,7 +39,21 @@ DECLARE_GLOBAL_DATA_PTR;
> > >  #ifdef CONFIG_SPL_BUILD
> > >  void board_init_f(ulong bootflag)
> > >  {
> > > -	relocate_code(CONFIG_SPL_TEXT_BASE);
> > > +	/*
> > > +	 * copy ourselves from where we are running to where we were
> > > +	 * linked at. Use ulong pointers as all addresses involved
> > > +	 * are 4-byte-aligned.
> > > +	 */
> > > +	ulong *start_ptr, *end_ptr, *link_ptr, *run_ptr, *dst;
> > > +	asm volatile ("ldr %0, =_start" : "=r"(start_ptr));
> > > +	asm volatile ("ldr %0, =_end" : "=r"(end_ptr));
> > 
> > Why not __image_copy_start/end instead? I know that the result will be the
> > same
> > here, but the naming would be more appropriate. The existing u-boot-spl.lds
> > still gives access to __image_copy_*.
> 
> Well, yes, the naming seems appropriate, and I thought and said so
> myself some time ago. But then, I realize that __image_copy_start and
> __image_copy_end are tightly coupled with relocation, and I want to
> avoid creating any additional ties between relocation and SPL just when
> I am severing them. IOW, I want to keep the option of having a reduced
> SPL linker file, distinct from the u-boot one, and where none of
> __image_copy_*, __rel_dyn_* or __dynsym_start exist.

OK, that's fine.

Since _end is linker-generated contrary to the other symbols that you use in
those asm lines, have you checked that no R_ARM_ABS32 relocation data is
generated here? OTOH, -pie is not used with SPL, so there should be no
relocation data at all generated here, and the true final addresses should be in
the binary image.

Best regards,
Benoît
Albert ARIBAUD May 14, 2013, 6:24 p.m. UTC | #4
Hi Benoît,

On Tue, 14 May 2013 19:10:10 +0200 (CEST), Benoît Thébaudeau
<benoit.thebaudeau@advansee.com> wrote:

> Hi Albert,
> 
> On Tuesday, May 14, 2013 6:13:55 PM, Albert ARIBAUD wrote:
> > Hi Benoît,
> > 
> > On Tue, 14 May 2013 17:14:12 +0200 (CEST), Benoît Thébaudeau
> > <benoit.thebaudeau@advansee.com> wrote:
> > 
> > > Hi Albert,
> > > 
> > > On Tuesday, May 14, 2013 11:50:27 AM, Albert ARIBAUD wrote:
> > > > Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net>
> > > > ---
> > > > Changes in v2:
> > > > - dropped relocate_code() call from mx31pdk SPL
> > > > 
> > > >  board/freescale/mx31pdk/mx31pdk.c |   16 +++++++++++++++-
> > > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/board/freescale/mx31pdk/mx31pdk.c
> > > > b/board/freescale/mx31pdk/mx31pdk.c
> > > > index 49158bd..4f6cfee 100644
> > > > --- a/board/freescale/mx31pdk/mx31pdk.c
> > > > +++ b/board/freescale/mx31pdk/mx31pdk.c
> > > > @@ -39,7 +39,21 @@ DECLARE_GLOBAL_DATA_PTR;
> > > >  #ifdef CONFIG_SPL_BUILD
> > > >  void board_init_f(ulong bootflag)
> > > >  {
> > > > -	relocate_code(CONFIG_SPL_TEXT_BASE);
> > > > +	/*
> > > > +	 * copy ourselves from where we are running to where we were
> > > > +	 * linked at. Use ulong pointers as all addresses involved
> > > > +	 * are 4-byte-aligned.
> > > > +	 */
> > > > +	ulong *start_ptr, *end_ptr, *link_ptr, *run_ptr, *dst;
> > > > +	asm volatile ("ldr %0, =_start" : "=r"(start_ptr));
> > > > +	asm volatile ("ldr %0, =_end" : "=r"(end_ptr));
> > > 
> > > Why not __image_copy_start/end instead? I know that the result will be the
> > > same
> > > here, but the naming would be more appropriate. The existing u-boot-spl.lds
> > > still gives access to __image_copy_*.
> > 
> > Well, yes, the naming seems appropriate, and I thought and said so
> > myself some time ago. But then, I realize that __image_copy_start and
> > __image_copy_end are tightly coupled with relocation, and I want to
> > avoid creating any additional ties between relocation and SPL just when
> > I am severing them. IOW, I want to keep the option of having a reduced
> > SPL linker file, distinct from the u-boot one, and where none of
> > __image_copy_*, __rel_dyn_* or __dynsym_start exist.
> 
> OK, that's fine.
> 
> Since _end is linker-generated contrary to the other symbols that you use in
> those asm lines, have you checked that no R_ARM_ABS32 relocation data is
> generated here? OTOH, -pie is not used with SPL, so there should be no
> relocation data at all generated here, and the true final addresses should be in
> the binary image.

Indeed, SPL is meant to be loaded at its link-time address -- or, in the
case of mx31pdk and tx25, loaded in a read buffer then moved by itself
to its link-time address. In no case is SPL supposed to be relocated.
You can verify with readelf -r that no relocation records are generated
for SPL builds.

> Best regards,
> Benoît

Amicalement,
diff mbox

Patch

diff --git a/board/freescale/mx31pdk/mx31pdk.c b/board/freescale/mx31pdk/mx31pdk.c
index 49158bd..4f6cfee 100644
--- a/board/freescale/mx31pdk/mx31pdk.c
+++ b/board/freescale/mx31pdk/mx31pdk.c
@@ -39,7 +39,21 @@  DECLARE_GLOBAL_DATA_PTR;
 #ifdef CONFIG_SPL_BUILD
 void board_init_f(ulong bootflag)
 {
-	relocate_code(CONFIG_SPL_TEXT_BASE);
+	/*
+	 * copy ourselves from where we are running to where we were
+	 * linked at. Use ulong pointers as all addresses involved
+	 * are 4-byte-aligned.
+	 */
+	ulong *start_ptr, *end_ptr, *link_ptr, *run_ptr, *dst;
+	asm volatile ("ldr %0, =_start" : "=r"(start_ptr));
+	asm volatile ("ldr %0, =_end" : "=r"(end_ptr));
+	asm volatile ("ldr %0, =board_init_f" : "=r"(link_ptr));
+	asm volatile ("adr %0, board_init_f" : "=r"(run_ptr));
+	for (dst = start_ptr; dst < end_ptr; dst++)
+		*dst = *(dst+(run_ptr-link_ptr));
+	/*
+	 * branch to nand_boot's link-time address.
+	 */
 	asm volatile("ldr pc, =nand_boot");
 }
 #endif