[PATCH 2/2] mmc: sdhci-esdhc-imx: calclute data timeout value based on clock

[PATCH 2/2] mmc: sdhci-esdhc-imx: calclute data timeout value based on clock

[ziniu.wang_1]

From: Luke Wang <ziniu.wang_1@nxp.com>

Calclute data timeout value based on clock instead of using max value.

Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index ff78a7c6a04c..e7316ecff64e 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -31,6 +31,7 @@
 #include "cqhci.h"
 
 #define ESDHC_SYS_CTRL_DTOCV_MASK	GENMASK(19, 16)
+#define ESDHC_SYS_CTRL_DTOCV_SHIFT	16
 #define ESDHC_SYS_CTRL_IPP_RST_N	BIT(23)
 #define	ESDHC_CTRL_D3CD			0x08
 #define ESDHC_BURST_LEN_EN_INCR		(1 << 27)
@@ -1387,12 +1388,16 @@ static unsigned int esdhc_get_max_timeout_count(struct sdhci_host *host)
 
 static void esdhc_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 {
-	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
-	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
+	bool too_big = false;
+	u8 count = sdhci_calc_timeout(host, cmd, &too_big);
 
-	/* use maximum timeout counter */
+	/*
+	 * ESDHC_SYSTEM_CONTROL bit[23] used to control hardware reset
+	 * pin of the card. Write 0 to bit[23] will reset the card.
+	 * Only write DTOCV filed here.
+	 */
 	esdhc_clrset_le(host, ESDHC_SYS_CTRL_DTOCV_MASK,
-			esdhc_is_usdhc(imx_data) ? 0xF0000 : 0xE0000,
+			count << ESDHC_SYS_CTRL_DTOCV_SHIFT,
 			ESDHC_SYSTEM_CONTROL);
 }
 
@@ -1777,6 +1782,8 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
 		 * to distinguish the card type.
 		 */
 		host->mmc_host_ops.init_card = usdhc_init_card;
+
+		host->max_timeout_count = 0xF;
 	}
 
 	if (imx_data->socdata->flags & ESDHC_FLAG_MAN_TUNING)
-- 
2.34.1

RE: [PATCH 2/2] mmc: sdhci-esdhc-imx: calclute data timeout value based on clock

[Bough Chen]

> -----Original Message-----
> From: Luke Wang <ziniu.wang_1@nxp.com>
> Sent: 2025年3月24日 17:24
> To: adrian.hunter@intel.com; ulf.hansson@linaro.org; Bough Chen
> <haibo.chen@nxp.com>
> Cc: shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; imx@lists.linux.dev; linux-mmc@vger.kernel.org; dl-S32
> <S32@nxp.com>; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org
> Subject: [PATCH 2/2] mmc: sdhci-esdhc-imx: calclute data timeout value based
> on clock
> 
> From: Luke Wang <ziniu.wang_1@nxp.com>
> 
> Calclute data timeout value based on clock instead of using max value.
> 
> Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> b/drivers/mmc/host/sdhci-esdhc-imx.c
> index ff78a7c6a04c..e7316ecff64e 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -31,6 +31,7 @@
>  #include "cqhci.h"
> 
>  #define ESDHC_SYS_CTRL_DTOCV_MASK	GENMASK(19, 16)
> +#define ESDHC_SYS_CTRL_DTOCV_SHIFT	16
>  #define ESDHC_SYS_CTRL_IPP_RST_N	BIT(23)
>  #define	ESDHC_CTRL_D3CD			0x08
>  #define ESDHC_BURST_LEN_EN_INCR		(1 << 27)
> @@ -1387,12 +1388,16 @@ static unsigned int
> esdhc_get_max_timeout_count(struct sdhci_host *host)
> 
>  static void esdhc_set_timeout(struct sdhci_host *host, struct mmc_command
> *cmd)  {
> -	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> -	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
> +	bool too_big = false;
> +	u8 count = sdhci_calc_timeout(host, cmd, &too_big);
> 
> -	/* use maximum timeout counter */
> +	/*
> +	 * ESDHC_SYSTEM_CONTROL bit[23] used to control hardware reset
> +	 * pin of the card. Write 0 to bit[23] will reset the card.
> +	 * Only write DTOCV filed here.
> +	 */
No need to mention the bit[23] here, this is a bit confuse here.
Just drop the comment here.

Then for this patch:
Reviewed-by: Haibo Chen <haibo.chen@nxp.com>

Regards
Haibo Chen
>  	esdhc_clrset_le(host, ESDHC_SYS_CTRL_DTOCV_MASK,
> -			esdhc_is_usdhc(imx_data) ? 0xF0000 : 0xE0000,
> +			count << ESDHC_SYS_CTRL_DTOCV_SHIFT,
>  			ESDHC_SYSTEM_CONTROL);
>  }
> 
> @@ -1777,6 +1782,8 @@ static int sdhci_esdhc_imx_probe(struct
> platform_device *pdev)
>  		 * to distinguish the card type.
>  		 */
>  		host->mmc_host_ops.init_card = usdhc_init_card;
> +
> +		host->max_timeout_count = 0xF;
>  	}
> 
>  	if (imx_data->socdata->flags & ESDHC_FLAG_MAN_TUNING)
> --
> 2.34.1

