From mboxrd@z Thu Jan  1 00:00:00 1970
From: Bough Chen <haibo.chen@nxp.com>
Date: Mon, 25 Jan 2021 10:43:42 +0000
Subject: IMX8MM SD UHS support
In-Reply-To: <VI1PR04MB5294B4B9E9B4F5F41CF129DD90A00@VI1PR04MB5294.eurprd04.prod.outlook.com>
References: <CAJ+vNU3FFxrXea-9cgEuE4AFDk6d=SiBqaeA+cTx6kAwW8D9hA@mail.gmail.com>
 <AM6PR06MB4691EC9446305A16D833D79FA6D70@AM6PR06MB4691.eurprd06.prod.outlook.com>
 <CAJ+vNU22jh-O+38ay65yEsf+HqA5kUUjfQz65RYAZ=-ajt5JZA@mail.gmail.com>
 <CAOMZO5D9kipMrxALb_i-M6Q5FekjR9+z558Nk-HnsSGEJ+y99w@mail.gmail.com>
 <CAHCN7x+u4C9AFV3AUR5W=8HGFPVJqM9eaGPvdvLUe3YJ1Xx12Q@mail.gmail.com>
 <CAJ+vNU0HLjLdW0z2dE3YU5sKNYMyKUxN2JivuZmONVhO+t+AVA@mail.gmail.com>
 <AM6PR06MB4691A8CE29586691DE7F177BA6D70@AM6PR06MB4691.eurprd06.prod.outlook.com>
 <CAJ+vNU2br93XbEnih8kAsAJZR4cEQ9aGC+fXLu84L4rU0xNRgw@mail.gmail.com>
 <AM6PR06MB4691C592870E022E6672DFE9A6A40@AM6PR06MB4691.eurprd06.prod.outlook.com>
 <CAJ+vNU3dBQOrBKSqyQUxVKZVGM=0C7WU-_cCc_5qG6rSvDWbQg@mail.gmail.com>
 <AM6PR06MB46912815D884D3196FA3477EA6A30@AM6PR06MB4691.eurprd06.prod.outlook.com>
 <VI1PR04MB5294B4B9E9B4F5F41CF129DD90A00@VI1PR04MB5294.eurprd04.prod.outlook.com>
Message-ID: <VI1PR04MB5294D1B1AD833DD59552E81390BD0@VI1PR04MB5294.eurprd04.prod.outlook.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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   

> -----Original Message-----
> From: Bough Chen
> Sent: 2021?1?22? 20:42
> To: 'ZHIZHIKIN Andrey' <andrey.zhizhikin@leica-geosystems.com>;
> tharvey at gateworks.com
> Cc: Adam Ford <aford173@gmail.com>; Fabio Estevam
> <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>; u-boot
> <u-boot@lists.denx.de>; Stefano Babic <sbabic@denx.de>; Jaehoon Chung
> <jh80.chung@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@nxp.com>; Adam Ford
> <aford173@gmail.com>;
> > Fabio Estevam <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>;
> > u-boot <u-boot@lists.denx.de>; Stefano Babic <sbabic@denx.de>; Jaehoon
> > Chung <jh80.chung@samsung.com>
> > Subject: RE: IMX8MM SD UHS support
> >
> > Hello Tim,
> >
> > > -----Original Message-----
> > > From: Tim Harvey <tharvey@gateworks.com>
> > > Sent: Tuesday, January 19, 2021 6:32 PM
> > > To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>
> > > Cc: haibo.chen at nxp.com; Adam Ford <aford173@gmail.com>; Fabio
> > Estevam
> > > <festevam@gmail.com>; Peng Fan <Peng.Fan@nxp.com>; u-boot <u-
> > > boot at lists.denx.de>; Stefano Babic <sbabic@denx.de>; Jaehoon Chung
> > > <jh80.chung@samsung.com>
> > > Subject: Re: IMX8MM SD UHS support
> > >
> > > On Mon, Jan 18, 2021 at 11:38 AM ZHIZHIKIN Andrey
> > > <andrey.zhizhikin@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@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