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