Introduce generic relocation feature

 README                    |    4 ++++
 arch/arm/config.mk        |    3 +++
 arch/avr32/config.mk      |    3 +++
 arch/blackfin/config.mk   |    3 +++
 arch/m68k/config.mk       |    3 +++
 arch/microblaze/config.mk |    3 +++

 arch/mips/config.mk       |    3 +++
 arch/nds32/config.mk      |    3 +++
 arch/nios2/config.mk      |    3 +++
 arch/powerpc/config.mk    |    3 +++
 arch/sandbox/config.mk    |    3 +++
 arch/sh/config.mk         |    3 +++
 arch/sparc/config.mk      |    3 +++
 arch/x86/config.mk        |    3 +++

Quoted 6 lines · On Monday 20 February 2012 20:32:43 Simon Glass wrote:

?README ? ? ? ? ? ? ? ? ? ?| ? ?4 ++++
?arch/arm/config.mk ? ? ? ?| ? ?3 +++
?arch/avr32/config.mk ? ? ?| ? ?3 +++
?arch/blackfin/config.mk ? | ? ?3 +++
?arch/m68k/config.mk ? ? ? | ? ?3 +++
?arch/microblaze/config.mk | ? ?3 +++

Quoted 8 lines

?arch/mips/config.mk ? ? ? | ? ?3 +++
?arch/nds32/config.mk ? ? ?| ? ?3 +++
?arch/nios2/config.mk ? ? ?| ? ?3 +++
?arch/powerpc/config.mk ? ?| ? ?3 +++
?arch/sandbox/config.mk ? ?| ? ?3 +++
?arch/sh/config.mk ? ? ? ? | ? ?3 +++
?arch/sparc/config.mk ? ? ?| ? ?3 +++
?arch/x86/config.mk ? ? ? ?| ? ?3 +++

why config.mk and not asm/config.h ?

-mike

The __relocate_code() function is what we expect all architectures which support relocation will use eventually. For now, they all override this with their own version.

Hi Simon,

Le 21/02/2012 02:32, Simon Glass a ?crit :

Quoted 3 lines

The __relocate_code() function is what we expect all architectures which support relocation will use eventually. For now, they all override this with their own version.

Why the double underscore?

Amicalement,
--
Albert.

We don't want this in a common file, or at least not yet, so add this function to every start.S individually. The existing code tacked on the end of a long relocation function and does not suit our needs since it doesn't allow the address of board_init_r() to be passed in and cannot be called from C since it expects values in registers r4 and above.

Signed-off-by Simon Glass<[email protected]>

---
Changes in v4:
- Put start_call_board_init_r() into each start.S, sadly
- Split out start_call_board_init_r() addition into new patch

  arch/arm/cpu/arm1136/start.S   |   19 +++++++++++++++++++
  arch/arm/cpu/arm1176/start.S   |   19 +++++++++++++++++++
  arch/arm/cpu/arm720t/start.S   |   19 +++++++++++++++++++
  arch/arm/cpu/arm920t/start.S   |   19 +++++++++++++++++++
  arch/arm/cpu/arm925t/start.S   |   19 +++++++++++++++++++
  arch/arm/cpu/arm926ejs/start.S |   19 +++++++++++++++++++
  arch/arm/cpu/arm946es/start.S  |   19 +++++++++++++++++++
  arch/arm/cpu/arm_intcm/start.S |   19 +++++++++++++++++++
  arch/arm/cpu/armv7/start.S     |   19 +++++++++++++++++++
  arch/arm/cpu/ixp/start.S       |   19 +++++++++++++++++++
  arch/arm/cpu/lh7a40x/start.S   |   19 +++++++++++++++++++
  arch/arm/cpu/pxa/start.S       |   20 ++++++++++++++++++++
  arch/arm/cpu/s3c44b0/start.S   |   19 +++++++++++++++++++
  arch/arm/cpu/sa1100/start.S    |   19 +++++++++++++++++++
  14 files changed, 267 insertions(+), 0 deletions(-)

arch/arm/cpu/arm1136/start.S +0 -0

index 28e059e..e74d5f9 100644

Hi Simon,

Le 21/02/2012 02:32, Simon Glass a ?crit :

Quoted 6 lines

We don't want this in a common file, or at least not yet, so add this function to every start.S individually. The existing code tacked on the end of a long relocation function and does not suit our needs since it doesn't allow the address of board_init_r() to be passed in and cannot be called from C since it expects values in registers r4 and above.

It is not really an addition, as every start.S file involved already has this sequence at the end of its relocate_code routine.

Quoted 30 lines

Signed-off-by Simon Glass<[email protected]>

---
Changes in v4:
- Put start_call_board_init_r() into each start.S, sadly
- Split out start_call_board_init_r() addition into new patch

?arch/arm/cpu/arm1136/start.S ? | ? 19 +++++++++++++++++++
?arch/arm/cpu/arm1176/start.S ? | ? 19 +++++++++++++++++++
?arch/arm/cpu/arm720t/start.S ? | ? 19 +++++++++++++++++++
?arch/arm/cpu/arm920t/start.S ? | ? 19 +++++++++++++++++++
?arch/arm/cpu/arm925t/start.S ? | ? 19 +++++++++++++++++++
?arch/arm/cpu/arm926ejs/start.S | ? 19 +++++++++++++++++++
?arch/arm/cpu/arm946es/start.S ?| ? 19 +++++++++++++++++++
?arch/arm/cpu/arm_intcm/start.S | ? 19 +++++++++++++++++++
?arch/arm/cpu/armv7/start.S ? ? | ? 19 +++++++++++++++++++
?arch/arm/cpu/ixp/start.S ? ? ? | ? 19 +++++++++++++++++++
?arch/arm/cpu/lh7a40x/start.S ? | ? 19 +++++++++++++++++++
?arch/arm/cpu/pxa/start.S ? ? ? | ? 20 ++++++++++++++++++++
?arch/arm/cpu/s3c44b0/start.S ? | ? 19 +++++++++++++++++++
?arch/arm/cpu/sa1100/start.S ? ?| ? 19 +++++++++++++++++++

