On Mon, Jan 25, 2021 at 4:43 AM Bough Chen <haibo.c...@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.zhizhi...@leica-geosystems.com>; > > thar...@gateworks.com > > Cc: 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 > > > > 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; > > > > } 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
