spl: spi: Raw Kernel Image Support for Falcon Mode Boot Via SPI Devices

spl: spi: Raw Kernel Image Support for Falcon Mode Boot Via SPI Devices

[Nathan Barrett-Morrison]

Hi Tom,

I believe this patch is still relevant, so I'm resubmitting it.  It was previously marked as superseded.

Thanks,
Nathan

From 0bb98a42bcb01c078f63513d9151d307dbfd6ccd Mon Sep 17 00:00:00 2001
From: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
Date: Tue, 19 Apr 2022 17:35:21 -0400
Subject: [PATCH v2] 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 in spi_load_image_os() will never pass.  This check is entirely redundant and unnecessary,
as the spl_parse_image_header() call will also check for IH_MAGIC.

Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
Cc: Tom Rini <trini@konsulko.com>
---
Changes for v2:
   - Remove proposed CONFIG_SYS_SPI_KERNEL_SKIP_HEADER option, as we've determined the entire check is redundant and unnecessary.  Just delete it instead.

 common/spl/spl_spi.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
index cf3f7ef4c0..22e9c87eae 100644
--- a/common/spl/spl_spi.c
+++ b/common/spl/spl_spi.c
@@ -34,9 +34,6 @@ 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);
 
-	if (image_get_magic(header) != IH_MAGIC)
-		return -1;
-
 	err = spl_parse_image_header(spl_image, bootdev, header);
 	if (err)
 		return err;
-- 
2.30.2

Re: spl: spi: Raw Kernel Image Support for Falcon Mode Boot Via SPI Devices

[Sean Anderson]

Hi Nathan,

On 4/19/22 5:38 PM, Nathan Barrett-Morrison wrote:
> [You don't often get email from nathan.morrison@timesys.com. Learn why this is important at http://aka.ms/LearnAboutSenderIdentification.]
> 
> Hi Tom,
> 
> I believe this patch is still relevant, so I'm resubmitting it.  It was previously marked as superseded.
> 
> Thanks,
> Nathan
> 
> From 0bb98a42bcb01c078f63513d9151d307dbfd6ccd Mon Sep 17 00:00:00 2001
> From: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> Date: Tue, 19 Apr 2022 17:35:21 -0400
> Subject: [PATCH v2] 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 in spi_load_image_os() will never pass.  This check is entirely redundant and unnecessary,
> as the spl_parse_image_header() call will also check for IH_MAGIC.
> 
> Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> Cc: Tom Rini <trini@konsulko.com>
> ---
> Changes for v2:
>    - Remove proposed CONFIG_SYS_SPI_KERNEL_SKIP_HEADER option, as we've determined the entire check is redundant and unnecessary.  Just delete it instead.
> 
>  common/spl/spl_spi.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
> index cf3f7ef4c0..22e9c87eae 100644
> --- a/common/spl/spl_spi.c
> +++ b/common/spl/spl_spi.c
> @@ -34,9 +34,6 @@ 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);
> 
> -       if (image_get_magic(header) != IH_MAGIC)
> -               return -1;
> -
>         err = spl_parse_image_header(spl_image, bootdev, header);
>         if (err)
>                 return err;
> --
> 2.30.2
> 

Can you see if [1] fixes your problem? You will also need the first patch in the series.

--Sean

[1] https://lore.kernel.org/u-boot/20220401190405.1932697-8-sean.anderson@seco.com/

Re: spl: spi: Raw Kernel Image Support for Falcon Mode Boot Via SPI Devices

[Nathan Barrett-Morrison]

Hi Sean,

Thanks for the response.  I don't have my embedded board I'm doing this on quite up to this version of U-Boot, but it does appear your patchset should resolve my issue in the future.  So please disregard my patch, I think we're good!

Sincerely,
Nathan

