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

[Diogo Ivo]

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@xxxxxxxxxxxxxxx>
> > > > > Signed-off-by: Diogo Ivo (Schneider Electric) <diogo.ivo@xxxxxxxxxxx>
> > > > > ---
> > > > > 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