[PATCH v2 0/5] mmc: mmci: stm32: silent data corruption and HS200 fixes

[PATCH v2 0/5] mmc: mmci: stm32: silent data corruption and HS200 fixes

[Mateusz Nowicki]

The eMMC of the STM32MP257F-DK in HS200 returned stale data and lost
writes without any error. Patch 1 makes data CRC errors visible to the
core again. Patches 2 and 3 are the fixes of the ST downstream kernel
for the causes seen on the DK, as Christophe pointed out. Patches 4 and
5 move the eMMC of the DK back to the sdmmc2 pins_a groups and remove
the pins_b groups I added for it in 7.4: the corruption came from the
delay block, not from the slew rate, and the slower pins avoid
overshoots on the SDMMC2 clock. Patches 1-3 are for the mmc tree, 4 and
5 for the STM32 device tree, patch 4 needs patch 3.

Tested on the STM32MP257F-DK with a v7.3-rc6 based kernel with the same
changes, each run writing 832 MiB to the eMMC and reading it back:

- SDMMC2 kernel clock 200 MHz, HS200 at 100 MHz as in mainline: without
  patch 3 the writes failed with I/O errors in all 9 runs (sdmmc2 pins_a
  or pins_b, with or without NEGEDGE), with it all 6 runs were correct
  (pins_a and pins_b).
- SDMMC2 kernel clock 150 MHz, HS200 at 150 MHz with the delay block:
  all 6 runs correct (pins_a and pins_b), the tuning picked the same
  phase in all 10 rebinds on each pin group.

Build tested on v7.4-rc1 (arm64 defconfig, W=1). Not tested on
STM32MP1x.

Changes in v2:
- Drop "mmc: mmci: stm32: do not use NEGEDGE in HS200 mode". NEGEDGE
  has no effect with CLKDIV = 0 and is valid with a divider, I misread
  RM0457 (Christophe). The writes at 100 MHz fail because of the delay
  block, not NEGEDGE.
- Replace "mmc: mmci: stm32: lock the STM32MP25 delay block again before
  tuning" with the same change from the ST downstream kernel, "mmc: mmci:
  stm32: release the delay block before enabling it" (Christophe).
- Add "mmc: mmci: stm32: use the delay block above 100 MHz on stm32mp25
  SoC" from the ST downstream kernel (Christophe), with dlyb checked
  before reading min_freq from it.
- Add "arm64: dts: st: use sdmmc2 pins_a on stm32mp257f-dk" and
  "arm64: dts: st: remove sdmmc2 pins_b from stm32mp25" (Christophe).
- Rebase on v7.4-rc1.
- Link to v1: https://lore.kernel.org/r/20261031120700.42576-1-mateusz.nowicki@posteo.net

Christophe Kerello (2):
  mmc: mmci: stm32: release the delay block before enabling it
  mmc: mmci: stm32: use the delay block above 100 MHz on stm32mp25 SoC

Mateusz Nowicki (3):
  mmc: mmci: do not count a data CRC error as a complete transfer
  arm64: dts: st: use sdmmc2 pins_a on stm32mp257f-dk
  arm64: dts: st: remove sdmmc2 pins_b from stm32mp25

 arch/arm64/boot/dts/st/stm32mp25-pinctrl.dtsi | 58 -------------------
 arch/arm64/boot/dts/st/stm32mp257f-dk.dts     |  4 +-
 drivers/mmc/host/mmci.c                       |  5 +-
 drivers/mmc/host/mmci_stm32_sdmmc.c           | 28 +++++++--
 4 files changed, 27 insertions(+), 68 deletions(-)


base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.56.0

[PATCH v2 1/5] mmc: mmci: do not count a data CRC error as a complete transfer

[Mateusz Nowicki]

With datacnt_useless (STM32 variants) success is 0 on a data error and
a CRC error subtracts one from it. bytes_xfered becomes 0xfffffe00 and
mmc_blk completes the whole request as successful, although the
controller stopped at the failing block. Reads return stale data and
writes are lost without any error.

Tested on the STM32MP257F-DK eMMC in HS200 with a forced bad tuning:
without this change 5 of 14 reads returned zeros, with it all 10 reads
were correct.

