[U-Boot] [PATCH 1/2] sunxi: Create helper function veryfing valid boot signature on MMC
This patch extracts checking for valid SD card "eGON.BT0" signature from `board_mmc_init` into function `sunxi_mmc_has_egon_boot_signature`.
Buffer for mmc sector is allocated and freed at runtime. `panic` is triggered on malloc failure.
Signed-off-by: Daniel Kochmański dkochmanski@turtle-solutions.eu CC: Roy Spliet r.spliet@ultimaker.com Cc: Ian Campbell ijc@hellion.org.uk Cc: Hans De Goede hdegoede@redhat.com
helper --- arch/arm/include/asm/arch-sunxi/mmc.h | 1 + board/sunxi/board.c | 8 ++------ drivers/mmc/sunxi_mmc.c | 17 +++++++++++++++++ 3 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/arch/arm/include/asm/arch-sunxi/mmc.h b/arch/arm/include/asm/arch-sunxi/mmc.h index cb52e64..3da360b 100644 --- a/arch/arm/include/asm/arch-sunxi/mmc.h +++ b/arch/arm/include/asm/arch-sunxi/mmc.h @@ -127,4 +127,5 @@ struct sunxi_mmc { #define SUNXI_MMC_COMMON_RESET (1 << 18)
struct mmc *sunxi_mmc_init(int sdc_no); +int sunxi_mmc_has_egon_boot_signature(struct mmc *mmc); #endif /* _SUNXI_MMC_H */ diff --git a/board/sunxi/board.c b/board/sunxi/board.c index d5bed30..aa18a10 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -302,12 +302,8 @@ int board_mmc_init(bd_t *bis) * Both mmc0 and mmc2 are bootable, figure out where we're booting * from. Try mmc0 first, just like the brom does. */ - if (mmc_getcd(mmc0) && mmc_init(mmc0) == 0 && - mmc0->block_dev.block_read(0, 16, 1, buf) == 1) { - buf[12] = 0; - if (strcmp(&buf[4], "eGON.BT0") == 0) - return 0; - } + if (sunxi_mmc_has_egon_boot_signature(mmc0)) + return 0;
/* no bootable card in mmc0, so we must be booting from mmc2, swap */ mmc0->block_dev.dev = 1; diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c index c4300f2..c01d142 100644 --- a/drivers/mmc/sunxi_mmc.c +++ b/drivers/mmc/sunxi_mmc.c @@ -431,6 +431,23 @@ static int sunxi_mmc_getcd(struct mmc *mmc) return !gpio_get_value(cd_pin); }
+int sunxi_mmc_has_egon_boot_signature(struct mmc *mmc) +{ + char *buf = malloc(512); + int valid_signature = 0; + + if (buf == NULL) + panic("Failed to allocate memory\n"); + + if (mmc_getcd(mmc) && mmc_init(mmc) == 0 && + mmc->block_dev.block_read(0, 16, 1, buf) == 1 && + strncmp(&buf[4], "eGON.BT0", 8) == 0) + valid_signature = 1; + + free(buf); + return valid_signature; +} + static const struct mmc_ops sunxi_mmc_ops = { .send_cmd = sunxi_mmc_send_cmd, .set_ios = sunxi_mmc_set_ios,
Make possible using single `u-boot-sunxi-with-spl.bin` binary for both NAND memory and SD card. Detection where SPL was read from is implemented in `spl_boot_device`.
Detection is performed only if `SPL_NAND_SUPPORT` is enabled. Unless SD card contains valid signature we assume, that SPL was read from NAND.
V2: - Move signature verification to helper function - Avoid unnecessary condition nesting
Signed-off-by: Daniel Kochmański dkochmanski@turtle-solutions.eu CC: Roy Spliet r.spliet@ultimaker.com Cc: Ian Campbell ijc@hellion.org.uk Cc: Hans De Goede hdegoede@redhat.com --- arch/arm/cpu/armv7/sunxi/board.c | 46 +++++++++++++++++++++++----------------- include/configs/sunxi-common.h | 2 -- 2 files changed, 26 insertions(+), 22 deletions(-)
diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c index 70f413f..b98fb51 100644 --- a/arch/arm/cpu/armv7/sunxi/board.c +++ b/arch/arm/cpu/armv7/sunxi/board.c @@ -11,6 +11,7 @@ */
#include <common.h> +#include <mmc.h> #include <i2c.h> #include <serial.h> #ifdef CONFIG_SPL_BUILD @@ -22,6 +23,7 @@ #include <asm/arch/gpio.h> #include <asm/arch/sys_proto.h> #include <asm/arch/timer.h> +#include <asm/arch/mmc.h>
#include <linux/compiler.h>
@@ -109,12 +111,10 @@ void s_init(void) }
#ifdef CONFIG_SPL_BUILD +DECLARE_GLOBAL_DATA_PTR; + /* The sunxi internal brom will try to loader external bootloader * from mmc0, nand flash, mmc2. - * - * Unfortunately we can't check how SPL was loaded so assume it's - * always the first SD/MMC controller, unless it was explicitly - * stated that SPL is on nand flash. */ u32 spl_boot_device(void) { @@ -124,17 +124,13 @@ u32 spl_boot_device(void) * enabled build. It has many restrictions and can only boot over USB. */ return BOOT_DEVICE_BOARD; -#elif defined(CONFIG_SPL_NAND_SUPPORT) - /* - * This is compile time configuration informing SPL, that it - * was loaded from nand flash. - */ - return BOOT_DEVICE_NAND; #else + __maybe_unused struct mmc *mmc0; + /* - * When booting from the SD card, the "eGON.BT0" signature is expected - * to be found in memory at the address 0x0004 (see the "mksunxiboot" - * tool, which generates this header). + * When booting from the SD card or NAND memory, the "eGON.BT0" + * signature is expected to be found in memory at the address 0x0004 + * (see the "mksunxiboot" tool, which generates this header). * * When booting in the FEL mode over USB, this signature is patched in * memory and replaced with something else by the 'fel' tool. This other @@ -142,15 +138,25 @@ u32 spl_boot_device(void) * valid bootable SD card image (because the BROM would refuse to * execute the SPL in this case). * - * This branch is just making a decision at runtime whether to load - * the main u-boot binary from the SD card (if the "eGON.BT0" signature - * is found) or return to the FEL code in the BROM to wait and receive - * the main u-boot binary over USB. + * This checks for the signature and if it is not found returns to + * the FEL code in the BROM to wait and receive the main u-boot + * binary over USB. If it is found, it determines where SPL was + * read from. */ - if (readl(4) == 0x4E4F4765 && readl(8) == 0x3054422E) /* eGON.BT0 */ - return BOOT_DEVICE_MMC1; - else + if (readl(4) != 0x4E4F4765 || readl(8) != 0x3054422E) /* eGON.BT0 */ return BOOT_DEVICE_BOARD; + + /* If NAND isn't enabled, then we boot either from mmc0 or mmc2. */ + if (!IS_ENABLED(CONFIG_SPL_NAND_SUPPORT)) + return BOOT_DEVICE_MMC1; + + /* The BROM will try to boot from mmc0 first, so try that first. */ + mmc_initialize(gd->bd); + mmc0 = find_mmc_device(0); + if (sunxi_mmc_has_egon_boot_signature(mmc0)) + return BOOT_DEVICE_MMC1; + + return BOOT_DEVICE_NAND; #endif }
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h index cce0441..e4db777 100644 --- a/include/configs/sunxi-common.h +++ b/include/configs/sunxi-common.h @@ -106,10 +106,8 @@ #define CONFIG_CMD_MMC #define CONFIG_MMC_SUNXI #define CONFIG_MMC_SUNXI_SLOT 0 -#if !defined(CONFIG_SPL_NAND_SUPPORT) #define CONFIG_ENV_IS_IN_MMC #define CONFIG_SYS_MMC_ENV_DEV 0 /* first detected MMC controller */ -#endif /* CONFIG_SPL_NAND_SUPPORT */ #endif
/* 4MB of malloc() pool */
Hi,
On 28-05-15 15:18, Daniel Kochmański wrote:
Make possible using single `u-boot-sunxi-with-spl.bin` binary for both NAND memory and SD card. Detection where SPL was read from is implemented in `spl_boot_device`.
Detection is performed only if `SPL_NAND_SUPPORT` is enabled. Unless SD card contains valid signature we assume, that SPL was read from NAND.
V2:
- Move signature verification to helper function
- Avoid unnecessary condition nesting
Thanks for the v2, unfortunately I've found an issue which needs fixing before I can merge this, see comments inline.
Signed-off-by: Daniel Kochmański dkochmanski@turtle-solutions.eu CC: Roy Spliet r.spliet@ultimaker.com Cc: Ian Campbell ijc@hellion.org.uk Cc: Hans De Goede hdegoede@redhat.com
arch/arm/cpu/armv7/sunxi/board.c | 46 +++++++++++++++++++++++----------------- include/configs/sunxi-common.h | 2 -- 2 files changed, 26 insertions(+), 22 deletions(-)
diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c index 70f413f..b98fb51 100644 --- a/arch/arm/cpu/armv7/sunxi/board.c +++ b/arch/arm/cpu/armv7/sunxi/board.c @@ -11,6 +11,7 @@ */
#include <common.h> +#include <mmc.h> #include <i2c.h> #include <serial.h> #ifdef CONFIG_SPL_BUILD @@ -22,6 +23,7 @@ #include <asm/arch/gpio.h> #include <asm/arch/sys_proto.h> #include <asm/arch/timer.h> +#include <asm/arch/mmc.h>
#include <linux/compiler.h>
@@ -109,12 +111,10 @@ void s_init(void) }
#ifdef CONFIG_SPL_BUILD +DECLARE_GLOBAL_DATA_PTR;
- /* The sunxi internal brom will try to loader external bootloader
- from mmc0, nand flash, mmc2.
- Unfortunately we can't check how SPL was loaded so assume it's
- always the first SD/MMC controller, unless it was explicitly
*/ u32 spl_boot_device(void) {
- stated that SPL is on nand flash.
@@ -124,17 +124,13 @@ u32 spl_boot_device(void) * enabled build. It has many restrictions and can only boot over USB. */ return BOOT_DEVICE_BOARD; -#elif defined(CONFIG_SPL_NAND_SUPPORT)
- /*
* This is compile time configuration informing SPL, that it* was loaded from nand flash.*/- return BOOT_DEVICE_NAND; #else
- __maybe_unused struct mmc *mmc0;
No need for maybe unused now.
/*
* When booting from the SD card, the "eGON.BT0" signature is expected* to be found in memory at the address 0x0004 (see the "mksunxiboot"* tool, which generates this header).
* When booting from the SD card or NAND memory, the "eGON.BT0"* signature is expected to be found in memory at the address 0x0004* (see the "mksunxiboot" tool, which generates this header).
- When booting in the FEL mode over USB, this signature is patched in
- memory and replaced with something else by the 'fel' tool. This other
@@ -142,15 +138,25 @@ u32 spl_boot_device(void) * valid bootable SD card image (because the BROM would refuse to * execute the SPL in this case). *
* This branch is just making a decision at runtime whether to load* the main u-boot binary from the SD card (if the "eGON.BT0" signature* is found) or return to the FEL code in the BROM to wait and receive* the main u-boot binary over USB.
* This checks for the signature and if it is not found returns to* the FEL code in the BROM to wait and receive the main u-boot* binary over USB. If it is found, it determines where SPL was* read from.
- if (readl(4) == 0x4E4F4765 && readl(8) == 0x3054422E) /* eGON.BT0 */
return BOOT_DEVICE_MMC1;- else
- if (readl(4) != 0x4E4F4765 || readl(8) != 0x3054422E) /* eGON.BT0 */ return BOOT_DEVICE_BOARD;
- /* If NAND isn't enabled, then we boot either from mmc0 or mmc2. */
- if (!IS_ENABLED(CONFIG_SPL_NAND_SUPPORT))
return BOOT_DEVICE_MMC1;
I would really prefer for this block to be below the mmc check, I do not like the inverted test here, and this also "checks" things in a different order then the BROM does, it would be nice for the code flow in this function to match the flow from the datasheets for the BROM.
Yes this means always doing the mmc check, that should be fine if it fails we apparently cannot access the sdcard, and thus cannot load the real u-boot binary / the next stage and things will fail anyways.
- /* The BROM will try to boot from mmc0 first, so try that first. */
- mmc_initialize(gd->bd);
Ugh, I missed this while reviewing the previous version. mmc_initialize will get called a second time from common/spl/spl_mmc.c when we are actually booting from the mmc and doing this twice is undesirable, looking at the code I guess it will work more or less (it leaks memory) but doing so is just wrong.
The best way I can come up with to fix this is to protect mmc_initialize() against being called twice, simply returning success immediately on the second call.
Please do a v3 with a preparation patch making this change to mmc_initialize(), and Cc the mmc maintainer on the posting of v3 of this set. Also please base v3 on:
http://git.denx.de/?p=u-boot/u-boot-sunxi.git;a=shortlog;h=refs/heads/next
There are some commits there which conclict with the tree you're basing your patches on.
- mmc0 = find_mmc_device(0);
- if (sunxi_mmc_has_egon_boot_signature(mmc0))
return BOOT_DEVICE_MMC1;
- return BOOT_DEVICE_NAND;
And replace this with:
/* Fallback to booting NAND if enabled */ if (IS_ENABLED(CONFIG_SPL_NAND_SUPPORT)) return BOOT_DEVICE_NAND;
panic("Could not determine boot source\n"); return -1; /* Never reached */
Note that the check whether to boot from mmc2 rather then mmc0 also really should be moved here (after the nand check) but I can do that myself, since you probably do not have hardware to test this.
Regards,
Hans
#endif }
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h index cce0441..e4db777 100644 --- a/include/configs/sunxi-common.h +++ b/include/configs/sunxi-common.h @@ -106,10 +106,8 @@ #define CONFIG_CMD_MMC #define CONFIG_MMC_SUNXI #define CONFIG_MMC_SUNXI_SLOT 0 -#if !defined(CONFIG_SPL_NAND_SUPPORT) #define CONFIG_ENV_IS_IN_MMC #define CONFIG_SYS_MMC_ENV_DEV 0 /* first detected MMC controller */ -#endif /* CONFIG_SPL_NAND_SUPPORT */ #endif
/* 4MB of malloc() pool */
Hi,
Hans de Goede writes:
Hi,
On 28-05-15 15:18, Daniel Kochmański wrote:
Make possible using single `u-boot-sunxi-with-spl.bin` binary for both NAND memory and SD card. Detection where SPL was read from is implemented in `spl_boot_device`.
Detection is performed only if `SPL_NAND_SUPPORT` is enabled. Unless SD card contains valid signature we assume, that SPL was read from NAND.
V2:
- Move signature verification to helper function
- Avoid unnecessary condition nesting
Thanks for the v2, unfortunately I've found an issue which needs fixing before I can merge this, see comments inline.
Signed-off-by: Daniel Kochmański dkochmanski@turtle-solutions.eu CC: Roy Spliet r.spliet@ultimaker.com Cc: Ian Campbell ijc@hellion.org.uk Cc: Hans De Goede hdegoede@redhat.com
arch/arm/cpu/armv7/sunxi/board.c | 46 +++++++++++++++++++++++----------------- include/configs/sunxi-common.h | 2 -- 2 files changed, 26 insertions(+), 22 deletions(-)
diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c index 70f413f..b98fb51 100644 --- a/arch/arm/cpu/armv7/sunxi/board.c +++ b/arch/arm/cpu/armv7/sunxi/board.c @@ -11,6 +11,7 @@ */
#include <common.h> +#include <mmc.h> #include <i2c.h> #include <serial.h> #ifdef CONFIG_SPL_BUILD @@ -22,6 +23,7 @@ #include <asm/arch/gpio.h> #include <asm/arch/sys_proto.h> #include <asm/arch/timer.h> +#include <asm/arch/mmc.h>
#include <linux/compiler.h>
@@ -109,12 +111,10 @@ void s_init(void) }
#ifdef CONFIG_SPL_BUILD +DECLARE_GLOBAL_DATA_PTR;
- /* The sunxi internal brom will try to loader external bootloader
- from mmc0, nand flash, mmc2.
- Unfortunately we can't check how SPL was loaded so assume it's
- always the first SD/MMC controller, unless it was explicitly
*/ u32 spl_boot_device(void) {
- stated that SPL is on nand flash.
@@ -124,17 +124,13 @@ u32 spl_boot_device(void) * enabled build. It has many restrictions and can only boot over USB. */ return BOOT_DEVICE_BOARD; -#elif defined(CONFIG_SPL_NAND_SUPPORT)
- /*
* This is compile time configuration informing SPL, that it* was loaded from nand flash.*/- return BOOT_DEVICE_NAND; #else
- __maybe_unused struct mmc *mmc0;
No need for maybe unused now.
Ack
/*
* When booting from the SD card, the "eGON.BT0" signature is expected* to be found in memory at the address 0x0004 (see the "mksunxiboot"* tool, which generates this header).
* When booting from the SD card or NAND memory, the "eGON.BT0"* signature is expected to be found in memory at the address 0x0004* (see the "mksunxiboot" tool, which generates this header).
- When booting in the FEL mode over USB, this signature is patched in
- memory and replaced with something else by the 'fel' tool. This other
@@ -142,15 +138,25 @@ u32 spl_boot_device(void) * valid bootable SD card image (because the BROM would refuse to * execute the SPL in this case). *
* This branch is just making a decision at runtime whether to load* the main u-boot binary from the SD card (if the "eGON.BT0" signature* is found) or return to the FEL code in the BROM to wait and receive* the main u-boot binary over USB.
* This checks for the signature and if it is not found returns to* the FEL code in the BROM to wait and receive the main u-boot* binary over USB. If it is found, it determines where SPL was* read from.
- if (readl(4) == 0x4E4F4765 && readl(8) == 0x3054422E) /* eGON.BT0 */
return BOOT_DEVICE_MMC1;- else
- if (readl(4) != 0x4E4F4765 || readl(8) != 0x3054422E) /* eGON.BT0 */ return BOOT_DEVICE_BOARD;
- /* If NAND isn't enabled, then we boot either from mmc0 or mmc2. */
- if (!IS_ENABLED(CONFIG_SPL_NAND_SUPPORT))
return BOOT_DEVICE_MMC1;I would really prefer for this block to be below the mmc check, I do not like the inverted test here, and this also "checks" things in a different order then the BROM does, it would be nice for the code flow in this function to match the flow from the datasheets for the BROM.
Yes this means always doing the mmc check, that should be fine if it fails we apparently cannot access the sdcard, and thus cannot load the real u-boot binary / the next stage and things will fail anyways.
But it also means, that if we boot from mmc2 (as pointed out below, I have no hardware to test it), then boot will fail. Currently SPL assumes, that if boot is performed from MMC1 and there is no signature in mmc0, then it was read from mmc2.
- /* The BROM will try to boot from mmc0 first, so try that first. */
- mmc_initialize(gd->bd);
Ugh, I missed this while reviewing the previous version. mmc_initialize will get called a second time from common/spl/spl_mmc.c when we are actually booting from the mmc and doing this twice is undesirable, looking at the code I guess it will work more or less (it leaks memory) but doing so is just wrong.
I'm not sure where it leaks memory? Unless there is no panic in `sunxi_mmc_has_egon_boot_signature`, buf is always freed before returning result of the check. Do you refer to some other function?
The best way I can come up with to fix this is to protect mmc_initialize() against being called twice, simply returning success immediately on the second call.
Please do a v3 with a preparation patch making this change to mmc_initialize(), and Cc the mmc maintainer on the posting of v3 of this set. Also please base v3 on:
http://git.denx.de/?p=u-boot/u-boot-sunxi.git;a=shortlog;h=refs/heads/next
There are some commits there which conclict with the tree you're basing your patches on.
Ack
- mmc0 = find_mmc_device(0);
- if (sunxi_mmc_has_egon_boot_signature(mmc0))
return BOOT_DEVICE_MMC1;
- return BOOT_DEVICE_NAND;
And replace this with:
/* Fallback to booting NAND if enabled */ if (IS_ENABLED(CONFIG_SPL_NAND_SUPPORT)) return BOOT_DEVICE_NAND;
panic("Could not determine boot source\n"); return -1; /* Never reached */
Note that the check whether to boot from mmc2 rather then mmc0 also really should be moved here (after the nand check) but I can do that myself, since you probably do not have hardware to test this.
Should I mention in commit message, that state of SPL after this patches disable booting from MMC2 until next patch, or should I omit that?
Regards,
Hans
BR, Daniel
#endif }
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h index cce0441..e4db777 100644 --- a/include/configs/sunxi-common.h +++ b/include/configs/sunxi-common.h @@ -106,10 +106,8 @@ #define CONFIG_CMD_MMC #define CONFIG_MMC_SUNXI #define CONFIG_MMC_SUNXI_SLOT 0 -#if !defined(CONFIG_SPL_NAND_SUPPORT) #define CONFIG_ENV_IS_IN_MMC #define CONFIG_SYS_MMC_ENV_DEV 0 /* first detected MMC controller */ -#endif /* CONFIG_SPL_NAND_SUPPORT */ #endif
/* 4MB of malloc() pool */
Hi,
On 29-05-15 14:18, Daniel Kochmański wrote:
Hi,
Hans de Goede writes:
Hi,
On 28-05-15 15:18, Daniel Kochmański wrote:
Make possible using single `u-boot-sunxi-with-spl.bin` binary for both NAND memory and SD card. Detection where SPL was read from is implemented in `spl_boot_device`.
Detection is performed only if `SPL_NAND_SUPPORT` is enabled. Unless SD card contains valid signature we assume, that SPL was read from NAND.
V2:
- Move signature verification to helper function
- Avoid unnecessary condition nesting
Thanks for the v2, unfortunately I've found an issue which needs fixing before I can merge this, see comments inline.
Signed-off-by: Daniel Kochmański dkochmanski@turtle-solutions.eu CC: Roy Spliet r.spliet@ultimaker.com Cc: Ian Campbell ijc@hellion.org.uk Cc: Hans De Goede hdegoede@redhat.com
arch/arm/cpu/armv7/sunxi/board.c | 46 +++++++++++++++++++++++----------------- include/configs/sunxi-common.h | 2 -- 2 files changed, 26 insertions(+), 22 deletions(-)
diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c index 70f413f..b98fb51 100644 --- a/arch/arm/cpu/armv7/sunxi/board.c +++ b/arch/arm/cpu/armv7/sunxi/board.c @@ -11,6 +11,7 @@ */
#include <common.h> +#include <mmc.h> #include <i2c.h> #include <serial.h> #ifdef CONFIG_SPL_BUILD @@ -22,6 +23,7 @@ #include <asm/arch/gpio.h> #include <asm/arch/sys_proto.h> #include <asm/arch/timer.h> +#include <asm/arch/mmc.h>
#include <linux/compiler.h>
@@ -109,12 +111,10 @@ void s_init(void) }
#ifdef CONFIG_SPL_BUILD +DECLARE_GLOBAL_DATA_PTR;
- /* The sunxi internal brom will try to loader external bootloader
- from mmc0, nand flash, mmc2.
- Unfortunately we can't check how SPL was loaded so assume it's
- always the first SD/MMC controller, unless it was explicitly
u32 spl_boot_device(void) {
- stated that SPL is on nand flash. */
@@ -124,17 +124,13 @@ u32 spl_boot_device(void) * enabled build. It has many restrictions and can only boot over USB. */ return BOOT_DEVICE_BOARD; -#elif defined(CONFIG_SPL_NAND_SUPPORT)
- /*
* This is compile time configuration informing SPL, that it* was loaded from nand flash.*/- return BOOT_DEVICE_NAND; #else
- __maybe_unused struct mmc *mmc0;
No need for maybe unused now.
Ack
/*
* When booting from the SD card, the "eGON.BT0" signature is expected* to be found in memory at the address 0x0004 (see the "mksunxiboot"* tool, which generates this header).
* When booting from the SD card or NAND memory, the "eGON.BT0"* signature is expected to be found in memory at the address 0x0004* (see the "mksunxiboot" tool, which generates this header).
- When booting in the FEL mode over USB, this signature is patched in
- memory and replaced with something else by the 'fel' tool. This other
@@ -142,15 +138,25 @@ u32 spl_boot_device(void) * valid bootable SD card image (because the BROM would refuse to * execute the SPL in this case). *
* This branch is just making a decision at runtime whether to load* the main u-boot binary from the SD card (if the "eGON.BT0" signature* is found) or return to the FEL code in the BROM to wait and receive* the main u-boot binary over USB.
* This checks for the signature and if it is not found returns to* the FEL code in the BROM to wait and receive the main u-boot* binary over USB. If it is found, it determines where SPL was* read from.
- if (readl(4) == 0x4E4F4765 && readl(8) == 0x3054422E) /* eGON.BT0 */
return BOOT_DEVICE_MMC1;- else
- if (readl(4) != 0x4E4F4765 || readl(8) != 0x3054422E) /* eGON.BT0 */ return BOOT_DEVICE_BOARD;
- /* If NAND isn't enabled, then we boot either from mmc0 or mmc2. */
- if (!IS_ENABLED(CONFIG_SPL_NAND_SUPPORT))
return BOOT_DEVICE_MMC1;I would really prefer for this block to be below the mmc check, I do not like the inverted test here, and this also "checks" things in a different order then the BROM does, it would be nice for the code flow in this function to match the flow from the datasheets for the BROM.
Yes this means always doing the mmc check, that should be fine if it fails we apparently cannot access the sdcard, and thus cannot load the real u-boot binary / the next stage and things will fail anyways.
But it also means, that if we boot from mmc2 (as pointed out below, I have no hardware to test it), then boot will fail. Currently SPL assumes, that if boot is performed from MMC1 and there is no signature in mmc0, then it was read from mmc2.
Ah good point, ok I think the best way to solve this is to just move the mmc2 detect code into here (where one could argue it should have been anyways) in the same commit. Can you please do that? I can test that it actually works before merging.
So the end result would look something like this:
struct mmc *mmc0, *mmc1;
/* * When booting from the SD card or NAND memory, the "eGON.BT0" * signature is expected to be found in memory at the address 0x0004 * (see the "mksunxiboot" tool, which generates this header). * * When booting in the FEL mode over USB, this signature is patched * in memory and replaced with something else by the 'fel' tool. * * This checks for the signature and if it is not found returns to * the FEL code in the BROM to wait and receive the main u-boot * binary over USB. */ if (readl(4) != 0x4E4F4765 || readl(8) != 0x3054422E) /* eGON.BT0 */ return BOOT_DEVICE_BOARD;
/* The BROM will try to boot from mmc0 first, so try that first */ mmc_initialize(gd->bd); mmc0 = find_mmc_device(0); if (sunxi_mmc_has_egon_boot_signature(mmc0)) return BOOT_DEVICE_MMC1;
/* Fallback to booting NAND if enabled */ if (IS_ENABLED(SPL_NAND_SUPPORT)) return BOOT_DEVICE_NAND;
/* Fallback to booting from mmc2 if enabled */ if (CONFIG_MMC_SUNXI_SLOT_EXTRA == 2) { /* * spl_mmc.c: spl_mmc_load_image() is hard-coded to use * find_mmc_device(0), no matter what we return, * swap mmc0 and mmc2 to make this work. */ mmc1 = find_mmc_device(1); mmc0->block_dev.dev = 1; mmc1->block_dev.dev = 0; return BOOT_DEVICE_MMC2; }
panic("Could not determine boot source\n"); return -1; /* Never reached */
Note untested :)
- /* The BROM will try to boot from mmc0 first, so try that first. */
- mmc_initialize(gd->bd);
Ugh, I missed this while reviewing the previous version. mmc_initialize will get called a second time from common/spl/spl_mmc.c when we are actually booting from the mmc and doing this twice is undesirable, looking at the code I guess it will work more or less (it leaks memory) but doing so is just wrong.
I'm not sure where it leaks memory? Unless there is no panic in `sunxi_mmc_has_egon_boot_signature`, buf is always freed before returning result of the check. Do you refer to some other function?
mmc_initalize calls mmc_board_init which calls sunxi_mmc_init which calls mmc_create which does a calloc(). More over the mmc controller(s) will get reset twice, the mmc_devices list head gets initialized twice (removing the references to the mmc controller structs created in the first call into mmc_initalize), etc. This is just wrong, it all happens to work by accident, but we should not rely on that.
The best way I can come up with to fix this is to protect mmc_initialize() against being called twice, simply returning success immediately on the second call.
Please do a v3 with a preparation patch making this change to mmc_initialize(), and Cc the mmc maintainer on the posting of v3 of this set. Also please base v3 on:
http://git.denx.de/?p=u-boot/u-boot-sunxi.git;a=shortlog;h=refs/heads/next
There are some commits there which conclict with the tree you're basing your patches on.
Ack
- mmc0 = find_mmc_device(0);
- if (sunxi_mmc_has_egon_boot_signature(mmc0))
return BOOT_DEVICE_MMC1;
- return BOOT_DEVICE_NAND;
And replace this with:
/* Fallback to booting NAND if enabled */ if (IS_ENABLED(CONFIG_SPL_NAND_SUPPORT)) return BOOT_DEVICE_NAND;
panic("Could not determine boot source\n"); return -1; /* Never reached */
Note that the check whether to boot from mmc2 rather then mmc0 also really should be moved here (after the nand check) but I can do that myself, since you probably do not have hardware to test this.
Should I mention in commit message, that state of SPL after this patches disable booting from MMC2 until next patch, or should I omit that?
I guess we need to squash the 2 commits together, see above. This has the advantage of making the flow almost the same as the "Boot Diagram" in the A1x / A20 user manuals.
Once we add nand support for the A31 we need to take into account that the boot diagram is different there but lets worry about that later.
Regards,
Hans
participants (2)
-
Daniel Kochmański -
Hans de Goede