From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Date: Thu, 28 May 2015 12:26:08 +0200 Subject: [U-Boot] [PATCH] sunxi/spl: Detect at runtime where SPL was read from In-Reply-To: <87k2vtm2z1.fsf@turtle-solutions.eu> References: <1432802600-12355-1-git-send-email-dkochmanski@turtle-solutions.eu> <5566E561.6000100@redhat.com> <87k2vtm2z1.fsf@turtle-solutions.eu> Message-ID: <5566ED40.10707@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 12:05, Daniel Kochma?ski wrote: > 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`? Yes that is a good place for it. > Definition could be in > `board/sunxi/board.c` as separate function. If not, where does such > helper functions belong? Putting the function in board/sunxi/board.c is fine. Regards, Hans