* [PATCH v2 0/4] mmc: sdhci_am654: Fix DDR50 mode for cards without tuning support
@ 2026-08-03 13:47 Diogo Ivo (Schneider Electric)
2026-08-03 13:47 ` [PATCH v2 1/4] mmc: sdhci_am654: Move tuning_loop to local variable Diogo Ivo (Schneider Electric)
` (4 more replies)
0 siblings, 5 replies; 14+ messages in thread
From: Diogo Ivo (Schneider Electric) @ 2026-08-03 13:47 UTC (permalink / raw)
To: Adrian Hunter, Ulf Hansson, Faiz Abbas, Judith Mendez
Cc: linux-mmc, linux-kernel, Pascal EBERHARD, Thomas Petazzoni,
Miquel Raynal, Diogo Ivo (Schneider Electric), stable
DDR50 mode is not required to support the tuning command CMD19 by the
eMMC/SD specification, meaning that calibration may fail on cards that
do not implement it. This series fixes four issues discovered when
using such cards:
- Patch 1 moves the tuning_loop counter from a persistent struct field
to a local variable, preventing failed tuning attempts from depleting
the retry budget across calls.
- Patch 2 ensures the command/data line reset is always performed
after tuning, even on failure, preventing stale data in the
controller buffer.
- Patch 3 clears the ITAPDLY enable and delay values on tuning
failure so that leftover calibration values do not interfere with
subsequent I/O.
- Patch 4 falls back to the DT-provided itap delay value for DDR50
when tuning fails in this mode, making a best-effort attempt at a
known-good delay being programmed.
Together these changes make DDR50 functional on TI J721E SoCs with
cards that lack CMD19 tuning support. This second version addresses
all comments from Sashiko, which I found to be correct.
Signed-off-by: Diogo Ivo (Schneider Electric) <diogo.ivo@bootlin.com>
---
Changes in v2:
- Add new patch moving tuning_loop to a local variable (Patch 1)
- Clear ITAPDLY array values alongside HW registers on tuning failure (Patch 3)
- Save DT-provided itap delay values for DDR50 in dedicated variables
to preserve them across tuning cycles (Patch 4)
- Link to v1: https://patch.msgid.link/20260729-am654-sdhci-v1-0-7568108e4b9a@bootlin.com
---
Diogo Ivo (Schneider Electric) (4):
mmc: sdhci_am654: Move tuning_loop to local variable
mmc: sdhci_am654: Reset command and data lines on failed tuning
mmc: sdhci_am654: Clear ITAPDLY on tuning failure
mmc: sdhci_am654: Fallback to DT-provided itap delay on DDR50 tuning failure
drivers/mmc/host/sdhci_am654.c | 42 +++++++++++++++++++++++++++---------------
1 file changed, 27 insertions(+), 15 deletions(-)
---
base-commit: f932188d6b0b684ee8555c8ecf1cac3567c4b106
change-id: 20260728-am654-sdhci-85094d458bae
Best regards,
--
Diogo Ivo <diogo.ivo@bootlin.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/4] mmc: sdhci_am654: Move tuning_loop to local variable
2026-08-03 13:47 [PATCH v2 0/4] mmc: sdhci_am654: Fix DDR50 mode for cards without tuning support Diogo Ivo (Schneider Electric)
@ 2026-08-03 13:47 ` Diogo Ivo (Schneider Electric)
2026-08-03 19:19 ` Mendez, Judith
2026-08-03 13:47 ` [PATCH v2 2/4] mmc: sdhci_am654: Reset command and data lines on failed tuning Diogo Ivo (Schneider Electric)
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Diogo Ivo (Schneider Electric) @ 2026-08-03 13:47 UTC (permalink / raw)
To: Adrian Hunter, Ulf Hansson, Faiz Abbas, Judith Mendez
Cc: linux-mmc, linux-kernel, Pascal EBERHARD, Thomas Petazzoni,
Miquel Raynal, Diogo Ivo (Schneider Electric)
The tuning_loop field in struct sdhci_am654_data is only used within
sdhci_am654_platform_execute_tuning() as a loop counter that is
initialized to 0 in sdhci_am654_init(). Since it shouldn't persist across
function calls, otherwise every failure expends its "budget", move it to a
local variable and remove the struct field along with the now-unnecessary
initialization.
Signed-off-by: Diogo Ivo (Schneider Electric) <diogo.ivo@bootlin.com>
---
Changes in v2:
- New patch suggested by Sashiko
---
drivers/mmc/host/sdhci_am654.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index d235b0aecfdb..35c3efd92112 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -151,7 +151,6 @@ struct sdhci_am654_data {
u32 flags;
u32 quirks;
bool dll_enable;
- u32 tuning_loop;
#define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
#define SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA BIT(1)
@@ -576,13 +575,14 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
unsigned char timing = host->mmc->ios.timing;
struct device *dev = mmc_dev(host->mmc);
+ unsigned int tuning_loop = 0;
int itapdly;
do {
itapdly = sdhci_am654_do_tuning(host, opcode);
if (itapdly >= 0)
break;
- } while (++sdhci_am654->tuning_loop < RETRY_TUNING_MAX);
+ } while (++tuning_loop < RETRY_TUNING_MAX);
if (itapdly < 0) {
dev_err(dev, "Failed to find itapdly, fail tuning\n");
@@ -806,9 +806,6 @@ static int sdhci_am654_init(struct sdhci_host *host)
regmap_update_bits(sdhci_am654->base, CTL_CFG_3, TUNINGFORSDR50_MASK,
TUNINGFORSDR50_MASK);
- /* Use to re-execute tuning */
- sdhci_am654->tuning_loop = 0;
-
ret = sdhci_setup_host(host);
if (ret)
return ret;
--
2.55.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 2/4] mmc: sdhci_am654: Reset command and data lines on failed tuning
2026-08-03 13:47 [PATCH v2 0/4] mmc: sdhci_am654: Fix DDR50 mode for cards without tuning support Diogo Ivo (Schneider Electric)
2026-08-03 13:47 ` [PATCH v2 1/4] mmc: sdhci_am654: Move tuning_loop to local variable Diogo Ivo (Schneider Electric)
@ 2026-08-03 13:47 ` Diogo Ivo (Schneider Electric)
2026-08-03 19:28 ` Mendez, Judith
2026-08-03 13:47 ` [PATCH v2 3/4] mmc: sdhci_am654: Clear ITAPDLY on tuning failure Diogo Ivo (Schneider Electric)
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Diogo Ivo (Schneider Electric) @ 2026-08-03 13:47 UTC (permalink / raw)
To: Adrian Hunter, Ulf Hansson, Faiz Abbas, Judith Mendez
Cc: linux-mmc, linux-kernel, Pascal EBERHARD, Thomas Petazzoni,
Miquel Raynal, Diogo Ivo (Schneider Electric), stable
The CMD/DATA reset after tuning should be performed regardless of
whether tuning succeeded or failed, since tuning data may remain in
the buffer in either case. Move the error return after the reset so
that the controller is always cleaned up.
Fixes: de31f6ab68a3 ("mmc: sdhci_am654: Reset Command and Data line after tuning")
Cc: <stable@vger.kernel.org>
Signed-off-by: Diogo Ivo (Schneider Electric) <diogo.ivo@bootlin.com>
---
Changes in v2:
- No changes
---
drivers/mmc/host/sdhci_am654.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index 35c3efd92112..0db587e702ac 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -442,15 +442,13 @@ static int sdhci_am654_execute_tuning(struct mmc_host *mmc, u32 opcode)
struct sdhci_host *host = mmc_priv(mmc);
int err = sdhci_execute_tuning(mmc, opcode);
- if (err)
- return err;
/*
* Tuning data remains in the buffer after tuning.
* Do a command and data reset to get rid of it
*/
sdhci_reset(host, SDHCI_RESET_CMD | SDHCI_RESET_DATA);
- return 0;
+ return err;
}
static u32 sdhci_am654_cqhci_irq(struct sdhci_host *host, u32 intmask)
--
2.55.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 3/4] mmc: sdhci_am654: Clear ITAPDLY on tuning failure
2026-08-03 13:47 [PATCH v2 0/4] mmc: sdhci_am654: Fix DDR50 mode for cards without tuning support Diogo Ivo (Schneider Electric)
2026-08-03 13:47 ` [PATCH v2 1/4] mmc: sdhci_am654: Move tuning_loop to local variable Diogo Ivo (Schneider Electric)
2026-08-03 13:47 ` [PATCH v2 2/4] mmc: sdhci_am654: Reset command and data lines on failed tuning Diogo Ivo (Schneider Electric)
@ 2026-08-03 13:47 ` Diogo Ivo (Schneider Electric)
2026-08-03 19:40 ` Mendez, Judith
2026-08-03 13:47 ` [PATCH v2 4/4] mmc: sdhci_am654: Fallback to DT-provided itap delay on DDR50 " Diogo Ivo (Schneider Electric)
2026-08-07 8:16 ` [PATCH v2 0/4] mmc: sdhci_am654: Fix DDR50 mode for cards without tuning support Adrian Hunter
4 siblings, 1 reply; 14+ messages in thread
From: Diogo Ivo (Schneider Electric) @ 2026-08-03 13:47 UTC (permalink / raw)
To: Adrian Hunter, Ulf Hansson, Faiz Abbas, Judith Mendez
Cc: linux-mmc, linux-kernel, Pascal EBERHARD, Thomas Petazzoni,
Miquel Raynal, Diogo Ivo (Schneider Electric), stable
When tuning fails, stale ITAPDLY values can persist and interfere with
subsequent I/O accesses, for example in DDR50 mode in cards with no tuning
support. Move the ITAPDLY enable setting out of the tuning loop to after
successful tuning, and explicitly clear ITAPDLY (delay and enable) when
tuning fails so that we are sure only working values are actually left in
hardware.
Fixes: 901d16e46296 ("mmc: sdhci_am654: Add retry tuning")
Cc: <stable@vger.kernel.org>
Signed-off-by: Diogo Ivo (Schneider Electric) <diogo.ivo@bootlin.com>
---
Changes in v2:
- As suggested by Sashiko clear the values in itap_del_sel[] and itap_del_ena[]
so that they agree with what is written in HW and subsequent runtime_suspend()
and runtime_resume() can never write stale values.
- Remove now unused timing variable
---
drivers/mmc/host/sdhci_am654.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index 0db587e702ac..8ce10422bba0 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -527,7 +527,6 @@ static int sdhci_am654_do_tuning(struct sdhci_host *host,
{
struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
- unsigned char timing = host->mmc->ios.timing;
struct window fail_window[ITAPDLY_LENGTH];
struct device *dev = mmc_dev(host->mmc);
u8 curr_pass, itap;
@@ -536,11 +535,8 @@ static int sdhci_am654_do_tuning(struct sdhci_host *host,
memset(fail_window, 0, sizeof(fail_window));
- /* Enable ITAPDLY */
- sdhci_am654->itap_del_ena[timing] = 0x1;
-
for (itap = 0; itap < ITAPDLY_LENGTH; itap++) {
- sdhci_am654_write_itapdly(sdhci_am654, itap, sdhci_am654->itap_del_ena[timing]);
+ sdhci_am654_write_itapdly(sdhci_am654, itap, 0x1);
curr_pass = !mmc_send_tuning(host->mmc, opcode, NULL);
@@ -584,10 +580,16 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
if (itapdly < 0) {
dev_err(dev, "Failed to find itapdly, fail tuning\n");
+ sdhci_am654_write_itapdly(sdhci_am654, 0, 0);
+ sdhci_am654->itap_del_ena[timing] = 0;
+ sdhci_am654->itap_del_sel[timing] = 0;
return -1;
}
dev_dbg(dev, "Passed tuning, final itapdly=%d\n", itapdly);
+
+ /* Enable ITAPDLY */
+ sdhci_am654->itap_del_ena[timing] = 0x1;
sdhci_am654_write_itapdly(sdhci_am654, itapdly, sdhci_am654->itap_del_ena[timing]);
/* Save ITAPDLY */
sdhci_am654->itap_del_sel[timing] = itapdly;
--
2.55.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 4/4] mmc: sdhci_am654: Fallback to DT-provided itap delay on DDR50 tuning failure
2026-08-03 13:47 [PATCH v2 0/4] mmc: sdhci_am654: Fix DDR50 mode for cards without tuning support Diogo Ivo (Schneider Electric)
` (2 preceding siblings ...)
2026-08-03 13:47 ` [PATCH v2 3/4] mmc: sdhci_am654: Clear ITAPDLY on tuning failure Diogo Ivo (Schneider Electric)
@ 2026-08-03 13:47 ` Diogo Ivo (Schneider Electric)
2026-08-03 19:59 ` Mendez, Judith
2026-08-07 8:16 ` [PATCH v2 0/4] mmc: sdhci_am654: Fix DDR50 mode for cards without tuning support Adrian Hunter
4 siblings, 1 reply; 14+ messages in thread
From: Diogo Ivo (Schneider Electric) @ 2026-08-03 13:47 UTC (permalink / raw)
To: Adrian Hunter, Ulf Hansson, Faiz Abbas, Judith Mendez
Cc: linux-mmc, linux-kernel, Pascal EBERHARD, Thomas Petazzoni,
Miquel Raynal, Diogo Ivo (Schneider Electric), stable
DDR50 mode is not required to support the tuning command CMD19, meaning
that calibration may fail on cards that do not implement it, in which
case a known-good itap delay value should be programmed into the host
controller.
Do this by reading the (already defined) itap delay DT property for DDR50
and, if tuning fails for this mode, fall back to the DT-provided itap delay
value. If the DT does not provide a value for DDR50 fallback then this
simply disables using itapdly.
Fixes: 901d16e46296 ("mmc: sdhci_am654: Add retry tuning")
Cc: <stable@vger.kernel.org>
Signed-off-by: Diogo Ivo (Schneider Electric) <diogo.ivo@bootlin.com>
---
Changes in v2:
- As suggested by Sashiko save values read from DT for DDR50 in separate
variables in order to keep them across multiple tuning cycles without
overwriting them.
---
drivers/mmc/host/sdhci_am654.c | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index 8ce10422bba0..27ef79420eba 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -126,7 +126,7 @@ static const struct timing_data td[] = {
NULL,
MMC_CAP_UHS_SDR104},
[MMC_TIMING_UHS_DDR50] = {"ti,otap-del-sel-ddr50",
- NULL,
+ "ti,itap-del-sel-ddr50",
MMC_CAP_UHS_DDR50},
[MMC_TIMING_MMC_DDR52] = {"ti,otap-del-sel-ddr52",
"ti,itap-del-sel-ddr52",
@@ -144,6 +144,8 @@ struct sdhci_am654_data {
u32 otap_del_sel[ARRAY_SIZE(td)];
u32 itap_del_sel[ARRAY_SIZE(td)];
u32 itap_del_ena[ARRAY_SIZE(td)];
+ u32 itap_del_sel_dt_ddr50;
+ u32 itap_del_ena_dt_ddr50;
int clkbuf_sel;
int trm_icp;
int drv_strength;
@@ -579,10 +581,18 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
} while (++tuning_loop < RETRY_TUNING_MAX);
if (itapdly < 0) {
- dev_err(dev, "Failed to find itapdly, fail tuning\n");
- sdhci_am654_write_itapdly(sdhci_am654, 0, 0);
- sdhci_am654->itap_del_ena[timing] = 0;
- sdhci_am654->itap_del_sel[timing] = 0;
+ if (timing == MMC_TIMING_UHS_DDR50) {
+ sdhci_am654->itap_del_sel[timing] = sdhci_am654->itap_del_sel_dt_ddr50;
+ sdhci_am654->itap_del_ena[timing] = sdhci_am654->itap_del_ena_dt_ddr50;
+ } else {
+ dev_err(dev, "Failed to find itapdly, fail tuning\n");
+ sdhci_am654->itap_del_ena[timing] = 0;
+ sdhci_am654->itap_del_sel[timing] = 0;
+ }
+
+ sdhci_am654_write_itapdly(sdhci_am654,
+ sdhci_am654->itap_del_sel[timing],
+ sdhci_am654->itap_del_ena[timing]);
return -1;
}
@@ -758,6 +768,11 @@ static int sdhci_am654_get_otap_delay(struct sdhci_host *host,
}
}
+ sdhci_am654->itap_del_sel_dt_ddr50 =
+ sdhci_am654->itap_del_sel[MMC_TIMING_UHS_DDR50];
+ sdhci_am654->itap_del_ena_dt_ddr50 =
+ sdhci_am654->itap_del_ena[MMC_TIMING_UHS_DDR50];
+
return 0;
}
--
2.55.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/4] mmc: sdhci_am654: Move tuning_loop to local variable
2026-08-03 13:47 ` [PATCH v2 1/4] mmc: sdhci_am654: Move tuning_loop to local variable Diogo Ivo (Schneider Electric)
@ 2026-08-03 19:19 ` Mendez, Judith
0 siblings, 0 replies; 14+ messages in thread
From: Mendez, Judith @ 2026-08-03 19:19 UTC (permalink / raw)
To: Diogo Ivo (Schneider Electric), Adrian Hunter, Ulf Hansson,
Faiz Abbas
Cc: linux-mmc, linux-kernel, Pascal EBERHARD, Thomas Petazzoni,
Miquel Raynal
Hi Diogo,
On 8/3/2026 8:47 AM, Diogo Ivo (Schneider Electric) wrote:
> The tuning_loop field in struct sdhci_am654_data is only used within
> sdhci_am654_platform_execute_tuning() as a loop counter that is
> initialized to 0 in sdhci_am654_init(). Since it shouldn't persist across
> function calls, otherwise every failure expends its "budget", move it to a
> local variable and remove the struct field along with the now-unnecessary
> initialization.
>
> Signed-off-by: Diogo Ivo (Schneider Electric) <diogo.ivo@bootlin.com>
Thanks for your patch. Seems logical to me.
Reviewed-by: Judith Mendez <jm@ti.com>
> ---
> Changes in v2:
> - New patch suggested by Sashiko
> ---
> drivers/mmc/host/sdhci_am654.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index d235b0aecfdb..35c3efd92112 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -151,7 +151,6 @@ struct sdhci_am654_data {
> u32 flags;
> u32 quirks;
> bool dll_enable;
> - u32 tuning_loop;
>
> #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
> #define SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA BIT(1)
> @@ -576,13 +575,14 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
> struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
> unsigned char timing = host->mmc->ios.timing;
> struct device *dev = mmc_dev(host->mmc);
> + unsigned int tuning_loop = 0;
> int itapdly;
>
> do {
> itapdly = sdhci_am654_do_tuning(host, opcode);
> if (itapdly >= 0)
> break;
> - } while (++sdhci_am654->tuning_loop < RETRY_TUNING_MAX);
> + } while (++tuning_loop < RETRY_TUNING_MAX);
>
> if (itapdly < 0) {
> dev_err(dev, "Failed to find itapdly, fail tuning\n");
> @@ -806,9 +806,6 @@ static int sdhci_am654_init(struct sdhci_host *host)
> regmap_update_bits(sdhci_am654->base, CTL_CFG_3, TUNINGFORSDR50_MASK,
> TUNINGFORSDR50_MASK);
>
> - /* Use to re-execute tuning */
> - sdhci_am654->tuning_loop = 0;
> -
> ret = sdhci_setup_host(host);
> if (ret)
> return ret;
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/4] mmc: sdhci_am654: Reset command and data lines on failed tuning
2026-08-03 13:47 ` [PATCH v2 2/4] mmc: sdhci_am654: Reset command and data lines on failed tuning Diogo Ivo (Schneider Electric)
@ 2026-08-03 19:28 ` Mendez, Judith
0 siblings, 0 replies; 14+ messages in thread
From: Mendez, Judith @ 2026-08-03 19:28 UTC (permalink / raw)
To: Diogo Ivo (Schneider Electric), Adrian Hunter, Ulf Hansson
Cc: linux-mmc, linux-kernel, Pascal EBERHARD, Thomas Petazzoni,
Miquel Raynal, stable
Hi Diogo,
On 8/3/2026 8:47 AM, Diogo Ivo (Schneider Electric) wrote:
> The CMD/DATA reset after tuning should be performed regardless of
> whether tuning succeeded or failed, since tuning data may remain in
> the buffer in either case. Move the error return after the reset so
> that the controller is always cleaned up.
>
> Fixes: de31f6ab68a3 ("mmc: sdhci_am654: Reset Command and Data line after tuning")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Diogo Ivo (Schneider Electric) <diogo.ivo@bootlin.com>
Reviewed-by: Judith Mendez <jm@ti.com>
> ---
> Changes in v2:
> - No changes
> ---
> drivers/mmc/host/sdhci_am654.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 35c3efd92112..0db587e702ac 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -442,15 +442,13 @@ static int sdhci_am654_execute_tuning(struct mmc_host *mmc, u32 opcode)
> struct sdhci_host *host = mmc_priv(mmc);
> int err = sdhci_execute_tuning(mmc, opcode);
>
> - if (err)
> - return err;
> /*
> * Tuning data remains in the buffer after tuning.
> * Do a command and data reset to get rid of it
> */
> sdhci_reset(host, SDHCI_RESET_CMD | SDHCI_RESET_DATA);
>
> - return 0;
> + return err;
> }
>
> static u32 sdhci_am654_cqhci_irq(struct sdhci_host *host, u32 intmask)
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/4] mmc: sdhci_am654: Clear ITAPDLY on tuning failure
2026-08-03 13:47 ` [PATCH v2 3/4] mmc: sdhci_am654: Clear ITAPDLY on tuning failure Diogo Ivo (Schneider Electric)
@ 2026-08-03 19:40 ` Mendez, Judith
0 siblings, 0 replies; 14+ messages in thread
From: Mendez, Judith @ 2026-08-03 19:40 UTC (permalink / raw)
To: Diogo Ivo (Schneider Electric), Adrian Hunter, Ulf Hansson
Cc: linux-mmc, linux-kernel, Pascal EBERHARD, Thomas Petazzoni,
Miquel Raynal, stable
Hi Diogo,
Thanks for the fix.
On 8/3/2026 8:47 AM, Diogo Ivo (Schneider Electric) wrote:
> When tuning fails, stale ITAPDLY values can persist and interfere with
> subsequent I/O accesses, for example in DDR50 mode in cards with no tuning
> support. Move the ITAPDLY enable setting out of the tuning loop to after
> successful tuning, and explicitly clear ITAPDLY (delay and enable) when
> tuning fails so that we are sure only working values are actually left in
> hardware.
>
> Fixes: 901d16e46296 ("mmc: sdhci_am654: Add retry tuning")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Diogo Ivo (Schneider Electric) <diogo.ivo@bootlin.com>
Reviewed-by: Judith Mendez <jm@ti.com>
> ---
> Changes in v2:
> - As suggested by Sashiko clear the values in itap_del_sel[] and itap_del_ena[]
> so that they agree with what is written in HW and subsequent runtime_suspend()
> and runtime_resume() can never write stale values.
> - Remove now unused timing variable
> ---
> drivers/mmc/host/sdhci_am654.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 0db587e702ac..8ce10422bba0 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -527,7 +527,6 @@ static int sdhci_am654_do_tuning(struct sdhci_host *host,
> {
> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
> - unsigned char timing = host->mmc->ios.timing;
> struct window fail_window[ITAPDLY_LENGTH];
> struct device *dev = mmc_dev(host->mmc);
> u8 curr_pass, itap;
> @@ -536,11 +535,8 @@ static int sdhci_am654_do_tuning(struct sdhci_host *host,
>
> memset(fail_window, 0, sizeof(fail_window));
>
> - /* Enable ITAPDLY */
> - sdhci_am654->itap_del_ena[timing] = 0x1;
> -
> for (itap = 0; itap < ITAPDLY_LENGTH; itap++) {
> - sdhci_am654_write_itapdly(sdhci_am654, itap, sdhci_am654->itap_del_ena[timing]);
> + sdhci_am654_write_itapdly(sdhci_am654, itap, 0x1);
>
> curr_pass = !mmc_send_tuning(host->mmc, opcode, NULL);
>
> @@ -584,10 +580,16 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
>
> if (itapdly < 0) {
> dev_err(dev, "Failed to find itapdly, fail tuning\n");
> + sdhci_am654_write_itapdly(sdhci_am654, 0, 0);
> + sdhci_am654->itap_del_ena[timing] = 0;
> + sdhci_am654->itap_del_sel[timing] = 0;
> return -1;
> }
>
> dev_dbg(dev, "Passed tuning, final itapdly=%d\n", itapdly);
> +
> + /* Enable ITAPDLY */
> + sdhci_am654->itap_del_ena[timing] = 0x1;
> sdhci_am654_write_itapdly(sdhci_am654, itapdly, sdhci_am654->itap_del_ena[timing]);
> /* Save ITAPDLY */
> sdhci_am654->itap_del_sel[timing] = itapdly;
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 4/4] mmc: sdhci_am654: Fallback to DT-provided itap delay on DDR50 tuning failure
2026-08-03 13:47 ` [PATCH v2 4/4] mmc: sdhci_am654: Fallback to DT-provided itap delay on DDR50 " Diogo Ivo (Schneider Electric)
@ 2026-08-03 19:59 ` Mendez, Judith
2026-08-04 8:45 ` Diogo Ivo
0 siblings, 1 reply; 14+ messages in thread
From: Mendez, Judith @ 2026-08-03 19:59 UTC (permalink / raw)
To: Diogo Ivo (Schneider Electric), Adrian Hunter, Ulf Hansson,
Faiz Abbas
Cc: linux-mmc, linux-kernel, Pascal EBERHARD, Thomas Petazzoni,
Miquel Raynal, stable
Hi Diogo,
Thanks for your patch, couple of doubts below.
On 8/3/2026 8:47 AM, Diogo Ivo (Schneider Electric) wrote:
> DDR50 mode is not required to support the tuning command CMD19, meaning
> that calibration may fail on cards that do not implement it, in which
> case a known-good itap delay value should be programmed into the host
> controller.
>
> Do this by reading the (already defined) itap delay DT property for DDR50
> and, if tuning fails for this mode, fall back to the DT-provided itap delay
> value. If the DT does not provide a value for DDR50 fallback then this
> simply disables using itapdly.
>
> Fixes: 901d16e46296 ("mmc: sdhci_am654: Add retry tuning")
Should this commit be the one to blame or should it be the commit that
introduces DDR50 support?
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Diogo Ivo (Schneider Electric) <diogo.ivo@bootlin.com>
> ---
> Changes in v2:
> - As suggested by Sashiko save values read from DT for DDR50 in separate
> variables in order to keep them across multiple tuning cycles without
> overwriting them.
> ---
> drivers/mmc/host/sdhci_am654.c | 25 ++++++++++++++++++++-----
> 1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 8ce10422bba0..27ef79420eba 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -126,7 +126,7 @@ static const struct timing_data td[] = {
> NULL,
> MMC_CAP_UHS_SDR104},
> [MMC_TIMING_UHS_DDR50] = {"ti,otap-del-sel-ddr50",
> - NULL,
> + "ti,itap-del-sel-ddr50",
> MMC_CAP_UHS_DDR50},
> [MMC_TIMING_MMC_DDR52] = {"ti,otap-del-sel-ddr52",
> "ti,itap-del-sel-ddr52",
> @@ -144,6 +144,8 @@ struct sdhci_am654_data {
> u32 otap_del_sel[ARRAY_SIZE(td)];
> u32 itap_del_sel[ARRAY_SIZE(td)];
> u32 itap_del_ena[ARRAY_SIZE(td)];
> + u32 itap_del_sel_dt_ddr50;
> + u32 itap_del_ena_dt_ddr50;
> int clkbuf_sel;
> int trm_icp;
> int drv_strength;
> @@ -579,10 +581,18 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
> } while (++tuning_loop < RETRY_TUNING_MAX);
>
> if (itapdly < 0) {
> - dev_err(dev, "Failed to find itapdly, fail tuning\n");
> - sdhci_am654_write_itapdly(sdhci_am654, 0, 0);
> - sdhci_am654->itap_del_ena[timing] = 0;
> - sdhci_am654->itap_del_sel[timing] = 0;
> + if (timing == MMC_TIMING_UHS_DDR50) {
Should dev_dbg be added here to let the user know that tuning failed but
DT defaults were set?
~ Judith
> + sdhci_am654->itap_del_sel[timing] = sdhci_am654->itap_del_sel_dt_ddr50;
> + sdhci_am654->itap_del_ena[timing] = sdhci_am654->itap_del_ena_dt_ddr50;
> + } else {
> + dev_err(dev, "Failed to find itapdly, fail tuning\n");
> + sdhci_am654->itap_del_ena[timing] = 0;
> + sdhci_am654->itap_del_sel[timing] = 0;
> + }
> +
> + sdhci_am654_write_itapdly(sdhci_am654,
> + sdhci_am654->itap_del_sel[timing],
> + sdhci_am654->itap_del_ena[timing]);
> return -1;
> }
>
> @@ -758,6 +768,11 @@ static int sdhci_am654_get_otap_delay(struct sdhci_host *host,
> }
> }
>
> + sdhci_am654->itap_del_sel_dt_ddr50 =
> + sdhci_am654->itap_del_sel[MMC_TIMING_UHS_DDR50];
> + sdhci_am654->itap_del_ena_dt_ddr50 =
> + sdhci_am654->itap_del_ena[MMC_TIMING_UHS_DDR50];
> +
> return 0;
> }
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 4/4] mmc: sdhci_am654: Fallback to DT-provided itap delay on DDR50 tuning failure
2026-08-03 19:59 ` Mendez, Judith
@ 2026-08-04 8:45 ` Diogo Ivo
2026-08-05 23:13 ` Mendez, Judith
0 siblings, 1 reply; 14+ messages in thread
From: Diogo Ivo @ 2026-08-04 8:45 UTC (permalink / raw)
To: Mendez, Judith, Adrian Hunter, Ulf Hansson, Faiz Abbas
Cc: linux-mmc, linux-kernel, Pascal EBERHARD, Thomas Petazzoni,
Miquel Raynal, stable
Hi Judith,
Thanks for the review!
On 8/3/26 9:59 PM, Mendez, Judith wrote:
> Hi Diogo,
>
> Thanks for your patch, couple of doubts below.
>
> On 8/3/2026 8:47 AM, Diogo Ivo (Schneider Electric) wrote:
>> DDR50 mode is not required to support the tuning command CMD19, meaning
>> that calibration may fail on cards that do not implement it, in which
>> case a known-good itap delay value should be programmed into the host
>> controller.
>>
>> Do this by reading the (already defined) itap delay DT property for DDR50
>> and, if tuning fails for this mode, fall back to the DT-provided itap
>> delay
>> value. If the DT does not provide a value for DDR50 fallback then this
>> simply disables using itapdly.
>>
>> Fixes: 901d16e46296 ("mmc: sdhci_am654: Add retry tuning")
>
> Should this commit be the one to blame or should it be the commit that
> introduces DDR50 support?
I'm not sure I follow you here, as I wasn't able to pinpoint a commit
that specifically introduced support for DDR50. Could you please clarify?
Taking a look at the git log of the driver I see a0a62497f6aa, where
reading itapdly from DT was added but without reading the DDR50 value
and 13ebeae68ac9, the commit where tuning was originally added, but the
"responsibility" seems split between the two.
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Diogo Ivo (Schneider Electric) <diogo.ivo@bootlin.com>
>> ---
>> Changes in v2:
>> - As suggested by Sashiko save values read from DT for DDR50 in
>> separate
>> variables in order to keep them across multiple tuning cycles without
>> overwriting them.
>> ---
>> drivers/mmc/host/sdhci_am654.c | 25 ++++++++++++++++++++-----
>> 1 file changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/
>> sdhci_am654.c
>> index 8ce10422bba0..27ef79420eba 100644
>> --- a/drivers/mmc/host/sdhci_am654.c
>> +++ b/drivers/mmc/host/sdhci_am654.c
>> @@ -126,7 +126,7 @@ static const struct timing_data td[] = {
>> NULL,
>> MMC_CAP_UHS_SDR104},
>> [MMC_TIMING_UHS_DDR50] = {"ti,otap-del-sel-ddr50",
>> - NULL,
>> + "ti,itap-del-sel-ddr50",
>> MMC_CAP_UHS_DDR50},
>> [MMC_TIMING_MMC_DDR52] = {"ti,otap-del-sel-ddr52",
>> "ti,itap-del-sel-ddr52",
>> @@ -144,6 +144,8 @@ struct sdhci_am654_data {
>> u32 otap_del_sel[ARRAY_SIZE(td)];
>> u32 itap_del_sel[ARRAY_SIZE(td)];
>> u32 itap_del_ena[ARRAY_SIZE(td)];
>> + u32 itap_del_sel_dt_ddr50;
>> + u32 itap_del_ena_dt_ddr50;
>> int clkbuf_sel;
>> int trm_icp;
>> int drv_strength;
>> @@ -579,10 +581,18 @@ static int
>> sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
>> } while (++tuning_loop < RETRY_TUNING_MAX);
>> if (itapdly < 0) {
>> - dev_err(dev, "Failed to find itapdly, fail tuning\n");
>> - sdhci_am654_write_itapdly(sdhci_am654, 0, 0);
>> - sdhci_am654->itap_del_ena[timing] = 0;
>> - sdhci_am654->itap_del_sel[timing] = 0;
>> + if (timing == MMC_TIMING_UHS_DDR50) {
>
> Should dev_dbg be added here to let the user know that tuning failed but
> DT defaults were set?
That sounds useful, I'll add a print here.
> ~ Judith
>
Best regards,
Diogo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 4/4] mmc: sdhci_am654: Fallback to DT-provided itap delay on DDR50 tuning failure
2026-08-04 8:45 ` Diogo Ivo
@ 2026-08-05 23:13 ` Mendez, Judith
2026-08-07 6:35 ` Adrian Hunter
0 siblings, 1 reply; 14+ messages in thread
From: Mendez, Judith @ 2026-08-05 23:13 UTC (permalink / raw)
To: Diogo Ivo, Adrian Hunter, Ulf Hansson, Faiz Abbas
Cc: linux-mmc, linux-kernel, Pascal EBERHARD, Thomas Petazzoni,
Miquel Raynal, stable
Hi Diogo,
On 8/4/2026 3:45 AM, Diogo Ivo wrote:
> Hi Judith,
>
> Thanks for the review!
>
> On 8/3/26 9:59 PM, Mendez, Judith wrote:
>> Hi Diogo,
>>
>> Thanks for your patch, couple of doubts below.
>>
>> On 8/3/2026 8:47 AM, Diogo Ivo (Schneider Electric) wrote:
>>> DDR50 mode is not required to support the tuning command CMD19, meaning
>>> that calibration may fail on cards that do not implement it, in which
>>> case a known-good itap delay value should be programmed into the host
>>> controller.
>>>
>>> Do this by reading the (already defined) itap delay DT property for
>>> DDR50
>>> and, if tuning fails for this mode, fall back to the DT-provided itap
>>> delay
>>> value. If the DT does not provide a value for DDR50 fallback then this
>>> simply disables using itapdly.
>>>
>>> Fixes: 901d16e46296 ("mmc: sdhci_am654: Add retry tuning")
>>
>> Should this commit be the one to blame or should it be the commit that
>> introduces DDR50 support?
>
> I'm not sure I follow you here, as I wasn't able to pinpoint a commit
> that specifically introduced support for DDR50. Could you please clarify?
>
> Taking a look at the git log of the driver I see a0a62497f6aa, where
> reading itapdly from DT was added but without reading the DDR50 value
> and 13ebeae68ac9, the commit where tuning was originally added, but the
> "responsibility" seems split between the two.
Yea I think because each of the three commits in question are adding
support for something else. This bug fix that you are sending was never
really discovered or debugged before. IMO (: Not sure if this requires
a fixes tag then.
I will let Adrian or someone else decide what to do here. ^.^
~ Judith
>
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Diogo Ivo (Schneider Electric) <diogo.ivo@bootlin.com>
>>> ---
>>> Changes in v2:
>>> - As suggested by Sashiko save values read from DT for DDR50 in
>>> separate
>>> variables in order to keep them across multiple tuning cycles
>>> without
>>> overwriting them.
>>> ---
>>> drivers/mmc/host/sdhci_am654.c | 25 ++++++++++++++++++++-----
>>> 1 file changed, 20 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/
>>> sdhci_am654.c
>>> index 8ce10422bba0..27ef79420eba 100644
>>> --- a/drivers/mmc/host/sdhci_am654.c
>>> +++ b/drivers/mmc/host/sdhci_am654.c
>>> @@ -126,7 +126,7 @@ static const struct timing_data td[] = {
>>> NULL,
>>> MMC_CAP_UHS_SDR104},
>>> [MMC_TIMING_UHS_DDR50] = {"ti,otap-del-sel-ddr50",
>>> - NULL,
>>> + "ti,itap-del-sel-ddr50",
>>> MMC_CAP_UHS_DDR50},
>>> [MMC_TIMING_MMC_DDR52] = {"ti,otap-del-sel-ddr52",
>>> "ti,itap-del-sel-ddr52",
>>> @@ -144,6 +144,8 @@ struct sdhci_am654_data {
>>> u32 otap_del_sel[ARRAY_SIZE(td)];
>>> u32 itap_del_sel[ARRAY_SIZE(td)];
>>> u32 itap_del_ena[ARRAY_SIZE(td)];
>>> + u32 itap_del_sel_dt_ddr50;
>>> + u32 itap_del_ena_dt_ddr50;
>>> int clkbuf_sel;
>>> int trm_icp;
>>> int drv_strength;
>>> @@ -579,10 +581,18 @@ static int
>>> sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
>>> } while (++tuning_loop < RETRY_TUNING_MAX);
>>> if (itapdly < 0) {
>>> - dev_err(dev, "Failed to find itapdly, fail tuning\n");
>>> - sdhci_am654_write_itapdly(sdhci_am654, 0, 0);
>>> - sdhci_am654->itap_del_ena[timing] = 0;
>>> - sdhci_am654->itap_del_sel[timing] = 0;
>>> + if (timing == MMC_TIMING_UHS_DDR50) {
>>
>> Should dev_dbg be added here to let the user know that tuning failed but
>> DT defaults were set?
>
> That sounds useful, I'll add a print here.
>
>> ~ Judith
>>
>
> Best regards,
> Diogo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 4/4] mmc: sdhci_am654: Fallback to DT-provided itap delay on DDR50 tuning failure
2026-08-05 23:13 ` Mendez, Judith
@ 2026-08-07 6:35 ` Adrian Hunter
2026-08-07 10:52 ` Diogo Ivo
0 siblings, 1 reply; 14+ messages in thread
From: Adrian Hunter @ 2026-08-07 6:35 UTC (permalink / raw)
To: Mendez, Judith, Diogo Ivo, Ulf Hansson, Faiz Abbas
Cc: linux-mmc, linux-kernel, Pascal EBERHARD, Thomas Petazzoni,
Miquel Raynal, stable
On 06/08/2026 02:13, Mendez, Judith wrote:
> Hi Diogo,
>
> On 8/4/2026 3:45 AM, Diogo Ivo wrote:
>> Hi Judith,
>>
>> Thanks for the review!
>>
>> On 8/3/26 9:59 PM, Mendez, Judith wrote:
>>> Hi Diogo,
>>>
>>> Thanks for your patch, couple of doubts below.
>>>
>>> On 8/3/2026 8:47 AM, Diogo Ivo (Schneider Electric) wrote:
>>>> DDR50 mode is not required to support the tuning command CMD19, meaning
>>>> that calibration may fail on cards that do not implement it, in which
>>>> case a known-good itap delay value should be programmed into the host
>>>> controller.
>>>>
>>>> Do this by reading the (already defined) itap delay DT property for DDR50
>>>> and, if tuning fails for this mode, fall back to the DT-provided itap delay
>>>> value. If the DT does not provide a value for DDR50 fallback then this
>>>> simply disables using itapdly.
>>>>
>>>> Fixes: 901d16e46296 ("mmc: sdhci_am654: Add retry tuning")
>>>
>>> Should this commit be the one to blame or should it be the commit that
>>> introduces DDR50 support?
>>
>> I'm not sure I follow you here, as I wasn't able to pinpoint a commit
>> that specifically introduced support for DDR50. Could you please clarify?
>>
>> Taking a look at the git log of the driver I see a0a62497f6aa, where
>> reading itapdly from DT was added but without reading the DDR50 value
>> and 13ebeae68ac9, the commit where tuning was originally added, but the
>> "responsibility" seems split between the two.
>
> Yea I think because each of the three commits in question are adding
> support for something else. This bug fix that you are sending was never
> really discovered or debugged before. IMO (: Not sure if this requires
> a fixes tag then.
>
> I will let Adrian or someone else decide what to do here. ^.^
The Fixes tag tells people whether they might need to backport the
patch. It says "if you need Fixes commit then you need this commit too".
It should be the first commit where DDR50 could fail because of this issue.
>
> ~ Judith
>
>>
>>>> Cc: <stable@vger.kernel.org>
>>>> Signed-off-by: Diogo Ivo (Schneider Electric) <diogo.ivo@bootlin.com>
>>>> ---
>>>> Changes in v2:
>>>> - As suggested by Sashiko save values read from DT for DDR50 in separate
>>>> variables in order to keep them across multiple tuning cycles without
>>>> overwriting them.
>>>> ---
>>>> drivers/mmc/host/sdhci_am654.c | 25 ++++++++++++++++++++-----
>>>> 1 file changed, 20 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/ sdhci_am654.c
>>>> index 8ce10422bba0..27ef79420eba 100644
>>>> --- a/drivers/mmc/host/sdhci_am654.c
>>>> +++ b/drivers/mmc/host/sdhci_am654.c
>>>> @@ -126,7 +126,7 @@ static const struct timing_data td[] = {
>>>> NULL,
>>>> MMC_CAP_UHS_SDR104},
>>>> [MMC_TIMING_UHS_DDR50] = {"ti,otap-del-sel-ddr50",
>>>> - NULL,
>>>> + "ti,itap-del-sel-ddr50",
>>>> MMC_CAP_UHS_DDR50},
>>>> [MMC_TIMING_MMC_DDR52] = {"ti,otap-del-sel-ddr52",
>>>> "ti,itap-del-sel-ddr52",
>>>> @@ -144,6 +144,8 @@ struct sdhci_am654_data {
>>>> u32 otap_del_sel[ARRAY_SIZE(td)];
>>>> u32 itap_del_sel[ARRAY_SIZE(td)];
>>>> u32 itap_del_ena[ARRAY_SIZE(td)];
>>>> + u32 itap_del_sel_dt_ddr50;
>>>> + u32 itap_del_ena_dt_ddr50;
>>>> int clkbuf_sel;
>>>> int trm_icp;
>>>> int drv_strength;
>>>> @@ -579,10 +581,18 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
>>>> } while (++tuning_loop < RETRY_TUNING_MAX);
>>>> if (itapdly < 0) {
>>>> - dev_err(dev, "Failed to find itapdly, fail tuning\n");
>>>> - sdhci_am654_write_itapdly(sdhci_am654, 0, 0);
>>>> - sdhci_am654->itap_del_ena[timing] = 0;
>>>> - sdhci_am654->itap_del_sel[timing] = 0;
>>>> + if (timing == MMC_TIMING_UHS_DDR50) {
>>>
>>> Should dev_dbg be added here to let the user know that tuning failed but
>>> DT defaults were set?
>>
>> That sounds useful, I'll add a print here.
>>
>>> ~ Judith
>>>
>>
>> Best regards,
>> Diogo
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 0/4] mmc: sdhci_am654: Fix DDR50 mode for cards without tuning support
2026-08-03 13:47 [PATCH v2 0/4] mmc: sdhci_am654: Fix DDR50 mode for cards without tuning support Diogo Ivo (Schneider Electric)
` (3 preceding siblings ...)
2026-08-03 13:47 ` [PATCH v2 4/4] mmc: sdhci_am654: Fallback to DT-provided itap delay on DDR50 " Diogo Ivo (Schneider Electric)
@ 2026-08-07 8:16 ` Adrian Hunter
4 siblings, 0 replies; 14+ messages in thread
From: Adrian Hunter @ 2026-08-07 8:16 UTC (permalink / raw)
To: Diogo Ivo (Schneider Electric), Ulf Hansson, Faiz Abbas,
Judith Mendez
Cc: linux-mmc, linux-kernel, Pascal EBERHARD, Thomas Petazzoni,
Miquel Raynal, stable
On 03/08/2026 16:47, Diogo Ivo (Schneider Electric) wrote:
> DDR50 mode is not required to support the tuning command CMD19 by the
> eMMC/SD specification, meaning that calibration may fail on cards that
> do not implement it. This series fixes four issues discovered when
> using such cards:
>
> - Patch 1 moves the tuning_loop counter from a persistent struct field
> to a local variable, preventing failed tuning attempts from depleting
> the retry budget across calls.
>
> - Patch 2 ensures the command/data line reset is always performed
> after tuning, even on failure, preventing stale data in the
> controller buffer.
>
> - Patch 3 clears the ITAPDLY enable and delay values on tuning
> failure so that leftover calibration values do not interfere with
> subsequent I/O.
>
> - Patch 4 falls back to the DT-provided itap delay value for DDR50
> when tuning fails in this mode, making a best-effort attempt at a
> known-good delay being programmed.
>
> Together these changes make DDR50 functional on TI J721E SoCs with
> cards that lack CMD19 tuning support. This second version addresses
> all comments from Sashiko, which I found to be correct.
>
> Signed-off-by: Diogo Ivo (Schneider Electric) <diogo.ivo@bootlin.com>
For all 4:
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
> Changes in v2:
> - Add new patch moving tuning_loop to a local variable (Patch 1)
> - Clear ITAPDLY array values alongside HW registers on tuning failure (Patch 3)
> - Save DT-provided itap delay values for DDR50 in dedicated variables
> to preserve them across tuning cycles (Patch 4)
> - Link to v1: https://patch.msgid.link/20260729-am654-sdhci-v1-0-7568108e4b9a@bootlin.com
>
> ---
> Diogo Ivo (Schneider Electric) (4):
> mmc: sdhci_am654: Move tuning_loop to local variable
> mmc: sdhci_am654: Reset command and data lines on failed tuning
> mmc: sdhci_am654: Clear ITAPDLY on tuning failure
> mmc: sdhci_am654: Fallback to DT-provided itap delay on DDR50 tuning failure
>
> drivers/mmc/host/sdhci_am654.c | 42 +++++++++++++++++++++++++++---------------
> 1 file changed, 27 insertions(+), 15 deletions(-)
> ---
> base-commit: f932188d6b0b684ee8555c8ecf1cac3567c4b106
> change-id: 20260728-am654-sdhci-85094d458bae
>
> Best regards,
> --
> Diogo Ivo <diogo.ivo@bootlin.com>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 4/4] mmc: sdhci_am654: Fallback to DT-provided itap delay on DDR50 tuning failure
2026-08-07 6:35 ` Adrian Hunter
@ 2026-08-07 10:52 ` Diogo Ivo
0 siblings, 0 replies; 14+ messages in thread
From: Diogo Ivo @ 2026-08-07 10:52 UTC (permalink / raw)
To: Adrian Hunter, Mendez, Judith, Ulf Hansson, Faiz Abbas
Cc: linux-mmc, linux-kernel, Pascal EBERHARD, Thomas Petazzoni,
Miquel Raynal, stable
Hi Adrian,
On 8/7/26 8:35 AM, Adrian Hunter wrote:
> On 06/08/2026 02:13, Mendez, Judith wrote:
>> Hi Diogo,
>>
>> On 8/4/2026 3:45 AM, Diogo Ivo wrote:
>>> Hi Judith,
>>>
>>> Thanks for the review!
>>>
>>> On 8/3/26 9:59 PM, Mendez, Judith wrote:
>>>> Hi Diogo,
>>>>
>>>> Thanks for your patch, couple of doubts below.
>>>>
>>>> On 8/3/2026 8:47 AM, Diogo Ivo (Schneider Electric) wrote:
>>>>> DDR50 mode is not required to support the tuning command CMD19, meaning
>>>>> that calibration may fail on cards that do not implement it, in which
>>>>> case a known-good itap delay value should be programmed into the host
>>>>> controller.
>>>>>
>>>>> Do this by reading the (already defined) itap delay DT property for DDR50
>>>>> and, if tuning fails for this mode, fall back to the DT-provided itap delay
>>>>> value. If the DT does not provide a value for DDR50 fallback then this
>>>>> simply disables using itapdly.
>>>>>
>>>>> Fixes: 901d16e46296 ("mmc: sdhci_am654: Add retry tuning")
>>>>
>>>> Should this commit be the one to blame or should it be the commit that
>>>> introduces DDR50 support?
>>>
>>> I'm not sure I follow you here, as I wasn't able to pinpoint a commit
>>> that specifically introduced support for DDR50. Could you please clarify?
>>>
>>> Taking a look at the git log of the driver I see a0a62497f6aa, where
>>> reading itapdly from DT was added but without reading the DDR50 value
>>> and 13ebeae68ac9, the commit where tuning was originally added, but the
>>> "responsibility" seems split between the two.
>>
>> Yea I think because each of the three commits in question are adding
>> support for something else. This bug fix that you are sending was never
>> really discovered or debugged before. IMO (: Not sure if this requires
>> a fixes tag then.
>>
>> I will let Adrian or someone else decide what to do here. ^.^
>
> The Fixes tag tells people whether they might need to backport the
> patch. It says "if you need Fixes commit then you need this commit too".
>
> It should be the first commit where DDR50 could fail because of this issue.
Sounds good. In that case commit 901d16e46296 ("mmc: sdhci_am654: Add retry
tuning") is the correct commit, since before it the code was writing itap=0
in the case where tuning failed. I have also tested the driver before and
after 901d16e46296 and it is indeed the commit that breaks I/O. I will
send a v3 with the debug print suggested by Judith.
Best regards,
Diogo
>>
>> ~ Judith
>>
>>>
>>>>> Cc: <stable@vger.kernel.org>
>>>>> Signed-off-by: Diogo Ivo (Schneider Electric) <diogo.ivo@bootlin.com>
>>>>> ---
>>>>> Changes in v2:
>>>>> - As suggested by Sashiko save values read from DT for DDR50 in separate
>>>>> variables in order to keep them across multiple tuning cycles without
>>>>> overwriting them.
>>>>> ---
>>>>> drivers/mmc/host/sdhci_am654.c | 25 ++++++++++++++++++++-----
>>>>> 1 file changed, 20 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/ sdhci_am654.c
>>>>> index 8ce10422bba0..27ef79420eba 100644
>>>>> --- a/drivers/mmc/host/sdhci_am654.c
>>>>> +++ b/drivers/mmc/host/sdhci_am654.c
>>>>> @@ -126,7 +126,7 @@ static const struct timing_data td[] = {
>>>>> NULL,
>>>>> MMC_CAP_UHS_SDR104},
>>>>> [MMC_TIMING_UHS_DDR50] = {"ti,otap-del-sel-ddr50",
>>>>> - NULL,
>>>>> + "ti,itap-del-sel-ddr50",
>>>>> MMC_CAP_UHS_DDR50},
>>>>> [MMC_TIMING_MMC_DDR52] = {"ti,otap-del-sel-ddr52",
>>>>> "ti,itap-del-sel-ddr52",
>>>>> @@ -144,6 +144,8 @@ struct sdhci_am654_data {
>>>>> u32 otap_del_sel[ARRAY_SIZE(td)];
>>>>> u32 itap_del_sel[ARRAY_SIZE(td)];
>>>>> u32 itap_del_ena[ARRAY_SIZE(td)];
>>>>> + u32 itap_del_sel_dt_ddr50;
>>>>> + u32 itap_del_ena_dt_ddr50;
>>>>> int clkbuf_sel;
>>>>> int trm_icp;
>>>>> int drv_strength;
>>>>> @@ -579,10 +581,18 @@ static int sdhci_am654_platform_execute_tuning(struct sdhci_host *host,
>>>>> } while (++tuning_loop < RETRY_TUNING_MAX);
>>>>> if (itapdly < 0) {
>>>>> - dev_err(dev, "Failed to find itapdly, fail tuning\n");
>>>>> - sdhci_am654_write_itapdly(sdhci_am654, 0, 0);
>>>>> - sdhci_am654->itap_del_ena[timing] = 0;
>>>>> - sdhci_am654->itap_del_sel[timing] = 0;
>>>>> + if (timing == MMC_TIMING_UHS_DDR50) {
>>>>
>>>> Should dev_dbg be added here to let the user know that tuning failed but
>>>> DT defaults were set?
>>>
>>> That sounds useful, I'll add a print here.
>>>
>>>> ~ Judith
>>>>
>>>
>>> Best regards,
>>> Diogo
>>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2026-08-07 10:52 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-03 13:47 [PATCH v2 0/4] mmc: sdhci_am654: Fix DDR50 mode for cards without tuning support Diogo Ivo (Schneider Electric)
2026-08-03 13:47 ` [PATCH v2 1/4] mmc: sdhci_am654: Move tuning_loop to local variable Diogo Ivo (Schneider Electric)
2026-08-03 19:19 ` Mendez, Judith
2026-08-03 13:47 ` [PATCH v2 2/4] mmc: sdhci_am654: Reset command and data lines on failed tuning Diogo Ivo (Schneider Electric)
2026-08-03 19:28 ` Mendez, Judith
2026-08-03 13:47 ` [PATCH v2 3/4] mmc: sdhci_am654: Clear ITAPDLY on tuning failure Diogo Ivo (Schneider Electric)
2026-08-03 19:40 ` Mendez, Judith
2026-08-03 13:47 ` [PATCH v2 4/4] mmc: sdhci_am654: Fallback to DT-provided itap delay on DDR50 " Diogo Ivo (Schneider Electric)
2026-08-03 19:59 ` Mendez, Judith
2026-08-04 8:45 ` Diogo Ivo
2026-08-05 23:13 ` Mendez, Judith
2026-08-07 6:35 ` Adrian Hunter
2026-08-07 10:52 ` Diogo Ivo
2026-08-07 8:16 ` [PATCH v2 0/4] mmc: sdhci_am654: Fix DDR50 mode for cards without tuning support Adrian Hunter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox