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

[Jaehoon Chung <jh80.chung@samsung.com>]

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