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

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

[Lukasz Majewski]

I'd like to ask for your opinion about the following problem:

TRATS # saveenv
Saving Environment to MMC...
Writing to MMC(0)... Controller never released inhibit bit(s).
Controller never released inhibit bit(s).
Controller never released inhibit bit(s).
...
failed

The same is for e.g. ext4.

The provided patch seems to solve the problem, but I DO NOT think that
increasing delay is an acceptable solution to any problem.

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

[Wolfgang Denk]

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.

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

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
f u cn rd ths, u cn gt a gd jb n cmptr prgrmmng.

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

[Jaehoon Chung]

Hi All,

I think this problem is produced when card is running write/erase operation.
We used the mmc_send_status() into driver/mmc/mmc.c.
When That command is sending, i found the inhibit released log.

I think problem that SDHCI_DATA_INHIBIT is set at every command.
if didn't have data and response type is not busy-wait type,
SDHCI_DATA_INHIBIT didn't need to set.

How about this? It is more reasonable than increasing timeout value.

@@ -141,7 +143,10 @@ int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
 	timeout = 10;
 
 	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;
 
 	/* We shouldn't wait for data inihibit for stop commands, even
 	   though they might use busy signaling */

Best Regards,
Jaehoon Chung

On 01/10/2013 05:12 AM, Wolfgang Denk wrote:
> 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.
> 
>> --- 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.
> 
> Best regards,
> 
> Wolfgang Denk
> 

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

[Lukasz Majewski]

Hi Jaehoon,

> Hi All,
> 
> I think this problem is produced when card is running write/erase
> operation. We used the mmc_send_status() into driver/mmc/mmc.c.
> When That command is sending, i found the inhibit released log.
> 
> I think problem that SDHCI_DATA_INHIBIT is set at every command.
> if didn't have data and response type is not busy-wait type,
> SDHCI_DATA_INHIBIT didn't need to set.
> 
> How about this? It is more reasonable than increasing timeout value.
> 
> @@ -141,7 +143,10 @@ int sdhci_send_command(struct mmc *mmc, struct
> mmc_cmd *cmd, timeout = 10;
>  
>  	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 will test this and let you know.

>  	/* We shouldn't wait for data inihibit for stop commands,
> even though they might use busy signaling */
> 
> Best Regards,
> Jaehoon Chung
> 
> On 01/10/2013 05:12 AM, Wolfgang Denk wrote:
> > 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.
> > 
> >> --- 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.
> > 
> > Best regards,
> > 
> > Wolfgang Denk
> > 
> 



-- 
Best regards,

Lukasz Majewski

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

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

[Lukasz Majewski]

Hi Jaehoon,

> Hi All,
> 
> I think this problem is produced when card is running write/erase
> operation. We used the mmc_send_status() into driver/mmc/mmc.c.
> When That command is sending, i found the inhibit released log.
> 
> I think problem that SDHCI_DATA_INHIBIT is set at every command.
> if didn't have data and response type is not busy-wait type,
> SDHCI_DATA_INHIBIT didn't need to set.
> 
> How about this? It is more reasonable than increasing timeout value.
> 
> @@ -141,7 +143,10 @@ int sdhci_send_command(struct mmc *mmc, struct
> mmc_cmd *cmd, timeout = 10;
>  
>  	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;
>  

Ive tested this code and data abort appears when used with ext4.

I think, that we need to look around for another solution.

>  	/* We shouldn't wait for data inihibit for stop commands,
> even though they might use busy signaling */
> 
> Best Regards,
> Jaehoon Chung
> 
> On 01/10/2013 05:12 AM, Wolfgang Denk wrote:
> > 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.
> > 
> >> --- 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.
> > 
> > Best regards,
> > 
> > Wolfgang Denk
> > 
> 



-- 
Best regards,

Lukasz Majewski

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

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

[Lukasz Majewski]

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?

> 
> Best regards,
> 
> Wolfgang Denk
> 



-- 
Best regards,

Lukasz Majewski

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

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

[Jagan Teki]

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.
As per my latest debug, the issue is fire for CMD6 (SWITCH_FUNC).

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] [RFC] mmc:fix: Increase the timeout value for SDHCI_send_command()

[Jaehoon Chung]

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
> 

[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@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@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;

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

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

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

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

[Jaehoon Chung]

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