[U-Boot] [PATCH] sunxi/spl: Detect at runtime where SPL was read from
Hans de Goede
hdegoede at redhat.com
Thu May 28 12:26:08 CEST 2015
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 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`?
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
More information about the U-Boot
mailing list