[PATCH] mmc: host: sdhci-esdhc-imx: reset usdhc before sending tuning command for manual tuning method

[PATCH] mmc: host: sdhci-esdhc-imx: reset usdhc before sending tuning command for manual tuning method

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

According to IC suggestion, everytime before sending the tuning command,
need to reset the usdhc, so to reset the tuning circuit, to let every
tuning command work well for the manual tuning method. For standard
tuning method, IC already add the reset operation in the logic.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index a76b4513fbec..e4694eb1b914 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -990,6 +990,7 @@ static void esdhc_prepare_tuning(struct sdhci_host *host, u32 val)
 
 	/* FIXME: delay a bit for card to be ready for next tuning due to errors */
 	mdelay(1);
+	sdhci_reset(host, SDHCI_RESET_ALL);
 
 	reg = readl(host->ioaddr + ESDHC_MIX_CTRL);
 	reg |= ESDHC_MIX_CTRL_EXE_TUNE | ESDHC_MIX_CTRL_SMPCLK_SEL |
-- 
2.17.1

Re: [PATCH] mmc: host: sdhci-esdhc-imx: reset usdhc before sending tuning command for manual tuning method

[Adrian Hunter]

On 6/08/20 2:48 pm, haibo.chen@nxp.com wrote:
> From: Haibo Chen <haibo.chen@nxp.com>
> 
> According to IC suggestion, everytime before sending the tuning command,
> need to reset the usdhc, so to reset the tuning circuit, to let every
> tuning command work well for the manual tuning method. For standard
> tuning method, IC already add the reset operation in the logic.
> 
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index a76b4513fbec..e4694eb1b914 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -990,6 +990,7 @@ static void esdhc_prepare_tuning(struct sdhci_host *host, u32 val)
>  
>  	/* FIXME: delay a bit for card to be ready for next tuning due to errors */
>  	mdelay(1);
> +	sdhci_reset(host, SDHCI_RESET_ALL);

Doesn't that reset all registers i.e. the entire ios state?

>  
>  	reg = readl(host->ioaddr + ESDHC_MIX_CTRL);
>  	reg |= ESDHC_MIX_CTRL_EXE_TUNE | ESDHC_MIX_CTRL_SMPCLK_SEL |
> 

RE: [PATCH] mmc: host: sdhci-esdhc-imx: reset usdhc before sending tuning command for manual tuning method

[Bough Chen]

> -----Original Message-----
> From: Adrian Hunter [mailto:adrian.hunter@intel.com]
> Sent: 2020年8月6日 20:11
> To: Bough Chen <haibo.chen@nxp.com>; ulf.hansson@linaro.org;
> linux-mmc@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com
> Subject: Re: [PATCH] mmc: host: sdhci-esdhc-imx: reset usdhc before sending
> tuning command for manual tuning method
> 
> On 6/08/20 2:48 pm, haibo.chen@nxp.com wrote:
> > From: Haibo Chen <haibo.chen@nxp.com>
> >
> > According to IC suggestion, everytime before sending the tuning
> > command, need to reset the usdhc, so to reset the tuning circuit, to
> > let every tuning command work well for the manual tuning method. For
> > standard tuning method, IC already add the reset operation in the logic.
> >
> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > ---
> >  drivers/mmc/host/sdhci-esdhc-imx.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> > b/drivers/mmc/host/sdhci-esdhc-imx.c
> > index a76b4513fbec..e4694eb1b914 100644
> > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > @@ -990,6 +990,7 @@ static void esdhc_prepare_tuning(struct sdhci_host
> > *host, u32 val)
> >
> >  	/* FIXME: delay a bit for card to be ready for next tuning due to errors */
> >  	mdelay(1);
> > +	sdhci_reset(host, SDHCI_RESET_ALL);
> 
> Doesn't that reset all registers i.e. the entire ios state?
> 

Hi Adrian,
For i.MX usdhc, RESET_ALL operation do not impact any register value, it just reset the internal logic setting. I already verified that.
Here I just want to set the SDHCI_RESET_ALL bit, but I notice that the API sdhci_reset() will also touch the host->clock, so I'm not sure whether it is good enough to use this API directly. Any suggestion? At least, current patch can fix the manual tuning issue on imx7d/imx8qxp.

> >
> >  	reg = readl(host->ioaddr + ESDHC_MIX_CTRL);
> >  	reg |= ESDHC_MIX_CTRL_EXE_TUNE | ESDHC_MIX_CTRL_SMPCLK_SEL |
> >

Re: [PATCH] mmc: host: sdhci-esdhc-imx: reset usdhc before sending tuning command for manual tuning method

[Adrian Hunter]

