[U-Boot] [RFC] mmc:fix: Increase the timeout value for SDHCI_send_command()

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

Hi Lukasz,

On 01/28/2013 04:02 PM, Lukasz Majewski wrote:
> Hi Jaehoon,
> 
>> On 01/25/2013 08:44 PM, Jagan Teki wrote:
>>> Hi,
>>>
>>> On Fri, Jan 11, 2013 at 8:49 PM, Lukasz Majewski
>>> <l.majewski@samsung.com> wrote:
>>>> Hi Wolfgang,
>>>>
>>>>> Dear Lukasz Majewski,
>>>>>
>>>>> In message
>>>>> <1357665792-8141-1-git-send-email-l.majewski@samsung.com> you
>>>>> wrote:
>>>>>> I'd like to ask for your opinion about the following problem:
>>>>>
>>>>> I cannot comment on the problem - only a bit about the proposed
>>>>> patch ;-)
>>>>>
>>>>>> From a brief checking I can say that it happens when we are doing
>>>>>> consecutive MMC operations (i.e. many reads), and the 10ms
>>>>>> timeout might be too short when eMMC firmware is forced to do
>>>>>> some internal time consuming operations (e.g. flash blocks
>>>>>> management, wear leveling).
>>>>>> In this situation, the SDHCI_CMD_INHIBIT bit is set, which means
>>>>>> that SDHCI controller didn't received response from eMMC.
>>>>>>
>>>>>> One proposition would be to define the per device/per memory chip
>>>>>> specific timeouts, to replace those defined
>>>>>> at ./drivers/mmc/sdhci.c file.
>>>>>
>>>>> Is there no way to ask the device and/or controller when it is
>>>>> done, so we can poll for ready state instead of adding delays,
>>>>> which will always have to be tailored for the so far known worst
>>>>> case, i. e. they will be always too long on all almost all
>>>>> systems.
>>>>
>>>> We are doing this already - the SDHCI_PRESENT_STATE register's bit
>>>> 0 (SDHCI_CMD_INHIBIT) and bit 1 (DATA_INHIBIT) are for this
>>>> purpose. Those indicate when host controller can send further
>>>> command/data to the card.
>>>>
>>>> Moreover, there are also timeouts in the case when someone pull
>>>> out SD card inserted to the slot (or any other use case which I'm
>>>> not aware).
>>>>
>>>>
>>>>>
>>>>>> --- a/drivers/mmc/sdhci.c
>>>>>> +++ b/drivers/mmc/sdhci.c
>>>>>> @@ -137,8 +137,8 @@ int sdhci_send_command(struct mmc *mmc,
>>>>>> struct mmc_cmd *cmd, unsigned int timeout, start_addr = 0;
>>>>>>     unsigned int retry = 10000;
>>>>>>
>>>>>> -   /* Wait max 10 ms */
>>>>>> -   timeout = 10;
>>>>>> +   /* Wait max 100 ms */
>>>>>> +   timeout = 100;
>>>>>
>>>>> We have cases where we struggle for sub-second boot times.  Adding
>>>>> 100 ms delay here is clearly prohbitive.  [Even the 10 ms are way
>>>>> too long IMHO.]  There must be a better way to handle this.
>>>>
>>>> That's why I'm asking.
>>>>
>>>> It is strange that, when I'm increasing delay it works.
>>>>
>>>> Maybe we will find some areas of optimization?
>>>
>>> BTW: I am also finding the similar issue.
>>>
>>> But when I enabled CONFIG_MMC_TRACE for log traces, i never see the
>>> issue..it's pretty much working fine.
>> It's not important to enable the MMC_TRACE. It should be increased
>> the delay.
> 
> You don't see problem, since CONFIG_MMC_TRACE causes extra delays to
> write log information to serial console.
Right...So i mentioned that CONFIG_MMC_TRACE isn't important.(added delay).
> 
>>> As per my latest debug, the issue is fire for CMD6 (SWITCH_FUNC).
>> Right, i also find the error log for CMD6.
>> Could you test this point?
>>
>>  	sdhci_writel(host, SDHCI_INT_ALL_MASK, SDHCI_INT_STATUS);
>> -	mask = SDHCI_CMD_INHIBIT | SDHCI_DATA_INHIBIT;
>> +	mask = SDHCI_CMD_INHIBIT;
>> +
>> +	if ((data != NULL) || (cmd->resp_type & MMC_RSP_BUSY))
>> +		mask |= SDHCI_DATA_INHIBIT;
> 
> I've tested it on trats, but mentioned errors also appear (with lower
> frequency though).
Right..But I think that this patch is correct regardless of this problem.
if you agree this, i will send the patch with this.
> 
> 
>>
>> Best Regards,
>> Jaehoon Chung
>>>
>>> May be we need to update the logic on this while loop...
how about using get_timer()?

Best Regards,
Jaehoon Chung
> 
> Yep.
> 
>>>
>>> Thanks,
>>> Jagan.
>>>
>>>>
>>>>>
>>>>> Best regards,
>>>>>
>>>>> Wolfgang Denk
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Best regards,
>>>>
>>>> Lukasz Majewski
>>>>
>>>> Samsung R&D Poland (SRPOL) | Linux Platform Group
>>>> _______________________________________________
>>>> U-Boot mailing list
>>>> U-Boot at lists.denx.de
>>>> http://lists.denx.de/mailman/listinfo/u-boot
>>> _______________________________________________
>>> U-Boot mailing list
>>> U-Boot at lists.denx.de
>>> http://lists.denx.de/mailman/listinfo/u-boot
>>>
>>
> 
> 
>