[PATCH] mmci: make sure DMA transfers wait for FIFO drain

[PATCH] mmci: make sure DMA transfers wait for FIFO drain

[Linus Walleij]

The DMA mode also needs to wait until the FIFO is drained before
enabling the MCI_DATAEND IRQ.

Cc: Ulf Hansson <ulf.hansson@stericsson.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c |   31 ++++++++++++++++++++++++++-----
 1 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 800b69c..bac15a3 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -334,6 +334,24 @@ static void mmci_dma_data_error(struct mmci_host *host)
 	dmaengine_terminate_all(host->dma_current);
 }
 
+static void mmci_dma_callback(void *arg)
+{
+	unsigned long flags;
+	struct mmci_host *host = arg;
+
+	dev_vdbg(mmc_dev(host->mmc), "DMA transfer done!\n");
+	spin_lock_irqsave(&host->lock, flags);
+	host->size = 0;
+	mmci_set_mask1(host, 0);
+	/*
+	 * This will make sure the DATAEND IRQ trigger after the DMA is
+	 * finished and not while we're still reading/writing the FIFO
+	 */
+	writel(readl(host->base + MMCIMASK0) | MCI_DATAENDMASK,
+	       host->base + MMCIMASK0);
+	spin_unlock_irqrestore(&host->lock, flags);
+}
+
 static int mmci_dma_start_data(struct mmci_host *host, unsigned int datactrl)
 {
 	struct variant_data *variant = host->variant;
@@ -373,9 +391,12 @@ static int mmci_dma_start_data(struct mmci_host *host, unsigned int datactrl)
 
 	dmaengine_slave_config(chan, &conf);
 	desc = device->device_prep_slave_sg(chan, data->sg, nr_sg,
-					    conf.direction, DMA_CTRL_ACK);
+					    conf.direction,
+					    DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 	if (!desc)
 		goto unmap_exit;
+	desc->callback = mmci_dma_callback;
+	desc->callback_param = host;
 
 	/* Okay, go for it. */
 	host->dma_current = chan;
@@ -393,11 +414,11 @@ static int mmci_dma_start_data(struct mmci_host *host, unsigned int datactrl)
 	writel(datactrl, host->base + MMCIDATACTRL);
 
 	/*
-	 * Let the MMCI say when the data is ended and it's time
-	 * to fire next DMA request. When that happens, MMCI will
-	 * call mmci_data_end()
+	 * Mask off the MCI_DATAEND IRQ until the DMA has completed,
+	 * it will then be enabled by the callback so that the transfer
+	 * can complete through the interrupt handler.
 	 */
-	writel(readl(host->base + MMCIMASK0) | MCI_DATAENDMASK,
+	writel(readl(host->base + MMCIMASK0) & ~MCI_DATAENDMASK,
 	       host->base + MMCIMASK0);
 	return 0;
 
-- 
1.7.3.2

[PATCH] mmci: make sure DMA transfers wait for FIFO drain

[Russell King - ARM Linux]

On Mon, Jan 31, 2011 at 10:39:40PM +0100, Linus Walleij wrote:
> The DMA mode also needs to wait until the FIFO is drained before
> enabling the MCI_DATAEND IRQ.

Hmm.  This will cause attempted DMA usage on ARM MMCI primecells to get
stuck as we'll wait for the never-delivered callback.

With the code I have in place, you'll notice I have:

        /* Wait up to 1ms for the DMA to complete */
        for (i = 0; ; i++) {
                status = readl(host->base + MMCISTATUS);
                if (!(status & MCI_RXDATAAVLBLMASK) || i >= 100)
                        break;
                udelay(10);
        }

This waits for the DMA controller to read the last data out of the FIFO
before allowing the request to complete.  As it has a timeout, it is
able to detect ARMs broken DMA setup on their MMCI/DMAC, and disable DMA
support for this primecell rather than getting stuck.

This may be sufficient without using the DMA callbacks.

[PATCH] mmci: make sure DMA transfers wait for FIFO drain

[Linus Walleij]

2011/2/1 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Mon, Jan 31, 2011 at 10:39:40PM +0100, Linus Walleij wrote:
>> The DMA mode also needs to wait until the FIFO is drained before
>> enabling the MCI_DATAEND IRQ.
>
> Hmm. ?This will cause attempted DMA usage on ARM MMCI primecells to get
> stuck as we'll wait for the never-delivered callback.
>
> With the code I have in place, you'll notice I have:
>
> ? ? ? ?/* Wait up to 1ms for the DMA to complete */
> ? ? ? ?for (i = 0; ; i++) {
> ? ? ? ? ? ? ? ?status = readl(host->base + MMCISTATUS);
> ? ? ? ? ? ? ? ?if (!(status & MCI_RXDATAAVLBLMASK) || i >= 100)
> ? ? ? ? ? ? ? ? ? ? ? ?break;
> ? ? ? ? ? ? ? ?udelay(10);
> ? ? ? ?}
>
> This waits for the DMA controller to read the last data out of the FIFO
> before allowing the request to complete. ?As it has a timeout, it is
> able to detect ARMs broken DMA setup on their MMCI/DMAC, and disable DMA
> support for this primecell rather than getting stuck.
>
> This may be sufficient without using the DMA callbacks.

Seems like so. When I try it with the last patch to use the FIFO
for small xfers it works nicely.

So we'll drop this approach for now.

Linus Walleij

[PATCH] mmci: make sure DMA transfers wait for FIFO drain

[Linus Walleij]

After discussing this with Ulf I think something like this is
still needed...

2011/2/1 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Mon, Jan 31, 2011 at 10:39:40PM +0100, Linus Walleij wrote:
>> The DMA mode also needs to wait until the FIFO is drained before
>> enabling the MCI_DATAEND IRQ.
>
> Hmm. ?This will cause attempted DMA usage on ARM MMCI primecells to get
> stuck as we'll wait for the never-delivered callback.

I don' know if we have semantics for the DMA completion callback,
either:

A) it's to be called whenever the transaction is complete without
 errors - in this case the driver using it must supply some timeout
 mechanism to avoid hanging on DMA or:

B) It's supposed to be called whenever the DMA stops working
 no matter whether this was due to errors or not.