Re: [PATCH 2/2] mmc: sdhci-esdhc-imx: calclute data timeout value based on clock

[Adrian Hunter]

On 24/03/25 11:23, ziniu.wang_1@nxp.com wrote:
> From: Luke Wang <ziniu.wang_1@nxp.com>
> 
> Calclute data timeout value based on clock instead of using max value.

And the subject:

	Calclute -> Calculate

Presumably the driver has been working OK up until now with max value.
Is there any particular reason to change it?

> 
> Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index ff78a7c6a04c..e7316ecff64e 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -31,6 +31,7 @@
>  #include "cqhci.h"
>  
>  #define ESDHC_SYS_CTRL_DTOCV_MASK	GENMASK(19, 16)
> +#define ESDHC_SYS_CTRL_DTOCV_SHIFT	16
>  #define ESDHC_SYS_CTRL_IPP_RST_N	BIT(23)
>  #define	ESDHC_CTRL_D3CD			0x08
>  #define ESDHC_BURST_LEN_EN_INCR		(1 << 27)
> @@ -1387,12 +1388,16 @@ static unsigned int esdhc_get_max_timeout_count(struct sdhci_host *host)
>  
>  static void esdhc_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
>  {
> -	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> -	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
> +	bool too_big = false;
> +	u8 count = sdhci_calc_timeout(host, cmd, &too_big);
>  
> -	/* use maximum timeout counter */
> +	/*
> +	 * ESDHC_SYSTEM_CONTROL bit[23] used to control hardware reset
> +	 * pin of the card. Write 0 to bit[23] will reset the card.
> +	 * Only write DTOCV filed here.

filed -> field ?

> +	 */
>  	esdhc_clrset_le(host, ESDHC_SYS_CTRL_DTOCV_MASK,
> -			esdhc_is_usdhc(imx_data) ? 0xF0000 : 0xE0000,
> +			count << ESDHC_SYS_CTRL_DTOCV_SHIFT,

Could use FIELD_PREP() here

>  			ESDHC_SYSTEM_CONTROL);

Another way to do this could be to add SDHCI_TIMEOUT_CONTROL to
esdhc_writeb_le() and remove esdhc_set_timeout().  That would
avoid having to export sdhci_calc_timeout() and is perhaps
slightly more consistent with other code in this driver.
Probably look something like below:

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index ff78a7c6a04c..66477fc0ba82 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -870,6 +870,16 @@ static void esdhc_writeb_le(struct sdhci_host *host, u8 val, int reg)
 
 		esdhc_clrset_le(host, mask, new_val, reg);
 		return;
+	case SDHCI_TIMEOUT_CONTROL:
+		/*
+		 * ESDHC_SYSTEM_CONTROL bit[23] used to control hardware reset
+		 * pin of the card. Write 0 to bit[23] will reset the card.
+		 * Only write DTOCV field here.
+		 */
+		esdhc_clrset_le(host, ESDHC_SYS_CTRL_DTOCV_MASK,
+				FIELD_PREP(ESDHC_SYS_CTRL_DTOCV_MASK, val),
+				ESDHC_SYSTEM_CONTROL);
+		return;
 	case SDHCI_SOFTWARE_RESET:
 		if (val & SDHCI_RESET_DATA)
 			new_val = readl(host->ioaddr + SDHCI_HOST_CONTROL);
@@ -1385,17 +1395,6 @@ static unsigned int esdhc_get_max_timeout_count(struct sdhci_host *host)
 	return esdhc_is_usdhc(imx_data) ? 1 << 29 : 1 << 27;
 }
 
