Hi Simon,
On Sun, 18 Jan 2015 11:55:36 -0700 Simon Glass <s...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot