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

[Marten Lindahl <martenli@axis.com>]

On Tue, Nov 09, 2021 at 12:46:17AM +0100, Jaehoon Chung wrote:
> Hi Marten,

Hi Jaehoon!

> 
> On 11/8/21 8:36 PM, Mårten Lindahl 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>
> > ---
> > 
> > v2:
> >  - Calculate new value before checking boundaries
> >  - Include CLKDIV register to get proper value
> > 
> >  drivers/mmc/host/dw_mmc.c | 32 +++++++++++++++++++++++++++++++-
> >  1 file changed, 31 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > index 6578cc64ae9e..6edd7a231448 100644
> > --- a/drivers/mmc/host/dw_mmc.c
> > +++ b/drivers/mmc/host/dw_mmc.c
> > @@ -1283,6 +1283,36 @@ 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 * clk_div);
> > +
> > +	if (!tmp || tmp > 0xFFFFFF) {
> > +		/* Set maximum */
> 
> "Set maximum value about all Timeout"?

Do you mean just changing the comment here? Or do you wonder about the
0xFFFFFF check? 0xFFFFFF is the upper limit for this HW timer. If we
want to support a longer timer than this, a software timer should be
used, but in a separate patch.

> 
> > +		tmout = 0xFFFFFFFF;
> > +		goto tmout_done;
> > +	}
> 
> It doesn't need to use "goto". Instead, if-else can be used.

If you prefer it I can change goto to if-else

> 
> > +
> > +	/* TMOUT[7:0] (RESPONSE_TIMEOUT) */
> > +	tmout = 0xFF; /* Set maximum */
> 
> To prevent a confusion, how about add "Set a maximum response timeout"
> And this line can be removed.

But if removing the lines above, the comment will also be removed. I see
your point, but couldn't there be more confusion by merging both fields
into one line? My intention was to specify the TMOUT register fields
separately to make it more clear.

> 
> > +
> > +	/* TMOUT[31:8] (DATA_TIMEOUT) */
> > +	tmout |= (tmp & 0xFFFFFF) << 8;
> 
> tmout = (0xFF | ((tmp & 0xFFFFFF) << 8));
> 
> The entire code can be below
> 
> if (!tmp || ....)
> 	tmout = 0xFFFFFFFF;
> else 
> 	tmout = (0xFF | ((tmp & 0xFFFFFF) << 8));
> 
> writel(TMOUT, ...)
> 
> How about this?

I agree that this is smaller code, but as I said above it may not be
clear that there are more than one field in the TMOUT register. Wouldn't
it raise questions about the 0xFF?

Kind regards
Mårten

> 
> Best Regards,
> Jaehoon Chung
> 
> > +
> > +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 +1333,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);
> >  	}
> > 
>