[PATCH v2] Revert "mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output"

[PATCH v2] Revert "mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output"

[Fabio Estevam]

This reverts commit 63756575b42b8b4fb3f59cbbf0cedf03331bc2d2.

Since this commit a imx6qdl-pico board boots extremely slowly
in both SPL as well as U-Boot proper.

Fix this regression by reverting the offending commit for now.

Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
Changes since v1:
- Include the author of the offending commit on Cc.

 drivers/mmc/fsl_esdhc_imx.c | 29 ++++++++---------------------
 include/fsl_esdhc_imx.h     |  2 --
 2 files changed, 8 insertions(+), 23 deletions(-)

diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
index a4675838e58a..b49d5ede2711 100644
--- a/drivers/mmc/fsl_esdhc_imx.c
+++ b/drivers/mmc/fsl_esdhc_imx.c
@@ -653,10 +653,7 @@ 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_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_clrbits32(&regs->vendorspec, VENDORSPEC_CKEN);
 #else
 	esdhc_clrbits32(&regs->sysctl, SYSCTL_CKEN);
 #endif
@@ -668,7 +665,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_FRC_SDCLK_ON);
+	esdhc_setbits32(&regs->vendorspec, VENDORSPEC_PEREN | VENDORSPEC_CKEN);
 #else
 	esdhc_setbits32(&regs->sysctl, SYSCTL_PEREN | SYSCTL_CKEN);
 #endif
@@ -723,14 +720,8 @@ 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);
 
 		/*
@@ -748,7 +739,6 @@ 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);
 	}
 }
 
@@ -980,18 +970,14 @@ 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
-		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");
+		esdhc_clrbits32(&regs->vendorspec, VENDORSPEC_CKEN);
 #else
 		esdhc_clrbits32(&regs->sysctl, SYSCTL_CKEN);
 #endif
 	} else {
 #ifdef CONFIG_FSL_USDHC
-		esdhc_setbits32(&regs->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
+		esdhc_setbits32(&regs->vendorspec, VENDORSPEC_PEREN |
+				VENDORSPEC_CKEN);
 #else
 		esdhc_setbits32(&regs->sysctl, SYSCTL_PEREN | SYSCTL_CKEN);
 #endif
@@ -1067,7 +1053,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_FRC_SDCLK_ON);
+	esdhc_setbits32(&regs->vendorspec, VENDORSPEC_HCKEN | VENDORSPEC_IPGEN);
 #endif
 
 	/* Set the initial clock speed */
@@ -1205,7 +1191,8 @@ 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_FRC_SDCLK_ON);
+	esdhc_setbits32(&regs->vendorspec, VENDORSPEC_PEREN |
+			VENDORSPEC_HCKEN | VENDORSPEC_IPGEN | VENDORSPEC_CKEN);
 #endif
 
 	if (priv->vs18_enable)
diff --git a/include/fsl_esdhc_imx.h b/include/fsl_esdhc_imx.h
index b0920344641c..45ed635a77bf 100644
--- a/include/fsl_esdhc_imx.h
+++ b/include/fsl_esdhc_imx.h
@@ -39,7 +39,6 @@
 #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)
@@ -97,7 +96,6 @@
 #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.25.1

RE: [PATCH v2] Revert "mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output"

[Bough Chen]

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

> -----Original Message-----
> From: Fabio Estevam [mailto:festevam@gmail.com]
> Sent: 2021年6月8日 4:40
> To: Peng Fan <peng.fan@nxp.com>
> Cc: Bough Chen <haibo.chen@nxp.com>; u-boot@lists.denx.de;
> tharvey@gateworks.com; Fabio Estevam <festevam@gmail.com>
> Subject: [PATCH v2] Revert "mmc: fsl_esdhc_imx: use
> VENDORSPEC_FRC_SDCLK_ON to control card clock output"
> 
> This reverts commit 63756575b42b8b4fb3f59cbbf0cedf03331bc2d2.
> 
> Since this commit a imx6qdl-pico board boots extremely slowly in both SPL
> as well as U-Boot proper.
> 
> Fix this regression by reverting the offending commit for now.

Hi Fabio,

Force clock on did help us fix some issue, like voltage switch for UHS card.
According to your commit log, seems this patch affect the booting time, do
you mean
the API readx_poll_timeout() cost a lot time? Can you show us the detail
info about
booting time effected by this patch?

Regards,
Haibo Chen

