[U-Boot] [PATCH] mmc:sdhci: Fix card ready status timeout.

[U-Boot] [PATCH] mmc:sdhci: Fix card ready status timeout.

[Przemyslaw Marczak]

According to JEDEC eMMC specification, after data transfer
(multiple or single block) host must wait for card ready
status. This is done by waiting for command and data lines
to be at idle state after transfer. JEDEC does not specify
maximum timeout.

Before this change max timeout was 10 ms but in case of UMS
- when system does multiple read/write operations on random
card blocks - timeout causes I/O errors.
The timeout has been increased to 200ms after data transfer.
For other transfers it stays unchanged.

Tested on Goni and Trats.

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/mmc/sdhci.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
index 4261991..c495482 100644
--- a/drivers/mmc/sdhci.c
+++ b/drivers/mmc/sdhci.c
@@ -121,8 +121,18 @@ 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;
+	/*
+	 * For some commands this function is called with NULL mmc_data
+	 * pointer. One of those is CMD13 - send card status.
+	 * After read/write data transfer or block erase commands - host sends
+	 * CMD13 and is waiting for card ready status with some timeout.
+	 * According to some internal cards operations after those commands
+	 * this time must be increased.
+	 */
+	if (data)
+		timeout = 10; /* ms */
+	else
+		timeout = 200;
 
 	sdhci_writel(host, SDHCI_INT_ALL_MASK, SDHCI_INT_STATUS);
 	mask = SDHCI_CMD_INHIBIT | SDHCI_DATA_INHIBIT;
-- 
1.7.9.5

[U-Boot] [PATCH] mmc:sdhci: Fix card ready status timeout.

[Pantelis Antoniou]

Hi there,

On Sep 3, 2013, at 3:50 PM, Przemyslaw Marczak wrote:

> According to JEDEC eMMC specification, after data transfer
> (multiple or single block) host must wait for card ready
> status. This is done by waiting for command and data lines
> to be at idle state after transfer. JEDEC does not specify
> maximum timeout.
> 
> Before this change max timeout was 10 ms but in case of UMS
> - when system does multiple read/write operations on random
> card blocks - timeout causes I/O errors.
> The timeout has been increased to 200ms after data transfer.
> For other transfers it stays unchanged.
> 
> Tested on Goni and Trats.
> 
> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> ---
> drivers/mmc/sdhci.c |   14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
> index 4261991..c495482 100644
> --- a/drivers/mmc/sdhci.c
> +++ b/drivers/mmc/sdhci.c
> @@ -121,8 +121,18 @@ 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;
> +	/*
> +	 * For some commands this function is called with NULL mmc_data
> +	 * pointer. One of those is CMD13 - send card status.
> +	 * After read/write data transfer or block erase commands - host sends
> +	 * CMD13 and is waiting for card ready status with some timeout.
> +	 * According to some internal cards operations after those commands
> +	 * this time must be increased.
> +	 */
> +	if (data)
> +		timeout = 10; /* ms */
> +	else
> +		timeout = 200;
> 
> 	sdhci_writel(host, SDHCI_INT_ALL_MASK, SDHCI_INT_STATUS);
> 	mask = SDHCI_CMD_INHIBIT | SDHCI_DATA_INHIBIT;
> -- 
> 1.7.9.5
> 

Can we have a config option for these two values instead of magic numbers?

With the defaults being set at 10 & 200 ms.

Regards

-- Pantelis


> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] mmc:sdhci: Fix card ready status timeout.

[Przemyslaw Marczak]

Hello Pantelis,

 > On 09/06/2013 03:24 PM, Pantelis Antoniou wrote:
> Hi there,
>
> Can we have a config option for these two values instead of magic numbers?
>
> With the defaults being set at 10 & 200 ms.
>
> Regards
>
> -- Pantelis

I'm not sure that this option is needed. Some cards I/O errors can be 
avoided by increasing timeout and has no negative influence on other 
cards read/write operations performance.
Moreover there are a lot of timeout values defined in sdhci and mmc 
drivers, so why should I put at config just only one? Maybe the simplest 
solution is to leave at this code only 200 ms value.
What do you think?

Regards,

-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH] mmc:sdhci: Fix card ready status timeout.

[Pantelis Antoniou]

Hi there,

On Sep 6, 2013, at 6:23 PM, Przemyslaw Marczak wrote:

> Hello Pantelis,
> 
> > On 09/06/2013 03:24 PM, Pantelis Antoniou wrote:
>> Hi there,
>> 
>> Can we have a config option for these two values instead of magic numbers?
>> 
>> With the defaults being set at 10 & 200 ms.
>> 
>> Regards
>> 
>> -- Pantelis
> 
> I'm not sure that this option is needed. Some cards I/O errors can be avoided by increasing timeout and has no negative influence on other cards read/write operations performance.
> Moreover there are a lot of timeout values defined in sdhci and mmc drivers, so why should I put at config just only one? Maybe the simplest solution is to leave at this code only 200 ms value.
> What do you think?
> 

