Skip to contentSkip to conversation

mmc: set timeout for SDHCI host before sending busy cmd

PATCH In review Chuanxiao Dong 5 participants 8 messages Feb 24, 2011 – Feb 25, 2011

Subsystems (2)
MMC, SD and SDIO Secure digital host controller interface (SDHCI) driver
lore
Share & tools
lore

Chuanxiao Dong

author
#0 lore raw

PATCH 1/1 mmc: set timeout for SDHCI host before sending busy cmd

Set the timeout control register for SDHCI host when it needs to send some commands which need busy signal. Use the maximum timeout value will be safe.

Signed-off-by Chuanxiao Dong <[email protected]>
---
 drivers/mmc/host/sdhci.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)
drivers/mmc/host/sdhci.c +10 -4
index 9e15f41..32b7475 100644
@@ -40,7 +40,6 @@
 
 static unsigned int debug_quirks = 0;
 
-static void sdhci_prepare_data(struct sdhci_host *, struct mmc_data *);
 static void sdhci_finish_data(struct sdhci_host *);
 
 static void sdhci_send_command(struct sdhci_host *, struct mmc_command *);

@@ -651,16 +650,23 @@ static void sdhci_set_transfer_irqs(struct sdhci_host *host)
 		sdhci_clear_set_irqs(host, dma_irqs, pio_irqs);
 }
 
-static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
+static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 {
 	u8 count;
 	u8 ctrl;
+	struct mmc_data *data = cmd->data;
 	int ret;
 
 	WARN_ON(host->data);
 
-	if (data == NULL)
+	if (data == NULL) {
+		/*
+		 * set timeout to be maximum value for command with busy signal.
+		 */
+		if (cmd->flags & MMC_RSP_BUSY)
+			sdhci_writeb(host, 0xE, SDHCI_TIMEOUT_CONTROL);
 		return;
+	}
 
 	/* Sanity checks */
 	BUG_ON(data->blksz * data->blocks > 524288);

@@ -920,7 +926,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 
 	host->cmd = cmd;
 
-	sdhci_prepare_data(host, cmd->data);
+	sdhci_prepare_data(host, cmd);
 
 	sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT);
Signature · 1 line
1.6.6.1
#1 lore raw

Hi

This patch looks like this. I sent to the RFC patch..

http://marc.info/?l=linux-mmc&m=129109794815028&w=3

Signed-off-by Jaehoon Chung <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>
---
 drivers/mmc/host/sdhci.c  |    6 +++++-
 include/linux/mmc/sdhci.h |    3 ++-
 2 files changed, 7 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci.c +0 -0
index 782c0ee..3b93d97 100644
@@ -655,8 +655,12 @@ static void sdhci_prepare_data(struct sdhci_host

*host, struct mmc_data *data)

       WARN_ON(host->data);

- if (data == NULL)
+ if (data == NULL) {
+ if ((host->quirks & SDHCI_QUIRK_SET_DATA_TIMEOUT_VAL) &&
+ (host->cmd->flags & MMC_RSP_BUSY))

+                       sdhci_writel(host, 0xE, SDHCI_TIMEOUT_CONTROL);
               return;

+ }

       /* Sanity checks */
       BUG_ON(data->blksz * data->blocks > 524288);
include/linux/mmc/sdhci.h +2 -1
index 1fdc673..315ff49 100644
@@ -83,7 +83,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12             (1<<28)
 /* Controller doesn't have HISPD bit field in HI-SPEED SD card */
 #define SDHCI_QUIRK_NO_HISPD_BIT                       (1<<29)
-
+/* Controller need set data timeout value when card is busy */
+#define SDHCI_QUIRK_SET_DATA_TIMEOUT_VAL               (1<<30)
       int irq;                /* Device IRQ */
       void __iomem *ioaddr;   /* Mapped address */

--
1.6.0.4

2011/2/24 Chuanxiao Dong <[email protected]>:

Quoted 63 lines

