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

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

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