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@xxxxxxxx> 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@xxxxxxxx>
> > ---
> > 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
