On Sun, Oct 6, 2019 at 6:30 AM Adam Ford <aford...@gmail.com> wrote: > > On Sun, Oct 6, 2019 at 5:23 AM Baruch Siach <bar...@tkos.co.il> wrote: > > > > Hi Adam, > > > > (Adding MMC and i.MX maintainers to Cc) > > > > On Fri, Sep 27 2019, Adam Ford wrote: > > > On Fri, Sep 27, 2019 at 4:38 AM Jonathan Gray <jsg at jsg.id.au> wrote: > > >> > > >> On Thu, Sep 26, 2019 at 05:07:21PM -0300, Fabio Estevam wrote: > > >> > Hi Vagrant, > > >> > > > >> > On Thu, Sep 26, 2019 at 4:16 PM Vagrant Cascadian <vagrant at > > >> > debian.org> wrote: > > >> > > > > >> > > I just tested mx6cuboxi with 2019.10-rc4, and it fails to load > > >> > > u-boot.img from MMC: > > >> > > > > >> > > 1 2019-09-26_17:31:27.63089 U-Boot SPL 2019.10-rc4+dfsg-1 (Sep 24 > > >> > > 2019 - > > >> > > 08:03:23 +0000) > > >> > > 2 2019-09-26_17:31:27.63092 Trying to boot from MMC2 > > >> > > 3 2019-09-26_17:31:27.63095 MMC Device 1 not found > > >> > > 4 2019-09-26_17:31:27.63097 spl: could not find mmc device 1. error: > > >> > > -19 > > >> > > 5 2019-09-26_17:31:27.63099 SPL: failed to boot from all boot devices > > >> > > 6 2019-09-26_17:31:27.63101 ### ERROR ### Please RESET the board ### > > >> > > > >> > Thanks for reporting this issue. > > >> > > > >> > Unfortunately, I don't have access to my Cuboxi, so I am adding Jon > > >> > and Baruch on Cc. > > >> > > >> Works after reverting the following commit. > > >> > > > I am going to argue that making the board comply with DM_MMC is why I > > > needed to make the patch, because when booting from MMC2, the function > > > was returning MMC1 which was clearly not the boot source. > > > > > > If the boards that fail accept MMC2 as a response when booting from > > > MMC2, that seems like a bug on the indvidual boards. Instead they > > > should setup their boot sequence to configure MMC2 when MMC2 is the > > > boot source. Instead, it seems like some boards are configuring MMC1 > > > with MMC2 info which only prolongs the conversion to DM_MMC. > > > > > > If we revert the patch, then boards like imx6_logic who rely solely on > > > device tree and DM_MMC for booting will have to manually override the > > > MMC driver in order to boot from MMC2, and that seems like a step > > > backwards. I would argue that this board should migrate to DM_MMC and > > > use the device tree to boot, and the problem should go away. > > > > I started working on migration to DM_MMC as you suggested. Unfortunately > > I can't see how this solves the problem for Cubox-i/Hummingboard, nor in > > the general case. > > > > The imx6_logic board happens to use only usdhc1 and usdhc2 for boot, and > > both are always enabled. This matches perfectly to BOOT_DEVICE_MMC{1,2}, > > and their corresponding DT representation. > > > > However, the 'index' parameter in uclass_get_device() that is set > > according to BOOT_DEVICE_MMC{1,2} selection has nothing to do with the > > usdhcX sequence number. It simply returns the Nth probed SD/eMMC device > > (see uclass_find_device()). In the case of Cubox-i/Hummingboard, usdhc1 > > is never used for boot, usdhc2 is always an SD card, and usdhc3 is an > > optional eMMC. When booting from SD card, uclass_get_device(), returns > > -ENODEV when eMMC is not available, or the eMMC device when it is > > available. In both cases, boot fails. > > > > In addition, your patch returns BOOT_DEVICE_MMC2 only for usdhc2 > > boot. All others return BOOT_DEVICE_MMC1. What about usdhc{3,4}? > > > > My patch only extended it to support MMC1 or MMC2. I don't have > hardware to test MMC3 or MMC4, nor where they defined in the boot > table. > The intention what to eliminate all functions from board files which > did a something like: > > static int mmc_init_spl(bd_t *bis) > { > struct src *psrc = (struct src *)SRC_BASE_ADDR; > unsigned reg = readl(&psrc->sbmr1) >> 11; > > /* > * Upon reading BOOT_CFG register the following map is done: > * Bit 11 and 12 of BOOT_CFG register can determine the current > * mmc port > * 0x1 SD2 > * 0x2 SD3 > */ > switch (reg & 0x3) { > ... > } > } > > > How is all that intended to work? > > Basically the above function determines which BOOT_CFG regiser is used > and returns sets MMC1 values to the returned value. In my case MMC1 > was going to be configured with the clock and pin mux of mmc1 or 2. > In your case, mmc1 gets configured with the information for mmc2 or 3. > Since it appears that arch/arm/mach-imx/spl.c is supposed to be able > to return the correct boot source, my goal was to make that function > actually return that which could eliminate the above function on all > boards. Unfortunately, I don't have hardware with MMC3 or MMC4, so I > couldn't test it and therefore didn't write it into the code. It was > my hope that someone with MMC3 or MMC4 would be able to easily expand > it in the hope to better facilitate support for DM_MMC and device tree > in SPL. > > > > > Aren't other i.MX boards impacted by this commit? > > Yes and no. If they only support MMC1 or MMC2 and have DM_MMC with > device tree support, the theory is that mmc_init_spl(bd_t *bis) > function can be completely eliminated. People with MMC3 and MMC4 as > boot sources are quite possibly impacted, but like I said before, I > was trying to lay the foundation for people to migrate into a > direction to eliminate individual functions and share common files > more easily. > > You can try this: > > diff --git a/arch/arm/include/asm/spl.h b/arch/arm/include/asm/spl.h > index e568af2561..e94a295eda 100644 > --- a/arch/arm/include/asm/spl.h > +++ b/arch/arm/include/asm/spl.h > @@ -18,6 +18,8 @@ enum { > BOOT_DEVICE_MMC1, > BOOT_DEVICE_MMC2, > BOOT_DEVICE_MMC2_2, > + BOOT_DEVICE_MMC3, > + BOOT_DEVICE_MMC4, > BOOT_DEVICE_NAND, > BOOT_DEVICE_ONENAND, > BOOT_DEVICE_NOR, > diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c > index 1f230aca33..bf72d03eee 100644 > --- a/arch/arm/mach-imx/spl.c > +++ b/arch/arm/mach-imx/spl.c > @@ -87,7 +87,11 @@ u32 spl_boot_device(void) > case IMX6_BMODE_ESD: > case IMX6_BMODE_MMC: > case IMX6_BMODE_EMMC: > - if (mmc_index == 1) > + if (mmc_index == 3) > + return BOOT_DEVICE_MMC4; > + else if (mmc_index == 2) > + return BOOT_DEVICE_MMC3; > + else if (mmc_index == 1) > return BOOT_DEVICE_MMC2; > else > return BOOT_DEVICE_MMC1; > > > It's only compile-only tested. > > I am hoping someone from NXP or the MMC maintainer might having some > thoughts on what might be missing (if anything) > > adam > >
I should also note that people who are only booting from MMC1 or MMC2 should be able to do something like was done here: https://patchwork.ozlabs.org/patch/1104361/ Eventually, if boards can fit the pinmux stuff into SPL, that function modified in the above patch should be able to go away. adam > > Thanks, > > baruch > > > > >> 14d319b1856b86e593e01abd0a1e3c2d63b52a8a is the first bad commit > > >> commit 14d319b1856b86e593e01abd0a1e3c2d63b52a8a > > >> Author: Adam Ford <aford173 at gmail.com> > > >> Date: Thu May 23 14:11:30 2019 -0500 > > >> > > >> spl: imx6: Let spl_boot_device return USDHC1 or USDHC2 > > >> > > >> Currently, when the spl_boot_device checks the boot device, it > > >> will only return MMC1 when it's either sd or eMMC regardless > > >> of whether or not it's MMC1 or MMC2. This is a problem when > > >> booting from MMC2 if MMC isn't being manually configured like in > > >> the DM_SPL case with SPL_OF_CONTROL. > > >> > > >> This patch will check the register and return either MMC1 or MMC2. > > >> > > >> Signed-off-by: Adam Ford <aford173 at gmail.com> > > >> > > >> arch/arm/mach-imx/spl.c | 8 +++++--- > > >> 1 file changed, 5 insertions(+), 3 deletions(-) > > > > -- > > http://baruch.siach.name/blog/ ~. .~ Tk Open Systems > > =}------------------------------------------------ooO--U--Ooo------------{= > > - bar...@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il - _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot