Re: [PATCH] OMAP: MMC: replace infinite loops with timeouts (was Re: [PATCH] OMAP: MMC: recover from transfer failures - Resend)

[Jean Pihet <jpihet@mvista.com>]

[-- Attachment #1: Type: text/plain, Size: 6479 bytes --]

Hi,

Here is the new version of the patch, which now uses cpu_relax in an inline 
function. The new function can be used from an irq handler as well.

Regards,
Jean

From 9d1993dcf4d217837437a1cd9f6e9c1b81533549 Mon Sep 17 00:00:00 2001
From: Jean Pihet <jpihet@mvista.com>
Date: Fri, 6 Feb 2009 16:42:51 +0100
Subject: [PATCH] OMAP: MMC: Change while(); loops with finite version

Replace the infinite 'while() ;' loops
with a finite loop version.

Signed-off-by: Jean Pihet <jpihet@mvista.com>
---
 drivers/mmc/host/omap_hsmmc.c |   54 ++++++++++++++++++++++------------------
 1 files changed, 30 insertions(+), 24 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 1ac6918..aabf28d 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -375,6 +375,32 @@ static void mmc_omap_report_irq(struct mmc_omap_host 
*host, u32 status)
 }
 #endif  /* CONFIG_MMC_DEBUG */

+/*
+ * MMC controller internal state machines reset
+ *
+ * Used to reset command or data internal state machines, using respectively
+ *  SRC or SRD bit of SYSCTL register
+ * Can be called from interrupt context
+ */
+static inline void mmc_omap_reset_controller_fsm(struct mmc_omap_host *host,
+               unsigned long bit)
+{
+       unsigned long i = 0;
+       unsigned long limit = (loops_per_jiffy *
+                               msecs_to_jiffies(MMC_TIMEOUT_MS));
+
+       OMAP_HSMMC_WRITE(host->base, SYSCTL,
+                        OMAP_HSMMC_READ(host->base, SYSCTL) | bit);
+
+       while ((OMAP_HSMMC_READ(host->base, SYSCTL) & bit) &&
+               (i++ < limit))
+               cpu_relax();
+
+       if (OMAP_HSMMC_READ(host->base, SYSCTL) & bit)
+               dev_err(mmc_dev(host->mmc),
+                       "Timeout waiting on controller reset in %s\n",
+                       __func__);
+}

 /*
  * MMC controller IRQ handler
@@ -403,13 +429,7 @@ static irqreturn_t mmc_omap_irq(int irq, void *dev_id)
                        (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)
-                                               ;
-
+                                       mmc_omap_reset_controller_fsm(host, 
SRC);
                                        host->cmd->error = -ETIMEDOUT;
                                } else {
                                        host->cmd->error = -EILSEQ;
@@ -418,12 +438,7 @@ static irqreturn_t mmc_omap_irq(int irq, void *dev_id)
                        }
                        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)
-                                       ;
+                               mmc_omap_reset_controller_fsm(host, SRD);
                        }
                }
                if ((status & DATA_TIMEOUT) ||
@@ -433,12 +448,7 @@ static irqreturn_t mmc_omap_irq(int irq, void *dev_id)
                                        mmc_dma_cleanup(host);
                                else
                                        host->data->error = -EILSEQ;
-                               OMAP_HSMMC_WRITE(host->base, SYSCTL,
-                                       OMAP_HSMMC_READ(host->base,
-                                                       SYSCTL) | SRD);
-                               while (OMAP_HSMMC_READ(host->base,
-                                               SYSCTL) & SRD)
-                                       ;
+                               mmc_omap_reset_controller_fsm(host, SRD);
                                end_trans = 1;
                        }
                }
@@ -529,11 +539,7 @@ static void mmc_omap_detect(struct work_struct *work)
        if (host->carddetect) {
                mmc_detect_change(host->mmc, (HZ * 200) / 1000);
        } else {
-               OMAP_HSMMC_WRITE(host->base, SYSCTL,
-                       OMAP_HSMMC_READ(host->base, SYSCTL) | SRD);
-               while (OMAP_HSMMC_READ(host->base, SYSCTL) & SRD)
-                       ;
-
+               mmc_omap_reset_controller_fsm(host, SRD);
                mmc_detect_change(host->mmc, (HZ * 50) / 1000);
        }
 }
--
1.6.0.1.42.gf8c9c



On Monday 09 February 2009 16:58:14 Adrian Hunter wrote:
> Jean Pihet wrote:
> > Hi,
> >
> >>>> On Thu, 5 Feb 2009, Andrew Morton wrote:
> >>>>> 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) :)
> >
> > 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.


[-- Attachment #2: hsmmc_finite_timeout.patch --]
[-- Type: text/x-diff, Size: 3379 bytes --]

From 9d1993dcf4d217837437a1cd9f6e9c1b81533549 Mon Sep 17 00:00:00 2001
From: Jean Pihet <jpihet@mvista.com>
Date: Fri, 6 Feb 2009 16:42:51 +0100
Subject: [PATCH] OMAP: MMC: Change while(); loops with finite version

Replace the infinite 'while() ;' loops
with a finite loop version.

Signed-off-by: Jean Pihet <jpihet@mvista.com>
---
 drivers/mmc/host/omap_hsmmc.c |   54 ++++++++++++++++++++++------------------
 1 files changed, 30 insertions(+), 24 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 1ac6918..aabf28d 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -375,6 +375,32 @@ static void mmc_omap_report_irq(struct mmc_omap_host *host, u32 status)
 }
 #endif  /* CONFIG_MMC_DEBUG */
 
