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.zhizhi...@leica-geosystems.com] > Sent: 2021年1月20日 4:48 > To: thar...@gateworks.com > Cc: Bough Chen <haibo.c...@nxp.com>; Adam Ford > <aford...@gmail.com>; Fabio Estevam <feste...@gmail.com>; Peng Fan > <peng....@nxp.com>; u-boot <u-boot@lists.denx.de>; Stefano Babic > <sba...@denx.de>; Jaehoon Chung <jh80.ch...@samsung.com> > Subject: RE: IMX8MM SD UHS support > > Hello Tim, > > > -----Original Message----- > > From: Tim Harvey <thar...@gateworks.com> > > Sent: Tuesday, January 19, 2021 6:32 PM > > To: ZHIZHIKIN Andrey <andrey.zhizhi...@leica-geosystems.com> > > Cc: haibo.c...@nxp.com; Adam Ford <aford...@gmail.com>; Fabio > Estevam > > <feste...@gmail.com>; Peng Fan <peng....@nxp.com>; u-boot <u- > > b...@lists.denx.de>; Stefano Babic <sba...@denx.de>; Jaehoon Chung > > <jh80.ch...@samsung.com> > > Subject: Re: IMX8MM SD UHS support > > > > On Mon, Jan 18, 2021 at 11:38 AM ZHIZHIKIN Andrey > > <andrey.zhizhi...@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.c...@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
