[PATCH v2 0/2] spl: spl_nor: add alternative SPL_LOAD_IMAGE_METHOD

[PATCH v2 0/2] spl: spl_nor: add alternative SPL_LOAD_IMAGE_METHOD

[Mario Kicherer]

Add a second SPL_LOAD_IMAGE_METHOD BOOT_DEVICE_NOR2 to enable booting
from an alternative NOR address in case loading from the first address
fails - e.g., if no valid header is found.

Changes since v1:
 - addressed right email recipients

Mario Kicherer (2):
  spl: spl_nor: add BOOT_DEVICE_NOR2 as alternative
    SPL_LOAD_IMAGE_METHOD
  spl: spl_nor: add spl_boot_device parameter to
    spl_nor_get_uboot_base()

 arch/arm/include/asm/spl.h             |  1 +
 arch/arm/mach-imx/image-container.c    |  2 +-
 arch/mips/include/asm/spl.h            |  1 +
 arch/mips/mach-mtmips/mt7621/spl/spl.c |  2 +-
 arch/mips/mach-mtmips/spl.c            |  2 +-
 arch/riscv/include/asm/spl.h           |  1 +
 common/spl/spl_nor.c                   | 11 ++++++-----
 7 files changed, 12 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] spl: spl_nor: add BOOT_DEVICE_NOR2 as alternative SPL_LOAD_IMAGE_METHOD

[Mario Kicherer]

Add BOOT_DEVICE_NOR2 as a second SPL_LOAD_IMAGE_METHOD to enable a
board-specific spl_nor_get_uboot_base() function to return an alternative
address in the NOR flash in case booting from BOOT_DEVICE_NOR fails.

Signed-off-by: Mario Kicherer <dev@kicherer.org>
---
 arch/arm/include/asm/spl.h   | 1 +
 arch/mips/include/asm/spl.h  | 1 +
 arch/riscv/include/asm/spl.h | 1 +
 common/spl/spl_nor.c         | 1 +
 4 files changed, 4 insertions(+)

diff --git a/arch/arm/include/asm/spl.h b/arch/arm/include/asm/spl.h
index 0ece4b0906..3ca4b95713 100644
--- a/arch/arm/include/asm/spl.h
+++ b/arch/arm/include/asm/spl.h
@@ -20,6 +20,7 @@ enum {
 	BOOT_DEVICE_NAND,
 	BOOT_DEVICE_ONENAND,
 	BOOT_DEVICE_NOR,
+	BOOT_DEVICE_NOR2,
 	BOOT_DEVICE_UART,
 	BOOT_DEVICE_SPI,
 	BOOT_DEVICE_USB,
diff --git a/arch/mips/include/asm/spl.h b/arch/mips/include/asm/spl.h
index 0a847edec8..a3b20ac98f 100644
--- a/arch/mips/include/asm/spl.h
+++ b/arch/mips/include/asm/spl.h
@@ -14,6 +14,7 @@ enum {
 	BOOT_DEVICE_NAND,
 	BOOT_DEVICE_ONENAND,
 	BOOT_DEVICE_NOR,
+	BOOT_DEVICE_NOR2,
 	BOOT_DEVICE_UART,
 	BOOT_DEVICE_SPI,
 	BOOT_DEVICE_USB,
diff --git a/arch/riscv/include/asm/spl.h b/arch/riscv/include/asm/spl.h
index 2898a770ee..8d7a25be33 100644
--- a/arch/riscv/include/asm/spl.h
+++ b/arch/riscv/include/asm/spl.h
@@ -16,6 +16,7 @@ enum {
 	BOOT_DEVICE_NAND,
 	BOOT_DEVICE_ONENAND,
 	BOOT_DEVICE_NOR,
+	BOOT_DEVICE_NOR2,
 	BOOT_DEVICE_UART,
 	BOOT_DEVICE_SPI,
 	BOOT_DEVICE_USB,
diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c
index 3ca44b06d6..5e8b3bf621 100644
--- a/common/spl/spl_nor.c
+++ b/common/spl/spl_nor.c
@@ -123,3 +123,4 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
 	return 0;
 }
 SPL_LOAD_IMAGE_METHOD("NOR", 0, BOOT_DEVICE_NOR, spl_nor_load_image);
+SPL_LOAD_IMAGE_METHOD("NOR2", 0, BOOT_DEVICE_NOR2, spl_nor_load_image);
-- 
2.34.1

Re: [PATCH v2 1/2] spl: spl_nor: add BOOT_DEVICE_NOR2 as alternative SPL_LOAD_IMAGE_METHOD

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 860 bytes --]

On Thu, Jan 19, 2023 at 04:28:21PM +0100, Mario Kicherer wrote:

> Add BOOT_DEVICE_NOR2 as a second SPL_LOAD_IMAGE_METHOD to enable a
> board-specific spl_nor_get_uboot_base() function to return an alternative
> address in the NOR flash in case booting from BOOT_DEVICE_NOR fails.
> 
> Signed-off-by: Mario Kicherer <dev@kicherer.org>
> ---
>  arch/arm/include/asm/spl.h   | 1 +
>  arch/mips/include/asm/spl.h  | 1 +
>  arch/riscv/include/asm/spl.h | 1 +
>  common/spl/spl_nor.c         | 1 +
>  4 files changed, 4 insertions(+)

This breaks a lot of platforms, as it only covers a few of the cases
where BOOT_DEVICE_NOR is listed. I would also really like to see how
this ends up being used in the board specific case as I do wonder if we
can't solve this some other way that won't have impact so many other
platforms.  Thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 1/2] spl: spl_nor: add BOOT_DEVICE_NOR2 as alternative SPL_LOAD_IMAGE_METHOD

