RE: [EXT] Re: [PATCH v2] mmc: sdhci-esdhc-imx: wait for data transfer completion before reset

-----Original Message-----
From: Ulf Hansson <redacted>
Sent: Tuesday, December 9, 2025 9:48 PM
To: Bough Chen <haibo.chen@nxp.com>; Luke Wang
[off-list ref]
Cc: adrian.hunter@intel.com; shawnguo@kernel.org;
s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
linux-mmc@vger.kernel.org; imx@lists.linux.dev; dl-S32 [off-list ref];
linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
Subject: [EXT] Re: [PATCH v2] mmc: sdhci-esdhc-imx: wait for data transfer
completion before reset

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 Fri, 5 Dec 2025 at 09:12, Bough Chen [off-list ref] wrote:

quoted

quoted

-----Original Message-----
From: Luke Wang <redacted>
Sent: 2025年12月5日 15:17
To: adrian.hunter@intel.com; Bough Chen <haibo.chen@nxp.com>;
ulf.hansson@linaro.org
Cc: shawnguo@kernel.org; s.hauer@pengutronix.de;

kernel@pengutronix.de;

quoted

quoted

festevam@gmail.com; linux-mmc@vger.kernel.org; imx@lists.linux.dev;

dl-S32

quoted

quoted

[off-list ref]; linux-arm-kernel@lists.infradead.org;
linux-kernel@vger.kernel.org
Subject: [PATCH v2] mmc: sdhci-esdhc-imx: wait for data transfer

completion

quoted

quoted

before reset

From: Luke Wang <redacted>

On IMX7ULP platforms, certain SD cards (e.g. Kingston Canvas Go! Plus)

cause

quoted

quoted

system hangs and reboots during manual tuning. These cards exhibit large

gaps

quoted

quoted

(~16us) between tuning command response and data transmission.
When cmd CRC errors occur during tuning, the code assumes data errors

even

quoted

quoted

tuning data hasn't been fully received and then reset host data circuit.

Per IMX7ULP reference manual, reset operations (RESET_DATA/ALL) need

to

quoted

quoted

make sure no active data transfers. Previously, resetting while data was in-

flight

quoted

quoted

would clear data circuit, including ADMA/SDMA address, causing data to

be

quoted

quoted

transmitted to incorrect memory address. This patch adds polling for data
transfer completion before executing resets.

Signed-off-by: Luke Wang <redacted>
---
v2: amend commit message
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c

b/drivers/mmc/host/sdhci-esdhc-imx.c
index a7a5df673b0f..affde1936510 100644

--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c

@@ -1453,6 +1453,21 @@ static void esdhc_set_uhs_signaling(struct

sdhci_host *host, unsigned timing)

 static void esdhc_reset(struct sdhci_host *host, u8 mask)  {
+     u32 present_state;
+     int ret;
+
+     /*
+      * For data or full reset, ensure any active data transfer completes
+      * before resetting to avoid system hang.
+      */
+     if (mask & (SDHCI_RESET_DATA | SDHCI_RESET_ALL)) {
+             ret = readl_poll_timeout_atomic(host->ioaddr + ESDHC_PRSSTAT,
present_state,

I'm okay with the patch, but I find one thing here:

I notice you use _atomic here, I guess you want to cover the case when the

reset function is called in hardware irq handler sdhci_irq().

quoted

I'm not sure whether it is suitable to add delay in hardware iqr handler, I

find there is also udelay in sdhci_reset(), sdhci_reset can also be called in
hardware irq handler when there is cmd_crc/data_crc error.

quoted

Adrian/Ulf, do you notice this issue before? Any comments?

You are right. In general it's certainly not preferred to poll/delay
in an atomic context. But if I understand correctly here, polling in
atomic context should not happen that frequently, right?

Moreover, I wonder if we really need to have a polling-timeout of
100ms? Where did that value come from?

I referred to the 100ms timeout setting of sdhci_reset. 

Actual measurements show that during tuning, waiting for data transfer
completion typically only requires tens of microseconds. However, considering
that the tuning block size is very small, when transferring larger blocks, it
may require a longer waiting time. 

The probability of encountering CRC errors that require a reset when not in tuning
is quite low, so waiting for some additional time may not cause significant impact.

In any case, please add a define to specify the timeout and make sure
the define-name has the suffix corresponding to the unit (_US,
_MS,...)

Sure, will do in next version

Thanks,
Luke Wang

quoted

Regards
Haibo Chen

quoted

+                                             !(present_state & SDHCI_DATA_INHIBIT), 2,
100000);
+             if (ret == -ETIMEDOUT)
+                     dev_warn(mmc_dev(host->mmc),
+                              "timeout waiting for data transfer completion\n");
+     }
+
      sdhci_and_cqhci_reset(host, mask);

      sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
--
2.34.1

Kind regards
Uffe