From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel =?utf-8?Q?Kochma=C5=84ski?= Date: Thu, 28 May 2015 12:05:38 +0200 Subject: [U-Boot] [PATCH] sunxi/spl: Detect at runtime where SPL was read from In-Reply-To: <5566E561.6000100@redhat.com> References: <1432802600-12355-1-git-send-email-dkochmanski@turtle-solutions.eu> <5566E561.6000100@redhat.com> Message-ID: <87k2vtm2z1.fsf@turtle-solutions.eu> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > > 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). 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`? Definition could be in `board/sunxi/board.c` as separate function. If not, where does such helper functions belong? > > >> > >>> + 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 */ > Ack > Regards, > > Hans > > Best regards, 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 */ >>> -- >>> 2.4.1 >>> >>> >> -- Daniel Kochma?ski | Pozna?, Poland ;; aka jackdaniel "Be the change that you wish to see in the world." - Mahatma Gandhi