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

["Daniel Kochmański" <dkochmanski@turtle-solutions.eu>]

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