[U-Boot] [PATCH v6 4/6] arm: spl: Allow board_init_r() to run with a larger stack

[Simon Glass]

Hi Tom,

On 3 March 2015 at 10:49, Tom Rini <trini at konsulko.com> wrote:
> On Tue, Mar 03, 2015 at 08:03:00AM -0700, Simon Glass wrote:
>
>> At present SPL uses a single stack, either CONFIG_SPL_STACK or
>> CONFIG_SYS_INIT_SP_ADDR. Since some SPL features (such as MMC and
>> environment) require a lot of stack, some boards set CONFIG_SPL_STACK to
>> point into SDRAM. They then set up SDRAM very early, before board_init_f(),
>> so that the larger stack can be used.
>>
>> This is an abuse of lowlevel_init(). That function should only be used for
>> essential start-up code which cannot be delayed. An example of a valid use is
>> when only part of the SPL code is visible/executable, and the SoC must be set
>> up so that board_init_f() can be reached. It should not be used for SDRAM
>> init, console init, etc.
>>
>> Add a CONFIG_SPL_STACK_R option, which allows the stack to be moved to a new
>> address before board_init_r() is called in SPL.
>>
>> The expected SPL flow (for CONFIG_SPL_FRAMEWORK) is documented in the README.
>>
>> Signed-off-by: Simon Glass <sjg at chromium.org>
>> For version 1:
>> Acked-by: Albert ARIBAUD <albert.u.boot at aribaud.net>
>> Reviewed-by: Stefan Roese <sr at denx.de>
>> Tested-by: Bo Shen <voice.shen at atmel.com>
>> Acked-by: Bo Shen <voice.shen at atmel.com>
>> Acked-by: Heiko Schocher <hs at denx.de>
>> Tested-by: Heiko Schocher <hs at denx.de>
>> ---
>>
>> Changes in v6: None
>> Changes in v5:
>> - Rebase to master
>>
>> Changes in v4:
>> - Adjust README to mention that lowlevel_init() should have no stack
>>
>> Changes in v3: None
>> Changes in v2:
>> - Move docs to top-level README file and expand them to cover U-Boot proper
>> - Add Kconfig settings
>>
>>  Kconfig             | 18 ++++++++++++++
>>  README              | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  arch/arm/lib/crt0.S | 13 +++++++---
>>  common/spl/spl.c    | 35 +++++++++++++++++++++++++++
>>  4 files changed, 132 insertions(+), 3 deletions(-)
> [snip]
>> diff --git a/arch/arm/lib/crt0.S b/arch/arm/lib/crt0.S
>> index 22df3e5..7939ced 100644
>> --- a/arch/arm/lib/crt0.S
>> +++ b/arch/arm/lib/crt0.S
>> @@ -113,7 +113,14 @@ here:
>>  /* Set up final (full) environment */
>>
>>       bl      c_runtime_cpu_setup     /* we still call old routine here */
>> -
>> +#endif
>> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_FRAMEWORK)
>> +# ifdef CONFIG_SPL_BUILD
>> +     /* Use a DRAM stack for the rest of SPL, if requested */
>> +     bl      spl_relocate_stack_gd
>> +     cmp     r0, #0
>> +     movne   sp, r0
>> +# endif
>>       ldr     r0, =__bss_start        /* this is auto-relocated! */
>>       ldr     r1, =__bss_end          /* this is auto-relocated! */
>>
>> @@ -124,9 +131,10 @@ clbss_l:cmp      r0, r1                  /* while not at end of BSS */
>>       addlo   r0, r0, #4              /* move to next */
>>       blo     clbss_l
>>
>> +#if ! defined(CONFIG_SPL_BUILD)
>>       bl coloured_LED_init
>>       bl red_led_on
>> -
>> +#endif
>>       /* call board_init_r(gd_t *id, ulong dest_addr) */
>>       mov     r0, r9                  /* gd_t */
>>       ldr     r1, [r9, #GD_RELOCADDR] /* dest_addr */
>> @@ -134,7 +142,6 @@ clbss_l:cmp       r0, r1                  /* while not at end of BSS */
>>       ldr     pc, =board_init_r       /* this is auto-relocated! */
>>
>>       /* we should not return here. */
>> -
>>  #endif
>>
>>  ENDPROC(_main)
>> diff --git a/common/spl/spl.c b/common/spl/spl.c
>> index ded0f30..cd75bbc 100644
>> --- a/common/spl/spl.c
>> +++ b/common/spl/spl.c
>> @@ -281,3 +281,38 @@ void preloader_console_init(void)
>>       spl_display_print();
>>  #endif
>>  }
>> +
>> +/**
>> + * spl_relocate_stack_gd() - Relocate stack ready for board_init_r() execution
>> + *
>> + * Sometimes board_init_f() runs with a stack in SRAM but we want to use SDRAM
>> + * for the main board_init_r() execution. This is typically because we need
>> + * more stack space for things like the MMC sub-system.
>> + *
>> + * This function calculates the stack position, copies the global_data into
>> + * place and returns the new stack position. The caller is responsible for
>> + * setting up the sp register.
>> + *
>> + * @return new stack location, or 0 to use the same stack
>> + */
>> +ulong spl_relocate_stack_gd(void)
>> +{
>> +#ifdef CONFIG_SPL_STACK_R
>> +     gd_t *new_gd;
>> +     ulong ptr;
>> +
>> +     /* Get stack position: use 8-byte alignment for ABI compliance */
>> +     ptr = CONFIG_SPL_STACK_R - sizeof(gd_t);
>> +     ptr &= ~7;
>> +     new_gd = (gd_t *)ptr;
>> +     memcpy(new_gd, (void *)gd, sizeof(gd_t));
>> +     gd = new_gd;
>> +
>> +     /* Clear the BSS. */
>> +     memset(__bss_start, 0, __bss_end - __bss_start);
>> +
>> +     return ptr;
>> +#else
>> +     return 0;
>> +#endif
>> +}
>
> All of this _does_ move gd into where CONFIG_SPL_STACK_R points.  It
> does _not_ move the stack itself into where CONFIG_SPL_STACK_R points so
> the big use case (am335x_boneblack for example where
> CONFIG_SPL_ENV_SUPPORT is set) doesn't work and blows up as we overflow
> the area for stack.

OK I'll have to test more. What sort of problem should i see?

The return value from spl_relocate_stack_gd() should be shoved into
sp. Does that not happen?

Regards,
Simon