| Message ID | 1368223012-17609-3-git-send-email-albert.u.boot@aribaud.net |
|---|---|
| State | Superseded |
| Delegated to: | Albert ARIBAUD |
| Headers | show |
Hi Albert, On Friday, May 10, 2013 11:56:50 PM, Albert ARIBAUD wrote: > Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net> > --- > arch/arm/cpu/arm1136/start.S | 7 +++---- > arch/arm/cpu/arm1136/u-boot-spl.lds | 3 ++- > arch/arm/cpu/arm720t/start.S | 11 +++++++---- > arch/arm/cpu/arm920t/ep93xx/u-boot.lds | 6 +++++- > arch/arm/cpu/arm926ejs/start.S | 7 +++---- > arch/arm/cpu/armv7/am33xx/u-boot-spl.lds | 2 -- > arch/arm/cpu/armv7/omap-common/u-boot-spl.lds | 2 -- > arch/arm/cpu/armv7/socfpga/u-boot-spl.lds | 1 - > arch/arm/cpu/armv7/start.S | 6 ++---- > arch/arm/cpu/ixp/u-boot.lds | 6 +++++- > arch/arm/cpu/u-boot-spl.lds | 3 +-- > arch/arm/cpu/u-boot.lds | 7 +++++-- > arch/arm/lib/sections.c | 4 +++- > 13 files changed, 36 insertions(+), 29 deletions(-) > > 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. > -_image_copy_end_ofs: > - .word __image_copy_end - _start > - > .globl _bss_end_ofs > _bss_end_ofs: > .word __bss_end - _start > @@ -239,6 +235,9 @@ relocate_done: > > bx lr > > +_image_copy_end_ofs: > + .word __image_copy_end - _start > + > #ifndef CONFIG_SPL_BUILD > > _rel_dyn_start_ofs: > 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. > + > _end = .; > > .bss : [...] > 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? > CPUDIR/start.o (.text*) > *(.text*) > } > @@ -57,7 +57,10 @@ SECTIONS > > . = ALIGN(4); > > - __image_copy_end = .; > + .image_copy_end : > + { > + *(.__image_copy_end); > + } > > .rel.dyn : { > __rel_dyn_start = .; > 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 > @@ -21,7 +21,7 @@ > */ > > /** > - * These two symbols are declared in a C file so that the linker > + * The following symbols are declared in a C file so that the linker > * uses R_ARM_RELATIVE relocation, rather than the R_ARM_ABS32 one > * it would use if the symbols were defined in the linker file. > * Using only R_ARM_RELATIVE relocation ensures that references to > @@ -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. > +char __image_copy_end[0] __attribute__((used, > section(".__image_copy_end"))); Best regards, Benoît
Hi Benoît, On Sat, 11 May 2013 02:25:02 +0200 (CEST), Benoît Thébaudeau <benoit.thebaudeau@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,
Hi Albert, On Saturday, May 11, 2013 10:02:48 AM, Albert ARIBAUD wrote: > Hi Benoît, > > On Sat, 11 May 2013 02:25:02 +0200 (CEST), Benoît Thébaudeau > <benoit.thebaudeau@advansee.com> wrote: > > > Hi Albert, [...] > > > 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. Then, why do you keep the old definition of __image_copy_end in such linker scripts? Probably because start.S can't be linked in otherwise, but this is no longer true at the end of this series with the new relocate.S that is garbage- collected for those SPLs. And in all cases, shouldn't all linker scripts requiring __image_copy_end be converted to the new definition? [...] Best regards, Benoît
Hi Benoît, On Sat, 11 May 2013 19:52:17 +0200 (CEST), Benoît Thébaudeau <benoit.thebaudeau@advansee.com> wrote: > Hi Albert, > > On Saturday, May 11, 2013 10:02:48 AM, Albert ARIBAUD wrote: > > Hi Benoît, > > > > On Sat, 11 May 2013 02:25:02 +0200 (CEST), Benoît Thébaudeau > > <benoit.thebaudeau@advansee.com> wrote: > > > > > Hi Albert, > > [...] > > > > > 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. > > Then, why do you keep the old definition of __image_copy_end in such linker > scripts? Probably because start.S can't be linked in otherwise, but this is no > longer true at the end of this series with the new relocate.S that is garbage- > collected for those SPLs. And in all cases, shouldn't all linker scripts > requiring __image_copy_end be converted to the new definition? You are right on both accounts: all linker scripts that need __image_copy_* should be converted to the new definition, and some linker scripts probably don't need __image_copy_end even though they define it. I intend to achieve this, not by editing various linker scripts, but by factoring them into a single linker script (or two at most, one for U-boot and one for SPL). So I prefer to do as little work on the scripts as possible right now; here, only what's needed for relocation factoring to work. Hence, if an SPL script sort-of-hardcodes image start and end (or relocation symbols as some do), then I prefer to leave it alone for now and handle in the linker script factoring series later. (on an unrelated note, V2 will have an additional patch removing absolute relocation record type support in relocate_code, as it should be useless now too. But I'll pair this removal with a build step that makes sure the ELF u-boot and SPL binaries only have R_ARM_RELATIVE relocation records.) > Best regards, > Benoît Amicalement,
On Sat, 11 May 2013 22:13:51 +0200, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote: > Hence, if an SPL script sort-of-hardcodes image start and end (or > relocation symbols as some do), then I prefer to leave it alone for now > and handle in the linker script factoring series later. ... but I do realize in some cases I removed __image_copy_end and in others I didn't. I'll go over this again in V2 by following the rule of least change -- not doing a change unless required by the objective of the series, that is, factoring relocate code. Amicalement,
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 -_image_copy_end_ofs: - .word __image_copy_end - _start - .globl _bss_end_ofs _bss_end_ofs: .word __bss_end - _start @@ -239,6 +235,9 @@ relocate_done: bx lr +_image_copy_end_ofs: + .word __image_copy_end - _start + #ifndef CONFIG_SPL_BUILD _rel_dyn_start_ofs: 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 = .; + _end = .; .bss : diff --git a/arch/arm/cpu/arm720t/start.S b/arch/arm/cpu/arm720t/start.S index 9facc7e..b85509c 100644 --- a/arch/arm/cpu/arm720t/start.S +++ b/arch/arm/cpu/arm720t/start.S @@ -101,10 +101,6 @@ _TEXT_BASE: _bss_start_ofs: .word __bss_start - _start -.globl _image_copy_end_ofs -_image_copy_end_ofs: - .word __image_copy_end - _start - .globl _bss_end_ofs _bss_end_ofs: .word __bss_end - _start @@ -221,6 +217,11 @@ relocate_done: mov pc, lr +_image_copy_end_ofs: + .word __image_copy_end - _start + +#ifndef CONFIG_SPL_BUILD + _rel_dyn_start_ofs: .word __rel_dyn_start - _start _rel_dyn_end_ofs: @@ -228,6 +229,8 @@ _rel_dyn_end_ofs: _dynsym_start_ofs: .word __dynsym_start - _start +#endif + .globl c_runtime_cpu_setup c_runtime_cpu_setup: diff --git a/arch/arm/cpu/arm920t/ep93xx/u-boot.lds b/arch/arm/cpu/arm920t/ep93xx/u-boot.lds index cf55bf7..2b32c0a 100644 --- a/arch/arm/cpu/arm920t/ep93xx/u-boot.lds +++ b/arch/arm/cpu/arm920t/ep93xx/u-boot.lds @@ -31,6 +31,7 @@ SECTIONS . = ALIGN(4); .text : { + *(.__image_copy_start) arch/arm/cpu/arm920t/start.o (.text*) /* the EP93xx expects to find the pattern 'CRUS' at 0x1000 */ . = 0x1000; @@ -56,7 +57,10 @@ SECTIONS . = ALIGN(4); - __image_copy_end = .; + .image_copy_end : + { + *(.__image_copy_end); + } __bss_start = .; .bss : { *(.bss*) } diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S index 4c56711..736361a 100644 --- a/arch/arm/cpu/arm926ejs/start.S +++ b/arch/arm/cpu/arm926ejs/start.S @@ -136,10 +136,6 @@ _TEXT_BASE: _bss_start_ofs: .word __bss_start - _start -.globl _image_copy_end_ofs -_image_copy_end_ofs: - .word __image_copy_end - _start - .globl _bss_end_ofs _bss_end_ofs: .word __bss_end - _start @@ -256,6 +252,9 @@ relocate_done: bx lr +_image_copy_end_ofs: + .word __image_copy_end - _start + #ifndef CONFIG_SPL_BUILD _rel_dyn_start_ofs: diff --git a/arch/arm/cpu/armv7/am33xx/u-boot-spl.lds b/arch/arm/cpu/armv7/am33xx/u-boot-spl.lds index b6a929f..29cefd0 100644 --- a/arch/arm/cpu/armv7/am33xx/u-boot-spl.lds +++ b/arch/arm/cpu/armv7/am33xx/u-boot-spl.lds @@ -37,7 +37,6 @@ SECTIONS { .text : { - __start = .; arch/arm/cpu/armv7/start.o (.text) *(.text*) } >.sram @@ -53,7 +52,6 @@ SECTIONS } >.sram . = ALIGN(4); - __image_copy_end = .; _end = .; .bss : diff --git a/arch/arm/cpu/armv7/omap-common/u-boot-spl.lds b/arch/arm/cpu/armv7/omap-common/u-boot-spl.lds index bd218c0..81cafe1 100644 --- a/arch/arm/cpu/armv7/omap-common/u-boot-spl.lds +++ b/arch/arm/cpu/armv7/omap-common/u-boot-spl.lds @@ -37,7 +37,6 @@ SECTIONS { .text : { - __start = .; arch/arm/cpu/armv7/start.o (.text*) *(.text*) } >.sram @@ -49,7 +48,6 @@ SECTIONS .data : { *(SORT_BY_ALIGNMENT(.data*)) } >.sram . = ALIGN(4); - __image_copy_end = .; _end = .; .bss : diff --git a/arch/arm/cpu/armv7/socfpga/u-boot-spl.lds b/arch/arm/cpu/armv7/socfpga/u-boot-spl.lds index 15f8c01..c0dcfd7 100644 --- a/arch/arm/cpu/armv7/socfpga/u-boot-spl.lds +++ b/arch/arm/cpu/armv7/socfpga/u-boot-spl.lds @@ -38,7 +38,6 @@ SECTIONS .data : { *(SORT_BY_ALIGNMENT(.data*)) } >.sdram . = ALIGN(4); - __image_copy_end = .; _end = .; .bss : { diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S index e9e57e6..3ade510 100644 --- a/arch/arm/cpu/armv7/start.S +++ b/arch/arm/cpu/armv7/start.S @@ -94,10 +94,6 @@ _TEXT_BASE: _bss_start_ofs: .word __bss_start - _start -.globl _image_copy_end_ofs -_image_copy_end_ofs: - .word __image_copy_end - _start - .globl _bss_end_ofs _bss_end_ofs: .word __bss_end - _start @@ -231,6 +227,8 @@ 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: diff --git a/arch/arm/cpu/ixp/u-boot.lds b/arch/arm/cpu/ixp/u-boot.lds index 553589c..1a0ba17 100644 --- a/arch/arm/cpu/ixp/u-boot.lds +++ b/arch/arm/cpu/ixp/u-boot.lds @@ -31,6 +31,7 @@ SECTIONS . = ALIGN(4); .text : { + *(.__image_copy_start) arch/arm/cpu/ixp/start.o(.text*) *(.text*) } @@ -54,7 +55,10 @@ SECTIONS . = ALIGN(4); - __image_copy_end = .; + .image_copy_end : + { + *(.__image_copy_end); + } .rel.dyn : { __rel_dyn_start = .; diff --git a/arch/arm/cpu/u-boot-spl.lds b/arch/arm/cpu/u-boot-spl.lds index 1408f03..87341c1 100644 --- a/arch/arm/cpu/u-boot-spl.lds +++ b/arch/arm/cpu/u-boot-spl.lds @@ -33,7 +33,6 @@ SECTIONS . = ALIGN(4); .text : { - __image_copy_start = .; CPUDIR/start.o (.text*) *(.text*) } @@ -80,7 +79,7 @@ SECTIONS } #if defined(CONFIG_SPL_MAX_SIZE) -ASSERT(__image_copy_end - __image_copy_start < (CONFIG_SPL_MAX_SIZE), \ +ASSERT(__image_copy_end - _start < (CONFIG_SPL_MAX_SIZE), \ "SPL image too big"); #endif 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) CPUDIR/start.o (.text*) *(.text*) } @@ -57,7 +57,10 @@ SECTIONS . = ALIGN(4); - __image_copy_end = .; + .image_copy_end : + { + *(.__image_copy_end); + } .rel.dyn : { __rel_dyn_start = .; 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 @@ -21,7 +21,7 @@ */ /** - * These two symbols are declared in a C file so that the linker + * The following symbols are declared in a C file so that the linker * uses R_ARM_RELATIVE relocation, rather than the R_ARM_ABS32 one * it would use if the symbols were defined in the linker file. * Using only R_ARM_RELATIVE relocation ensures that references to @@ -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"))); +char __image_copy_end[0] __attribute__((used, section(".__image_copy_end")));
Signed-off-by: Albert ARIBAUD <albert.u.boot@aribaud.net> --- arch/arm/cpu/arm1136/start.S | 7 +++---- arch/arm/cpu/arm1136/u-boot-spl.lds | 3 ++- arch/arm/cpu/arm720t/start.S | 11 +++++++---- arch/arm/cpu/arm920t/ep93xx/u-boot.lds | 6 +++++- arch/arm/cpu/arm926ejs/start.S | 7 +++---- arch/arm/cpu/armv7/am33xx/u-boot-spl.lds | 2 -- arch/arm/cpu/armv7/omap-common/u-boot-spl.lds | 2 -- arch/arm/cpu/armv7/socfpga/u-boot-spl.lds | 1 - arch/arm/cpu/armv7/start.S | 6 ++---- arch/arm/cpu/ixp/u-boot.lds | 6 +++++- arch/arm/cpu/u-boot-spl.lds | 3 +-- arch/arm/cpu/u-boot.lds | 7 +++++-- arch/arm/lib/sections.c | 4 +++- 13 files changed, 36 insertions(+), 29 deletions(-)