> 
> Signed-off-by: Fabio Estevam <festevam@gmail.com>
> ---
> Changes since v1:
> - Include the author of the offending commit on Cc.
> 
>  drivers/mmc/fsl_esdhc_imx.c | 29 ++++++++---------------------
>  include/fsl_esdhc_imx.h     |  2 --
>  2 files changed, 8 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
> index a4675838e58a..b49d5ede2711 100644
> --- a/drivers/mmc/fsl_esdhc_imx.c
> +++ b/drivers/mmc/fsl_esdhc_imx.c
> @@ -653,10 +653,7 @@ 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_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_clrbits32(&regs->vendorspec, VENDORSPEC_CKEN);
>  #else
>  	esdhc_clrbits32(&regs->sysctl, SYSCTL_CKEN);  #endif @@ -668,7
> +665,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_FRC_SDCLK_ON);
> +	esdhc_setbits32(&regs->vendorspec, VENDORSPEC_PEREN |
> +VENDORSPEC_CKEN);
>  #else
>  	esdhc_setbits32(&regs->sysctl, SYSCTL_PEREN | SYSCTL_CKEN);
> #endif @@ -723,14 +720,8 @@ 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);
> 
>  		/*
> @@ -748,7 +739,6 @@ 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);
>  	}
>  }
> 
> @@ -980,18 +970,14 @@ 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
> -		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");
> +		esdhc_clrbits32(&regs->vendorspec, VENDORSPEC_CKEN);
>  #else
>  		esdhc_clrbits32(&regs->sysctl, SYSCTL_CKEN);  #endif
>  	} else {
>  #ifdef CONFIG_FSL_USDHC
> -		esdhc_setbits32(&regs->vendorspec,
> VENDORSPEC_FRC_SDCLK_ON);
> +		esdhc_setbits32(&regs->vendorspec, VENDORSPEC_PEREN |
> +				VENDORSPEC_CKEN);
>  #else
>  		esdhc_setbits32(&regs->sysctl, SYSCTL_PEREN | SYSCTL_CKEN);
> #endif @@ -1067,7 +1053,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_FRC_SDCLK_ON);
> +	esdhc_setbits32(&regs->vendorspec, VENDORSPEC_HCKEN |
> +VENDORSPEC_IPGEN);
>  #endif
> 
>  	/* Set the initial clock speed */
> @@ -1205,7 +1191,8 @@ 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_FRC_SDCLK_ON);
> +	esdhc_setbits32(&regs->vendorspec, VENDORSPEC_PEREN |
> +			VENDORSPEC_HCKEN | VENDORSPEC_IPGEN |
> VENDORSPEC_CKEN);
>  #endif
> 
>  	if (priv->vs18_enable)
> diff --git a/include/fsl_esdhc_imx.h b/include/fsl_esdhc_imx.h index
> b0920344641c..45ed635a77bf 100644
> --- a/include/fsl_esdhc_imx.h
> +++ b/include/fsl_esdhc_imx.h
> @@ -39,7 +39,6 @@
>  #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)
> @@ -97,7 +96,6 @@
>  #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.25.1


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

Re: [PATCH v2] Revert "mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output"

[Fabio Estevam]

Hi Haibo,

On Mon, Jun 7, 2021 at 10:57 PM Bough Chen <haibo.chen@nxp.com> wrote:

> Hi Fabio,
>
> Force clock on did help us fix some issue, like voltage switch for UHS card.
> According to your commit log, seems this patch affect the booting time, do
> you mean
> the API readx_poll_timeout() cost a lot time? Can you show us the detail
> info about
> booting time effected by this patch?

If I revert the patch SPL/U-Boot boot in one second.

With this patch, it takes around 20 seconds, which is unacceptable.

Thanks

Re: [PATCH v2] Revert "mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output"

[Jaehoon Chung]

Hi,

On 6/8/21 11:07 AM, Fabio Estevam wrote:
> Hi Haibo,
> 
> On Mon, Jun 7, 2021 at 10:57 PM Bough Chen <haibo.chen@nxp.com> wrote:
> 
>> Hi Fabio,
>>
>> Force clock on did help us fix some issue, like voltage switch for UHS card.
>> According to your commit log, seems this patch affect the booting time, do
>> you mean
>> the API readx_poll_timeout() cost a lot time? Can you show us the detail
>> info about
>> booting time effected by this patch?
> 
> If I revert the patch SPL/U-Boot boot in one second.
> 
> With this patch, it takes around 20 seconds, which is unacceptable.

Is your target success to boot finally after 20sec?
I didn't have target to use fsl_esdhc_imx driver.

If booted after 20sec, how about below code?

