[PATCH] mmci: sync DATAEND irq with dma transfer done

[PATCH] mmci: sync DATAEND irq with dma transfer done

[Linus Walleij]

From: Ulf Hansson <ulf.hansson@stericsson.com>

The end of a dma job must be synced with a DATAEND irq. This will
prevent the mmci driver from ending the mmc request before the
dma driver is completely done. By using DMA_PREP_INTERRUPT we
register a callback function which will is called when from the
dma driver when the dma transfer is completed.

Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c |  128 +++++++++++++++++++++++++++++++----------------
 drivers/mmc/host/mmci.h |    3 +
 2 files changed, 87 insertions(+), 44 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 4941e06..67e3e4b 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -196,6 +196,53 @@ static void mmci_init_sg(struct mmci_host *host, struct mmc_data *data)
 	sg_miter_start(&host->sg_miter, data->sg, data->sg_len, flags);
 }
 
+static void
+mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c)
+{
+	void __iomem *base = host->base;
+
+	dev_dbg(mmc_dev(host->mmc), "op %02x arg %08x flags %08x\n",
+	    cmd->opcode, cmd->arg, cmd->flags);
+
+	if (readl(base + MMCICOMMAND) & MCI_CPSM_ENABLE) {
+		writel(0, base + MMCICOMMAND);
+		udelay(1);
+	}
+
+	c |= cmd->opcode | MCI_CPSM_ENABLE;
+	if (cmd->flags & MMC_RSP_PRESENT) {
+		if (cmd->flags & MMC_RSP_136)
+			c |= MCI_CPSM_LONGRSP;
+		c |= MCI_CPSM_RESPONSE;
+	}
+	if (/*interrupt*/0)
+		c |= MCI_CPSM_INTERRUPT;
+
+	host->cmd = cmd;
+
+	writel(cmd->arg, base + MMCIARGUMENT);
+	writel(c, base + MMCICOMMAND);
+}
+
+static void mmci_complete_data_xfer(struct mmci_host *host)
+{
+	struct mmc_data *data = host->data;
+
+	if ((host->size == 0) || data->error) {
+
+		mmci_stop_data(host);
+
+		if (!data->error)
+			data->bytes_xfered = data->blksz * data->blocks;
+
+		if (!data->stop) {
+			mmci_request_end(host, data->mrq);
+		} else {
+			mmci_start_command(host, data->stop, 0);
+		}
+	}
+}
+
 /*
  * All the DMA operation mode stuff goes inside this ifdef.
  * This assumes that you have a generic DMA device interface,
@@ -334,6 +381,28 @@ static void mmci_dma_unmap(struct mmci_host *host, struct mmc_data *data)
 	}
 }
 
+static void mmci_dma_callback(void *arg)
+{
+	unsigned long flags;
+	struct mmci_host *host = arg;
+
+	spin_lock_irqsave(&host->lock, flags);
+
+	mmci_dma_unmap(host, host->data);
+
+	/* Mark that the entire data is transferred for this dma session. */
+	host->size = 0;
+
+	/*
+	 * Make sure we have received a MCI_DATAEND irq before
+	 * completing the data transfer.
+	 */
+	if (host->dataend)
+		mmci_complete_data_xfer(host);
+
+	spin_unlock_irqrestore(&host->lock, flags);
+}
+
 static void mmci_dma_data_error(struct mmci_host *host)
 {
 	dev_err(mmc_dev(host->mmc), "error during DMA transfer!\n");
@@ -382,10 +451,15 @@ 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_CTRL_ACK | DMA_PREP_INTERRUPT);
 	if (!desc)
 		goto unmap_exit;
 
+	/* Setup dma callback function. */
+	desc->callback = mmci_dma_callback;
+	desc->callback_param = host;
+
 	/* Okay, go for it. */
 	host->dma_current = chan;
 
@@ -451,6 +525,7 @@ static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
 
 	host->data = data;
 	host->size = data->blksz * data->blocks;
