[U-Boot] [PATCH] mmc: dw_mmc: reduce timeout detection cycle

[U-Boot] [PATCH] mmc: dw_mmc: reduce timeout detection cycle

[Ziyuan Xu]

It's no need to speed 10 seconds to wait the mmc device out from busy
status. 500 milliseconds enough.

Signed-off-by: Ziyuan Xu <xzy.xu@rock-chips.com>
---

 drivers/mmc/dw_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
index 2cf7bae..790a166 100644
--- a/drivers/mmc/dw_mmc.c
+++ b/drivers/mmc/dw_mmc.c
@@ -195,7 +195,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
 	ALLOC_CACHE_ALIGN_BUFFER(struct dwmci_idmac, cur_idmac,
 				 data ? DIV_ROUND_UP(data->blocks, 8) : 0);
 	int ret = 0, flags = 0, i;
-	unsigned int timeout = 100000;
+	unsigned int timeout = 500;
 	u32 retry = 100000;
 	u32 mask, ctrl;
 	ulong start = get_timer(0);
-- 
1.9.1

[U-Boot] [PATCH] mmc: dw_mmc: reduce timeout detection cycle

[Jaehoon Chung]

Hi Ziyuan,

On 07/19/2016 10:38 AM, Ziyuan Xu wrote:
> It's no need to speed 10 seconds to wait the mmc device out from busy
> status. 500 milliseconds enough.

I agreed that 10 seconds is too big..
Could you explain more how you get 500ms and feel enough?

Best Regards,
Jaehoon Chung

> 
> Signed-off-by: Ziyuan Xu <xzy.xu@rock-chips.com>
> ---
> 
>  drivers/mmc/dw_mmc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
> index 2cf7bae..790a166 100644
> --- a/drivers/mmc/dw_mmc.c
> +++ b/drivers/mmc/dw_mmc.c
> @@ -195,7 +195,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
>  	ALLOC_CACHE_ALIGN_BUFFER(struct dwmci_idmac, cur_idmac,
>  				 data ? DIV_ROUND_UP(data->blocks, 8) : 0);
>  	int ret = 0, flags = 0, i;
> -	unsigned int timeout = 100000;
> +	unsigned int timeout = 500;
>  	u32 retry = 100000;
>  	u32 mask, ctrl;
>  	ulong start = get_timer(0);
> 

[U-Boot] [PATCH] mmc: dw_mmc: reduce timeout detection cycle

[Ziyuan Xu]

Hi Jaehoon,

On 2016?07?19? 10:03, Jaehoon Chung wrote:
> Hi Ziyuan,
>
> On 07/19/2016 10:38 AM, Ziyuan Xu wrote:
>> It's no need to speed 10 seconds to wait the mmc device out from busy
>> status. 500 milliseconds enough.
> I agreed that 10 seconds is too big..
> Could you explain more how you get 500ms and feel enough?
Ordinarily, there are 3 types of scenarios that the mmc device was busy:
1. The mmc interface didn't initialize (eg. gpio  iomux)
The device will be busy status until gpio iomux.

2. The last command with data transfer.
The maximum value of data timeout is 0xffffff cyles(see dwi databook 
Timeout Register), and the clock is up to 52MHZ under high speed mode.
timeout = 0xffffff * 1/52M = 0.32s

3. voltage switch
U-BOOT doesn't support voltage switch.

In summary, I think 500 milliseconds is enough. What do you think?

>
> Best Regards,
> Jaehoon Chung
>
>> Signed-off-by: Ziyuan Xu <xzy.xu@rock-chips.com>
>> ---
>>
>>   drivers/mmc/dw_mmc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
>> index 2cf7bae..790a166 100644
>> --- a/drivers/mmc/dw_mmc.c
>> +++ b/drivers/mmc/dw_mmc.c
>> @@ -195,7 +195,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
>>   	ALLOC_CACHE_ALIGN_BUFFER(struct dwmci_idmac, cur_idmac,
>>   				 data ? DIV_ROUND_UP(data->blocks, 8) : 0);
>>   	int ret = 0, flags = 0, i;
>> -	unsigned int timeout = 100000;
>> +	unsigned int timeout = 500;
>>   	u32 retry = 100000;
>>   	u32 mask, ctrl;
>>   	ulong start = get_timer(0);
>>
>
>
>

