[PATCH 1/2] mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output

[PATCH 1/2] mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output

[haibo.chen at nxp.com]

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

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>
---
 drivers/mmc/fsl_esdhc_imx.c | 29 +++++++++++++++++++++--------
 include/fsl_esdhc_imx.h     |  2 ++
 2 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
index e0e132698e..af36558b3c 100644
--- a/drivers/mmc/fsl_esdhc_imx.c
+++ b/drivers/mmc/fsl_esdhc_imx.c
@@ -654,7 +654,10 @@ static void set_sysctl(struct fsl_esdhc_priv *priv, struct mmc *mmc, uint clock)
 	clk = (pre_div << 8) | (div << 4);
 
 #ifdef CONFIG_FSL_USDHC
-	esdhc_clrbits32(&regs->vendorspec, VENDORSPEC_CKEN);
+	esdhc_clrbits32(&regs->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
+	ret = readx_poll_timeout(esdhc_read32, &regs->prsstat, tmp, tmp & PRSSTAT_SDOFF, 100);
+	if (ret)
+		pr_warn("fsl_esdhc_imx: Internal clock never gate off.\n");
 #else
 	esdhc_clrbits32(&regs->sysctl, SYSCTL_CKEN);
 #endif
@@ -666,7 +669,7 @@ static void set_sysctl(struct fsl_esdhc_priv *priv, struct mmc *mmc, uint clock)
 		pr_warn("fsl_esdhc_imx: Internal clock never stabilised.\n");
 
 #ifdef CONFIG_FSL_USDHC
-	esdhc_setbits32(&regs->vendorspec, VENDORSPEC_PEREN | VENDORSPEC_CKEN);
+	esdhc_setbits32(&regs->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
 #else
 	esdhc_setbits32(&regs->sysctl, SYSCTL_PEREN | SYSCTL_CKEN);
 #endif
@@ -721,8 +724,14 @@ static void esdhc_set_strobe_dll(struct mmc *mmc)
 	struct fsl_esdhc_priv *priv = dev_get_priv(mmc->dev);
 	struct fsl_esdhc *regs = priv->esdhc_regs;
 	u32 val;
+	u32 tmp;
+	int ret;
 
 	if (priv->clock > ESDHC_STROBE_DLL_CLK_FREQ) {
+		esdhc_clrbits32(&regs->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
+		ret = readx_poll_timeout(esdhc_read32, &regs->prsstat, tmp, tmp & PRSSTAT_SDOFF, 100);
+		if (ret)
+			pr_warn("fsl_esdhc_imx: Internal clock never gate off.\n");
 		esdhc_write32(&regs->strobe_dllctrl, ESDHC_STROBE_DLL_CTRL_RESET);
 
 		/*
@@ -740,6 +749,7 @@ static void esdhc_set_strobe_dll(struct mmc *mmc)
 			pr_warn("HS400 strobe DLL status REF not lock!\n");
 		if (!(val & ESDHC_STROBE_DLL_STS_SLV_LOCK))
 			pr_warn("HS400 strobe DLL status SLV not lock!\n");
+		esdhc_setbits32(&regs->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
 	}
 }
 
@@ -963,14 +973,18 @@ static int esdhc_set_ios_common(struct fsl_esdhc_priv *priv, struct mmc *mmc)
 #ifdef MMC_SUPPORTS_TUNING
 	if (mmc->clk_disable) {
 #ifdef CONFIG_FSL_USDHC
-		esdhc_clrbits32(&regs->vendorspec, VENDORSPEC_CKEN);
+		u32 tmp;
+
+		esdhc_clrbits32(&regs->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
+		ret = readx_poll_timeout(esdhc_read32, &regs->prsstat, tmp, tmp & PRSSTAT_SDOFF, 100);
+		if (ret)
+			pr_warn("fsl_esdhc_imx: Internal clock never gate off.\n");
 #else
 		esdhc_clrbits32(&regs->sysctl, SYSCTL_CKEN);
 #endif
 	} else {
 #ifdef CONFIG_FSL_USDHC
-		esdhc_setbits32(&regs->vendorspec, VENDORSPEC_PEREN |
-				VENDORSPEC_CKEN);
+		esdhc_setbits32(&regs->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
 #else
 		esdhc_setbits32(&regs->sysctl, SYSCTL_PEREN | SYSCTL_CKEN);
 #endif
@@ -1046,7 +1060,7 @@ static int esdhc_init_common(struct fsl_esdhc_priv *priv, struct mmc *mmc)
 #ifndef CONFIG_FSL_USDHC
 	esdhc_setbits32(&regs->sysctl, SYSCTL_HCKEN | SYSCTL_IPGEN);
 #else
-	esdhc_setbits32(&regs->vendorspec, VENDORSPEC_HCKEN | VENDORSPEC_IPGEN);
+	esdhc_setbits32(&regs->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
 #endif
 
 	/* Set the initial clock speed */
@@ -1184,8 +1198,7 @@ static int fsl_esdhc_init(struct fsl_esdhc_priv *priv,
 	esdhc_write32(&regs->autoc12err, 0);
 	esdhc_write32(&regs->clktunectrlstatus, 0);
 #else
-	esdhc_setbits32(&regs->vendorspec, VENDORSPEC_PEREN |
-			VENDORSPEC_HCKEN | VENDORSPEC_IPGEN | VENDORSPEC_CKEN);
+	esdhc_setbits32(&regs->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
 #endif
 
 	if (priv->vs18_enable)
diff --git a/include/fsl_esdhc_imx.h b/include/fsl_esdhc_imx.h
index 45ed635a77..b092034464 100644
--- a/include/fsl_esdhc_imx.h
+++ b/include/fsl_esdhc_imx.h
@@ -39,6 +39,7 @@
 #define VENDORSPEC_HCKEN	0x00001000
 #define VENDORSPEC_IPGEN	0x00000800
 #define VENDORSPEC_INIT		0x20007809
+#define VENDORSPEC_FRC_SDCLK_ON 0x00000100
 
 #define IRQSTAT			0x0002e030
 #define IRQSTAT_DMAE		(0x10000000)
@@ -96,6 +97,7 @@
 #define PRSSTAT_CINS		(0x00010000)
 #define PRSSTAT_BREN		(0x00000800)
 #define PRSSTAT_BWEN		(0x00000400)
+#define PRSSTAT_SDOFF		(0x00000080)
 #define PRSSTAT_SDSTB		(0X00000008)
 #define PRSSTAT_DLA		(0x00000004)
 #define PRSSTAT_CICHB		(0x00000002)
-- 
2.17.1

[PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.

[haibo.chen at nxp.com]

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

Common code already handle the voltage switch sequence based on spec,
so remove the redundant voltage switch code.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/mmc/fsl_esdhc_imx.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
index af36558b3c..a199cf3df6 100644
--- a/drivers/mmc/fsl_esdhc_imx.c
+++ b/drivers/mmc/fsl_esdhc_imx.c
@@ -515,15 +515,6 @@ static int esdhc_send_cmd_common(struct fsl_esdhc_priv *priv, struct mmc *mmc,
 		goto out;
 	}
 
-	/* Switch voltage to 1.8V if CMD11 succeeded */
-	if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V) {
-		esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
-
-		printf("Run CMD11 1.8V switch\n");
-		/* Sleep for 5 ms - max time for card to switch to 1.8V */
-		udelay(5000);
-	}
-
 	/* Workaround for ESDHC errata ENGcm03648 */
 	if (!data && (cmd->resp_type & MMC_RSP_BUSY)) {
 		int timeout = 50000;
@@ -839,6 +830,7 @@ static int esdhc_set_voltage(struct mmc *mmc)
 		}
 #endif
 		esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
+		mdelay(5);
 		if (esdhc_read32(&regs->vendorspec) & ESDHC_VENDORSPEC_VSELECT)
 			return 0;
 
-- 
2.17.1

[PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.

[Adam Ford]

On Wed, Mar 3, 2021 at 3:18 AM <haibo.chen@nxp.com> wrote:
>
> From: Haibo Chen <haibo.chen@nxp.com>
>
> Common code already handle the voltage switch sequence based on spec,
> so remove the redundant voltage switch code.
>
Nice!  Thank you.
This lets my eMMC run at HS400ES and the microSD card run as SDR104.

For the series,

Tested-by: Adam Ford <aford173@gmail.com> #imx8mn_beacon

adam
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> ---
>  drivers/mmc/fsl_esdhc_imx.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
> index af36558b3c..a199cf3df6 100644
> --- a/drivers/mmc/fsl_esdhc_imx.c
> +++ b/drivers/mmc/fsl_esdhc_imx.c
> @@ -515,15 +515,6 @@ static int esdhc_send_cmd_common(struct fsl_esdhc_priv *priv, struct mmc *mmc,
>                 goto out;
>         }
>
> -       /* Switch voltage to 1.8V if CMD11 succeeded */
> -       if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V) {
> -               esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
> -
> -               printf("Run CMD11 1.8V switch\n");
> -               /* Sleep for 5 ms - max time for card to switch to 1.8V */
> -               udelay(5000);
> -       }
> -
>         /* Workaround for ESDHC errata ENGcm03648 */
>         if (!data && (cmd->resp_type & MMC_RSP_BUSY)) {
>                 int timeout = 50000;
> @@ -839,6 +830,7 @@ static int esdhc_set_voltage(struct mmc *mmc)
>                 }
>  #endif
>                 esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
> +               mdelay(5);
>                 if (esdhc_read32(&regs->vendorspec) & ESDHC_VENDORSPEC_VSELECT)
>                         return 0;
>
> --
> 2.17.1
>

[PATCH 1/2] mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output

[ZHIZHIKIN Andrey]

Hello Haibo,

> -----Original Message-----
> From: haibo.chen at nxp.com <haibo.chen@nxp.com>
> Sent: Wednesday, March 3, 2021 10:06 AM
> To: peng.fan at nxp.com; u-boot at lists.denx.de; sbabic at denx.de
> Cc: haibo.chen at nxp.com; uboot-imx at nxp.com; tharvey at gateworks.com;
> ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>;
> festevam at gmail.com; ye.li at nxp.com
> Subject: [PATCH 1/2] mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to
> control card clock output
> 
> From: Haibo Chen <haibo.chen@nxp.com>
> 
> 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.

I believe this patch is a RESEND of the previous one, right? I do recall I was trying to test the same patch, but was missing the second one from this series.

Maybe it make sense to deprecate the old submission then, so it's not dangling in Patchwork.

> 
> 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>
> ---
>  drivers/mmc/fsl_esdhc_imx.c | 29 +++++++++++++++++++++--------
>  include/fsl_esdhc_imx.h     |  2 ++
>  2 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index
> e0e132698e..af36558b3c 100644
> --- a/drivers/mmc/fsl_esdhc_imx.c
> +++ b/drivers/mmc/fsl_esdhc_imx.c
> @@ -654,7 +654,10 @@ static void set_sysctl(struct fsl_esdhc_priv *priv, struct
> mmc *mmc, uint clock)
>         clk = (pre_div << 8) | (div << 4);
> 
>  #ifdef CONFIG_FSL_USDHC
> -       esdhc_clrbits32(&regs->vendorspec, VENDORSPEC_CKEN);
> +       esdhc_clrbits32(&regs->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
> +       ret = readx_poll_timeout(esdhc_read32, &regs->prsstat, tmp, tmp &
> PRSSTAT_SDOFF, 100);
> +       if (ret)
> +               pr_warn("fsl_esdhc_imx: Internal clock never gate
> + off.\n");
>  #else
>         esdhc_clrbits32(&regs->sysctl, SYSCTL_CKEN);  #endif @@ -666,7 +669,7 @@
> static void set_sysctl(struct fsl_esdhc_priv *priv, struct mmc *mmc, uint clock)
>                 pr_warn("fsl_esdhc_imx: Internal clock never stabilised.\n");
> 
>  #ifdef CONFIG_FSL_USDHC
> -       esdhc_setbits32(&regs->vendorspec, VENDORSPEC_PEREN |
> VENDORSPEC_CKEN);
> +       esdhc_setbits32(&regs->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
>  #else
>         esdhc_setbits32(&regs->sysctl, SYSCTL_PEREN | SYSCTL_CKEN);  #endif @@ -
> 721,8 +724,14 @@ static void esdhc_set_strobe_dll(struct mmc *mmc)
>         struct fsl_esdhc_priv *priv = dev_get_priv(mmc->dev);
>         struct fsl_esdhc *regs = priv->esdhc_regs;
>         u32 val;
> +       u32 tmp;
> +       int ret;
> 
>         if (priv->clock > ESDHC_STROBE_DLL_CLK_FREQ) {
> +               esdhc_clrbits32(&regs->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
> +               ret = readx_poll_timeout(esdhc_read32, &regs->prsstat, tmp, tmp &
> PRSSTAT_SDOFF, 100);
> +               if (ret)
> +                       pr_warn("fsl_esdhc_imx: Internal clock never
> + gate off.\n");
>                 esdhc_write32(&regs->strobe_dllctrl, ESDHC_STROBE_DLL_CTRL_RESET);
> 
>                 /*
> @@ -740,6 +749,7 @@ static void esdhc_set_strobe_dll(struct mmc *mmc)
>                         pr_warn("HS400 strobe DLL status REF not lock!\n");
>                 if (!(val & ESDHC_STROBE_DLL_STS_SLV_LOCK))
>                         pr_warn("HS400 strobe DLL status SLV not lock!\n");
> +               esdhc_setbits32(&regs->vendorspec,
> + VENDORSPEC_FRC_SDCLK_ON);
>         }
>  }
> 
> @@ -963,14 +973,18 @@ static int esdhc_set_ios_common(struct fsl_esdhc_priv
> *priv, struct mmc *mmc)  #ifdef MMC_SUPPORTS_TUNING
>         if (mmc->clk_disable) {
>  #ifdef CONFIG_FSL_USDHC
> -               esdhc_clrbits32(&regs->vendorspec, VENDORSPEC_CKEN);
> +               u32 tmp;
> +
> +               esdhc_clrbits32(&regs->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
> +               ret = readx_poll_timeout(esdhc_read32, &regs->prsstat, tmp, tmp &
> PRSSTAT_SDOFF, 100);
> +               if (ret)
> +                       pr_warn("fsl_esdhc_imx: Internal clock never
> + gate off.\n");
>  #else
>                 esdhc_clrbits32(&regs->sysctl, SYSCTL_CKEN);  #endif
>         } else {
>  #ifdef CONFIG_FSL_USDHC
> -               esdhc_setbits32(&regs->vendorspec, VENDORSPEC_PEREN |
> -                               VENDORSPEC_CKEN);
> +               esdhc_setbits32(&regs->vendorspec,
> + VENDORSPEC_FRC_SDCLK_ON);
>  #else
>                 esdhc_setbits32(&regs->sysctl, SYSCTL_PEREN | SYSCTL_CKEN);  #endif
> @@ -1046,7 +1060,7 @@ static int esdhc_init_common(struct fsl_esdhc_priv
> *priv, struct mmc *mmc)  #ifndef CONFIG_FSL_USDHC
>         esdhc_setbits32(&regs->sysctl, SYSCTL_HCKEN | SYSCTL_IPGEN);  #else
> -       esdhc_setbits32(&regs->vendorspec, VENDORSPEC_HCKEN |
> VENDORSPEC_IPGEN);
> +       esdhc_setbits32(&regs->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
>  #endif
> 
>         /* Set the initial clock speed */ @@ -1184,8 +1198,7 @@ static int
> fsl_esdhc_init(struct fsl_esdhc_priv *priv,
>         esdhc_write32(&regs->autoc12err, 0);
>         esdhc_write32(&regs->clktunectrlstatus, 0);  #else
> -       esdhc_setbits32(&regs->vendorspec, VENDORSPEC_PEREN |
> -                       VENDORSPEC_HCKEN | VENDORSPEC_IPGEN | VENDORSPEC_CKEN);
> +       esdhc_setbits32(&regs->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
>  #endif
> 
>         if (priv->vs18_enable)
> diff --git a/include/fsl_esdhc_imx.h b/include/fsl_esdhc_imx.h index
> 45ed635a77..b092034464 100644
> --- a/include/fsl_esdhc_imx.h
> +++ b/include/fsl_esdhc_imx.h
> @@ -39,6 +39,7 @@
>  #define VENDORSPEC_HCKEN       0x00001000
>  #define VENDORSPEC_IPGEN       0x00000800
>  #define VENDORSPEC_INIT                0x20007809
> +#define VENDORSPEC_FRC_SDCLK_ON 0x00000100
> 
>  #define IRQSTAT                        0x0002e030
>  #define IRQSTAT_DMAE           (0x10000000)
> @@ -96,6 +97,7 @@
>  #define PRSSTAT_CINS           (0x00010000)
>  #define PRSSTAT_BREN           (0x00000800)
>  #define PRSSTAT_BWEN           (0x00000400)
> +#define PRSSTAT_SDOFF          (0x00000080)
>  #define PRSSTAT_SDSTB          (0X00000008)
>  #define PRSSTAT_DLA            (0x00000004)
>  #define PRSSTAT_CICHB          (0x00000002)
> --
> 2.17.1

-- andrey

[PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.

[ZHIZHIKIN Andrey]

Hello Haibo,

> -----Original Message-----
> From: haibo.chen at nxp.com <haibo.chen@nxp.com>
> Sent: Wednesday, March 3, 2021 10:06 AM
> To: peng.fan at nxp.com; u-boot at lists.denx.de; sbabic at denx.de
> Cc: haibo.chen at nxp.com; uboot-imx at nxp.com; tharvey at gateworks.com;
> ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>;
> festevam at gmail.com; ye.li at nxp.com
> Subject: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related
> code.
> 
> From: Haibo Chen <haibo.chen@nxp.com>
> 
> Common code already handle the voltage switch sequence based on spec, so
> remove the redundant voltage switch code.
> 
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> ---
>  drivers/mmc/fsl_esdhc_imx.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index
> af36558b3c..a199cf3df6 100644
> --- a/drivers/mmc/fsl_esdhc_imx.c
> +++ b/drivers/mmc/fsl_esdhc_imx.c
> @@ -515,15 +515,6 @@ static int esdhc_send_cmd_common(struct
> fsl_esdhc_priv *priv, struct mmc *mmc,
>                 goto out;
>         }
> 
> -       /* Switch voltage to 1.8V if CMD11 succeeded */
> -       if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V) {
> -               esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
> -
> -               printf("Run CMD11 1.8V switch\n");
> -               /* Sleep for 5 ms - max time for card to switch to 1.8V */
> -               udelay(5000);
> -       }
> -
>         /* Workaround for ESDHC errata ENGcm03648 */
>         if (!data && (cmd->resp_type & MMC_RSP_BUSY)) {
>                 int timeout = 50000;
> @@ -839,6 +830,7 @@ static int esdhc_set_voltage(struct mmc *mmc)
>                 }
>  #endif
>                 esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
> +               mdelay(5);
>                 if (esdhc_read32(&regs->vendorspec) & ESDHC_VENDORSPEC_VSELECT)
>                         return 0;
> 
> --
> 2.17.1

Just tested the whole series on i.MX8M Mini EVK, boot log with MMC info:
---------------
U-Boot SPL 2021.04-rc3-00009-g1333570cee (Mar 03 2021 - 11:34:24 +0100)
Normal Boot
WDT:   Started with servicing (60s timeout)
Trying to boot from MMC1
NOTICE:  BL31: v2.2(release):rel_imx_5.4.70_2.3.0-0-gf1d7187f2
NOTICE:  BL31: Built : 22:29:05, Jan 17 2021


U-Boot 2021.04-rc3-00009-g1333570cee (Mar 03 2021 - 11:34:24 +0100)

CPU:   Freescale i.MX8MMQ rev1.0 at 1200 MHz
Reset cause: POR
Model: FSL i.MX8MM EVK board
DRAM:  2 GiB
WDT:   Started with servicing (60s timeout)
MMC:   FSL_SDHC: 1, FSL_SDHC: 2
Loading Environment from MMC... *** Warning - bad CRC, using default environment

In:    serial
Out:   serial
Err:   serial
Net:   eth0: ethernet at 30be0000
Hit any key to stop autoboot:  0
u-boot=> mmc dev 1
switch to partitions #0, OK
mmc1 is current device
u-boot=> mmc info
Device: FSL_SDHC
Manufacturer ID: 41
OEM: 3432
Name: SD32G
Bus Speed: 200000000
Mode: UHS SDR104 (208MHz)
Rd Block Len: 512
SD version 3.0
High Capacity: Yes
Capacity: 29.3 GiB
Bus Width: 4-bit
Erase Group Size: 512 Bytes
u-boot=> mmc dev 2
switch to partitions #0, OK
mmc2(part 0) is current device
u-boot=> mmc info
Device: FSL_SDHC
Manufacturer ID: 45
OEM: 100
Name: DG401
Bus Speed: 200000000
Mode: HS400ES (200MHz)
Rd Block Len: 512
MMC version 5.1
High Capacity: Yes
Capacity: 14.7 GiB
Bus Width: 8-bit DDR
Erase Group Size: 512 KiB
HC WP Group Size: 8 MiB
User Capacity: 14.7 GiB WRREL
Boot Capacity: 4 MiB ENH
RPMB Capacity: 4 MiB ENH
Boot area 0 is not write protected
Boot area 1 is not write protected
u-boot=>
------------

For the series:
Tested-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com> # imx8mm_evk

-- andrey

[PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.

[ZHIZHIKIN Andrey]

> -----Original Message-----
> From: haibo.chen at nxp.com <haibo.chen@nxp.com>
> Sent: Wednesday, March 3, 2021 10:06 AM
> To: peng.fan at nxp.com; u-boot at lists.denx.de; sbabic at denx.de
> Cc: haibo.chen at nxp.com; uboot-imx at nxp.com; tharvey at gateworks.com;
> ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>;
> festevam at gmail.com; ye.li at nxp.com
> Subject: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related
> code.
> 
> From: Haibo Chen <haibo.chen@nxp.com>
> 
> Common code already handle the voltage switch sequence based on spec, so
> remove the redundant voltage switch code.
> 
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> ---
>  drivers/mmc/fsl_esdhc_imx.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index
> af36558b3c..a199cf3df6 100644
> --- a/drivers/mmc/fsl_esdhc_imx.c
> +++ b/drivers/mmc/fsl_esdhc_imx.c
> @@ -515,15 +515,6 @@ static int esdhc_send_cmd_common(struct
> fsl_esdhc_priv *priv, struct mmc *mmc,
>                 goto out;
>         }
> 
> -       /* Switch voltage to 1.8V if CMD11 succeeded */
> -       if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V) {
> -               esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
> -
> -               printf("Run CMD11 1.8V switch\n");
> -               /* Sleep for 5 ms - max time for card to switch to 1.8V */
> -               udelay(5000);
> -       }
> -
>         /* Workaround for ESDHC errata ENGcm03648 */
>         if (!data && (cmd->resp_type & MMC_RSP_BUSY)) {
>                 int timeout = 50000;
> @@ -839,6 +830,7 @@ static int esdhc_set_voltage(struct mmc *mmc)
>                 }
>  #endif
>                 esdhc_setbits32(&regs->vendorspec, ESDHC_VENDORSPEC_VSELECT);
> +               mdelay(5);

Why is this delay introduced here? It is not clear from the commit message whether and why it is required here.

If this is kept from the removed block - maybe it is better to move the corresponding comment here as well.

>                 if (esdhc_read32(&regs->vendorspec) & ESDHC_VENDORSPEC_VSELECT)
>                         return 0;
> 
> --
> 2.17.1

-- andrey

[PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.

[Bough Chen]

> -----Original Message-----
> From: ZHIZHIKIN Andrey [mailto:andrey.zhizhikin at leica-geosystems.com]
> Sent: 2021?3?3? 19:00
> To: Bough Chen <haibo.chen@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> u-boot at lists.denx.de; sbabic at denx.de
> Cc: dl-uboot-imx <uboot-imx@nxp.com>; tharvey at gateworks.com;
> festevam at gmail.com; Ye Li <ye.li@nxp.com>
> Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> related code.
> 
> 
> 
> > -----Original Message-----
> > From: haibo.chen at nxp.com <haibo.chen@nxp.com>
> > Sent: Wednesday, March 3, 2021 10:06 AM
> > To: peng.fan at nxp.com; u-boot at lists.denx.de; sbabic at denx.de
> > Cc: haibo.chen at nxp.com; uboot-imx at nxp.com; tharvey at gateworks.com;
> > ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>;
> > festevam at gmail.com; ye.li at nxp.com
> > Subject: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> > related code.
> >
> > From: Haibo Chen <haibo.chen@nxp.com>
> >
> > Common code already handle the voltage switch sequence based on spec,
> > so remove the redundant voltage switch code.
> >
> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > ---
> >  drivers/mmc/fsl_esdhc_imx.c | 10 +---------
> >  1 file changed, 1 insertion(+), 9 deletions(-)
> >
> > diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
> > index
> > af36558b3c..a199cf3df6 100644
> > --- a/drivers/mmc/fsl_esdhc_imx.c
> > +++ b/drivers/mmc/fsl_esdhc_imx.c
> > @@ -515,15 +515,6 @@ static int esdhc_send_cmd_common(struct
> > fsl_esdhc_priv *priv, struct mmc *mmc,
> >                 goto out;
> >         }
> >
> > -       /* Switch voltage to 1.8V if CMD11 succeeded */
> > -       if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V) {
> > -               esdhc_setbits32(&regs->vendorspec,
> ESDHC_VENDORSPEC_VSELECT);
> > -
> > -               printf("Run CMD11 1.8V switch\n");
> > -               /* Sleep for 5 ms - max time for card to switch to 1.8V */
> > -               udelay(5000);
> > -       }
> > -
> >         /* Workaround for ESDHC errata ENGcm03648 */
> >         if (!data && (cmd->resp_type & MMC_RSP_BUSY)) {
> >                 int timeout = 50000;
> > @@ -839,6 +830,7 @@ static int esdhc_set_voltage(struct mmc *mmc)
> >                 }
> >  #endif
> >                 esdhc_setbits32(&regs->vendorspec,
> > ESDHC_VENDORSPEC_VSELECT);
> > +               mdelay(5);
> 
> Why is this delay introduced here? It is not clear from the commit message
> whether and why it is required here.
> 
> If this is kept from the removed block - maybe it is better to move the
> corresponding comment here as well.

Hi Andrev,

Thanks for your careful review!
Without this 5ms delay, with patch1 and remove the upper redundant cmd11 related code,
mmc_switch_voltage() will fail, due to the second mmc_wait_dat0() return timeout. Seems for usdhc,
gate off clock for 10ms is not enough. If total delay 15ms, then the second mmc_wait_dat0() can return normal.
So I add 5ms delay here. Yes, I should add a comment for this 5ms in the code.
You can also do the test on your side.

Best Regards
Haibo

> 
> >                 if (esdhc_read32(&regs->vendorspec) &
> ESDHC_VENDORSPEC_VSELECT)
> >                         return 0;
> >
> > --
> > 2.17.1
> 
> -- andrey
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 9571 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210303/614dede6/attachment.bin>

[PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.

[ZHIZHIKIN Andrey]

Hello Haibo,

> -----Original Message-----
> From: Bough Chen <haibo.chen@nxp.com>
> Sent: Wednesday, March 3, 2021 12:27 PM
> To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>; Peng Fan
> <peng.fan@nxp.com>; u-boot at lists.denx.de; sbabic at denx.de
> Cc: dl-uboot-imx <uboot-imx@nxp.com>; tharvey at gateworks.com;
> festevam at gmail.com; Ye Li <ye.li@nxp.com>
> Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related
> code.
> 
> > -----Original Message-----
> > From: ZHIZHIKIN Andrey [mailto:andrey.zhizhikin at leica-geosystems.com]
> > Sent: 2021?3?3? 19:00
> > To: Bough Chen <haibo.chen@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> > u-boot at lists.denx.de; sbabic at denx.de
> > Cc: dl-uboot-imx <uboot-imx@nxp.com>; tharvey at gateworks.com;
> > festevam at gmail.com; Ye Li <ye.li@nxp.com>
> > Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> > related code.
> >
> >
> >
> > > -----Original Message-----
> > > From: haibo.chen at nxp.com <haibo.chen@nxp.com>
> > > Sent: Wednesday, March 3, 2021 10:06 AM
> > > To: peng.fan at nxp.com; u-boot at lists.denx.de; sbabic at denx.de
> > > Cc: haibo.chen at nxp.com; uboot-imx at nxp.com; tharvey at gateworks.com;
> > > ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>;
> > > festevam at gmail.com; ye.li at nxp.com
> > > Subject: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> > > related code.
> > >
> > > From: Haibo Chen <haibo.chen@nxp.com>
> > >
> > > Common code already handle the voltage switch sequence based on spec,
> > > so remove the redundant voltage switch code.
> > >
> > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > > ---
> > >  drivers/mmc/fsl_esdhc_imx.c | 10 +---------
> > >  1 file changed, 1 insertion(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
> > > index
> > > af36558b3c..a199cf3df6 100644
> > > --- a/drivers/mmc/fsl_esdhc_imx.c
> > > +++ b/drivers/mmc/fsl_esdhc_imx.c
> > > @@ -515,15 +515,6 @@ static int esdhc_send_cmd_common(struct
> > > fsl_esdhc_priv *priv, struct mmc *mmc,
> > >                 goto out;
> > >         }
> > >
> > > -       /* Switch voltage to 1.8V if CMD11 succeeded */
> > > -       if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V) {
> > > -               esdhc_setbits32(&regs->vendorspec,
> > ESDHC_VENDORSPEC_VSELECT);
> > > -
> > > -               printf("Run CMD11 1.8V switch\n");
> > > -               /* Sleep for 5 ms - max time for card to switch to 1.8V */
> > > -               udelay(5000);
> > > -       }
> > > -
> > >         /* Workaround for ESDHC errata ENGcm03648 */
> > >         if (!data && (cmd->resp_type & MMC_RSP_BUSY)) {
> > >                 int timeout = 50000;
> > > @@ -839,6 +830,7 @@ static int esdhc_set_voltage(struct mmc *mmc)
> > >                 }
> > >  #endif
> > >                 esdhc_setbits32(&regs->vendorspec,
> > > ESDHC_VENDORSPEC_VSELECT);
> > > +               mdelay(5);
> >
> > Why is this delay introduced here? It is not clear from the commit message
> > whether and why it is required here.
> >
> > If this is kept from the removed block - maybe it is better to move the
> > corresponding comment here as well.
> 
> Hi Andrev,
> 
> Thanks for your careful review!

Thanks for the patch series on the first place! This allows to switch uSDHC into high-speed modes, which is quite valuable.

> Without this 5ms delay, with patch1 and remove the upper redundant cmd11
> related code,
> mmc_switch_voltage() will fail, due to the second mmc_wait_dat0() return
> timeout. Seems for usdhc,
> gate off clock for 10ms is not enough. If total delay 15ms, then the second
> mmc_wait_dat0() can return normal.
> So I add 5ms delay here. Yes, I should add a comment for this 5ms in the code.

Exactly this information is missing with the patch, as later on it would be quite difficult to grasp on why this mdelay() was added on the first place.

> You can also do the test on your side.

I've already did and reported with the boot log, mmc info, and my Tested-by: tag.

> 
> Best Regards
> Haibo
> 
> >
> > >                 if (esdhc_read32(&regs->vendorspec) &
> > ESDHC_VENDORSPEC_VSELECT)
> > >                         return 0;
> > >
> > > --
> > > 2.17.1
> >
> > -- andrey

-- andrey

[PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.

[Bough Chen]

> -----Original Message-----
> From: ZHIZHIKIN Andrey [mailto:andrey.zhizhikin at leica-geosystems.com]
> Sent: 2021?3?3? 20:50
> To: Bough Chen <haibo.chen@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> u-boot at lists.denx.de; sbabic at denx.de
> Cc: dl-uboot-imx <uboot-imx@nxp.com>; tharvey at gateworks.com;
> festevam at gmail.com; Ye Li <ye.li@nxp.com>
> Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> related code.
> 
> Hello Haibo,
> 
> > -----Original Message-----
> > From: Bough Chen <haibo.chen@nxp.com>
> > Sent: Wednesday, March 3, 2021 12:27 PM
> > To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>; Peng Fan
> > <peng.fan@nxp.com>; u-boot at lists.denx.de; sbabic at denx.de
> > Cc: dl-uboot-imx <uboot-imx@nxp.com>; tharvey at gateworks.com;
> > festevam at gmail.com; Ye Li <ye.li@nxp.com>
> > Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> > related code.
> >
> > > -----Original Message-----
> > > From: ZHIZHIKIN Andrey
> > > [mailto:andrey.zhizhikin at leica-geosystems.com]
> > > Sent: 2021?3?3? 19:00
> > > To: Bough Chen <haibo.chen@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> > > u-boot at lists.denx.de; sbabic at denx.de
> > > Cc: dl-uboot-imx <uboot-imx@nxp.com>; tharvey at gateworks.com;
> > > festevam at gmail.com; Ye Li <ye.li@nxp.com>
> > > Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> > > related code.
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: haibo.chen at nxp.com <haibo.chen@nxp.com>
> > > > Sent: Wednesday, March 3, 2021 10:06 AM
> > > > To: peng.fan at nxp.com; u-boot at lists.denx.de; sbabic at denx.de
> > > > Cc: haibo.chen at nxp.com; uboot-imx at nxp.com;
> tharvey at gateworks.com;
> > > > ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>;
> > > > festevam at gmail.com; ye.li at nxp.com
> > > > Subject: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> > > > related code.
> > > >
> > > > From: Haibo Chen <haibo.chen@nxp.com>
> > > >
> > > > Common code already handle the voltage switch sequence based on
> > > > spec, so remove the redundant voltage switch code.
> > > >
> > > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > > > ---
> > > >  drivers/mmc/fsl_esdhc_imx.c | 10 +---------
> > > >  1 file changed, 1 insertion(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/mmc/fsl_esdhc_imx.c
> > > > b/drivers/mmc/fsl_esdhc_imx.c index
> > > > af36558b3c..a199cf3df6 100644
> > > > --- a/drivers/mmc/fsl_esdhc_imx.c
> > > > +++ b/drivers/mmc/fsl_esdhc_imx.c
> > > > @@ -515,15 +515,6 @@ static int esdhc_send_cmd_common(struct
> > > > fsl_esdhc_priv *priv, struct mmc *mmc,
> > > >                 goto out;
> > > >         }
> > > >
> > > > -       /* Switch voltage to 1.8V if CMD11 succeeded */
> > > > -       if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V) {
> > > > -               esdhc_setbits32(&regs->vendorspec,
> > > ESDHC_VENDORSPEC_VSELECT);
> > > > -
> > > > -               printf("Run CMD11 1.8V switch\n");
> > > > -               /* Sleep for 5 ms - max time for card to switch to 1.8V
> */
> > > > -               udelay(5000);
> > > > -       }
> > > > -
> > > >         /* Workaround for ESDHC errata ENGcm03648 */
> > > >         if (!data && (cmd->resp_type & MMC_RSP_BUSY)) {
> > > >                 int timeout = 50000; @@ -839,6 +830,7 @@ static
> > > > int esdhc_set_voltage(struct mmc *mmc)
> > > >                 }
> > > >  #endif
> > > >                 esdhc_setbits32(&regs->vendorspec,
> > > > ESDHC_VENDORSPEC_VSELECT);
> > > > +               mdelay(5);
> > >
> > > Why is this delay introduced here? It is not clear from the commit
> > > message whether and why it is required here.
> > >
> > > If this is kept from the removed block - maybe it is better to move
> > > the corresponding comment here as well.
> >
> > Hi Andrev,
> >
> > Thanks for your careful review!
> 
> Thanks for the patch series on the first place! This allows to switch uSDHC into
> high-speed modes, which is quite valuable.
> 
> > Without this 5ms delay, with patch1 and remove the upper redundant
> > cmd11 related code,
> > mmc_switch_voltage() will fail, due to the second mmc_wait_dat0()
> > return timeout. Seems for usdhc, gate off clock for 10ms is not
> > enough. If total delay 15ms, then the second
> > mmc_wait_dat0() can return normal.
> > So I add 5ms delay here. Yes, I should add a comment for this 5ms in the
> code.
> 
> Exactly this information is missing with the patch, as later on it would be quite
> difficult to grasp on why this mdelay() was added on the first place.
> 
> > You can also do the test on your side.
> 
> I've already did and reported with the boot log, mmc info, and my Tested-by:
> tag.
> 

I spend some time to dig into the extra 5ms, find this is board design issue. I test three boards, 
Imx8mn-ddr4-evk and imx8mp-evk board do not need this 5ms delay, only imx8mm-evkb board need this extra 5ms delay.
This is because on imx8mm-evkb board, the voltage switch circuit is controlled by our PMIC, and this LD0 output connect one
10uF capacitance, makes the voltage switch from 3.3v to 1,8v need around 18ms. For other imx8mn and imx8mp board, the capacitance
connected is only 1uF/4.7uF, the voltage switch time is 400us/8ms, so software 10ms delay can cover these two boards.

Seems this is board specific issue, I will send v2 patch, only imx8mm-evkb board need this delay, and will add a comment in the code.

Thanks!
> >
> > Best Regards
> > Haibo
> >
> > >
> > > >                 if (esdhc_read32(&regs->vendorspec) &
> > > ESDHC_VENDORSPEC_VSELECT)
> > > >                         return 0;
> > > >
> > > > --
> > > > 2.17.1
> > >
> > > -- andrey
> 
> -- andrey
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 9571 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210308/c6977eeb/attachment.bin>

[PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.

[Bough Chen]

> -----Original Message-----
> From: Bough Chen
> Sent: 2021?3?8? 17:50
> To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>; Peng Fan
> <peng.fan@nxp.com>; u-boot at lists.denx.de; sbabic at denx.de
> Cc: dl-uboot-imx <uboot-imx@nxp.com>; tharvey at gateworks.com;
> festevam at gmail.com; Ye Li <ye.li@nxp.com>
> Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> related code.
> 
> 
> > -----Original Message-----
> > From: ZHIZHIKIN Andrey [mailto:andrey.zhizhikin at leica-geosystems.com]
> > Sent: 2021?3?3? 20:50
> > To: Bough Chen <haibo.chen@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> > u-boot at lists.denx.de; sbabic at denx.de
> > Cc: dl-uboot-imx <uboot-imx@nxp.com>; tharvey at gateworks.com;
> > festevam at gmail.com; Ye Li <ye.li@nxp.com>
> > Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> > related code.
> >
> > Hello Haibo,
> >
> > > -----Original Message-----
> > > From: Bough Chen <haibo.chen@nxp.com>
> > > Sent: Wednesday, March 3, 2021 12:27 PM
> > > To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>; Peng Fan
> > > <peng.fan@nxp.com>; u-boot at lists.denx.de; sbabic at denx.de
> > > Cc: dl-uboot-imx <uboot-imx@nxp.com>; tharvey at gateworks.com;
> > > festevam at gmail.com; Ye Li <ye.li@nxp.com>
> > > Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> > > related code.
> > >
> > > > -----Original Message-----
> > > > From: ZHIZHIKIN Andrey
> > > > [mailto:andrey.zhizhikin at leica-geosystems.com]
> > > > Sent: 2021?3?3? 19:00
> > > > To: Bough Chen <haibo.chen@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> > > > u-boot at lists.denx.de; sbabic at denx.de
> > > > Cc: dl-uboot-imx <uboot-imx@nxp.com>; tharvey at gateworks.com;
> > > > festevam at gmail.com; Ye Li <ye.li@nxp.com>
> > > > Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> > > > related code.
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: haibo.chen at nxp.com <haibo.chen@nxp.com>
> > > > > Sent: Wednesday, March 3, 2021 10:06 AM
> > > > > To: peng.fan at nxp.com; u-boot at lists.denx.de; sbabic at denx.de
> > > > > Cc: haibo.chen at nxp.com; uboot-imx at nxp.com;
> > tharvey at gateworks.com;
> > > > > ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>;
> > > > > festevam at gmail.com; ye.li at nxp.com
> > > > > Subject: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11
> > > > > related code.
> > > > >
> > > > > From: Haibo Chen <haibo.chen@nxp.com>
> > > > >
> > > > > Common code already handle the voltage switch sequence based on
> > > > > spec, so remove the redundant voltage switch code.
> > > > >
> > > > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > > > > ---
> > > > >  drivers/mmc/fsl_esdhc_imx.c | 10 +---------
> > > > >  1 file changed, 1 insertion(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/drivers/mmc/fsl_esdhc_imx.c
> > > > > b/drivers/mmc/fsl_esdhc_imx.c index
> > > > > af36558b3c..a199cf3df6 100644
> > > > > --- a/drivers/mmc/fsl_esdhc_imx.c
> > > > > +++ b/drivers/mmc/fsl_esdhc_imx.c
> > > > > @@ -515,15 +515,6 @@ static int esdhc_send_cmd_common(struct
> > > > > fsl_esdhc_priv *priv, struct mmc *mmc,
> > > > >                 goto out;
> > > > >         }
> > > > >
> > > > > -       /* Switch voltage to 1.8V if CMD11 succeeded */
> > > > > -       if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V) {
> > > > > -               esdhc_setbits32(&regs->vendorspec,
> > > > ESDHC_VENDORSPEC_VSELECT);
> > > > > -
> > > > > -               printf("Run CMD11 1.8V switch\n");
> > > > > -               /* Sleep for 5 ms - max time for card to switch to
> 1.8V
> > */
> > > > > -               udelay(5000);
> > > > > -       }
> > > > > -
> > > > >         /* Workaround for ESDHC errata ENGcm03648 */
> > > > >         if (!data && (cmd->resp_type & MMC_RSP_BUSY)) {
> > > > >                 int timeout = 50000; @@ -839,6 +830,7 @@ static
> > > > > int esdhc_set_voltage(struct mmc *mmc)
> > > > >                 }
> > > > >  #endif
> > > > >                 esdhc_setbits32(&regs->vendorspec,
> > > > > ESDHC_VENDORSPEC_VSELECT);
> > > > > +               mdelay(5);
> > > >
> > > > Why is this delay introduced here? It is not clear from the commit
> > > > message whether and why it is required here.
> > > >
> > > > If this is kept from the removed block - maybe it is better to move
> > > > the corresponding comment here as well.
> > >
> > > Hi Andrev,
> > >
> > > Thanks for your careful review!
> >
> > Thanks for the patch series on the first place! This allows to switch uSDHC
> into
> > high-speed modes, which is quite valuable.
> >
> > > Without this 5ms delay, with patch1 and remove the upper redundant
> > > cmd11 related code,
> > > mmc_switch_voltage() will fail, due to the second mmc_wait_dat0()
> > > return timeout. Seems for usdhc, gate off clock for 10ms is not
> > > enough. If total delay 15ms, then the second
> > > mmc_wait_dat0() can return normal.
> > > So I add 5ms delay here. Yes, I should add a comment for this 5ms in the
> > code.
> >
> > Exactly this information is missing with the patch, as later on it would be
> quite
> > difficult to grasp on why this mdelay() was added on the first place.
> >
> > > You can also do the test on your side.
> >
> > I've already did and reported with the boot log, mmc info, and my Tested-by:
> > tag.
> >
> 
> I spend some time to dig into the extra 5ms, find this is board design issue. I
> test three boards,
> Imx8mn-ddr4-evk and imx8mp-evk board do not need this 5ms delay, only
> imx8mm-evkb board need this extra 5ms delay.
> This is because on imx8mm-evkb board, the voltage switch circuit is controlled
> by our PMIC, and this LD0 output connect one
> 10uF capacitance, makes the voltage switch from 3.3v to 1,8v need around
> 18ms. For other imx8mn and imx8mp board, the capacitance
> connected is only 1uF/4.7uF, the voltage switch time is 400us/8ms, so software
> 10ms delay can cover these two boards.
> 
> Seems this is board specific issue, I will send v2 patch, only imx8mm-evkb board
> need this delay, and will add a comment in the code.
> 

Hi all, 

Double confirm with our board design team, this is also related with the PMIC chip.
We are asking our PMIC team, whether can config the LDO transition time from the PMIC side.
So let's first hold this patch.

But for patch 1, there is no problem, Peng/Stefano, can you help pick that patch?
Only apply patch 1 can also fix the previous issue.


Best Regards
Haibo Chen


> Thanks!
> > >
> > > Best Regards
> > > Haibo
> > >
> > > >
> > > > >                 if (esdhc_read32(&regs->vendorspec) &
> > > > ESDHC_VENDORSPEC_VSELECT)
> > > > >                         return 0;
> > > > >
> > > > > --
> > > > > 2.17.1
> > > >
> > > > -- andrey
> >
> > -- andrey
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 9571 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210309/668fffed/attachment.bin>

[PATCH 1/2] mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output

[sbabic at denx.de]

> From: Haibo Chen <haibo.chen@nxp.com>
> 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>
Applied to u-boot-imx, master, thanks !

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================