[U-Boot] [PATCH 2/4] arm: make __image_copy_{start, end} compiler-generated

[Albert ARIBAUD]

Hi Benoît,

On Sat, 11 May 2013 02:25:02 +0200 (CEST), Benoît Thébaudeau
<benoit.thebaudeau at advansee.com> wrote:

> Hi Albert,

> > diff --git a/arch/arm/cpu/arm1136/start.S b/arch/arm/cpu/arm1136/start.S
> > index ccea2d5..ab8fd56 100644
> > --- a/arch/arm/cpu/arm1136/start.S
> > +++ b/arch/arm/cpu/arm1136/start.S
> > @@ -104,10 +104,6 @@ _TEXT_BASE:
> >  _bss_start_ofs:
> >  	.word __bss_start - _start
> >  
> > -.globl _image_copy_end_ofs
> 
> Wasn't _image_copy_end_ofs used outside of start.S? Same question for all the
> start.S files.

No -- and the build would have failed if it was. The only user of
__image_copy_end_ofs is the start.S which defines it, and only for the
purpose of preventing the assembler from generating an R_ARM_ABS32
relocation to __image_copy_end.

> > diff --git a/arch/arm/cpu/arm1136/u-boot-spl.lds
> > b/arch/arm/cpu/arm1136/u-boot-spl.lds
> > index 8296e5d..04fc881 100644
> > --- a/arch/arm/cpu/arm1136/u-boot-spl.lds
> > +++ b/arch/arm/cpu/arm1136/u-boot-spl.lds
> > @@ -37,7 +37,6 @@ SECTIONS
> >  {
> >  	.text      :
> >  	{
> > -	__start = .;
> >  	  arch/arm/cpu/arm1136/start.o	(.text*)
> >  	  *(.text*)
> >  	} >.sram
> > @@ -48,7 +47,9 @@ SECTIONS
> >  	. = ALIGN(4);
> >  	.data : { *(SORT_BY_ALIGNMENT(.data*)) } >.sram
> >  	. = ALIGN(4);
> > +
> >  	__image_copy_end = .;
> 
> Why aren't all linker scripts treated equally?
> 
> Here, start.S is still used, so '*(.__image_copy_end)' and the related stuff
> should be like what you did for arch/arm/cpu/u-boot.lds below. Or am I missing
> something?
> 
> Same question for several other linker scripts below.

Not all SPLs use relocation -- actually, most SPLs do not use
relocation, and thus do not need image and relocaton section symbols.

> > diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> > index d9bbee3..5b43621 100644
> > --- a/arch/arm/cpu/u-boot.lds
> > +++ b/arch/arm/cpu/u-boot.lds
> > @@ -33,7 +33,7 @@ SECTIONS
> >  	. = ALIGN(4);
> >  	.text :
> >  	{
> > -		__image_copy_start = .;
> > +		*(.__image_copy_start)
> 
> Are there any users of __image_copy_start?

(see below)

> > diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
> > index 99eda59..80a0c38 100644
> > --- a/arch/arm/lib/sections.c
> +++ b/arch/arm/lib/sections.c

> > @@ -37,3 +37,5 @@
> >  
> >  char __bss_start[0] __attribute__((used, section(".__bss_start")));
> >  char __bss_end[0] __attribute__((used, section(".__bss_end")));
> > +char __image_copy_start[0] __attribute__((used,
> > section(".__image_copy_start")));
> 
> Ditto.

The only user of __image_copy_start is the relocation routine which
uses _start but should actually use __image_copy_start (will fix in V2),
to match with the semantics introduced when fixing CONFIG_SPL_MAX_SIZE
semantics in 6ebc3461.
 
> Best regards,
> Benoît

Thanks for your review!

Amicalement,
-- 
Albert.