Hi Adam, On Sun, Oct 06 2019, Adam Ford wrote: > On Sun, Oct 6, 2019 at 7:48 AM Baruch Siach <bar...@tkos.co.il> wrote: >> On Sun, Oct 06 2019, Adam Ford wrote: >> > On Sun, Oct 6, 2019 at 7:30 AM Adam Ford <aford...@gmail.com> wrote: >> >> >> >> On Sun, Oct 6, 2019 at 7:22 AM Baruch Siach <bar...@tkos.co.il> wrote: >> >> > >> >> > Hi Adam, >> >> > >> >> > On Sun, Oct 06 2019, Adam Ford wrote: >> >> > > On Sun, Oct 6, 2019 at 5:23 AM Baruch Siach <bar...@tkos.co.il> wrote: >> >> > >> (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. >> >> > >> >> > I think you missed this part. See more below. >> >> > >> >> > >> 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. >> >> > >> >> > But there is another side effect to this change. The code in spl_mmc.c >> >> > uses BOOT_DEVICE_MMC* macros to determine the boot device as I mentioned >> >> > above. These macros have nothing to do with usdhcX sequence >> >> > numbering. When usdhc1 is missing, BOOT_DEVICE_MMC1 refers to usdhc2 >> >> > which happens to be the first probed device, and BOOT_DEVICE_MMC2 >> >> > becomes usdhc3. This code is broken since commit 14d319b185. >> >> > >> >> > spl_boot_device() can not blindly return BOOT_DEVICE_MMC{1,2} without >> >> > knowing which devices are actually available. >> >> >> >> It returns MMC1 or MMC2 based on what loaded SPL. If SPL was loaded >> >> from MMC1, it should return MMC1. If if SPL is loaded from MMC2, it >> >> should return MMC2, not return MMC1. The previous code always assumed >> >> that spl_boot_device() would always return MMC1. Each individual board >> >> would then do a manual check to see what their boot source was, then >> >> pin-mux and configure MMC1 with the clocking and pin mux for whatever >> >> their MMC source is. >> >> >> >> > >> >> > There must be some other way to achieve what you want without breaking >> >> > boot when usdhc1 is missing. >> >> >> >> If your board is not using SPL_OF_CONTROL, maybe we can put an ifdef >> >> before my patch so people who don't use device tree will default the >> >> older style where all MMC/SDHC controllers return MMC1. >> > >> > For boards without SPL_OF_CONTROL, this might help, I don't know if >> > you use OF_CONTROL or not. >> >> I do use SPL_OF_CONTROL. usdhc1 is not enabled in DT because there is no >> SD/eMMC there. > > I guess i am confused as to how it works for you with the patch > reverted. If this function returns MMC1 as the boot source with the > patch reverted, you and everyone else will need to check the register > to see what the boot source is. This is the same register I am > reading in the patch. From there, the MMC data gets loaded based on > the boot source. This is the very thing my patch was trying to > address, but admittedly without automatic support for MMC3 or MMC4, > but my thought process was that those board would be no worse off than > they are now because mmc3 and mmc4 would still return mmc1 as it does > now.
I think I explained the problem clearly enough already. I'll only add this advice: Carefully read spl_mmc_find_device() and spl_mmc_get_device_index() in common/spl/spl_mmc.c. Keep in mind that when 'mmc_dev' is BOOT_DEVICE_MMC1 it refers to the first _PROBED_ SD/eMMC device. That is, the first device that is both enabled in DT and probed successfully, regardless of the value of X in usdhcX. For the rest, see above. Thanks, baruch > I'd like to hear from either the NXP people or the MMC maintainer to > better determine how to utilize the return values of MMC1 or MMC2. > > adam > >> >> > Does this help: >> > >> > diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c >> > index 1f230aca33..27eb8cc0df 100644 >> > --- a/arch/arm/mach-imx/spl.c >> > +++ b/arch/arm/mach-imx/spl.c >> > @@ -24,7 +24,9 @@ u32 spl_boot_device(void) >> > { >> > unsigned int bmode = readl(&src_base->sbmr2); >> > u32 reg = imx6_src_get_boot_mode(); >> > +#ifdef CONFIG_SPL_OF_CONTROL >> > u32 mmc_index = ((reg >> 11) & 0x03); >> > +#endif >> > >> > /* >> > * Check for BMODE if serial downloader is enabled >> > @@ -87,10 +89,13 @@ u32 spl_boot_device(void) >> > case IMX6_BMODE_ESD: >> > case IMX6_BMODE_MMC: >> > case IMX6_BMODE_EMMC: >> > +#ifdef CONFIG_SPL_OF_CONTROL >> > if (mmc_index == 1) >> > return BOOT_DEVICE_MMC2; >> > else >> > +#else >> > return BOOT_DEVICE_MMC1; >> > +#endif >> > /* NAND Flash: 8.5.2, Table 8-10 */ >> > case IMX6_BMODE_NAND_MIN ... IMX6_BMODE_NAND_MAX: >> > return BOOT_DEVICE_NAND; >> >> So this patch can't help either. >> >> baruch >> >> >> > > 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. >> >> > >> >> > This patch deals with another issue that commit 14d319b185 causes. But >> >> > I'm afraid this patch can not fix boot for me, as explained above. >> >> > >> >> > baruch >> >> > >> >> > > I am hoping someone from NXP or the MMC maintainer might having some >> >> > > thoughts on what might be missing (if anything) >> >> > > >> >> > > 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