[U-Boot] [PATCH] sunxi/spl: Detect at runtime where SPL was read from

[U-Boot] [PATCH] sunxi/spl: Detect at runtime where SPL was read from

[Daniel Kochmański]

This patch makes 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 `CONFIG_SPL_NAND_SUPPORT` is defined. Unless
SD card contains valid signature we assume, that SPL was read from NAND.

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 | 53 ++++++++++++++++++++++++++--------------
 include/configs/sunxi-common.h   |  2 --
 2 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c
index 70f413f..5e441ba 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
@@ -109,12 +110,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 +123,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;
+	__maybe_unused char buf[512];
 	/*
-	 * 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,13 +137,35 @@ 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 branch is just making a decision at runtime whether to load the
+	 * main u-boot binary from the SD card or NAND memory (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. If signature is present,
+	 * decision where to boot from (SD card or NAND memory) depends on
+	 * compile options (if SPL_NAND_SUPPORT isn't defined, we assume we boot
+	 * from SD card), and runtime check - if SD card doesn't contain valid
+	 * signature we assume that SPL was loaded from NAND.
 	 */
-	if (readl(4) == 0x4E4F4765 && readl(8) == 0x3054422E) /* eGON.BT0 */
+	if (readl(4) == 0x4E4F4765 && readl(8) == 0x3054422E) { /* eGON.BT0 */
+#if !defined(CONFIG_SPL_NAND_SUPPORT)
 		return BOOT_DEVICE_MMC1;
+#else
+		mmc_initialize(gd->bd);
+		mmc0 = find_mmc_device(0);
+		/*
+		 * Figure out where we're booting from. Try mmc0 first, just
+		 * like the brom does. If it doesn't contain valid signature,
+		 * assume SPL was loaded from NAND memory.
+		 */
+		if (mmc0 && 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 BOOT_DEVICE_MMC1;
+		}
+		return BOOT_DEVICE_NAND;
+#endif
+	}
 	else
 		return BOOT_DEVICE_BOARD;
 #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 */
-- 
2.4.1

[U-Boot] [PATCH] sunxi/spl: Detect at runtime where SPL was read from

[Roy Spliet]

Hey Daniel,

The approach seems good. Some comments inline

2015-05-28 10:43 GMT+02:00 Daniel Kochma?ski <
dkochmanski@turtle-solutions.eu>:

> This patch makes 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 `CONFIG_SPL_NAND_SUPPORT` is defined. Unless
> SD card contains valid signature we assume, that SPL was read from NAND.
>
> 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 | 53
> ++++++++++++++++++++++++++--------------
>  include/configs/sunxi-common.h   |  2 --
>  2 files changed, 35 insertions(+), 20 deletions(-)
>
> diff --git a/arch/arm/cpu/armv7/sunxi/board.c
> b/arch/arm/cpu/armv7/sunxi/board.c
> index 70f413f..5e441ba 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
> @@ -109,12 +110,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 +123,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;
> +       __maybe_unused char buf[512];
>
?Maybe_unused? Either reserve it or don't. If you only need them for
??CONFIG_SPL_NAND_SUPPORT, then ifdef around it. Also, why are you
reserving half a K if you are only going to use 12 bytes? And if you need
to round up, can't you just malloc it from SDRAM instead of reserving this
space in SRAM like this?

>         /*
> -        * 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,13 +137,35 @@ 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 branch is just making a decision at runtime whether to
> load the
> +        * main u-boot binary from the SD card or NAND memory (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. If signature is
> present,
> +        * decision where to boot from (SD card or NAND memory) depends on
> +        * compile options (if SPL_NAND_SUPPORT isn't defined, we assume
> we boot
> +        * from SD card), and runtime check - if SD card doesn't contain
> valid
> +        * signature we assume that SPL was loaded from NAND.
>          */
> -       if (readl(4) == 0x4E4F4765 && readl(8) == 0x3054422E) /* eGON.BT0
> */
> +       if (readl(4) == 0x4E4F4765 && readl(8) == 0x3054422E) { /*
> eGON.BT0 */
> +#if !defined(
> ??
> CONFIG_SPL_NAND_SUPPORT)
>                 return BOOT_DEVICE_MMC1;
> +#else
> +               mmc_initialize(gd->bd);
> +
> ??
> mmc0 = find_mmc_device(0);
> ?
>
?Is this not supposed to be ?mmc0 =
find_mmc_device(CONFIG_SYS_MMC_ENV_DEV?);
 ? I'm not entirely sure, Hans, how should this constant be interpreted?