[U-Boot] [PATCH] mmc: dw_mmc: reduce timeout detection cycle

[Jaehoon Chung]

Hi Ziyuan,

On 07/19/2016 11:33 AM, Ziyuan Xu wrote:
> Hi Jaehoon,
> 
> On 2016?07?19? 10:03, Jaehoon Chung wrote:
>> Hi Ziyuan,
>>
>> On 07/19/2016 10:38 AM, Ziyuan Xu wrote:
>>> It's no need to speed 10 seconds to wait the mmc device out from busy
>>> status. 500 milliseconds enough.
>> I agreed that 10 seconds is too big..
>> Could you explain more how you get 500ms and feel enough?
> Ordinarily, there are 3 types of scenarios that the mmc device was busy:
> 1. The mmc interface didn't initialize (eg. gpio  iomux)
> The device will be busy status until gpio iomux.
> 
> 2. The last command with data transfer.
> The maximum value of data timeout is 0xffffff cyles(see dwi databook Timeout Register), and the clock is up to 52MHZ under high speed mode.
> timeout = 0xffffff * 1/52M = 0.32s
> 
> 3. voltage switch
> U-BOOT doesn't support voltage switch.
> 
> In summary, I think 500 milliseconds is enough. What do you think?

I think it's not important thing. 

This is for checking whether card is busy or not before sending command.
I think it's not relevant to Timeout register. Just ensure that card is not busy before sending command.
And there is no effect for I/O performance, isn't?

But 50ms is not bad. :) It's personal preference.

Best Regards,
Jaehoon Chung

> 
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>> Signed-off-by: Ziyuan Xu <xzy.xu@rock-chips.com>
>>> ---
>>>
>>>   drivers/mmc/dw_mmc.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
>>> index 2cf7bae..790a166 100644
>>> --- a/drivers/mmc/dw_mmc.c
>>> +++ b/drivers/mmc/dw_mmc.c
>>> @@ -195,7 +195,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
>>>       ALLOC_CACHE_ALIGN_BUFFER(struct dwmci_idmac, cur_idmac,
>>>                    data ? DIV_ROUND_UP(data->blocks, 8) : 0);
>>>       int ret = 0, flags = 0, i;
>>> -    unsigned int timeout = 100000;
>>> +    unsigned int timeout = 500;
>>>       u32 retry = 100000;
>>>       u32 mask, ctrl;
>>>       ulong start = get_timer(0);
>>>
>>
>>
>>
> 
> 
> 
> 
> 

[U-Boot] [PATCH] mmc: dw_mmc: reduce timeout detection cycle

[Ziyuan Xu]

Hi Jaehoon,