?14 files changed, 267 insertions(+), 0 deletions(-)

arch/arm/cpu/arm1136/start.S +0 -0

index 28e059e..e74d5f9 100644
@@ -295,6 +295,25 @@ _board_init_r_ofs:

? ? ? ?.word board_init_r - _start
?#endif

Quoted 1 line

+/**

Do we agree to Doxygen annotations in U-Boot?

Quoted 11 lines

+ * Jump to board_init_r with a new stack pointer
+ *
+ * @param gd ? Pointer to global data
+ * @param dest_addr ? ?Destination address from global data
+ * @param func Address of board_init_r function (relocated)
+ * @param sp ? New stack pointer
+ */
+.globl start_call_board_init_r +start_call_board_init_r:

Why "start_call_board_init_r"? "start_" may mislead readers into thinking we're starting something there and finishing it elsewhere, and "call_board_init_r" doesn't tell we're switching stacks etc.

I suggest "pivot_to_board_init_r", in analogy with Linux 'pivot_root'. Other suggestions are welcome.

Better yet, we could avoid this new function altogether and make start.S much cleaner by rewriting the startup sequence as:

1. start.S sets up the initial stack
2. start.S calls C function board_init_f
3. board_init_f allocates initial GD and stores final stack, final GD, ? and final base address within initial GD, then returns normally
4. start.S sets up the final stack and final GD
4. start.S calls C function relocate_code using relevant GD fields ? as its arguments
5. relocate_code returns normally
6. start.S calls either nand_boot or board_init_r()

This way all the logic would be in start.S and board_init_f(), relocate_code() and board_init_r() would be simple and independent functions.

Amicalement,
--
Albert.

Hi Albert,

Quoted 46 lines · Albert ARIBAUD

Hi Simon,

Le 21/02/2012 02:32, Simon Glass a ?crit :

Quoted 6 lines

We don't want this in a common file, or at least not yet, so add this function to every start.S individually. The existing code tacked on the end of a long relocation function and does not suit our needs since it doesn't allow the address of board_init_r() to be passed in and cannot be called from C since it expects values in registers r4 and above.

It is not really an addition, as every start.S file involved already has this sequence at the end of its relocate_code routine.

Quoted 30 lines

Signed-off-by Simon Glass<[email protected]>

---
Changes in v4:
- Put start_call_board_init_r() into each start.S, sadly
- Split out start_call_board_init_r() addition into new patch

?arch/arm/cpu/arm1136/start.S ? | ? 19 +++++++++++++++++++
?arch/arm/cpu/arm1176/start.S ? | ? 19 +++++++++++++++++++
?arch/arm/cpu/arm720t/start.S ? | ? 19 +++++++++++++++++++
?arch/arm/cpu/arm920t/start.S ? | ? 19 +++++++++++++++++++
?arch/arm/cpu/arm925t/start.S ? | ? 19 +++++++++++++++++++
?arch/arm/cpu/arm926ejs/start.S | ? 19 +++++++++++++++++++
?arch/arm/cpu/arm946es/start.S ?| ? 19 +++++++++++++++++++
?arch/arm/cpu/arm_intcm/start.S | ? 19 +++++++++++++++++++
?arch/arm/cpu/armv7/start.S ? ? | ? 19 +++++++++++++++++++
?arch/arm/cpu/ixp/start.S ? ? ? | ? 19 +++++++++++++++++++
?arch/arm/cpu/lh7a40x/start.S ? | ? 19 +++++++++++++++++++
?arch/arm/cpu/pxa/start.S ? ? ? | ? 20 ++++++++++++++++++++
?arch/arm/cpu/s3c44b0/start.S ? | ? 19 +++++++++++++++++++
?arch/arm/cpu/sa1100/start.S ? ?| ? 19 +++++++++++++++++++

?14 files changed, 267 insertions(+), 0 deletions(-)

arch/arm/cpu/arm1136/start.S +0 -0

index 28e059e..e74d5f9 100644
@@ -295,6 +295,25 @@ _board_init_r_ofs:

? ? ? ?.word board_init_r - _start
?#endif

Quoted 4 lines

Quoted 1 line

+/**

Do we agree to Doxygen annotations in U-Boot?

I hope so, it makes things easy to read for those with editors that support it.

Quoted 13 lines

Quoted 11 lines

+ * Jump to board_init_r with a new stack pointer
+ *
+ * @param gd ? Pointer to global data
+ * @param dest_addr ? ?Destination address from global data
+ * @param func Address of board_init_r function (relocated)
+ * @param sp ? New stack pointer
+ */
+.globl start_call_board_init_r +start_call_board_init_r:

Quoted 6 lines

Why "start_call_board_init_r"? "start_" may mislead readers into thinking we're starting something there and finishing it elsewhere, and "call_board_init_r" doesn't tell we're switching stacks etc.

I suggest "pivot_to_board_init_r", in analogy with Linux 'pivot_root'. Other suggestions are welcome.

OK I will go with pivot_to_board_init_r.

Quoted 15 lines

Better yet, we could avoid this new function altogether and make start.S much cleaner by rewriting the startup sequence as:

1. start.S sets up the initial stack
2. start.S calls C function board_init_f
3. board_init_f allocates initial GD and stores final stack, final GD, ? and final base address within initial GD, then returns normally
4. start.S sets up the final stack and final GD
4. start.S calls C function relocate_code using relevant GD fields ? as its arguments
5. relocate_code returns normally
6. start.S calls either nand_boot or board_init_r()

Quoted 3 lines

This way all the logic would be in start.S and board_init_f(), relocate_code() and board_init_r() would be simple and independent functions.

Please see the generic relocation effort which I think is a better place to put this. If you like I can revisit this once we have that sorted out.

Quoted 2 lines

These basic functions are needed by relocation. To avoid bringing in all string.c functions (and the resulting code bloat for architectures where -ffunction-sections is not used), move these into their own file.

Quoted 3 lines · On Monday 20 February 2012 20:32:47 Simon Glass wrote:

