Raw Kernel Image Support for Falcon Mode Boot Via SPI Devices

Hi All,

While trying to bring up Falcon Mode boot on an ARM64 board, I've
discovered that the SPL+SPI(spl_spi.c) driver does not allow us to load a
raw kernel image and subsequently call the bootz_setup() function which
resides in spl_parse_image_header().

I've added a new config option (CONFIG_SYS_SPI_KERNEL_SKIP_HEADER) which
will skip the mkimage header check, allowing the subsequent
spl_parse_image_header() call to successfully fall through to bootz_setup()
and load/boot a raw kernel image.

Sincerely,
Nathan Barrett-Morrison

On Mon, Jan 17, 2022 at 08:02:58PM -0500, Nathan Barrett-Morrison wrote:

> Hi All,
> 
> While trying to bring up Falcon Mode boot on an ARM64 board, I've
> discovered that the SPL+SPI(spl_spi.c) driver does not allow us to load a
> raw kernel image and subsequently call the bootz_setup() function which
> resides in spl_parse_image_header().
> 
> I've added a new config option (CONFIG_SYS_SPI_KERNEL_SKIP_HEADER) which
> will skip the mkimage header check, allowing the subsequent
> spl_parse_image_header() call to successfully fall through to bootz_setup()
> and load/boot a raw kernel image.
> 
> Sincerely,
> Nathan Barrett-Morrison

> From e5a15a8ad2fd007e6d8d48dd64767d194bbd1833 Mon Sep 17 00:00:00 2001
> From: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> Date: Mon, 17 Jan 2022 19:42:59 -0500
> Subject: [PATCH] Allow Falcon Mode boot to use raw kernel image when
>  booting via SPI.  When using Falcon Mode boot with a raw, unwrapped kernel
>  image, the bootz_setup() call inside of spl_parse_image_header() is
>  unreachable because the mkimage header magic check will never pass.  Adding
>  CONFIG_SYS_SPI_KERNEL_SKIP_HEADER gives us the ability to pass through to the
>  desired bootz_setup() call.
> 
> Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Aneesh V <aneesh@ti.com>
> ---
>  common/spl/spl_spi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
> index 4e20a23dea..62dad1d2fb 100644
> --- a/common/spl/spl_spi.c
> +++ b/common/spl/spl_spi.c
> @@ -33,8 +33,10 @@ static int spi_load_image_os(struct spl_image_info *spl_image,
>  	spi_flash_read(flash, CONFIG_SYS_SPI_KERNEL_OFFS, sizeof(*header),
>  		       (void *)header);
>  
> +#ifndef CONFIG_SYS_SPI_KERNEL_SKIP_HEADER
>  	if (image_get_magic(header) != IH_MAGIC)
>  		return -1;
> +#endif
>  
>  	err = spl_parse_image_header(spl_image, header);
>  	if (err)

I'm not sure this is the right path.  Why is this part of the code
checking for IH_MAGIC at all, and should it not instead allow for
graceful falling back so that zImage/Image/etc work?

Hi Tom,

Yes -- I believe this section of code should not be checking for IH_MAGIC
at all, as parse_image_header does it as well.

Would you like me to resubmit this patch with the offending lines deleted?

Sincerely,
Nathan

On Mon, Jan 31, 2022 at 10:14 AM Tom Rini <trini@konsulko.com> wrote:

> On Mon, Jan 17, 2022 at 08:02:58PM -0500, Nathan Barrett-Morrison wrote:
>
> > Hi All,
> >
> > While trying to bring up Falcon Mode boot on an ARM64 board, I've
> > discovered that the SPL+SPI(spl_spi.c) driver does not allow us to load a
> > raw kernel image and subsequently call the bootz_setup() function which
> > resides in spl_parse_image_header().
> >
> > I've added a new config option (CONFIG_SYS_SPI_KERNEL_SKIP_HEADER) which
> > will skip the mkimage header check, allowing the subsequent
> > spl_parse_image_header() call to successfully fall through to
> bootz_setup()
> > and load/boot a raw kernel image.
> >
> > Sincerely,
> > Nathan Barrett-Morrison
>
> > From e5a15a8ad2fd007e6d8d48dd64767d194bbd1833 Mon Sep 17 00:00:00 2001
> > From: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> > Date: Mon, 17 Jan 2022 19:42:59 -0500
> > Subject: [PATCH] Allow Falcon Mode boot to use raw kernel image when
> >  booting via SPI.  When using Falcon Mode boot with a raw, unwrapped
> kernel
> >  image, the bootz_setup() call inside of spl_parse_image_header() is
> >  unreachable because the mkimage header magic check will never pass.
> Adding
> >  CONFIG_SYS_SPI_KERNEL_SKIP_HEADER gives us the ability to pass through
> to the
> >  desired bootz_setup() call.
> >
> > Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> > Cc: Tom Rini <trini@konsulko.com>
> > Cc: Aneesh V <aneesh@ti.com>
> > ---
> >  common/spl/spl_spi.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
> > index 4e20a23dea..62dad1d2fb 100644
> > --- a/common/spl/spl_spi.c
> > +++ b/common/spl/spl_spi.c
> > @@ -33,8 +33,10 @@ static int spi_load_image_os(struct spl_image_info
> *spl_image,
> >       spi_flash_read(flash, CONFIG_SYS_SPI_KERNEL_OFFS, sizeof(*header),
> >                      (void *)header);
> >
> > +#ifndef CONFIG_SYS_SPI_KERNEL_SKIP_HEADER
> >       if (image_get_magic(header) != IH_MAGIC)
> >               return -1;
> > +#endif
> >
> >       err = spl_parse_image_header(spl_image, header);
> >       if (err)
>
> I'm not sure this is the right path.  Why is this part of the code
> checking for IH_MAGIC at all, and should it not instead allow for
> graceful falling back so that zImage/Image/etc work?
>
> --
> Tom
>

On Mon, Jan 31, 2022 at 10:16:08AM -0500, Nathan Barrett-Morrison wrote:

> Hi Tom,
> 
> Yes -- I believe this section of code should not be checking for IH_MAGIC
> at all, as parse_image_header does it as well.
> 
> Would you like me to resubmit this patch with the offending lines deleted?

Well, please take a good look over the whole area of code and see if it
really makes sense to do what it's doing there or not.

Hi Tom,

I've attached v2 of this patch.  I've also confirmed again that the
offending lines are entirely unnecessary and amended the patch to delete
them instead.

Sincerely,
Nathan

On Mon, Jan 31, 2022 at 10:23 AM Tom Rini <trini@konsulko.com> wrote:

> On Mon, Jan 31, 2022 at 10:16:08AM -0500, Nathan Barrett-Morrison wrote:
>
> > Hi Tom,
> >
> > Yes -- I believe this section of code should not be checking for IH_MAGIC
> > at all, as parse_image_header does it as well.
> >
> > Would you like me to resubmit this patch with the offending lines
> deleted?
>
> Well, please take a good look over the whole area of code and see if it
> really makes sense to do what it's doing there or not.
>
> --
> Tom
>