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

[Masahiro Yamada]

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.






> @@ -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.



>  }
> +
> +/**
> + * 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.)





> 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..



> +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.






Best Regards
Masahiro Yamada