[U-Boot] [PATCH 2/2] sunxi/spl: Detect at runtime where from SPL was read
Hans de Goede
hdegoede at redhat.com
Fri May 29 15:05:16 CEST 2015
Hi,
On 29-05-15 14:18, Daniel Kochmański wrote:
> Hi,
>
> Hans de Goede writes:
>
>> Hi,
>>
>> On 28-05-15 15:18, Daniel Kochmański wrote:
>>> Make 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 `SPL_NAND_SUPPORT` is enabled. Unless
>>> SD card contains valid signature we assume, that SPL was read from
>>> NAND.
>>>
>>> V2:
>>> - Move signature verification to helper function
>>> - Avoid unnecessary condition nesting
>>
>> Thanks for the v2, unfortunately I've found an issue which
>> needs fixing before I can merge this, see comments inline.
>>
>>>
>>> Signed-off-by: Daniel Kochmański <dkochmanski at turtle-solutions.eu>
>>> 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 | 46 +++++++++++++++++++++++-----------------
>>> include/configs/sunxi-common.h | 2 --
>>> 2 files changed, 26 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c
>>> index 70f413f..b98fb51 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
>>> @@ -22,6 +23,7 @@
>>> #include <asm/arch/gpio.h>
>>> #include <asm/arch/sys_proto.h>
>>> #include <asm/arch/timer.h>
>>> +#include <asm/arch/mmc.h>
>>>
>>> #include <linux/compiler.h>
>>>
>>> @@ -109,12 +111,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 +124,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;
>>> +
>>
>> No need for maybe unused now.
>
> 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,15 +138,25 @@ 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 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 it is found, it determines where SPL was
>>> + * read from.
>>> */
>>> - if (readl(4) == 0x4E4F4765 && readl(8) == 0x3054422E) /* eGON.BT0 */
>>> - return BOOT_DEVICE_MMC1;
>>> - else
>>> + if (readl(4) != 0x4E4F4765 || readl(8) != 0x3054422E) /* eGON.BT0 */
>>> return BOOT_DEVICE_BOARD;
>>> +
>>> + /* If NAND isn't enabled, then we boot either from mmc0 or mmc2. */
>>> + if (!IS_ENABLED(CONFIG_SPL_NAND_SUPPORT))
>>> + return BOOT_DEVICE_MMC1;
>>
>> I would really prefer for this block to be below the mmc check, I do not
>> like the inverted test here, and this also "checks" things in a different
>> order then the BROM does, it would be nice for the code flow in this
>> function to match the flow from the datasheets for the BROM.
>>
>> Yes this means always doing the mmc check, that should be fine if it fails
>> we apparently cannot access the sdcard, and thus cannot load the real
>> u-boot binary / the next stage and things will fail anyways.
>
> But it also means, that if we boot from mmc2 (as pointed out below, I
> have no hardware to test it), then boot will fail. Currently SPL
> assumes, that if boot is performed from MMC1 and there is no signature
> in mmc0, then it was read from mmc2.
Ah good point, ok I think the best way to solve this is to just move
the mmc2 detect code into here (where one could argue it should have
been anyways) in the same commit. Can you please do that? I can test
that it actually works before merging.
So the end result would look something like this:
struct mmc *mmc0, *mmc1;
/*
* 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 */
mmc_initialize(gd->bd);
mmc0 = find_mmc_device(0);
if (sunxi_mmc_has_egon_boot_signature(mmc0))
return BOOT_DEVICE_MMC1;
/* Fallback to booting NAND if enabled */
if (IS_ENABLED(SPL_NAND_SUPPORT))
return BOOT_DEVICE_NAND;
/* Fallback to booting from mmc2 if enabled */
if (CONFIG_MMC_SUNXI_SLOT_EXTRA == 2) {
/*
* spl_mmc.c: spl_mmc_load_image() is hard-coded to use
* find_mmc_device(0), no matter what we return,
* swap mmc0 and mmc2 to make this work.
*/
mmc1 = find_mmc_device(1);
mmc0->block_dev.dev = 1;
mmc1->block_dev.dev = 0;
return BOOT_DEVICE_MMC2;
}
panic("Could not determine boot source\n");
return -1; /* Never reached */
Note untested :)
>>
>>
>>> +
>>> + /* The BROM will try to boot from mmc0 first, so try that first. */
>>> + mmc_initialize(gd->bd);
>>
>> Ugh, I missed this while reviewing the previous version. mmc_initialize
>> will get called a second time from common/spl/spl_mmc.c when we are
>> actually booting from the mmc and doing this twice is undesirable,
>> looking at the code I guess it will work more or less (it leaks memory)
>> but doing so is just wrong.
>
> I'm not sure where it leaks memory? Unless there is no panic in
> `sunxi_mmc_has_egon_boot_signature`, buf is always freed before
> returning result of the check. Do you refer to some other function?
mmc_initalize calls mmc_board_init which calls sunxi_mmc_init which
calls mmc_create which does a calloc(). More over the mmc controller(s)
will get reset twice, the mmc_devices list head gets initialized twice
(removing the references to the mmc controller structs created in the
first call into mmc_initalize), etc. This is just wrong, it all
happens to work by accident, but we should not rely on that.
>>
>> The best way I can come up with to fix this is to protect
>> mmc_initialize() against being called twice, simply returning success
>> immediately on the second call.
>>
>> Please do a v3 with a preparation patch making this change to
>> mmc_initialize(), and Cc the mmc maintainer on the posting of v3
>> of this set. Also please base v3 on:
>>
>> http://git.denx.de/?p=u-boot/u-boot-sunxi.git;a=shortlog;h=refs/heads/next
>>
>> There are some commits there which conclict with the tree you're
>> basing your patches on.
>>
>
> Ack
>>> + mmc0 = find_mmc_device(0);
>>> + if (sunxi_mmc_has_egon_boot_signature(mmc0))
>>> + return BOOT_DEVICE_MMC1;
>>> +
>>
>>
>>> + return BOOT_DEVICE_NAND;
>>
>> And replace this with:
>>
>> /* Fallback to booting NAND if enabled */
>> if (IS_ENABLED(CONFIG_SPL_NAND_SUPPORT))
>> return BOOT_DEVICE_NAND;
>>
>> panic("Could not determine boot source\n");
>> return -1; /* Never reached */
>>
>>
>> Note that the check whether to boot from mmc2 rather then mmc0 also really
>> should be moved here (after the nand check) but I can do that myself, since
>> you probably do not have hardware to test this.
>
> Should I mention in commit message, that state of SPL after this patches
> disable booting from MMC2 until next patch, or should I omit that?
I guess we need to squash the 2 commits together, see above. This has
the advantage of making the flow almost the same as the "Boot Diagram"
in the A1x / A20 user manuals.
Once we add nand support for the A31 we need to take into account that the boot
diagram is different there but lets worry about that later.
Regards,
Hans
More information about the U-Boot
mailing list