#ifdef CONFIG_FSL_ESDHC_IMX_TIMEOUT
#define FSL_ESDHC_IMX_TIMEOUT	CONFIG_FSL_ESDHC_IMX_TIMEOUT
#else
#define FSL_ESDHC_IMX_TIMEOUT 0
#endif


ret = readx_poll_timeout(..., FSL_ESDHC_IMX_TIMEOUT);
if (FSL_ESDHC_IMX_TIMEOUT && ret)
	pr_warn("...");

If there isn't other issue, 

Best Regards,
Jaehoon Chung

> 
> Thanks
> 

Re: [PATCH v2] Revert "mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output"

[Fabio Estevam]

Hi Jaehoon,

On Mon, Jun 7, 2021 at 11:50 PM Jaehoon Chung <jh80.chung@samsung.com> wrote:

> Is your target success to boot finally after 20sec?

Yes, it boots fine after 20s.

> I didn't have target to use fsl_esdhc_imx driver.
>
> If booted after 20sec, how about below code?
>
> #ifdef CONFIG_FSL_ESDHC_IMX_TIMEOUT
> #define FSL_ESDHC_IMX_TIMEOUT   CONFIG_FSL_ESDHC_IMX_TIMEOUT
> #else
> #define FSL_ESDHC_IMX_TIMEOUT 0
> #endif
>
>
> ret = readx_poll_timeout(..., FSL_ESDHC_IMX_TIMEOUT);
> if (FSL_ESDHC_IMX_TIMEOUT && ret)
>         pr_warn("...");

Thanks for the suggestion, but it didn't work.

Re: [PATCH v2] Revert "mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output"

[Jaehoon Chung]

Hi Fabio,

On 6/9/21 6:47 AM, Fabio Estevam wrote:
> Hi Jaehoon,
> 
> On Mon, Jun 7, 2021 at 11:50 PM Jaehoon Chung <jh80.chung@samsung.com> wrote:
> 
>> Is your target success to boot finally after 20sec?
> 
> Yes, it boots fine after 20s.
> 
>> I didn't have target to use fsl_esdhc_imx driver.
>>
>> If booted after 20sec, how about below code?
>>
>> #ifdef CONFIG_FSL_ESDHC_IMX_TIMEOUT
>> #define FSL_ESDHC_IMX_TIMEOUT   CONFIG_FSL_ESDHC_IMX_TIMEOUT
>> #else
>> #define FSL_ESDHC_IMX_TIMEOUT 0
>> #endif
>>
>>
>> ret = readx_poll_timeout(..., FSL_ESDHC_IMX_TIMEOUT);
>> if (FSL_ESDHC_IMX_TIMEOUT && ret)
>>         pr_warn("...");
> 
> Thanks for the suggestion, but it didn't work.

If i find a board to use fs_esdhc_imx driver, I will check more.
I hope that Bough will suggest better solution to satisfy both case.

Best Regards,
Jaehoon Chung

> 

RE: [PATCH v2] Revert "mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output"

[Bough Chen]

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

> -----Original Message-----
> From: Jaehoon Chung [mailto:jh80.chung@samsung.com]
> Sent: 2021年6月9日 7:35
> To: Fabio Estevam <festevam@gmail.com>
> Cc: Bough Chen <haibo.chen@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> u-boot@lists.denx.de; tharvey@gateworks.com
> Subject: Re: [PATCH v2] Revert "mmc: fsl_esdhc_imx: use
> VENDORSPEC_FRC_SDCLK_ON to control card clock output"
> 
> Hi Fabio,
> 
> On 6/9/21 6:47 AM, Fabio Estevam wrote:
> > Hi Jaehoon,
> >
> > On Mon, Jun 7, 2021 at 11:50 PM Jaehoon Chung
> <jh80.chung@samsung.com> wrote:
> >
> >> Is your target success to boot finally after 20sec?
> >
> > Yes, it boots fine after 20s.
> >
> >> I didn't have target to use fsl_esdhc_imx driver.
> >>
> >> If booted after 20sec, how about below code?
> >>
> >> #ifdef CONFIG_FSL_ESDHC_IMX_TIMEOUT
> >> #define FSL_ESDHC_IMX_TIMEOUT
> CONFIG_FSL_ESDHC_IMX_TIMEOUT
> >> #else
> >> #define FSL_ESDHC_IMX_TIMEOUT 0
> >> #endif
> >>
> >>
> >> ret = readx_poll_timeout(..., FSL_ESDHC_IMX_TIMEOUT); if
> >> (FSL_ESDHC_IMX_TIMEOUT && ret)
> >>         pr_warn("...");
> >
> > Thanks for the suggestion, but it didn't work.
> 
> If i find a board to use fs_esdhc_imx driver, I will check more.
> I hope that Bough will suggest better solution to satisfy both case.