These basic functions are needed by relocation. To avoid bringing in all string.c functions (and the resulting code bloat for architectures where -ffunction-sections is not used), move these into their own file.

seems like fixing -f{data,function}-sections would be a more useful goal. ?i don't know why arm doesn't support this today ... it's fairly easy to do.

-mike

Quoted 8 lines · Mike Frysinger

Quoted 3 lines · On Monday 20 February 2012 20:32:47 Simon Glass wrote:

These basic functions are needed by relocation. To avoid bringing in all string.c functions (and the resulting code bloat for architectures where -ffunction-sections is not used), move these into their own file.

seems like fixing -f{data,function}-sections would be a more useful goal.
i don't know why arm doesn't support this today ... it's fairly easy to
do.

I did enable it for Tegra at one point with no ill effects except for breaking warmboot, which relied on function ordering. I notice the no-toplevel-reorder flag on x86 but not with ARM. But I assumed there must be some reason it is not used and thought this patch safer, since it is just a minor tweak on a series that has has some review.

Quoted 10 lines · On Tuesday 06 March 2012 01:34:24 Simon Glass wrote:

Quoted 8 lines · Mike Frysinger

Quoted 3 lines · On Monday 20 February 2012 20:32:47 Simon Glass wrote:

These basic functions are needed by relocation. To avoid bringing in all string.c functions (and the resulting code bloat for architectures where -ffunction-sections is not used), move these into their own file.

seems like fixing -f{data,function}-sections would be a more useful goal. ?i don't know why arm doesn't support this today ... it's fairly easy to do.

Quoted 5 lines

I did enable it for Tegra at one point with no ill effects except for breaking warmboot, which relied on function ordering. I notice the no-toplevel-reorder flag on x86 but not with ARM. But I assumed there must be some reason it is not used and thought this patch safer, since it is just a minor tweak on a series that has has some review.

if we keep assuming that, then it'll never get fixed, and we'll sit on a steady stream of "small safe patches". ?plus, fixing this yields general improvements in anti-bloat for everyone.

function ordering sounds like something that should be imposed at the linker level ... for Blackfin, all i had to care about was the entry point was at the start of the .text, although warmbooting is fairly low level magic, and i handle that in the SPL-like core.

-mike

Quoted 10 lines · On Tuesday 06 March 2012 01:34:24 Simon Glass wrote:

Quoted 8 lines · Mike Frysinger

Quoted 3 lines · On Monday 20 February 2012 20:32:47 Simon Glass wrote:

These basic functions are needed by relocation. To avoid bringing in all string.c functions (and the resulting code bloat for architectures where -ffunction-sections is not used), move these into their own file.

seems like fixing -f{data,function}-sections would be a more useful goal. ?i don't know why arm doesn't support this today ... it's fairly easy to do.

Quoted 5 lines

I did enable it for Tegra at one point with no ill effects except for breaking warmboot, which relied on function ordering. I notice the no-toplevel-reorder flag on x86 but not with ARM. But I assumed there must be some reason it is not used and thought this patch safer, since it is just a minor tweak on a series that has has some review.

if we keep assuming that, then it'll never get fixed, and we'll sit on a steady stream of "small safe patches". ?plus, fixing this yields general improvements in anti-bloat for everyone.

function ordering sounds like something that should be imposed at the linker level ... for Blackfin, all i had to care about was the entry point was at the start of the .text, although warmbooting is fairly low level magic, and i handle that in the SPL-like core.

Hi Mike,

Quoted 17 lines · Mike Frysinger

Quoted 10 lines · On Tuesday 06 March 2012 01:34:24 Simon Glass wrote:

Quoted 8 lines · Mike Frysinger

Quoted 3 lines · On Monday 20 February 2012 20:32:47 Simon Glass wrote:

These basic functions are needed by relocation. To avoid bringing in all string.c functions (and the resulting code bloat for architectures where -ffunction-sections is not used), move these into their own file.

seems like fixing -f{data,function}-sections would be a more useful goal. ?i don't know why arm doesn't support this today ... it's fairly easy to do.

Quoted 5 lines

I did enable it for Tegra at one point with no ill effects except for breaking warmboot, which relied on function ordering. I notice the no-toplevel-reorder flag on x86 but not with ARM. But I assumed there must be some reason it is not used and thought this patch safer, since it is just a minor tweak on a series that has has some review.

Quoted 8 lines

if we keep assuming that, then it'll never get fixed, and we'll sit on a steady stream of "small safe patches". ?plus, fixing this yields general improvements in anti-bloat for everyone.

function ordering sounds like something that should be imposed at the linker level ... for Blackfin, all i had to care about was the entry point was at the start of the .text, although warmbooting is fairly low level magic, and i handle that in the SPL-like core.

Can't the entry point be forced to be at the start of the u-boot image by putting that function in a dedicated section and specifying that section as the first in the linker script?

This has always bothered me:

#########################################################################
# U-Boot objects....order is important (i.e. start must be first)

OBJS ?= $(CPUDIR)/start.o
ifeq ($(CPU),x86)
OBJS += $(CPUDIR)/start16.o
OBJS += $(CPUDIR)/resetvec.o
endif
ifeq ($(CPU),ppc4xx)
OBJS += $(CPUDIR)/resetvec.o
endif
ifeq ($(CPU),mpc85xx)
OBJS += $(CPUDIR)/resetvec.o
endif

I've been meaning to get rid of this for x86 as I believe this can be adequately handled by putting the code in relevant sections and locating the sections at the correct location using the linker script.

For x86 there are only two requirements:
?- The first byte of resetvec.o sits 16 bytes from the end of flash ?- The first byte of start.o is the first byte of the core u-boot image ? (there is other cruft above it in the flash like an embedded kernel ? ?image)

Regards,

Graeme

Quoted 22 lines · Mike Frysinger

Quoted 11 lines · On Tuesday 06 March 2012 01:34:24 Simon Glass wrote:

Quoted 9 lines · Mike Frysinger

