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

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: <[email protected]>
> Signed-off-by: Diogo Ivo (Schneider Electric) <[email protected]>
> ---
> 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;
>   }
>   
>