[U-Boot] [PATCH] sunxi/spl: Detect at runtime where SPL was read from
Daniel Kochmański
dkochmanski at turtle-solutions.eu
Thu May 28 12:05:38 CEST 2015
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 at turtle-solutions.eu>
>
> Daniel, thanks for working on this. I've added my comments to Roy's
> comments.
>
>>> CC: Roy Spliet <r.spliet at ultimaker.com>
>>> Cc: Ian Campbell <ijc at hellion.org.uk>
>>> Cc: Hans De Goede <hdegoede at 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
More information about the U-Boot
mailing list