> ?
>
> +               /*
> +                * Figure out where we're booting from. Try mmc0 first,
> just
> +                * like the brom does. If it doesn't contain valid
> signature,
> +                * assume SPL was loaded from NAND memory.
> +                */
> +               if (mmc0 && mmc_getcd(mmc0) && mmc_init(mmc0) == 0 &&
> +                   mmc0->block_dev.block_read(0, 16, 1, buf) == 1) {
> +                       buf[12] = 0;
>
?String terminator? I'd just use strncmp or try the same comparisons as
above.?

> +                       if (strcmp(&buf[4], "eGON.BT0") == 0)
> +                               return BOOT_DEVICE_MMC1;
> +               }
> +               return BOOT_DEVICE_NAND;
> +#endif
> +       }
>         else
>                 return BOOT_DEVICE_BOARD;
>  #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 */
> --
> 2.4.1
>
>

-- 


IMAGINE IT >> MAKE IT

Meet us online at Twitter <http://twitter.com/ultimaker>, Facebook 
<http://facebook.com/ultimaker>, Google+ <http://google.com/+Ultimaker>

www.ultimaker.com

[U-Boot] [PATCH] sunxi/spl: Detect at runtime where SPL was read from

[Hans de Goede]

Hi,

On 28-05-15 11:08, Roy Spliet wrote:
> Hey Daniel,
>
> The approach seems good. Some comments inline
>
> 2015-05-28 10:43 GMT+02:00 Daniel Kochma?ski <
> dkochmanski at turtle-solutions.eu>:
>
>> This patch makes 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 `CONFIG_SPL_NAND_SUPPORT` is defined. Unless
>> SD card contains valid signature we assume, that SPL was read from NAND.
>>
>> Signed-off-by: Daniel Kochma?ski <dkochmanski@turtle-solutions.eu>

Daniel, thanks for working on this. I've added my comments to Roy's
comments.

>> 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 | 53
>> ++++++++++++++++++++++++++--------------
>>   include/configs/sunxi-common.h   |  2 --
>>   2 files changed, 35 insertions(+), 20 deletions(-)
>>
>> diff --git a/arch/arm/cpu/armv7/sunxi/board.c
>> b/arch/arm/cpu/armv7/sunxi/board.c
>> index 70f413f..5e441ba 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
>> @@ -109,12 +110,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 +123,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;
>> +       __maybe_unused char buf[512];
>>
> ?Maybe_unused? Either reserve it or don't.  If you only need them for
> ??CONFIG_SPL_NAND_SUPPORT, then ifdef around it.

No, __maybe_unused is appropriate here in u-boot (and AFAIK the kernel too)
this is preferred over sprinkling #ifdef's everywhere.

> Also, why are you
> reserving half a K if you are only going to use 12 bytes?

Because mmc-s emulate disks and we are reading a sector from the mmc here,
which requires a sector-sized buffer.

> And if you need
> to round up, can't you just malloc it from SDRAM instead of reserving this
> space in SRAM like this?

I think that we're post dram init here, so yes malloc should work, and
given the stack size limitations it is probably a good idea to switch to
using malloc (and panic on malloc failure).

>
>>          /*
>> -        * 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,13 +137,35 @@ 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 branch is just making a decision at runtime whether to
>> load the
>> +        * main u-boot binary from the SD card or NAND memory (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. If signature is
>> present,
>> +        * decision where to boot from (SD card or NAND memory) depends on
>> +        * compile options (if SPL_NAND_SUPPORT isn't defined, we assume
>> we boot
>> +        * from SD card), and runtime check - if SD card doesn't contain
>> valid
>> +        * signature we assume that SPL was loaded from NAND.
>>           */
>> -       if (readl(4) == 0x4E4F4765 && readl(8) == 0x3054422E) /* eGON.BT0
>> */
>> +       if (readl(4) == 0x4E4F4765 && readl(8) == 0x3054422E) { /*
>> eGON.BT0 */
>> +#if !defined(
>> ??
>> CONFIG_SPL_NAND_SUPPORT)
>>                  return BOOT_DEVICE_MMC1;
>> +#else
>> +               mmc_initialize(gd->bd);
>> +
>> ??
>> mmc0 = find_mmc_device(0);
>> ?
>>
> ?Is this not supposed to be ?mmc0 =
> find_mmc_device(CONFIG_SYS_MMC_ENV_DEV?);
>   ? I'm not entirely sure, Hans, how should this constant be interpreted?