On 2016?07?19? 12:22, Jaehoon Chung wrote:
> Hi Ziyuan,
>
> On 07/19/2016 11:33 AM, Ziyuan Xu wrote:
>> Hi Jaehoon,
>>
>> On 2016?07?19? 10:03, Jaehoon Chung wrote:
>>> Hi Ziyuan,
>>>
>>> On 07/19/2016 10:38 AM, Ziyuan Xu wrote:
>>>> It's no need to speed 10 seconds to wait the mmc device out from busy
>>>> status. 500 milliseconds enough.
>>> I agreed that 10 seconds is too big..
>>> Could you explain more how you get 500ms and feel enough?
>> Ordinarily, there are 3 types of scenarios that the mmc device was busy:
>> 1. The mmc interface didn't initialize (eg. gpio  iomux)
>> The device will be busy status until gpio iomux.
>>
>> 2. The last command with data transfer.
>> The maximum value of data timeout is 0xffffff cyles(see dwi databook Timeout Register), and the clock is up to 52MHZ under high speed mode.
>> timeout = 0xffffff * 1/52M = 0.32s
>>
>> 3. voltage switch
>> U-BOOT doesn't support voltage switch.
>>
>> In summary, I think 500 milliseconds is enough. What do you think?
> I think it's not important thing.
>
> This is for checking whether card is busy or not before sending command.
> I think it's not relevant to Timeout register. Just ensure that card is not busy before sending command.
> And there is no effect for I/O performance, isn't?
Yup,  I agree with you.  For scenarios 2, I mean that if the last 
command with data transfer, we will hit data_busy assertion probably. If 
the mmc device remains in a busy state more than 500ms, I think it may 
also be busy state after 10s.
>
> But 50ms is not bad. :) It's personal preference.
BTW, the timeout value is 500ms in kernel.
>
> Best Regards,
> Jaehoon Chung
>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>>> Signed-off-by: Ziyuan Xu <xzy.xu@rock-chips.com>
>>>> ---
>>>>
>>>>    drivers/mmc/dw_mmc.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
>>>> index 2cf7bae..790a166 100644
>>>> --- a/drivers/mmc/dw_mmc.c
>>>> +++ b/drivers/mmc/dw_mmc.c
>>>> @@ -195,7 +195,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
>>>>        ALLOC_CACHE_ALIGN_BUFFER(struct dwmci_idmac, cur_idmac,
>>>>                     data ? DIV_ROUND_UP(data->blocks, 8) : 0);
>>>>        int ret = 0, flags = 0, i;
>>>> -    unsigned int timeout = 100000;
>>>> +    unsigned int timeout = 500;
>>>>        u32 retry = 100000;
>>>>        u32 mask, ctrl;
>>>>        ulong start = get_timer(0);
>>>>
>>>
>>>
>>
>>
>>
>>
>
>
>

[U-Boot] [PATCH] mmc: dw_mmc: reduce timeout detection cycle

[Jaehoon Chung]

Hi,

On 07/19/2016 04:40 PM, Ziyuan Xu wrote:
> Hi Jaehoon,
> 
> On 2016?07?19? 12:22, Jaehoon Chung wrote:
>> Hi Ziyuan,
>>
>> On 07/19/2016 11:33 AM, Ziyuan Xu wrote:
>>> Hi Jaehoon,
>>>
>>> On 2016?07?19? 10:03, Jaehoon Chung wrote:
>>>> Hi Ziyuan,
>>>>
>>>> On 07/19/2016 10:38 AM, Ziyuan Xu wrote:
>>>>> It's no need to speed 10 seconds to wait the mmc device out from busy
>>>>> status. 500 milliseconds enough.
>>>> I agreed that 10 seconds is too big..
>>>> Could you explain more how you get 500ms and feel enough?
>>> Ordinarily, there are 3 types of scenarios that the mmc device was busy:
>>> 1. The mmc interface didn't initialize (eg. gpio  iomux)
>>> The device will be busy status until gpio iomux.
>>>
>>> 2. The last command with data transfer.
>>> The maximum value of data timeout is 0xffffff cyles(see dwi databook Timeout Register), and the clock is up to 52MHZ under high speed mode.
>>> timeout = 0xffffff * 1/52M = 0.32s
>>>
>>> 3. voltage switch
>>> U-BOOT doesn't support voltage switch.
>>>
>>> In summary, I think 500 milliseconds is enough. What do you think?
>> I think it's not important thing.
>>
>> This is for checking whether card is busy or not before sending command.
>> I think it's not relevant to Timeout register. Just ensure that card is not busy before sending command.
>> And there is no effect for I/O performance, isn't?
> Yup,  I agree with you.  For scenarios 2, I mean that if the last command with data transfer, we will hit data_busy assertion probably. If the mmc device remains in a busy state more than 500ms, I think it may also be busy state after 10s.
>>
>> But 50ms is not bad. :) It's personal preference.
> BTW, the timeout value is 500ms in kernel.