Quoted 4 lines · On Monday 20 February 2012 20:32:47 Simon Glass wrote:

These basic functions are needed by relocation. To avoid bringing in all string.c functions (and the resulting code bloat for architectures where -ffunction-sections is not used), move these into their own file.

seems like fixing -f{data,function}-sections would be a more useful goal. i don't know why arm doesn't support this today ... it's fairly easy to do.

Quoted 5 lines

I did enable it for Tegra at one point with no ill effects except for breaking warmboot, which relied on function ordering. I notice the no-toplevel-reorder flag on x86 but not with ARM. But I assumed there must be some reason it is not used and thought this patch safer, since it is just a minor tweak on a series that has has some review.

if we keep assuming that, then it'll never get fixed, and we'll sit on a steady stream of "small safe patches". plus, fixing this yields general improvements in anti-bloat for everyone.

Quoted 4 lines

function ordering sounds like something that should be imposed at the linker level ... for Blackfin, all i had to care about was the entry point was at the start of the .text, although warmbooting is fairly low level magic, and i handle that in the SPL-like core.

Can't the entry point be forced to be at the start of the u-boot image by putting that function in a dedicated section and specifying that section as the first in the linker script?

This fixes the following warnings in an SPL build when libcommon is in use:

spl.c:37: warning: 'gdata' defined but not used
spl.c:38: warning: 'bdata' defined but not used

Signed-off-by Simon Glass <[email protected]>

This fixes the following warnings in an SPL build when libcommon is in use:

spl.c:37: warning: 'gdata' defined but not used
spl.c:38: warning: 'bdata' defined but not used

Signed-off-by Simon Glass <[email protected]>

---
Changes in v4:
- Add new patch to fix davinci build warnings

 arch/arm/cpu/arm926ejs/davinci/spl.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

arch/arm/cpu/arm926ejs/davinci/spl.c +0 -0

index b1eff26..2861907 100644

hi Simon,

Quoted 22 lines · Simon Glass

This fixes the following warnings in an SPL build when libcommon is in use:

spl.c:37: warning: 'gdata' defined but not used
spl.c:38: warning: 'bdata' defined but not used

Signed-off-by Simon Glass <[email protected]>

---
Changes in v4:
- Add new patch to fix davinci build warnings

?arch/arm/cpu/arm926ejs/davinci/spl.c | ? ?2 ++
?1 files changed, 2 insertions(+), 0 deletions(-)

arch/arm/cpu/arm926ejs/davinci/spl.c +0 -0

index b1eff26..2861907 100644
@@ -32,10 +32,12 @@
 

?#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT

Quoted 6 lines

+#ifdef CONFIG_SPL_SPI_LOAD ?DECLARE_GLOBAL_DATA_PTR;
?/* Define global data structure pointer to it*/ ?static gd_t gdata __attribute__ ((section(".data"))); ?static bd_t bdata __attribute__ ((section(".data"))); +#endif

?Can you specify which boards you get this warning for. With your ?patch to add libcommon to hawkboard's spl image, this is now also ?needed for hawkboard which uses CONFIG_SPL_NAND_LOAD.

-sughosh

Hi Sughosh,

Quoted 25 lines · Sughosh Ganu

hi Simon,

Quoted 22 lines · Simon Glass

This fixes the following warnings in an SPL build when libcommon is in use:

spl.c:37: warning: 'gdata' defined but not used
spl.c:38: warning: 'bdata' defined but not used

Signed-off-by Simon Glass <[email protected]>

---
Changes in v4:
- Add new patch to fix davinci build warnings

?arch/arm/cpu/arm926ejs/davinci/spl.c | ? ?2 ++
?1 files changed, 2 insertions(+), 0 deletions(-)

arch/arm/cpu/arm926ejs/davinci/spl.c +0 -0

index b1eff26..2861907 100644
@@ -32,10 +32,12 @@
 

?#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT

Quoted 10 lines

Quoted 6 lines

+#ifdef CONFIG_SPL_SPI_LOAD ?DECLARE_GLOBAL_DATA_PTR;
?/* Define global data structure pointer to it*/ ?static gd_t gdata __attribute__ ((section(".data"))); ?static bd_t bdata __attribute__ ((section(".data"))); +#endif

?Can you specify which boards you get this warning for. With your ?patch to add libcommon to hawkboard's spl image, this is now also ?needed for hawkboard which uses CONFIG_SPL_NAND_LOAD.

Perhaps it is any davinci board, with SPI? I don't have any of these - I was just fixing what I thought was a minor #ifdef bug in the code.

hi Simon,

Quoted 26 lines · Simon Glass

Hi Sughosh,

Quoted 23 lines · Sughosh Ganu

hi Simon,

Quoted 20 lines · Simon Glass

This fixes the following warnings in an SPL build when libcommon is in use:

spl.c:37: warning: 'gdata' defined but not used
spl.c:38: warning: 'bdata' defined but not used

Signed-off-by Simon Glass <[email protected]>

---
Changes in v4:
- Add new patch to fix davinci build warnings

?arch/arm/cpu/arm926ejs/davinci/spl.c | ? ?2 ++
?1 files changed, 2 insertions(+), 0 deletions(-)

arch/arm/cpu/arm926ejs/davinci/spl.c +0 -0

index b1eff26..2861907 100644
@@ -32,10 +32,12 @@
 

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

?#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT

+#ifdef CONFIG_SPL_SPI_LOAD ?DECLARE_GLOBAL_DATA_PTR;
?/* Define global data structure pointer to it*/ ?static gd_t gdata __attribute__ ((section(".data"))); ?static bd_t bdata __attribute__ ((section(".data"))); +#endif

Quoted 3 lines

Quoted 3 lines

?Can you specify which boards you get this warning for. With your ?patch to add libcommon to hawkboard's spl image, this is now also ?needed for hawkboard which uses CONFIG_SPL_NAND_LOAD.

Hi,

On Mon, Feb 27, 2012 at 11:16 AM, Sughosh Ganu <[email protected]> wrote:

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

