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

[Nikita Kiryanov]

Hi Simon,
On Thu, Oct 29, 2015 at 11:19:38AM -0600, Simon Glass wrote:
> 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.

OK

> 
> >
> >  /* 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()?

The next patch takes care of that.

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