Yep, it looks good to me. :)
I have tested your patch with Exynos SoCs.

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
Tested-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

>>
>> Best Regards,
>> Jaehoon Chung
>>
>>>> Best Regards,
>>>> Jaehoon Chung
>>>>
>>>>> Signed-off-by: Ziyuan Xu <xzy.xu@rock-chips.com>
>>>>> ---
>>>>>
>>>>>    drivers/mmc/dw_mmc.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
>>>>> index 2cf7bae..790a166 100644
>>>>> --- a/drivers/mmc/dw_mmc.c
>>>>> +++ b/drivers/mmc/dw_mmc.c
>>>>> @@ -195,7 +195,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
>>>>>        ALLOC_CACHE_ALIGN_BUFFER(struct dwmci_idmac, cur_idmac,
>>>>>                     data ? DIV_ROUND_UP(data->blocks, 8) : 0);
>>>>>        int ret = 0, flags = 0, i;
>>>>> -    unsigned int timeout = 100000;
>>>>> +    unsigned int timeout = 500;
>>>>>        u32 retry = 100000;
>>>>>        u32 mask, ctrl;
>>>>>        ulong start = get_timer(0);
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>
>>
>>
> 
> 
> 
> 
> 

[U-Boot] [PATCH] mmc: dw_mmc: reduce timeout detection cycle

[Ziyuan Xu]

Hi Jaehoon,

On 2016?07?19? 15:51, Jaehoon Chung wrote:
> Hi,
>
> On 07/19/2016 04:40 PM, Ziyuan Xu wrote:
>> Hi Jaehoon,
>>
>> On 2016?07?19? 12:22, Jaehoon Chung wrote:
>>> Hi Ziyuan,
>>>
>>> On 07/19/2016 11:33 AM, Ziyuan Xu wrote:
>>>> Hi Jaehoon,
>>>>
>>>> On 2016?07?19? 10:03, Jaehoon Chung wrote:
>>>>> Hi Ziyuan,
>>>>>
>>>>> On 07/19/2016 10:38 AM, Ziyuan Xu wrote:
>>>>>> It's no need to speed 10 seconds to wait the mmc device out from busy
>>>>>> status. 500 milliseconds enough.
>>>>> I agreed that 10 seconds is too big..
>>>>> Could you explain more how you get 500ms and feel enough?
>>>> Ordinarily, there are 3 types of scenarios that the mmc device was busy:
>>>> 1. The mmc interface didn't initialize (eg. gpio  iomux)
>>>> The device will be busy status until gpio iomux.
>>>>
>>>> 2. The last command with data transfer.
>>>> The maximum value of data timeout is 0xffffff cyles(see dwi databook Timeout Register), and the clock is up to 52MHZ under high speed mode.
>>>> timeout = 0xffffff * 1/52M = 0.32s
>>>>
>>>> 3. voltage switch
>>>> U-BOOT doesn't support voltage switch.
>>>>
>>>> In summary, I think 500 milliseconds is enough. What do you think?
>>> I think it's not important thing.
>>>
>>> This is for checking whether card is busy or not before sending command.
>>> I think it's not relevant to Timeout register. Just ensure that card is not busy before sending command.
>>> And there is no effect for I/O performance, isn't?
>> Yup,  I agree with you.  For scenarios 2, I mean that if the last command with data transfer, we will hit data_busy assertion probably. If the mmc device remains in a busy state more than 500ms, I think it may also be busy state after 10s.
>>> But 50ms is not bad. :) It's personal preference.
>> BTW, the timeout value is 500ms in kernel.
> Yep, it looks good to me. :)
> I have tested your patch with Exynos SoCs.
>
> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
> Tested-by: Jaehoon Chung <jh80.chung@samsung.com>
Thanks for your test and review!
>
> Best Regards,
> Jaehoon Chung
>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>>>> Best Regards,
>>>>> Jaehoon Chung
>>>>>
>>>>>> Signed-off-by: Ziyuan Xu <xzy.xu@rock-chips.com>
>>>>>> ---
>>>>>>
>>>>>>     drivers/mmc/dw_mmc.c | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
>>>>>> index 2cf7bae..790a166 100644
>>>>>> --- a/drivers/mmc/dw_mmc.c
>>>>>> +++ b/drivers/mmc/dw_mmc.c
>>>>>> @@ -195,7 +195,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
>>>>>>         ALLOC_CACHE_ALIGN_BUFFER(struct dwmci_idmac, cur_idmac,
>>>>>>                      data ? DIV_ROUND_UP(data->blocks, 8) : 0);
>>>>>>         int ret = 0, flags = 0, i;
>>>>>> -    unsigned int timeout = 100000;
>>>>>> +    unsigned int timeout = 500;
>>>>>>         u32 retry = 100000;
>>>>>>         u32 mask, ctrl;
>>>>>>         ulong start = get_timer(0);
>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>>
>>
>
>
>

