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

[PATCH v4] 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.
This is normally good enough to complete the request, 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>
---

v2:
 - Calculate new value before checking boundaries
 - Include CLKDIV register to get proper value

v3:
 - Use 'if-else' instead of 'goto'
 - Don't touch response field when maximize data field

v4:
 - Prevent 32bit divider overflow by splitting the operation
 - Changed %06x to %#08x as suggested by Doug
 - Rephrased commit msg as suggested by Doug

 drivers/mmc/host/dw_mmc.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index d977f34f6b55..8e9d33e1b96c 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1283,6 +1283,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,
+				    unsigned int timeout_ns)
+{
+	unsigned int clk_div, tmp, tmout;
+
+	clk_div = (mci_readl(host, CLKDIV) & 0xFF) * 2;
+	if (clk_div == 0)
+		clk_div = 1;
+
+	tmp = DIV_ROUND_UP_ULL((u64)timeout_ns * host->bus_hz, NSEC_PER_SEC);
+	tmp = DIV_ROUND_UP(tmp, clk_div);
+
+	/* TMOUT[7:0] (RESPONSE_TIMEOUT) */
+	tmout = 0xFF; /* Set maximum */
+
+	/* TMOUT[31:8] (DATA_TIMEOUT) */
+	if (!tmp || tmp > 0xFFFFFF)
+		tmout |= (0xFFFFFF << 8);
+	else
+		tmout |= (tmp & 0xFFFFFF) << 8;
+
+	mci_writel(host, TMOUT, tmout);
+	dev_dbg(host->dev, "timeout_ns: %u => TMOUT[31:8]: 0x%#08x",
+		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 +1329,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 v4] mmc: dw_mmc: Allow lower TMOUT value than maximum

[Doug Anderson]

Hi,

On Wed, Nov 17, 2021 at 8:09 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.
> This is normally good enough to complete the request, 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>
> ---
>
> v2:
>  - Calculate new value before checking boundaries
>  - Include CLKDIV register to get proper value
>
> v3:
>  - Use 'if-else' instead of 'goto'
>  - Don't touch response field when maximize data field
>
> v4:
>  - Prevent 32bit divider overflow by splitting the operation
>  - Changed %06x to %#08x as suggested by Doug
>  - Rephrased commit msg as suggested by Doug
>
>  drivers/mmc/host/dw_mmc.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index d977f34f6b55..8e9d33e1b96c 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1283,6 +1283,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,
> +                                   unsigned int timeout_ns)
> +{
> +       unsigned int clk_div, tmp, tmout;

didn't notice before, but nit that I usually make it a policy that
things that represent cpu registers are the "sized" types. Thus I'd
rather see these locals as u32 even though the parameter (which
represents a logical value and not a CPU register) stays as "unsigned
int").


> +       clk_div = (mci_readl(host, CLKDIV) & 0xFF) * 2;
> +       if (clk_div == 0)
> +               clk_div = 1;
> +
> +       tmp = DIV_ROUND_UP_ULL((u64)timeout_ns * host->bus_hz, NSEC_PER_SEC);
> +       tmp = DIV_ROUND_UP(tmp, clk_div);

I guess in some extreme cases you still have an overflow. Not sure how
many people really use "div", but...

The case I'm thinking of is if the timeout is 80 ms, the bus_hz is 200
MHz, and clk_div is 20 (register contains 10). I think that would mean
you're feeding the controller a 4GHz clock which it probably couldn't
_really_ handle, so maybe this isn't super realistic. In any case, I
think the first statement would be the equivalent of 80 * 200MHz =
0x3b9aca000 and that blows out the 32-bit "tmp" variable.

Why not just keep it as 64-bit until you're done dividing to be safe?

-Doug

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

[Marten Lindahl]

On Thu, Nov 18, 2021 at 12:29:46AM +0100, Doug Anderson wrote:

Hi Doug!

> Hi,
> 
> On Wed, Nov 17, 2021 at 8:09 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.
> > This is normally good enough to complete the request, 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>
> > ---
> >
> > v2:
> >  - Calculate new value before checking boundaries
> >  - Include CLKDIV register to get proper value
> >
> > v3:
> >  - Use 'if-else' instead of 'goto'
> >  - Don't touch response field when maximize data field
> >
> > v4:
> >  - Prevent 32bit divider overflow by splitting the operation
> >  - Changed %06x to %#08x as suggested by Doug
> >  - Rephrased commit msg as suggested by Doug
> >
> >  drivers/mmc/host/dw_mmc.c | 28 +++++++++++++++++++++++++++-
> >  1 file changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > index d977f34f6b55..8e9d33e1b96c 100644
> > --- a/drivers/mmc/host/dw_mmc.c
> > +++ b/drivers/mmc/host/dw_mmc.c
> > @@ -1283,6 +1283,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,
> > +                                   unsigned int timeout_ns)
> > +{
> > +       unsigned int clk_div, tmp, tmout;
> 
> didn't notice before, but nit that I usually make it a policy that
> things that represent cpu registers are the "sized" types. Thus I'd
> rather see these locals as u32 even though the parameter (which
> represents a logical value and not a CPU register) stays as "unsigned
> int").
>

Thanks, will fix.

> 
> > +       clk_div = (mci_readl(host, CLKDIV) & 0xFF) * 2;
> > +       if (clk_div == 0)
> > +               clk_div = 1;
> > +
> > +       tmp = DIV_ROUND_UP_ULL((u64)timeout_ns * host->bus_hz, NSEC_PER_SEC);
> > +       tmp = DIV_ROUND_UP(tmp, clk_div);
> 
> I guess in some extreme cases you still have an overflow. Not sure how
> many people really use "div", but...
> 
> The case I'm thinking of is if the timeout is 80 ms, the bus_hz is 200
> MHz, and clk_div is 20 (register contains 10). I think that would mean
> you're feeding the controller a 4GHz clock which it probably couldn't
> _really_ handle, so maybe this isn't super realistic. In any case, I
> think the first statement would be the equivalent of 80 * 200MHz =
> 0x3b9aca000 and that blows out the 32-bit "tmp" variable.

I'm sorry but I fail to follow your calculation here. With 80ms timeout
and 200MHz bus_hz, I get:

80000000 * 200000000 / 1000000000 = 0xF42400

The only way I manage to get an overflow of tmp is with:

timeout = INT_MAX * bus_hz = (value greater than 1000000000) / 1000000000

So my reasoning is that tmp = DIV_ROUND_UP(tmp, clk_div) is safe within
these values, but I can of course make tmp an unsigned long, and in that
case do the clk_div division as:

tmp = DIV_ROUND_UP_ULL(tmp, clk_div)

Kind regards
Mårten

> 
> Why not just keep it as 64-bit until you're done dividing to be safe?
> 
> -Doug

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

[Doug Anderson]

Hi,

On Thu, Nov 18, 2021 at 2:51 AM Marten Lindahl <martenli@axis.com> wrote:
>
> On Thu, Nov 18, 2021 at 12:29:46AM +0100, Doug Anderson wrote:
>
> Hi Doug!
>
> > Hi,
> >
> > On Wed, Nov 17, 2021 at 8:09 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.
> > > This is normally good enough to complete the request, 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>
> > > ---
> > >
> > > v2:
> > >  - Calculate new value before checking boundaries
> > >  - Include CLKDIV register to get proper value
> > >
> > > v3:
> > >  - Use 'if-else' instead of 'goto'
> > >  - Don't touch response field when maximize data field
> > >
> > > v4:
> > >  - Prevent 32bit divider overflow by splitting the operation
> > >  - Changed %06x to %#08x as suggested by Doug
> > >  - Rephrased commit msg as suggested by Doug
> > >
> > >  drivers/mmc/host/dw_mmc.c | 28 +++++++++++++++++++++++++++-
> > >  1 file changed, 27 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > > index d977f34f6b55..8e9d33e1b96c 100644
> > > --- a/drivers/mmc/host/dw_mmc.c
> > > +++ b/drivers/mmc/host/dw_mmc.c
> > > @@ -1283,6 +1283,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,
> > > +                                   unsigned int timeout_ns)
> > > +{
> > > +       unsigned int clk_div, tmp, tmout;
> >
> > didn't notice before, but nit that I usually make it a policy that
> > things that represent cpu registers are the "sized" types. Thus I'd
> > rather see these locals as u32 even though the parameter (which
> > represents a logical value and not a CPU register) stays as "unsigned
> > int").
> >
>
> Thanks, will fix.
>
> >
> > > +       clk_div = (mci_readl(host, CLKDIV) & 0xFF) * 2;
> > > +       if (clk_div == 0)
> > > +               clk_div = 1;
> > > +
> > > +       tmp = DIV_ROUND_UP_ULL((u64)timeout_ns * host->bus_hz, NSEC_PER_SEC);
> > > +       tmp = DIV_ROUND_UP(tmp, clk_div);
> >
> > I guess in some extreme cases you still have an overflow. Not sure how
> > many people really use "div", but...
> >
> > The case I'm thinking of is if the timeout is 80 ms, the bus_hz is 200
> > MHz, and clk_div is 20 (register contains 10). I think that would mean
> > you're feeding the controller a 4GHz clock which it probably couldn't
> > _really_ handle, so maybe this isn't super realistic. In any case, I
> > think the first statement would be the equivalent of 80 * 200MHz =
> > 0x3b9aca000 and that blows out the 32-bit "tmp" variable.
>
> I'm sorry but I fail to follow your calculation here. With 80ms timeout
> and 200MHz bus_hz, I get:
>
> 80000000 * 200000000 / 1000000000 = 0xF42400

Sorry, it's just my brain not working properly. Yeah, I think you were
fine assuming it was 32-bit. It seems terribly unlikely that bus_hz
could be anywhere approaching 32-bit max. Even if it was, the timeout
is documented to be max on the order of 80 ms:

/* data timeout (in ns, max 80ms) */

...and even if that's wrong and it's 800 ms _and_ bus_hz is the
absurdly large 0xffffffff then we still don't timeout.

Sorry for getting that wrong. :(

-Doug

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

[Marten Lindahl]

On Fri, Nov 19, 2021 at 03:44:16PM +0100, Doug Anderson wrote:
> Hi,
> 
> On Thu, Nov 18, 2021 at 2:51 AM Marten Lindahl <martenli@axis.com> wrote:
> >
> > On Thu, Nov 18, 2021 at 12:29:46AM +0100, Doug Anderson wrote:
> >
> > Hi Doug!
> >
> > > Hi,
> > >
> > > On Wed, Nov 17, 2021 at 8:09 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.
> > > > This is normally good enough to complete the request, 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>
> > > > ---
> > > >
> > > > v2:
> > > >  - Calculate new value before checking boundaries
> > > >  - Include CLKDIV register to get proper value
> > > >
> > > > v3:
> > > >  - Use 'if-else' instead of 'goto'
> > > >  - Don't touch response field when maximize data field
> > > >
> > > > v4:
> > > >  - Prevent 32bit divider overflow by splitting the operation
> > > >  - Changed %06x to %#08x as suggested by Doug
> > > >  - Rephrased commit msg as suggested by Doug
> > > >
> > > >  drivers/mmc/host/dw_mmc.c | 28 +++++++++++++++++++++++++++-
> > > >  1 file changed, 27 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > > > index d977f34f6b55..8e9d33e1b96c 100644
> > > > --- a/drivers/mmc/host/dw_mmc.c
> > > > +++ b/drivers/mmc/host/dw_mmc.c
> > > > @@ -1283,6 +1283,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,
> > > > +                                   unsigned int timeout_ns)
> > > > +{
> > > > +       unsigned int clk_div, tmp, tmout;
> > >
> > > didn't notice before, but nit that I usually make it a policy that
> > > things that represent cpu registers are the "sized" types. Thus I'd
> > > rather see these locals as u32 even though the parameter (which
> > > represents a logical value and not a CPU register) stays as "unsigned
> > > int").
> > >
> >
> > Thanks, will fix.
> >
> > >
> > > > +       clk_div = (mci_readl(host, CLKDIV) & 0xFF) * 2;
> > > > +       if (clk_div == 0)
> > > > +               clk_div = 1;
> > > > +
> > > > +       tmp = DIV_ROUND_UP_ULL((u64)timeout_ns * host->bus_hz, NSEC_PER_SEC);
> > > > +       tmp = DIV_ROUND_UP(tmp, clk_div);
> > >
> > > I guess in some extreme cases you still have an overflow. Not sure how
> > > many people really use "div", but...
> > >
> > > The case I'm thinking of is if the timeout is 80 ms, the bus_hz is 200
> > > MHz, and clk_div is 20 (register contains 10). I think that would mean
> > > you're feeding the controller a 4GHz clock which it probably couldn't
> > > _really_ handle, so maybe this isn't super realistic. In any case, I
> > > think the first statement would be the equivalent of 80 * 200MHz =
> > > 0x3b9aca000 and that blows out the 32-bit "tmp" variable.
> >
> > I'm sorry but I fail to follow your calculation here. With 80ms timeout
> > and 200MHz bus_hz, I get:
> >
> > 80000000 * 200000000 / 1000000000 = 0xF42400
> 
> Sorry, it's just my brain not working properly. Yeah, I think you were
> fine assuming it was 32-bit. It seems terribly unlikely that bus_hz
> could be anywhere approaching 32-bit max. Even if it was, the timeout
> is documented to be max on the order of 80 ms:
> 
> /* data timeout (in ns, max 80ms) */
> 
> ...and even if that's wrong and it's 800 ms _and_ bus_hz is the
> absurdly large 0xffffffff then we still don't timeout.
> 
> Sorry for getting that wrong. :(

No problem. Reviews are for twisting and turning the code.

To twist it even more, there is no real need to use DIV_ROUND_UP(_ULL)
on the clkdiv division right? I mean the round up has already been made,
and it shouldn't be needed twice?

So,
	tmp = DIV_ROUND_UP_(ULL)(tmp, clk_div);

could be a

	tmp /= clk_div;

Kind regards
Mårten
> 
> -Doug

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

[Doug Anderson]

Hi,

On Fri, Nov 19, 2021 at 7:30 AM Marten Lindahl <martenli@axis.com> wrote:
>
> On Fri, Nov 19, 2021 at 03:44:16PM +0100, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Nov 18, 2021 at 2:51 AM Marten Lindahl <martenli@axis.com> wrote:
> > >
> > > On Thu, Nov 18, 2021 at 12:29:46AM +0100, Doug Anderson wrote:
> > >
> > > Hi Doug!
> > >
> > > > Hi,
> > > >
> > > > On Wed, Nov 17, 2021 at 8:09 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.
> > > > > This is normally good enough to complete the request, 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>
> > > > > ---
> > > > >
> > > > > v2:
> > > > >  - Calculate new value before checking boundaries
> > > > >  - Include CLKDIV register to get proper value
> > > > >
> > > > > v3:
> > > > >  - Use 'if-else' instead of 'goto'
> > > > >  - Don't touch response field when maximize data field
> > > > >
> > > > > v4:
> > > > >  - Prevent 32bit divider overflow by splitting the operation
> > > > >  - Changed %06x to %#08x as suggested by Doug
> > > > >  - Rephrased commit msg as suggested by Doug
> > > > >
> > > > >  drivers/mmc/host/dw_mmc.c | 28 +++++++++++++++++++++++++++-
> > > > >  1 file changed, 27 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > > > > index d977f34f6b55..8e9d33e1b96c 100644
> > > > > --- a/drivers/mmc/host/dw_mmc.c
> > > > > +++ b/drivers/mmc/host/dw_mmc.c
> > > > > @@ -1283,6 +1283,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,
> > > > > +                                   unsigned int timeout_ns)
> > > > > +{
> > > > > +       unsigned int clk_div, tmp, tmout;
> > > >
> > > > didn't notice before, but nit that I usually make it a policy that
> > > > things that represent cpu registers are the "sized" types. Thus I'd
> > > > rather see these locals as u32 even though the parameter (which
> > > > represents a logical value and not a CPU register) stays as "unsigned
> > > > int").
> > > >
> > >
> > > Thanks, will fix.
> > >
> > > >
> > > > > +       clk_div = (mci_readl(host, CLKDIV) & 0xFF) * 2;
> > > > > +       if (clk_div == 0)
> > > > > +               clk_div = 1;
> > > > > +
> > > > > +       tmp = DIV_ROUND_UP_ULL((u64)timeout_ns * host->bus_hz, NSEC_PER_SEC);
> > > > > +       tmp = DIV_ROUND_UP(tmp, clk_div);
> > > >
> > > > I guess in some extreme cases you still have an overflow. Not sure how
> > > > many people really use "div", but...
> > > >
> > > > The case I'm thinking of is if the timeout is 80 ms, the bus_hz is 200
> > > > MHz, and clk_div is 20 (register contains 10). I think that would mean
> > > > you're feeding the controller a 4GHz clock which it probably couldn't
> > > > _really_ handle, so maybe this isn't super realistic. In any case, I
> > > > think the first statement would be the equivalent of 80 * 200MHz =
> > > > 0x3b9aca000 and that blows out the 32-bit "tmp" variable.
> > >
> > > I'm sorry but I fail to follow your calculation here. With 80ms timeout
> > > and 200MHz bus_hz, I get:
> > >
> > > 80000000 * 200000000 / 1000000000 = 0xF42400
> >
> > Sorry, it's just my brain not working properly. Yeah, I think you were
> > fine assuming it was 32-bit. It seems terribly unlikely that bus_hz
> > could be anywhere approaching 32-bit max. Even if it was, the timeout
> > is documented to be max on the order of 80 ms:
> >
> > /* data timeout (in ns, max 80ms) */
> >
> > ...and even if that's wrong and it's 800 ms _and_ bus_hz is the
> > absurdly large 0xffffffff then we still don't timeout.
> >
> > Sorry for getting that wrong. :(
>
> No problem. Reviews are for twisting and turning the code.
>
> To twist it even more, there is no real need to use DIV_ROUND_UP(_ULL)
> on the clkdiv division right? I mean the round up has already been made,
> and it shouldn't be needed twice?
>
> So,
>         tmp = DIV_ROUND_UP_(ULL)(tmp, clk_div);
>
> could be a
>
>         tmp /= clk_div;

I think you still need the round up, but I wouldn't swear to it.
You've divided by one value, but not the other and each division could
separately need rounding.

-Doug

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

[Marten Lindahl]

On Fri, Nov 19, 2021 at 04:35:59PM +0100, Doug Anderson wrote:
> Hi,
> 
> On Fri, Nov 19, 2021 at 7:30 AM Marten Lindahl <martenli@axis.com> wrote:
> >
> > On Fri, Nov 19, 2021 at 03:44:16PM +0100, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Thu, Nov 18, 2021 at 2:51 AM Marten Lindahl <martenli@axis.com> wrote:
> > > >
> > > > On Thu, Nov 18, 2021 at 12:29:46AM +0100, Doug Anderson wrote:
> > > >
> > > > Hi Doug!
> > > >
> > > > > Hi,
> > > > >
> > > > > On Wed, Nov 17, 2021 at 8:09 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.
> > > > > > This is normally good enough to complete the request, 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>
> > > > > > ---
> > > > > >
> > > > > > v2:
> > > > > >  - Calculate new value before checking boundaries
> > > > > >  - Include CLKDIV register to get proper value
> > > > > >
> > > > > > v3:
> > > > > >  - Use 'if-else' instead of 'goto'
> > > > > >  - Don't touch response field when maximize data field
> > > > > >
> > > > > > v4:
> > > > > >  - Prevent 32bit divider overflow by splitting the operation
> > > > > >  - Changed %06x to %#08x as suggested by Doug
> > > > > >  - Rephrased commit msg as suggested by Doug
> > > > > >
> > > > > >  drivers/mmc/host/dw_mmc.c | 28 +++++++++++++++++++++++++++-
> > > > > >  1 file changed, 27 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > > > > > index d977f34f6b55..8e9d33e1b96c 100644
> > > > > > --- a/drivers/mmc/host/dw_mmc.c
> > > > > > +++ b/drivers/mmc/host/dw_mmc.c
> > > > > > @@ -1283,6 +1283,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,
> > > > > > +                                   unsigned int timeout_ns)
> > > > > > +{
> > > > > > +       unsigned int clk_div, tmp, tmout;
> > > > >
> > > > > didn't notice before, but nit that I usually make it a policy that
> > > > > things that represent cpu registers are the "sized" types. Thus I'd
> > > > > rather see these locals as u32 even though the parameter (which
> > > > > represents a logical value and not a CPU register) stays as "unsigned
> > > > > int").
> > > > >
> > > >
> > > > Thanks, will fix.
> > > >
> > > > >
> > > > > > +       clk_div = (mci_readl(host, CLKDIV) & 0xFF) * 2;
> > > > > > +       if (clk_div == 0)
> > > > > > +               clk_div = 1;
> > > > > > +
> > > > > > +       tmp = DIV_ROUND_UP_ULL((u64)timeout_ns * host->bus_hz, NSEC_PER_SEC);
> > > > > > +       tmp = DIV_ROUND_UP(tmp, clk_div);
> > > > >
> > > > > I guess in some extreme cases you still have an overflow. Not sure how
> > > > > many people really use "div", but...
> > > > >
> > > > > The case I'm thinking of is if the timeout is 80 ms, the bus_hz is 200
> > > > > MHz, and clk_div is 20 (register contains 10). I think that would mean
> > > > > you're feeding the controller a 4GHz clock which it probably couldn't
> > > > > _really_ handle, so maybe this isn't super realistic. In any case, I
> > > > > think the first statement would be the equivalent of 80 * 200MHz =
> > > > > 0x3b9aca000 and that blows out the 32-bit "tmp" variable.
> > > >
> > > > I'm sorry but I fail to follow your calculation here. With 80ms timeout
> > > > and 200MHz bus_hz, I get:
> > > >
> > > > 80000000 * 200000000 / 1000000000 = 0xF42400
> > >
> > > Sorry, it's just my brain not working properly. Yeah, I think you were
> > > fine assuming it was 32-bit. It seems terribly unlikely that bus_hz
> > > could be anywhere approaching 32-bit max. Even if it was, the timeout
> > > is documented to be max on the order of 80 ms:
> > >
> > > /* data timeout (in ns, max 80ms) */
> > >
> > > ...and even if that's wrong and it's 800 ms _and_ bus_hz is the
> > > absurdly large 0xffffffff then we still don't timeout.
> > >
> > > Sorry for getting that wrong. :(
> >
> > No problem. Reviews are for twisting and turning the code.
> >
> > To twist it even more, there is no real need to use DIV_ROUND_UP(_ULL)
> > on the clkdiv division right? I mean the round up has already been made,
> > and it shouldn't be needed twice?
> >
> > So,
> >         tmp = DIV_ROUND_UP_(ULL)(tmp, clk_div);
> >
> > could be a
> >
> >         tmp /= clk_div;
> 
> I think you still need the round up, but I wouldn't swear to it.
> You've divided by one value, but not the other and each division could
> separately need rounding.

Ok, thanks. I'll keep the round up then. I change the unsigned long to
u64 in v6.

Kind regards
Mårten
> 
> -Doug