IMX8MM SD UHS support

[Bough Chen]

Hi Tim and Andrey

I can reproduce this issue on my side, after revert my patch which you point out, SD can work on SDR104 mode.
Till now, I do not know why wait for data0 status will cause this issue. Give me more time, I will try to dig that out.

Again, thanks for report this issue.

Haibo 

> -----Original Message-----
> From: ZHIZHIKIN Andrey [mailto:andrey.zhizhikin at leica-geosystems.com]
> Sent: 2021年1月20日 4:48
> To: tharvey at gateworks.com
> Cc: Bough Chen <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
> 
> 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.
> 
> >
> > Best regards,
> >
> > Tim
> 
> Cheers,
> Andrey