Skip to contentSkip to conversation

arm: mmc: increase MMC SDHCI read status timeout

PATCH New Steve Rae 4 participants 9 messages Jun 27, 2016 – Jun 29, 2016 MMC, SD and SDIO

lore
Share & tools
lore

Steve Rae

author
#0 lore raw

PATCH 1/1 arm: mmc: increase MMC SDHCI read status timeout

Otherwise, ocassionally see errors like this:
Flashing sparse image at offset 2078720 Flashing Sparse Image
sdhci_send_command: Timeout for status update! mmc fail to send stop cmd
write_sparse_image: Write failed, block #2181088 [0]

This does not affect the actual writing speed, which is controlled by the default value:
CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT

It only increases the retries when reading:
SDHCI_INT_STATUS
to avoid the timeout error.

Signed-off-by Steve Rae <[email protected]>

---
as per the discussion in:
http://lists.denx.de/pipermail/u-boot/2016-June/258966.html
this supercedes:
http://patchwork.ozlabs.org/patch/615994/

 drivers/mmc/sdhci.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/mmc/sdhci.c +3 -2
index 5c71ab8..aa4cd4f 100644
@@ -127,6 +127,7 @@ static int sdhci_transfer_data(struct sdhci_host *host, struct mmc_data *data,
 #define CONFIG_SDHCI_CMD_MAX_TIMEOUT		3200
 #endif
 #define CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT	100
+#define CONFIG_SDHCI_READ_STATUS_TIMEOUT	1000
 
 static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
 		       struct mmc_data *data)

@@ -243,9 +244,9 @@ static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
 		if (stat & SDHCI_INT_ERROR)
 			break;
 	} while (((stat & mask) != mask) &&
-		 (get_timer(start) < CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT));
+		 (get_timer(start) < CONFIG_SDHCI_READ_STATUS_TIMEOUT));
 