On 4/19/22 17:43, Sean Anderson wrote:
> Hi Nathan,
> 
> On 4/19/22 5:38 PM, Nathan Barrett-Morrison wrote:
>> [You don't often get email from nathan.morrison@timesys.com. Learn why this is important at http://aka.ms/LearnAboutSenderIdentification.]
>>
>> Hi Tom,
>>
>> I believe this patch is still relevant, so I'm resubmitting it.  It was previously marked as superseded.
>>
>> Thanks,
>> Nathan
>>
>> From 0bb98a42bcb01c078f63513d9151d307dbfd6ccd Mon Sep 17 00:00:00 2001
>> From: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
>> Date: Tue, 19 Apr 2022 17:35:21 -0400
>> Subject: [PATCH v2] 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 in spi_load_image_os() will never pass.  This check is entirely redundant and unnecessary,
>> as the spl_parse_image_header() call will also check for IH_MAGIC.
>>
>> Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
>> Cc: Tom Rini <trini@konsulko.com>
>> ---
>> Changes for v2:
>>    - Remove proposed CONFIG_SYS_SPI_KERNEL_SKIP_HEADER option, as we've determined the entire check is redundant and unnecessary.  Just delete it instead.
>>
>>  common/spl/spl_spi.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
>> index cf3f7ef4c0..22e9c87eae 100644
>> --- a/common/spl/spl_spi.c
>> +++ b/common/spl/spl_spi.c
>> @@ -34,9 +34,6 @@ 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);
>>
>> -       if (image_get_magic(header) != IH_MAGIC)
>> -               return -1;
>> -
>>         err = spl_parse_image_header(spl_image, bootdev, header);
>>         if (err)
>>                 return err;
>> --
>> 2.30.2
>>
> 
> Can you see if [1] fixes your problem? You will also need the first patch in the series.
> 
> --Sean
> 
> [1] https://lore.kernel.org/u-boot/20220401190405.1932697-8-sean.anderson@seco.com/

Re: spl: spi: Raw Kernel Image Support for Falcon Mode Boot Via SPI Devices

[Sean Anderson]

Hi Nathan,

On 4/19/22 5:49 PM, Nathan Barrett-Morrison wrote:
> [You don't often get email from nathan.morrison@timesys.com. Learn why this is important at http://aka.ms/LearnAboutSenderIdentification.]
> 
> Hi Sean,
> 
> Thanks for the response.  I don't have my embedded board I'm doing this on quite up to this version of U-Boot, but it does appear your patchset should resolve my issue in the future.  So please disregard my patch, I think we're good!

Actually, I'm not sure it will resolve your issue, since you seem to be having issues
with falcon boot, which I didn't modify. Can you try the following patch in addition
to the ones I linked above? I haven't tested it at all, so ymmv.

diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
index 037db1a19f..0ab8a10300 100644
--- a/common/spl/spl_spi.c
+++ b/common/spl/spl_spi.c
@@ -18,41 +18,6 @@
 #include <asm/global_data.h>
 #include <dm/ofnode.h>
 
-#if CONFIG_IS_ENABLED(OS_BOOT)
-/*
- * Load the kernel, check for a valid header we can parse, and if found load
- * the kernel and then device tree.
- */
-static int spi_load_image_os(struct spl_image_info *spl_image,
-			     struct spl_boot_device *bootdev,
-			     struct spi_flash *flash,
-			     struct image_header *header)
-{
-	int err;
-
-	/* Read for a header, parse or error out. */
-	spi_flash_read(flash, CONFIG_SYS_SPI_KERNEL_OFFS, sizeof(*header),
-		       (void *)header);
-
-	if (image_get_magic(header) != IH_MAGIC)
-		return -1;
-
-	err = spl_parse_image_header(spl_image, bootdev, header);
-	if (err)
-		return err;
-
-	spi_flash_read(flash, CONFIG_SYS_SPI_KERNEL_OFFS,
-		       spl_image->size, (void *)spl_image->load_addr);
-
-	/* Read device tree. */
-	spi_flash_read(flash, CONFIG_SYS_SPI_ARGS_OFFS,
-		       CONFIG_SYS_SPI_ARGS_SIZE,
-		       (void *)CONFIG_SYS_SPL_ARGS_ADDR);
-
-	return 0;
-}
-#endif
-
 static ulong spl_spi_fit_read(struct spl_load_info *load, ulong sector,
 			      ulong count, void *buf)
 {
@@ -71,6 +36,29 @@ unsigned int __weak spl_spi_get_uboot_offs(struct spi_flash *flash)
 	return CONFIG_SYS_SPI_U_BOOT_OFFS;
 }
 
+static int spi_do_load_image(struct spl_image_info *spl_image,
+			     struct spl_boot_device *bootdev,
+			     struct spl_load_info *load,
+			     unsigned int payload_offs)
+{
+	int ret;
+	struct image_header *header;
+
+	header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
+
+	/* mkimage header is 64 bytes. */
+	ret = spi_flash_read(flash, payload_offs, sizeof(*header),
+			     (void *)header);
+	if (ret) {
+		debug("%s: Failed to read from SPI flash (err=%d)\n",
+		      __func__, ret);
+		return ret;
+	}
+
+	return spl_load(spl_image, bootdev, &load, header, 0,
+			payload_offs);
+}
+
 /*
  * The main entry for SPI booting. It's necessary that SDRAM is already
  * configured and available since this code loads the main U-Boot image
@@ -82,7 +70,6 @@ static int spl_spi_load_image(struct spl_image_info *spl_image,
 	int err = 0;
 	unsigned int payload_offs;
 	struct spi_flash *flash;
-	struct image_header *header;
 	struct spl_load_info load = {
 		.bl_len = 1,
 		.read = spl_spi_fit_read,
@@ -106,31 +93,24 @@ static int spl_spi_load_image(struct spl_image_info *spl_image,
 	load.dev = flash;
 	payload_offs = spl_spi_get_uboot_offs(flash);
 
-	header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
-
 	if (CONFIG_IS_ENABLED(OF_REAL)) {
 		payload_offs = ofnode_conf_read_int("u-boot,spl-payload-offset",
 						    payload_offs);
 	}
 
 #if CONFIG_IS_ENABLED(OS_BOOT)
-	if (spl_start_uboot() || spi_load_image_os(spl_image, bootdev, flash, header))
-#endif
-	{
-		/* Load u-boot, mkimage header is 64 bytes. */
-		err = spi_flash_read(flash, payload_offs, sizeof(*header),
-				     (void *)header);
-		if (err) {
-			debug("%s: Failed to read from SPI flash (err=%d)\n",
-			      __func__, err);
-			return err;
-		}
-
-		err = spl_load(spl_image, bootdev, &load, header, 0,
-			       payload_offs);
+	if (spl_start_uboot()) {
+		err = spi_do_load_image(spl_image, bootdev, &load,
+					CONFIG_SYS_SPI_KERNEL_OFFS);
+		if (!err)
+			/* Read device tree. */
+			return spi_flash_read(flash, CONFIG_SYS_SPI_ARGS_OFFS,
+					      CONFIG_SYS_SPI_ARGS_SIZE,
+					      (void *)CONFIG_SYS_SPL_ARGS_ADDR);
 	}
+#endif
 
-	return err;
+	return spi_do_load_image(spl_image, bootdev, &load, payload_offs);
 }
 /* Use priorty 1 so that boards can override this */
 SPL_LOAD_IMAGE_METHOD("SPI", 1, BOOT_DEVICE_SPI, spl_spi_load_image);