We really want mmc0 here since that is what the RROM uses so 0 is correct.

>> ?
>>
>> +               /*
>> +                * Figure out where we're booting from. Try mmc0 first,
>> just
>> +                * like the brom does. If it doesn't contain valid
>> signature,
>> +                * assume SPL was loaded from NAND memory.
>> +                */
>> +               if (mmc0 && mmc_getcd(mmc0) && mmc_init(mmc0) == 0 &&
>> +                   mmc0->block_dev.block_read(0, 16, 1, buf) == 1) {
>> +                       buf[12] = 0;
>>
> ?String terminator? I'd just use strncmp or try the same comparisons as
> above.?

True that would work too, actually this is code I wrote, which Daniel
has copied from board/sunxi/board.c .

Daniel for the next revision of this patch set please factor the
code to check for a bootable sdcard in mmc0 out of sunxi/board/board.c
into a helper function, and use that helper function in both code paths.

Also please do the factoring out in a separate preparation patch.



>

>> +                       if (strcmp(&buf[4], "eGON.BT0") == 0)
>> +                               return BOOT_DEVICE_MMC1;
>> +               }
>> +               return BOOT_DEVICE_NAND;
>> +#endif
>> +       }
>>          else
>>                  return BOOT_DEVICE_BOARD;

I'm not thrilled about the nesting here, IMHO it would be better to have
something like this:


	/*
          * 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 */
	if (sunxi_mmc0_has_egon_boot_signature())
		return BOOT_DEVICE_MMC1;

	/* Fallback to booting NAND if enabled */
	if (IS_ENABLED(SPL_NAND_SUPPORT))
		return BOOT_DEVICE_NAND;

	panic("Could not determine boot source\n");
	return -1; /* Never reached */

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

[U-Boot] [PATCH] sunxi/spl: Detect at runtime where SPL was read from

[Daniel Kochmański]

Hello,

Hans de Goede writes:

> Hi,
>
> On 28-05-15 11:08, Roy Spliet wrote:
>> Hey Daniel,
>>
>> The approach seems good. Some comments inline
>>
>> 2015-05-28 10:43 GMT+02:00 Daniel Kochma?ski <
>> dkochmanski at turtle-solutions.eu>:
>>
>>> This patch makes 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 `CONFIG_SPL_NAND_SUPPORT` is defined. Unless
>>> SD card contains valid signature we assume, that SPL was read from NAND.
>>>
>>> Signed-off-by: Daniel Kochma?ski <dkochmanski@turtle-solutions.eu>
>
> Daniel, thanks for working on this. I've added my comments to Roy's
> comments.
>
>>> 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 | 53
>>> ++++++++++++++++++++++++++--------------
>>>   include/configs/sunxi-common.h   |  2 --
>>>   2 files changed, 35 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/arch/arm/cpu/armv7/sunxi/board.c
>>> b/arch/arm/cpu/armv7/sunxi/board.c
>>> index 70f413f..5e441ba 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
>>> @@ -109,12 +110,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 +123,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;
>>> +       __maybe_unused char buf[512];
>>>
>> ?Maybe_unused? Either reserve it or don't.  If you only need them for
>> ??CONFIG_SPL_NAND_SUPPORT, then ifdef around it.
>
> No, __maybe_unused is appropriate here in u-boot (and AFAIK the kernel too)
> this is preferred over sprinkling #ifdef's everywhere.
>
>> Also, why are you
>> reserving half a K if you are only going to use 12 bytes?
>
> Because mmc-s emulate disks and we are reading a sector from the mmc here,
> which requires a sector-sized buffer.
>
>> And if you need
>> to round up, can't you just malloc it from SDRAM instead of reserving this
>> space in SRAM like this?
>
> I think that we're post dram init here, so yes malloc should work, and
> given the stack size limitations it is probably a good idea to switch to
> using malloc (and panic on malloc failure).

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,13 +137,35 @@ 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 branch is just making a decision at runtime whether to
>>> load the
>>> +        * main u-boot binary from the SD card or NAND memory (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. If signature is
>>> present,
>>> +        * decision where to boot from (SD card or NAND memory) depends on
>>> +        * compile options (if SPL_NAND_SUPPORT isn't defined, we assume
>>> we boot
>>> +        * from SD card), and runtime check - if SD card doesn't contain
>>> valid
>>> +        * signature we assume that SPL was loaded from NAND.
>>>           */
>>> -       if (readl(4) == 0x4E4F4765 && readl(8) == 0x3054422E) /* eGON.BT0
>>> */
>>> +       if (readl(4) == 0x4E4F4765 && readl(8) == 0x3054422E) { /*
>>> eGON.BT0 */
>>> +#if !defined(
>>> ??
>>> CONFIG_SPL_NAND_SUPPORT)
>>>                  return BOOT_DEVICE_MMC1;
>>> +#else
>>> +               mmc_initialize(gd->bd);
>>> +
>>> ??
>>> mmc0 = find_mmc_device(0);
>>> ?
>>>
>> ?Is this not supposed to be ?mmc0 =
>> find_mmc_device(CONFIG_SYS_MMC_ENV_DEV?);
>>   ? I'm not entirely sure, Hans, how should this constant be interpreted?
>
> We really want mmc0 here since that is what the RROM uses so 0 is correct.
>
>>> ?
>>>
>>> +               /*
>>> +                * Figure out where we're booting from. Try mmc0 first,
>>> just
>>> +                * like the brom does. If it doesn't contain valid
>>> signature,
>>> +                * assume SPL was loaded from NAND memory.
>>> +                */
>>> +               if (mmc0 && mmc_getcd(mmc0) && mmc_init(mmc0) == 0 &&
>>> +                   mmc0->block_dev.block_read(0, 16, 1, buf) == 1) {
>>> +                       buf[12] = 0;
>>>
>> ?String terminator? I'd just use strncmp or try the same comparisons as
>> above.?
>
> True that would work too, actually this is code I wrote, which Daniel
> has copied from board/sunxi/board.c .
>
> Daniel for the next revision of this patch set please factor the
> code to check for a bootable sdcard in mmc0 out of sunxi/board/board.c
> into a helper function, and use that helper function in both code paths.
>
> Also please do the factoring out in a separate preparation patch.
>

Ok - I'm not sure however, where I should put function declaration - in
`arch/arm/include/asm/arch-sunxi/mmc.h`? Definition could be in
`board/sunxi/board.c` as separate function. If not, where does such
helper functions belong?
>
>
>>
>
>>> +                       if (strcmp(&buf[4], "eGON.BT0") == 0)
>>> +                               return BOOT_DEVICE_MMC1;
>>> +               }
>>> +               return BOOT_DEVICE_NAND;
>>> +#endif
>>> +       }
>>>          else
>>>                  return BOOT_DEVICE_BOARD;
>
> I'm not thrilled about the nesting here, IMHO it would be better to have
> something like this:
>
>
> 	/*
>           * 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 */
> 	if (sunxi_mmc0_has_egon_boot_signature())
> 		return BOOT_DEVICE_MMC1;
>
> 	/* Fallback to booting NAND if enabled */
> 	if (IS_ENABLED(SPL_NAND_SUPPORT))
> 		return BOOT_DEVICE_NAND;
>
> 	panic("Could not determine boot source\n");
> 	return -1; /* Never reached */
>

Ack
> Regards,
>
> Hans
>
>
Best regards,
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 */
>>> --
>>> 2.4.1
>>>
>>>
>>

-- 
Daniel Kochma?ski | Pozna?, Poland
;; aka jackdaniel

"Be the change that you wish to see in the world." - Mahatma Gandhi

[U-Boot] [PATCH] sunxi/spl: Detect at runtime where SPL was read from

[Hans de Goede]

Hi,