Set the timeout control register for SDHCI host when it needs to send some commands which need busy signal. Use the maximum timeout value will be safe.

Signed-off-by Chuanxiao Dong <[email protected]>

---
 drivers/mmc/host/sdhci.c |   14 ++++++++++----

 1 files changed, 10 insertions(+), 4 deletions(-)
drivers/mmc/host/sdhci.c +10 -5
index 9e15f41..32b7475 100644
@@ -40,7 +40,6 @@
 
  static unsigned int debug_quirks = 0;
 
-static void sdhci_prepare_data(struct sdhci_host *, struct mmc_data *);
  static void sdhci_finish_data(struct sdhci_host *);
 
  static void sdhci_send_command(struct sdhci_host *, struct mmc_command *);

@@ -651,16 +650,23 @@ static void sdhci_set_transfer_irqs(struct sdhci_host *host)
                sdhci_clear_set_irqs(host, dma_irqs, pio_irqs);
  }
 
-static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
+static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
  {
        u8 count;
        u8 ctrl;
+       struct mmc_data *data = cmd->data;
        int ret;
 
        WARN_ON(host->data);
 
-       if (data == NULL)
+       if (data == NULL) {
+               /*
+                * set timeout to be maximum value for command with busy signal.
+                */
+               if (cmd->flags & MMC_RSP_BUSY)
+                       sdhci_writeb(host, 0xE, SDHCI_TIMEOUT_CONTROL);
                return;
+       }
 
        /* Sanity checks */
        BUG_ON(data->blksz * data->blocks > 524288);

@@ -920,7 +926,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 
        host->cmd = cmd;
 
-       sdhci_prepare_data(host, cmd->data);
+       sdhci_prepare_data(host, cmd);
 
        sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT);
 
--

1.6.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

author
#2 lore raw

Great. Maybe we wouldn't need to add this quirk since give a maximum timeout value is safe for all host controller I think. We would better save the quirk for other really critical silicon bugs.

Quoted 129 lines

-----Original Message-----
From: Jae hoon Chung [mailto:[email protected]]
Sent: Thursday, February 24, 2011 10:18 PM
To: Dong, Chuanxiao
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Kyungmin Park
Subject: Re: [PATCH 1/1]mmc: set timeout for SDHCI host before sending busy cmd

Hi

This patch looks like this. I sent to the RFC patch..

http://marc.info/?l=linux-mmc&m=129109794815028&w=3

Signed-off-by Jaehoon Chung <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>
---
 drivers/mmc/host/sdhci.c  |    6 +++++-
 include/linux/mmc/sdhci.h |    3 ++-
 2 files changed, 7 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci.c +2 -1
index 782c0ee..3b93d97 100644
@@ -655,8 +655,12 @@ static void sdhci_prepare_data(struct sdhci_host
 *host, struct mmc_data *data)
 
       WARN_ON(host->data);
 
-       if (data == NULL)
+       if (data == NULL) {
+               if ((host->quirks & SDHCI_QUIRK_SET_DATA_TIMEOUT_VAL)

&&

drivers/mmc/host/sdhci.c +3 -0
+                               (host->cmd->flags & MMC_RSP_BUSY))
+                       sdhci_writel(host, 0xE, SDHCI_TIMEOUT_CONTROL);
               return;
+       }
 
       /* Sanity checks */
       BUG_ON(data->blksz * data->blocks > 524288);
include/linux/mmc/sdhci.h +2 -2
index 1fdc673..315ff49 100644
@@ -83,7 +83,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12             (1<<28)
 /* Controller doesn't have HISPD bit field in HI-SPEED SD card */
 #define SDHCI_QUIRK_NO_HISPD_BIT                       (1<<29)
-
+/* Controller need set data timeout value when card is busy */
+#define SDHCI_QUIRK_SET_DATA_TIMEOUT_VAL               (1<<30)
       int irq;                /* Device IRQ */
       void __iomem *ioaddr;   /* Mapped address */
 
