[U-Boot] [PATCH V2 08/13] spl: change return values of spl_*_load_image()

[Simon Glass]

Hi Nikita,

On 28 October 2015 at 03:23, Nikita Kiryanov <nikita at compulab.co.il> wrote:
> Make spl_*_load_image() functions return a value instead of
> hanging if a problem is encountered. This enables main spl code
> to make the decision whether to hang or not, thus preparing
> it to support alternative boot devices.
>
> Some boot devices (namely nand and spi) do not hang on error.
> Instead, they return normally and SPL proceeds to boot the
> contents of the load address. This is considered a bug and
> is rectified by hanging on error for these devices as well.
>
> Signed-off-by: Nikita Kiryanov <nikita at compulab.co.il>
> Cc: Igor Grinberg <grinberg at compulab.co.il>
> Cc: Tom Rini <trini at konsulko.com>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Ian Campbell <ijc at hellion.org.uk>
> Cc: Hans De Goede <hdegoede at redhat.com>
> Cc: Albert Aribaud <albert.u.boot at aribaud.net>
> Cc: Jagan Teki <jteki at openedev.com>
> ---
> Changes in V2:
>         - Minor collateral adjustments from changes in patch 2 (only one return
>           statement at the end of spl_mmc_load_image).
>
>  arch/arm/cpu/armv7/sunxi/board.c |  4 +++-
>  arch/arm/include/asm/spl.h       |  2 +-
>  common/spl/spl.c                 | 43 +++++++++++++++++++++++++++-------------
>  common/spl/spl_mmc.c             | 26 ++++++++++++++----------
>  common/spl/spl_nand.c            | 18 +++++++++++------
>  common/spl/spl_net.c             |  9 ++++++---
>  common/spl/spl_nor.c             |  6 ++++--
>  common/spl/spl_onenand.c         |  4 +++-
>  common/spl/spl_sata.c            | 11 +++++++---
>  common/spl/spl_usb.c             | 17 ++++++++++------
>  common/spl/spl_ymodem.c          |  5 +++--
>  drivers/mtd/spi/spi_spl_load.c   | 17 +++++++++++-----
>  include/spl.h                    | 18 ++++++++---------
>  13 files changed, 116 insertions(+), 64 deletions(-)
>
> diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c
> index 4785ac6..9b5c46b 100644
> --- a/arch/arm/cpu/armv7/sunxi/board.c
> +++ b/arch/arm/cpu/armv7/sunxi/board.c
> @@ -95,10 +95,12 @@ static int gpio_init(void)
>         return 0;
>  }
>
> -void spl_board_load_image(void)
> +int spl_board_load_image(void)
>  {
>         debug("Returning to FEL sp=%x, lr=%x\n", fel_stash.sp, fel_stash.lr);
>         return_to_fel(fel_stash.sp, fel_stash.lr);
> +
> +       return 0;
>  }
>
>  void s_init(void)
> diff --git a/arch/arm/include/asm/spl.h b/arch/arm/include/asm/spl.h
> index 6db405d..f8092c7 100644
> --- a/arch/arm/include/asm/spl.h
> +++ b/arch/arm/include/asm/spl.h
> @@ -32,7 +32,7 @@ enum {
>  #endif
>
>  /* Board-specific load method */
> -void spl_board_load_image(void);
> +int spl_board_load_image(void);

Please add a comment about what this does, and the return value.

>
>  /* Linker symbols. */
>  extern char __bss_start[], __bss_end[];
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index 4b319d6..ff1bad2 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -132,7 +132,7 @@ __weak void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
>  }
>
>  #ifdef CONFIG_SPL_RAM_DEVICE
> -static void spl_ram_load_image(void)
> +static int spl_ram_load_image(void)
>  {
>         const struct image_header *header;
>
> @@ -145,6 +145,8 @@ static void spl_ram_load_image(void)
>                 (CONFIG_SYS_TEXT_BASE - sizeof(struct image_header));
>
>         spl_parse_image_header(header);
> +
> +       return 0;
>  }
>  #endif
>
> @@ -208,68 +210,81 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
>         switch (boot_device) {
>  #ifdef CONFIG_SPL_RAM_DEVICE
>         case BOOT_DEVICE_RAM:
> -               spl_ram_load_image();
> +               if (spl_ram_load_image())
> +                       hang();
>                 break;
>  #endif
>  #ifdef CONFIG_SPL_MMC_SUPPORT
>         case BOOT_DEVICE_MMC1:
>         case BOOT_DEVICE_MMC2:
>         case BOOT_DEVICE_MMC2_2:
> -               spl_mmc_load_image();
> +               if (spl_mmc_load_image())
> +                       hang();

Can you use something like:

ret = spl_mmc_load_image()

and put the hang() after the switch()?

>                 break;
>  #endif
>  #ifdef CONFIG_SPL_NAND_SUPPORT
>         case BOOT_DEVICE_NAND:
> -               spl_nand_load_image();
> +               if (spl_nand_load_image())
> +                       hang();
>                 break;
>  #endif
>  #ifdef CONFIG_SPL_ONENAND_SUPPORT
>         case BOOT_DEVICE_ONENAND:
> -               spl_onenand_load_image();
> +               if (spl_onenand_load_image())
> +                       hang();
>                 break;
>  #endif
>  #ifdef CONFIG_SPL_NOR_SUPPORT
>         case BOOT_DEVICE_NOR:
> -               spl_nor_load_image();
> +               if (spl_nor_load_image())
> +                       hang();
>                 break;
>  #endif
>  #ifdef CONFIG_SPL_YMODEM_SUPPORT
>         case BOOT_DEVICE_UART:
> -               spl_ymodem_load_image();
> +               if (spl_ymodem_load_image())
> +                       hang();
>                 break;
>  #endif
>  #ifdef CONFIG_SPL_SPI_SUPPORT
>         case BOOT_DEVICE_SPI:
> -               spl_spi_load_image();
> +               if (spl_spi_load_image())
> +                       hang();
>                 break;
>  #endif
>  #ifdef CONFIG_SPL_ETH_SUPPORT
>         case BOOT_DEVICE_CPGMAC:
>  #ifdef CONFIG_SPL_ETH_DEVICE
> -               spl_net_load_image(CONFIG_SPL_ETH_DEVICE);
> +               if (spl_net_load_image(CONFIG_SPL_ETH_DEVICE))
> +                       hang();
>  #else
> -               spl_net_load_image(NULL);
> +               if (spl_net_load_image(NULL))
> +                       hang();
>  #endif
>                 break;
>  #endif

[snip]

Regards,
Simon