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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <72e24e74-8c9d-4d4e-8a22-c2f55941c6ef@intel.com>
Date: Tue, 8 Apr 2025 20:58:49 +0300
From: Adrian Hunter <adrian.hunter@...el.com>
To: <ziniu.wang_1@....com>, <ulf.hansson@...aro.org>, <haibo.chen@....com>
CC: <shawnguo@...nel.org>, <s.hauer@...gutronix.de>, <kernel@...gutronix.de>,
	<festevam@...il.com>, <imx@...ts.linux.dev>, <linux-mmc@...r.kernel.org>,
	<s32@....com>, <linux-arm-kernel@...ts.infradead.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] mmc: sdhci-esdhc-imx: calclute data timeout value
 based on clock

On 24/03/25 11:23, ziniu.wang_1@....com wrote:
> From: Luke Wang <ziniu.wang_1@....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@....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)