[Mario Kicherer]

Hello Tom,

On 2023-01-26 20:17, Tom Rini wrote:
> This breaks a lot of platforms, as it only covers a few of the cases
> where BOOT_DEVICE_NOR is listed. I would also really like to see how
> this ends up being used in the board specific case as I do wonder if we
> can't solve this some other way that won't have impact so many other
> platforms.  Thanks!

Hm, I did a grep through the source code and that were the only places
where the new value is used as part of an enum. BOOT_DEVICE_NOR is used
in more places but they also #define this themselves - if I did not
miss something.

Furthermore, BOOT_DEVICE_NOR2 will not be used by default. A board has
to define its own board_boot_order() function like this to use the new
BOOT_DEVICE_NOR2 value:

void board_boot_order(u32 *spl_boot_list)
{
	spl_boot_list[0] = BOOT_DEVICE_NOR;
	spl_boot_list[1] = BOOT_DEVICE_NOR2;
}

If they do not explicitly add BOOT_DEVICE_NOR2, they should not be
affected by this patch, as far as I understood the code. I tried to
model this similar to the BOOT_DEVICE_MMC1 and _MMC2 values.

Then, they can also define an own spl_nor_get_uboot_base() like the
following that should return an address where U-Boot tries to load
a valid image:

unsigned long spl_nor_get_uboot_base(struct spl_boot_device *bootdev)
{
         if (bootdev->boot_device == BOOT_DEVICE_NOR) {
                 /* first address that is tried */
                 return UBOOT_PARTITION_1_IN_NOR;
         else if (bootdev->boot_device == BOOT_DEVICE_NOR2) {
                 /*
                  * if loading of the first image failed for whatever
                  * reason, try this address as well:
                  */
                 return UBOOT_PARTITION_2_IN_NOR;
         }
}

With this patch, it is possible to provide a fallback U-Boot image like
above or to use an A/B slot scheme in case a bootloader update could be
necessary in the field in the future.

Best regards,
Mario

Re: [PATCH v2 1/2] spl: spl_nor: add BOOT_DEVICE_NOR2 as alternative SPL_LOAD_IMAGE_METHOD

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2582 bytes --]

On Fri, Jan 27, 2023 at 05:56:48PM +0100, Mario Kicherer wrote:
> Hello Tom,
> 
> On 2023-01-26 20:17, Tom Rini wrote:
> > This breaks a lot of platforms, as it only covers a few of the cases
> > where BOOT_DEVICE_NOR is listed. I would also really like to see how
> > this ends up being used in the board specific case as I do wonder if we
> > can't solve this some other way that won't have impact so many other
> > platforms.  Thanks!
> 
> Hm, I did a grep through the source code and that were the only places
> where the new value is used as part of an enum. BOOT_DEVICE_NOR is used
> in more places but they also #define this themselves - if I did not
> miss something.

