Re: [PATCH] mmc: dw_mmc: Allow lower TMOUT value than maximum

[Marten Lindahl <martenli@axis.com>]

On Sat, Nov 06, 2021 at 01:14:48AM +0100, Doug Anderson wrote:
> Hi,

Hi Doug!

> 
> On Wed, Nov 3, 2021 at 8:24 AM Mårten Lindahl <marten.lindahl@axis.com> wrote:
> >
> > The TMOUT register is always set with a full value for every transfer,
> > which (with a 200MHz clock) will give a full DRTO of ~84 milliseconds.
> > Since the software dto_timer acts as a backup in cases when this timeout
> > is not long enough, it is normally not a problem. But setting a full
> > value makes it impossible to test shorter timeouts, when for example
> > testing data read times on different SD cards.
> >
> > Add a function to set any value smaller than the maximum of 0xFFFFFF.
> >
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> > ---
> >  drivers/mmc/host/dw_mmc.c | 29 ++++++++++++++++++++++++++++-
> >  1 file changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > index 6578cc64ae9e..0d23b8ed9403 100644
> > --- a/drivers/mmc/host/dw_mmc.c
> > +++ b/drivers/mmc/host/dw_mmc.c
> > @@ -54,6 +54,7 @@
> >
> >  #define DW_MCI_FREQ_MAX        200000000       /* unit: HZ */
> >  #define DW_MCI_FREQ_MIN        100000          /* unit: HZ */
> > +#define DW_MCI_DATA_TMOUT_NS_MAX       83886075
> >
> >  #define IDMAC_INT_CLR          (SDMMC_IDMAC_INT_AI | SDMMC_IDMAC_INT_NI | \
> >                                  SDMMC_IDMAC_INT_CES | SDMMC_IDMAC_INT_DU | \
> > @@ -1283,6 +1284,32 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
> >         mci_writel(host, CTYPE, (slot->ctype << slot->id));
> >  }
> >
> > +static void dw_mci_set_data_timeout(struct dw_mci *host, u32 timeout_ns)
> 
> The type of "timeout_ns" should match `struct mmc_data`, which is
> unsigned int, not u32.

Will fix.

> 
> 
> > +{
> > +       u32 timeout, freq_mhz, tmp, tmout;
> > +
> > +       if (!timeout_ns || timeout_ns > DW_MCI_DATA_TMOUT_NS_MAX) {
> > +               /* Set maximum */
> > +               tmout = 0xFFFFFFFF;
> > +               goto tmout_done;
> > +       }
> 
> I don't think that the above is right. If the card clock is 50 Hz
> instead of 200 Hz then 0xffffffff is actually ~83 ms * 4 = ~332 ms. It
> would be better to attempt to program it correctly.
> 
> Can you just do the math below and if the number is greater than can
> be represented then you can just put in the max?
> 

Yes. Good point. Much better way to do it.

> Interestingly enough, in `struct mmc_data` this is documented as a max
> of 80 ms, though I don't think your code should care about that. Just
> cap to the maximum value after your math.
> 
> 
> > +       timeout = timeout_ns;
> > +       freq_mhz = DIV_ROUND_UP(host->bus_hz, NSEC_PER_MSEC);
> > +
> > +       /* TMOUT[7:0] (RESPONSE_TIMEOUT) */
> > +       tmout = 0xFF; /* Set maximum */
> > +
> > +       /* TMOUT[31:8] (DATA_TIMEOUT) */
> > +       tmp = DIV_ROUND_UP_ULL((u64)timeout * freq_mhz, MSEC_PER_SEC);
> > +       tmout |= (tmp & 0xFFFFFF) << 8;
> 
> Combining your two calculations, I guess you have:
> 
> tmp = timeout * (bus_hz / 1000000) / 1000
> 
> Why isn't this just:
> 
> tmp = (timeout * bus_hz) / 1000000000
> 
> Since you're doing 64-bit math anyway I don't think you need to worry
> about that calculation overflowing. Multiplying two 32-bit numbers
> can't exceed 64-bits, right?

Will do so.
> 
> 
> Also: I think "bus_hz" is the wrong thing to be using here. You need
> to take CLKDIV into account like dw_mci_set_drto() does.
> 

Will do so.
Good comments. Thanks!

Kind regards
Mårten
> 
> -Doug