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

[Marten Lindahl <martenli@axis.com>]

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