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

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

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.
> 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;

Best Regards,
Jaehoon Chung
> 
> May be we need to update the logic on this while loop...
> 
> 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
>