Re: [PATCH] mmc: dw_mmc: Support more time for data read timeouts

[Vincent Whitchurch <vincent.whitchurch@axis.com>]

On Mon, Sep 06, 2021 at 06:36:33PM +0200, Ulf Hansson wrote:
> On Fri, 27 Aug 2021 at 10:56, Mårten Lindahl <marten.lindahl@axis.com> wrote:
> > For data read transfers a data transfer over timer (dto_timer) is
> > started to make sure the data command can be completed in cases the Data
> > Transfer Over (DTO) interrupt does not come. This timer was originally
> > introduced as a quirk in commit 57e104864bc48 ("mmc: dw_mmc: add quirk
> > for broken data transfer over scheme"), but is since a while back part
> > of the running code.
> >
> > The dto timer picks the DATA_TIMEOUT value in the TMOUT register for its
> > timeout, which will give a max timeout of approximately 84 + (10 spare)
> > milliseconds on a 200MHz clock. But this register is not intended to be
> > used like that, since it is a counter for data read timeouts (DRTO) and
> > response timeouts (RTO), which will result in error interrupts in case
> > of data read and response delays.
> >
> > The TMOUT register is always set with a full value for every transfer,
> > which according to the manual (and with 200MHz clock) will give a full
> > DRTO of:
> > ((TMOUT[10:8] -1) * 0xFFFFFF + TMOUT[31:11] * 8) / 200000000 => ~587 ms
> >
> > But as the same register is used for the dto_timer, the dto_timer will
> > always have a fixed timeout.
> >
> > Instead of always setting a fixed value in TMOUT register, we can use
> > data->timeout_ns for the DRTO interrupts that actually matches what was
> > provided per requested command. Likewise we can also use timeout_ns for
> > the dto_timer, which will allow a max timeout of 587 ms, instead of the
> > fixed 94 ms. Furthermore, if a data error interrupt comes, it shouldn't
> > be necessary to wait for the dto_timer before we finish the command, but
> > instead we can handle it in the interrupt handler.
> >
> > Lets fix this. In most cases data->timeout_ns values are given, but in
> > case it is not given, the maximum (default) timeout for the dto_timer,
> > and the DRTO, is set to approximately 587 ms.
> >
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> 
> As this touches common dw_mmc code, I would appreciate some
> tests/reviews to be done before I apply this. Therefore I looped in
> some of the people who have been involved with dw_mmc lately.
>
> + Doug, Shawn and Vincent (maybe they can also help to review/test this change). 

I have the same hardware as Mårten (ARTPEC-8) so I don't think my
testing would add much.  We'd ideally need someone with a different
platform to help to test this.

As for the patch itself, it would perhaps be helpful if it were broken
down into three separate patches (not necessarily in the order listed
below) since it's doing three different things:

(1) Increasing the length of the software timeout.

This is the primary reason for the patch since the driver is clearly
using a wrong value for the software timeout for the entire transfer
which is lower than the timeouts required by eMMC chips' TAAC/NSAC and
the controller's timeout for (IIUC) one block.

We haven't seen any problems in practice (yet) though.

(As an aside, if I'm reading the eMMC specs correctly, the TAAC/NSAC
 applies to each block of a multi-block transfer.  If that's the case,
 then the entire transfer could, at least in theory, take several
 seconds.

 Is the data->timeout_ns calculated by mmc_set_data_timeout() supposed
 to be a per-block timeout or a timeout for the entire transfer?)

(2) Not waiting for DRTO on errors.

When attempting to increase the length of the software timeout as in (1)
or to remove it entirely, one realizes that the driver actually depends
on the software timeout to prevent hanging indefinitely when CRC errors
are received during tuning.  It's unclear if this is something specific
to our version of the IP or if this is an old regression in the driver
(given that the driver apparently used to work fine without a software
timeout at some point of time on some platforms.)

So increasing the software timeout length will make each failing step in
tuning take half a second which would increase boot time significantly,
so that's why I think the early exit is needed.

(3) Reducing the length of the hardware timeout.

I *think* this is also to reduce initialization time if there are some
transfers which are meant to time out (without any other errors) which
would now take longer to time out after (1), but this part of the change
could use a bit more rationale.