diff mbox

OMAP: MMC: recover from transfer failures - Resend

Message ID 200902031505.58697.jpihet@mvista.com
State Awaiting Upstream, archived
Headers show

Commit Message

Jean Pihet Feb. 3, 2009, 2:05 p.m. UTC
Hi,

This is a re-send of the 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.

Regards,
Jean

From d143f6b2e705aa4e9d2b032097fd1c82f8163262 Mon Sep 17 00:00:00 2001
From: Jean Pihet <jpihet@mvista.com>
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 <alowe@mvista.com>
Signed-off-by: Jean Pihet <jpihet@mvista.com>
Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
---
 drivers/mmc/host/omap_hsmmc.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

--
1.5.4.4.21.gc4a6c


On Monday 02 February 2009 20:05:06 Tony Lindgren wrote:
> Hi,
>
> * Jean Pihet <jpihet@mvista.com> [090202 00:46]:
> > 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

Comments

Andrew Morton Feb. 5, 2009, 8:10 p.m. UTC | #1
On Tue, 3 Feb 2009 15:05:58 +0100
Jean Pihet <jpihet@mvista.com> wrote:

> +	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..

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Walmsley Feb. 5, 2009, 8:32 p.m. UTC | #2
On Thu, 5 Feb 2009, Andrew Morton wrote:

> On Tue, 3 Feb 2009 15:05:58 +0100
> Jean Pihet <jpihet@mvista.com> wrote:
> 
> > +	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.

> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jean Pihet Feb. 6, 2009, 1:22 p.m. UTC | #3
On Thursday 05 February 2009 21:32:03 Paul Walmsley wrote:
> On Thu, 5 Feb 2009, Andrew Morton wrote:
> > On Tue, 3 Feb 2009 15:05:58 +0100
> >
> > Jean Pihet <jpihet@mvista.com> wrote:
> > > +	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.
Ok.
>
> > 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.

>
> - Paul

Regards,
Jean
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pierre Ossman Feb. 6, 2009, 1:53 p.m. UTC | #4
On Fri, 6 Feb 2009 14:22:32 +0100
Jean Pihet <jpihet@mvista.com> wrote:

> On Thursday 05 February 2009 21:32:03 Paul Walmsley wrote:
> > 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) :)


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


Rgds
David Brownell Feb. 8, 2009, 8:27 p.m. UTC | #5
On Thursday 05 February 2009, Paul Walmsley wrote:
> 
> > > +   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.

It's still good policy to have a cpu_relax() in
such loops.  Not that it'll do much on most ARMs,
but empty statements are in general worth avoiding.


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarkko Lavinen Feb. 9, 2009, 5:58 p.m. UTC | #6
Pierre Ossman wrote:
> 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.

Cheers
Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren Feb. 9, 2009, 6:46 p.m. UTC | #7
* Jarkko Lavinen <jarkko.lavinen@nokia.com> [090209 10:06]:
> Pierre Ossman wrote:
> > 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.

Yeah it would be great if Jarkkko can queue up all the omap MMC
patches from now on.

> 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.

And then all your PM patches for the next merge window..

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Walmsley Feb. 10, 2009, 12:09 a.m. UTC | #8
On Fri, 6 Feb 2009, Jean Pihet wrote:

> 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.

Just as an aside, from the standpoint of minimizing CPU usage, there seems 
little point in using udelay() here, since it is implemented as a CPU 
busy-wait.


- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

From d143f6b2e705aa4e9d2b032097fd1c82f8163262 Mon Sep 17 00:00:00 2001
From: Jean Pihet <jpihet@mvista.com>
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 <alowe@mvista.com>
Signed-off-by: Jean Pihet <jpihet@mvista.com>
Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
---
 drivers/mmc/host/omap_hsmmc.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index d5c1e9d..97150c0 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -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