Skip to contentSkip to conversation

Timeout for SDHCI commands

PATCH New Eli Billauer 4 participants 7 messages Jun 12, 2014 – Jul 4, 2014 MMC, SD and SDIO

lore
Share & tools
lore

Eli Billauer

author
#0 lore raw

PATCH 1/1 Timeout for SDHCI commands

The patch in the following mail is a result of a problem I had using an eMMC device on a Xilinx Zynq ARM processor. It turned out that the waiting for a certain response from the SDIO interface was made with a plain loop, with no absolute time measurement. Since I'm using a relatively fast processor, the timeout expired before the eMMC chip managed to acknowledge an mmc_switch() with EXT_CSD_HS_TIMING.

This patch fixes the problem on my board, and eMMC works properly with it. However I can't say that I really understand what I did, and the 100 ms timeout was chosen with a finger in the wind. If there's a reason why the timeout should be longer or shorter, this is a good time to come forward.

Thanks,
Eli

Eli Billauer (1):
mmc: sdhci: Fixed timeout for sdhci_send_command()

 drivers/mmc/sdhci.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Signature · 1 line
1.7.2.3
author
#1 lore raw

PATCH 1/1 mmc: sdhci: Fixed timeout for sdhci_send_command()

The current wait loop just reads the status 10000 times, which makes the actual timeout period platform-dependent. The udelay() call within the loop makes the new timeout ~100 ms.

Signed-off-by Eli Billauer <[email protected]>
---
 drivers/mmc/sdhci.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/mmc/sdhci.c +1 -0
index 3125d13..80f3a91 100644
@@ -226,6 +226,7 @@ int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
 			break;
 		if (--retry == 0)
 			break;
+		udelay(10);
 	} while ((stat & mask) != mask);
 
 	if (retry == 0) {
Signature · 1 line
1.7.2.3
#2 lore raw

PATCH 1/1 mmc: sdhci: Fixed timeout for sdhci_send_command()

Quoted 14 lines · Eli Billauer

The current wait loop just reads the status 10000 times, which makes the actual timeout period platform-dependent. The udelay() call within the loop makes the new timeout ~100 ms.

Signed-off-by Eli Billauer <[email protected]>
---
 drivers/mmc/sdhci.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/mmc/sdhci.c +0 -0
index 3125d13..80f3a91 100644

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

Quoted 5 lines
drivers/mmc/sdhci.c +1 -0
@@ -226,6 +226,7 @@ int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
                        break;
                if (--retry == 0)
                        break;
+               udelay(10);
        } while ((stat & mask) != mask);

Hmmm...

Is 100ms part of the spec? I like the idea of making the timeout more time-based, but it seems to me that this changes the timeout quite significantly. If it took N ms before, it now takes N + 100 ms.

I think, if we want the timeout to be ~100ms, we should use a udelay of 100 or 1000, and then reduce "retry" accordingly.

Andy

author
#3 lore raw

PATCH 1/1 mmc: sdhci: Fixed timeout for sdhci_send_command()

Quoted 9 lines · Andy Fleming
Quoted 6 lines · Eli Billauer

The current wait loop just reads the status 10000 times, which makes the actual timeout period platform-dependent. The udelay() call within the loop makes the new timeout ~100 ms.

[ snipped patch ]

Quoted 9 lines

Hmmm...

Is 100ms part of the spec? I like the idea of making the timeout more time-based, but it seems to me that this changes the timeout quite significantly. If it took N ms before, it now takes N + 100 ms.

I think, if we want the timeout to be ~100ms, we should use a udelay of 100 or 1000, and then reduce "retry" accordingly.

Hi,

As I said in the mail preceding this patch, I don't know what the timeout should be. Maybe someone with a better knowledge on MMC could come forward.

Regards,
Eli

Quoted 3 lines

Andy

#4 lore raw

PATCH 1/1 mmc: sdhci: Fixed timeout for sdhci_send_command()

In reply to Eli Billauer

Hi Eli,

Quoted 23 lines · Eli Billauer

The current wait loop just reads the status 10000 times, which makes the actual timeout period platform-dependent. The udelay() call within the loop makes the new timeout ~100 ms.

