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

2013-05-12 Thread Albert ARIBAUD 
On Sat, 11 May 2013 22:13:51 +0200, Albert ARIBAUD
albert.u.b...@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,
-- 
Albert.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-05-11 Thread Albert ARIBAUD 
Hi Benoît,

On Sat, 11 May 2013 02:25:02 +0200 (CEST), Benoît Thébaudeau
benoit.thebaud...@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.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-05-11 Thread Benoît Thébaudeau 
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.thebaud...@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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-05-11 Thread Albert ARIBAUD 
Hi Benoît,

On Sat, 11 May 2013 19:52:17 +0200 (CEST), Benoît Thébaudeau
benoit.thebaud...@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.thebaud...@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,
-- 
Albert.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2013-05-10 Thread Albert ARIBAUD 

Signed-off-by: Albert ARIBAUD albert.u.b...@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
-_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
 {

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

2013-05-10 Thread Benoît Thébaudeau 
Hi Albert,

On Friday, May 10, 2013 11:56:50 PM, Albert ARIBAUD wrote:
 Signed-off-by: Albert ARIBAUD albert.u.b...@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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot