IMX8MM SD UHS support

[Adam Ford]

On Tue, Jan 19, 2021 at 2:47 PM ZHIZHIKIN Andrey
<andrey.zhizhikin at leica-geosystems.com> wrote:
>
> Hello Tim,
>
> > -----Original Message-----
> > From: Tim Harvey <tharvey at gateworks.com>
> > Sent: Tuesday, January 19, 2021 6:32 PM
> > To: ZHIZHIKIN Andrey <andrey.zhizhikin at leica-geosystems.com>
> > Cc: haibo.chen at nxp.com; Adam Ford <aford173 at gmail.com>; Fabio Estevam
> > <festevam at gmail.com>; Peng Fan <Peng.Fan at nxp.com>; u-boot <u-
> > boot at lists.denx.de>; Stefano Babic <sbabic at denx.de>; Jaehoon Chung
> > <jh80.chung at samsung.com>
> > Subject: Re: IMX8MM SD UHS support
> >
> > On Mon, Jan 18, 2021 at 11:38 AM ZHIZHIKIN Andrey
> > <andrey.zhizhikin at leica-geosystems.com> wrote:
> > >
> > > Hello Tim,
> > >
> > > Sorry it took me quite some time to get this sorted out, but I believe I was able
> > to identify an offending commit that is preventing the USDHC to switch to higher
> > speed modes.
> > >
> > > It is in fact b5874b552f ("mmc: fsl_esdhc_imx: add wait_dat0() support"),
> > reverting it makes SD Card to properly report capabilities and switch to high
> > speed modes.
> > >
> > > Can you try to revert this on your end to see if the SD Card would start to
> > operate in high speed mode?
> > >
> > > I'm still investigating why this addition of wait_dat0() caused this, I believe this
> > is due to the fact that the same wait is already performed while voltage switch to
> > handle the errata, thus this addition wait might erroneously timeout.
> > >
> > > ++ Haibo Chen <haibo.chen at nxp.com>
> > >
> > > Haibo,
> > >
> > > Can you please explain the purpose of adding wait_dat0() Introduced with
> > commit b5874b552f? It is not clear from the commit message what was the
> > purpose of adding it. Have you tested the USDHC switch to higher modes with
> > this change?
> >
> > Andrey,
> >
> > Reverting b5874b552f ("mmc: fsl_esdhc_imx: add wait_dat0() support")
> > does not resolve the issue. That function waits for a specified
> > timeout for the card to assert DAT[0] either high or low depending on
> > arg and is used to check for command busy or failure. The patch in
> > question adds that function so that the common mmc code can utilize
> > it. If the function does not exist in the host controller driver any
> > call to mmc_wait_dat0 returns -ENOSYS so it must be there to support
> > UHS anyway.
>
> Perhaps, this is due to the fact that the same wait cycle is already
> executed during the invocation of mmc_send_cmd above the
> mmc_wait_dat0() in drivers/mmc/mmc.c.
>
> mmc_send_cmd calls esdhc_send_cmd_common in
> drivers/mmc/fsl_esdhc_imx.c, which already has a wait loop
> implemented to cover the "Workaround for ESDHC errata ENGcm03648"
> (as seen from the comment), which might explain why consecutive
> wait fails to return within timeout value.
>
> >
> > What is not clear to me is why the card would hold DAT[0] low on the
> > voltage switch indicating a failure as it does not in the Linux driver
> > which appears to me to be doing the same thing.
>
> This behavior might be explained by the implementation I traced above.
>
> > Again, if we ignore the return code UHS works fine and I'm not at all clear why you
> > wouldn't run into this issue:
> > diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> > index a6394bc..5ea3cd2 100644
> > --- a/drivers/mmc/mmc.c
> > +++ b/drivers/mmc/mmc.c
> > @@ -579,10 +579,12 @@ static int mmc_switch_voltage(struct mmc *mmc,
> > int signal_voltage)
> >          * dat[0:3] low. Wait for at least 1 ms according to spec
> >          */
> >         err = mmc_wait_dat0(mmc, 1, 1000);
> > +#if 0 // hack: not clear why card always holds DAT[0] high here on
> > fsl_esdhc_imx
> >         if (err == -ENOSYS)
> >                 udelay(1000);
> >         else if (err)
> >                 return -ETIMEDOUT;
> > +#endif
> >
> >         return 0;
> >  }
>
> This is expected. When the wait_dat0 callback is undefined in dm_mmc_ops
> structure - it defaults to return -ENOSYS, which effectively just inhibit a delay
> in mmc_switch_voltage and returns 0 indicating that call was successful. This
> all can be seen from the code snippet you've posted above and had commented
> out under #if0 block.
>
> Looking at the change you've posted, it seems to me that you haven't attempted
> to revert the patch I mentioned, as by reverting it - the "if (err == -ENOSYS)"
> path would've been taken and the desired return 0; would've been called.
>
> >
> > Honestly I don't have the time to keep delving into this. I don't have
> > any reason to believe that UHS has ever worked with fsl_esdhc_imx and
> > because my boards boot from eMMC (and HS200/HS400 works) I'm not
> > missing out on much by having a slow microSD.
>
> I still believe (and witnessed it myself) that the original implementation was
> indeed capable of successfully switching the USDHC to high speed modes.
>
> At this point in time it might not be relevant for your implementation,
> but could help those who has a severe impact from the microSD RW
> performance in U-Boot.
>
> Anyways, thanks a lot for writing back on the findings you have!
>
> As for me, it would still be beneficial if the patch author (Haibo) could
> comment on the intention of its introduction, because I clearly see
> that reverting it on the current master branch does improve the behavior.

I am planning on testing the revert and some other stuff related to
this issue, but I probably won't be able to get to it until tomorrow.

adam

>
> >
> > Best regards,
> >
> > Tim
>
> Cheers,
> Andrey