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.
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;
> adam
> >
> > > 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