-static void esdhc_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
-{
-	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
-	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
-
-	/* use maximum timeout counter */
-	esdhc_clrset_le(host, ESDHC_SYS_CTRL_DTOCV_MASK,
-			esdhc_is_usdhc(imx_data) ? 0xF0000 : 0xE0000,
-			ESDHC_SYSTEM_CONTROL);
-}
-
 static u32 esdhc_cqhci_irq(struct sdhci_host *host, u32 intmask)
 {
 	int cmd_error = 0;
@@ -1432,7 +1431,6 @@ static struct sdhci_ops sdhci_esdhc_ops = {
 	.get_min_clock = esdhc_pltfm_get_min_clock,
 	.get_max_timeout_count = esdhc_get_max_timeout_count,
 	.get_ro = esdhc_pltfm_get_ro,
-	.set_timeout = esdhc_set_timeout,
 	.set_bus_width = esdhc_pltfm_set_bus_width,
 	.set_uhs_signaling = esdhc_set_uhs_signaling,
 	.reset = esdhc_reset,


>  }
>  
> @@ -1777,6 +1782,8 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
>  		 * to distinguish the card type.
>  		 */
>  		host->mmc_host_ops.init_card = usdhc_init_card;
> +
> +		host->max_timeout_count = 0xF;
>  	}
>  
>  	if (imx_data->socdata->flags & ESDHC_FLAG_MAN_TUNING)

RE: [EXT] Re: [PATCH 2/2] mmc: sdhci-esdhc-imx: calclute data timeout value based on clock

[Luke Wang]

> -----Original Message-----
> From: Adrian Hunter <adrian.hunter@intel.com>
> Sent: Wednesday, April 9, 2025 1:59 AM
> To: Luke Wang <ziniu.wang_1@nxp.com>; ulf.hansson@linaro.org; Bough
> Chen <haibo.chen@nxp.com>
> Cc: shawnguo@kernel.org; s.hauer@pengutronix.de;
> kernel@pengutronix.de; festevam@gmail.com; imx@lists.linux.dev; linux-
> mmc@vger.kernel.org; dl-S32 <S32@nxp.com>; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [EXT] Re: [PATCH 2/2] mmc: sdhci-esdhc-imx: calclute data timeout
> value based on clock
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> On 24/03/25 11:23, ziniu.wang_1@nxp.com wrote:
> > From: Luke Wang <ziniu.wang_1@nxp.com>
> >
> > Calclute data timeout value based on clock instead of using max value.
> 
> And the subject:
> 
>         Calclute -> Calculate
> 
> Presumably the driver has been working OK up until now with max value.
> Is there any particular reason to change it?

Hi Adrian,

Yes, the max value works fine. We want the value to align with the spec recommendation.