[U-Boot] [PATCH] mmc: dw_mmc: reduce timeout detection cycle

[Simon Glass]

On 19 July 2016 at 02:08, Ziyuan Xu <xzy.xu@rock-chips.com> wrote:
> Hi Jaehoon,
>
>
> On 2016?07?19? 15:51, Jaehoon Chung wrote:
>>
>> Hi,
>>
>> On 07/19/2016 04:40 PM, Ziyuan Xu wrote:
>>>
>>> Hi Jaehoon,
>>>
>>> On 2016?07?19? 12:22, Jaehoon Chung wrote:
>>>>
>>>> Hi Ziyuan,
>>>>
>>>> On 07/19/2016 11:33 AM, Ziyuan Xu wrote:
>>>>>
>>>>> Hi Jaehoon,
>>>>>
>>>>> On 2016?07?19? 10:03, Jaehoon Chung wrote:
>>>>>>
>>>>>> Hi Ziyuan,
>>>>>>
>>>>>> On 07/19/2016 10:38 AM, Ziyuan Xu wrote:
>>>>>>>
>>>>>>> It's no need to speed 10 seconds to wait the mmc device out from busy
>>>>>>> status. 500 milliseconds enough.
>>>>>>
>>>>>> I agreed that 10 seconds is too big..
>>>>>> Could you explain more how you get 500ms and feel enough?
>>>>>
>>>>> Ordinarily, there are 3 types of scenarios that the mmc device was
>>>>> busy:
>>>>> 1. The mmc interface didn't initialize (eg. gpio  iomux)
>>>>> The device will be busy status until gpio iomux.
>>>>>
>>>>> 2. The last command with data transfer.
>>>>> The maximum value of data timeout is 0xffffff cyles(see dwi databook
>>>>> Timeout Register), and the clock is up to 52MHZ under high speed mode.
>>>>> timeout = 0xffffff * 1/52M = 0.32s
>>>>>
>>>>> 3. voltage switch
>>>>> U-BOOT doesn't support voltage switch.
>>>>>
>>>>> In summary, I think 500 milliseconds is enough. What do you think?
>>>>
>>>> I think it's not important thing.
>>>>
>>>> This is for checking whether card is busy or not before sending command.
>>>> I think it's not relevant to Timeout register. Just ensure that card is
>>>> not busy before sending command.
>>>> And there is no effect for I/O performance, isn't?
>>>
>>> Yup,  I agree with you.  For scenarios 2, I mean that if the last command
>>> with data transfer, we will hit data_busy assertion probably. If the mmc
>>> device remains in a busy state more than 500ms, I think it may also be busy
>>> state after 10s.
>>>>
>>>> But 50ms is not bad. :) It's personal preference.
>>>
>>> BTW, the timeout value is 500ms in kernel.
>>
>> Yep, it looks good to me. :)
>> I have tested your patch with Exynos SoCs.
>>
>> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
>> Tested-by: Jaehoon Chung <jh80.chung@samsung.com>
>
> Thanks for your test and review!
>

Let's get others to test this.

Applied to u-boot-dm, thanks!