Still, it's a magic constant in the code; you don't have to export it to boards, just put it in the same source file
just before it's use.

Protect it with an #ifndef statement in case someone else would like to override it.

Regards

-- Pantelis

> Regards,
> 
> -- 
> Przemyslaw Marczak
> Samsung R&D Institute Poland
> Samsung Electronics
> p.marczak at samsung.com

[U-Boot] [PATCH] mmc:sdhci: Fix card ready status timeout.

[Przemyslaw Marczak]

According to JEDEC eMMC specification, after data transfer
(multiple or single block) host must wait for card ready
status. This is done by waiting for command and data lines
to be at idle state after transfer. JEDEC does not specify
maximum timeout.

Before this change max timeout was 10 ms but in case of UMS
- when system does multiple read/write operations on random
card blocks - timeout causes I/O errors.
The timeout has been increased to 200ms after data transfer.
For other transfers it stays unchanged. Default values are
now defined with "if defined" directive so it can be redefined
at board config if needed.

Tested on Goni and Trats.

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/mmc/sdhci.c |   20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
index 4261991..35bdb37 100644
--- a/drivers/mmc/sdhci.c
+++ b/drivers/mmc/sdhci.c
@@ -110,6 +110,20 @@ static int sdhci_transfer_data(struct sdhci_host *host, struct mmc_data *data,
 	return 0;
 }
 
+#if !defined(SDHCI_DATA_CMD_TIMEOUT)
+#define SDHCI_DATA_CMD_TIMEOUT		10
+#endif
+#if !defined(SDHCI_STATUS_CMD_TIMEOUT)
+#define SDHCI_STATUS_CMD_TIMEOUT	200
+#endif
+/*
+ * For some commands this function is called with NULL mmc_data
+ * pointer. One of those is CMD13 - send card status.
+ * After read/write data transfer or block erase commands - host sends
+ * CMD13 and is waiting for card ready status with some timeout.
+ * According to some internal cards operations after those commands
+ * this time must be increased.
+ */
 int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
 		       struct mmc_data *data)
 {
@@ -121,8 +135,10 @@ 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;
+	if (data)
+		timeout = SDHCI_DATA_CMD_TIMEOUT; /* ms */
+	else
+		timeout = SDHCI_STATUS_CMD_TIMEOUT;
 
 	sdhci_writel(host, SDHCI_INT_ALL_MASK, SDHCI_INT_STATUS);
 	mask = SDHCI_CMD_INHIBIT | SDHCI_DATA_INHIBIT;
-- 
1.7.9.5

[U-Boot] [PATCH] mmc:sdhci: Fix card ready status timeout.

[Przemyslaw Marczak]

Dear Pantelis,

On 09/09/2013 02:58 PM, Przemyslaw Marczak wrote:
> According to JEDEC eMMC specification, after data transfer
> (multiple or single block) host must wait for card ready
> status. This is done by waiting for command and data lines
> to be at idle state after transfer. JEDEC does not specify
> maximum timeout.
>
> Before this change max timeout was 10 ms but in case of UMS
> - when system does multiple read/write operations on random
> card blocks - timeout causes I/O errors.
> The timeout has been increased to 200ms after data transfer.
> For other transfers it stays unchanged. Default values are
> now defined with "if defined" directive so it can be redefined
> at board config if needed.
>
> Tested on Goni and Trats.
>
> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>

Please do not apply this patch yet due to still not enough results on 
some targets.
Timeout value should depends on internal cards operations execution time 
but this time is unpredictably and that is why JEDEC not specifies it. 
Maybe u-boot sdhci driver needs some more changes to be more flexible 
for such operations. In example sdhci background operations timeout at 
kernel is specified to 4 minutes.
I need to make more research.

Regards,

-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH] mmc:sdhci: Fix card ready status timeout.

[Pantelis Antoniou]

Hi there,

On Sep 13, 2013, at 3:59 PM, Przemyslaw Marczak wrote:

> Dear Pantelis,
> 
> On 09/09/2013 02:58 PM, Przemyslaw Marczak wrote:
>> According to JEDEC eMMC specification, after data transfer
>> (multiple or single block) host must wait for card ready
>> status. This is done by waiting for command and data lines
>> to be at idle state after transfer. JEDEC does not specify
>> maximum timeout.
>> 
>> Before this change max timeout was 10 ms but in case of UMS
>> - when system does multiple read/write operations on random
>> card blocks - timeout causes I/O errors.
>> The timeout has been increased to 200ms after data transfer.
>> For other transfers it stays unchanged. Default values are
>> now defined with "if defined" directive so it can be redefined
>> at board config if needed.
>> 
>> Tested on Goni and Trats.
>> 
>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> 
> Please do not apply this patch yet due to still not enough results on some targets.
> Timeout value should depends on internal cards operations execution time but this time is unpredictably and that is why JEDEC not specifies it. Maybe u-boot sdhci driver needs some more changes to be more flexible for such operations. In example sdhci background operations timeout at kernel is specified to 4 minutes.
> I need to make more research.
> 