-	if (get_timer(start) >= CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT) {
+	if (get_timer(start) >= CONFIG_SDHCI_READ_STATUS_TIMEOUT) {
 		if (host->quirks & SDHCI_QUIRK_BROKEN_R1B)
 			return 0;
 		else {
Signature · 1 line
1.8.5
#1 lore raw

PATCH 1/1 arm: mmc: increase MMC SDHCI read status timeout

Hi Steve,

Quoted 15 lines · Steve Rae

Otherwise, ocassionally see errors like this:
Flashing sparse image at offset 2078720 Flashing Sparse Image
sdhci_send_command: Timeout for status update! mmc fail to send stop cmd
write_sparse_image: Write failed, block #2181088 [0]

This does not affect the actual writing speed, which is controlled by the default value:
CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT

It only increases the retries when reading:
SDHCI_INT_STATUS
to avoid the timeout error.

Quoted 6 lines
Signed-off-by Steve Rae <[email protected]>

---
as per the discussion in:
http://lists.denx.de/pipermail/u-boot/2016-June/258966.html
this supercedes:
http://patchwork.ozlabs.org/patch/615994/

IIRC, I've suggested to move SDHCI_CMD_DEFAULT_TIMEOUT to Kconfig and use the old value as default value. So that you can overwrite it for your board / platform via your defconfig. But I have no strong feelings here - your current version also works for me and does not "clutter" the Kconfig subsystem with too many values. So:

Reviewed-by Stefan Roese <[email protected]>

Thanks,
Stefan

Quoted 18 lines
  drivers/mmc/sdhci.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
drivers/mmc/sdhci.c +1 -0
index 5c71ab8..aa4cd4f 100644
@@ -127,6 +127,7 @@ static int sdhci_transfer_data(struct sdhci_host *host, struct mmc_data *data,
  #define CONFIG_SDHCI_CMD_MAX_TIMEOUT		3200
  #endif
  #define CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT	100
+#define CONFIG_SDHCI_READ_STATUS_TIMEOUT	1000
 
  static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
  		       struct mmc_data *data)

@@ -243,9 +244,9 @@ static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
  		if (stat & SDHCI_INT_ERROR)

drivers/mmc/sdhci.c static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd, · line 252

Quoted 10 lines
drivers/mmc/sdhci.c +2 -2
  			break;
  	} while (((stat & mask) != mask) &&
-		 (get_timer(start) < CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT));
+		 (get_timer(start) < CONFIG_SDHCI_READ_STATUS_TIMEOUT));
 
-	if (get_timer(start) >= CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT) {
+	if (get_timer(start) >= CONFIG_SDHCI_READ_STATUS_TIMEOUT) {
  		if (host->quirks & SDHCI_QUIRK_BROKEN_R1B)
  			return 0;
  		else {

Viele Gr??e,
Stefan

Signature · 3 lines
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de
author
#2 lore raw

PATCH 1/1 arm: mmc: increase MMC SDHCI read status timeout

Hi Stefan,

Quoted 34 lines · Stefan Roese

Hi Steve,

Quoted 16 lines · Steve Rae

Otherwise, ocassionally see errors like this:
Flashing sparse image at offset 2078720 Flashing Sparse Image
sdhci_send_command: Timeout for status update! mmc fail to send stop cmd
write_sparse_image: Write failed, block #2181088 [0]

This does not affect the actual writing speed, which is controlled by the default value:
CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT

It only increases the retries when reading:
SDHCI_INT_STATUS
to avoid the timeout error.

Quoted 6 lines
Signed-off-by Steve Rae <[email protected]>

---
as per the discussion in:
http://lists.denx.de/pipermail/u-boot/2016-June/258966.html
this supercedes:
http://patchwork.ozlabs.org/patch/615994/

IIRC, I've suggested to move SDHCI_CMD_DEFAULT_TIMEOUT to Kconfig and use the old value as default value. So that you can overwrite it for your board / platform via your defconfig. But I have no strong feelings here - your current version also works for me and does not "clutter" the Kconfig subsystem with too many values. So:

Quoted 5 lines
Reviewed-by Stefan Roese <[email protected]>

Thanks,
Stefan

Thanks for the review...
I didn't want to touch the "performance" algorithm related to SDHCI_CMD_DEFAULT_TIMEOUT (which maybe should be in Kconfig). However, the retry loop related to SDHCI_READ_STATUS_TIMEOUT doesn't need to be in Kconfig -- it is just a define.
Thanks, Steve

Quoted 38 lines
Quoted 28 lines
  drivers/mmc/sdhci.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
drivers/mmc/sdhci.c +1 -0
index 5c71ab8..aa4cd4f 100644
@@ -127,6 +127,7 @@ static int sdhci_transfer_data(struct sdhci_host
 *host, struct mmc_data *data,
  #define CONFIG_SDHCI_CMD_MAX_TIMEOUT          3200
  #endif
  #define CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT      100
+#define CONFIG_SDHCI_READ_STATUS_TIMEOUT       1000
 
  static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,

struct mmc_data *data)

drivers/mmc/sdhci.c +2 -2
@@ -243,9 +244,9 @@ static int sdhci_send_command(struct mmc *mmc, struct
 mmc_cmd *cmd,
                if (stat & SDHCI_INT_ERROR)
                        break;
        } while (((stat & mask) != mask) &&
-                (get_timer(start) < CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT));
+                (get_timer(start) < CONFIG_SDHCI_READ_STATUS_TIMEOUT));
 
-       if (get_timer(start) >= CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT) {
+       if (get_timer(start) >= CONFIG_SDHCI_READ_STATUS_TIMEOUT) {
                if (host->quirks & SDHCI_QUIRK_BROKEN_R1B)
                        return 0;
Quoted 2 lines
                else {

Viele Gr??e,
Stefan

--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de

#3 lore raw

PATCH 1/1 arm: mmc: increase MMC SDHCI read status timeout

Quoted 43 lines · Steve Rae

Hi Stefan,

Quoted 34 lines · Stefan Roese

Hi Steve,

Quoted 16 lines · Steve Rae

Otherwise, ocassionally see errors like this:
Flashing sparse image at offset 2078720 Flashing Sparse Image
sdhci_send_command: Timeout for status update! mmc fail to send stop cmd
write_sparse_image: Write failed, block #2181088 [0]

This does not affect the actual writing speed, which is controlled by the default value:
CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT

It only increases the retries when reading:
SDHCI_INT_STATUS
to avoid the timeout error.

Quoted 6 lines
Signed-off-by Steve Rae <[email protected]>

---
as per the discussion in:
http://lists.denx.de/pipermail/u-boot/2016-June/258966.html
this supercedes:
http://patchwork.ozlabs.org/patch/615994/

IIRC, I've suggested to move SDHCI_CMD_DEFAULT_TIMEOUT to Kconfig and use the old value as default value. So that you can overwrite it for your board / platform via your defconfig. But I have no strong feelings here - your current version also works for me and does not "clutter" the Kconfig subsystem with too many values. So:

Quoted 5 lines
Reviewed-by Stefan Roese <[email protected]>

Thanks,
Stefan

Quoted 5 lines

Thanks for the review...
I didn't want to touch the "performance" algorithm related to SDHCI_CMD_DEFAULT_TIMEOUT (which maybe should be in Kconfig). However, the retry loop related to SDHCI_READ_STATUS_TIMEOUT doesn't need to be in Kconfig -- it is just a define.

... so how is this handled in the kernel? I'm assuming some DT property..

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160628/924186b4/attachment.sig>
author
#4 lore raw

PATCH 1/1 arm: mmc: increase MMC SDHCI read status timeout

Hi Tom,

Quoted 50 lines · Tom Rini
Quoted 43 lines · Steve Rae

Hi Stefan,

Quoted 34 lines · Stefan Roese

Hi Steve,

Quoted 16 lines · Steve Rae

Otherwise, ocassionally see errors like this:
Flashing sparse image at offset 2078720 Flashing Sparse Image
sdhci_send_command: Timeout for status update! mmc fail to send stop cmd
write_sparse_image: Write failed, block #2181088 [0]

This does not affect the actual writing speed, which is controlled by the default value:
CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT

It only increases the retries when reading:
SDHCI_INT_STATUS
to avoid the timeout error.

Quoted 6 lines
Signed-off-by Steve Rae <[email protected]>

---
as per the discussion in:
http://lists.denx.de/pipermail/u-boot/2016-June/258966.html
this supercedes:
http://patchwork.ozlabs.org/patch/615994/

IIRC, I've suggested to move SDHCI_CMD_DEFAULT_TIMEOUT to Kconfig and use the old value as default value. So that you can overwrite it for your board / platform via your defconfig. But I have no strong feelings here - your current version also works for me and does not "clutter" the Kconfig subsystem with too many values. So:

Quoted 5 lines
Reviewed-by Stefan Roese <[email protected]>

Thanks,
Stefan

Quoted 5 lines

Thanks for the review...
I didn't want to touch the "performance" algorithm related to SDHCI_CMD_DEFAULT_TIMEOUT (which maybe should be in Kconfig). However, the retry loop related to SDHCI_READ_STATUS_TIMEOUT doesn't need to be in Kconfig -- it is just a define.

Quoted 5 lines

... so how is this handled in the kernel? I'm assuming some DT property..

--
Tom

( we discussed this in the other email thread; but I'll copy it here... )

It looks like (v.4.6) the code loops for max_loops=16,
and it looks like the loop delay is created by a write (which does not exist in the U-Boot code):
sdhci_writel(host, mask, SDHCI_INT_STATUS);

Thanks, Steve

#5 lore raw

PATCH 1/1 arm: mmc: increase MMC SDHCI read status timeout

Quoted 53 lines · Steve Rae

Hi Tom,

Quoted 50 lines · Tom Rini
Quoted 48 lines · Steve Rae

Hi Stefan,

Quoted 34 lines · Stefan Roese

Hi Steve,

Quoted 16 lines · Steve Rae

Otherwise, ocassionally see errors like this:
Flashing sparse image at offset 2078720 Flashing Sparse Image
sdhci_send_command: Timeout for status update! mmc fail to send stop cmd
write_sparse_image: Write failed, block #2181088 [0]

This does not affect the actual writing speed, which is controlled by the default value:
CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT

It only increases the retries when reading:
SDHCI_INT_STATUS
to avoid the timeout error.

Quoted 6 lines
Signed-off-by Steve Rae <[email protected]>

---
as per the discussion in:
http://lists.denx.de/pipermail/u-boot/2016-June/258966.html
this supercedes:
http://patchwork.ozlabs.org/patch/615994/

IIRC, I've suggested to move SDHCI_CMD_DEFAULT_TIMEOUT to Kconfig and use the old value as default value. So that you can overwrite it for your board / platform via your defconfig. But I have no strong feelings here - your current version also works for me and does not "clutter" the Kconfig subsystem with too many values. So:

Quoted 5 lines
Reviewed-by Stefan Roese <[email protected]>

Thanks,
Stefan

Thanks for the review...
I didn't want to touch the "performance" algorithm related to SDHCI_CMD_DEFAULT_TIMEOUT (which maybe should be in Kconfig). However, the retry loop related to SDHCI_READ_STATUS_TIMEOUT doesn't need to be in Kconfig -- it is just a define.

Quoted 7 lines
Quoted 5 lines

... so how is this handled in the kernel? I'm assuming some DT property..

--
Tom

( we discussed this in the other email thread; but I'll copy it here... )

I thought I saw this in another thread, thanks.

Quoted 4 lines

It looks like (v.4.6) the code loops for max_loops=16,
and it looks like the loop delay is created by a write (which does not exist in the U-Boot code):
sdhci_writel(host, mask, SDHCI_INT_STATUS);

Maybe we should rewrite the area in question, for the next release to be like the kernel?

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160628/9387bdb2/attachment.sig>
author
#6 lore raw

PATCH 1/1 arm: mmc: increase MMC SDHCI read status timeout

Quoted 64 lines · Tom Rini
Quoted 53 lines · Steve Rae

Hi Tom,

Quoted 50 lines · Tom Rini
Quoted 48 lines · Steve Rae

Hi Stefan,

Quoted 34 lines · Stefan Roese

Hi Steve,

Quoted 16 lines · Steve Rae

Otherwise, ocassionally see errors like this:
Flashing sparse image at offset 2078720 Flashing Sparse Image
sdhci_send_command: Timeout for status update! mmc fail to send stop cmd
write_sparse_image: Write failed, block #2181088 [0]

This does not affect the actual writing speed, which is controlled by the default value:
CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT

It only increases the retries when reading:
SDHCI_INT_STATUS
to avoid the timeout error.

Quoted 6 lines
Signed-off-by Steve Rae <[email protected]>

---
as per the discussion in:
http://lists.denx.de/pipermail/u-boot/2016-June/258966.html
this supercedes:
http://patchwork.ozlabs.org/patch/615994/

IIRC, I've suggested to move SDHCI_CMD_DEFAULT_TIMEOUT to Kconfig and use the old value as default value. So that you can overwrite it for your board / platform via your defconfig. But I have no strong feelings here - your current version also works for me and does not "clutter" the Kconfig subsystem with too many values. So:

Quoted 5 lines
Reviewed-by Stefan Roese <[email protected]>

Thanks,
Stefan

Thanks for the review...
I didn't want to touch the "performance" algorithm related to SDHCI_CMD_DEFAULT_TIMEOUT (which maybe should be in Kconfig). However, the retry loop related to SDHCI_READ_STATUS_TIMEOUT doesn't need to be in Kconfig -- it is just a define.

Quoted 7 lines
Quoted 5 lines

... so how is this handled in the kernel? I'm assuming some DT property..

--
Tom

( we discussed this in the other email thread; but I'll copy it here... )

I thought I saw this in another thread, thanks.

Quoted 8 lines
Quoted 4 lines

It looks like (v.4.6) the code loops for max_loops=16,
and it looks like the loop delay is created by a write (which does not exist in the U-Boot code):
sdhci_writel(host, mask, SDHCI_INT_STATUS);

Maybe we should rewrite the area in question, for the next release to be like the kernel?

Maybe - I don't know enough about it -- it looks like there are interrupts involved....
( Who could this be assigned to? )
I was just trying to get it to run without hitting the time out.... Thanks, Steve

Quoted 8 lines

--
Tom

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

#7 lore raw

PATCH 1/1 arm: mmc: increase MMC SDHCI read status timeout

Hi Steve,

Review static int sdhci_transfer_data(struct sdhci_host *host, struct mmc_data *data, · line 133

Quoted 8 lines
(quoted diff) +1 -0
@@ -127,6 +127,7 @@ static int sdhci_transfer_data(struct sdhci_host *host, struct mmc_data *data,
 #define CONFIG_SDHCI_CMD_MAX_TIMEOUT           3200
 #endif
 #define CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT       100
+#define CONFIG_SDHCI_READ_STATUS_TIMEOUT       1000
 
 static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
                       struct mmc_data *data)

When I see CONFIG_ prefix, I imagine this parameter can be configurable via Kconfig or board header,
but this is not actually overridden.

Nor do I believe it should be moved to Kconfig,
because I doubt it is a CONFIG.

Setting such things aside, one important thing is, this patch fixes my problem.

BTW, why did you add "arm: " to the git subject?
Is this patch related to ARM?

Otherwise,

Tested-by Masahiro Yamada <[email protected]>
Signature · 2 lines
Best Regards
Masahiro Yamada
author
#8 lore raw

PATCH 1/1 arm: mmc: increase MMC SDHCI read status timeout

Hi Masahiro,

Quoted 9 lines · Masahiro Yamada

Hi Steve,

Quoted 6 lines
(quoted diff) +1 -0
@@ -127,6 +127,7 @@ static int sdhci_transfer_data(struct sdhci_host *host, struct mmc_data *data,
 #define CONFIG_SDHCI_CMD_MAX_TIMEOUT           3200
 #endif
 #define CONFIG_SDHCI_CMD_DEFAULT_TIMEOUT       100
+#define CONFIG_SDHCI_READ_STATUS_TIMEOUT       1000
Quoted 8 lines

Review static int sdhci_transfer_data(struct sdhci_host *host, struct mmc_data *data, · line 133

Quoted 2 lines
(quoted diff) +0 -0
 static int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
                       struct mmc_data *data)

When I see CONFIG_ prefix, I imagine this parameter can be configurable via Kconfig or board header,
but this is not actually overridden.

that makes sense....

Quoted 3 lines

Nor do I believe it should be moved to Kconfig,
because I doubt it is a CONFIG.

correct!
so maybe the define should be ? :
SDHCI_READ_STATUS_TIMEOUT

Quoted 9 lines

Setting such things aside, one important thing is, this patch fixes my problem.

BTW, why did you add "arm: " to the git subject?
Is this patch related to ARM?

no (sorry) , I don't think so....

Quoted 10 lines

Otherwise,

Tested-by Masahiro Yamada <[email protected]>

--
Best Regards
Masahiro Yamada