Omap MMC init updates and new controller for 2.6.29 merge window

arch/arm/mach-omap1/board-h2-mmc.c +0 -0

index 504ae88..409fa56 100644

arch/arm/mach-omap1/board-h2-mmc.c +4 -0

+static void mmc_shutdown(struct device *dev)
+{
+	gpio_free(H2_TPS_GPIO_MMC_PWR_EN);
+}

Quoted 4 lines · Tony Lindgren

arch/arm/mach-omap1/board-h2-mmc.c +0 -0

index 504ae88..409fa56 100644

[snip]

Quoted 4 lines

arch/arm/mach-omap1/board-h2-mmc.c +4 -0

+static void mmc_shutdown(struct device *dev)
+{
+	gpio_free(H2_TPS_GPIO_MMC_PWR_EN);
+}

arch/arm/mach-omap1/board-h2-mmc.c

Quoted 10 lines

arch/arm/mach-omap1/board-h2-mmc.c +10 -0

+
+/*
+ * H2 could use the following functions tested:
+ * - mmc_get_cover_state that uses OMAP_MPUIO(1)
+ * - mmc_get_wp that uses OMAP_MPUIO(3)
+ */
+static struct omap_mmc_platform_data mmc1_data = {
+	.nr_slots                       = 1,
+	.init				= mmc_late_init,
+	.shutdown			= mmc_shutdown,

It seems that shutdown is no-op, so what about patch below?

Quoted 6 lines · Ladislav Michl

Quoted 4 lines · Tony Lindgren

arch/arm/mach-omap1/board-h2-mmc.c +0 -0

index 504ae88..409fa56 100644

[snip]

Quoted 15 lines

Quoted 4 lines

arch/arm/mach-omap1/board-h2-mmc.c +4 -0

+static void mmc_shutdown(struct device *dev)
+{
+	gpio_free(H2_TPS_GPIO_MMC_PWR_EN);
+}

arch/arm/mach-omap1/board-h2-mmc.c

Quoted 10 lines

arch/arm/mach-omap1/board-h2-mmc.c +10 -0

+
+/*
+ * H2 could use the following functions tested:
+ * - mmc_get_cover_state that uses OMAP_MPUIO(1)
+ * - mmc_get_wp that uses OMAP_MPUIO(3)
+ */
+static struct omap_mmc_platform_data mmc1_data = {
+	.nr_slots                       = 1,
+	.init				= mmc_late_init,
+	.shutdown			= mmc_shutdown,

It seems that shutdown is no-op, so what about patch below?

After all, lets remove some unused fields.

Signed-off-by Ladislav Michl <[email protected]>

arch/arm/plat-omap/include/mach/mmc.h +0 -1

index 031250f..1129e97 100644
@@ -51,7 +51,6 @@ struct omap_mmc_platform_data {
 	 * not supported */
 	int (* init)(struct device *dev);
 	void (* cleanup)(struct device *dev);
-	void (* shutdown)(struct device *dev);
 
 	/* To handle board related suspend/resume functionality for MMC */
 	int (*suspend)(struct device *dev, int slot);

drivers/mmc/host/omap.c +0 -0

index 67d7b7f..84de289 100644

Quoted 21 lines

arch/arm/plat-omap/include/mach/mmc.h +0 -5

index 031250f..1129e97 100644
@@ -51,7 +51,6 @@ struct omap_mmc_platform_data {
 	 * not supported */
 	int (* init)(struct device *dev);
 	void (* cleanup)(struct device *dev);
-	void (* shutdown)(struct device *dev);
 
 	/* To handle board related suspend/resume functionality for MMC */
 	int (*suspend)(struct device *dev, int slot);

@@ -77,10 +76,6 @@ struct omap_mmc_platform_data {
 
 		/* use the internal clock */
 		unsigned internal_clock:1;
-		s16 power_pin;
-
-		int switch_pin;			/* gpio (card detect) */
-		int gpio_wp;			/* gpio (write protect) */
 

arch/arm/plat-omap/include/mach/mmc.h struct omap_mmc_platform_data { · line 81

Quoted 2 lines

arch/arm/plat-omap/include/mach/mmc.h +0 -0

 		int (* set_bus_mode)(struct device *dev, int slot, int bus_mode);
 		int (* set_power)(struct device *dev, int slot, int power_on, int vdd);

Hmm, aren't switch_pin and gpio_wp used at least in the mmc-twl4030.c?

I guess they could be internal to mmc-twl4030.c if not used in the drivers directly.

Quoted 8 lines

drivers/mmc/host/omap.c +0 -0

index 67d7b7f..84de289 100644
@@ -157,8 +157,6 @@ struct mmc_omap_host {
 	struct timer_list	dma_timer;
 	unsigned		dma_len;
 

drivers/mmc/host/omap.c struct mmc_omap_host { · line 162

Quoted 6 lines

drivers/mmc/host/omap.c +0 -2

-	short			power_pin;
-
 	struct mmc_omap_slot    *slots[OMAP_MMC_MAX_SLOTS];
 	struct mmc_omap_slot    *current_slot;
 	spinlock_t              slot_lock;

Looks like power_pin could go though.

This will simplify the MMC low-level init, and make it more flexible to add support for a newer MMC controller in the following patches.

drivers/mmc/host/omap_hsmmc.c new file +13 -0

+static int omap_mmc_remove(struct platform_device *pdev)
+{
+	struct mmc_omap_host *host = platform_get_drvdata(pdev);
+	struct resource *res;
+	u16 vdd = 0;
+
+	if (!(OMAP_HSMMC_READ(host->base, HCTL) & SDVSDET)) {
+	/*
+	 * Set the vdd back to 3V,
+	 * applicable for dual volt support.
+	 */
+		vdd = fls(host->mmc->ocr_avail) - 1;
+		if (omap_mmc_switch_opcond(host, vdd) != 0)

Review

Quoted 6 lines · On Sun, 07 Dec 2008 13:51:39 -0800 Tony Lindgren <[email protected]> wrote:

(quoted diff) +6 -0

+	/*
+	 * Unlike OMAP1 controller, the cmdtype does not seem to be based on
+	 * ac, bc, adtc, bcr. Only CMD12 needs a val of 0x3, rest 0x0.
+	 */
+	if (cmd->opcode == 12)
+		cmdtype = 0x3;

Isn't it more likely that it needs 0x3 for any commands that ends an open ended transfer? I.e. every time cmd == mrq.stop.

Review

Quoted 6 lines

(quoted diff) +6 -0

+	host->dbclk = clk_get(&pdev->dev, "mmchsdb_fck");
+	/*
+	 * MMC can still work without debounce clock.
+	 */
+	if (IS_ERR(host->dbclk))
+		dev_dbg(mmc_dev(host->mmc), "Failed to get debounce clock\n");

Perhaps a warning should be printed though, as there could be some problems caused by trying to init a card that is still bouncing.

Quoted 18 lines

(quoted diff) +18 -0

+static int omap_mmc_remove(struct platform_device *pdev)
+{
+	struct mmc_omap_host *host = platform_get_drvdata(pdev);
+	struct resource *res;
+	u16 vdd = 0;
+
+	if (!(OMAP_HSMMC_READ(host->base, HCTL) & SDVSDET)) {
+	/*
+	 * Set the vdd back to 3V,
+	 * applicable for dual volt support.
+	 */
+		vdd = fls(host->mmc->ocr_avail) - 1;
+		if (omap_mmc_switch_opcond(host, vdd) != 0)
+			host->mmc->ios.vdd = vdd;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (res)

Review

Quoted 5 lines

(quoted diff) +5 -0

+		release_mem_region(res->start, res->end - res->start + 1);
+
+	platform_set_drvdata(pdev, NULL);
+	if (host) {
+		mmc_remove_host(host->mmc);

You can't go fiddling with the voltage and removing the MMIO until you've made sure that there is no more activity on the slot.

Fix the above issues and you can let it go via Russell with a:

Acked-by Pierre Ossman <[email protected]>

From f201a545403fdecaf3e2a77359e4adaf803871f5 Mon Sep 17 00:00:00 2001

Hi,

Quoted 12 lines · Pierre Ossman

Review

Quoted 6 lines · On Sun, 07 Dec 2008 13:51:39 -0800 Tony Lindgren <[email protected]> wrote:

(quoted diff) +6 -0

+	/*
+	 * Unlike OMAP1 controller, the cmdtype does not seem to be based on
+	 * ac, bc, adtc, bcr. Only CMD12 needs a val of 0x3, rest 0x0.
+	 */
+	if (cmd->opcode == 12)
+		cmdtype = 0x3;

Isn't it more likely that it needs 0x3 for any commands that ends an open ended transfer? I.e. every time cmd == mrq.stop.

OK, changed and still works :)

Quoted 9 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+	host->dbclk = clk_get(&pdev->dev, "mmchsdb_fck");
+	/*
+	 * MMC can still work without debounce clock.
+	 */
+	if (IS_ERR(host->dbclk))
+		dev_dbg(mmc_dev(host->mmc), "Failed to get debounce clock\n");

Perhaps a warning should be printed though, as there could be some problems caused by trying to init a card that is still bouncing.

OK

Quoted 18 lines

Quoted 18 lines

(quoted diff) +18 -0

+static int omap_mmc_remove(struct platform_device *pdev)
+{
+	struct mmc_omap_host *host = platform_get_drvdata(pdev);
+	struct resource *res;
+	u16 vdd = 0;
+
+	if (!(OMAP_HSMMC_READ(host->base, HCTL) & SDVSDET)) {
+	/*
+	 * Set the vdd back to 3V,
+	 * applicable for dual volt support.
+	 */
+		vdd = fls(host->mmc->ocr_avail) - 1;
+		if (omap_mmc_switch_opcond(host, vdd) != 0)
+			host->mmc->ios.vdd = vdd;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (res)

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+		release_mem_region(res->start, res->end - res->start + 1);
+
+	platform_set_drvdata(pdev, NULL);
+	if (host) {
+		mmc_remove_host(host->mmc);

You can't go fiddling with the voltage and removing the MMIO until you've made sure that there is no more activity on the slot.

Changed in the attached patch. I've changed the code to reset the controller voltage setting after powering down the socket instead of trying to do it in two earlier places..

Do you have any better suggestions for this? Basically it looks like the controller may fail to detect the next card after unplugging a 1.8V card unless the controller is reset to 3V.

Quoted 3 lines

Fix the above issues and you can let it go via Russell with a:

Acked-by Pierre Ossman <[email protected]>

Thank you for your comments. Care to take a one more look at the attached patch before I add your Ack? I also fixed the remaining checkpatch warnings.

Regards,

Tony
From ce15009433bcc482c439326fd08960e6d1f0eb1a Mon Sep 17 00:00:00 2001

Here's one more version that leaves out the unnecessary TWL4030 dependency in the Kconfig as pointed out by David Brownell.

Quoted 4 lines · On Tue, 30 Dec 2008 10:36:26 +0200 Tony Lindgren <[email protected]> wrote:

Here's one more version that leaves out the unnecessary TWL4030 dependency in the Kconfig as pointed out by David Brownell.

Looks ok.

Acked-by Pierre Ossman <[email protected]>

Signature · 15 lines

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 9 lines · Pierre Ossman

On Tue, 30 Dec 2008 10:36:26 +0200

Quoted 2 lines · Tony Lindgren

Here's one more version that leaves out the unnecessary TWL4030 dependency in the Kconfig as pointed out by David Brownell.

Looks ok.

Acked-by Pierre Ossman <[email protected]>

Thanks, I'll queue it up with Russell with one change to include <mach/dma.h> instead of <asm/dma.h>.

All further patches to this driver will come to you via LKML.

Regards,

Tony

Quoted 6 lines

--
-- Pierre Ossman

Linux kernel, MMC maintainer http://www.kernel.org
rdesktop, core developer http://www.rdesktop.org

Quoted 9 lines

WARNING: This correspondence is being monitored by the Swedish government. Make sure your server uses encryption for SMTP traffic and consider using PGP for end-to-end encryption.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

-------------------------------------------------------------------

List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php

Tony, Pierre,

Here is a patch that fixes a MMC host controller deadlock. The problem happens when removing the MMC/SD device when a transfer is on-going.

It has been tested on OMAP3430 but this fix should apply to OMAP2 chips as well, as seen from the TRMs.

What do you think?

Regards,
Jean.

OMAP: MMC: recover from transfer failures

Timeouts during a command that has a data phase can result in the next command issued after the command that failed not being processed, i.e. no interrupt ever occurs to indicate the command has completed. This failure can result in a deadlock.

The indication that the mmc host controller is in this error state is that the DATI bit is set in the PSTATE register. This patch checks to see if the DATI bit is set before starting a new command, and if it is set it resets the data state machine to clear the error.

Tested on OMAP3430 chip and intensive MMC/SD device removal while transferring data.

Regards,
Jean

From 48a40925a4d57f6e7e45809a4b5c09b20f9314d7 Mon Sep 17 00:00:00 2001
From: Jean Pihet <[email protected]>
Date: Tue, 16 Dec 2008 19:44:12 +0100
Subject: [PATCH] OMAP: MMC: recover from transfer failures

Timeouts during a command that has a data phase can result in the next command issued after the command that failed not being processed, i.e. no interrupt ever occurs to indicate the command has completed. This failure can result in a deadlock.

The indication that the mmc host controller is in this error state is that the DATI bit is set in the PSTATE register. This patch checks to see if the DATI bit is set before starting a new command, and if it is set it resets the data state machine to clear the error.

Tested on OMAP3430 chip and intensive MMC/SD device removal while transferring data.

Signed-off-by Andy Lowe <[email protected]>

Signed-off-by Jean Pihet <[email protected]>

---
 drivers/mmc/host/omap_hsmmc.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

drivers/mmc/host/omap_hsmmc.c +11 -0

index 1fcc544..b7d111e 100644
@@ -44,6 +44,7 @@
 #define OMAP_HSMMC_RSP54       0x0118
 #define OMAP_HSMMC_RSP76       0x011C
 #define OMAP_HSMMC_DATA                0x0120
+#define OMAP_HSMMC_PSTATE      0x0124
 #define OMAP_HSMMC_HCTL                0x0128
 #define OMAP_HSMMC_SYSCTL      0x012C
 #define OMAP_HSMMC_STAT                0x0130

@@ -89,6 +90,7 @@
 #define DUAL_VOLT_OCR_BIT      7
 #define SRC                    (1 << 25)
 #define SRD                    (1 << 26)
+#define DATI                   (1 << 1)
 
 /*
  * FIXME: Most likely all the data using these _DEVID defines should come

@@ -273,6 +275,15 @@ mmc_omap_start_command(struct mmc_omap_host *host, struct 
 mmc_command *cmd,
                mmc_hostname(host->mmc), cmd->opcode, cmd->arg);
        host->cmd = cmd;
 
+       if (OMAP_HSMMC_READ(host->base, PSTATE) & DATI) {
+               dev_dbg(mmc_dev(host->mmc), "mmc_omap_start_command: "
+                       " resetting data state machine\n");
+               OMAP_HSMMC_WRITE(host->base, SYSCTL,
+                       OMAP_HSMMC_READ(host->base, SYSCTL) | SRD);
+               while (OMAP_HSMMC_READ(host->base, SYSCTL) & SRD)
+                       ;
+       }
+
        /*
         * Clear status bits and enable interrupts

         */

Signature · 1 line

1.5.4.4.21.gc4a6c

Quoted 37 lines · On Wednesday 07 January 2009 11:18:13 Tony Lindgren wrote:

Quoted 9 lines · Pierre Ossman

On Tue, 30 Dec 2008 10:36:26 +0200

Quoted 2 lines · Tony Lindgren

Here's one more version that leaves out the unnecessary TWL4030 dependency in the Kconfig as pointed out by David Brownell.

Looks ok.

Acked-by Pierre Ossman <[email protected]>

Thanks, I'll queue it up with Russell with one change to include <mach/dma.h> instead of <asm/dma.h>.

All further patches to this driver will come to you via LKML.

Regards,

Tony

Quoted 6 lines

--
-- Pierre Ossman

Linux kernel, MMC maintainer http://www.kernel.org
rdesktop, core developer http://www.rdesktop.org

Quoted 9 lines

WARNING: This correspondence is being monitored by the Swedish government. Make sure your server uses encryption for SMTP traffic and consider using PGP for end-to-end encryption.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 4 lines

-------------------------------------------------------------------

List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php

From 48a40925a4d57f6e7e45809a4b5c09b20f9314d7 Mon Sep 17 00:00:00 2001
From: Jean Pihet <[email protected]>
Date: Tue, 16 Dec 2008 19:44:12 +0100
Subject: [PATCH] OMAP: MMC: recover from transfer failures

Timeouts during a command that has a data phase can result in the next command issued after the command that failed not being processed, i.e. no interrupt ever occurs to indicate the command has completed. This failure can result in a deadlock.

The indication that the mmc host controller is in this error state is that the DATI bit is set in the PSTATE register. This patch checks to see if the DATI bit is set before starting a new command, and if it is set it resets the data state machine to clear the error.

Tested on OMAP3430 chip and intensive MMC/SD device removal while transferring data.

Signed-off-by Andy Lowe <[email protected]>

Signed-off-by Jean Pihet <[email protected]>

---
 drivers/mmc/host/omap_hsmmc.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

drivers/mmc/host/omap_hsmmc.c +11 -0

index 1fcc544..b7d111e 100644
@@ -44,6 +44,7 @@
 #define OMAP_HSMMC_RSP54	0x0118
 #define OMAP_HSMMC_RSP76	0x011C
 #define OMAP_HSMMC_DATA		0x0120
+#define OMAP_HSMMC_PSTATE	0x0124
 #define OMAP_HSMMC_HCTL		0x0128
 #define OMAP_HSMMC_SYSCTL	0x012C
 #define OMAP_HSMMC_STAT		0x0130

@@ -89,6 +90,7 @@
 #define DUAL_VOLT_OCR_BIT	7
 #define SRC			(1 << 25)
 #define SRD			(1 << 26)
+#define DATI			(1 << 1)
 
 /*
  * FIXME: Most likely all the data using these _DEVID defines should come

@@ -273,6 +275,15 @@ mmc_omap_start_command(struct mmc_omap_host *host, struct mmc_command *cmd,
 		mmc_hostname(host->mmc), cmd->opcode, cmd->arg);
 	host->cmd = cmd;
 
+	if (OMAP_HSMMC_READ(host->base, PSTATE) & DATI) {
+		dev_dbg(mmc_dev(host->mmc), "mmc_omap_start_command: "
+			" resetting data state machine\n");
+		OMAP_HSMMC_WRITE(host->base, SYSCTL,
+			OMAP_HSMMC_READ(host->base, SYSCTL) | SRD);
+		while (OMAP_HSMMC_READ(host->base, SYSCTL) & SRD)
+			;
+	}
+
 	/*
 	 * Clear status bits and enable interrupts
 	 */

Signature · 1 line

1.5.4.4.21.gc4a6c

Tony, Pierre,

Here is a patch that fixes a MMC host controller deadlock. The problem happens when removing the MMC/SD device when a transfer is on-going.

It has been tested on OMAP3430 but this fix should apply to OMAP2 chips as well, as seen from the TRMs.

What do you think?

Regards,
Jean.

OMAP: MMC: recover from transfer failures

Timeouts during a command that has a data phase can result in the next command issued after the command that failed not being processed, i.e. no interrupt ever occurs to indicate the command has completed. This failure can result in a deadlock.

The indication that the mmc host controller is in this error state is that the DATI bit is set in the PSTATE register. This patch checks to see if the DATI bit is set before starting a new command, and if it is set it resets the data state machine to clear the error.

Tested on OMAP3430 chip and intensive MMC/SD device removal while transferring data.

Regards,
Jean

From 48a40925a4d57f6e7e45809a4b5c09b20f9314d7 Mon Sep 17 00:00:00 2001
From: Jean Pihet <[email protected]>
Date: Tue, 16 Dec 2008 19:44:12 +0100
Subject: [PATCH] OMAP: MMC: recover from transfer failures

Timeouts during a command that has a data phase can result in the next command issued after the command that failed not being processed, i.e. no interrupt ever occurs to indicate the command has completed. This failure can result in a deadlock.

The indication that the mmc host controller is in this error state is that the DATI bit is set in the PSTATE register. This patch checks to see if the DATI bit is set before starting a new command, and if it is set it resets the data state machine to clear the error.

Tested on OMAP3430 chip and intensive MMC/SD device removal while transferring data.

Signed-off-by Andy Lowe <[email protected]>

Signed-off-by Jean Pihet <[email protected]>

---
 drivers/mmc/host/omap_hsmmc.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

drivers/mmc/host/omap_hsmmc.c +11 -0

index 1fcc544..b7d111e 100644
@@ -44,6 +44,7 @@
 #define OMAP_HSMMC_RSP54       0x0118
 #define OMAP_HSMMC_RSP76       0x011C
 #define OMAP_HSMMC_DATA                0x0120
+#define OMAP_HSMMC_PSTATE      0x0124
 #define OMAP_HSMMC_HCTL                0x0128
 #define OMAP_HSMMC_SYSCTL      0x012C
 #define OMAP_HSMMC_STAT                0x0130

@@ -89,6 +90,7 @@
 #define DUAL_VOLT_OCR_BIT      7
 #define SRC                    (1 << 25)
 #define SRD                    (1 << 26)
+#define DATI                   (1 << 1)
 
 /*
  * FIXME: Most likely all the data using these _DEVID defines should come

@@ -273,6 +275,15 @@ mmc_omap_start_command(struct mmc_omap_host *host, struct 
 mmc_command *cmd,
                mmc_hostname(host->mmc), cmd->opcode, cmd->arg);
        host->cmd = cmd;
 
+       if (OMAP_HSMMC_READ(host->base, PSTATE) & DATI) {
+               dev_dbg(mmc_dev(host->mmc), "mmc_omap_start_command: "
+                       " resetting data state machine\n");
+               OMAP_HSMMC_WRITE(host->base, SYSCTL,
+                       OMAP_HSMMC_READ(host->base, SYSCTL) | SRD);
+               while (OMAP_HSMMC_READ(host->base, SYSCTL) & SRD)
+                       ;
+       }
+
        /*
         * Clear status bits and enable interrupts

         */

Why not do the reset in mmc_omap_irq() like the other resets? e.g.

static irqreturn_t mmc_omap_irq(int irq, void *dev_id)
...
                if ((status & CMD_TIMEOUT) ||
                        (status & CMD_CRC)) {
                        if (host->cmd) {
                                if (status & CMD_TIMEOUT) { OMAP_HSMMC_WRITE(host->base, SYSCTL, OMAP_HSMMC_READ(host->base, SYSCTL) | SRC); while (OMAP_HSMMC_READ(host->base, SYSCTL) & SRC) ;

                                        host->cmd->error = -ETIMEDOUT; } else {
                                        host->cmd->error = -EILSEQ; }
                                end_cmd = 1;
                        }
-                       if (host->data)
+                       if (host->data) { mmc_dma_cleanup(host);
+                               OMAP_HSMMC_WRITE(host->base, SYSCTL, +                                       OMAP_HSMMC_READ(host->base,

+                                                       SYSCTL) | SRD); +                               while (OMAP_HSMMC_READ(host->base, +                                               SYSCTL) & SRD) +                                       ;
+                       }
                }
                if ((status & DATA_TIMEOUT) ||
                        (status & DATA_CRC)) {

Thanks for the suggestion, it works great (stress tested) and is a cleaner fix. Here is a patch the latest linux-omap-2.6 tree. Is this OK?

Regards,
Jean

From d143f6b2e705aa4e9d2b032097fd1c82f8163262 Mon Sep 17 00:00:00 2001
From: Jean Pihet <[email protected]>
Date: Thu, 8 Jan 2009 12:35:21 +0100
Subject: [PATCH] OMAP: MMC: recover from transfer failures

Timeouts during a command that has a data phase can result in the next command issued after the command that failed not being processed, i.e. no interrupt ever occurs to indicate the command has completed. This failure can result in a deadlock.

This patch resets the data state machine to clear the error in case of a command timeout.

Tested on OMAP3430 chip and intensive MMC/SD device removal while transferring data.

Signed-off-by Andy Lowe <[email protected]>

Signed-off-by Jean Pihet <[email protected]>

Signed-off-by Adrian Hunter <[email protected]>

---
 drivers/mmc/host/omap_hsmmc.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

drivers/mmc/host/omap_hsmmc.c +8 -2

index d5c1e9d..97150c0 100644
@@ -464,8 +464,15 @@ static irqreturn_t mmc_omap_irq(int irq, void *dev_id)
                                }
                                end_cmd = 1;
                        }
-                       if (host->data)
+                       if (host->data) {
                                mmc_dma_cleanup(host);
+                               OMAP_HSMMC_WRITE(host->base, SYSCTL,
+                                       OMAP_HSMMC_READ(host->base,
+                                                       SYSCTL) | SRD);
+                               while (OMAP_HSMMC_READ(host->base,
+                                               SYSCTL) & SRD)
+                                       ;
+                       }
                }
                if ((status & DATA_TIMEOUT) ||
                        (status & DATA_CRC)) {
--

1.5.4.4.21.gc4a6c

Quoted 32 lines · On Thursday 08 January 2009 10:02:24 Adrian Hunter wrote:

Why not do the reset in mmc_omap_irq() like the other resets? e.g.

static irqreturn_t mmc_omap_irq(int irq, void *dev_id)
...

                if ((status & CMD_TIMEOUT) ||
                        (status & CMD_CRC)) {
                        if (host->cmd) {
                                if (status & CMD_TIMEOUT) {
                                        OMAP_HSMMC_WRITE(host->base,

SYSCTL, OMAP_HSMMC_READ(host->base, SYSCTL) | SRC); while (OMAP_HSMMC_READ(host->base, SYSCTL) & SRC) ;

                                        host->cmd->error = -ETIMEDOUT;
                                } else {
                                        host->cmd->error = -EILSEQ;
                                }
                                end_cmd = 1;
                        }

drivers/mmc/host/omap_hsmmc.c +8 -1

-                       if (host->data)
+                       if (host->data) {
                                mmc_dma_cleanup(host);
+                               OMAP_HSMMC_WRITE(host->base, SYSCTL,
+                                       OMAP_HSMMC_READ(host->base,
+                                                       SYSCTL) | SRD);
+                               while (OMAP_HSMMC_READ(host->base,
+                                               SYSCTL) & SRD)
+                                       ;
+                       }
                }
                if ((status & DATA_TIMEOUT) ||
                        (status & DATA_CRC)) {

Thanks for the suggestion, it works great (stress tested) and is a cleaner fix.
Here is a patch the latest linux-omap-2.6 tree. Is this OK?

Regards,
Jean

From d143f6b2e705aa4e9d2b032097fd1c82f8163262 Mon Sep 17 00:00:00 2001
From: Jean Pihet <[email protected]>
Date: Thu, 8 Jan 2009 12:35:21 +0100
Subject: [PATCH] OMAP: MMC: recover from transfer failures

Timeouts during a command that has a data phase can result in the next command issued after the command that failed not being processed, i.e. no interrupt ever occurs to indicate the command has completed. This failure can result in a deadlock.

This patch resets the data state machine to clear the error in case of a command timeout.

Tested on OMAP3430 chip and intensive MMC/SD device removal while transferring data.

Signed-off-by Andy Lowe <[email protected]>

Signed-off-by Jean Pihet <[email protected]>

Signed-off-by Adrian Hunter <[email protected]>

---
 drivers/mmc/host/omap_hsmmc.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

drivers/mmc/host/omap_hsmmc.c +8 -2

index d5c1e9d..97150c0 100644
@@ -464,8 +464,15 @@ static irqreturn_t mmc_omap_irq(int irq, void *dev_id)
                                }
                                end_cmd = 1;
                        }
-                       if (host->data)
+                       if (host->data) {
                                mmc_dma_cleanup(host);
+                               OMAP_HSMMC_WRITE(host->base, SYSCTL,
+                                       OMAP_HSMMC_READ(host->base,
+                                                       SYSCTL) | SRD);
+                               while (OMAP_HSMMC_READ(host->base,
+                                               SYSCTL) & SRD)
+                                       ;
+                       }
                }
                if ((status & DATA_TIMEOUT) ||
                        (status & DATA_CRC)) {
--

1.5.4.4.21.gc4a6c

Quoted 32 lines · On Thursday 08 January 2009 10:02:24 Adrian Hunter wrote:

Why not do the reset in mmc_omap_irq() like the other resets? e.g.

static irqreturn_t mmc_omap_irq(int irq, void *dev_id)
...
                if ((status & CMD_TIMEOUT) ||
                        (status & CMD_CRC)) {
                        if (host->cmd) {
                                if (status & CMD_TIMEOUT) { OMAP_HSMMC_WRITE(host->base, SYSCTL, OMAP_HSMMC_READ(host->base, SYSCTL) | SRC); while (OMAP_HSMMC_READ(host->base, SYSCTL) & SRC) ;

                                        host->cmd->error = -ETIMEDOUT; } else {
                                        host->cmd->error = -EILSEQ; }
                                end_cmd = 1;
                        }
-                       if (host->data)
+                       if (host->data) { mmc_dma_cleanup(host);
+                               OMAP_HSMMC_WRITE(host->base, SYSCTL, +                                       OMAP_HSMMC_READ(host->base, +                                                       SYSCTL) | SRD); +                               while (OMAP_HSMMC_READ(host->base, +                                               SYSCTL) & SRD) +                                       ;
+                       }
                }
                if ((status & DATA_TIMEOUT) ||
                        (status & DATA_CRC)) {

Tony,

Has this patch been applied to the linux-omap tree? Does it need to go the patchwork?

Cf. http://marc.info/?l=linux-omap&m=123141577308177&w=2

Regards,
Jean

Quoted 87 lines · On Thursday 08 January 2009 12:49:19 Jean Pihet wrote:

Thanks for the suggestion, it works great (stress tested) and is a cleaner fix.
Here is a patch the latest linux-omap-2.6 tree. Is this OK?

Regards,
Jean

From d143f6b2e705aa4e9d2b032097fd1c82f8163262 Mon Sep 17 00:00:00 2001
From: Jean Pihet <[email protected]>
Date: Thu, 8 Jan 2009 12:35:21 +0100
Subject: [PATCH] OMAP: MMC: recover from transfer failures

Timeouts during a command that has a data phase can result in the next command issued after the command that failed not being processed, i.e. no interrupt ever occurs to indicate the command has completed. This failure can result in a deadlock.

This patch resets the data state machine to clear the error in case of a command timeout.

Tested on OMAP3430 chip and intensive MMC/SD device removal while transferring data.

Signed-off-by Andy Lowe <[email protected]>

Signed-off-by Jean Pihet <[email protected]>

Signed-off-by Adrian Hunter <[email protected]>

---
 drivers/mmc/host/omap_hsmmc.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

drivers/mmc/host/omap_hsmmc.c +8 -2

index d5c1e9d..97150c0 100644
@@ -464,8 +464,15 @@ static irqreturn_t mmc_omap_irq(int irq, void *dev_id)
                                }
                                end_cmd = 1;
                        }
-                       if (host->data)
+                       if (host->data) {
                                mmc_dma_cleanup(host);
+                               OMAP_HSMMC_WRITE(host->base, SYSCTL,
+                                       OMAP_HSMMC_READ(host->base,
+                                                       SYSCTL) | SRD);
+                               while (OMAP_HSMMC_READ(host->base,
+                                               SYSCTL) & SRD)
+                                       ;
+                       }
                }
                if ((status & DATA_TIMEOUT) ||
                        (status & DATA_CRC)) {
--

1.5.4.4.21.gc4a6c

Quoted 32 lines · On Thursday 08 January 2009 10:02:24 Adrian Hunter wrote:

Why not do the reset in mmc_omap_irq() like the other resets? e.g.

static irqreturn_t mmc_omap_irq(int irq, void *dev_id)
...
                if ((status & CMD_TIMEOUT) ||
                        (status & CMD_CRC)) {
                        if (host->cmd) {
                                if (status & CMD_TIMEOUT) { OMAP_HSMMC_WRITE(host->base, SYSCTL, OMAP_HSMMC_READ(host->base, SYSCTL) | SRC); while (OMAP_HSMMC_READ(host->base, SYSCTL) & SRC) ;

                                        host->cmd->error = -ETIMEDOUT; } else {
                                        host->cmd->error = -EILSEQ; }
                                end_cmd = 1;
                        }
-                       if (host->data)
+                       if (host->data) { mmc_dma_cleanup(host);
+                               OMAP_HSMMC_WRITE(host->base, SYSCTL, +                                       OMAP_HSMMC_READ(host->base, +                                                       SYSCTL) | SRD); +                               while (OMAP_HSMMC_READ(host->base, +                                               SYSCTL) & SRD) +                                       ;
+                       }
                }
                if ((status & DATA_TIMEOUT) ||
                        (status & DATA_CRC)) {

Hi,

Quoted 6 lines · Jean Pihet

Tony,

Has this patch been applied to the linux-omap tree? Does it need to go the patchwork?

Cf. http://marc.info/?l=linux-omap&m=123141577308177&w=2

I'm not applying MMC patches, we need to move that discussion to LKML and keep Pierre involved.

Jarkko Lavinen was planning to set up a git branch against the mainline tree for the omap mmc patches, so until we have that, let's just let the patches float on the list for a while.

But yeah, if you want patchwork to pick up your patch so it does not get lost, just please resend it to the linux-omap list and it should get automatically picked up by patchwork.

Regards,

Tony

drivers/mmc/host/omap_hsmmc.c static irqreturn_t mmc_omap_irq(int irq, void *dev_id) · line 474

Quoted 3 lines · On Tue, 3 Feb 2009 15:05:58 +0100 Jean Pihet <[email protected]> wrote:

drivers/mmc/host/omap_hsmmc.c +3 -0

+	while (OMAP_HSMMC_READ(host->base,
+ 			SYSCTL) & SRD)
+ 		;

Is a __raw_readl() sufficient to prevent the cpu from burning up here, or should we add cpu_relax()?

An infinite loop which assumes the hardware is perfect is always a worry. But I see the driver already does that, so we're no worse off..

Quoted 9 lines · Andrew Morton

On Tue, 3 Feb 2009 15:05:58 +0100

drivers/mmc/host/omap_hsmmc.c static irqreturn_t mmc_omap_irq(int irq, void *dev_id) · line 474

Quoted 3 lines · Jean Pihet

drivers/mmc/host/omap_hsmmc.c +3 -0

+	while (OMAP_HSMMC_READ(host->base,
+ 			SYSCTL) & SRD)
+ 		;

Is a __raw_readl() sufficient to prevent the cpu from burning up here, or should we add cpu_relax()?

The __raw_readl() should be sufficient. The MMC controller is located on the L4 CORE interconnect, so the round trip latency for the read from MMC is at least 90 ns, while the CPU cycle time is only about 1 to 2 ns.

Quoted 3 lines

An infinite loop which assumes the hardware is perfect is always a worry. But I see the driver already does that, so we're no worse off..

- Paul

Quoted 3 lines · On Thursday 05 February 2009 21:32:03 Paul Walmsley wrote:

Quoted 2 lines · Andrew Morton

An infinite loop which assumes the hardware is perfect is always a worry. But I see the driver already does that, so we're no worse off..

Do you want a finite loop with udelay in it? I located 4 places were this could be used. If so I can generate a new patch for that.

Quoted 7 lines

Quoted 4 lines · Andrew Morton

Quoted 3 lines

An infinite loop which assumes the hardware is perfect is always a worry. But I see the driver already does that, so we're no worse off..

Do you want a finite loop with udelay in it? I located 4 places were this could be used. If so I can generate a new patch for that.

Even if Andrew doesn't, I'd sure like it. (the finite bit at least) :)

Related, who is the maintainer of this driver? Tony? I'd like to have someone who checks patches before I queue them up.

Rgds

Hi,

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines · Andrew Morton

Quoted 3 lines

An infinite loop which assumes the hardware is perfect is always a worry. But I see the driver already does that, so we're no worse off..

Quoted 3 lines

Quoted 2 lines

Do you want a finite loop with udelay in it? I located 4 places were this could be used. If so I can generate a new patch for that.

Even if Andrew doesn't, I'd sure like it. (the finite bit at least) :)

Ok here is a patch that replaces the infinite loops with a timeout version. This patch applies on top of the previous one I sent ('[PATCH] OMAP: MMC:
recover from transfer failures - Resend'). Is that OK?

Quoted 7 lines

Related, who is the maintainer of this driver? Tony? I'd like to have someone who checks patches before I queue them up.

Rgds

From 5ee867d09efe22a903ac7373a05e9e047bad6544 Mon Sep 17 00:00:00 2001

From: Jean Pihet <[email protected]>
Date: Fri, 6 Feb 2009 16:42:51 +0100
Subject: [PATCH] OMAP: MMC: replace infinite loops with timeouts

Replace the 'while() ;' with a timeout

Signed-off-by Jean Pihet <[email protected]>

---
 drivers/mmc/host/omap_hsmmc.c |   56
+++++++++++++++++++++++++++++++++--------
 1 files changed, 45 insertions(+), 11 deletions(-)

drivers/mmc/host/omap_hsmmc.c +14 -3

index 1ac6918..7ddc77e 100644
@@ -102,6 +102,8 @@
 #define OMAP_MMC_DATADIR_READ  1
 #define OMAP_MMC_DATADIR_WRITE 2
 #define MMC_TIMEOUT_MS         20
+#define MMC_TIMEOUT_WAIT       100     /* Active wait duration, in usec */
+#define MMC_TIMEOUT_WAIT_LOOPS ((MMC_TIMEOUT_MS * 1000) / MMC_TIMEOUT_WAIT)
 #define OMAP_MMC_MASTER_CLOCK  96000000
 #define DRIVER_NAME            "mmci-omap-hs"
 

@@ -384,6 +386,7 @@ static irqreturn_t mmc_omap_irq(int irq, void *dev_id)
        struct mmc_omap_host *host = dev_id;
        struct mmc_data *data;
        int end_cmd = 0, end_trans = 0, status;
+       unsigned long timeout_counter;
 
        if (host->cmd == NULL && host->data == NULL) {
                OMAP_HSMMC_WRITE(host->base, STAT,

@@ -406,9 +409,17 @@ static irqreturn_t mmc_omap_irq(int irq, void *dev_id)
                                        OMAP_HSMMC_WRITE(host->base, SYSCTL,
                                                OMAP_HSMMC_READ(host->base,
                                                                SYSCTL) |
 SRC);
-                                       while (OMAP_HSMMC_READ(host->base,
-                                                       SYSCTL) & SRC)
-                                               ;
+                                       timeout_counter = 0;
+                                       while ((OMAP_HSMMC_READ(host->base,
+                                                               SYSCTL) & SRC)
+                                               && (timeout_counter++ <
+
 MMC_TIMEOUT_WAIT_LOOPS))
+
 udelay(MMC_TIMEOUT_WAIT);
+
+                                       if (OMAP_HSMMC_READ(host->base,
+                                                               SYSCTL) & SRC)
+                                               dev_err(mmc_dev(host->mmc),
+                                                       "Timeout waiting on

SRC\n");

                                        host->cmd->error = -ETIMEDOUT;
                                } else {

drivers/mmc/host/omap_hsmmc.c +11 -3

@@ -421,9 +432,17 @@ static irqreturn_t mmc_omap_irq(int irq, void *dev_id)
                                OMAP_HSMMC_WRITE(host->base, SYSCTL,
                                        OMAP_HSMMC_READ(host->base,
                                                        SYSCTL) | SRD);
-                               while (OMAP_HSMMC_READ(host->base,
-                                               SYSCTL) & SRD)
-                                       ;
+                               timeout_counter = 0;
+                               while ((OMAP_HSMMC_READ(host->base,
+                                                       SYSCTL) & SRD)
+                                       && (timeout_counter++ <
+
 MMC_TIMEOUT_WAIT_LOOPS))
+                                       udelay(MMC_TIMEOUT_WAIT);
+
+                               if (OMAP_HSMMC_READ(host->base,
+                                                       SYSCTL) & SRD)
+                                       dev_err(mmc_dev(host->mmc),
+                                               "Timeout waiting on SRD\n");
                        }
                }

                if ((status & DATA_TIMEOUT) ||

drivers/mmc/host/omap_hsmmc.c +11 -3

@@ -436,9 +455,17 @@ static irqreturn_t mmc_omap_irq(int irq, void *dev_id)
                                OMAP_HSMMC_WRITE(host->base, SYSCTL,
                                        OMAP_HSMMC_READ(host->base,
                                                        SYSCTL) | SRD);
-                               while (OMAP_HSMMC_READ(host->base,
-                                               SYSCTL) & SRD)
-                                       ;
+                               timeout_counter = 0;
+                               while ((OMAP_HSMMC_READ(host->base,
+                                                       SYSCTL) & SRD)
+                                       && (timeout_counter++ <
+
 MMC_TIMEOUT_WAIT_LOOPS))
+                                       udelay(MMC_TIMEOUT_WAIT);
+
+                               if (OMAP_HSMMC_READ(host->base,
+                                                       SYSCTL) & SRD)
+                                       dev_err(mmc_dev(host->mmc),
+                                               "Timeout waiting on SRD\n");
                                end_trans = 1;
                        }

                }

drivers/mmc/host/omap_hsmmc.c +9 -2

@@ -524,6 +551,7 @@ static void mmc_omap_detect(struct work_struct *work)
 {
        struct mmc_omap_host *host = container_of(work, struct mmc_omap_host,
                                                mmc_carddetect_work);
+       unsigned long timeout;
 
        sysfs_notify(&host->mmc->class_dev.kobj, NULL, "cover_switch");
        if (host->carddetect) {

@@ -531,8 +559,14 @@ static void mmc_omap_detect(struct work_struct *work)
        } else {
                OMAP_HSMMC_WRITE(host->base, SYSCTL,
                        OMAP_HSMMC_READ(host->base, SYSCTL) | SRD);
-               while (OMAP_HSMMC_READ(host->base, SYSCTL) & SRD)
-                       ;
+               /* Wait till the SRD bit is reset */
+               timeout = jiffies + msecs_to_jiffies(MMC_TIMEOUT_MS);
+               while ((OMAP_HSMMC_READ(host->base, SYSCTL) & SRD)
+                       && time_before(jiffies, timeout))
+                       msleep(1);
+
+               if (OMAP_HSMMC_READ(host->base, SYSCTL) & SRD)
+                       dev_err(mmc_dev(host->mmc), "Timeout waiting on
 SRD\n");
 
                mmc_detect_change(host->mmc, (HZ * 50) / 1000);

        }

--
1.6.0.1.42.gf8c9c

Quoted 7 lines · Jean Pihet

Hi,

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines · Andrew Morton

Quoted 3 lines

An infinite loop which assumes the hardware is perfect is always a worry. But I see the driver already does that, so we're no worse off..

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Do you want a finite loop with udelay in it? I located 4 places were this could be used. If so I can generate a new patch for that.

Even if Andrew doesn't, I'd sure like it. (the finite bit at least) :)

Ok here is a patch that replaces the infinite loops with a timeout version. This patch applies on top of the previous one I sent ('[PATCH]
OMAP: MMC: recover from transfer failures - Resend'). Is that OK?

What about making use of a function like this:

static void reset_host_controller(struct mmc_omap_host *host, unsigned bit)

{
	unsigned long i, limit = loops_per_jiffy;

	OMAP_HSMMC_WRITE(host->base, SYSCTL,
			 OMAP_HSMMC_READ(host->base, SYSCTL) | bit);
	for (i = 0; i < limit; i++) {
		if (!(OMAP_HSMMC_READ(host->base, SYSCTL) & bit))
			return;
		cpu_relax();
	}
	dev_err(mmc_dev(host->mmc), "%s: timeout waiting on software reset\n",
		mmc_hostname(host->mmc));
}

And then just put:

	reset_host_controller(host, SRD);

and

	reset_host_controller(host, SRC);

in the right places.

Related, who is the maintainer of this driver? Tony? I'd like to have someone who checks patches before I queue them up.

Quoted 2 lines · Pierre Ossman

Related, who is the maintainer of this driver? Tony? I'd like to have someone who checks patches before I queue them up.

I can be. Tony asked me a month ago to set up git tree for OMAP HSMMC and put the workaround for the HSMMC multi-block DMA read corruption there, but hate to admit things have been hanging on todo list.

There are some other fixes waiting. Omap_mmc_set_ios() doesn't handle power off case correctly and resume from suspend can hang if the SDBP bit fails to set.

Do you want a finite loop with udelay in it? I located 4 places were this could be used. If so I can generate a new patch for that.

Quoted 7 lines

Quoted 4 lines

+   while (OMAP_HSMMC_READ(host->base,
+                   SYSCTL) & SRD)
+           ;

Is a __raw_readl() sufficient to prevent the cpu from burning up here, or should we add cpu_relax()?

The __raw_readl() should be sufficient.  The MMC controller is located on the L4 CORE interconnect, so the round trip latency for the read from MMC is at least 90 ns, while the CPU cycle time is only about 1 to 2 ns.

Hi Pierre & Russell,

This series contains a better platform init for omap MMC as discussed earlier on the linux-omap mailing list. The last patch in the series adds support for a newer omap MMC controller available on 2430 and 34xx omaps.

The first four patches should get merged via Russell. The last one adding the new controller depends on comments from Pierre, so I can separate that out if desired.

The third patch in the series depends on the "Omap3 updates for 2.6.29 merge window" series for the changes to Pandora board support.

Comments welcome as always!