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").
> + 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