On 28-05-15 12:05, Daniel Kochma?ski wrote:
> Hello,
>
> Hans de Goede writes:
>
>> Hi,
>>
>> On 28-05-15 11:08, Roy Spliet wrote:
>>> Hey Daniel,
>>>
>>> The approach seems good. Some comments inline
>>>
>>> 2015-05-28 10:43 GMT+02:00 Daniel Kochma?ski <
>>> dkochmanski at turtle-solutions.eu>:
>>>
>>>> This patch makes 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 `CONFIG_SPL_NAND_SUPPORT` is defined. Unless
>>>> SD card contains valid signature we assume, that SPL was read from NAND.
>>>>
>>>> Signed-off-by: Daniel Kochma?ski <dkochmanski@turtle-solutions.eu>
>>
>> Daniel, thanks for working on this. I've added my comments to Roy's
>> comments.
>>
>>>> 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 | 53
>>>> ++++++++++++++++++++++++++--------------
>>>>    include/configs/sunxi-common.h   |  2 --
>>>>    2 files changed, 35 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/arch/arm/cpu/armv7/sunxi/board.c
>>>> b/arch/arm/cpu/armv7/sunxi/board.c
>>>> index 70f413f..5e441ba 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
>>>> @@ -109,12 +110,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 +123,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;
>>>> +       __maybe_unused char buf[512];
>>>>
>>> ?Maybe_unused? Either reserve it or don't.  If you only need them for
>>> ??CONFIG_SPL_NAND_SUPPORT, then ifdef around it.
>>
>> No, __maybe_unused is appropriate here in u-boot (and AFAIK the kernel too)
>> this is preferred over sprinkling #ifdef's everywhere.
>>
>>> Also, why are you
>>> reserving half a K if you are only going to use 12 bytes?
>>
>> Because mmc-s emulate disks and we are reading a sector from the mmc here,
>> which requires a sector-sized buffer.
>>
>>> And if you need
>>> to round up, can't you just malloc it from SDRAM instead of reserving this
>>> space in SRAM like this?
>>
>> I think that we're post dram init here, so yes malloc should work, and
>> given the stack size limitations it is probably a good idea to switch to
>> using malloc (and panic on malloc failure).
>
> 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,13 +137,35 @@ 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 branch is just making a decision at runtime whether to
>>>> load the
>>>> +        * main u-boot binary from the SD card or NAND memory (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. If signature is
>>>> present,
>>>> +        * decision where to boot from (SD card or NAND memory) depends on
>>>> +        * compile options (if SPL_NAND_SUPPORT isn't defined, we assume
>>>> we boot
>>>> +        * from SD card), and runtime check - if SD card doesn't contain
>>>> valid
>>>> +        * signature we assume that SPL was loaded from NAND.
>>>>            */
>>>> -       if (readl(4) == 0x4E4F4765 && readl(8) == 0x3054422E) /* eGON.BT0
>>>> */
>>>> +       if (readl(4) == 0x4E4F4765 && readl(8) == 0x3054422E) { /*
>>>> eGON.BT0 */
>>>> +#if !defined(
>>>> ??
>>>> CONFIG_SPL_NAND_SUPPORT)
>>>>                   return BOOT_DEVICE_MMC1;
>>>> +#else
>>>> +               mmc_initialize(gd->bd);
>>>> +
>>>> ??
>>>> mmc0 = find_mmc_device(0);
>>>> ?
>>>>
>>> ?Is this not supposed to be ?mmc0 =
>>> find_mmc_device(CONFIG_SYS_MMC_ENV_DEV?);
>>>    ? I'm not entirely sure, Hans, how should this constant be interpreted?
>>
>> We really want mmc0 here since that is what the RROM uses so 0 is correct.
>>
>>>> ?
>>>>
>>>> +               /*
>>>> +                * Figure out where we're booting from. Try mmc0 first,
>>>> just
>>>> +                * like the brom does. If it doesn't contain valid
>>>> signature,
>>>> +                * assume SPL was loaded from NAND memory.
>>>> +                */
>>>> +               if (mmc0 && mmc_getcd(mmc0) && mmc_init(mmc0) == 0 &&
>>>> +                   mmc0->block_dev.block_read(0, 16, 1, buf) == 1) {
>>>> +                       buf[12] = 0;
>>>>
>>> ?String terminator? I'd just use strncmp or try the same comparisons as
>>> above.?
>>
>> True that would work too, actually this is code I wrote, which Daniel
>> has copied from board/sunxi/board.c .
>>
>> Daniel for the next revision of this patch set please factor the
>> code to check for a bootable sdcard in mmc0 out of sunxi/board/board.c
>> into a helper function, and use that helper function in both code paths.
>>
>> Also please do the factoring out in a separate preparation patch.
>>
>
> Ok - I'm not sure however, where I should put function declaration - in
> `arch/arm/include/asm/arch-sunxi/mmc.h`?

Yes that is a good place for it.

> Definition could be in
> `board/sunxi/board.c` as separate function. If not, where does such
> helper functions belong?

Putting the function in board/sunxi/board.c is fine.

Regards,

Hans