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

[PATCH v2] 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.
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 */
+		tmout = 0xFFFFFFFF;
+		goto tmout_done;
+	}
+
+	/* TMOUT[7:0] (RESPONSE_TIMEOUT) */
+	tmout = 0xFF; /* Set maximum */
+
+	/* TMOUT[31:8] (DATA_TIMEOUT) */
+	tmout |= (tmp & 0xFFFFFF) << 8;
+
+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);
 	}
-- 
2.20.1

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

[Jaehoon Chung]

Hi Marten,

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"?

> +		tmout = 0xFFFFFFFF;
> +		goto tmout_done;
> +	}

It doesn't need to use "goto". Instead, if-else can be used.

> +
> +	/* 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.

> +
> +	/* 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?

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);
>  	}
> 

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

[Marten Lindahl]

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);
> >  	}
> > 
> 

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

[Jaehoon Chung]

On 11/9/21 10:27 PM, Marten Lindahl wrote:
> 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.

My mean is how about changing the comment to clarify than now.
At below, tmout is set to maximum value about data/response timeout.

> 
>>
>>> +		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

Well, it's just my  preference. If you're ok, I hope that so.

> 
>>
>>> +
>>> +	/* 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.

Agreed. It's more clear than my opinion.

Best Regards,
Jaehoon Chung

> 
>>
>>> +
>>> +	/* 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);
>>>  	}
>>>
>>
>