Fixes: b79220b3e0af ("mmc: mmci: add variant property to not read datacnt")
Cc: stable@vger.kernel.org
Signed-off-by: Mateusz Nowicki <mateusz.nowicki@posteo.net>
---
 drivers/mmc/host/mmci.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 416bdb184ed4..10eb0439fc52 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1422,8 +1422,9 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
 		dev_dbg(mmc_dev(host->mmc), "MCI ERROR IRQ, status 0x%08x at 0x%08x\n",
 			status_err, success);
 		if (status_err & MCI_DATACRCFAIL) {
-			/* Last block was not successful */
-			success -= 1;
+			/* Last block was not successful, nothing counted when DATACNT is useless */
+			if (success)
+				success -= 1;
 			data->error = -EILSEQ;
 		} else if (status_err & MCI_DATATIMEOUT) {
 			data->error = -ETIMEDOUT;
-- 
2.56.0

[PATCH v2 2/5] mmc: mmci: stm32: release the delay block before enabling it

[Mateusz Nowicki]

From: Christophe Kerello <christophe.kerello@foss.st.com>

The delay block must be locked on the last frequency set. To avoid it
staying locked on a frequency used before, disable it first and wait for
the lock to drop before enabling it again.

Fixes: b5c3eb385797 ("mmc: mmci: stm32: add delay block support for STM32MP25")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
[Mateusz: reworded the commit message, added Fixes and Cc stable]
Signed-off-by: Mateusz Nowicki <mateusz.nowicki@posteo.net>
---

Notes:
    On the STM32MP257F-DK eMMC in HS200 at 150 MHz, switching from DDR52 to
    HS200 made the tuning pick a wrong phase in 9 of 18 tries without this
    change, the phase was right in all 12 tries with it.

 drivers/mmc/host/mmci_stm32_sdmmc.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/mmci_stm32_sdmmc.c b/drivers/mmc/host/mmci_stm32_sdmmc.c
index 9dc51859c2e5..bb058e0910ad 100644
--- a/drivers/mmc/host/mmci_stm32_sdmmc.c
+++ b/drivers/mmc/host/mmci_stm32_sdmmc.c
@@ -537,10 +537,20 @@ static int sdmmc_dlyb_mp15_prepare(struct mmci_host *host)
 static int sdmmc_dlyb_mp25_enable(struct sdmmc_dlyb *dlyb)
 {
 	u32 cr, sr;
+	int ret;
 
 	cr = readl_relaxed(dlyb->base + SYSCFG_DLYBSD_CR);
-	cr |= DLYBSD_CR_EN;
 
+	cr &= ~DLYBSD_CR_EN;
+	writel_relaxed(cr, dlyb->base + SYSCFG_DLYBSD_CR);
+
+	ret = readl_relaxed_poll_timeout(dlyb->base + SYSCFG_DLYBSD_SR,
+					 sr, !(sr & DLYBSD_SR_LOCK), 1,
+					 DLYBSD_TIMEOUT_1S_IN_US);
+	if (ret)
+		return ret;
+
+	cr |= DLYBSD_CR_EN;
 	writel_relaxed(cr, dlyb->base + SYSCFG_DLYBSD_CR);
 
 	return readl_relaxed_poll_timeout(dlyb->base + SYSCFG_DLYBSD_SR,
-- 
2.56.0

[PATCH v2 3/5] mmc: mmci: stm32: use the delay block above 100 MHz on stm32mp25 SoC

[Mateusz Nowicki]

From: Christophe Kerello <christophe.kerello@foss.st.com>

On stm32mp25 SoC, the delay block has to be used only for an external
data frequency above 100 MHz. Skip the tuning up to that frequency, keep
50 MHz for stm32mp15 as before.

Fixes: b5c3eb385797 ("mmc: mmci: stm32: add delay block support for STM32MP25")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
[Mateusz: reworded the commit message, check dlyb before reading
min_freq, added Fixes and Cc stable]
Signed-off-by: Mateusz Nowicki <mateusz.nowicki@posteo.net>
---

Notes:
    On the STM32MP257F-DK (SDMMC2 kernel clock 200 MHz, eMMC in HS200 at
    100 MHz) the writes failed with I/O errors in all 9 test runs without
    this change, with sdmmc2 pins_a or pins_b and with or without NEGEDGE.
    With it all 6 runs were correct, on both pin groups.

 drivers/mmc/host/mmci_stm32_sdmmc.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/mmci_stm32_sdmmc.c b/drivers/mmc/host/mmci_stm32_sdmmc.c
index bb058e0910ad..a50dc58336de 100644
--- a/drivers/mmc/host/mmci_stm32_sdmmc.c
+++ b/drivers/mmc/host/mmci_stm32_sdmmc.c
@@ -75,6 +75,7 @@ struct sdmmc_dlyb {
 	void __iomem *base;
 	u32 unit;
 	u32 max;
+	unsigned int min_freq;
 	struct sdmmc_tuning_ops *ops;
 };
 
@@ -636,14 +637,16 @@ static int sdmmc_execute_tuning(struct mmc_host *mmc, u32 opcode)
 	u32 clk;
 	int ret;
 
-	if ((host->mmc->ios.timing != MMC_TIMING_UHS_SDR104 &&
-	     host->mmc->ios.timing != MMC_TIMING_MMC_HS200) ||
-	    host->mmc->actual_clock <= 50000000)
+	if (host->mmc->ios.timing != MMC_TIMING_UHS_SDR104 &&
+	    host->mmc->ios.timing != MMC_TIMING_MMC_HS200)
 		return 0;
 
 	if (!dlyb || !dlyb->base)
 		return -EINVAL;
 
+	if (host->mmc->actual_clock <= dlyb->min_freq)
+		return 0;
+
 	ret = dlyb->ops->dlyb_enable(dlyb);
 	if (ret)
 		return ret;
@@ -749,10 +752,13 @@ void sdmmc_variant_init(struct mmci_host *host)
 		return;
 
 	dlyb->base = base_dlyb;
-	if (of_device_is_compatible(np, "st,stm32mp25-sdmmc2"))
+	if (of_device_is_compatible(np, "st,stm32mp25-sdmmc2")) {
 		dlyb->ops = &dlyb_tuning_mp25_ops;
-	else
+		dlyb->min_freq = 100000000;
+	} else {
 		dlyb->ops = &dlyb_tuning_mp15_ops;
+		dlyb->min_freq = 50000000;
+	}
 
 	host->variant_priv = dlyb;
 	host->mmc_ops->execute_tuning = sdmmc_execute_tuning;
-- 
2.56.0

[PATCH v2 4/5] arm64: dts: st: use sdmmc2 pins_a on stm32mp257f-dk

[Mateusz Nowicki]

The eMMC of the STM32MP257F-DK was moved to the faster sdmmc2 pins_b
groups because HS200 at 100 MHz returned corrupted data with pins_a. The
cause was the delay block, which was used at 100 MHz, not the slew rate
of the pins. With "mmc: mmci: stm32: use the delay block above 100 MHz
on stm32mp25 SoC" HS200 works with pins_a at 100 MHz, and at 150 MHz
with the delay block.

Use pins_a again. Their slower outputs avoid overshoots on the SDMMC2
clock, which can disturb other blocks such as USB.

Fixes: 94911ce62b45 ("arm64: dts: st: enable eMMC on stm32mp257f-dk")
Suggested-by: Christophe Kerello <christophe.kerello@foss.st.com>
Signed-off-by: Mateusz Nowicki <mateusz.nowicki@posteo.net>
---

Notes:
    This needs patch 3 of the series: without it HS200 at 100 MHz loses writes
    with pins_a. With it, the eMMC of the DK passed every 832 MiB write and
    read-back run on pins_a at 100 MHz and at 150 MHz (see the cover letter).

 arch/arm64/boot/dts/st/stm32mp257f-dk.dts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/st/stm32mp257f-dk.dts b/arch/arm64/boot/dts/st/stm32mp257f-dk.dts
index a8f18508db3e..3776bf915160 100644
--- a/arch/arm64/boot/dts/st/stm32mp257f-dk.dts
+++ b/arch/arm64/boot/dts/st/stm32mp257f-dk.dts
@@ -357,8 +357,8 @@ &sdmmc1 {
 
 &sdmmc2 {
 	pinctrl-names = "default", "opendrain", "sleep";
-	pinctrl-0 = <&sdmmc2_b4_pins_b &sdmmc2_d47_pins_b>;
-	pinctrl-1 = <&sdmmc2_b4_od_pins_b &sdmmc2_d47_pins_b>;
+	pinctrl-0 = <&sdmmc2_b4_pins_a &sdmmc2_d47_pins_a>;
+	pinctrl-1 = <&sdmmc2_b4_od_pins_a &sdmmc2_d47_pins_a>;
 	pinctrl-2 = <&sdmmc2_b4_sleep_pins_a &sdmmc2_d47_sleep_pins_a>;
 	non-removable;
 	no-sd;
-- 
2.56.0

[PATCH v2 5/5] arm64: dts: st: remove sdmmc2 pins_b from stm32mp25

[Mateusz Nowicki]

The sdmmc2 pins_b groups were only added for the eMMC of the
STM32MP257F-DK, which uses pins_a again. Nothing else references them,
remove them.

Suggested-by: Christophe Kerello <christophe.kerello@foss.st.com>
Signed-off-by: Mateusz Nowicki <mateusz.nowicki@posteo.net>
---
 arch/arm64/boot/dts/st/stm32mp25-pinctrl.dtsi | 58 -------------------
 1 file changed, 58 deletions(-)

diff --git a/arch/arm64/boot/dts/st/stm32mp25-pinctrl.dtsi b/arch/arm64/boot/dts/st/stm32mp25-pinctrl.dtsi
index 56c2c99a05d4..75d08fe33ec5 100644
--- a/arch/arm64/boot/dts/st/stm32mp25-pinctrl.dtsi
+++ b/arch/arm64/boot/dts/st/stm32mp25-pinctrl.dtsi
@@ -757,51 +757,6 @@ pins3 {
 		};
 	};
 
-	/omit-if-no-ref/
-	sdmmc2_b4_pins_b: sdmmc2-b4-1 {
-		pins1 {
-			pinmux = <STM32_PINMUX('E', 13, AF12)>, /* SDMMC2_D0 */
-				 <STM32_PINMUX('E', 11, AF12)>, /* SDMMC2_D1 */
-				 <STM32_PINMUX('E', 8, AF12)>, /* SDMMC2_D2 */
-				 <STM32_PINMUX('E', 12, AF12)>, /* SDMMC2_D3 */
-				 <STM32_PINMUX('E', 15, AF12)>; /* SDMMC2_CMD */
-			slew-rate = <2>;
-			drive-push-pull;
-			bias-pull-up;
-		};
-		pins2 {
-			pinmux = <STM32_PINMUX('E', 14, AF12)>; /* SDMMC2_CK */
-			slew-rate = <3>;
-			drive-push-pull;
-			bias-disable;
-		};
-	};
-
-	/omit-if-no-ref/
-	sdmmc2_b4_od_pins_b: sdmmc2-b4-od-1 {
-		pins1 {
-			pinmux = <STM32_PINMUX('E', 13, AF12)>, /* SDMMC2_D0 */
-				 <STM32_PINMUX('E', 11, AF12)>, /* SDMMC2_D1 */
-				 <STM32_PINMUX('E', 8, AF12)>, /* SDMMC2_D2 */
-				 <STM32_PINMUX('E', 12, AF12)>; /* SDMMC2_D3 */
-			slew-rate = <2>;
-			drive-push-pull;
-			bias-pull-up;
-		};
-		pins2 {
-			pinmux = <STM32_PINMUX('E', 14, AF12)>; /* SDMMC2_CK */
-			slew-rate = <3>;
-			drive-push-pull;
-			bias-disable;
-		};
-		pins3 {
-			pinmux = <STM32_PINMUX('E', 15, AF12)>; /* SDMMC2_CMD */
-			slew-rate = <2>;
-			drive-open-drain;
-			bias-pull-up;
-		};
-	};
-
 	/omit-if-no-ref/
 	sdmmc2_d47_pins_a: sdmmc2-d47-0 {
 		pins {
@@ -914,19 +869,6 @@ pins {
 		};
 	};
 
-	/omit-if-no-ref/
-	sdmmc2_d47_pins_b: sdmmc2-d47-1 {
-		pins {
-			pinmux = <STM32_PINMUX('E', 10, AF12)>, /* SDMMC2_D4 */
-				 <STM32_PINMUX('E', 9, AF12)>, /* SDMMC2_D5 */
-				 <STM32_PINMUX('E', 6, AF12)>, /* SDMMC2_D6 */
-				 <STM32_PINMUX('E', 7, AF12)>; /* SDMMC2_D7 */
-			slew-rate = <2>;
-			drive-push-pull;
-			bias-pull-up;
-		};
-	};
-
 	/omit-if-no-ref/
 	spi1_pins_a: spi1-0 {
 		pins1 {
-- 
2.56.0