OK, this need to be fleshed out a bit more.

Please keep me in the loop cause this sounds board-specific.
Perhaps the CONFIG_* option is a sound idea; real world is messy like that.

> Regards,
> 
> -- 
> Przemyslaw Marczak
> Samsung R&D Institute Poland
> Samsung Electronics
> p.marczak at samsung.com

Regards

-- Pantelis

[U-Boot] [PATCH] mmc:sdhci: Fix card ready status timeout.

[Przemyslaw Marczak]

According to JEDEC eMMC specification, after data transfer
(multiple or single block) host must wait for card ready
status. This is done by waiting for command and data lines
to be at idle state after transfer. JEDEC does not specify
maximum timeout.

Before this change max timeout was 10 ms but in case of UMS
- when system do multiple read/write operations on random
card blocks - timeout causes I/O errors.
The timeout has been increased to 200ms after data transfer.
For other transfers it stays unchanged.

Tested on Goni and Trats.

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/mmc/sdhci.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
index 4261991..22c18d1 100644
--- a/drivers/mmc/sdhci.c
+++ b/drivers/mmc/sdhci.c
@@ -121,8 +121,10 @@ 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;
+	if (!data)
+		timeout = 200;
+	else
+		timeout = 10;
 
 	sdhci_writel(host, SDHCI_INT_ALL_MASK, SDHCI_INT_STATUS);
 	mask = SDHCI_CMD_INHIBIT | SDHCI_DATA_INHIBIT;
-- 
1.7.9.5

[U-Boot] [PATCH] mmc:sdhci: Fix card ready status timeout.

[Jaehoon Chung]

Hi Przemyslaw,

Could you give me the test-case?
I want to test this problem.

On 08/29/2013 01:49 AM, Przemyslaw Marczak wrote:
> According to JEDEC eMMC specification, after data transfer
> (multiple or single block) host must wait for card ready
> status. This is done by waiting for command and data lines
> to be at idle state after transfer. JEDEC does not specify
> maximum timeout.
> 
> Before this change max timeout was 10 ms but in case of UMS
> - when system do multiple read/write operations on random
> card blocks - timeout causes I/O errors.
> The timeout has been increased to 200ms after data transfer.
> For other transfers it stays unchanged.
> 
> Tested on Goni and Trats.
> 
> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> ---
>  drivers/mmc/sdhci.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
> index 4261991..22c18d1 100644
> --- a/drivers/mmc/sdhci.c
> +++ b/drivers/mmc/sdhci.c
> @@ -121,8 +121,10 @@ 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;
> +	if (!data)
> +		timeout = 200;
> +	else
> +		timeout = 10;
And timeout = data ? 10 : 200; ?

Best Regards,
Jaehoon Chung
>  
>  	sdhci_writel(host, SDHCI_INT_ALL_MASK, SDHCI_INT_STATUS);
>  	mask = SDHCI_CMD_INHIBIT | SDHCI_DATA_INHIBIT;
> 

[U-Boot] [PATCH] mmc:sdhci: Fix card ready status timeout.

[Przemyslaw Marczak]

On 08/29/2013 05:16 AM, Jaehoon Chung wrote:
> Hi Przemyslaw,
>
> Could you give me the test-case?
> I want to test this problem.
>
> On 08/29/2013 01:49 AM, Przemyslaw Marczak wrote:
>> According to JEDEC eMMC specification, after data transfer
>> (multiple or single block) host must wait for card ready
>> status. This is done by waiting for command and data lines
>> to be at idle state after transfer. JEDEC does not specify
>> maximum timeout.
>>
>> Before this change max timeout was 10 ms but in case of UMS
>> - when system do multiple read/write operations on random
>> card blocks - timeout causes I/O errors.
>> The timeout has been increased to 200ms after data transfer.
>> For other transfers it stays unchanged.
>>
>> Tested on Goni and Trats.
>>
>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>


Hello,

You can ease reproduce this problem by typing "ums" on prompt(Trats).
If you are using Goni (without ums config), the problem is hard to 
reproduce in this case. Sometimes read one block from mmc just after 
start causes error and then you can see information "Controller never
released inhibit bit(s)".
Patch will be completed with some code comment.

Thank you

-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com