?arch/arm/cpu/arm926ejs/davinci/spl.c | ? ?2 ++
?1 files changed, 2 insertions(+), 0 deletions(-)

arch/arm/cpu/arm926ejs/davinci/spl.c +0 -0

index b1eff26..2861907 100644
@@ -32,10 +32,12 @@
 

?#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT

+#ifdef CONFIG_SPL_SPI_LOAD ?DECLARE_GLOBAL_DATA_PTR;
?/* Define global data structure pointer to it*/ ?static gd_t gdata __attribute__ ((section(".data"))); ?static bd_t bdata __attribute__ ((section(".data"))); +#endif

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

?Can you specify which boards you get this warning for. With your ?patch to add libcommon to hawkboard's spl image, this is now also ?needed for hawkboard which uses CONFIG_SPL_NAND_LOAD.

Simon's patch is for the hawkboard, since due to another patch in his patchset LIBCOMMON is enabled in hawkboard's SPL. Now we have a board that boots from NAND with SPL and has LIBCOMMON enabled (Simon, I did not check the rest of your patchset, why do you need LIBCOMMON on the hawkboard at all?)

However, the patch fixes the warning, but now we use putc and puts from LIBCOMMON without initializing them. Thus it breaks the console output of the hawkboard SPL.

So the correct solution would be to keep the ifdefs around the definition of gdata and bdata as they are, but change board_init_f() like this:

