[PATCH v2 0/2] spl: spl_nor: add alternative SPL_LOAD_IMAGE_METHOD
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(-)
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);
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!
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
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?
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
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.
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
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); }
participants (2)
-
Mario Kicherer -
Tom Rini