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

[Hans de Goede <hdegoede@redhat.com>]

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
>>
>>
>