IMX8MM SD UHS support

Bough Chen haibo.chen at nxp.com
Fri Jan 14 04:12:37 CET 2022 

> -----Original Message-----
> From: Adam Ford [mailto:aford173 at gmail.com]
> Sent: 2022年1月12日 7:32
> To: Bough Chen <haibo.chen at nxp.com>
> Cc: ZHIZHIKIN Andrey <andrey.zhizhikin at leica-geosystems.com>;
> tharvey at gateworks.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 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.
> 

HI Adam,

The original fix patch is reverted few month ago.
Please refer to commit f132aab403271 " Revert "mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output""
According to my test, I find for some mmc card, we can't keep the clock always on, especially when mmc internal status get stuck, seems must need the clock gate off, then the card can recover back. Otherwise, the card can't response any command. I meet this issue on one eMMC card, after meet switch error, must gate off and gate on the clock rate, then this emmc Card can recover back. I think this would be the reason of why this patched is reverted.
I'm working on this issue these days, will try to send another patch to fix all these issue.

Best Regards
Haibo Chen
> >
> >
> > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 9551 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20220114/f9768440/attachment.bin>

More information about the U-Boot mailing list