IMX8MM SD UHS support

Wed Jan 12 00:32:24 CET 2022

On Mon, Jan 25, 2021 at 4:43 AM Bough Chen <haibo.chen at nxp.com> wrote:
>
> Hi Tim and Andrey,
>
> I find the root cause, it is because during the 1.8v voltage switch process, SD clock do not gate off/ on according to the SD spec.
> For i.MX usdhc, if want to gate off the sd card clock, need to clear the bit 8 of VEND_SPEC, and set this bit to gate on the sd card clock.
> For the bit[14:11] of VEND_SPEC, our IP do not implement the function, just mark as reserved bits.
> So for current logic, we'll see the second mmc_wait_dat0() in mmc_switch_voltage() always return timeout.
>
> Let me double confirm with our IC team, and will send a formal fix patch tomorrow.

Haibo,

Sorry to resurrect an old thread, but it's been almost a year since we
last discussed this, and from what I can tell, the MMC switching to
SDR104 is still broken.

I was hoping you might have a suggestion as to how to move forward.

>
>
> Haibo
>
> > -----Original Message-----
> > From: Bough Chen
> > Sent: 2021年1月22日 20:42
> > To: 'ZHIZHIKIN Andrey' <andrey.zhizhikin at leica-geosystems.com>;
> > tharvey at gateworks.com
> > Cc: 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
> >
> > 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;
> > > >  }

I implemented the above hack and I can get the card to operate in
sdr104 mode, but I doubt that would be an acceptable solution.  Does
anyone have any ideas how we might be able to work around this quirk?

thanks,

adam

> > >
> > > 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