--

1.6.0.4

2011/2/24 Chuanxiao Dong <[email protected]>:

Quoted 19 lines

Set the timeout control register for SDHCI host when it needs to send some commands which need busy signal. Use the maximum timeout value will be safe.

Signed-off-by Chuanxiao Dong <[email protected]>

---
 drivers/mmc/host/sdhci.c |   14 ++++++++++----

 1 files changed, 10 insertions(+), 4 deletions(-)
drivers/mmc/host/sdhci.c +0 -1
index 9e15f41..32b7475 100644
@@ -40,7 +40,6 @@
 
  static unsigned int debug_quirks = 0;
 
-static void sdhci_prepare_data(struct sdhci_host *, struct mmc_data *);
  static void sdhci_finish_data(struct sdhci_host *);
Quoted 1 line

 static void sdhci_send_command(struct sdhci_host *, struct mmc_command

*);

drivers/mmc/host/sdhci.c static void sdhci_set_transfer_irqs(struct sdhci_host

Quoted 1 line
drivers/mmc/host/sdhci.c +0 -0
@@ -651,16 +650,23 @@ static void sdhci_set_transfer_irqs(struct sdhci_host

*host)

drivers/mmc/host/sdhci.c static void sdhci_set_transfer_irqs(struct sdhci_host · line 653

Quoted 5 lines
drivers/mmc/host/sdhci.c +1 -1
                sdhci_clear_set_irqs(host, dma_irqs, pio_irqs);
  }
 
-static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
+static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command

*cmd)