Signed-off-by Eli Billauer <[email protected]>

---
drivers/mmc/sdhci.c | 1 +

1 files changed, 1 insertions(+), 0 deletions(-)
drivers/mmc/sdhci.c +1 -0
index 3125d13..80f3a91 100644
@@ -226,6 +226,7 @@ int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
 			break;
 		if (--retry == 0)
 			break;
+		udelay(10);
 	} while ((stat & mask) != mask);
 
 	if (retry == 0) {
Signature · 1 line
1.7.2.3

Looking at the linux sources is no good, cause linux is interrupt driven. This delay is used because the driver is not interrupt driven, so you have to wait until the interrupt indication is delivered.

The only reference to interrupt latency I found is related to tuning and is set to 50ms which I supposed is very pessimistic.
I think a timeout of 100ms would be fine.

Regards

-- Pantelis

#5 lore raw

PATCH 1/1 mmc: sdhci: Fixed timeout for sdhci_send_command()

Quoted 30 lines · Pantelis Antoniou

Hi Eli,

Quoted 2 lines · Eli Billauer

The current wait loop just reads the status 10000 times, which makes the actual timeout period platform-dependent. The udelay() call within the

loop

drivers/mmc/sdhci.c int sdhci_send_command(struct mmc *mmc, struct

Quoted 12 lines

makes the new timeout ~100 ms.

Signed-off-by Eli Billauer <[email protected]>

---
drivers/mmc/sdhci.c | 1 +

1 files changed, 1 insertions(+), 0 deletions(-)
drivers/mmc/sdhci.c +0 -0
index 3125d13..80f3a91 100644
@@ -226,6 +226,7 @@ int sdhci_send_command(struct mmc *mmc, struct

mmc_cmd *cmd,

Quoted 9 lines
drivers/mmc/sdhci.c +1 -1
                      break;
              if (--retry == 0)
                      break;
+             udelay(10);
      } while ((stat & mask) != mask);
 
      if (retry == 0) {
--

1.7.2.3

Quoted 9 lines

Looking at the linux sources is no good, cause linux is interrupt driven. This delay is used because the driver is not interrupt driven, so you have to wait until the interrupt indication is delivered.

The only reference to interrupt latency I found is related to tuning and is set to 50ms which I supposed is very pessimistic.
I think a timeout of 100ms would be fine.

I suspect the timeout of 100ms is fine (though it's always nice when we tie such numbers to something more concrete than: "it works if I make it wait longer"). My main point was that this actually *adds* 100ms to the preexisting timeout, instead of making the timeout ~100ms. If we reduced the number of checks and increased the delay, the delay would completely dominate the timeout loop, and total time would become closer to ~100ms.

Andy

#6 lore raw

PATCH 1/1 mmc: sdhci: Fixed timeout for sdhci_send_command()

In reply to Pantelis Antoniou
Tested-by Steve Rae <[email protected]>

(does resolve the issue on our board!)

Quoted 45 lines · Pantelis Antoniou

Hi Eli,

Quoted 22 lines · Eli Billauer

The current wait loop just reads the status 10000 times, which makes the actual timeout period platform-dependent. The udelay() call within the loop makes the new timeout ~100 ms.

Signed-off-by Eli Billauer <[email protected]>

---
drivers/mmc/sdhci.c | 1 +

1 files changed, 1 insertions(+), 0 deletions(-)
drivers/mmc/sdhci.c +1 -1
index 3125d13..80f3a91 100644
@@ -226,6 +226,7 @@ int sdhci_send_command(struct mmc *mmc, struct mmc_cmd *cmd,
 			break;
 		if (--retry == 0)
 			break;
+		udelay(10);
 	} while ((stat & mask) != mask);
 
 	if (retry == 0) {
--
Quoted 1 line

1.7.2.3

Looking at the linux sources is no good, cause linux is interrupt driven. This delay is used because the driver is not interrupt driven, so you have to wait until the interrupt indication is delivered.

The only reference to interrupt latency I found is related to tuning and is set to 50ms which I supposed is very pessimistic.
I think a timeout of 100ms would be fine.

Regards

-- Pantelis

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