Since the callback isn't supplying any status indication I
assume it's case (A). But if we're allowed to call
.device_tx_status() on the channel in callback context, we can
get the status here and determine whether the transaction was
OK or not, and then it's case (B).

So I should either reconsider this patch to either do:

(A) a timeout that terminates the DMA if too much time passes
   (which will in turn have to be caclulated from the xfer size and
   MCLK frequency) or

(B) a call to device_tx_status() and check whether there was an
  error and set data->error accordingly in the error case.
  (This may in turn require changed to amba-pl08x to e.g. timeout
  and report such errors properly.) or

(C) (Dan) will dma_sync_wait(chan, cookie) on the descriptor
  in the MCI_DATAEND IRQ path suffice? If that happens we
  may have a bug in the DMA engine(s) in that it doesn't
  timeout or error out if a DMA job locks up.
  I don't particularly like dma_sync_wait() since it is hardwired
  to wait 5 seconds, which seems arbitrarily chosen. If that is
  the approach, a new sync wait function taking timeout as
  argument will likely be needed.

My current assumption is something like (A)...

> With the code I have in place, you'll notice I have:
>
> ? ? ? ?/* Wait up to 1ms for the DMA to complete */
> ? ? ? ?for (i = 0; ; i++) {
> ? ? ? ? ? ? ? ?status = readl(host->base + MMCISTATUS);
> ? ? ? ? ? ? ? ?if (!(status & MCI_RXDATAAVLBLMASK) || i >= 100)
> ? ? ? ? ? ? ? ? ? ? ? ?break;
> ? ? ? ? ? ? ? ?udelay(10);
> ? ? ? ?}
>
> This waits for the DMA controller to read the last data out of the FIFO
> before allowing the request to complete. ?As it has a timeout, it is
> able to detect ARMs broken DMA setup on their MMCI/DMAC, and disable DMA
> support for this primecell rather than getting stuck.
>
> This may be sufficient without using the DMA callbacks.

According to our tests this does not really cut it on
Ux500. Sometimes we get the DMA completion before the
MCI_DATAEND IRQ, and sometimes after.

When  we get the callback *after* the MCI_DATAEND irq
this does not work properly, since we still don't know if the DMA
job is complete, so the DMA engine is not in sync and we mess up
the channels by issuing a new job, hammering the DMA engine
while it's still busy on the channel.

We either have to sync in the DMA engine or timeout
on the DMA job as far as I can tell. One of A thru C.

Yours,
Linus Walleij

[PATCH] mmci: make sure DMA transfers wait for FIFO drain

[Russell King - ARM Linux]

On Fri, Feb 11, 2011 at 10:46:46AM +0100, Linus Walleij wrote:
> After discussing this with Ulf I think something like this is
> still needed...
> 
> 2011/2/1 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> > With the code I have in place, you'll notice I have:
> >
> > ? ? ? ?/* Wait up to 1ms for the DMA to complete */
> > ? ? ? ?for (i = 0; ; i++) {
> > ? ? ? ? ? ? ? ?status = readl(host->base + MMCISTATUS);
> > ? ? ? ? ? ? ? ?if (!(status & MCI_RXDATAAVLBLMASK) || i >= 100)
> > ? ? ? ? ? ? ? ? ? ? ? ?break;
> > ? ? ? ? ? ? ? ?udelay(10);
> > ? ? ? ?}
> >
> > This waits for the DMA controller to read the last data out of the FIFO
> > before allowing the request to complete. ?As it has a timeout, it is
> > able to detect ARMs broken DMA setup on their MMCI/DMAC, and disable DMA
> > support for this primecell rather than getting stuck.
> >
> > This may be sufficient without using the DMA callbacks.
> 
> According to our tests this does not really cut it on
> Ux500. Sometimes we get the DMA completion before the
> MCI_DATAEND IRQ, and sometimes after.
> 
> When  we get the callback *after* the MCI_DATAEND irq
> this does not work properly, since we still don't know if the DMA
> job is complete, so the DMA engine is not in sync and we mess up
> the channels by issuing a new job, hammering the DMA engine
> while it's still busy on the channel.

That may be a problem if the channel is still busy, but it in any case
it shouldn't result in the DMA engine being hammered.  The DMA
engine API is a queue based API - you submit requests to it and it
deals with them one after each other.  If you submit two requests in
succession, it should process the first request, complete that, before
it starts to process the second request.  I'd suggest fixing this in
any case.

Maybe a solution to this is to use the DMA callback to signal that the
data path has completed, but still have the data end interrupt in place
so that it can trigger the stop command.  That shouldn't result in
additional delays or even the requirement for a timeout.

[PATCH] mmci: make sure DMA transfers wait for FIFO drain

[Linus Walleij]

2011/2/11 Russell King - ARM Linux <linux@arm.linux.org.uk>:

> Maybe a solution to this is to use the DMA callback to signal that the
> data path has completed, but still have the data end interrupt in place
> so that it can trigger the stop command. ?That shouldn't result in
> additional delays or even the requirement for a timeout.

That should be possible, I'll try to fix up a patch like that.

Thanks,
Linus Walleij