drivers/mmc/host/sdhci.c static void sdhci_set_transfer_irqs(struct sdhci_host · line 664

Quoted 12 lines
drivers/mmc/host/sdhci.c +4 -1
  {
        u8 count;
        u8 ctrl;
+       struct mmc_data *data = cmd->data;
        int ret;
 
        WARN_ON(host->data);
 
-       if (data == NULL)
+       if (data == NULL) {
+               /*
+                * set timeout to be maximum value for command with busy

signal.

drivers/mmc/host/sdhci.c static void sdhci_set_transfer_irqs(struct sdhci_host · line 667

Quoted 3 lines
drivers/mmc/host/sdhci.c +3 -0
+                */
+               if (cmd->flags & MMC_RSP_BUSY)
+                       sdhci_writeb(host, 0xE,

SDHCI_TIMEOUT_CONTROL);

drivers/mmc/host/sdhci.c static void sdhci_send_command(struct sdhci_host *host,

Quoted 6 lines
drivers/mmc/host/sdhci.c +1 -0
                return;
+       }
 
        /* Sanity checks */
        BUG_ON(data->blksz * data->blocks > 524288);

@@ -920,7 +926,7 @@ static void sdhci_send_command(struct sdhci_host *host,

struct mmc_command *cmd)

Quoted 16 lines
drivers/mmc/host/sdhci.c +1 -2
        host->cmd = cmd;
 
-       sdhci_prepare_data(host, cmd->data);
+       sdhci_prepare_data(host, cmd);
 
        sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT);
 
--

1.6.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

#3 lore raw
Quoted 1 line · Chuanxiao

Great. Maybe we wouldn't need to add this quirk since give a maximum timeout value is safe for all host controller I think. We would better save the quirk for other really critical silicon bugs.

proposed this a while ago and strongly support just removing the quirk for broken timeout and setting the timeout value to maximum of 0xE.

This also handles the case of the sd device having a timeout value too low. In my testing I have come across SD cards that do not provide the correct value. We force our pxa168, pxa910, and mmp2 controllers to have 0xE.

Philip

Quoted 134 lines
Quoted 106 lines

-----Original Message-----
From: Jae hoon Chung [mailto:[email protected]]
Sent: Thursday, February 24, 2011 10:18 PM
To: Dong, Chuanxiao
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Kyungmin Park
Subject: Re: [PATCH 1/1]mmc: set timeout for SDHCI host before sending busy cmd

Hi

This patch looks like this. I sent to the RFC patch..

http://marc.info/?l=linux-mmc&m=129109794815028&w=3

Signed-off-by Jaehoon Chung <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>

---

drivers/mmc/host/sdhci.c  |    6 +++++-
include/linux/mmc/sdhci.h |    3 ++-
2 files changed, 7 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci.c +2 -1
index 782c0ee..3b93d97 100644
@@ -655,8 +655,12 @@ static void sdhci_prepare_data(struct sdhci_host
 *host, struct mmc_data *data)
 
      WARN_ON(host->data);
 
-       if (data == NULL)
+       if (data == NULL) {
+               if ((host->quirks & SDHCI_QUIRK_SET_DATA_TIMEOUT_VAL)

&&

drivers/mmc/host/sdhci.c +3 -0
+                               (host->cmd->flags & MMC_RSP_BUSY))
+                       sdhci_writel(host, 0xE, SDHCI_TIMEOUT_CONTROL);
              return;
+       }
 
      /* Sanity checks */
      BUG_ON(data->blksz * data->blocks > 524288);
include/linux/mmc/sdhci.h +2 -2
index 1fdc673..315ff49 100644
@@ -83,7 +83,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12             (1<<28)
 /* Controller doesn't have HISPD bit field in HI-SPEED SD card */
 #define SDHCI_QUIRK_NO_HISPD_BIT                       (1<<29)
-
+/* Controller need set data timeout value when card is busy */
+#define SDHCI_QUIRK_SET_DATA_TIMEOUT_VAL               (1<<30)
      int irq;                /* Device IRQ */
      void __iomem *ioaddr;   /* Mapped address */
 
--

1.6.0.4

2011/2/24 Chuanxiao Dong <[email protected]>:

Quoted 13 lines

Set the timeout control register for SDHCI host when it needs to send some commands which need busy signal. Use the maximum timeout value will be safe.

Signed-off-by Chuanxiao Dong <[email protected]>
---
 drivers/mmc/host/sdhci.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)
drivers/mmc/host/sdhci.c +0 -0
index 9e15f41..32b7475 100644

drivers/mmc/host/sdhci.c line 45

Quoted 7 lines
drivers/mmc/host/sdhci.c +0 -1
@@ -40,7 +40,6 @@
 
 static unsigned int debug_quirks = 0;
 
-static void sdhci_prepare_data(struct sdhci_host *, struct mmc_data *);
 static void sdhci_finish_data(struct sdhci_host *);
 
 static void sdhci_send_command(struct sdhci_host *, struct mmc_command

*);

drivers/mmc/host/sdhci.c static void sdhci_set_transfer_irqs(struct sdhci_host

Quoted 1 line
drivers/mmc/host/sdhci.c +0 -0
@@ -651,16 +650,23 @@ static void sdhci_set_transfer_irqs(struct sdhci_host

*host)

drivers/mmc/host/sdhci.c static void sdhci_set_transfer_irqs(struct sdhci_host · line 653

Quoted 5 lines
drivers/mmc/host/sdhci.c +1 -1
               sdhci_clear_set_irqs(host, dma_irqs, pio_irqs);
 }
 
-static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
+static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command

*cmd)

drivers/mmc/host/sdhci.c static void sdhci_set_transfer_irqs(struct sdhci_host · line 664

Quoted 12 lines
drivers/mmc/host/sdhci.c +4 -1
 {
       u8 count;
       u8 ctrl;
+       struct mmc_data *data = cmd->data;
       int ret;
 
       WARN_ON(host->data);
 
-       if (data == NULL)
+       if (data == NULL) {
+               /*
+                * set timeout to be maximum value for command with busy

signal.

drivers/mmc/host/sdhci.c static void sdhci_set_transfer_irqs(struct sdhci_host · line 667

Quoted 3 lines
drivers/mmc/host/sdhci.c +3 -0
+                */
+               if (cmd->flags & MMC_RSP_BUSY)
+                       sdhci_writeb(host, 0xE,
Quoted 24 lines

SDHCI_TIMEOUT_CONTROL);

drivers/mmc/host/sdhci.c static void sdhci_send_command(struct sdhci_host *host,

Quoted 6 lines
drivers/mmc/host/sdhci.c +1 -0
               return;
+       }
 
       /* Sanity checks */
       BUG_ON(data->blksz * data->blocks > 524288);

@@ -920,7 +926,7 @@ static void sdhci_send_command(struct sdhci_host *host,

struct mmc_command *cmd)

Quoted 16 lines
drivers/mmc/host/sdhci.c +1 -2
       host->cmd = cmd;
 
-       sdhci_prepare_data(host, cmd->data);
+       sdhci_prepare_data(host, cmd);
 
       sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT);
 
--

1.6.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#4 lore raw
Quoted 4 lines · Philip Rakity

proposed this a while ago and strongly support just removing the quirk for broken timeout and setting the timeout value to maximum of 0xE.

This also handles the case of the sd device having a timeout value too low. In my testing I have come across SD cards that do not provide the correct value. We force our pxa168, pxa910, and mmp2 controllers to have 0xE.

Yeah, OLPC's CaFe controller -- which might be the same hardware as yours, actually -- has the same problem.

Does anyone know of a reason (beyond strict spec-compliance, I suppose) for honoring the timeout value rather than using 0xE everywhere? If not, I'm willing to try out Philip's suggestion.

Thanks,

Signature · 2 lines
Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child
#5 lore raw
Quoted 6 lines · Chris Ball
Quoted 4 lines · Philip Rakity

proposed this a while ago and strongly support just removing the quirk for broken timeout and setting the timeout value to maximum of 0xE.

This also handles the case of the sd device having a timeout value too low. In my testing I have come across SD cards that do not provide the correct value. We force our pxa168, pxa910, and mmp2 controllers to have 0xE.

Quoted 6 lines

Yeah, OLPC's CaFe controller -- which might be the same hardware as yours, actually -- has the same problem.

Does anyone know of a reason (beyond strict spec-compliance, I suppose) for honoring the timeout value rather than using 0xE everywhere? If not, I'm willing to try out Philip's suggestion.

+1. A full cycle in linux-next might an idea to be on the safe side? That would be 2.6.40-material then. Or too slow?

Wolfram

Signature · 2 lines
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#6 lore raw

Hi,

Quoted 2 lines · Wolfram Sang

+1. A full cycle in linux-next might an idea to be on the safe side? That would be 2.6.40-material then. Or too slow?

I think it could be okay to merge for .39, but that's mainly because I don't think we start getting testing with a lot of cards until patches appear in an -rc1 -- so for something that requires broad testing, I'd rather merge it for -rc1 with a plan to revert it if we find anything unexpected.

But I don't have a strong opinion, so if anyone thinks there's a reason to wait (for example, an existence-proof of a card that misbehaves when configured with a max timeout value) then I'm happy to do so.

Thanks,

Signature · 2 lines
Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child
#7 lore raw
In reply to Chris Ball
Quoted 8 lines · Chris Ball
Quoted 4 lines · Philip Rakity

proposed this a while ago and strongly support just removing the quirk for broken timeout and setting the timeout value to maximum of 0xE.

This also handles the case of the sd device having a timeout value too low. In my testing I have come across SD cards that do not provide the correct value. We force our pxa168, pxa910, and mmp2 controllers to have 0xE.

Yeah, OLPC's CaFe controller -- which might be the same hardware as yours, actually -- has the same problem.

different controller not same as on mmp2, pxa168, pxa910

Quoted 10 lines

Does anyone know of a reason (beyond strict spec-compliance, I suppose) for honoring the timeout value rather than using 0xE everywhere? If not, I'm willing to try out Philip's suggestion.

Thanks,

Signature · 2 lines
Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child