+	host->dataend = false;
 	data->bytes_xfered = 0;
 
 	clks = (unsigned long long)data->timeout_ns * host->cclk;
@@ -509,34 +584,6 @@ static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
 }
 
 static void
-mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c)
-{
-	void __iomem *base = host->base;
-
-	dev_dbg(mmc_dev(host->mmc), "op %02x arg %08x flags %08x\n",
-	    cmd->opcode, cmd->arg, cmd->flags);
-
-	if (readl(base + MMCICOMMAND) & MCI_CPSM_ENABLE) {
-		writel(0, base + MMCICOMMAND);
-		udelay(1);
-	}
-
-	c |= cmd->opcode | MCI_CPSM_ENABLE;
-	if (cmd->flags & MMC_RSP_PRESENT) {
-		if (cmd->flags & MMC_RSP_136)
-			c |= MCI_CPSM_LONGRSP;
-		c |= MCI_CPSM_RESPONSE;
-	}
-	if (/*interrupt*/0)
-		c |= MCI_CPSM_INTERRUPT;
-
-	host->cmd = cmd;
-
-	writel(cmd->arg, base + MMCIARGUMENT);
-	writel(c, base + MMCICOMMAND);
-}
-
-static void
 mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
 	      unsigned int status)
 {
@@ -581,21 +628,11 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
 	if (status & MCI_DATABLOCKEND)
 		dev_err(mmc_dev(host->mmc), "stray MCI_DATABLOCKEND interrupt\n");
 
-	if (status & MCI_DATAEND || data->error) {
-		if (dma_inprogress(host))
-			mmci_dma_unmap(host, data);
-		mmci_stop_data(host);
-
-		if (!data->error)
-			/* The error clause is handled above, success! */
-			data->bytes_xfered = data->blksz * data->blocks;
+	if (status & MCI_DATAEND)
+		host->dataend = true;
 
-		if (!data->stop) {
-			mmci_request_end(host, data->mrq);
-		} else {
-			mmci_start_command(host, data->stop, 0);
-		}
-	}
+	if (host->dataend || data->error)
+		mmci_complete_data_xfer(host);
 }
 
 static void
