[PATCH] fix random SD/MMC card recognition failures on ARM Versatile

[PATCH] fix random SD/MMC card recognition failures on ARM Versatile

[Vitaly Wool]

Hello Pierre,

currently sometimes the SD/MMC card inserted results in recognition failure on ARM Versatile board:

   <<<Plug in MMC card>>>

root@versatile:~# mmcblk0: mmc0:0001 SDMB-32 31360KiB
 mmcblk0:<3>mmcblk0: error 3 transferring data
end_request: I/O error, dev mmcblk0, sector 0
Buffer I/O error on device mmcblk0, logical block 0
mmcblk0: error 3 transferring data
end_request: I/O error, dev mmcblk0, sector 0
Buffer I/O error on device mmcblk0, logical block 0
 unable to read partition table

This patch fixes the problem.

 drivers/mmc/mmci.c |    2 ++
 1 file changed, 2 insertions(+)

Signed-off-by: Vitaly Wool <vitalywool@gmail.com>

Index: linux-2.6.18/drivers/mmc/mmci.c
===================================================================
--- linux-2.6.18.orig/drivers/mmc/mmci.c
+++ linux-2.6.18/drivers/mmc/mmci.c
@@ -41,6 +41,8 @@ static void
 mmci_request_end(struct mmci_host *host, struct mmc_request *mrq)
 {
 	writel(0, host->base + MMCICOMMAND);
+	writel(0, host->base + MMCIDATACTRL);
+	writel(0, host->base + MMCIMASK1);
 
 	host->mrq = NULL;
 	host->cmd = NULL;

Re: [PATCH] fix random SD/MMC card recognition failures on ARM Versatile

[Russell King]

On Thu, Nov 23, 2006 at 06:46:06PM +0300, Vitaly Wool wrote:
> Hello Pierre,
> 
> currently sometimes the SD/MMC card inserted results in recognition failure on ARM Versatile board:
> 
>    <<<Plug in MMC card>>>
> 
> root@versatile:~# mmcblk0: mmc0:0001 SDMB-32 31360KiB
>  mmcblk0:<3>mmcblk0: error 3 transferring data
> end_request: I/O error, dev mmcblk0, sector 0
> Buffer I/O error on device mmcblk0, logical block 0
> mmcblk0: error 3 transferring data
> end_request: I/O error, dev mmcblk0, sector 0
> Buffer I/O error on device mmcblk0, logical block 0
>  unable to read partition table
> 
> This patch fixes the problem.

Doubtful.  mmci_stop_data() already does this, which will be called
immediately prior to mmci_request_end().  So you're doubling up the
writes to registers again.

Since this is not the first occurance that you've had to do this with
your board (the other being the SIC) I suggest that your board is
faulty in some way, causing writes to registers to be occasionally
dropped.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH] fix random SD/MMC card recognition failures on ARM Versatile

[Vitaly Wool]

On 11/23/06, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> Doubtful.  mmci_stop_data() already does this, which will be called
> immediately prior to mmci_request_end().  So you're doubling up the
> writes to registers again.

There's the case (mmci_cmd_irq) where mmc_stop_data is not called
prior to mmci_request_end(), so it's not that simple.

> Since this is not the first occurance that you've had to do this with
> your board (the other being the SIC) I suggest that your board is
> faulty in some way, causing writes to registers to be occasionally
> dropped.

I can't 100% guarantee that it's not the case, but the problem has
been reproduced by at least 2 people on 2 different boards and on 2
different sides of the Atlantic. So I'd suspect there's either a SW
problem or a HW revision problem, at least.

Vitaly

Re: [PATCH] fix random SD/MMC card recognition failures on ARM Versatile

[Russell King]

On Thu, Nov 23, 2006 at 10:29:30PM +0300, Vitaly Wool wrote:
> On 11/23/06, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> >Doubtful.  mmci_stop_data() already does this, which will be called
> >immediately prior to mmci_request_end().  So you're doubling up the
> >writes to registers again.
> 
> There's the case (mmci_cmd_irq) where mmc_stop_data is not called
> prior to mmci_request_end(), so it's not that simple.

Ah, I see it.  In that case we need to call mmc_stop_data() when
we're ending the initial command due to an error.  IOW, like this:

