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

[Daniel Kochmański]

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.
>
>
>> +
>> +	/* 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?
>
> 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?
>
> Regards,
>
> Hans
>

BR,
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 */
>>

-- 
Daniel Kochmański | Poznań, Poland
;; aka jackdaniel

"Be the change that you wish to see in the world." - Mahatma Gandhi