Yes, all of the platforms that define the value (since it roughly means
"ROM set this value in something we can check") instead of enum'ing it
still compile that file and now fail to build.

> Furthermore, BOOT_DEVICE_NOR2 will not be used by default. A board has
> to define its own board_boot_order() function like this to use the new
> BOOT_DEVICE_NOR2 value:
> 
> void board_boot_order(u32 *spl_boot_list)
> {
> 	spl_boot_list[0] = BOOT_DEVICE_NOR;
> 	spl_boot_list[1] = BOOT_DEVICE_NOR2;
> }
> 
> If they do not explicitly add BOOT_DEVICE_NOR2, they should not be
> affected by this patch, as far as I understood the code. I tried to
> model this similar to the BOOT_DEVICE_MMC1 and _MMC2 values.
> 
> Then, they can also define an own spl_nor_get_uboot_base() like the
> following that should return an address where U-Boot tries to load
> a valid image:
> 
> unsigned long spl_nor_get_uboot_base(struct spl_boot_device *bootdev)
> {
>         if (bootdev->boot_device == BOOT_DEVICE_NOR) {
>                 /* first address that is tried */
>                 return UBOOT_PARTITION_1_IN_NOR;
>         else if (bootdev->boot_device == BOOT_DEVICE_NOR2) {
>                 /*
>                  * if loading of the first image failed for whatever
>                  * reason, try this address as well:
>                  */
>                 return UBOOT_PARTITION_2_IN_NOR;
>         }
> }
> 
> With this patch, it is possible to provide a fallback U-Boot image like
> above or to use an A/B slot scheme in case a bootloader update could be
> necessary in the field in the future.

Ah, OK. Keep in mind that MMC1/MMC2 are for different physical MMC
devices on a given platform. I think this is more like the case where
you should be able to override spl_nor_get_uboot_base at the board level
to say if you're loading A or B?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 1/2] spl: spl_nor: add BOOT_DEVICE_NOR2 as alternative SPL_LOAD_IMAGE_METHOD

[Mario Kicherer]

On 2023-01-27 18:10, Tom Rini wrote:
> Yes, all of the platforms that define the value (since it roughly means
> "ROM set this value in something we can check") instead of enum'ing it
> still compile that file and now fail to build.

Okay, I think I understand your point now. I am not sure what's the best
way to proceed here. Should I try to build all targets that contain
BOOT_DEVICE_NOR and add a #define for BOOT_DEVICE_NOR2 if my patch 
really
breaks the build? Or should I just add a #define BOOT_DEVICE_NOR2 to 
every
#define BOOT_DEVICE_NOR?

It looks like a change would be necessary in these files then:

arch/arm/include/asm/arch-am33xx/spl.h
arch/arm/include/asm/arch-orion5x/spl.h
arch/arm/mach-k3/include/mach/j721e_spl.h
arch/arm/mach-k3/include/mach/j721s2_spl.h
arch/microblaze/include/asm/spl.h
arch/powerpc/include/asm/spl.h

> Ah, OK. Keep in mind that MMC1/MMC2 are for different physical MMC
> devices on a given platform. I think this is more like the case where
> you should be able to override spl_nor_get_uboot_base at the board 
> level
> to say if you're loading A or B?

Ah yes, it's been a while since I wrote this patch originally.
spl_nor_get_uboot_base() chooses between A and B and BOOT_DEVICE_NOR2
signals that I should choose the other one now.

Thank you!
Mario

Re: [PATCH v2 1/2] spl: spl_nor: add BOOT_DEVICE_NOR2 as alternative SPL_LOAD_IMAGE_METHOD

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1537 bytes --]

