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

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

[Mårten Lindahl]

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)
+{
+	u32 timeout, freq_mhz, tmp, tmout;
+
+	if (!timeout_ns || timeout_ns > DW_MCI_DATA_TMOUT_NS_MAX) {
+		/* Set maximum */
+		tmout = 0xFFFFFFFF;
+		goto tmout_done;
+	}
+
+	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;
+
+tmout_done:
+	mci_writel(host, TMOUT, tmout);
+	dev_dbg(host->dev, "timeout_ns: %u => TMOUT[31:8]: 0x%06x",
+		timeout_ns, tmout >> 8);
+}
+
 static void __dw_mci_start_request(struct dw_mci *host,
 				   struct dw_mci_slot *slot,
 				   struct mmc_command *cmd)
@@ -1303,7 +1330,7 @@ static void __dw_mci_start_request(struct dw_mci *host,
 
 	data = cmd->data;
 	if (data) {
-		mci_writel(host, TMOUT, 0xFFFFFFFF);
+		dw_mci_set_data_timeout(host, data->timeout_ns);
 		mci_writel(host, BYTCNT, data->blksz*data->blocks);
 		mci_writel(host, BLKSIZ, data->blksz);
 	}
-- 
2.20.1

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

[Doug Anderson]

Hi,

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.


> +{
> +       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?

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?


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.


-Doug

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

[Marten Lindahl]

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