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

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

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