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

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: <[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?
>>>
>>> That sounds useful, I'll add a print here.
>>>
>>>> ~ Judith
>>>>
>>>
>>> Best regards,
>>> Diogo
>>
>