@@ -618,8 +655,11 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
 	}
 
 	if (!cmd->data || cmd->error) {
-		if (host->data)
+		if (host->data) {
+			if (dma_inprogress(host))
+				mmci_dma_data_error(host);
 			mmci_stop_data(host);
+		}
 		mmci_request_end(host, cmd->mrq);
 	} else if (!(cmd->data->flags & MMC_DATA_READ)) {
 		mmci_start_data(host, cmd->data);
diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
index bb32e21..3f98d9d 100644
--- a/drivers/mmc/host/mmci.h
+++ b/drivers/mmc/host/mmci.h
@@ -195,6 +195,9 @@ struct mmci_host {
 	unsigned int		size;
 	struct regulator	*vcc;
 
+	/* sync of DATAEND irq */
+	bool			dataend;
+
 #ifdef CONFIG_DMA_ENGINE
 	/* DMA stuff */
 	struct dma_chan		*dma_current;
-- 
1.7.3.2

[PATCH] mmci: sync DATAEND irq with dma transfer done

[Russell King - ARM Linux]

On Tue, Apr 19, 2011 at 11:02:34AM +0200, Linus Walleij wrote:
> From: Ulf Hansson <ulf.hansson@stericsson.com>
> 
> The end of a dma job must be synced with a DATAEND irq. This will
> prevent the mmci driver from ending the mmc request before the
> dma driver is completely done. By using DMA_PREP_INTERRUPT we
> register a callback function which will is called when from the
> dma driver when the dma transfer is completed.

Why is what we currently do not sufficient?

This will break the detection of dead DMA controllers like on the ARM
reference platforms, as I've said before.

[PATCH] mmci: sync DATAEND irq with dma transfer done

[Linus Walleij]

On Tue, Apr 19, 2011 at 11:20 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Apr 19, 2011 at 11:02:34AM +0200, Linus Walleij wrote:
>> From: Ulf Hansson <ulf.hansson@stericsson.com>
>>
>> The end of a dma job must be synced with a DATAEND irq. This will
>> prevent the mmci driver from ending the mmc request before the
>> dma driver is completely done. By using DMA_PREP_INTERRUPT we
>> register a callback function which will is called when from the
>> dma driver when the dma transfer is completed.
>
> Why is what we currently do not sufficient?

On a high-speeded ux500 the DATAEND IRQ will assert before the
DMA data is actually finished, thus if we start hammering in the next
request we break an ongoing transfer. :-(

> This will break the detection of dead DMA controllers like on the ARM
> reference platforms, as I've said before.

Yeah ... I'm trying to think of something clever, can we put a
bool sync_dma_job in the vendor data and only activate this
for ux500 and U300 ST variants where we know DMA always
works then?

The only thing I worry about is if the plain vanilla PL180 would
exhibit the same behaviour when used with high-speed DMA, then
we're effectively deactivating a bugfix for it.

Linus

[PATCH] mmci: sync DATAEND irq with dma transfer done

[Russell King - ARM Linux]

On Tue, Apr 19, 2011 at 02:00:17PM +0200, Linus Walleij wrote:
> On Tue, Apr 19, 2011 at 11:20 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Tue, Apr 19, 2011 at 11:02:34AM +0200, Linus Walleij wrote:
> >> From: Ulf Hansson <ulf.hansson@stericsson.com>
> >>
> >> The end of a dma job must be synced with a DATAEND irq. This will
> >> prevent the mmci driver from ending the mmc request before the
> >> dma driver is completely done. By using DMA_PREP_INTERRUPT we
> >> register a callback function which will is called when from the
> >> dma driver when the dma transfer is completed.
> >
> > Why is what we currently do not sufficient?
> 
> On a high-speeded ux500 the DATAEND IRQ will assert before the
> DMA data is actually finished, thus if we start hammering in the next
> request we break an ongoing transfer. :-(

Yes, you've already said that in the past.  And this is partly why we
have this code in the dma unmap:

        /* 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);
        }

So, we wait until the DMA has drained the FIFO before we fire off the
next request - or even unmap the DMA buffer.  Should the DMA fail to
drain the FIFO in a reasonable time, we timeout and disable DMA.

Again, I ask, why is this not sufficient to cover the case where the
data end IRQ occurs before the DMA engine has completed the transfer -
which is likely to take a very short time indeed.

[PATCH] mmci: sync DATAEND irq with dma transfer done

[Linus Walleij]

On Tue, Apr 19, 2011 at 2:03 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Apr 19, 2011 at 02:00:17PM +0200, Linus Walleij wrote:
>>
>> On a high-speeded ux500 the DATAEND IRQ will assert before the
>> DMA data is actually finished, thus if we start hammering in the next
>> request we break an ongoing transfer. :-(
>
> Yes, you've already said that in the past. ?And this is partly why we
> have this code in the dma unmap:
>
> ? ? ? ?/* 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);
> ? ? ? ?}
>
> So, we wait until the DMA has drained the FIFO before we fire off the
> next request - or even unmap the DMA buffer. ?Should the DMA fail to
> drain the FIFO in a reasonable time, we timeout and disable DMA.
>
> Again, I ask, why is this not sufficient to cover the case where the
> data end IRQ occurs before the DMA engine has completed the transfer -
> which is likely to take a very short time indeed.

It doesn't help, we have really tested this and at high speed transfers
(especially if we use Per Fridens speed-up patches) apparently
the flag RXDATAAVLBL goes to zero before the block is really
finished.

My rough guess (after looking at the VHDL code) is that
RXDATAVLBL flag goes low when the FIFO is empty, but that
doesn't mean that the DMA handshake logic is out of its send/recieve
state and thus we screw it up if we hammer in another transfer before
it has had time to deassert the single/burst request signals and go to
idle state. This can only be seen by the side effect of the DMA
transfer actually terminating, and the DMA engine calling its
callback.

Our version is not that far away from the pure ARM versions
so I think this problem is actually present in an unmodified
PL180 as well.

Yours,
Linus Walleij

[PATCH] mmci: sync DATAEND irq with dma transfer done

[Vitaly Wool]

Hi Linus,

> My rough guess (after looking at the VHDL code) is that
> RXDATAVLBL flag goes low when the FIFO is empty, but that
> doesn't mean that the DMA handshake logic is out of its send/recieve
> state and thus we screw it up if we hammer in another transfer before
> it has had time to deassert the single/burst request signals and go to
> idle state. This can only be seen by the side effect of the DMA
> transfer actually terminating, and the DMA engine calling its
> callback.

[PATCH] mmci: sync DATAEND irq with dma transfer done

[Linus Walleij]

On Wed, Apr 20, 2011 at 6:46 PM, Vitaly Wool <vitalywool@gmail.com> wrote:
>>
>> My rough guess (after looking at the VHDL code) is that
>> RXDATAVLBL flag goes low when the FIFO is empty, but that
>> doesn't mean that the DMA handshake logic is out of its send/recieve
>> state and thus we screw it up if we hammer in another transfer before
>> it has had time to deassert the single/burst request signals and go to
>> idle state. This can only be seen by the side effect of the DMA
>> transfer actually terminating, and the DMA engine calling its
>> callback.
>
> From what you are saying, RXDATAAVLBL seems to be equal to
> !RXFIFOEMPTY in your case.

Yes that is exactly what the VHDL says:

RXDATAAVL <= not(FifoEmpty) [roughly]

> Did you try to change the former to the
> latter to see how it works?

It's equivalent, neither FIFO watermark is connected to the
state of the DMA request signals...

What we need is a DMAXFERCOMPLETE flag so we know that
the DMA state machine is really finished. Sadly the hardware lacks
such a status flag, so there is no way to know that the DMA state
machine is finished by looking at any status registers in the
MMCI block.

We are thus reliant on the other end of the bitpipe telling us
it's finished.

Yours,
Linus Walleij

[PATCH] mmci: sync DATAEND irq with dma transfer done

[Vitaly Wool]

On Wed, Apr 20, 2011 at 7:17 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> What we need is a DMAXFERCOMPLETE flag so we know that
> the DMA state machine is really finished. Sadly the hardware lacks
> such a status flag, so there is no way to know that the DMA state
> machine is finished by looking at any status registers in the
> MMCI block.
>
> We are thus reliant on the other end of the bitpipe telling us
> it's finished.

Okay, but can't we have something like this then in the unmap function:

       for (i = 0; i < 100; i++) {
               status = readl(host->base + MMCISTATUS);
               if (host->unreliable_rxflags) {
                       if (host->dataend)
                              break;
               } else if (!(status & MCI_RXDATAAVLBLMASK))
                       break;
               udelay(10);
       }

to minimize the changes and apparently address Russell's concern as well?

Thanks,
   Vitaly

[PATCH] mmci: sync DATAEND irq with dma transfer done

[Linus Walleij]

On Wed, Apr 20, 2011 at 7:58 PM, Vitaly Wool <vitalywool@gmail.com> wrote:

> Okay, but can't we have something like this then in the unmap function:
>
> ? ? ? for (i = 0; i < 100; i++) {
> ? ? ? ? ? ? ? status = readl(host->base + MMCISTATUS);
> ? ? ? ? ? ? ? if (host->unreliable_rxflags) {
> ? ? ? ? ? ? ? ? ? ? ? if (host->dataend)
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?break;
> ? ? ? ? ? ? ? } else if (!(status & MCI_RXDATAAVLBLMASK))
> ? ? ? ? ? ? ? ? ? ? ? break;
> ? ? ? ? ? ? ? udelay(10);
> ? ? ? }
>
> to minimize the changes and apparently address Russell's concern as well?

Yes that was exactly my suggestion in my first reply to Russell,
sorry if that wasn't clear. I called it bool sync_dma_job

The problem is that this HW bug is very likely present also in
unmodified ARM IP blocks, the only reason noone is seing it
is that they don't clock them very high and DMA is not really
working on them for other reasons anyway.

I.e. all versions of MMCI are likely as unreliable...

Yours,
Linus Walleij

[PATCH] mmci: sync DATAEND irq with dma transfer done

[Russell King - ARM Linux]

On Wed, Apr 20, 2011 at 06:29:40PM +0200, Linus Walleij wrote:
> On Tue, Apr 19, 2011 at 2:03 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Tue, Apr 19, 2011 at 02:00:17PM +0200, Linus Walleij wrote:
> >>
> >> On a high-speeded ux500 the DATAEND IRQ will assert before the
> >> DMA data is actually finished, thus if we start hammering in the next
> >> request we break an ongoing transfer. :-(
> >
> > Yes, you've already said that in the past. ?And this is partly why we
> > have this code in the dma unmap:
> >
> > ? ? ? ?/* 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);
> > ? ? ? ?}
> >
> > So, we wait until the DMA has drained the FIFO before we fire off the
> > next request - or even unmap the DMA buffer. ?Should the DMA fail to
> > drain the FIFO in a reasonable time, we timeout and disable DMA.
> >
> > Again, I ask, why is this not sufficient to cover the case where the
> > data end IRQ occurs before the DMA engine has completed the transfer -
> > which is likely to take a very short time indeed.
> 
> It doesn't help, we have really tested this and at high speed transfers
> (especially if we use Per Fridens speed-up patches) apparently
> the flag RXDATAAVLBL goes to zero before the block is really
> finished.
> 
> My rough guess (after looking at the VHDL code) is that
> RXDATAVLBL flag goes low when the FIFO is empty, but that
> doesn't mean that the DMA handshake logic is out of its send/recieve
> state and thus we screw it up if we hammer in another transfer before
> it has had time to deassert the single/burst request signals and go to
> idle state. This can only be seen by the side effect of the DMA
> transfer actually terminating, and the DMA engine calling its
> callback.

That's rather unfortunate, because it means that trying it on ARM
hardware is going to hang indefinitely waiting for the nonexistent
DMA stuff to finish.

I remain unconvinced whether this problem applies only to ARMs
evaluation boards as I believe the whole primecell DMA stuff from
the outset is fundamentally misdesigned.  I suspect there maybe SoCs
out there which suffer from the same broken DMA issues which ARMs
eval boards do.

Maybe an alternative solution is on data end to set a timer, which
is cancelled when the DMA engine callback arrives.  If the timer
expires, it means we have broken DMA and that needs to be shutdown
for that instance.

However, one thing worries me - what if the DMA callback comes before
we get the data end interrupt.  Given the weirdnesses of your
implementation found so far (which are well beyond what's visible
on ARMs own implementation) I wouldn't put any guarantees on the
relative ordering of that either.

[PATCH] mmci: sync DATAEND irq with dma transfer done

[Martin Furmanski]

On Thu, Apr 28, 2011 at 7:03 PM, Russell King - ARM Linux <
linux@arm.linux.org.uk> wrote:

> On Wed, Apr 20, 2011 at 06:29:40PM +0200, Linus Walleij wrote:
> > On Tue, Apr 19, 2011 at 2:03 PM, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> > > On Tue, Apr 19, 2011 at 02:00:17PM +0200, Linus Walleij wrote:
> > >>
> > >> On a high-speeded ux500 the DATAEND IRQ will assert before the
> > >> DMA data is actually finished, thus if we start hammering in the next
> > >> request we break an ongoing transfer. :-(
> > >
> > > Yes, you've already said that in the past.  And this is partly why we
> > > have this code in the dma unmap:
> > >
> > >        /* 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);
> > >        }
> > >
> > > So, we wait until the DMA has drained the FIFO before we fire off the
> > > next request - or even unmap the DMA buffer.  Should the DMA fail to
> > > drain the FIFO in a reasonable time, we timeout and disable DMA.
> > >
> > > Again, I ask, why is this not sufficient to cover the case where the
> > > data end IRQ occurs before the DMA engine has completed the transfer -
> > > which is likely to take a very short time indeed.
> >
> > It doesn't help, we have really tested this and at high speed transfers
> > (especially if we use Per Fridens speed-up patches) apparently
> > the flag RXDATAAVLBL goes to zero before the block is really
> > finished.
> >
> > My rough guess (after looking at the VHDL code) is that
> > RXDATAVLBL flag goes low when the FIFO is empty, but that
> > doesn't mean that the DMA handshake logic is out of its send/recieve
> > state and thus we screw it up if we hammer in another transfer before
> > it has had time to deassert the single/burst request signals and go to
> > idle state. This can only be seen by the side effect of the DMA
> > transfer actually terminating, and the DMA engine calling its
> > callback.
>
>
Even this would be no guarantee. The callback is hopefully a guarantee
that the clr has been asserted, but for the peripheral's state machine
there is still an uncertain time left, so the callback is a guess as good as
any other, although the reference point has been shifted forwards. Since the
time is uncertain, might just add a suitable delay. Preferably based on VHDL
analysis and as a function of peripheral clock.

At the very least the last data element should be out of the FIFO when
RxDataAvlbl
deasserts and a read of status should not arrive any sooner than that
element
to the DMAC. If this is true, then the read of status itself implies that
the DMAC
has asserted clr (or in some very pessimistic scenario within a few
cycles?),
and the delay can be chosen fixed from this point in time. Since udelays are
already tolerated, I don't see much more ugliness added by this.


> That's rather unfortunate, because it means that trying it on ARM
> hardware is going to hang indefinitely waiting for the nonexistent
> DMA stuff to finish.
>
> I remain unconvinced whether this problem applies only to ARMs
> evaluation boards as I believe the whole primecell DMA stuff from
> the outset is fundamentally misdesigned.  I suspect there maybe SoCs
> out there which suffer from the same broken DMA issues which ARMs
> eval boards do.
>
> Maybe an alternative solution is on data end to set a timer, which
> is cancelled when the DMA engine callback arrives.  If the timer
> expires, it means we have broken DMA and that needs to be shutdown
> for that instance.
>
> However, one thing worries me - what if the DMA callback comes before
> we get the data end interrupt.  Given the weirdnesses of your
> implementation found so far (which are well beyond what's visible
> on ARMs own implementation) I wouldn't put any guarantees on the
> relative ordering of that either.
>
>
So this would build on the old solution, and won't break the broken ones.


> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110429/0de4a203/attachment-0001.html>

[PATCH] mmci: sync DATAEND irq with dma transfer done

[Ulf Hansson]

> 
> That's rather unfortunate, because it means that trying it on ARM
> hardware is going to hang indefinitely waiting for the nonexistent
> DMA stuff to finish.

I see the problem, we need a way of being able to switch between using 
the dma callback and not using it. I think the variant data should be 
used for this, what do you think?

> 
> I remain unconvinced whether this problem applies only to ARMs
> evaluation boards as I believe the whole primecell DMA stuff from
> the outset is fundamentally misdesigned.  I suspect there maybe SoCs
> out there which suffer from the same broken DMA issues which ARMs
> eval boards do.
> 
> Maybe an alternative solution is on data end to set a timer, which
> is cancelled when the DMA engine callback arrives.  If the timer
> expires, it means we have broken DMA and that needs to be shutdown
> for that instance.

This could be a very good alternative for error handling of the DMA job. 
I will try to add some code that handles this.

> 
> However, one thing worries me - what if the DMA callback comes before
> we get the data end interrupt.  Given the weirdnesses of your
> implementation found so far (which are well beyond what's visible
> on ARMs own implementation) I wouldn't put any guarantees on the
> relative ordering of that either.
> 

host->dataend and host->size==0 controls whether the data transfer has 
finished successfully. I believe this should be handled correctly in my 
patch. Maybe it is possible to make some minor restructuring to make it 
more clear what the end condition really is, I can see if I can figure 
something out.


BR
Ulf Hansson

[PATCH] mmci: sync DATAEND irq with dma transfer done

[Russell King - ARM Linux]

On Fri, Apr 29, 2011 at 02:44:59PM +0200, Ulf Hansson wrote:
>>
>> That's rather unfortunate, because it means that trying it on ARM
>> hardware is going to hang indefinitely waiting for the nonexistent
>> DMA stuff to finish.
>
> I see the problem, we need a way of being able to switch between using  
> the dma callback and not using it. I think the variant data should be  
> used for this, what do you think?

How can we do that when it actually depends on how the primecell is wired
and the characteristics of the DMA controller to which it is connected?

> host->dataend and host->size==0 controls whether the data transfer has  
> finished successfully. I believe this should be handled correctly in my  
> patch. Maybe it is possible to make some minor restructuring to make it  
> more clear what the end condition really is, I can see if I can figure  
> something out.

host->size never goes to zero when DMA is in progress, as it doesn't
track the progress of the transfer.  That's only tracked with PIO,
and the remainder is calculated on error from the primecells counters.

[PATCH] mmci: sync DATAEND irq with dma transfer done

[Ulf Hansson]

Russell King - ARM Linux wrote:
> On Fri, Apr 29, 2011 at 02:44:59PM +0200, Ulf Hansson wrote:
>>> That's rather unfortunate, because it means that trying it on ARM
>>> hardware is going to hang indefinitely waiting for the nonexistent
>>> DMA stuff to finish.
>> I see the problem, we need a way of being able to switch between using  
>> the dma callback and not using it. I think the variant data should be  
>> used for this, what do you think?
> 
> How can we do that when it actually depends on how the primecell is wired
> and the characteristics of the DMA controller to which it is connected?

OK, lets add a new member in the mmci amba platform struct then. That 
should do it!?

> 
>> host->dataend and host->size==0 controls whether the data transfer has  
>> finished successfully. I believe this should be handled correctly in my  
>> patch. Maybe it is possible to make some minor restructuring to make it  
>> more clear what the end condition really is, I can see if I can figure  
>> something out.
> 
> host->size never goes to zero when DMA is in progress, as it doesn't
> track the progress of the transfer.  That's only tracked with PIO,
> and the remainder is calculated on error from the primecells counters.
> 

The error handling is almost the same for PIO and DMA. If we get 
MCI_DATACRCFAIL | MCI_DATATIMEOUT | MCI_TXUNDERRUN | MCI_RXOVERRUN, then 
we use the DATACNT register to find out the remainder of data, done in 
mmci_data_irq function. There is no difference between DMA and PIO here.

But, if we are "hanging" for a dma_callback to be called to fully 
complete the data transfer there is no error handling. You proposed to 
use a timer, which should be set when DATAEND occurs and if we still are 
waiting for the dma callback. I think this is good approach!

If that timer runs out, we could potentially read the DATACNT register 
to find out the remainder of data as we do when we handle 
MCI_DATACRCFAIL | MCI_DATATIMEOUT | MCI_TXUNDERRUN | MCI_RXOVERRUN 
errors. We must also take actions to terminate the dma job.

[PATCH] mmci: sync DATAEND irq with dma transfer done

[Linus Walleij]

2011/4/29 Martin Furmanski <martin.furmanski@gmail.com>:

[About waiting for the DMA engine to complete]

> Even this would be no guarantee. The callback is hopefully a guarantee
> that the clr has been asserted, but for the peripheral's state machine
> there is still an uncertain time left, so the callback is a guess as good as
> any other, although the reference point has been shifted forwards. Since the
> time is uncertain, might just add a suitable delay. Preferably based on VHDL
> analysis and as a function of peripheral clock.

The proper thing is to get the VHDL up in the testbench (synopsys)
analyze it by running it and measure the number of cycles until idle.
Then we can calculate the necessary delay from
udelay(DIV_ROUND_UP(clockrate * cycles, 1000000));

Now I need to figure out if I can pull that off :-/

> At the very least the last data element should be out of the FIFO when
> RxDataAvlbl deasserts

Yes, but which FIFO?

> and a read of status should not arrive any
> sooner than that element to the DMAC.

As far as I can see there is no guarantee for that in the PL180 controller.
IIRC the FIFO toward the card is a separate state machine, and it's the
status of that state machine you're reading out in RxDataAvlbl.

It hands the read tokens to the MMCI DMA state machine with a simple
handshake, and from that point on the position of the data in the system
is the matter of the DMA statemachine, which in turn talks to the DMA
controller. The state of the DMA statemachine (like when it goes idle) is
not visible on the outside of the MMCI block in any flag register or so.

So when the MMCI DMA state machine takes over the last tokens,
the DMA engine can for example stop handshaking for an indefinate
amount of time, while there are still tokens in the DMA part of MMCI.
And in that time the driver may choose to initialize the MMCI for the
next transfer, which screws everything up.

So getting the callback from the DMA engine is what gives us the
certainty that CLR has been asserted.

Actually you probably need both: wait for the DMA ACK *and*
a few cycles more determined by analysis for the state machine
to settle, to be absolutely sure.

But waiting for the DMA engine to complete the transfer is necessary
as far as I can tell. Then you know there are no tokens stuck
anywhere in the pipeline...

> If this is true, then the read
> of status itself implies that the DMAC has asserted clr (or in some
> very pessimistic scenario within a few cycles?),

As said, the status comes from the FIFO toward the card, the
token queue in the DMA part of MMCI is something else.

Or am I getting things backwards? If you like we can have a
look at the hardware code together sometime and see if we can
figure it out...

Thanks,
Linus Walleij

[PATCH] mmci: sync DATAEND irq with dma transfer done

[Per Forlin]

On Wed, Apr 20, 2011 at 6:29 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Apr 19, 2011 at 2:03 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Tue, Apr 19, 2011 at 02:00:17PM +0200, Linus Walleij wrote:
>>>
>>> On a high-speeded ux500 the DATAEND IRQ will assert before the
>>> DMA data is actually finished, thus if we start hammering in the next
>>> request we break an ongoing transfer. :-(
>>
>> Yes, you've already said that in the past. ?And this is partly why we
>> have this code in the dma unmap:
>>
>> ? ? ? ?/* 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);
>> ? ? ? ?}
>>
>> So, we wait until the DMA has drained the FIFO before we fire off the
>> next request - or even unmap the DMA buffer. ?Should the DMA fail to
>> drain the FIFO in a reasonable time, we timeout and disable DMA.
>>
>> Again, I ask, why is this not sufficient to cover the case where the
>> data end IRQ occurs before the DMA engine has completed the transfer -
>> which is likely to take a very short time indeed.
>
> It doesn't help, we have really tested this and at high speed transfers
> (especially if we use Per Fridens speed-up patches) apparently
> the flag RXDATAAVLBL goes to zero before the block is really
> finished.
>
I haven't really done any specific testing on this. Ulf has reproduced
it without my "speed-up" patches an old version of mmci before the DMA
was in mainline, not sure if it's done on the current mmci. I run data
test (dt) for hours without running into this. But my main concern is
more about verifying the changes I make in the mmc framework by
running cross platform tests (u8500, u5500, pandaboard ...). My focus
hasn't been about testing potential race conditions in the mmci driver
on ux500. I am willingly to test this dma race condition if there are
any doubts whether it still exists or not.

> Yours,
> Linus Walleij
Regards,
Per

[PATCH] mmci: sync DATAEND irq with dma transfer done

[Linus Walleij]

2011/5/12 Per Forlin <per.forlin@linaro.org>:

> I am willingly to test this dma race condition if there are
> any doubts whether it still exists or not.

That'd be super, can you test on U8500 and U5500 alike and report
results, but if it's gone I'd very much like to know why, since in
theory the problem is still there (provided I'm not completely
misunderstanding the hardware, which is quite possible).

Thanks,
Linus Walleij