Give me a few days, I'm busy these days, will do when I have time.

Best Regards
Bough
> 
> Best Regards,
> Jaehoon Chung
> 
> >


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

Re: [PATCH v2] Revert "mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output"

[Fabio Estevam]

Hi Bough,

On Wed, Jun 9, 2021 at 7:44 AM Bough Chen <haibo.chen@nxp.com> wrote:

> Give me a few days, I'm busy these days, will do when I have time.

We are already in 2021.04-rc4, so I think it is safer to go with the
revert for now and then
you provide a proper fix later.

imx7s-warp is also affected by this.

Re: [PATCH v2] Revert "mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output"

[Peng Fan (OSS)]

Hi Fabio

On 2021/6/8 10:07, Fabio Estevam wrote:
> Hi Haibo,
> 
> On Mon, Jun 7, 2021 at 10:57 PM Bough Chen <haibo.chen@nxp.com> wrote:
> 
>> Hi Fabio,
>>
>> Force clock on did help us fix some issue, like voltage switch for UHS card.
>> According to your commit log, seems this patch affect the booting time, do
>> you mean
>> the API readx_poll_timeout() cost a lot time? Can you show us the detail
>> info about
>> booting time effected by this patch?
> 
> If I revert the patch SPL/U-Boot boot in one second.
> 
> With this patch, it takes around 20 seconds, which is unacceptable.

Do you have a chance to see where it consumes so much time?

Thanks,
Peng.

> 
> Thanks
> 

Re: [PATCH v2] Revert "mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output"

[Fabio Estevam]

Hi Peng,

On Mon, Jun 14, 2021 at 9:23 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:

> Do you have a chance to see where it consumes so much time?

Here is the place it consumes too much time. Here is an example on a
warp7 board:

U-Boot 2021.07-rc3 (Jun 17 2021 - 18:30:37 +0000)

CPU:   Freescale i.MX7S rev1.2 800 MHz (running at 792 MHz)
CPU:   Extended Commercial temperature grade (-20C to 105C) at 42C
Reset cause: POR
Model: Warp i.MX7 Board
Board: WARP7 in secure mode OPTEE DRAM 0x9d000000-0xa0000000
DRAM:  464 MiB
PMIC: PFUZE3000 DEV_ID=0x30 REV_ID=0x11
MMC:   FSL_SDHC: 3, FSL_SDHC: 0
Loading Environment from MMC... (takes 10 seconds to continue) OK

Can we go with the revert to avoid such regression?

Re: [PATCH v2] Revert "mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output"

[Jaehoon Chung]

Hi Fabio,

On 6/18/21 4:19 AM, Fabio Estevam wrote:
> Hi Peng,
> 
> On Mon, Jun 14, 2021 at 9:23 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
> 
>> Do you have a chance to see where it consumes so much time?
> 
> Here is the place it consumes too much time. Here is an example on a
> warp7 board:
> 
> U-Boot 2021.07-rc3 (Jun 17 2021 - 18:30:37 +0000)
> 
> CPU:   Freescale i.MX7S rev1.2 800 MHz (running at 792 MHz)
> CPU:   Extended Commercial temperature grade (-20C to 105C) at 42C
> Reset cause: POR
> Model: Warp i.MX7 Board
> Board: WARP7 in secure mode OPTEE DRAM 0x9d000000-0xa0000000
> DRAM:  464 MiB
> PMIC: PFUZE3000 DEV_ID=0x30 REV_ID=0x11
> MMC:   FSL_SDHC: 3, FSL_SDHC: 0
> Loading Environment from MMC... (takes 10 seconds to continue) OK
> 
> Can we go with the revert to avoid such regression?

I think that this patch can be applied. Because it's too late to boot.

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> 

Re: [PATCH v2] Revert "mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output"

[Pierre-Jean Texier]

Hi Fabio,

Le 07/06/2021 à 22:40, Fabio Estevam a écrit :
> This reverts commit 63756575b42b8b4fb3f59cbbf0cedf03331bc2d2.
> 
> Since this commit a imx6qdl-pico board boots extremely slowly
> in both SPL as well as U-Boot proper.
> 
> Fix this regression by reverting the offending commit for now.
> 
> Signed-off-by: Fabio Estevam <festevam@gmail.com>

This commit fixes the issue with the imx7s-warp.

Tested-by: Pierre-Jean Texier <texier.pj2@gmail.com>

Thanks,
-- 
Piere-Jean Texier