On 7/08/20 4:50 am, Bough Chen wrote:
> 
>> -----Original Message-----
>> From: Adrian Hunter [mailto:adrian.hunter@intel.com]
>> Sent: 2020年8月6日 20:11
>> To: Bough Chen <haibo.chen@nxp.com>; ulf.hansson@linaro.org;
>> linux-mmc@vger.kernel.org
>> Cc: dl-linux-imx <linux-imx@nxp.com>; shawnguo@kernel.org;
>> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com
>> Subject: Re: [PATCH] mmc: host: sdhci-esdhc-imx: reset usdhc before sending
>> tuning command for manual tuning method
>>
>> On 6/08/20 2:48 pm, haibo.chen@nxp.com wrote:
>>> From: Haibo Chen <haibo.chen@nxp.com>
>>>
>>> According to IC suggestion, everytime before sending the tuning
>>> command, need to reset the usdhc, so to reset the tuning circuit, to
>>> let every tuning command work well for the manual tuning method. For
>>> standard tuning method, IC already add the reset operation in the logic.
>>>
>>> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
>>> ---
>>>  drivers/mmc/host/sdhci-esdhc-imx.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
>>> b/drivers/mmc/host/sdhci-esdhc-imx.c
>>> index a76b4513fbec..e4694eb1b914 100644
>>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
>>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
>>> @@ -990,6 +990,7 @@ static void esdhc_prepare_tuning(struct sdhci_host
>>> *host, u32 val)
>>>
>>>  	/* FIXME: delay a bit for card to be ready for next tuning due to errors */
>>>  	mdelay(1);
>>> +	sdhci_reset(host, SDHCI_RESET_ALL);
>>
>> Doesn't that reset all registers i.e. the entire ios state?
>>
> 
> Hi Adrian,
> For i.MX usdhc, RESET_ALL operation do not impact any register value, it just reset the internal logic setting. I already verified that.
> Here I just want to set the SDHCI_RESET_ALL bit, but I notice that the API sdhci_reset() will also touch the host->clock, so I'm not sure whether it is good enough to use this API directly. Any suggestion? At least, current patch can fix the manual tuning issue on imx7d/imx8qxp.

It might be safer to write SDHCI_RESET_ALL directly because, for your case,
it does not have exactly the same semantics as sdhci_reset().

It is up to you, but either way, it would be worth adding a comment.

> 
>>>
>>>  	reg = readl(host->ioaddr + ESDHC_MIX_CTRL);
>>>  	reg |= ESDHC_MIX_CTRL_EXE_TUNE | ESDHC_MIX_CTRL_SMPCLK_SEL |
>>>
> 

RE: [PATCH] mmc: host: sdhci-esdhc-imx: reset usdhc before sending tuning command for manual tuning method

[Bough Chen]

> -----Original Message-----
> From: Adrian Hunter [mailto:adrian.hunter@intel.com]
> Sent: 2020年8月10日 19:26
> To: Bough Chen <haibo.chen@nxp.com>; ulf.hansson@linaro.org;
> linux-mmc@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com
> Subject: Re: [PATCH] mmc: host: sdhci-esdhc-imx: reset usdhc before sending
> tuning command for manual tuning method
> 
> On 7/08/20 4:50 am, Bough Chen wrote:
> >
> >> -----Original Message-----
> >> From: Adrian Hunter [mailto:adrian.hunter@intel.com]
> >> Sent: 2020年8月6日 20:11
> >> To: Bough Chen <haibo.chen@nxp.com>; ulf.hansson@linaro.org;
> >> linux-mmc@vger.kernel.org
> >> Cc: dl-linux-imx <linux-imx@nxp.com>; shawnguo@kernel.org;
> >> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com
> >> Subject: Re: [PATCH] mmc: host: sdhci-esdhc-imx: reset usdhc before
> >> sending tuning command for manual tuning method
> >>
> >> On 6/08/20 2:48 pm, haibo.chen@nxp.com wrote:
> >>> From: Haibo Chen <haibo.chen@nxp.com>
> >>>
> >>> According to IC suggestion, everytime before sending the tuning
> >>> command, need to reset the usdhc, so to reset the tuning circuit, to
> >>> let every tuning command work well for the manual tuning method. For
> >>> standard tuning method, IC already add the reset operation in the logic.
> >>>
> >>> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> >>> ---
> >>>  drivers/mmc/host/sdhci-esdhc-imx.c | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> >>> b/drivers/mmc/host/sdhci-esdhc-imx.c
> >>> index a76b4513fbec..e4694eb1b914 100644
> >>> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> >>> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> >>> @@ -990,6 +990,7 @@ static void esdhc_prepare_tuning(struct
> >>> sdhci_host *host, u32 val)
> >>>
> >>>  	/* FIXME: delay a bit for card to be ready for next tuning due to errors
> */
> >>>  	mdelay(1);
> >>> +	sdhci_reset(host, SDHCI_RESET_ALL);
> >>
> >> Doesn't that reset all registers i.e. the entire ios state?
> >>
> >
> > Hi Adrian,
> > For i.MX usdhc, RESET_ALL operation do not impact any register value, it just
> reset the internal logic setting. I already verified that.
> > Here I just want to set the SDHCI_RESET_ALL bit, but I notice that the API
> sdhci_reset() will also touch the host->clock, so I'm not sure whether it is good
> enough to use this API directly. Any suggestion? At least, current patch can fix
> the manual tuning issue on imx7d/imx8qxp.
> 
> It might be safer to write SDHCI_RESET_ALL directly because, for your case, it
> does not have exactly the same semantics as sdhci_reset().
> 
> It is up to you, but either way, it would be worth adding a comment.
> 

Thanks Adrian, I will write this bit directly and add a comment in the next V2 patch.

Best Regards
Haibo Chen
> >
> >>>
> >>>  	reg = readl(host->ioaddr + ESDHC_MIX_CTRL);
> >>>  	reg |= ESDHC_MIX_CTRL_EXE_TUNE |
> ESDHC_MIX_CTRL_SMPCLK_SEL |
> >>>
> >