[U-Boot] mx6cuboxi fails to load u-boot.img

[Baruch Siach <baruch@tkos.co.il>]

Hi Adam,

On Sun, Oct 06 2019, Adam Ford wrote:
> On Sun, Oct 6, 2019 at 5:23 AM Baruch Siach <baruch@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@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@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.

There must be some other way to achieve what you want without breaking
boot when usdhc1 is missing.

>   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@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@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------------{=
   - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -