sunxi: Create helper function veryfing valid boot signature on MMC

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 <[email protected]>

CC Roy Spliet <[email protected]>

Cc Ian Campbell <[email protected]>

Cc Hans De Goede <[email protected]>

---
  arch/arm/cpu/armv7/sunxi/board.c | 46 +++++++++++++++++++++++-----------------
  include/configs/sunxi-common.h   |  2 --
  2 files changed, 26 insertions(+), 22 deletions(-)

arch/arm/cpu/armv7/sunxi/board.c +4 -4

index 70f413f..b98fb51 100644
@@ -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.
  	 */

arch/arm/cpu/armv7/sunxi/board.c +7 -7

  	/*
-	 * 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.

arch/arm/cpu/armv7/sunxi/board.c +0 -0

  #endif

  }

include/configs/sunxi-common.h +0 -2

index cce0441..e4db777 100644
@@ -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,

Quoted 11 lines · Daniel Kochma?ski

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.

Quoted 52 lines

Signed-off-by Daniel Kochma?ski <[email protected]>

CC Roy Spliet <[email protected]>

Cc Ian Campbell <[email protected]>

Cc Hans De Goede <[email protected]>

---
  arch/arm/cpu/armv7/sunxi/board.c | 46 +++++++++++++++++++++++-----------------
  include/configs/sunxi-common.h   |  2 --
  2 files changed, 26 insertions(+), 22 deletions(-)

arch/arm/cpu/armv7/sunxi/board.c +4 -7

index 70f413f..b98fb51 100644
@@ -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

arch/arm/cpu/armv7/sunxi/board.c u32 spl_boot_device(void) · line 129

Quoted 6 lines

arch/arm/cpu/armv7/sunxi/board.c +2 -3

-	 * was loaded from nand flash.
-	 */
-	return BOOT_DEVICE_NAND;
  #else
+	__maybe_unused struct mmc *mmc0;
+

No need for maybe unused now.

Quoted 23 lines

arch/arm/cpu/armv7/sunxi/board.c +7 -7

  	/*
-	 * 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.

arch/arm/cpu/armv7/sunxi/board.c u32 spl_boot_device(void) · line 151

Quoted 10 lines

arch/arm/cpu/armv7/sunxi/board.c +5 -3

  	 */
-	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.

arch/arm/cpu/armv7/sunxi/board.c u32 spl_boot_device(void) · line 154

Quoted 5 lines

arch/arm/cpu/armv7/sunxi/board.c +3 -0

+
+	/* 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.

arch/arm/cpu/armv7/sunxi/board.c u32 spl_boot_device(void)

Quoted 7 lines

arch/arm/cpu/armv7/sunxi/board.c +5 -0

+	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

Quoted 22 lines

arch/arm/cpu/armv7/sunxi/board.c +0 -0

  #endif

  }

include/configs/sunxi-common.h +0 -2

index cce0441..e4db777 100644
@@ -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,

Quoted 70 lines · Hans de Goede

Hi,

Quoted 11 lines · Daniel Kochma?ski

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.

Quoted 52 lines

Signed-off-by Daniel Kochma?ski <[email protected]>

CC Roy Spliet <[email protected]>

Cc Ian Campbell <[email protected]>

Cc Hans De Goede <[email protected]>

---
   arch/arm/cpu/armv7/sunxi/board.c | 46 +++++++++++++++++++++++-----------------
   include/configs/sunxi-common.h   |  2 --
   2 files changed, 26 insertions(+), 22 deletions(-)

arch/arm/cpu/armv7/sunxi/board.c +4 -7

index 70f413f..b98fb51 100644
@@ -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

Quoted 8 lines

arch/arm/cpu/armv7/sunxi/board.c u32 spl_boot_device(void) · line 129

Quoted 6 lines

arch/arm/cpu/armv7/sunxi/board.c +2 -3

-	 * was loaded from nand flash.
-	 */
-	return BOOT_DEVICE_NAND;
   #else
+	__maybe_unused struct mmc *mmc0;
+

No need for maybe unused now.

Ack

Quoted 39 lines

Quoted 23 lines

arch/arm/cpu/armv7/sunxi/board.c +7 -7

   	/*
-	 * 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.

arch/arm/cpu/armv7/sunxi/board.c u32 spl_boot_device(void) · line 151

Quoted 10 lines

arch/arm/cpu/armv7/sunxi/board.c +5 -3

   	 */
-	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.

Quoted 3 lines

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.

Quoted 11 lines

arch/arm/cpu/armv7/sunxi/board.c u32 spl_boot_device(void) · line 154

Quoted 5 lines

arch/arm/cpu/armv7/sunxi/board.c +3 -0

+
+	/* 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?

Quoted 5 lines

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.

Quoted 9 lines

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

Quoted 18 lines

arch/arm/cpu/armv7/sunxi/board.c u32 spl_boot_device(void)

Quoted 7 lines

arch/arm/cpu/armv7/sunxi/board.c +5 -0

+	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 */

Quoted 3 lines

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?