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

[Lukasz Majewski]

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

You don't see problem, since CONFIG_MMC_TRACE causes extra delays to
write log information to serial console.

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


> 
> Best Regards,
> Jaehoon Chung
> > 
> > May be we need to update the logic on this while loop...

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



-- 
Best regards,

Lukasz Majewski

Samsung R&D Poland (SRPOL) | Linux Platform Group