On Fri, Jan 27, 2023 at 06:55:46PM +0100, Mario Kicherer wrote:
> On 2023-01-27 18:10, Tom Rini wrote:
> > Yes, all of the platforms that define the value (since it roughly means
> > "ROM set this value in something we can check") instead of enum'ing it
> > still compile that file and now fail to build.
> 
> Okay, I think I understand your point now. I am not sure what's the best
> way to proceed here. Should I try to build all targets that contain
> BOOT_DEVICE_NOR and add a #define for BOOT_DEVICE_NOR2 if my patch really
> breaks the build? Or should I just add a #define BOOT_DEVICE_NOR2 to every
> #define BOOT_DEVICE_NOR?
> 
> It looks like a change would be necessary in these files then:
> 
> arch/arm/include/asm/arch-am33xx/spl.h
> arch/arm/include/asm/arch-orion5x/spl.h
> arch/arm/mach-k3/include/mach/j721e_spl.h
> arch/arm/mach-k3/include/mach/j721s2_spl.h
> arch/microblaze/include/asm/spl.h
> arch/powerpc/include/asm/spl.h
> 
> > Ah, OK. Keep in mind that MMC1/MMC2 are for different physical MMC
> > devices on a given platform. I think this is more like the case where
> > you should be able to override spl_nor_get_uboot_base at the board level
> > to say if you're loading A or B?
> 
> Ah yes, it's been a while since I wrote this patch originally.
> spl_nor_get_uboot_base() chooses between A and B and BOOT_DEVICE_NOR2
> signals that I should choose the other one now.

I think you should be able to solve this problem without making further
changes upstream, yes.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 1/2] spl: spl_nor: add BOOT_DEVICE_NOR2 as alternative SPL_LOAD_IMAGE_METHOD

[Mario Kicherer]

On 2023-01-27 18:58, Tom Rini wrote:
>> Okay, I think I understand your point now. I am not sure what's the 
>> best
>> way to proceed here. Should I try to build all targets that contain
>> BOOT_DEVICE_NOR and add a #define for BOOT_DEVICE_NOR2 if my patch 
>> really
>> breaks the build? Or should I just add a #define BOOT_DEVICE_NOR2 to 
>> every
>> #define BOOT_DEVICE_NOR?
>> 
>> It looks like a change would be necessary in these files then:
>> 
>> arch/arm/include/asm/arch-am33xx/spl.h
>> arch/arm/include/asm/arch-orion5x/spl.h
>> arch/arm/mach-k3/include/mach/j721e_spl.h
>> arch/arm/mach-k3/include/mach/j721s2_spl.h
>> arch/microblaze/include/asm/spl.h
>> arch/powerpc/include/asm/spl.h
>> 
>> > Ah, OK. Keep in mind that MMC1/MMC2 are for different physical MMC
>> > devices on a given platform. I think this is more like the case where
>> > you should be able to override spl_nor_get_uboot_base at the board level
>> > to say if you're loading A or B?
>> 
>> Ah yes, it's been a while since I wrote this patch originally.
>> spl_nor_get_uboot_base() chooses between A and B and BOOT_DEVICE_NOR2
>> signals that I should choose the other one now.
> 
> I think you should be able to solve this problem without making further
> changes upstream, yes.

I am not sure I understand your answer. Both ways I mentioned would 
probably
require further changes. Or do you mean I should find a way to implement
this only with board-specific code?

I sent two additional patches to the mailing list:

[PATCH] spl: spl_nor: use panic instead of hang if booting fails
[PATCH] spl: spl-nor: return error if no valid image was loaded

which would make this possible.

However, I would prefer this patch series as it enables loading U-Boot
from an alternate location without additional resets and relying on a
boot counter.

Best regards,
Mario

[PATCH v2 2/2] spl: spl_nor: add spl_boot_device parameter to spl_nor_get_uboot_base()

[Mario Kicherer]

With this additional parameter, a board-specific implementation of this
function can return an alternative address in case booting from the first
address in the NOR flash fails.

