Re: [PATCH v3 4/4] mmc: sdhci_am654: Fallback to DT-provided itap delay on DDR50 tuning failure

["Mendez, Judith" <jm@ti.com>]

Hi Diogo,

On 8/7/2026 6:07 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")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Diogo Ivo (Schneider Electric) <diogo.ivo@bootlin.com>
> Acked-by: Adrian Hunter <adrian.hunter@intel.com>

Reviewed-by: Judith Mendez <jm@ti.com>

> ---
> Changes in v3:
>   - Collect A-b from Adrian
>   - Add debug print when applying DT fallback values
> 
> 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 | 26 +++++++++++++++++++++-----
>   1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 8ce10422bba0..8edfab5012ed 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,19 @@ 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) {
> +			dev_dbg(dev, "Failed DDR50 tuning, fallback to DT ITAP\n");

Its not a big deal, but it would be a bit better to use itapdly
according to the datasheet instead of itap (:

> +			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 +769,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;
>   }
>   
>