Re: [PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 11/11/22 05:16, Bayi Cheng (程八意) ha scritto:
> On Fri, 2022-11-04 at 07:53 +0000, Bayi Cheng (程八意) wrote:
>> On Thu, 2022-11-03 at 22:35 +0000, David Laight wrote:
>>> From: AngeloGioacchino Del Regno
>>>> Sent: 03 November 2022 09:44
>>>>
>>>> Il 03/11/22 06:28, Bayi Cheng ha scritto:
>>>>> From: bayi cheng <bayi.cheng@mediatek.com>
>>>>>
>>>>> The timeout value of the current dma read is unreasonable. For
>>>>> example,
>>>>> If the spi flash clock is 26Mhz, It will takes about 1.3ms to
>>>>> read a
>>>>> 4KB data in spi mode. But the actual measurement exceeds 50s
>>>>> when
>>>>> a
>>>>> dma read timeout is encountered.
>>>>>
>>>>> In order to be more accurately, It is necessary to use
>>>>> msecs_to_jiffies,
>>>>> After modification, the measured timeout value is about 130ms.
>>>>>
>>>>> Signed-off-by: bayi cheng <bayi.cheng@mediatek.com>
>>>>> ---
>>>>>    drivers/spi/spi-mtk-nor.c | 7 ++++---
>>>>>    1 file changed, 4 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-
>>>>> nor.c
>>>>> index d167699a1a96..3d989db80ee9 100644
>>>>> --- a/drivers/spi/spi-mtk-nor.c
>>>>> +++ b/drivers/spi/spi-mtk-nor.c
>>>>> @@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct mtk_nor
>>>>> *sp, u32 from, unsigned int length,
>>>>>    			    dma_addr_t dma_addr)
>>>>>    {
>>>>>    	int ret = 0;
>>>>> -	ulong delay;
>>>>> +	ulong delay, timeout;
>>>>>    	u32 reg;
>>>>>
>>>>>    	writel(from, sp->base + MTK_NOR_REG_DMA_FADR);
>>>>> @@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct
>>>>> mtk_nor
>>>>> *sp, u32 from, unsigned int length,
>>>>>    	mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START,
>>>>> 0);
>>>>>
>>>>>    	delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE);
>>>>> +	timeout = (delay + 1) * 100;
>>>>>
>>>>>    	if (sp->has_irq) {
>>>>>    		if (!wait_for_completion_timeout(&sp->op_done,
>>>>> -						 (delay + 1) *
>>>>> 100))
>>>>> +		    msecs_to_jiffies(max_t(size_t, timeout /
>>>>> 1000,
>>>>> 10))))
>>>>
>>>> You're giving a `size_t` variable to msecs_to_jiffies(), but
>>>> checking `jiffies.h`,
>>>> this function takes a `const unsigned int` param.
>>>> Please change the type to match that.
>>>
>>> The type shouldn't matter at all.
>>> What matters is the domain of the value.
>>>
>>> Quite why you need to use max_t(size_t, ...) is another matter.
>>> timeout is ulong so max(timeout/1000, 10ul) should be fine.
>>>
>>> But is ulong even right?
>>> The domain of the value is almost certainly the same on 32bit and
>>> 64bit.
>>> So you almost certainly need u32 or u64.
>>>
>>> 	David
>>>
>>
>> Hi David & Angelo
>>
>> Thank you for your comments!
>> To sum up, I think the next version will make the following two
>> changes:
>> 1, The timeout value will not exceed u32, so the type of timeout will
>> be changed from ulong to u32.
>> 2, Change msecs_to_jiffies(max_t(size_t, timeout / 1000, 10)) to be
>> msecs_to_jiffies(max(timeout / 1000, 10U)).
>>
>> If you think these changes are not enough, please let me know,
>> Thanks！
>>
>> Best Regards,
>> Bayi
>>
> 
> Hi Angelo, Hi David,
> 
> Just a gentle ping on this.
> Could you please review this patch and give us some suggestion?
> 
> PS: With your permission, I will make the following changes in the next
> version:
> 
> Change in v2:
>    -Change the type of "timeout" from ulong to u32.
>    -Replace max_t with max.
> 

I still recommend to use usecs_to_jiffies() when appropriate, instead of
converting usecs to msecs and using msecs_to_jiffies().

As for the rest in your list: yes, please.

Regards,
Angelo

> 
> Thanks.
> 
> BRs,
> Bayi Cheng
> 
>>>>
>>>> Aside from that, your `timeout` variable contains a timeout in
>>>> microseconds and
>>>> this means that actually using msecs_to_jiffies() is suboptimal
>>>> here.
>>>>
>>>> Please use usecs_to_jiffies() instead.
>>>>
>>>> Regards,
>>>> Angelo
>>>
>>> -
>>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton
>>> Keynes,
>>> MK1 1PT, UK
>>> Registration No: 1397386 (Wales)

-- 
AngeloGioacchino Del Regno
Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718