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

[Jagan Teki]

Hi Jaehoon Chung,

On Sat, Jan 26, 2013 at 6:01 AM, Jaehoon Chung <jh80.chung at samsung.com> wrote:
> On 01/25/2013 08:44 PM, Jagan Teki wrote:
>> Hi,
>>
>> On Fri, Jan 11, 2013 at 8:49 PM, Lukasz Majewski <l.majewski at samsung.com> wrote:
>>> Hi Wolfgang,
>>>
>>>> Dear Lukasz Majewski,
>>>>
>>>> In message <1357665792-8141-1-git-send-email-l.majewski at 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;

I found similar issue, no changes...

for masking data, the mask is ORed in CMD51 and CMD6 cases.
 mask |= SDHCI_DATA_INHIBIT;

But I have tried by putting the delay between the command transfer like..

        if (host->quirks & SDHCI_QUIRK_WAIT_SEND_CMD)
                udelay(1000);

I just enabled the above quirks on my sdhci driver, everything work fine.
But again I don't now does this delay really required?, or it may
causes any harm?

Thanks,
Jagan.

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