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

[Simon Glass]

Hi Masahiro,

On 21 January 2015 at 03:51, Masahiro Yamada <yamada.m at jp.panasonic.com> wrote:
> Hi Simon,
>
>
>
> On Sun, 18 Jan 2015 11:55:36 -0700
> Simon Glass <sjg at chromium.org> 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 now:
>>
>> Execution starts with start.S. Two main functions can be provided by the
>> board implementation. The purpose and limitations of each is described below.
>> After that, the common board_init_r() is called to perform the SPL task.
>>
>> lowlevel_init():
>>       - purpose: essential init to permit execution to reach board_init_f()
>>       - no global_data, but there is a stack
>>       - must not set up SDRAM or use console
>>       - must only do the bare minimum to allow execution to continue to
>>               board_init_f()
>>       - this is almost never needed
>>       - return normally from this function
>>
>> board_init_f():
>>       - purpose: set up the machine ready for running board_init_r():
>>               i.e. SDRAM and serial UART
>>       - global_data is available
>>       - preloader_console_init() can be called here in extremis
>>       - stack is in SRAM
>>       - should set up SDRAM, and anything needed to make the UART work
>>       - these is no need to clear BSS, it will be done by crt0.S
>>       - must return normally from this function (don't call board_init_r()
>>               directly)
>>
>> Here the BSS is cleared. If CONFIG_SPL_STACK_R is defined, then at this point
>> the stack and global_data are relocated to below that address.
>>
>> board_init_r():
>>       - purpose: main execution, common code
>>       - global_data is available
>>       - SDRAM is available
>>       - stack is optionally in SDRAM, if CONFIG_SPL_STACK_R is defined and
>>               points into SDRAM
>>       - preloader_console_init() can be called here - typically this is
>>               done by defining CONFIG_SPL_BOARD_INIT and then supplying a
>>               spl_board_init() function containing this call
>>       - loads U-Boot or (in falcon mode) Linux
>>
>> Note: This patch is intended to apply over the top of Tom's SPL changes and
>> this series:
>>
>> https://patchwork.ozlabs.org/patch/423785/
>
>
>
> I still have an opinion that global_data itself
> is a nightmare rather than a useful stuff.
>
>

I believe its function is to provide a place to put variables before
BSS is available. After that I suppose, since the values are sitting
there, it makes sense to keep using it. But it can be abused.

>
>
>
>
>> @@ -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
>
> It is not clear to me why this change is related to a larger stack.

We don't want to call this code from SPL.

>
>
>
>>  }
>> +
>> +/**
>> + * 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
>
> I guess CONFIG_SPL_STACK_R has the type, "hex", not "bool".
>
> In terms of Kconfig's way, the "ifdef CONFIG_SPL_STACK_R"
> is the abuse of the #ifdef conditonal.
>
> When Kconfig was introduced, I might have mentioned
> "Add a new CONFIG to Kconfig rather than headers when you introduce a new feature
> and document the usage in Kconfig".
>
> Most of people do not stick to that, so I think you can excuse here.
>
> I assume you (or somebody else) will implement it correctly when it is moved to Kconfig.
> (I should be easy.)

Yes agreed. I can do that. It seems like the concept is agreed at least.

So how about this:

CONFIG_SPL_STACK_R - bool
CONFIG_SPL_STACK_R _SIZE - hex

>
>
>
>
>
>> diff --git a/doc/README.SPL b/doc/README.SPL
>> index 3ba313c..327d3e2 100644
>> --- a/doc/README.SPL
>> +++ b/doc/README.SPL
>> @@ -95,3 +95,45 @@ cflow will spit out a number of warnings as it does not parse
>>  the config files and picks functions based on #ifdef.  Parsing the '.i'
>>  files instead introduces another set of headaches.  These warnings are
>>  not usually important to understanding the flow, however.
>> +
>> +
>> +ARM SPL Control Flow
>> +--------------------
>
>
> Is this flow ARM-specific?
> It looks like the following description is very generic
> althogh I do not familiar with the other architectures..
>

Yes it is at present. My change only affects ARM (32-bit) as that's
all I can test with.

That said, Albert has asked me to generalise this to cover U-Boot as
well as SPL, so I'll do that. I can change the heading here to be
generic, and then mention that it currently applies only for ARM.
>
>
>> +Execution starts with start.S. Two main functions can be provided by the
>> +board implementation. The purpose and limitations of each is described below.
>> +After that, the common board_init_r() is called to perform the SPL task.
>> +
>> +lowlevel_init():
>> +     - purpose: essential init to permit execution to reach board_init_f()
>> +     - no global_data, but there is a stack
>> +     - must not set up SDRAM or use console
>> +     - must only do the bare minimum to allow execution to continue to
>> +             board_init_f()
>> +     - this is almost never needed
>> +     - return normally from this function
>> +
>> +board_init_f():
>> +     - purpose: set up the machine ready for running board_init_r():
>> +             i.e. SDRAM and serial UART
>> +     - global_data is available
>> +     - preloader_console_init() can be called here in extremis
>> +     - stack is in SRAM
>> +     - should set up SDRAM, and anything needed to make the UART work
>
> I do not mean I object against this patch.
> From here, this is beyond the scope of this patch.
> Just comments about the current SPL boot flow that, I think, is not very nice.
>
> One of the most important tasks of a boot loader is DRAM initialization.
> This is sometimes problematic, so the printf debug (or any other early UART feature)
> would be really helpful.
> If DRAM init fails, I want some error messages on the console rather than the silent die.
>
> In the main U-Boot boot flow (common/board_f.c), the initcalls are invoked in this order:
>   initf_dm, serial_init, console_init_f, dram_init
>
> It is nice the UART is available in dram_init().
>
>
> On the other hand, what is happening in SPL is like this:
>  [1] You are introducing CONFIG_SPL_DM
>  [2] I assume the legacy drivers will be all dropped including UART of SPL
>  [3] The DM scan (dm_init_and_scan) is called in board_init_r()
>  [4] As you mentioned above in the README, DRAM should be setup in board_init_f()
>
> Both [3] and [4] together make it difficult the UART debug of dram_init().
>
> I guess [3] is just a temporary workaround in order to introduce DM into SPL and
> we will have to re-design the SPL boot flow someday.
>
> Perhaps what will happen next might be to reuse common/board_f.c for SPL.
> (i.e.  Generic Board for SPL,    CONFIG_SPL_GENERIC_BOARD?)
>
> Roughly, what we want to do in SPL is all included in common/board_f.c
>
> Moreover, if SPL is enabled, we can skip common/board_f.c in the main U-boot image.
> Most of the initializations have already been done in SPL.
> We do not have to do board_init_f() twice.

This idea of duplicate init is not well developed. I think we should
establish some principles here also - e.g. doing the same thing in SPL
and U-Boot proper seems wrong to me.

But for your particular case, I certainly would like the UART to be
available early in SPL. We are in the very early days of driver model
for SPL. Once we get it merged (thanks to Tom's work this should be
soon) we can look at how to get the UART available early. One hacky
idea is to do the DM scan in board_init_f() in SPL, for serial only.
But it can be done.

Yes generic board for SPL is where I'd like to head (but hopefully
using driver model for all init). This patch is a step along the way.

Regards,
Simon