Re: [PATCH v2 4/4] mmc: dw_mmc: Do not wait for DTO in case of error

[Marten Lindahl <martenli@axis.com>]

On Wed, Dec 08, 2021 at 03:53:54PM +0100, Ulf Hansson wrote:
> On Mon, 6 Dec 2021 at 15:29, Mårten Lindahl <marten.lindahl@axis.com> wrote:
> >
> > When running the ARTPEC-8 DWMMC IP version, and a data error interrupt
> > comes during a data read transfer, there is no guarantee for the data
> > transfer over interrupt (DTO) to come within the specified data timeout.
> > This case is handled by the dto_timer handler which will complete the
> > request with the comment:
> >
> >  /*
> >   * If DTO interrupt does NOT come in sending data state,
> >   * we should notify the driver to terminate current transfer
> >   * and report a data timeout to the core.
> >   */
> >
> > But since the ARTPEC-8 DWMMC IP version, supports an extended TMOUT
> > register which allows longer timeouts than the non ARTPEC-8 version
> > does, waiting for the dto_timer to complete the request in error cases
> > may cause the request to take significantly longer time than necessary.
> > This is specifically true for the failing steps during tuning of a
> > device.
> >
> > Fix this by completing the request when the error interrupt comes.
> >
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> 

Hi Ulf!

> Okay, this change looks a bit inconvenient to move into variant
> specific callbacks. So, maybe the "quirks" flag makes sense, after
> all. However, I would still look at using callbacks and library
> functions, for the part implemented in patch3.

Yes, I don't see how this patch can be easily made with callbacks, but
definitely for patch3. So then I move the definition of the quirk from
patch3 to this patch.

> 
> When it comes to the order of the patches in the series, I suggest
> flipping things around and making patch2 the final piece. Otherwise
> the support for the artpec variant will be broken between patch2 and
> patch4, right?

Ok, you mean there may be a risk that the ARTPEC-8 dw_mmc does not work
if the support is enabled in patch2, but patch3 and patch4 is not in
place? That is a good point, but it actually does work quite fine (most
of the time) without the extended timeout function. But it does not use
the full function of the data timeout, and the HW timeout is most often
set to full timeout (0xFFFFFF => 587ms with 200MHz), but the SW timer is
limited to a lower value (0xFFFFFF => 84 + 10 ms with 200MHz).

My reasoning is:
patch1 - dtbindings for ARTPEC-8
patch2 - adding ARTPEC-8 to dw_mmc-exynos
patch3 - implement ARTPEC-8 specific function for data timeout
patch4 - add quirk to abort the extended timeout in case of errors, used
by ARTPEC-8

so, this means patch3 and patch4 depends on patch2, and patch4 depends
on patch3.

Kind regards
Mårten

> 
> Kind regards
> Uffe
> 
> > ---
> >  drivers/mmc/host/dw_mmc.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > index 45ea9fd97a6a..d6b76f47b1a2 100644
> > --- a/drivers/mmc/host/dw_mmc.c
> > +++ b/drivers/mmc/host/dw_mmc.c
> > @@ -2777,11 +2777,19 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
> >                 if (pending & DW_MCI_DATA_ERROR_FLAGS) {
> >                         spin_lock(&host->irq_lock);
> >
> > +                       if (host->quirks & DW_MMC_QUIRK_EXTENDED_TMOUT)
> > +                               del_timer(&host->dto_timer);
> > +
> >                         /* if there is an error report DATA_ERROR */
> >                         mci_writel(host, RINTSTS, DW_MCI_DATA_ERROR_FLAGS);
> >                         host->data_status = pending;
> >                         smp_wmb(); /* drain writebuffer */
> >                         set_bit(EVENT_DATA_ERROR, &host->pending_events);
> > +
> > +                       if (host->quirks & DW_MMC_QUIRK_EXTENDED_TMOUT)
> > +                               /* In case of error, we cannot expect a DTO */
> > +                               set_bit(EVENT_DATA_COMPLETE, &host->pending_events);
> > +
> >                         tasklet_schedule(&host->tasklet);
> >
> >                         spin_unlock(&host->irq_lock);
> > --
> > 2.20.1
> >