+/*
+ * MMC controller internal state machines reset
+ *
+ * Used to reset command or data internal state machines, using respectively
+ *  SRC or SRD bit of SYSCTL register
+ * Can be called from interrupt context
+ */
+static inline void mmc_omap_reset_controller_fsm(struct mmc_omap_host *host,
+		unsigned long bit)
+{
+	unsigned long i = 0;
+	unsigned long limit = (loops_per_jiffy *
+				msecs_to_jiffies(MMC_TIMEOUT_MS));
+
+	OMAP_HSMMC_WRITE(host->base, SYSCTL,
+			 OMAP_HSMMC_READ(host->base, SYSCTL) | bit);
+
+	while ((OMAP_HSMMC_READ(host->base, SYSCTL) & bit) &&
+		(i++ < limit))
+		cpu_relax();
+
+	if (OMAP_HSMMC_READ(host->base, SYSCTL) & bit)
+		dev_err(mmc_dev(host->mmc),
+			"Timeout waiting on controller reset in %s\n",
+			__func__);
+}
 
 /*
  * MMC controller IRQ handler
@@ -403,13 +429,7 @@ static irqreturn_t mmc_omap_irq(int irq, void *dev_id)
 			(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)
-						;
-
+					mmc_omap_reset_controller_fsm(host, SRC);
 					host->cmd->error = -ETIMEDOUT;
 				} else {
 					host->cmd->error = -EILSEQ;
@@ -418,12 +438,7 @@ static irqreturn_t mmc_omap_irq(int irq, void *dev_id)
 			}
 			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)
-					;
+				mmc_omap_reset_controller_fsm(host, SRD);
 			}
 		}
 		if ((status & DATA_TIMEOUT) ||
@@ -433,12 +448,7 @@ static irqreturn_t mmc_omap_irq(int irq, void *dev_id)
 					mmc_dma_cleanup(host);
 				else
 					host->data->error = -EILSEQ;
-				OMAP_HSMMC_WRITE(host->base, SYSCTL,
-					OMAP_HSMMC_READ(host->base,
-							SYSCTL) | SRD);
-				while (OMAP_HSMMC_READ(host->base,
-						SYSCTL) & SRD)
-					;
+				mmc_omap_reset_controller_fsm(host, SRD);
 				end_trans = 1;
 			}
 		}
@@ -529,11 +539,7 @@ static void mmc_omap_detect(struct work_struct *work)
 	if (host->carddetect) {
 		mmc_detect_change(host->mmc, (HZ * 200) / 1000);
 	} else {
-		OMAP_HSMMC_WRITE(host->base, SYSCTL,
-			OMAP_HSMMC_READ(host->base, SYSCTL) | SRD);
-		while (OMAP_HSMMC_READ(host->base, SYSCTL) & SRD)
-			;
-
+		mmc_omap_reset_controller_fsm(host, SRD);
 		mmc_detect_change(host->mmc, (HZ * 50) / 1000);
 	}
 }
-- 
1.6.0.1.42.gf8c9c