-- 
2.35.1.1320.gc452695387.dirty

Re: spl: spi: Raw Kernel Image Support for Falcon Mode Boot Via SPI Devices

[Nathan Barrett-Morrison]

Hi Sean,

Unfortunately, I was testing this stuff out a few months ago and our
customer has moved on from using a raw kernel and is now using FIT.  I've
looked at your entire patchset more closely and agree that the last patch
you sent would be necessary as well.  It looks like that one *should *convert
SPL+SPI+Falcon into using your more generic spi_load() interface as well.

It's nice to see all of the SPL load interface being reworked.  I noticed a
ton of divergence between the various storage devices back when I was
looking at it.

If I get a chance, I will wrap back around and try your patchset, but it's
going to be awhile before our customer is enabling Falcon again I believe.

Sincerely,
Nathan

On Tue, Apr 19, 2022 at 6:03 PM Sean Anderson <sean.anderson@seco.com>
wrote:

> Hi Nathan,
>
> On 4/19/22 5:49 PM, Nathan Barrett-Morrison wrote:
> > [You don't often get email from nathan.morrison@timesys.com. Learn why
> this is important at http://aka.ms/LearnAboutSenderIdentification.]
> >
> > Hi Sean,
> >
> > Thanks for the response.  I don't have my embedded board I'm doing this
> on quite up to this version of U-Boot, but it does appear your patchset
> should resolve my issue in the future.  So please disregard my patch, I
> think we're good!
>
> Actually, I'm not sure it will resolve your issue, since you seem to be
> having issues
> with falcon boot, which I didn't modify. Can you try the following patch
> in addition
> to the ones I linked above? I haven't tested it at all, so ymmv.
>
> diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
> index 037db1a19f..0ab8a10300 100644
> --- a/common/spl/spl_spi.c
> +++ b/common/spl/spl_spi.c
> @@ -18,41 +18,6 @@
>  #include <asm/global_data.h>
>  #include <dm/ofnode.h>
>
> -#if CONFIG_IS_ENABLED(OS_BOOT)
> -/*
> - * Load the kernel, check for a valid header we can parse, and if found
> load
> - * the kernel and then device tree.
> - */
> -static int spi_load_image_os(struct spl_image_info *spl_image,
> -                            struct spl_boot_device *bootdev,
> -                            struct spi_flash *flash,
> -                            struct image_header *header)
> -{
> -       int err;
> -
> -       /* Read for a header, parse or error out. */
> -       spi_flash_read(flash, CONFIG_SYS_SPI_KERNEL_OFFS, sizeof(*header),
> -                      (void *)header);
> -
> -       if (image_get_magic(header) != IH_MAGIC)
> -               return -1;
> -
> -       err = spl_parse_image_header(spl_image, bootdev, header);
> -       if (err)
> -               return err;
> -
> -       spi_flash_read(flash, CONFIG_SYS_SPI_KERNEL_OFFS,
> -                      spl_image->size, (void *)spl_image->load_addr);
> -
> -       /* Read device tree. */
> -       spi_flash_read(flash, CONFIG_SYS_SPI_ARGS_OFFS,
> -                      CONFIG_SYS_SPI_ARGS_SIZE,
> -                      (void *)CONFIG_SYS_SPL_ARGS_ADDR);
> -
> -       return 0;
> -}
> -#endif
> -
>  static ulong spl_spi_fit_read(struct spl_load_info *load, ulong sector,
>                               ulong count, void *buf)
>  {
> @@ -71,6 +36,29 @@ unsigned int __weak spl_spi_get_uboot_offs(struct
> spi_flash *flash)
>         return CONFIG_SYS_SPI_U_BOOT_OFFS;
>  }
>
> +static int spi_do_load_image(struct spl_image_info *spl_image,
> +                            struct spl_boot_device *bootdev,
> +                            struct spl_load_info *load,
> +                            unsigned int payload_offs)
> +{
> +       int ret;
> +       struct image_header *header;
> +
> +       header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
> +
> +       /* mkimage header is 64 bytes. */
> +       ret = spi_flash_read(flash, payload_offs, sizeof(*header),
> +                            (void *)header);
> +       if (ret) {
> +               debug("%s: Failed to read from SPI flash (err=%d)\n",
> +                     __func__, ret);
> +               return ret;
> +       }
> +
> +       return spl_load(spl_image, bootdev, &load, header, 0,
> +                       payload_offs);
> +}
> +
>  /*
>   * The main entry for SPI booting. It's necessary that SDRAM is already
>   * configured and available since this code loads the main U-Boot image
> @@ -82,7 +70,6 @@ static int spl_spi_load_image(struct spl_image_info
> *spl_image,
>         int err = 0;
>         unsigned int payload_offs;
>         struct spi_flash *flash;
> -       struct image_header *header;
>         struct spl_load_info load = {
>                 .bl_len = 1,
>                 .read = spl_spi_fit_read,
> @@ -106,31 +93,24 @@ static int spl_spi_load_image(struct spl_image_info
> *spl_image,
>         load.dev = flash;
>         payload_offs = spl_spi_get_uboot_offs(flash);
>
> -       header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
> -
>         if (CONFIG_IS_ENABLED(OF_REAL)) {
>                 payload_offs =
> ofnode_conf_read_int("u-boot,spl-payload-offset",
>                                                     payload_offs);
>         }
>
>  #if CONFIG_IS_ENABLED(OS_BOOT)
> -       if (spl_start_uboot() || spi_load_image_os(spl_image, bootdev,
> flash, header))
> -#endif
> -       {
> -               /* Load u-boot, mkimage header is 64 bytes. */
> -               err = spi_flash_read(flash, payload_offs, sizeof(*header),
> -                                    (void *)header);
> -               if (err) {
> -                       debug("%s: Failed to read from SPI flash
> (err=%d)\n",
> -                             __func__, err);
> -                       return err;
> -               }
> -
> -               err = spl_load(spl_image, bootdev, &load, header, 0,
> -                              payload_offs);
> +       if (spl_start_uboot()) {
> +               err = spi_do_load_image(spl_image, bootdev, &load,
> +                                       CONFIG_SYS_SPI_KERNEL_OFFS);
> +               if (!err)
> +                       /* Read device tree. */
> +                       return spi_flash_read(flash,
> CONFIG_SYS_SPI_ARGS_OFFS,
> +                                             CONFIG_SYS_SPI_ARGS_SIZE,
> +                                             (void
> *)CONFIG_SYS_SPL_ARGS_ADDR);
>         }
> +#endif
>
> -       return err;
> +       return spi_do_load_image(spl_image, bootdev, &load, payload_offs);
>  }
>  /* Use priorty 1 so that boards can override this */
>  SPL_LOAD_IMAGE_METHOD("SPI", 1, BOOT_DEVICE_SPI, spl_spi_load_image);
> --
> 2.35.1.1320.gc452695387.dirty
>