diff --git a/drivers/mmc/mmci.c b/drivers/mmc/mmci.c
index 828503c..5ad0259 100644
--- a/drivers/mmc/mmci.c
+++ b/drivers/mmc/mmci.c
@@ -42,6 +42,8 @@ mmci_request_end(struct mmci_host *host,
 {
 	writel(0, host->base + MMCICOMMAND);
 
+	BUG_ON(host->data);
+
 	host->mrq = NULL;
 	host->cmd = NULL;
 
@@ -198,6 +200,8 @@ mmci_cmd_irq(struct mmci_host *host, str
 	}
 
 	if (!cmd->data || cmd->error != MMC_ERR_NONE) {
+		if (host->data)
+			mmci_stop_data(host);
 		mmci_request_end(host, cmd->mrq);
 	} else if (!(cmd->data->flags & MMC_DATA_READ)) {
 		mmci_start_data(host, cmd->data);


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH] fix random SD/MMC card recognition failures on ARM Versatile

[Russell King]

On Thu, Nov 23, 2006 at 07:42:36PM +0000, Russell King wrote:
> On Thu, Nov 23, 2006 at 10:29:30PM +0300, Vitaly Wool wrote:
> > On 11/23/06, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> > >Doubtful.  mmci_stop_data() already does this, which will be called
> > >immediately prior to mmci_request_end().  So you're doubling up the
> > >writes to registers again.
> > 
> > There's the case (mmci_cmd_irq) where mmc_stop_data is not called
> > prior to mmci_request_end(), so it's not that simple.
> 
> Ah, I see it.  In that case we need to call mmc_stop_data() when
> we're ending the initial command due to an error.  IOW, like this:

I'll also add that with the way we handle the MMCI, it is highly likely
that you _will_ see FIFO errors from time to time on this platform.

The problem is that we don't have DMA up and running on this platform,
so we are entirely at the mercy of interrupt-driven PIO.  In addition,
the MMCI FIFOs must be read _before_ they completely fill to avoid
overrun errors.  Coupling these two facts together, it's easy to see
that interrupt latency is _critical_ to avoiding FIFO overruns (error 3).

In general, if you do _anything_ with the board while it's trying to
access MMC cards, you will probably get some FIFO overruns.

There are three solutions:

1. Lower the maximum clock rate that the MMCI will allow, eg:
   insmod mmci fmax=257816

2. Avoid all other system activity while MMC is being accessed.

3. Someone needs to _sanely_ implement DMA on this platform.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH] fix random SD/MMC card recognition failures on ARM Versatile

[Vitaly Wool]

On Thu, 23 Nov 2006 19:42:36 +0000
Russell King <rmk+lkml@arm.linux.org.uk> wrote:

> Ah, I see it.  In that case we need to call mmc_stop_data() when
> we're ending the initial command due to an error.  IOW, like this:
<snip> 

I'd suggest arranging that in a bit different way. It looks like it works better when MMCIDATACTRL/MMCIMASK1 are cleared after MMCICOMMAND (and I think it makes more sense to clear the command register first, thus we have less change to get spurious interrupts after MMCIMASK1 is set).

diff --git a/drivers/mmc/mmci.c b/drivers/mmc/mmci.c
index 828503c..afbb63b 100644
--- a/drivers/mmc/mmci.c
+++ b/drivers/mmc/mmci.c
@@ -37,11 +37,21 @@ #define DBG(host,fmt,args...)	\
 
 static unsigned int fmax = 515633;
 
+static void mmci_stop_data(struct mmci_host *host)
+{
+	writel(0, host->base + MMCIDATACTRL);
+	writel(0, host->base + MMCIMASK1);
+	host->data = NULL;
+}
+
 static void
 mmci_request_end(struct mmci_host *host, struct mmc_request *mrq)
 {
 	writel(0, host->base + MMCICOMMAND);
 
+	if (host->data)
+		mmci_stop_data(host);
+
 	host->mrq = NULL;
 	host->cmd = NULL;
 
@@ -57,13 +67,6 @@ mmci_request_end(struct mmci_host *host,
 	spin_lock(&host->lock);
 }
 
-static void mmci_stop_data(struct mmci_host *host)
-{
-	writel(0, host->base + MMCIDATACTRL);
-	writel(0, host->base + MMCIMASK1);
-	host->data = NULL;
-}
-
 static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
 {
 	unsigned int datactrl, timeout, irqmask;
@@ -168,8 +171,6 @@ mmci_data_irq(struct mmci_host *host, st
 			flush_dcache_page(host->sg_ptr->page);
 	}
 	if (status & MCI_DATAEND) {
-		mmci_stop_data(host);
-
 		if (!data->stop) {
 			mmci_request_end(host, data->mrq);
 		} else {


Vitaly