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

[Bough Chen <haibo.chen@nxp.com>]

[-- 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 --]