Re: [PATCH v2] Revert "mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output"

[Fabio Estevam]

Hi Peng/Tom,

On Sun, Jun 20, 2021 at 1:45 PM Pierre-Jean Texier <texier.pj2@gmail.com> wrote:
>
> Hi Fabio,
>
> Le 07/06/2021 à 22:40, Fabio Estevam a écrit :
> > This reverts commit 63756575b42b8b4fb3f59cbbf0cedf03331bc2d2.
> >
> > Since this commit a imx6qdl-pico board boots extremely slowly
> > in both SPL as well as U-Boot proper.
> >
> > Fix this regression by reverting the offending commit for now.
> >
> > Signed-off-by: Fabio Estevam <festevam@gmail.com>
>
> This commit fixes the issue with the imx7s-warp.
>
> Tested-by: Pierre-Jean Texier <texier.pj2@gmail.com>

The 2021.07 release is near and I would still like to see this patch
applied to fix the regression.

It negatively affects the boot time of several i.MX boards.

Please consider applying it.

Re: [PATCH v2] Revert "mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output"

[Peng Fan (OSS)]

Applied to mmc/master, awaiting upstream after CI.

On 2021/6/8 4:40, Fabio Estevam wrote:
> This reverts commit 63756575b42b8b4fb3f59cbbf0cedf03331bc2d2.
> 
> Since this commit a imx6qdl-pico board boots extremely slowly
> in both SPL as well as U-Boot proper.
> 
> Fix this regression by reverting the offending commit for now.
> 
> Signed-off-by: Fabio Estevam <festevam@gmail.com>
> ---
> Changes since v1:
> - Include the author of the offending commit on Cc.
> 
>   drivers/mmc/fsl_esdhc_imx.c | 29 ++++++++---------------------
>   include/fsl_esdhc_imx.h     |  2 --
>   2 files changed, 8 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
> index a4675838e58a..b49d5ede2711 100644
> --- a/drivers/mmc/fsl_esdhc_imx.c
> +++ b/drivers/mmc/fsl_esdhc_imx.c
> @@ -653,10 +653,7 @@ 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_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_clrbits32(&regs->vendorspec, VENDORSPEC_CKEN);
>   #else
>   	esdhc_clrbits32(&regs->sysctl, SYSCTL_CKEN);
>   #endif
> @@ -668,7 +665,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_FRC_SDCLK_ON);
> +	esdhc_setbits32(&regs->vendorspec, VENDORSPEC_PEREN | VENDORSPEC_CKEN);
>   #else
>   	esdhc_setbits32(&regs->sysctl, SYSCTL_PEREN | SYSCTL_CKEN);
>   #endif
> @@ -723,14 +720,8 @@ 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);
>   
>   		/*
> @@ -748,7 +739,6 @@ 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);
>   	}
>   }
>   
> @@ -980,18 +970,14 @@ 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
> -		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");
> +		esdhc_clrbits32(&regs->vendorspec, VENDORSPEC_CKEN);
>   #else
>   		esdhc_clrbits32(&regs->sysctl, SYSCTL_CKEN);
>   #endif
>   	} else {
>   #ifdef CONFIG_FSL_USDHC
> -		esdhc_setbits32(&regs->vendorspec, VENDORSPEC_FRC_SDCLK_ON);
> +		esdhc_setbits32(&regs->vendorspec, VENDORSPEC_PEREN |
> +				VENDORSPEC_CKEN);
>   #else
>   		esdhc_setbits32(&regs->sysctl, SYSCTL_PEREN | SYSCTL_CKEN);
>   #endif
> @@ -1067,7 +1053,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_FRC_SDCLK_ON);
> +	esdhc_setbits32(&regs->vendorspec, VENDORSPEC_HCKEN | VENDORSPEC_IPGEN);
>   #endif
>   
>   	/* Set the initial clock speed */
> @@ -1205,7 +1191,8 @@ 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_FRC_SDCLK_ON);
> +	esdhc_setbits32(&regs->vendorspec, VENDORSPEC_PEREN |
> +			VENDORSPEC_HCKEN | VENDORSPEC_IPGEN | VENDORSPEC_CKEN);
>   #endif
>   
>   	if (priv->vs18_enable)
> diff --git a/include/fsl_esdhc_imx.h b/include/fsl_esdhc_imx.h
> index b0920344641c..45ed635a77bf 100644
> --- a/include/fsl_esdhc_imx.h
> +++ b/include/fsl_esdhc_imx.h
> @@ -39,7 +39,6 @@
>   #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)
> @@ -97,7 +96,6 @@
>   #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)
>