void board_init_r(gd_t *id, ulong dummy)
{
#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
mem_malloc_init(CONFIG_SYS_TEXT_BASE - CONFIG_SYS_MALLOC_LEN,
CONFIG_SYS_MALLOC_LEN);

hi Christian,

Quoted 3 lines · Christian Riesch

Hi,

On Mon, Feb 27, 2012 at 11:16 AM, Sughosh Ganu <[email protected]> wrote:

<snip>

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

?arch/arm/cpu/arm926ejs/davinci/spl.c | ? ?2 ++
?1 files changed, 2 insertions(+), 0 deletions(-)

arch/arm/cpu/arm926ejs/davinci/spl.c +0 -0

index b1eff26..2861907 100644
@@ -32,10 +32,12 @@
 

?#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT

+#ifdef CONFIG_SPL_SPI_LOAD ?DECLARE_GLOBAL_DATA_PTR;
?/* Define global data structure pointer to it*/ ?static gd_t gdata __attribute__ ((section(".data"))); ?static bd_t bdata __attribute__ ((section(".data"))); +#endif

<snip>

Quoted 9 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

?Can you specify which boards you get this warning for. With your ?patch to add libcommon to hawkboard's spl image, this is now also ?needed for hawkboard which uses CONFIG_SPL_NAND_LOAD.

Simon's patch is for the hawkboard, since due to another patch in his patchset LIBCOMMON is enabled in hawkboard's SPL. Now we have a board that boots from NAND with SPL and has LIBCOMMON enabled (Simon, I did not check the rest of your patchset, why do you need LIBCOMMON on the hawkboard at all?)

?LIBCOMMON is now needed as the generic relocation based functions ?are part of the libcommon.o, which are being enabled in the same ?patchset for all arm boards. So if i understand correct, all arm ?board based spl's now need libcommon and libgeneric.

?The only thing i see is that libcommon and libgeneric are not ?defined for cam_enc_4xx board which uses spl, and this patchset does ?not add it either. Not sure whether it got missed.

Quoted 7 lines

However, the patch fixes the warning, but now we use putc and puts from LIBCOMMON without initializing them. Thus it breaks the console output of the hawkboard SPL.

So the correct solution would be to keep the ifdefs around the definition of gdata and bdata as they are, but change board_init_f() like this:

?I have a patch for this, which i will send today. There is only one ?question i have though.

Quoted 1 line

Quoted 5 lines

void board_init_r(gd_t *id, ulong dummy)
{
#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
? ? ? ? mem_malloc_init(CONFIG_SYS_TEXT_BASE - CONFIG_SYS_MALLOC_LEN, ? ? ? ? ? ? ? ? ? ? ? ? CONFIG_SYS_MALLOC_LEN);

?Can you please explain why we need the mem_malloc_init. I did not ?include this, and spl boots up just fine on my board.

Hi Sughosh,

Quoted 39 lines · Sughosh Ganu

hi Christian,

Quoted 3 lines · Christian Riesch

Hi,

On Mon, Feb 27, 2012 at 11:16 AM, Sughosh Ganu <[email protected]> wrote:

<snip>

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

?arch/arm/cpu/arm926ejs/davinci/spl.c | ? ?2 ++
?1 files changed, 2 insertions(+), 0 deletions(-)

arch/arm/cpu/arm926ejs/davinci/spl.c +0 -0

index b1eff26..2861907 100644
@@ -32,10 +32,12 @@
 

?#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT

+#ifdef CONFIG_SPL_SPI_LOAD ?DECLARE_GLOBAL_DATA_PTR;
?/* Define global data structure pointer to it*/ ?static gd_t gdata __attribute__ ((section(".data"))); ?static bd_t bdata __attribute__ ((section(".data"))); +#endif

<snip>

Quoted 9 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

?Can you specify which boards you get this warning for. With your ?patch to add libcommon to hawkboard's spl image, this is now also ?needed for hawkboard which uses CONFIG_SPL_NAND_LOAD.

Simon's patch is for the hawkboard, since due to another patch in his patchset LIBCOMMON is enabled in hawkboard's SPL. Now we have a board that boots from NAND with SPL and has LIBCOMMON enabled (Simon, I did not check the rest of your patchset, why do you need LIBCOMMON on the hawkboard at all?)

Quoted 8 lines

?LIBCOMMON is now needed as the generic relocation based functions ?are part of the libcommon.o, which are being enabled in the same ?patchset for all arm boards. So if i understand correct, all arm ?board based spl's now need libcommon and libgeneric.

?The only thing i see is that libcommon and libgeneric are not ?defined for cam_enc_4xx board which uses spl, and this patchset does ?not add it either. Not sure whether it got missed.

When I asked Heiko Schocher a few month ago why he defined putc and puts in arch/arm/cpu/arm926ejs/davinci/spl.c he replied that he could not use LIBCOMMON due to size limitations for the SPL. So I guess that this board will not be able to use the generic relocation functions, unless the SPL is smaller than 16kB, right? Simon's patchset will break this board then, right?

Quoted 6 lines

Quoted 5 lines

void board_init_r(gd_t *id, ulong dummy)
{
#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
? ? ? ? mem_malloc_init(CONFIG_SYS_TEXT_BASE - CONFIG_SYS_MALLOC_LEN, ? ? ? ? ? ? ? ? ? ? ? ? CONFIG_SYS_MALLOC_LEN);

Quoted 3 lines

?Can you please explain why we need the mem_malloc_init. I did not ?include this, and spl boots up just fine on my board.

malloc is required for the SPI code only, so you could also put it within #ifdef CONFIG_SPL_SPI_LOAD

Quoted 6 lines

When I asked Heiko Schocher a few month ago why he defined putc and puts in arch/arm/cpu/arm926ejs/davinci/spl.c he replied that he could not use LIBCOMMON due to size limitations for the SPL. So I guess that this board will not be able to use the generic relocation functions, unless the SPL is smaller than 16kB, right? Simon's patchset will break this board then, right?

That is exactly what i reported in one of the threads in response to addition of libcommon and libgeneric to the hawkboard's spl. In fact, this might cause problems on quite a few boards with spl size restrictions. I am not sure, whether the generic relocation feature should be turned on by default on all boards or should be a config option -- at least for the spl builds. Another option would be to move it to a place where it is not needed to compile in the entire libcommon/libgeneric support that is not needed for the generic relocation code. I think that would help us keep the generic relocation without the size bloat that we see right now.

   http://lists.denx.de/pipermail/u-boot/2012-February/118567.html

Le 27/02/2012 13:02, Sughosh Ganu a ?crit :

Quoted 18 lines

Quoted 6 lines

When I asked Heiko Schocher a few month ago why he defined putc and puts in arch/arm/cpu/arm926ejs/davinci/spl.c he replied that he could not use LIBCOMMON due to size limitations for the SPL. So I guess that this board will not be able to use the generic relocation functions, unless the SPL is smaller than 16kB, right? Simon's patchset will break this board then, right?

That is exactly what i reported in one of the threads in response to addition of libcommon and libgeneric to the hawkboard's spl. In fact, this might cause problems on quite a few boards with spl size restrictions. I am not sure, whether the generic relocation feature should be turned on by default on all boards or should be a config option -- at least for the spl builds. Another option would be to move it to a place where it is not needed to compile in the entire libcommon/libgeneric support that is not needed for the generic relocation code. I think that would help us keep the generic relocation without the size bloat that we see right now.

Quoted 1 line

   http://lists.denx.de/pipermail/u-boot/2012-February/118567.html

Sorry for appearing dumb, but can someone explain to me how SPL relates to relocation in the first place? I thought SPL was meant to be a preloader for the full(er) U-boot, small enough to be loaded by some SoCs' ROM code and possibly even to fit in SRAM. Why does it need relocation? And if it does not, how come it is affected by a rework of the relocation feature? I really would like a heads-up on this.

hi Christian,

Quoted 57 lines · Christian Riesch

Hi Sughosh,

Quoted 39 lines · Sughosh Ganu

hi Christian,

Quoted 3 lines · Christian Riesch

Hi,

On Mon, Feb 27, 2012 at 11:16 AM, Sughosh Ganu <[email protected]> wrote:

<snip>

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

?arch/arm/cpu/arm926ejs/davinci/spl.c | ? ?2 ++
?1 files changed, 2 insertions(+), 0 deletions(-)

arch/arm/cpu/arm926ejs/davinci/spl.c +0 -0

index b1eff26..2861907 100644
@@ -32,10 +32,12 @@
 

?#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT

+#ifdef CONFIG_SPL_SPI_LOAD ?DECLARE_GLOBAL_DATA_PTR;
?/* Define global data structure pointer to it*/ ?static gd_t gdata __attribute__ ((section(".data"))); ?static bd_t bdata __attribute__ ((section(".data"))); +#endif

<snip>

Quoted 9 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

?Can you specify which boards you get this warning for. With your ?patch to add libcommon to hawkboard's spl image, this is now also ?needed for hawkboard which uses CONFIG_SPL_NAND_LOAD.

Simon's patch is for the hawkboard, since due to another patch in his patchset LIBCOMMON is enabled in hawkboard's SPL. Now we have a board that boots from NAND with SPL and has LIBCOMMON enabled (Simon, I did not check the rest of your patchset, why do you need LIBCOMMON on the hawkboard at all?)

Quoted 8 lines

?LIBCOMMON is now needed as the generic relocation based functions ?are part of the libcommon.o, which are being enabled in the same ?patchset for all arm boards. So if i understand correct, all arm ?board based spl's now need libcommon and libgeneric.

?The only thing i see is that libcommon and libgeneric are not ?defined for cam_enc_4xx board which uses spl, and this patchset does ?not add it either. Not sure whether it got missed.

When I asked Heiko Schocher a few month ago why he defined putc and puts in arch/arm/cpu/arm926ejs/davinci/spl.c he replied that he could not use LIBCOMMON due to size limitations for the SPL. So I guess that this board will not be able to use the generic relocation functions, unless the SPL is smaller than 16kB, right? Simon's patchset will break this board then, right?

?That is exactly what i reported in one of the threads in response to ?addition of libcommon and libgeneric to the hawkboard's spl. In ?fact, this might cause problems on quite a few boards with spl size ?restrictions. I am not sure, whether the generic relocation feature ?should be turned on by default on all boards or should be a config ?option -- at least for the spl builds. Another option would be to ?move it to a place where it is not needed to compile in the entire ?libcommon/libgeneric support that is not needed for the generic ?relocation code. I think that would help us keep the generic ?relocation without the size bloat that we see right now.

?http://lists.denx.de/pipermail/u-boot/2012-February/118567.html

<snip>

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

void board_init_r(gd_t *id, ulong dummy)
{
#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
? ? ? ? mem_malloc_init(CONFIG_SYS_TEXT_BASE - CONFIG_SYS_MALLOC_LEN, ? ? ? ? ? ? ? ? ? ? ? ? CONFIG_SYS_MALLOC_LEN);

?Can you please explain why we need the mem_malloc_init. I did not ?include this, and spl boots up just fine on my board.

Quoted 2 lines

malloc is required for the SPI code only, so you could also put it within #ifdef CONFIG_SPL_SPI_LOAD

?Ok, i will move the common changes to a static function, and call ?it from both the nand and spi load cases. Or, should i wait till a ?consensus is drawn on whether to enable this feature for spl images ?also. In case this is not needed for spl, then we don't need this ?change.

-sughosh

Hi,

Quoted 60 lines · Sughosh Ganu

hi Christian,

Quoted 57 lines · Christian Riesch

Hi Sughosh,

Quoted 39 lines · Sughosh Ganu

hi Christian,

Quoted 3 lines · Christian Riesch

Hi,

On Mon, Feb 27, 2012 at 11:16 AM, Sughosh Ganu <[email protected]> wrote:

<snip>

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

?arch/arm/cpu/arm926ejs/davinci/spl.c | ? ?2 ++
?1 files changed, 2 insertions(+), 0 deletions(-)

arch/arm/cpu/arm926ejs/davinci/spl.c +0 -0

index b1eff26..2861907 100644
@@ -32,10 +32,12 @@
 

?#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT

+#ifdef CONFIG_SPL_SPI_LOAD ?DECLARE_GLOBAL_DATA_PTR;
?/* Define global data structure pointer to it*/ ?static gd_t gdata __attribute__ ((section(".data"))); ?static bd_t bdata __attribute__ ((section(".data"))); +#endif

<snip>

Quoted 9 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

?Can you specify which boards you get this warning for. With your ?patch to add libcommon to hawkboard's spl image, this is now also ?needed for hawkboard which uses CONFIG_SPL_NAND_LOAD.

Simon's patch is for the hawkboard, since due to another patch in his patchset LIBCOMMON is enabled in hawkboard's SPL. Now we have a board that boots from NAND with SPL and has LIBCOMMON enabled (Simon, I did not check the rest of your patchset, why do you need LIBCOMMON on the hawkboard at all?)

Quoted 8 lines

?LIBCOMMON is now needed as the generic relocation based functions ?are part of the libcommon.o, which are being enabled in the same ?patchset for all arm boards. So if i understand correct, all arm ?board based spl's now need libcommon and libgeneric.

?The only thing i see is that libcommon and libgeneric are not ?defined for cam_enc_4xx board which uses spl, and this patchset does ?not add it either. Not sure whether it got missed.

When I asked Heiko Schocher a few month ago why he defined putc and puts in arch/arm/cpu/arm926ejs/davinci/spl.c he replied that he could not use LIBCOMMON due to size limitations for the SPL. So I guess that this board will not be able to use the generic relocation functions, unless the SPL is smaller than 16kB, right? Simon's patchset will break this board then, right?

If it pushes it over 16KB, although it's not clear that it will. So far I haven't seen this and I would hope that builds would fail in this case (normally I got a 'cannot move program counter backwards' error).

But clearly putting this code in spl.c is a bit ugly because it hard-codes the driver and duplicates code in common/console.c:

void putc(char c)
{
? ? ? ?if (c == '\n')
? ? ? ? ? ? ? ?NS16550_putc((NS16550_t)(CONFIG_SYS_NS16550_COM1), '\r');

? ? ? ?NS16550_putc((NS16550_t)(CONFIG_SYS_NS16550_COM1), c); }

Can someone please check what is the maximum SPL size on one of these boards? Generic relocation increases the spl from 5.9KB to 10KB. Is there an 8KB limit?

One option if this really is a problem is to move common/reloc.c to lib/reloc.c, although there isn't really a huge amount of justification for that given that it is U-Boot code. Perhaps we could claim that it is a relocation library and not U-Boot-specific?

Quoted 12 lines

?That is exactly what i reported in one of the threads in response to ?addition of libcommon and libgeneric to the hawkboard's spl. In ?fact, this might cause problems on quite a few boards with spl size ?restrictions. I am not sure, whether the generic relocation feature ?should be turned on by default on all boards or should be a config ?option -- at least for the spl builds. Another option would be to ?move it to a place where it is not needed to compile in the entire ?libcommon/libgeneric support that is not needed for the generic ?relocation code. I think that would help us keep the generic ?relocation without the size bloat that we see right now.

Quoted 1 line

?http://lists.denx.de/pipermail/u-boot/2012-February/118567.html

Well it would be nice to find out what the limit is. I might be able to squeeze it a bit more even in common. But failing that the putc() hack and lib/ might be the most expedient solution at this stage.

Quoted 13 lines

<snip>

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

void board_init_r(gd_t *id, ulong dummy)
{
#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
? ? ? ? mem_malloc_init(CONFIG_SYS_TEXT_BASE - CONFIG_SYS_MALLOC_LEN, ? ? ? ? ? ? ? ? ? ? ? ? CONFIG_SYS_MALLOC_LEN);

?Can you please explain why we need the mem_malloc_init. I did not ?include this, and spl boots up just fine on my board.

Quoted 8 lines

Quoted 2 lines

malloc is required for the SPI code only, so you could also put it within #ifdef CONFIG_SPL_SPI_LOAD

?Ok, i will move the common changes to a static function, and call ?it from both the nand and spi load cases. Or, should i wait till a ?consensus is drawn on whether to enable this feature for spl images ?also. In case this is not needed for spl, then we don't need this ?change.

My series removes the old relocation code, so it really does need to work for SPL also. I have put a bit of effort in here to tidy this up and I don't think it is hard to resolve this last problem.

I will investigate this a bit more and then suggest a solution.

Regards,
Simon

Quoted 2 lines

-sughosh

Add use of common/ and lib/ libraries for the SPL stage, and add the new link symbol required for generic relocation.

Signed-off-by Simon Glass <[email protected]>

Add use of common/ and lib/ libraries for the SPL stage, and add the new link symbol required for generic relocation.

Signed-off-by Simon Glass <[email protected]>

---
Changes in v4:
- Split out hawkboard changes into new patch

 board/davinci/da8xxevm/u-boot-spl-hawk.lds |    1 +
 include/configs/hawkboard.h                |    2 ++
 2 files changed, 3 insertions(+), 0 deletions(-)

hi Simon,

Quoted 11 lines · Simon Glass

Add use of common/ and lib/ libraries for the SPL stage, and add the new link symbol required for generic relocation.

Signed-off-by Simon Glass <[email protected]>

---
Changes in v4:
- Split out hawkboard changes into new patch

 board/davinci/da8xxevm/u-boot-spl-hawk.lds |    1 +
 include/configs/hawkboard.h                |    2 ++
 2 files changed, 3 insertions(+), 0 deletions(-)

Btw, i had quite a substantial increase in the spl image size with these additions with a 4.5 based toolchain(will provide exact toolchain details and size difference tonight). Building with a 4.6 based toolchain got the size increase down by quite a bit. Although i don't have any size restrictions with spl on my board, this could be a problem on boards which have these restrictions with toolchains before 4.6.

Quoted 15 lines · Sughosh Ganu

hi Simon,

Quoted 11 lines · Simon Glass

Add use of common/ and lib/ libraries for the SPL stage, and add the new link symbol required for generic relocation.

Signed-off-by Simon Glass <[email protected]>

---
Changes in v4:
- Split out hawkboard changes into new patch

?board/davinci/da8xxevm/u-boot-spl-hawk.lds | ? ?1 + ?include/configs/hawkboard.h ? ? ? ? ? ? ? ?| ? ?2 ++ ?2 files changed, 3 insertions(+), 0 deletions(-)

<snip>

Quoted 8 lines

?Btw, i had quite a substantial increase in the spl image size with ?these additions with a 4.5 based toolchain(will provide exact ?toolchain details and size difference tonight). Building with a 4.6 ?based toolchain got the size increase down by quite a bit. Although i ?don't have any size restrictions with spl on my board, this could be ?a problem on boards which have these restrictions with toolchains ?before 4.6.

gcc(arm-none-linux-gnueabi) -- 4.5.2
binutils -- 2.20

spl size without this patch
?? text??? ?? data??? ??? bss??? ??? dec??? ??? hex??? filename ?? 5208??? ??? 620??? ??? 464??? ?? 6292??? ?? 1894??? spl/u-boot-spl

spl size with this patch
?? text??? ?? data??? ??? bss??? ??? dec??? ??? hex??? filename ?? 8897??? ??? 620??? ??? 476??? ?? 9993??? ?? 2709??? spl/u-boot-spl

gcc(arm-none-eabi) -- 4.6.2
binutils -- 2.21

spl size without this patch
?? text??? ?? data??? ??? bss??? ??? dec??? ??? hex??? filename ?? 4692 ?? ??? 620??? ??? 464???? 5776 ?? ? 1690 ?? spl/u-boot-spl

spl size with this patch
?? text??? ?? data??? ??? bss??? ??? dec??? ??? hex??? filename ?? 4812 ?? ??? 620??? ? 476 ????? 5908 ?? ? 1714 ?? spl/u-boot-spl

-sughosh

Hi Simon,

One general question: am I right in saying that in the current implementation, relocate_code executes mostly on the final stack, whereas in your proposal, it runs on the initial stack?

Amicalement,
--
Albert.

Hi Albert,

Quoted 5 lines · Albert ARIBAUD

Hi Simon,

One general question: am I right in saying that in the current implementation, relocate_code executes mostly on the final stack, whereas in your proposal, it runs on the initial stack?

Well the current relocation code is all in assembler and I don't believe it uses the stack at all. It does set up the stack pointer at the beginning if that's what you mean.

Yes this series uses the initial stack for all processing until it reaches board_init_r().

Hi Simon, Albert.

Quoted 10 lines · Simon Glass

Hi Albert,

Quoted 5 lines · Albert ARIBAUD

Hi Simon,

One general question: am I right in saying that in the current implementation, relocate_code executes mostly on the final stack, whereas in your proposal, it runs on the initial stack?

Quoted 6 lines

Well the current relocation code is all in assembler and I don't believe it uses the stack at all. It does set up the stack pointer at the beginning if that's what you mean.

Yes this series uses the initial stack for all processing until it reaches board_init_r().

<broken record>

See what x86 does - Code copy and relocation in C using final stack and full cache enabled...

</broken record> :)

Regards,

Graeme

Hi Simon,

Le 21/02/2012 21:16, Simon Glass a ?crit :

Quoted 38 lines

Hi Graeme,

Quoted 32 lines · On Tue, Feb 21, 2012 at 12:10 PM, Graeme Russ<[email protected]> ?wrote:

Hi Simon, Albert.

Quoted 14 lines · Simon Glass

Hi Albert,

Quoted 7 lines · On Tue, Feb 21, 2012 at 11:40 AM, Albert ARIBAUD <[email protected]>…

Hi Simon,

One general question: am I right in saying that in the current implementation, relocate_code executes mostly on the final stack, whereas in
your proposal, it runs on the initial stack?

Quoted 6 lines

Well the current relocation code is all in assembler and I don't believe it uses the stack at all. It does set up the stack pointer at the beginning if that's what you mean.

Yes this series uses the initial stack for all processing until it reaches board_init_r().

<broken record>

See what x86 does - Code copy and relocation in C using final stack and full cache enabled...

</broken record> ?:)

Quoted 3 lines

Heading there, but we can't enable dcache prior to relocation on ARM yet. Generic relocation gives me something to work with that isn't just for one architecture,

I'm not sure what is the relation between stack and caches exactly -- nor why we could not have caches enabled before relocation, for that matter.

Amicalement,
--
Albert.