From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Date: Thu, 28 May 2015 11:52:33 +0200 Subject: [U-Boot] [PATCH] sunxi/spl: Detect at runtime where SPL was read from In-Reply-To: References: <1432802600-12355-1-git-send-email-dkochmanski@turtle-solutions.eu> Message-ID: <5566E561.6000100@redhat.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 Daniel, thanks for working on this. I've added my comments to Roy's comments. >> CC: Roy Spliet >> Cc: Ian Campbell >> Cc: Hans De Goede >> --- >> 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 >> +#include >> #include >> #include >> #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). > >> /* >> - * 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. > >> + 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 */ 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 */ >> -- >> 2.4.1 >> >> >