[PATCH] Revert "mmc: fsl_esdhc_imx: add wait_dat0() support"

[PATCH] Revert "mmc: fsl_esdhc_imx: add wait_dat0() support"

[Marek Vasut]

This reverts commit b5874b552ffa09bc1dc5dec6b5dd376c62dab45d.

It seems the iMX8MM SDHC controller always reports DAT0 line status
as zero after voltage switch at the end of mmc_switch_voltage(), even
if it is supposed to be high and scope confirms the DAT0 is high.
Reverting this patch makes SDR104 work on iMX8MM, however, it is
not clear why the DAT0 status is not correctly reported by the
controller.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Haibo Chen <haibo.chen@nxp.com>
Cc: Igor Opaniuk <igor.opaniuk@foundries.io>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Stefano Babic <sbabic@denx.de>
---
 drivers/mmc/fsl_esdhc_imx.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
index 9299635f509..5465992ec95 100644
--- a/drivers/mmc/fsl_esdhc_imx.c
+++ b/drivers/mmc/fsl_esdhc_imx.c
@@ -1552,20 +1552,6 @@ static int __maybe_unused fsl_esdhc_set_enhanced_strobe(struct udevice *dev)
 	return 0;
 }
 
-static int fsl_esdhc_wait_dat0(struct udevice *dev, int state,
-				int timeout_us)
-{
-	int ret;
-	u32 tmp;
-	struct fsl_esdhc_priv *priv = dev_get_priv(dev);
-	struct fsl_esdhc *regs = priv->esdhc_regs;
-
-	ret = readx_poll_timeout(esdhc_read32, &regs->prsstat, tmp,
-				!!(tmp & PRSSTAT_DAT0) == !!state,
-				timeout_us);
-	return ret;
-}
-
 static const struct dm_mmc_ops fsl_esdhc_ops = {
 	.get_cd		= fsl_esdhc_get_cd,
 	.send_cmd	= fsl_esdhc_send_cmd,
@@ -1576,7 +1562,6 @@ static const struct dm_mmc_ops fsl_esdhc_ops = {
 #if CONFIG_IS_ENABLED(MMC_HS400_ES_SUPPORT)
 	.set_enhanced_strobe = fsl_esdhc_set_enhanced_strobe,
 #endif
-	.wait_dat0 = fsl_esdhc_wait_dat0,
 };
 
 static struct esdhc_soc_data usdhc_imx7d_data = {
-- 
2.34.1

Re: [PATCH] Revert "mmc: fsl_esdhc_imx: add wait_dat0() support"

[Jaehoon Chung]

Hi Marek,

On 1/28/22 12:40, Marek Vasut wrote:
> This reverts commit b5874b552ffa09bc1dc5dec6b5dd376c62dab45d.
> 
> It seems the iMX8MM SDHC controller always reports DAT0 line status
> as zero after voltage switch at the end of mmc_switch_voltage(), even
> if it is supposed to be high and scope confirms the DAT0 is high.
> Reverting this patch makes SDR104 work on iMX8MM, however, it is
> not clear why the DAT0 status is not correctly reported by the
> controller.

I didn't have an imx8mm board, so I wonder that other boards which is using fsl_esdhci_imx driver have same problem.
I'm not sure...I remember that some controller doesn't support a busy signal.
In kernel driver, also doesn't report correct status?

Best Regards,
Jaehoon Chung

> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Haibo Chen <haibo.chen@nxp.com>
> Cc: Igor Opaniuk <igor.opaniuk@foundries.io>
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Stefano Babic <sbabic@denx.de>
> ---
>  drivers/mmc/fsl_esdhc_imx.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
> index 9299635f509..5465992ec95 100644
> --- a/drivers/mmc/fsl_esdhc_imx.c
> +++ b/drivers/mmc/fsl_esdhc_imx.c
> @@ -1552,20 +1552,6 @@ static int __maybe_unused fsl_esdhc_set_enhanced_strobe(struct udevice *dev)
>  	return 0;
>  }
>  
> -static int fsl_esdhc_wait_dat0(struct udevice *dev, int state,
> -				int timeout_us)
> -{
> -	int ret;
> -	u32 tmp;
> -	struct fsl_esdhc_priv *priv = dev_get_priv(dev);
> -	struct fsl_esdhc *regs = priv->esdhc_regs;
> -
> -	ret = readx_poll_timeout(esdhc_read32, &regs->prsstat, tmp,
> -				!!(tmp & PRSSTAT_DAT0) == !!state,
> -				timeout_us);
> -	return ret;
> -}
> -
>  static const struct dm_mmc_ops fsl_esdhc_ops = {
>  	.get_cd		= fsl_esdhc_get_cd,
>  	.send_cmd	= fsl_esdhc_send_cmd,
> @@ -1576,7 +1562,6 @@ static const struct dm_mmc_ops fsl_esdhc_ops = {
>  #if CONFIG_IS_ENABLED(MMC_HS400_ES_SUPPORT)
>  	.set_enhanced_strobe = fsl_esdhc_set_enhanced_strobe,
>  #endif
> -	.wait_dat0 = fsl_esdhc_wait_dat0,
>  };
>  
>  static struct esdhc_soc_data usdhc_imx7d_data = {

Re: [PATCH] Revert "mmc: fsl_esdhc_imx: add wait_dat0() support"

[Marek Vasut]

On 1/28/22 08:30, Jaehoon Chung wrote:
> Hi Marek,

Hi,

> On 1/28/22 12:40, Marek Vasut wrote:
>> This reverts commit b5874b552ffa09bc1dc5dec6b5dd376c62dab45d.
>>
>> It seems the iMX8MM SDHC controller always reports DAT0 line status
>> as zero after voltage switch at the end of mmc_switch_voltage(), even
>> if it is supposed to be high and scope confirms the DAT0 is high.
>> Reverting this patch makes SDR104 work on iMX8MM, however, it is
>> not clear why the DAT0 status is not correctly reported by the
>> controller.
> 
> I didn't have an imx8mm board, so I wonder that other boards which is using fsl_esdhci_imx driver have same problem.

I have multiple 8mm, just not the 8mm-evk.

> I'm not sure...I remember that some controller doesn't support a busy signal.
> In kernel driver, also doesn't report correct status?

Kernel doesn't use this DAT0 readback, that's why it works in kernel.

U-Boot has been augmented with this DAT0 readback some time ago, I just 
noticed it on an 8mm board recently that it doesn't really work. There 
is also no errata.

I think it would be good to have NXP check this.

Re: [PATCH] Revert "mmc: fsl_esdhc_imx: add wait_dat0() support"

[Fabio Estevam]

Hi Haibo and Peng,

On Fri, Jan 28, 2022 at 2:37 PM Marek Vasut <marex@denx.de> wrote:

> I think it would be good to have NXP check this.

Could you please help to review this?

Thanks

RE: [PATCH] Revert "mmc: fsl_esdhc_imx: add wait_dat0() support"

[Bough Chen]

[-- Attachment #1: Type: text/plain, Size: 3285 bytes --]


> -----Original Message-----
> From: Marek Vasut [mailto:marex@denx.de]
> Sent: 2022年1月29日 1:37
> To: Jaehoon Chung <jh80.chung@samsung.com>; u-boot@lists.denx.de
> Cc: sbabic@denx.de; Bough Chen <haibo.chen@nxp.com>; Igor Opaniuk
> <igor.opaniuk@foundries.io>; Peng Fan <peng.fan@nxp.com>
> Subject: Re: [PATCH] Revert "mmc: fsl_esdhc_imx: add wait_dat0() support"
> 
> On 1/28/22 08:30, Jaehoon Chung wrote:
> > Hi Marek,
> 
> Hi,
> 
> > On 1/28/22 12:40, Marek Vasut wrote:
> >> This reverts commit b5874b552ffa09bc1dc5dec6b5dd376c62dab45d.
> >>
> >> It seems the iMX8MM SDHC controller always reports DAT0 line status
> >> as zero after voltage switch at the end of mmc_switch_voltage(), even
> >> if it is supposed to be high and scope confirms the DAT0 is high.
> >> Reverting this patch makes SDR104 work on iMX8MM, however, it is not
> >> clear why the DAT0 status is not correctly reported by the
> >> controller.
> >
> > I didn't have an imx8mm board, so I wonder that other boards which is using
> fsl_esdhci_imx driver have same problem.
> 
> I have multiple 8mm, just not the 8mm-evk.
> 
> > I'm not sure...I remember that some controller doesn't support a busy signal.
> > In kernel driver, also doesn't report correct status?
> 
> Kernel doesn't use this DAT0 readback, that's why it works in kernel.
> 
> U-Boot has been augmented with this DAT0 readback some time ago, I just
> noticed it on an 8mm board recently that it doesn't really work. There is also no
> errata.
> 
> I think it would be good to have NXP check this.

Hi,

The issue you meet should be related to one patch that was revert recently, refer to 
Commit f132aab40327 " Revert "mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output""

The issue you meet seems the same as what I mentioned in my original commit log, I paste in the end.
Revert this patch because we did find this patch has some side effect, like enlarge the whole boot up time about 20s.
If you will, you can add back this patch on your side, and test it.
I will send one new patch this week, to fix issue and avoid side effect. 

Best Regards
Haibo Chen 


commit 63756575b42b8b4fb3f59cbbf0cedf03331bc2d2
Author: Haibo Chen <haibo.chen@nxp.com>
Date:   Wed Mar 3 17:05:46 2021 +0800

    mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output

    For FSL_USDHC, it do not implement VENDORSPEC_CKEN/PEREN/HCKEN/IPGEN, these
    are reserved bits. Instead, use VENDORSPEC_FRC_SDCLK_ON to gate on/off the
    card clock output.

    After commit b5874b552ffa ("mmc: fsl_esdhc_imx: add wait_dat0() support"),
    we meet SD3.0 card can't work at UHS mode, mmc_switch_voltage() fail because
    the second mmc_wait_dat0 return -ETIMEDOUT. According to SD spec, during
    voltage switch, need to gate off/on the card clock. If not set the FRC_SDCLK_ON,
    after CMD11, hardware will gate off the card clock automatically, so card do
    not detect the clock off/on behavior, so will draw the data0 line low until
    next command.

    Fixes: b5874b552ffa ("mmc: fsl_esdhc_imx: add wait_dat0() support")
    Tested-by: Tim Harvey <tharvey@gateworks.com>
    Signed-off-by: Haibo Chen <haibo.chen@nxp.com>



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 9551 bytes --]

Re: [PATCH] Revert "mmc: fsl_esdhc_imx: add wait_dat0() support"

[Marek Vasut]

On 2/7/22 08:59, Bough Chen wrote:
> 
>> -----Original Message-----
>> From: Marek Vasut [mailto:marex@denx.de]
>> Sent: 2022年1月29日 1:37
>> To: Jaehoon Chung <jh80.chung@samsung.com>; u-boot@lists.denx.de
>> Cc: sbabic@denx.de; Bough Chen <haibo.chen@nxp.com>; Igor Opaniuk
>> <igor.opaniuk@foundries.io>; Peng Fan <peng.fan@nxp.com>
>> Subject: Re: [PATCH] Revert "mmc: fsl_esdhc_imx: add wait_dat0() support"
>>
>> On 1/28/22 08:30, Jaehoon Chung wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> On 1/28/22 12:40, Marek Vasut wrote:
>>>> This reverts commit b5874b552ffa09bc1dc5dec6b5dd376c62dab45d.
>>>>
>>>> It seems the iMX8MM SDHC controller always reports DAT0 line status
>>>> as zero after voltage switch at the end of mmc_switch_voltage(), even
>>>> if it is supposed to be high and scope confirms the DAT0 is high.
>>>> Reverting this patch makes SDR104 work on iMX8MM, however, it is not
>>>> clear why the DAT0 status is not correctly reported by the
>>>> controller.
>>>
>>> I didn't have an imx8mm board, so I wonder that other boards which is using
>> fsl_esdhci_imx driver have same problem.
>>
>> I have multiple 8mm, just not the 8mm-evk.
>>
>>> I'm not sure...I remember that some controller doesn't support a busy signal.
>>> In kernel driver, also doesn't report correct status?
>>
>> Kernel doesn't use this DAT0 readback, that's why it works in kernel.
>>
>> U-Boot has been augmented with this DAT0 readback some time ago, I just
>> noticed it on an 8mm board recently that it doesn't really work. There is also no
>> errata.
>>
>> I think it would be good to have NXP check this.
> 
> Hi,
> 
> The issue you meet should be related to one patch that was revert recently, refer to
> Commit f132aab40327 " Revert "mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output""
> 
> The issue you meet seems the same as what I mentioned in my original commit log, I paste in the end.
> Revert this patch because we did find this patch has some side effect, like enlarge the whole boot up time about 20s.
> If you will, you can add back this patch on your side, and test it.
> I will send one new patch this week, to fix issue and avoid side effect.
> 
> Best Regards
> Haibo Chen
> 
> 
> commit 63756575b42b8b4fb3f59cbbf0cedf03331bc2d2
> Author: Haibo Chen <haibo.chen@nxp.com>
> Date:   Wed Mar 3 17:05:46 2021 +0800
> 
>      mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output
> 
>      For FSL_USDHC, it do not implement VENDORSPEC_CKEN/PEREN/HCKEN/IPGEN, these
>      are reserved bits. Instead, use VENDORSPEC_FRC_SDCLK_ON to gate on/off the
>      card clock output.
> 
>      After commit b5874b552ffa ("mmc: fsl_esdhc_imx: add wait_dat0() support"),
>      we meet SD3.0 card can't work at UHS mode, mmc_switch_voltage() fail because
>      the second mmc_wait_dat0 return -ETIMEDOUT. According to SD spec, during
>      voltage switch, need to gate off/on the card clock. If not set the FRC_SDCLK_ON,
>      after CMD11, hardware will gate off the card clock automatically, so card do
>      not detect the clock off/on behavior, so will draw the data0 line low until
>      next command.
> 
>      Fixes: b5874b552ffa ("mmc: fsl_esdhc_imx: add wait_dat0() support")
>      Tested-by: Tim Harvey <tharvey@gateworks.com>
>      Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> 

I see you already managed to post follow up patches, I already tested 
those and they indeed help. I'll provide RB shortly, thanks.