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

[Hans de Goede]

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.

>   	/*
> -	 * 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.


> +
> +	/* 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.

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.

> +	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.

Regards,

Hans




>   #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 */
>