Signed-off-by: Mario Kicherer <dev@kicherer.org>
---
 arch/arm/mach-imx/image-container.c    |  2 +-
 arch/mips/mach-mtmips/mt7621/spl/spl.c |  2 +-
 arch/mips/mach-mtmips/spl.c            |  2 +-
 common/spl/spl_nor.c                   | 10 +++++-----
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-imx/image-container.c b/arch/arm/mach-imx/image-container.c
index 0e76786482..41fb9470a4 100644
--- a/arch/arm/mach-imx/image-container.c
+++ b/arch/arm/mach-imx/image-container.c
@@ -243,7 +243,7 @@ uint32_t spl_nand_get_uboot_raw_page(void)
 #endif
 
 #ifdef CONFIG_SPL_NOR_SUPPORT
-unsigned long spl_nor_get_uboot_base(void)
+unsigned long spl_nor_get_uboot_base(struct spl_boot_device *bootdev)
 {
 	int end;
 
diff --git a/arch/mips/mach-mtmips/mt7621/spl/spl.c b/arch/mips/mach-mtmips/mt7621/spl/spl.c
index aa5b267bb9..76719ad2f6 100644
--- a/arch/mips/mach-mtmips/mt7621/spl/spl.c
+++ b/arch/mips/mach-mtmips/mt7621/spl/spl.c
@@ -61,7 +61,7 @@ void board_boot_order(u32 *spl_boot_list)
 #endif
 }
 
-unsigned long spl_nor_get_uboot_base(void)
+unsigned long spl_nor_get_uboot_base(struct spl_boot_device *bootdev)
 {
 	const struct tpl_info *tpli;
 	const struct legacy_img_hdr *hdr;
diff --git a/arch/mips/mach-mtmips/spl.c b/arch/mips/mach-mtmips/spl.c
index fe5b49e702..ac7ca20027 100644
--- a/arch/mips/mach-mtmips/spl.c
+++ b/arch/mips/mach-mtmips/spl.c
@@ -34,7 +34,7 @@ void board_boot_order(u32 *spl_boot_list)
 	spl_boot_list[0] = BOOT_DEVICE_NOR;
 }
 
-unsigned long spl_nor_get_uboot_base(void)
+unsigned long spl_nor_get_uboot_base(struct spl_boot_device *bootdev)
 {
 	void *uboot_base = __image_copy_end;
 
diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c
index 5e8b3bf621..e2ec2b9edf 100644
--- a/common/spl/spl_nor.c
+++ b/common/spl/spl_nor.c
@@ -17,7 +17,7 @@ static ulong spl_nor_load_read(struct spl_load_info *load, ulong sector,
 	return count;
 }
 
-unsigned long __weak spl_nor_get_uboot_base(void)
+unsigned long __weak spl_nor_get_uboot_base(struct spl_boot_device *bootdev)
 {
 	return CONFIG_SYS_UBOOT_BASE;
 }
@@ -91,13 +91,13 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
 	 * defined location in SDRAM
 	 */
 #ifdef CONFIG_SPL_LOAD_FIT
-	header = (const struct legacy_img_hdr *)spl_nor_get_uboot_base();
+	header = (const struct legacy_img_hdr *)spl_nor_get_uboot_base(bootdev);
 	if (image_get_magic(header) == FDT_MAGIC) {
 		debug("Found FIT format U-Boot\n");
 		load.bl_len = 1;
 		load.read = spl_nor_load_read;
 		return spl_load_simple_fit(spl_image, &load,
-					   spl_nor_get_uboot_base(),
+					   spl_nor_get_uboot_base(bootdev),
 					   (void *)header);
 	}
 #endif
@@ -105,7 +105,7 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
 		load.bl_len = 1;
 		load.read = spl_nor_load_read;
 		return spl_load_imx_container(spl_image, &load,
-					      spl_nor_get_uboot_base());
+					      spl_nor_get_uboot_base(bootdev));
 	}
 
 	/* Legacy image handling */
@@ -116,7 +116,7 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
 		load.read = spl_nor_load_read;
 		spl_nor_load_read(&load, spl_nor_get_uboot_base(bootdev), sizeof(hdr), &hdr);
 		return spl_load_legacy_img(spl_image, bootdev, &load,
-					   spl_nor_get_uboot_base(),
+					   spl_nor_get_uboot_base(bootdev),
 					   &hdr);
 	}
 
-- 
2.34.1