> 
> >
> > Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com>
> > ---
> >  drivers/mmc/host/sdhci-esdhc-imx.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-
> esdhc-imx.c
> > index ff78a7c6a04c..e7316ecff64e 100644
> > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > @@ -31,6 +31,7 @@
> >  #include "cqhci.h"
> >
> >  #define ESDHC_SYS_CTRL_DTOCV_MASK    GENMASK(19, 16)
> > +#define ESDHC_SYS_CTRL_DTOCV_SHIFT   16
> >  #define ESDHC_SYS_CTRL_IPP_RST_N     BIT(23)
> >  #define      ESDHC_CTRL_D3CD                 0x08
> >  #define ESDHC_BURST_LEN_EN_INCR              (1 << 27)
> > @@ -1387,12 +1388,16 @@ static unsigned int
> esdhc_get_max_timeout_count(struct sdhci_host *host)
> >
> >  static void esdhc_set_timeout(struct sdhci_host *host, struct
> mmc_command *cmd)
> >  {
> > -     struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > -     struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
> > +     bool too_big = false;
> > +     u8 count = sdhci_calc_timeout(host, cmd, &too_big);
> >
> > -     /* use maximum timeout counter */
> > +     /*
> > +      * ESDHC_SYSTEM_CONTROL bit[23] used to control hardware reset
> > +      * pin of the card. Write 0 to bit[23] will reset the card.
> > +      * Only write DTOCV filed here.
> 
> filed -> field ?

Yes, I will use --codespell to check next time.

> 
> > +      */
> >       esdhc_clrset_le(host, ESDHC_SYS_CTRL_DTOCV_MASK,
> > -                     esdhc_is_usdhc(imx_data) ? 0xF0000 : 0xE0000,
> > +                     count << ESDHC_SYS_CTRL_DTOCV_SHIFT,
> 
> Could use FIELD_PREP() here

OK

> 
> >                       ESDHC_SYSTEM_CONTROL);
> 
> Another way to do this could be to add SDHCI_TIMEOUT_CONTROL to
> esdhc_writeb_le() and remove esdhc_set_timeout().  That would
> avoid having to export sdhci_calc_timeout() and is perhaps
> slightly more consistent with other code in this driver.
> Probably look something like below:
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-
> esdhc-imx.c
> index ff78a7c6a04c..66477fc0ba82 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -870,6 +870,16 @@ static void esdhc_writeb_le(struct sdhci_host *host,
> u8 val, int reg)
> 
>                 esdhc_clrset_le(host, mask, new_val, reg);
>                 return;
> +       case SDHCI_TIMEOUT_CONTROL:
> +               /*
> +                * ESDHC_SYSTEM_CONTROL bit[23] used to control hardware reset
> +                * pin of the card. Write 0 to bit[23] will reset the card.
> +                * Only write DTOCV field here.
> +                */
> +               esdhc_clrset_le(host, ESDHC_SYS_CTRL_DTOCV_MASK,
> +                               FIELD_PREP(ESDHC_SYS_CTRL_DTOCV_MASK, val),
> +                               ESDHC_SYSTEM_CONTROL);
> +               return;
>         case SDHCI_SOFTWARE_RESET:
>                 if (val & SDHCI_RESET_DATA)
>                         new_val = readl(host->ioaddr + SDHCI_HOST_CONTROL);
> @@ -1385,17 +1395,6 @@ static unsigned int
> esdhc_get_max_timeout_count(struct sdhci_host *host)
>         return esdhc_is_usdhc(imx_data) ? 1 << 29 : 1 << 27;
>  }
> 
> -static void esdhc_set_timeout(struct sdhci_host *host, struct
> mmc_command *cmd)
> -{
> -       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> -       struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
> -
> -       /* use maximum timeout counter */
> -       esdhc_clrset_le(host, ESDHC_SYS_CTRL_DTOCV_MASK,
> -                       esdhc_is_usdhc(imx_data) ? 0xF0000 : 0xE0000,
> -                       ESDHC_SYSTEM_CONTROL);
> -}
> -
>  static u32 esdhc_cqhci_irq(struct sdhci_host *host, u32 intmask)
>  {
>         int cmd_error = 0;
> @@ -1432,7 +1431,6 @@ static struct sdhci_ops sdhci_esdhc_ops = {
>         .get_min_clock = esdhc_pltfm_get_min_clock,
>         .get_max_timeout_count = esdhc_get_max_timeout_count,
>         .get_ro = esdhc_pltfm_get_ro,
> -       .set_timeout = esdhc_set_timeout,
>         .set_bus_width = esdhc_pltfm_set_bus_width,
>         .set_uhs_signaling = esdhc_set_uhs_signaling,
>         .reset = esdhc_reset,
> 
> 
> >  }
> >
> > @@ -1777,6 +1782,8 @@ static int sdhci_esdhc_imx_probe(struct
> platform_device *pdev)
> >                * to distinguish the card type.
> >                */
> >               host->mmc_host_ops.init_card = usdhc_init_card;
> > +
> > +             host->max_timeout_count = 0xF;
> >       }
> >
> >       if (imx_data->socdata->flags & ESDHC_FLAG_MAN_TUNING)

It's indeed a better way. I will send v2 patch.

Thanks
Luke