[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Rafal Prylowski]

Implement double buffering for M2M DMA channels.

Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>

Index: slave-dma/drivers/dma/ep93xx_dma.c
===================================================================
--- slave-dma.orig/drivers/dma/ep93xx_dma.c
+++ slave-dma/drivers/dma/ep93xx_dma.c
@@ -69,6 +69,7 @@
 #define M2M_CONTROL_TM_SHIFT		13
 #define M2M_CONTROL_TM_TX		(1 << M2M_CONTROL_TM_SHIFT)
 #define M2M_CONTROL_TM_RX		(2 << M2M_CONTROL_TM_SHIFT)
+#define M2M_CONTROL_NFBINT		BIT(21)
 #define M2M_CONTROL_RSS_SHIFT		22
 #define M2M_CONTROL_RSS_SSPRX		(1 << M2M_CONTROL_RSS_SHIFT)
 #define M2M_CONTROL_RSS_SSPTX		(2 << M2M_CONTROL_RSS_SHIFT)
@@ -77,7 +78,23 @@
 #define M2M_CONTROL_PWSC_SHIFT		25
 
 #define M2M_INTERRUPT			0x0004
-#define M2M_INTERRUPT_DONEINT		BIT(1)
+#define M2M_INTERRUPT_MASK		6
+
+#define M2M_STATUS			0x000c
+#define M2M_STATUS_CTL_SHIFT		1
+#define M2M_STATUS_CTL_IDLE		(0 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_STALL		(1 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_MEMRD		(2 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_MEMWR		(3 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_BWCWAIT		(4 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_MASK		(7 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_BUF_SHIFT		4
+#define M2M_STATUS_BUF_NO		(0 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_BUF_ON		(1 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_BUF_NEXT		(2 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_BUF_MASK		(3 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_DONE			BIT(6)
+
 
 #define M2M_BCR0			0x0010
 #define M2M_BCR1			0x0014
@@ -426,15 +443,6 @@ static int m2p_hw_interrupt(struct ep93x
 
 /*
  * M2M DMA implementation
- *
- * For the M2M transfers we don't use NFB at all. This is because it simply
- * doesn't work well with memcpy transfers. When you submit both buffers it is
- * extremely unlikely that you get an NFB interrupt, but it instead reports
- * DONE interrupt and both buffers are already transferred which means that we
- * weren't able to update the next buffer.
- *
- * So for now we "simulate" NFB by just submitting buffer after buffer
- * without double buffering.
  */
 
 static int m2m_hw_setup(struct ep93xx_dma_chan *edmac)
@@ -543,6 +551,11 @@ static void m2m_hw_submit(struct ep93xx_
 	m2m_fill_desc(edmac);
 	control |= M2M_CONTROL_DONEINT;
 
+	if (ep93xx_dma_advance_active(edmac)) {
+		m2m_fill_desc(edmac);
+		control |= M2M_CONTROL_NFBINT;
+	}
+
 	/*
 	 * Now we can finally enable the channel. For M2M channel this must be
 	 * done _after_ the BCRx registers are programmed.
@@ -560,32 +573,85 @@ static void m2m_hw_submit(struct ep93xx_
 	}
 }
 
+/*
+ * According to EP93xx User's Guide, we should receive DONE interrupt when all
+ * M2M DMA controller transactions complete normally. This is not always the
+ * case - sometimes EP93xx M2M DMA asserts DONE interrupt when the DMA channel
+ * is still running (channel Buffer FSM in DMA_BUF_ON state, and channel
+ * Control FSM in DMA_MEM_RD state, observed at least in IDE-DMA operation).
+ * In effect, disabling the channel when only DONE bit is set could stop
+ * currently running DMA transfer. To avoid this, we use Buffer FSM and
+ * Control FSM to check current state of DMA channel.
+ */
 static int m2m_hw_interrupt(struct ep93xx_dma_chan *edmac)
 {
+	u32 status = readl(edmac->regs + M2M_STATUS);
+	u32 ctl_fsm = status & M2M_STATUS_CTL_MASK;
+	u32 buf_fsm = status & M2M_STATUS_BUF_MASK;
+	bool done = status & M2M_STATUS_DONE;
+	bool last;
 	u32 control;
 
-	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_DONEINT))
+	/* Accept only DONE and NFB interrupts */
+	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_MASK))
 		return INTERRUPT_UNKNOWN;
 
-	/* Clear the DONE bit */
-	writel(0, edmac->regs + M2M_INTERRUPT);
+	if (done)
+		/* Clear the DONE bit */
+		writel(0, edmac->regs + M2M_INTERRUPT);
 
-	/* Disable interrupts and the channel */
-	control = readl(edmac->regs + M2M_CONTROL);
-	control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_ENABLE);
-	writel(control, edmac->regs + M2M_CONTROL);
+	/*
+	 * Check whether we are done with descriptors or not. This, together
+	 * with DMA channel state, determines action to take in interrupt.
+	 */
+	last = list_first_entry(edmac->active.next,
+		struct ep93xx_dma_desc, node)->txd.cookie;
 
 	/*
-	 * Since we only get DONE interrupt we have to find out ourselves
-	 * whether there still is something to process. So we try to advance
-	 * the chain an see whether it succeeds.
+	 * Use M2M DMA Buffer FSM and Control FSM to check current state of
+	 * DMA channel. Using DONE and NFB bits from channel status register
+	 * or bits from channel interrupt register was proven not to be
+	 * reliable.
 	 */
-	if (ep93xx_dma_advance_active(edmac)) {
-		edmac->edma->hw_submit(edmac);
+	if (!last &&
+	    (buf_fsm == M2M_STATUS_BUF_NO ||
+	     buf_fsm == M2M_STATUS_BUF_ON)) {
+		/*
+		 * Two buffers are ready for update when Buffer FSM is in
+		 * DMA_NO_BUF state. Only one buffer can be prepared without
+		 * disabling the channel, or polling the DONE bit.
+		 * To simplify things, always prepare only one buffer.
+		 */
+		ep93xx_dma_advance_active(edmac);
+		m2m_fill_desc(edmac);
+		if (done && !edmac->chan.private) {
+			/* Software trigger for memcpy channel */
+			control = readl(edmac->regs + M2M_CONTROL);
+			control |= M2M_CONTROL_START;
+			writel(control, edmac->regs + M2M_CONTROL);
+		}
 		return INTERRUPT_NEXT_BUFFER;
 	}
 
-	return INTERRUPT_DONE;
+	/*
+	 * Disable the channel only when Buffer FSM is in DMA_NO_BUF state
+	 * and Control FSM is in DMA_STALL state.
+	 */
+	if (last &&
+	    buf_fsm == M2M_STATUS_BUF_NO &&
+	    ctl_fsm == M2M_STATUS_CTL_STALL) {
+		/* Disable interrupts and the channel */
+		control = readl(edmac->regs + M2M_CONTROL);
+		control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_NFBINT
+			    | M2M_CONTROL_ENABLE);
+		writel(control, edmac->regs + M2M_CONTROL);
+		return INTERRUPT_DONE;
+	}
+
+	/*
+	 * Nothing to do this time.
+	 */
+	return INTERRUPT_NEXT_BUFFER;
 }
 
 /*

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Rafal Prylowski]

Implement double buffering for M2M DMA channels.

Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>

Index: slave-dma/drivers/dma/ep93xx_dma.c
===================================================================
--- slave-dma.orig/drivers/dma/ep93xx_dma.c
+++ slave-dma/drivers/dma/ep93xx_dma.c
@@ -69,6 +69,7 @@
 #define M2M_CONTROL_TM_SHIFT		13
 #define M2M_CONTROL_TM_TX		(1 << M2M_CONTROL_TM_SHIFT)
 #define M2M_CONTROL_TM_RX		(2 << M2M_CONTROL_TM_SHIFT)
+#define M2M_CONTROL_NFBINT		BIT(21)
 #define M2M_CONTROL_RSS_SHIFT		22
 #define M2M_CONTROL_RSS_SSPRX		(1 << M2M_CONTROL_RSS_SHIFT)
 #define M2M_CONTROL_RSS_SSPTX		(2 << M2M_CONTROL_RSS_SHIFT)
@@ -77,7 +78,23 @@
 #define M2M_CONTROL_PWSC_SHIFT		25
 
 #define M2M_INTERRUPT			0x0004
-#define M2M_INTERRUPT_DONEINT		BIT(1)
+#define M2M_INTERRUPT_MASK		6
+
+#define M2M_STATUS			0x000c
+#define M2M_STATUS_CTL_SHIFT		1
+#define M2M_STATUS_CTL_IDLE		(0 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_STALL		(1 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_MEMRD		(2 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_MEMWR		(3 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_BWCWAIT		(4 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_MASK		(7 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_BUF_SHIFT		4
+#define M2M_STATUS_BUF_NO		(0 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_BUF_ON		(1 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_BUF_NEXT		(2 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_BUF_MASK		(3 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_DONE			BIT(6)
+
 
 #define M2M_BCR0			0x0010
 #define M2M_BCR1			0x0014
@@ -426,15 +443,6 @@ static int m2p_hw_interrupt(struct ep93x
 
 /*
  * M2M DMA implementation
- *
- * For the M2M transfers we don't use NFB at all. This is because it simply
- * doesn't work well with memcpy transfers. When you submit both buffers it is
- * extremely unlikely that you get an NFB interrupt, but it instead reports
- * DONE interrupt and both buffers are already transferred which means that we
- * weren't able to update the next buffer.
- *
- * So for now we "simulate" NFB by just submitting buffer after buffer
- * without double buffering.
  */
 
 static int m2m_hw_setup(struct ep93xx_dma_chan *edmac)
@@ -543,6 +551,11 @@ static void m2m_hw_submit(struct ep93xx_
 	m2m_fill_desc(edmac);
 	control |= M2M_CONTROL_DONEINT;
 
+	if (ep93xx_dma_advance_active(edmac)) {
+		m2m_fill_desc(edmac);
+		control |= M2M_CONTROL_NFBINT;
+	}
+
 	/*
 	 * Now we can finally enable the channel. For M2M channel this must be
 	 * done _after_ the BCRx registers are programmed.
@@ -560,32 +573,85 @@ static void m2m_hw_submit(struct ep93xx_
 	}
 }
 
+/*
+ * According to EP93xx User's Guide, we should receive DONE interrupt when all
+ * M2M DMA controller transactions complete normally. This is not always the
+ * case - sometimes EP93xx M2M DMA asserts DONE interrupt when the DMA channel
+ * is still running (channel Buffer FSM in DMA_BUF_ON state, and channel
+ * Control FSM in DMA_MEM_RD state, observed@least in IDE-DMA operation).
+ * In effect, disabling the channel when only DONE bit is set could stop
+ * currently running DMA transfer. To avoid this, we use Buffer FSM and
+ * Control FSM to check current state of DMA channel.
+ */
 static int m2m_hw_interrupt(struct ep93xx_dma_chan *edmac)
 {
+	u32 status = readl(edmac->regs + M2M_STATUS);
+	u32 ctl_fsm = status & M2M_STATUS_CTL_MASK;
+	u32 buf_fsm = status & M2M_STATUS_BUF_MASK;
+	bool done = status & M2M_STATUS_DONE;
+	bool last;
 	u32 control;
 
-	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_DONEINT))
+	/* Accept only DONE and NFB interrupts */
+	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_MASK))
 		return INTERRUPT_UNKNOWN;
 
-	/* Clear the DONE bit */
-	writel(0, edmac->regs + M2M_INTERRUPT);
+	if (done)
+		/* Clear the DONE bit */
+		writel(0, edmac->regs + M2M_INTERRUPT);
 
-	/* Disable interrupts and the channel */
-	control = readl(edmac->regs + M2M_CONTROL);
-	control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_ENABLE);
-	writel(control, edmac->regs + M2M_CONTROL);
+	/*
+	 * Check whether we are done with descriptors or not. This, together
+	 * with DMA channel state, determines action to take in interrupt.
+	 */
+	last = list_first_entry(edmac->active.next,
+		struct ep93xx_dma_desc, node)->txd.cookie;
 
 	/*
-	 * Since we only get DONE interrupt we have to find out ourselves
-	 * whether there still is something to process. So we try to advance
-	 * the chain an see whether it succeeds.
+	 * Use M2M DMA Buffer FSM and Control FSM to check current state of
+	 * DMA channel. Using DONE and NFB bits from channel status register
+	 * or bits from channel interrupt register was proven not to be
+	 * reliable.
 	 */
-	if (ep93xx_dma_advance_active(edmac)) {
-		edmac->edma->hw_submit(edmac);
+	if (!last &&
+	    (buf_fsm == M2M_STATUS_BUF_NO ||
+	     buf_fsm == M2M_STATUS_BUF_ON)) {
+		/*
+		 * Two buffers are ready for update when Buffer FSM is in
+		 * DMA_NO_BUF state. Only one buffer can be prepared without
+		 * disabling the channel, or polling the DONE bit.
+		 * To simplify things, always prepare only one buffer.
+		 */
+		ep93xx_dma_advance_active(edmac);
+		m2m_fill_desc(edmac);
+		if (done && !edmac->chan.private) {
+			/* Software trigger for memcpy channel */
+			control = readl(edmac->regs + M2M_CONTROL);
+			control |= M2M_CONTROL_START;
+			writel(control, edmac->regs + M2M_CONTROL);
+		}
 		return INTERRUPT_NEXT_BUFFER;
 	}
 
-	return INTERRUPT_DONE;
+	/*
+	 * Disable the channel only when Buffer FSM is in DMA_NO_BUF state
+	 * and Control FSM is in DMA_STALL state.
+	 */
+	if (last &&
+	    buf_fsm == M2M_STATUS_BUF_NO &&
+	    ctl_fsm == M2M_STATUS_CTL_STALL) {
+		/* Disable interrupts and the channel */
+		control = readl(edmac->regs + M2M_CONTROL);
+		control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_NFBINT
+			    | M2M_CONTROL_ENABLE);
+		writel(control, edmac->regs + M2M_CONTROL);
+		return INTERRUPT_DONE;
+	}
+
+	/*
+	 * Nothing to do this time.
+	 */
+	return INTERRUPT_NEXT_BUFFER;
 }
 
 /*

Re: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Mika Westerberg]

On Tue, Mar 20, 2012 at 09:09:26AM +0100, Rafal Prylowski wrote:
> Implement double buffering for M2M DMA channels.
> 
> Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>
> 
> Index: slave-dma/drivers/dma/ep93xx_dma.c
> ===================================================================
> --- slave-dma.orig/drivers/dma/ep93xx_dma.c
> +++ slave-dma/drivers/dma/ep93xx_dma.c
> @@ -69,6 +69,7 @@

Which tree this is supposed to apply? I tried both mainline and -next and it
doesn't apply cleanly there.

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Mika Westerberg]

On Tue, Mar 20, 2012 at 09:09:26AM +0100, Rafal Prylowski wrote:
> Implement double buffering for M2M DMA channels.
> 
> Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>
> 
> Index: slave-dma/drivers/dma/ep93xx_dma.c
> ===================================================================
> --- slave-dma.orig/drivers/dma/ep93xx_dma.c
> +++ slave-dma/drivers/dma/ep93xx_dma.c
> @@ -69,6 +69,7 @@

Which tree this is supposed to apply? I tried both mainline and -next and it
doesn't apply cleanly there.

Re: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Rafal Prylowski]

On 2012-03-21 08:07, Mika Westerberg wrote:
> 
> Which tree this is supposed to apply? I tried both mainline and -next and it
> doesn't apply cleanly there.
> 

I prepared it for git://git.infradead.org/users/vkoul/slave-dma.git tree.
Also, I checked that it applies cleanly to:
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
(I'm assuming this is mainline).

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Rafal Prylowski]

On 2012-03-21 08:07, Mika Westerberg wrote:
> 
> Which tree this is supposed to apply? I tried both mainline and -next and it
> doesn't apply cleanly there.
> 

I prepared it for git://git.infradead.org/users/vkoul/slave-dma.git tree.
Also, I checked that it applies cleanly to:
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
(I'm assuming this is mainline).

RE: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Tuesday, March 20, 2012 1:09 AM, Rafal Prylowski wrote:
>
> Implement double buffering for M2M DMA channels.
>
> Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>

Hmm... This isn't working on my ep9307 board...

When the system boots I get stormed with messages:

dma dma1chan0: unknown interrupt!
dma dma1chan0: unknown interrupt!
dma dma1chan0: unknown interrupt!
dma dma1chan1: unknown interrupt!
dma dma1chan1: unknown interrupt!
dma dma1chan1: unknown interrupt!
dma dma1chan0: unknown interrupt!
dma dma1chan0: unknown interrupt!
dma dma1chan1: unknown interrupt!

Note, I have use_dma set for the ep93xx_spi driver and am using mmc_spi.

Also note, without your patch I also get a couple messages:

dma dma1chan1: got interrupt while active list is empty

But, the mmc card is working fine. I haven't tracked down why I get these
yet. Mika, any ideas?

Regards,
Hartley

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Tuesday, March 20, 2012 1:09 AM, Rafal Prylowski wrote:
>
> Implement double buffering for M2M DMA channels.
>
> Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>

Hmm... This isn't working on my ep9307 board...

When the system boots I get stormed with messages:

dma dma1chan0: unknown interrupt!
dma dma1chan0: unknown interrupt!
dma dma1chan0: unknown interrupt!
dma dma1chan1: unknown interrupt!
dma dma1chan1: unknown interrupt!
dma dma1chan1: unknown interrupt!
dma dma1chan0: unknown interrupt!
dma dma1chan0: unknown interrupt!
dma dma1chan1: unknown interrupt!

Note, I have use_dma set for the ep93xx_spi driver and am using mmc_spi.

Also note, without your patch I also get a couple messages:

dma dma1chan1: got interrupt while active list is empty

But, the mmc card is working fine. I haven't tracked down why I get these
yet. Mika, any ideas?

Regards,
Hartley

Re: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Mika Westerberg]

On Wed, Mar 21, 2012 at 12:12:11PM -0500, H Hartley Sweeten wrote:
> On Tuesday, March 20, 2012 1:09 AM, Rafal Prylowski wrote:
> >
> > Implement double buffering for M2M DMA channels.
> >
> > Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>
> 
> Hmm... This isn't working on my ep9307 board...
> 
> When the system boots I get stormed with messages:
> 
> dma dma1chan0: unknown interrupt!
> dma dma1chan0: unknown interrupt!
> dma dma1chan0: unknown interrupt!
> dma dma1chan1: unknown interrupt!
> dma dma1chan1: unknown interrupt!
> dma dma1chan1: unknown interrupt!
> dma dma1chan0: unknown interrupt!
> dma dma1chan0: unknown interrupt!
> dma dma1chan1: unknown interrupt!
> 
> Note, I have use_dma set for the ep93xx_spi driver and am using mmc_spi.
> 
> Also note, without your patch I also get a couple messages:
> 
> dma dma1chan1: got interrupt while active list is empty
> 
> But, the mmc card is working fine. I haven't tracked down why I get these
> yet. Mika, any ideas?

I've noticed the same. But they also happen without this patch applied. I've
tracked it down to MULTI_IRQ_HANDLER support patches for ARM vic but not sure
what might be wrong there (or is the ep93xx_dma driver behaving badly). I
noticed that when we get that 'got interrupt while active list is empty' we
don't have any interrupts set in the M2M_INTERRUPT register but we still got
an interrupt.. weird.

Below is a workaround I've been using lately but it certainly is a hack and we
should figure out what is the root cause for this.

diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c
index dcb004a..cb6b49a 100644
--- a/arch/arm/common/vic.c
+++ b/arch/arm/common/vic.c
@@ -441,11 +441,9 @@ static int handle_one_vic(struct vic_device *vic, struct pt_regs *regs)
 	u32 stat, irq;
 	int handled = 0;
 
-	stat = readl_relaxed(vic->base + VIC_IRQ_STATUS);
-	while (stat) {
+	while ((stat = readl_relaxed(vic->base + VIC_IRQ_STATUS))) {
 		irq = ffs(stat) - 1;
 		handle_IRQ(irq_domain_to_irq(&vic->domain, irq), regs);
-		stat &= ~(1 << irq);
 		handled = 1;
 	}
 

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Mika Westerberg]

On Wed, Mar 21, 2012 at 12:12:11PM -0500, H Hartley Sweeten wrote:
> On Tuesday, March 20, 2012 1:09 AM, Rafal Prylowski wrote:
> >
> > Implement double buffering for M2M DMA channels.
> >
> > Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>
> 
> Hmm... This isn't working on my ep9307 board...
> 
> When the system boots I get stormed with messages:
> 
> dma dma1chan0: unknown interrupt!
> dma dma1chan0: unknown interrupt!
> dma dma1chan0: unknown interrupt!
> dma dma1chan1: unknown interrupt!
> dma dma1chan1: unknown interrupt!
> dma dma1chan1: unknown interrupt!
> dma dma1chan0: unknown interrupt!
> dma dma1chan0: unknown interrupt!
> dma dma1chan1: unknown interrupt!
> 
> Note, I have use_dma set for the ep93xx_spi driver and am using mmc_spi.
> 
> Also note, without your patch I also get a couple messages:
> 
> dma dma1chan1: got interrupt while active list is empty
> 
> But, the mmc card is working fine. I haven't tracked down why I get these
> yet. Mika, any ideas?

I've noticed the same. But they also happen without this patch applied. I've
tracked it down to MULTI_IRQ_HANDLER support patches for ARM vic but not sure
what might be wrong there (or is the ep93xx_dma driver behaving badly). I
noticed that when we get that 'got interrupt while active list is empty' we
don't have any interrupts set in the M2M_INTERRUPT register but we still got
an interrupt.. weird.

Below is a workaround I've been using lately but it certainly is a hack and we
should figure out what is the root cause for this.

diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c
index dcb004a..cb6b49a 100644
--- a/arch/arm/common/vic.c
+++ b/arch/arm/common/vic.c
@@ -441,11 +441,9 @@ static int handle_one_vic(struct vic_device *vic, struct pt_regs *regs)
 	u32 stat, irq;
 	int handled = 0;
 
-	stat = readl_relaxed(vic->base + VIC_IRQ_STATUS);
-	while (stat) {
+	while ((stat = readl_relaxed(vic->base + VIC_IRQ_STATUS))) {
 		irq = ffs(stat) - 1;
 		handle_IRQ(irq_domain_to_irq(&vic->domain, irq), regs);
-		stat &= ~(1 << irq);
 		handled = 1;
 	}
 

Re: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Mika Westerberg]

On Wed, Mar 21, 2012 at 08:47:22AM +0100, Rafal Prylowski wrote:
> On 2012-03-21 08:07, Mika Westerberg wrote:
> > 
> > Which tree this is supposed to apply? I tried both mainline and -next and it
> > doesn't apply cleanly there.
> > 
> 
> I prepared it for git://git.infradead.org/users/vkoul/slave-dma.git tree.
> Also, I checked that it applies cleanly to:
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
> (I'm assuming this is mainline).

Yes, it applies cleanly. I messed up when applying it. Sorry about that.

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Mika Westerberg]

On Wed, Mar 21, 2012 at 08:47:22AM +0100, Rafal Prylowski wrote:
> On 2012-03-21 08:07, Mika Westerberg wrote:
> > 
> > Which tree this is supposed to apply? I tried both mainline and -next and it
> > doesn't apply cleanly there.
> > 
> 
> I prepared it for git://git.infradead.org/users/vkoul/slave-dma.git tree.
> Also, I checked that it applies cleanly to:
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
> (I'm assuming this is mainline).

Yes, it applies cleanly. I messed up when applying it. Sorry about that.

Re: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Mika Westerberg]

On Tue, Mar 20, 2012 at 09:09:26AM +0100, Rafal Prylowski wrote:
> Implement double buffering for M2M DMA channels.
> 
> Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>

The subject line prefix should probably be 'dmaengine/ep93xx_dma: ' but
otherwise this looks good.

Acked-by: Mika Westerberg <mika.westerberg@iki.fi>

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Mika Westerberg]

On Tue, Mar 20, 2012 at 09:09:26AM +0100, Rafal Prylowski wrote:
> Implement double buffering for M2M DMA channels.
> 
> Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>

The subject line prefix should probably be 'dmaengine/ep93xx_dma: ' but
otherwise this looks good.

Acked-by: Mika Westerberg <mika.westerberg@iki.fi>

RE: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Wednesday, March 21, 2012 12:33 PM, Mika Westerberg wrote:
> On Wed, Mar 21, 2012 at 12:12:11PM -0500, H Hartley Sweeten wrote:
>> On Tuesday, March 20, 2012 1:09 AM, Rafal Prylowski wrote:
>>>
>>> Implement double buffering for M2M DMA channels.
>>>
>>> Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>
>> 
>> Hmm... This isn't working on my ep9307 board...
>> 
>> When the system boots I get stormed with messages:
>> 
>> dma dma1chan0: unknown interrupt!
>> dma dma1chan0: unknown interrupt!
>> dma dma1chan0: unknown interrupt!
>> dma dma1chan1: unknown interrupt!
>> dma dma1chan1: unknown interrupt!
>> dma dma1chan1: unknown interrupt!
>> dma dma1chan0: unknown interrupt!
>> dma dma1chan0: unknown interrupt!
>> dma dma1chan1: unknown interrupt!
>> 
>> Note, I have use_dma set for the ep93xx_spi driver and am using mmc_spi.
>> 
>> Also note, without your patch I also get a couple messages:
>> 
>> dma dma1chan1: got interrupt while active list is empty
>> 
>> But, the mmc card is working fine. I haven't tracked down why I get these
>> yet. Mika, any ideas?
>
> I've noticed the same. But they also happen without this patch applied. I've
> tracked it down to MULTI_IRQ_HANDLER support patches for ARM vic but not sure
> what might be wrong there (or is the ep93xx_dma driver behaving badly). I
> Noticed that when we get that 'got interrupt while active list is empty' we
> don't have any interrupts set in the M2M_INTERRUPT register but we still got
> an interrupt.. weird.

I think you misunderstood my comment above.

With this patch applied I get the "unknown interrupt!" storm. These messages
keep getting spewed until I power off the board.

Without this patch I get the "got interrupt while active list is empty" messages
but only occasionally. Other than the messages the mmc_spi driver seems to
be working ok with dma.

I hacked in a dump of the DMA Global Interrupt register when I get the
"got interrupt while active list is empty" messages and get this:

dma dma1chan1: got interrupt while active list is empty (00000000)

So, according to the DMAGlInt register, there are no channels with an
active interrupt.

Are we missing a write to the INTERRUPT registers somewhere to clear the
current interrupt?

Regards,
Hartley

> Below is a workaround I've been using lately but it certainly is a hack and we
> should figure out what is the root cause for this.
>
> diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c
> index dcb004a..cb6b49a 100644
> --- a/arch/arm/common/vic.c
> +++ b/arch/arm/common/vic.c
> @@ -441,11 +441,9 @@ static int handle_one_vic(struct vic_device *vic, struct pt_regs *regs)
>  	u32 stat, irq;
>  	int handled = 0;
>  
> -	stat = readl_relaxed(vic->base + VIC_IRQ_STATUS);
> -	while (stat) {
> +	while ((stat = readl_relaxed(vic->base + VIC_IRQ_STATUS))) {
>  		irq = ffs(stat) - 1;
>  		handle_IRQ(irq_domain_to_irq(&vic->domain, irq), regs);
> -		stat &= ~(1 << irq);
>  		handled = 1;
>  	}
 

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Wednesday, March 21, 2012 12:33 PM, Mika Westerberg wrote:
> On Wed, Mar 21, 2012 at 12:12:11PM -0500, H Hartley Sweeten wrote:
>> On Tuesday, March 20, 2012 1:09 AM, Rafal Prylowski wrote:
>>>
>>> Implement double buffering for M2M DMA channels.
>>>
>>> Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>
>> 
>> Hmm... This isn't working on my ep9307 board...
>> 
>> When the system boots I get stormed with messages:
>> 
>> dma dma1chan0: unknown interrupt!
>> dma dma1chan0: unknown interrupt!
>> dma dma1chan0: unknown interrupt!
>> dma dma1chan1: unknown interrupt!
>> dma dma1chan1: unknown interrupt!
>> dma dma1chan1: unknown interrupt!
>> dma dma1chan0: unknown interrupt!
>> dma dma1chan0: unknown interrupt!
>> dma dma1chan1: unknown interrupt!
>> 
>> Note, I have use_dma set for the ep93xx_spi driver and am using mmc_spi.
>> 
>> Also note, without your patch I also get a couple messages:
>> 
>> dma dma1chan1: got interrupt while active list is empty
>> 
>> But, the mmc card is working fine. I haven't tracked down why I get these
>> yet. Mika, any ideas?
>
> I've noticed the same. But they also happen without this patch applied. I've
> tracked it down to MULTI_IRQ_HANDLER support patches for ARM vic but not sure
> what might be wrong there (or is the ep93xx_dma driver behaving badly). I
> Noticed that when we get that 'got interrupt while active list is empty' we
> don't have any interrupts set in the M2M_INTERRUPT register but we still got
> an interrupt.. weird.

I think you misunderstood my comment above.

With this patch applied I get the "unknown interrupt!" storm. These messages
keep getting spewed until I power off the board.

Without this patch I get the "got interrupt while active list is empty" messages
but only occasionally. Other than the messages the mmc_spi driver seems to
be working ok with dma.

I hacked in a dump of the DMA Global Interrupt register when I get the
"got interrupt while active list is empty" messages and get this:

dma dma1chan1: got interrupt while active list is empty (00000000)

So, according to the DMAGlInt register, there are no channels with an
active interrupt.

Are we missing a write to the INTERRUPT registers somewhere to clear the
current interrupt?

Regards,
Hartley

> Below is a workaround I've been using lately but it certainly is a hack and we
> should figure out what is the root cause for this.
>
> diff --git a/arch/arm/common/vic.c b/arch/arm/common/vic.c
> index dcb004a..cb6b49a 100644
> --- a/arch/arm/common/vic.c
> +++ b/arch/arm/common/vic.c
> @@ -441,11 +441,9 @@ static int handle_one_vic(struct vic_device *vic, struct pt_regs *regs)
>  	u32 stat, irq;
>  	int handled = 0;
>  
> -	stat = readl_relaxed(vic->base + VIC_IRQ_STATUS);
> -	while (stat) {
> +	while ((stat = readl_relaxed(vic->base + VIC_IRQ_STATUS))) {
>  		irq = ffs(stat) - 1;
>  		handle_IRQ(irq_domain_to_irq(&vic->domain, irq), regs);
> -		stat &= ~(1 << irq);
>  		handled = 1;
>  	}
 

Re: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Ryan Mallon]

On 20/03/12 19:09, Rafal Prylowski wrote:

> Implement double buffering for M2M DMA channels.
> 


I haven't looked through the patch yet, since I'm waiting on more
information from Mika and Hartley's testing.

However, the commit log doesn't tell me why we want this change. Is it a
performance improvement? If so, do you have some numbers that we can
paste into the commit log?

Thanks,
~Ryan

> Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>
> 
> Index: slave-dma/drivers/dma/ep93xx_dma.c
> ===================================================================
> --- slave-dma.orig/drivers/dma/ep93xx_dma.c
> +++ slave-dma/drivers/dma/ep93xx_dma.c
> @@ -69,6 +69,7 @@
>  #define M2M_CONTROL_TM_SHIFT		13
>  #define M2M_CONTROL_TM_TX		(1 << M2M_CONTROL_TM_SHIFT)
>  #define M2M_CONTROL_TM_RX		(2 << M2M_CONTROL_TM_SHIFT)
> +#define M2M_CONTROL_NFBINT		BIT(21)
>  #define M2M_CONTROL_RSS_SHIFT		22
>  #define M2M_CONTROL_RSS_SSPRX		(1 << M2M_CONTROL_RSS_SHIFT)
>  #define M2M_CONTROL_RSS_SSPTX		(2 << M2M_CONTROL_RSS_SHIFT)
> @@ -77,7 +78,23 @@
>  #define M2M_CONTROL_PWSC_SHIFT		25
>  
>  #define M2M_INTERRUPT			0x0004
> -#define M2M_INTERRUPT_DONEINT		BIT(1)
> +#define M2M_INTERRUPT_MASK		6
> +
> +#define M2M_STATUS			0x000c
> +#define M2M_STATUS_CTL_SHIFT		1
> +#define M2M_STATUS_CTL_IDLE		(0 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_STALL		(1 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_MEMRD		(2 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_MEMWR		(3 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_BWCWAIT		(4 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_MASK		(7 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_BUF_SHIFT		4
> +#define M2M_STATUS_BUF_NO		(0 << M2M_STATUS_BUF_SHIFT)
> +#define M2M_STATUS_BUF_ON		(1 << M2M_STATUS_BUF_SHIFT)
> +#define M2M_STATUS_BUF_NEXT		(2 << M2M_STATUS_BUF_SHIFT)
> +#define M2M_STATUS_BUF_MASK		(3 << M2M_STATUS_BUF_SHIFT)
> +#define M2M_STATUS_DONE			BIT(6)
> +
>  
>  #define M2M_BCR0			0x0010
>  #define M2M_BCR1			0x0014
> @@ -426,15 +443,6 @@ static int m2p_hw_interrupt(struct ep93x
>  
>  /*
>   * M2M DMA implementation
> - *
> - * For the M2M transfers we don't use NFB at all. This is because it simply
> - * doesn't work well with memcpy transfers. When you submit both buffers it is
> - * extremely unlikely that you get an NFB interrupt, but it instead reports
> - * DONE interrupt and both buffers are already transferred which means that we
> - * weren't able to update the next buffer.
> - *
> - * So for now we "simulate" NFB by just submitting buffer after buffer
> - * without double buffering.
>   */
>  
>  static int m2m_hw_setup(struct ep93xx_dma_chan *edmac)
> @@ -543,6 +551,11 @@ static void m2m_hw_submit(struct ep93xx_
>  	m2m_fill_desc(edmac);
>  	control |= M2M_CONTROL_DONEINT;
>  
> +	if (ep93xx_dma_advance_active(edmac)) {
> +		m2m_fill_desc(edmac);
> +		control |= M2M_CONTROL_NFBINT;
> +	}
> +
>  	/*
>  	 * Now we can finally enable the channel. For M2M channel this must be
>  	 * done _after_ the BCRx registers are programmed.
> @@ -560,32 +573,85 @@ static void m2m_hw_submit(struct ep93xx_
>  	}
>  }
>  
> +/*
> + * According to EP93xx User's Guide, we should receive DONE interrupt when all
> + * M2M DMA controller transactions complete normally. This is not always the
> + * case - sometimes EP93xx M2M DMA asserts DONE interrupt when the DMA channel
> + * is still running (channel Buffer FSM in DMA_BUF_ON state, and channel
> + * Control FSM in DMA_MEM_RD state, observed at least in IDE-DMA operation).
> + * In effect, disabling the channel when only DONE bit is set could stop
> + * currently running DMA transfer. To avoid this, we use Buffer FSM and
> + * Control FSM to check current state of DMA channel.
> + */
>  static int m2m_hw_interrupt(struct ep93xx_dma_chan *edmac)
>  {
> +	u32 status = readl(edmac->regs + M2M_STATUS);
> +	u32 ctl_fsm = status & M2M_STATUS_CTL_MASK;
> +	u32 buf_fsm = status & M2M_STATUS_BUF_MASK;
> +	bool done = status & M2M_STATUS_DONE;
> +	bool last;
>  	u32 control;
>  
> -	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_DONEINT))
> +	/* Accept only DONE and NFB interrupts */
> +	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_MASK))
>  		return INTERRUPT_UNKNOWN;
>  
> -	/* Clear the DONE bit */
> -	writel(0, edmac->regs + M2M_INTERRUPT);
> +	if (done)
> +		/* Clear the DONE bit */
> +		writel(0, edmac->regs + M2M_INTERRUPT);
>  
> -	/* Disable interrupts and the channel */
> -	control = readl(edmac->regs + M2M_CONTROL);
> -	control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_ENABLE);
> -	writel(control, edmac->regs + M2M_CONTROL);
> +	/*
> +	 * Check whether we are done with descriptors or not. This, together
> +	 * with DMA channel state, determines action to take in interrupt.
> +	 */
> +	last = list_first_entry(edmac->active.next,
> +		struct ep93xx_dma_desc, node)->txd.cookie;
>  
>  	/*
> -	 * Since we only get DONE interrupt we have to find out ourselves
> -	 * whether there still is something to process. So we try to advance
> -	 * the chain an see whether it succeeds.
> +	 * Use M2M DMA Buffer FSM and Control FSM to check current state of
> +	 * DMA channel. Using DONE and NFB bits from channel status register
> +	 * or bits from channel interrupt register was proven not to be
> +	 * reliable.
>  	 */
> -	if (ep93xx_dma_advance_active(edmac)) {
> -		edmac->edma->hw_submit(edmac);
> +	if (!last &&
> +	    (buf_fsm == M2M_STATUS_BUF_NO ||
> +	     buf_fsm == M2M_STATUS_BUF_ON)) {
> +		/*
> +		 * Two buffers are ready for update when Buffer FSM is in
> +		 * DMA_NO_BUF state. Only one buffer can be prepared without
> +		 * disabling the channel, or polling the DONE bit.
> +		 * To simplify things, always prepare only one buffer.
> +		 */
> +		ep93xx_dma_advance_active(edmac);
> +		m2m_fill_desc(edmac);
> +		if (done && !edmac->chan.private) {
> +			/* Software trigger for memcpy channel */
> +			control = readl(edmac->regs + M2M_CONTROL);
> +			control |= M2M_CONTROL_START;
> +			writel(control, edmac->regs + M2M_CONTROL);
> +		}
>  		return INTERRUPT_NEXT_BUFFER;
>  	}
>  
> -	return INTERRUPT_DONE;
> +	/*
> +	 * Disable the channel only when Buffer FSM is in DMA_NO_BUF state
> +	 * and Control FSM is in DMA_STALL state.
> +	 */
> +	if (last &&
> +	    buf_fsm == M2M_STATUS_BUF_NO &&
> +	    ctl_fsm == M2M_STATUS_CTL_STALL) {
> +		/* Disable interrupts and the channel */
> +		control = readl(edmac->regs + M2M_CONTROL);
> +		control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_NFBINT
> +			    | M2M_CONTROL_ENABLE);
> +		writel(control, edmac->regs + M2M_CONTROL);
> +		return INTERRUPT_DONE;
> +	}
> +
> +	/*
> +	 * Nothing to do this time.
> +	 */
> +	return INTERRUPT_NEXT_BUFFER;
>  }
>  
>  /*

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Ryan Mallon]

On 20/03/12 19:09, Rafal Prylowski wrote:

> Implement double buffering for M2M DMA channels.
> 


I haven't looked through the patch yet, since I'm waiting on more
information from Mika and Hartley's testing.

However, the commit log doesn't tell me why we want this change. Is it a
performance improvement? If so, do you have some numbers that we can
paste into the commit log?

Thanks,
~Ryan

> Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>
> 
> Index: slave-dma/drivers/dma/ep93xx_dma.c
> ===================================================================
> --- slave-dma.orig/drivers/dma/ep93xx_dma.c
> +++ slave-dma/drivers/dma/ep93xx_dma.c
> @@ -69,6 +69,7 @@
>  #define M2M_CONTROL_TM_SHIFT		13
>  #define M2M_CONTROL_TM_TX		(1 << M2M_CONTROL_TM_SHIFT)
>  #define M2M_CONTROL_TM_RX		(2 << M2M_CONTROL_TM_SHIFT)
> +#define M2M_CONTROL_NFBINT		BIT(21)
>  #define M2M_CONTROL_RSS_SHIFT		22
>  #define M2M_CONTROL_RSS_SSPRX		(1 << M2M_CONTROL_RSS_SHIFT)
>  #define M2M_CONTROL_RSS_SSPTX		(2 << M2M_CONTROL_RSS_SHIFT)
> @@ -77,7 +78,23 @@
>  #define M2M_CONTROL_PWSC_SHIFT		25
>  
>  #define M2M_INTERRUPT			0x0004
> -#define M2M_INTERRUPT_DONEINT		BIT(1)
> +#define M2M_INTERRUPT_MASK		6
> +
> +#define M2M_STATUS			0x000c
> +#define M2M_STATUS_CTL_SHIFT		1
> +#define M2M_STATUS_CTL_IDLE		(0 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_STALL		(1 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_MEMRD		(2 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_MEMWR		(3 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_BWCWAIT		(4 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_MASK		(7 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_BUF_SHIFT		4
> +#define M2M_STATUS_BUF_NO		(0 << M2M_STATUS_BUF_SHIFT)
> +#define M2M_STATUS_BUF_ON		(1 << M2M_STATUS_BUF_SHIFT)
> +#define M2M_STATUS_BUF_NEXT		(2 << M2M_STATUS_BUF_SHIFT)
> +#define M2M_STATUS_BUF_MASK		(3 << M2M_STATUS_BUF_SHIFT)
> +#define M2M_STATUS_DONE			BIT(6)
> +
>  
>  #define M2M_BCR0			0x0010
>  #define M2M_BCR1			0x0014
> @@ -426,15 +443,6 @@ static int m2p_hw_interrupt(struct ep93x
>  
>  /*
>   * M2M DMA implementation
> - *
> - * For the M2M transfers we don't use NFB at all. This is because it simply
> - * doesn't work well with memcpy transfers. When you submit both buffers it is
> - * extremely unlikely that you get an NFB interrupt, but it instead reports
> - * DONE interrupt and both buffers are already transferred which means that we
> - * weren't able to update the next buffer.
> - *
> - * So for now we "simulate" NFB by just submitting buffer after buffer
> - * without double buffering.
>   */
>  
>  static int m2m_hw_setup(struct ep93xx_dma_chan *edmac)
> @@ -543,6 +551,11 @@ static void m2m_hw_submit(struct ep93xx_
>  	m2m_fill_desc(edmac);
>  	control |= M2M_CONTROL_DONEINT;
>  
> +	if (ep93xx_dma_advance_active(edmac)) {
> +		m2m_fill_desc(edmac);
> +		control |= M2M_CONTROL_NFBINT;
> +	}
> +
>  	/*
>  	 * Now we can finally enable the channel. For M2M channel this must be
>  	 * done _after_ the BCRx registers are programmed.
> @@ -560,32 +573,85 @@ static void m2m_hw_submit(struct ep93xx_
>  	}
>  }
>  
> +/*
> + * According to EP93xx User's Guide, we should receive DONE interrupt when all
> + * M2M DMA controller transactions complete normally. This is not always the
> + * case - sometimes EP93xx M2M DMA asserts DONE interrupt when the DMA channel
> + * is still running (channel Buffer FSM in DMA_BUF_ON state, and channel
> + * Control FSM in DMA_MEM_RD state, observed at least in IDE-DMA operation).
> + * In effect, disabling the channel when only DONE bit is set could stop
> + * currently running DMA transfer. To avoid this, we use Buffer FSM and
> + * Control FSM to check current state of DMA channel.
> + */
>  static int m2m_hw_interrupt(struct ep93xx_dma_chan *edmac)
>  {
> +	u32 status = readl(edmac->regs + M2M_STATUS);
> +	u32 ctl_fsm = status & M2M_STATUS_CTL_MASK;
> +	u32 buf_fsm = status & M2M_STATUS_BUF_MASK;
> +	bool done = status & M2M_STATUS_DONE;
> +	bool last;
>  	u32 control;
>  
> -	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_DONEINT))
> +	/* Accept only DONE and NFB interrupts */
> +	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_MASK))
>  		return INTERRUPT_UNKNOWN;
>  
> -	/* Clear the DONE bit */
> -	writel(0, edmac->regs + M2M_INTERRUPT);
> +	if (done)
> +		/* Clear the DONE bit */
> +		writel(0, edmac->regs + M2M_INTERRUPT);
>  
> -	/* Disable interrupts and the channel */
> -	control = readl(edmac->regs + M2M_CONTROL);
> -	control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_ENABLE);
> -	writel(control, edmac->regs + M2M_CONTROL);
> +	/*
> +	 * Check whether we are done with descriptors or not. This, together
> +	 * with DMA channel state, determines action to take in interrupt.
> +	 */
> +	last = list_first_entry(edmac->active.next,
> +		struct ep93xx_dma_desc, node)->txd.cookie;
>  
>  	/*
> -	 * Since we only get DONE interrupt we have to find out ourselves
> -	 * whether there still is something to process. So we try to advance
> -	 * the chain an see whether it succeeds.
> +	 * Use M2M DMA Buffer FSM and Control FSM to check current state of
> +	 * DMA channel. Using DONE and NFB bits from channel status register
> +	 * or bits from channel interrupt register was proven not to be
> +	 * reliable.
>  	 */
> -	if (ep93xx_dma_advance_active(edmac)) {
> -		edmac->edma->hw_submit(edmac);
> +	if (!last &&
> +	    (buf_fsm == M2M_STATUS_BUF_NO ||
> +	     buf_fsm == M2M_STATUS_BUF_ON)) {
> +		/*
> +		 * Two buffers are ready for update when Buffer FSM is in
> +		 * DMA_NO_BUF state. Only one buffer can be prepared without
> +		 * disabling the channel, or polling the DONE bit.
> +		 * To simplify things, always prepare only one buffer.
> +		 */
> +		ep93xx_dma_advance_active(edmac);
> +		m2m_fill_desc(edmac);
> +		if (done && !edmac->chan.private) {
> +			/* Software trigger for memcpy channel */
> +			control = readl(edmac->regs + M2M_CONTROL);
> +			control |= M2M_CONTROL_START;
> +			writel(control, edmac->regs + M2M_CONTROL);
> +		}
>  		return INTERRUPT_NEXT_BUFFER;
>  	}
>  
> -	return INTERRUPT_DONE;
> +	/*
> +	 * Disable the channel only when Buffer FSM is in DMA_NO_BUF state
> +	 * and Control FSM is in DMA_STALL state.
> +	 */
> +	if (last &&
> +	    buf_fsm == M2M_STATUS_BUF_NO &&
> +	    ctl_fsm == M2M_STATUS_CTL_STALL) {
> +		/* Disable interrupts and the channel */
> +		control = readl(edmac->regs + M2M_CONTROL);
> +		control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_NFBINT
> +			    | M2M_CONTROL_ENABLE);
> +		writel(control, edmac->regs + M2M_CONTROL);
> +		return INTERRUPT_DONE;
> +	}
> +
> +	/*
> +	 * Nothing to do this time.
> +	 */
> +	return INTERRUPT_NEXT_BUFFER;
>  }
>  
>  /*

Re: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Mika Westerberg]

On Wed, Mar 21, 2012 at 07:47:52PM -0500, H Hartley Sweeten wrote:

> I think you misunderstood my comment above.
> 
> With this patch applied I get the "unknown interrupt!" storm. These messages
> keep getting spewed until I power off the board.

Ah, right you are. Sorry.

With my Sim.One board, I don't see any interrupt storms at all and I also have
mmc_spi with DMA enabled.

> Without this patch I get the "got interrupt while active list is empty" messages
> but only occasionally. Other than the messages the mmc_spi driver seems to
> be working ok with dma.
> 
> I hacked in a dump of the DMA Global Interrupt register when I get the
> "got interrupt while active list is empty" messages and get this:
> 
> dma dma1chan1: got interrupt while active list is empty (00000000)

In addition to these messages my board hangs almost every boot.

> So, according to the DMAGlInt register, there are no channels with an
> active interrupt.
> 
> Are we missing a write to the INTERRUPT registers somewhere to clear the
> current interrupt?

In the current code we only enable DONE interrupt and we always clear that at
the beginning of the ISR.

The VIC code was changed in 3.3. Its behaviour is different now as it tries to
handle as many IRQs as possible in one go whereas before it only handled one
IRQ at a time. Could it be that the VIC in ep93xx doesn't cope with that?

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Mika Westerberg]

On Wed, Mar 21, 2012 at 07:47:52PM -0500, H Hartley Sweeten wrote:

> I think you misunderstood my comment above.
> 
> With this patch applied I get the "unknown interrupt!" storm. These messages
> keep getting spewed until I power off the board.

Ah, right you are. Sorry.

With my Sim.One board, I don't see any interrupt storms at all and I also have
mmc_spi with DMA enabled.

> Without this patch I get the "got interrupt while active list is empty" messages
> but only occasionally. Other than the messages the mmc_spi driver seems to
> be working ok with dma.
> 
> I hacked in a dump of the DMA Global Interrupt register when I get the
> "got interrupt while active list is empty" messages and get this:
> 
> dma dma1chan1: got interrupt while active list is empty (00000000)

In addition to these messages my board hangs almost every boot.

> So, according to the DMAGlInt register, there are no channels with an
> active interrupt.
> 
> Are we missing a write to the INTERRUPT registers somewhere to clear the
> current interrupt?

In the current code we only enable DONE interrupt and we always clear that at
the beginning of the ISR.

The VIC code was changed in 3.3. Its behaviour is different now as it tries to
handle as many IRQs as possible in one go whereas before it only handled one
IRQ at a time. Could it be that the VIC in ep93xx doesn't cope with that?

Re: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Rafal Prylowski]

On 2012-03-22 01:57, Ryan Mallon wrote:
> I haven't looked through the patch yet, since I'm waiting on more
> information from Mika and Hartley's testing.
> 
> However, the commit log doesn't tell me why we want this change. Is it a
> performance improvement? If so, do you have some numbers that we can
> paste into the commit log?

In principle, using double buffering should be faster than using only
one buffer and disabling/enabling channel each time. But my measurements
doesn't show any significant change.

The real reason for this change is that current code is not 100% reliable
in IDE-DMA (I'm planning to submit ep93xx ide driver to linux-ide soon).
Although using only one buffer and DONE interrupt is simple, surprisingly
we can get interrupt when M2M DMA is in DMA_BUF_ON and DMA_STALL state.
If we disable the channel at this moment, we end with ata timeout.

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Rafal Prylowski]

On 2012-03-22 01:57, Ryan Mallon wrote:
> I haven't looked through the patch yet, since I'm waiting on more
> information from Mika and Hartley's testing.
> 
> However, the commit log doesn't tell me why we want this change. Is it a
> performance improvement? If so, do you have some numbers that we can
> paste into the commit log?

In principle, using double buffering should be faster than using only
one buffer and disabling/enabling channel each time. But my measurements
doesn't show any significant change.

The real reason for this change is that current code is not 100% reliable
in IDE-DMA (I'm planning to submit ep93xx ide driver to linux-ide soon).
Although using only one buffer and DONE interrupt is simple, surprisingly
we can get interrupt when M2M DMA is in DMA_BUF_ON and DMA_STALL state.
If we disable the channel at this moment, we end with ata timeout.

Re: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Rafal Prylowski]

On 2012-03-21 18:12, H Hartley Sweeten wrote:
> Hmm... This isn't working on my ep9307 board...
> 
> When the system boots I get stormed with messages:
> 
> dma dma1chan0: unknown interrupt!
> dma dma1chan0: unknown interrupt!
> dma dma1chan0: unknown interrupt!
> dma dma1chan1: unknown interrupt!
> dma dma1chan1: unknown interrupt!
> dma dma1chan1: unknown interrupt!
> dma dma1chan0: unknown interrupt!
> dma dma1chan0: unknown interrupt!
> dma dma1chan1: unknown interrupt!
> 
> Note, I have use_dma set for the ep93xx_spi driver and am using mmc_spi.

Thanks for testing. Unfortunately, I'm unable to test this patch with SPI,
because my hardware uses it only for very simple communication (one byte
commands, I noticed that DMA is not used in this case, even if enabled).

In current mainline I also get these messages:

dma dma1chan0: unknown interrupt!
dma dma1chan1: got interrupt while active list is empty

With my patch applied they happen more often (maybe because NFB interrupt
is enabled), but the system is usable. Changing kernel to 3.2.x or using
hack proposed by Mika makes the problem is gone.

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Rafal Prylowski]

On 2012-03-21 18:12, H Hartley Sweeten wrote:
> Hmm... This isn't working on my ep9307 board...
> 
> When the system boots I get stormed with messages:
> 
> dma dma1chan0: unknown interrupt!
> dma dma1chan0: unknown interrupt!
> dma dma1chan0: unknown interrupt!
> dma dma1chan1: unknown interrupt!
> dma dma1chan1: unknown interrupt!
> dma dma1chan1: unknown interrupt!
> dma dma1chan0: unknown interrupt!
> dma dma1chan0: unknown interrupt!
> dma dma1chan1: unknown interrupt!
> 
> Note, I have use_dma set for the ep93xx_spi driver and am using mmc_spi.

Thanks for testing. Unfortunately, I'm unable to test this patch with SPI,
because my hardware uses it only for very simple communication (one byte
commands, I noticed that DMA is not used in this case, even if enabled).

In current mainline I also get these messages:

dma dma1chan0: unknown interrupt!
dma dma1chan1: got interrupt while active list is empty

With my patch applied they happen more often (maybe because NFB interrupt
is enabled), but the system is usable. Changing kernel to 3.2.x or using
hack proposed by Mika makes the problem is gone.

Re: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Sergei Shtylyov]

Hello.

On 22-03-2012 14:00, Rafal Prylowski wrote:

>> I haven't looked through the patch yet, since I'm waiting on more
>> information from Mika and Hartley's testing.

>> However, the commit log doesn't tell me why we want this change. Is it a
>> performance improvement? If so, do you have some numbers that we can
>> paste into the commit log?

> In principle, using double buffering should be faster than using only
> one buffer and disabling/enabling channel each time. But my measurements
> doesn't show any significant change.

> The real reason for this change is that current code is not 100% reliable
> in IDE-DMA (I'm planning to submit ep93xx ide driver to linux-ide soon).

    IDE or libata driver? Asking because IDE drivers are not accepted anymore.

WBR, Sergei

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Sergei Shtylyov]

Hello.

On 22-03-2012 14:00, Rafal Prylowski wrote:

>> I haven't looked through the patch yet, since I'm waiting on more
>> information from Mika and Hartley's testing.

>> However, the commit log doesn't tell me why we want this change. Is it a
>> performance improvement? If so, do you have some numbers that we can
>> paste into the commit log?

> In principle, using double buffering should be faster than using only
> one buffer and disabling/enabling channel each time. But my measurements
> doesn't show any significant change.

> The real reason for this change is that current code is not 100% reliable
> in IDE-DMA (I'm planning to submit ep93xx ide driver to linux-ide soon).

    IDE or libata driver? Asking because IDE drivers are not accepted anymore.

WBR, Sergei

Re: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Rafal Prylowski]

On 2012-03-22 14:14, Sergei Shtylyov wrote:
>    IDE or libata driver? Asking because IDE drivers are not accepted anymore.

Libata. Basically, it's the driver posted by Bartlomiej Zolnierkiewicz with
some changes (the most important being UDMA support).

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Rafal Prylowski]

On 2012-03-22 14:14, Sergei Shtylyov wrote:
>    IDE or libata driver? Asking because IDE drivers are not accepted anymore.

Libata. Basically, it's the driver posted by Bartlomiej Zolnierkiewicz with
some changes (the most important being UDMA support).

RE: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Thursday, March 22, 2012 12:38 AM, Mika Westerberg wrote:
> On Wed, Mar 21, 2012 at 07:47:52PM -0500, H Hartley Sweeten wrote:
>> Without this patch I get the "got interrupt while active list is empty" messages
>> but only occasionally. Other than the messages the mmc_spi driver seems to
>> be working ok with dma.
>> 
>> I hacked in a dump of the DMA Global Interrupt register when I get the
>> "got interrupt while active list is empty" messages and get this:
>> 
>> dma dma1chan1: got interrupt while active list is empty (00000000)

Mika,

I hacked this back to the handle_one_vic function. It looks like every time
I get the message above the stat read in that function shows an active
interrupt for DMAM2M0 (the dma rx channel) and DMAM2M1 (the dma
tx channel). Because of the way handle_one_vic processes the irq's, the
DMAM2M0 interrupt gets handled first. When it's done it must be clearing
the DMAM2M1 interrupt automatically for some reason. So, when it's
handled there is no interrupt pending so we get the message.

The "cleanest" fix I can think of is the following patch.  This is against
linux-next.

---

From: H Hartley Sweeten <hsweeten@visionengravers.com>

dma: ep93xx: check for spurious interrupts

The ep93xx dma controller generates spurious interrupts on dma1chan1 when
used with the mmc_spi driver. Catch these early by making sure there is an
interrupt to actually process.

Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>

---

diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c
index e6f133b..3cfabe6 100644
--- a/drivers/dma/ep93xx_dma.c
+++ b/drivers/dma/ep93xx_dma.c
@@ -38,7 +38,7 @@
 #define M2P_CONTROL_ENABLE		BIT(4)
 #define M2P_CONTROL_ICE			BIT(6)
 
-#define M2P_INTERRUPT			0x0004
+#define M2X_INTERRUPT			0x0004	/* M2P and M2M */
 #define M2P_INTERRUPT_STALL		BIT(0)
 #define M2P_INTERRUPT_NFB		BIT(1)
 #define M2P_INTERRUPT_ERROR		BIT(3)
@@ -78,7 +78,6 @@
 #define M2M_CONTROL_NO_HDSK		BIT(24)
 #define M2M_CONTROL_PWSC_SHIFT		25
 
-#define M2M_INTERRUPT			0x0004
 #define M2M_INTERRUPT_DONEINT		BIT(1)
 
 #define M2M_BCR0			0x0010
@@ -187,7 +186,8 @@ struct ep93xx_dma_engine {
 	int			(*hw_setup)(struct ep93xx_dma_chan *);
 	void			(*hw_shutdown)(struct ep93xx_dma_chan *);
 	void			(*hw_submit)(struct ep93xx_dma_chan *);
-	int			(*hw_interrupt)(struct ep93xx_dma_chan *);
+	int			(*hw_interrupt)(struct ep93xx_dma_chan *,
+						u32 irq_status);
 #define INTERRUPT_UNKNOWN	0
 #define INTERRUPT_DONE		1
 #define INTERRUPT_NEXT_BUFFER	2
@@ -376,16 +376,15 @@ static void m2p_hw_submit(struct ep93xx_dma_chan *edmac)
 	m2p_set_control(edmac, control);
 }
 
-static int m2p_hw_interrupt(struct ep93xx_dma_chan *edmac)
+static int m2p_hw_interrupt(struct ep93xx_dma_chan *edmac, u32 irq_status)
 {
-	u32 irq_status = readl(edmac->regs + M2P_INTERRUPT);
 	u32 control;
 
 	if (irq_status & M2P_INTERRUPT_ERROR) {
 		struct ep93xx_dma_desc *desc = ep93xx_dma_get_active(edmac);
 
 		/* Clear the error interrupt */
-		writel(1, edmac->regs + M2P_INTERRUPT);
+		writel(1, edmac->regs + M2X_INTERRUPT);
 
 		/*
 		 * It seems that there is no easy way of reporting errors back
@@ -560,15 +559,15 @@ static void m2m_hw_submit(struct ep93xx_dma_chan *edmac)
 	}
 }
 
-static int m2m_hw_interrupt(struct ep93xx_dma_chan *edmac)
+static int m2m_hw_interrupt(struct ep93xx_dma_chan *edmac, u32 irq_status)
 {
 	u32 control;
 
-	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_DONEINT))
+	if (!(irq_status & M2M_INTERRUPT_DONEINT))
 		return INTERRUPT_UNKNOWN;
 
 	/* Clear the DONE bit */
-	writel(0, edmac->regs + M2M_INTERRUPT);
+	writel(0, edmac->regs + M2X_INTERRUPT);
 
 	/* Disable interrupts and the channel */
 	control = readl(edmac->regs + M2M_CONTROL);
@@ -735,6 +734,12 @@ static irqreturn_t ep93xx_dma_interrupt(int irq, void *dev_id)
 	struct ep93xx_dma_chan *edmac = dev_id;
 	struct ep93xx_dma_desc *desc;
 	irqreturn_t ret = IRQ_HANDLED;
+	u32 irq_status;
+
+	/* Make sure we actually have an interrupt to process */
+	irq_status = readl(edmac->regs + M2X_INTERRUPT);
+	if (!irq_status)
+		return IRQ_NONE;
 
 	spin_lock(&edmac->lock);

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Thursday, March 22, 2012 12:38 AM, Mika Westerberg wrote:
> On Wed, Mar 21, 2012 at 07:47:52PM -0500, H Hartley Sweeten wrote:
>> Without this patch I get the "got interrupt while active list is empty" messages
>> but only occasionally. Other than the messages the mmc_spi driver seems to
>> be working ok with dma.
>> 
>> I hacked in a dump of the DMA Global Interrupt register when I get the
>> "got interrupt while active list is empty" messages and get this:
>> 
>> dma dma1chan1: got interrupt while active list is empty (00000000)

Mika,

I hacked this back to the handle_one_vic function. It looks like every time
I get the message above the stat read in that function shows an active
interrupt for DMAM2M0 (the dma rx channel) and DMAM2M1 (the dma
tx channel). Because of the way handle_one_vic processes the irq's, the
DMAM2M0 interrupt gets handled first. When it's done it must be clearing
the DMAM2M1 interrupt automatically for some reason. So, when it's
handled there is no interrupt pending so we get the message.

The "cleanest" fix I can think of is the following patch.  This is against
linux-next.

---

From: H Hartley Sweeten <hsweeten@visionengravers.com>

dma: ep93xx: check for spurious interrupts

The ep93xx dma controller generates spurious interrupts on dma1chan1 when
used with the mmc_spi driver. Catch these early by making sure there is an
interrupt to actually process.

Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>

---

diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c
index e6f133b..3cfabe6 100644
--- a/drivers/dma/ep93xx_dma.c
+++ b/drivers/dma/ep93xx_dma.c
@@ -38,7 +38,7 @@
 #define M2P_CONTROL_ENABLE		BIT(4)
 #define M2P_CONTROL_ICE			BIT(6)
 
-#define M2P_INTERRUPT			0x0004
+#define M2X_INTERRUPT			0x0004	/* M2P and M2M */
 #define M2P_INTERRUPT_STALL		BIT(0)
 #define M2P_INTERRUPT_NFB		BIT(1)
 #define M2P_INTERRUPT_ERROR		BIT(3)
@@ -78,7 +78,6 @@
 #define M2M_CONTROL_NO_HDSK		BIT(24)
 #define M2M_CONTROL_PWSC_SHIFT		25
 
-#define M2M_INTERRUPT			0x0004
 #define M2M_INTERRUPT_DONEINT		BIT(1)
 
 #define M2M_BCR0			0x0010
@@ -187,7 +186,8 @@ struct ep93xx_dma_engine {
 	int			(*hw_setup)(struct ep93xx_dma_chan *);
 	void			(*hw_shutdown)(struct ep93xx_dma_chan *);
 	void			(*hw_submit)(struct ep93xx_dma_chan *);
-	int			(*hw_interrupt)(struct ep93xx_dma_chan *);
+	int			(*hw_interrupt)(struct ep93xx_dma_chan *,
+						u32 irq_status);
 #define INTERRUPT_UNKNOWN	0
 #define INTERRUPT_DONE		1
 #define INTERRUPT_NEXT_BUFFER	2
@@ -376,16 +376,15 @@ static void m2p_hw_submit(struct ep93xx_dma_chan *edmac)
 	m2p_set_control(edmac, control);
 }
 
-static int m2p_hw_interrupt(struct ep93xx_dma_chan *edmac)
+static int m2p_hw_interrupt(struct ep93xx_dma_chan *edmac, u32 irq_status)
 {
-	u32 irq_status = readl(edmac->regs + M2P_INTERRUPT);
 	u32 control;
 
 	if (irq_status & M2P_INTERRUPT_ERROR) {
 		struct ep93xx_dma_desc *desc = ep93xx_dma_get_active(edmac);
 
 		/* Clear the error interrupt */
-		writel(1, edmac->regs + M2P_INTERRUPT);
+		writel(1, edmac->regs + M2X_INTERRUPT);
 
 		/*
 		 * It seems that there is no easy way of reporting errors back
@@ -560,15 +559,15 @@ static void m2m_hw_submit(struct ep93xx_dma_chan *edmac)
 	}
 }
 
-static int m2m_hw_interrupt(struct ep93xx_dma_chan *edmac)
+static int m2m_hw_interrupt(struct ep93xx_dma_chan *edmac, u32 irq_status)
 {
 	u32 control;
 
-	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_DONEINT))
+	if (!(irq_status & M2M_INTERRUPT_DONEINT))
 		return INTERRUPT_UNKNOWN;
 
 	/* Clear the DONE bit */
-	writel(0, edmac->regs + M2M_INTERRUPT);
+	writel(0, edmac->regs + M2X_INTERRUPT);
 
 	/* Disable interrupts and the channel */
 	control = readl(edmac->regs + M2M_CONTROL);
@@ -735,6 +734,12 @@ static irqreturn_t ep93xx_dma_interrupt(int irq, void *dev_id)
 	struct ep93xx_dma_chan *edmac = dev_id;
 	struct ep93xx_dma_desc *desc;
 	irqreturn_t ret = IRQ_HANDLED;
+	u32 irq_status;
+
+	/* Make sure we actually have an interrupt to process */
+	irq_status = readl(edmac->regs + M2X_INTERRUPT);
+	if (!irq_status)
+		return IRQ_NONE;
 
 	spin_lock(&edmac->lock);

Re: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Mika Westerberg]

On Thu, Mar 22, 2012 at 01:52:52PM -0500, H Hartley Sweeten wrote:

> I hacked this back to the handle_one_vic function. It looks like every time
> I get the message above the stat read in that function shows an active
> interrupt for DMAM2M0 (the dma rx channel) and DMAM2M1 (the dma
> tx channel). Because of the way handle_one_vic processes the irq's, the
> DMAM2M0 interrupt gets handled first. When it's done it must be clearing
> the DMAM2M1 interrupt automatically for some reason. So, when it's
> handled there is no interrupt pending so we get the message.
> 
> The "cleanest" fix I can think of is the following patch.  This is against
> linux-next.

I can't figure out any better fix unless we move ep93xx back to use
!MULTI_IRQ_HANDLER which obviously is not good thing to do.

> From: H Hartley Sweeten <hsweeten@visionengravers.com>
> 
> dma: ep93xx: check for spurious interrupts
> 
> The ep93xx dma controller generates spurious interrupts on dma1chan1 when
> used with the mmc_spi driver. Catch these early by making sure there is an
> interrupt to actually process.
> 
> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>

Tested-by: Mika Westerberg <mika.westerberg@iki.fi>

Thanks for investigating this. You missed one thing though, see below.
Otherwise it doesn't compile :)

diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c
index e1cc87a..614bff4 100644
--- a/drivers/dma/ep93xx_dma.c
+++ b/drivers/dma/ep93xx_dma.c
@@ -751,7 +751,7 @@ static irqreturn_t ep93xx_dma_interrupt(int irq, void *dev_id)
 		return IRQ_NONE;
 	}
 
-	switch (edmac->edma->hw_interrupt(edmac)) {
+	switch (edmac->edma->hw_interrupt(edmac, irq_status)) {
 	case INTERRUPT_DONE:
 		desc->complete = true;
 		tasklet_schedule(&edmac->tasklet);

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Mika Westerberg]

On Thu, Mar 22, 2012 at 01:52:52PM -0500, H Hartley Sweeten wrote:

> I hacked this back to the handle_one_vic function. It looks like every time
> I get the message above the stat read in that function shows an active
> interrupt for DMAM2M0 (the dma rx channel) and DMAM2M1 (the dma
> tx channel). Because of the way handle_one_vic processes the irq's, the
> DMAM2M0 interrupt gets handled first. When it's done it must be clearing
> the DMAM2M1 interrupt automatically for some reason. So, when it's
> handled there is no interrupt pending so we get the message.
> 
> The "cleanest" fix I can think of is the following patch.  This is against
> linux-next.

I can't figure out any better fix unless we move ep93xx back to use
!MULTI_IRQ_HANDLER which obviously is not good thing to do.

> From: H Hartley Sweeten <hsweeten@visionengravers.com>
> 
> dma: ep93xx: check for spurious interrupts
> 
> The ep93xx dma controller generates spurious interrupts on dma1chan1 when
> used with the mmc_spi driver. Catch these early by making sure there is an
> interrupt to actually process.
> 
> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>

Tested-by: Mika Westerberg <mika.westerberg@iki.fi>

Thanks for investigating this. You missed one thing though, see below.
Otherwise it doesn't compile :)

diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c
index e1cc87a..614bff4 100644
--- a/drivers/dma/ep93xx_dma.c
+++ b/drivers/dma/ep93xx_dma.c
@@ -751,7 +751,7 @@ static irqreturn_t ep93xx_dma_interrupt(int irq, void *dev_id)
 		return IRQ_NONE;
 	}
 
-	switch (edmac->edma->hw_interrupt(edmac)) {
+	switch (edmac->edma->hw_interrupt(edmac, irq_status)) {
 	case INTERRUPT_DONE:
 		desc->complete = true;
 		tasklet_schedule(&edmac->tasklet);

RE: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Thursday, March 22, 2012 1:04 PM, Mika Westerberg wrote:
> On Thu, Mar 22, 2012 at 01:52:52PM -0500, H Hartley Sweeten wrote:
>
>> I hacked this back to the handle_one_vic function. It looks like every time
>> I get the message above the stat read in that function shows an active
>> interrupt for DMAM2M0 (the dma rx channel) and DMAM2M1 (the dma
>> tx channel). Because of the way handle_one_vic processes the irq's, the
>> DMAM2M0 interrupt gets handled first. When it's done it must be clearing
>> the DMAM2M1 interrupt automatically for some reason. So, when it's
>> handled there is no interrupt pending so we get the message.
>> 
>> The "cleanest" fix I can think of is the following patch.  This is against
>> linux-next.
>
> I can't figure out any better fix unless we move ep93xx back to use
> !MULTI_IRQ_HANDLER which obviously is not good thing to do.

I agree.

>> From: H Hartley Sweeten <hsweeten@visionengravers.com>
>> 
>> dma: ep93xx: check for spurious interrupts
>> 
>> The ep93xx dma controller generates spurious interrupts on dma1chan1 when
>> used with the mmc_spi driver. Catch these early by making sure there is an
>> interrupt to actually process.
>> 
>> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
>
> Tested-by: Mika Westerberg <mika.westerberg@iki.fi>
>
> Thanks for investigating this. You missed one thing though, see below.
> Otherwise it doesn't compile :)
>
> diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c
> index e1cc87a..614bff4 100644
> --- a/drivers/dma/ep93xx_dma.c
> +++ b/drivers/dma/ep93xx_dma.c
> @@ -751,7 +751,7 @@ static irqreturn_t ep93xx_dma_interrupt(int irq, void *dev_id)
> 		return IRQ_NONE;
>  	}
>  
> -	switch (edmac->edma->hw_interrupt(edmac)) {
> +	switch (edmac->edma->hw_interrupt(edmac, irq_status)) {
> 	case INTERRUPT_DONE:
> 		desc->complete = true;
> 		tasklet_schedule(&edmac->tasklet);

Gah... Misstyped it when I created the patch.

I'll fix it and post a proper patch with your Tested-by.

Thanks,
Hartley

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Thursday, March 22, 2012 1:04 PM, Mika Westerberg wrote:
> On Thu, Mar 22, 2012 at 01:52:52PM -0500, H Hartley Sweeten wrote:
>
>> I hacked this back to the handle_one_vic function. It looks like every time
>> I get the message above the stat read in that function shows an active
>> interrupt for DMAM2M0 (the dma rx channel) and DMAM2M1 (the dma
>> tx channel). Because of the way handle_one_vic processes the irq's, the
>> DMAM2M0 interrupt gets handled first. When it's done it must be clearing
>> the DMAM2M1 interrupt automatically for some reason. So, when it's
>> handled there is no interrupt pending so we get the message.
>> 
>> The "cleanest" fix I can think of is the following patch.  This is against
>> linux-next.
>
> I can't figure out any better fix unless we move ep93xx back to use
> !MULTI_IRQ_HANDLER which obviously is not good thing to do.

I agree.

>> From: H Hartley Sweeten <hsweeten@visionengravers.com>
>> 
>> dma: ep93xx: check for spurious interrupts
>> 
>> The ep93xx dma controller generates spurious interrupts on dma1chan1 when
>> used with the mmc_spi driver. Catch these early by making sure there is an
>> interrupt to actually process.
>> 
>> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
>
> Tested-by: Mika Westerberg <mika.westerberg@iki.fi>
>
> Thanks for investigating this. You missed one thing though, see below.
> Otherwise it doesn't compile :)
>
> diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c
> index e1cc87a..614bff4 100644
> --- a/drivers/dma/ep93xx_dma.c
> +++ b/drivers/dma/ep93xx_dma.c
> @@ -751,7 +751,7 @@ static irqreturn_t ep93xx_dma_interrupt(int irq, void *dev_id)
> 		return IRQ_NONE;
>  	}
>  
> -	switch (edmac->edma->hw_interrupt(edmac)) {
> +	switch (edmac->edma->hw_interrupt(edmac, irq_status)) {
> 	case INTERRUPT_DONE:
> 		desc->complete = true;
> 		tasklet_schedule(&edmac->tasklet);

Gah... Misstyped it when I created the patch.

I'll fix it and post a proper patch with your Tested-by.

Thanks,
Hartley

RE: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Thursday, March 22, 2012 1:04 PM, Mika Westerberg wrote:
> On Thu, Mar 22, 2012 at 01:52:52PM -0500, H Hartley Sweeten wrote:
>
>> I hacked this back to the handle_one_vic function. It looks like every time
>> I get the message above the stat read in that function shows an active
>> interrupt for DMAM2M0 (the dma rx channel) and DMAM2M1 (the dma
>> tx channel). Because of the way handle_one_vic processes the irq's, the
>> DMAM2M0 interrupt gets handled first. When it's done it must be clearing
>> the DMAM2M1 interrupt automatically for some reason. So, when it's
>> handled there is no interrupt pending so we get the message.
>> 
>> The "cleanest" fix I can think of is the following patch.  This is against
>> linux-next.
>
> I can't figure out any better fix unless we move ep93xx back to use
> !MULTI_IRQ_HANDLER which obviously is not good thing to do.

Mika,

I wonder if the spurious interrupts were the real problem behind what
was causing the Oops that prompted your patch:

dma/ep93xx_dma: prevent ep93xx_dma_tasklet() to reference an empty list

???

Hartley

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Thursday, March 22, 2012 1:04 PM, Mika Westerberg wrote:
> On Thu, Mar 22, 2012 at 01:52:52PM -0500, H Hartley Sweeten wrote:
>
>> I hacked this back to the handle_one_vic function. It looks like every time
>> I get the message above the stat read in that function shows an active
>> interrupt for DMAM2M0 (the dma rx channel) and DMAM2M1 (the dma
>> tx channel). Because of the way handle_one_vic processes the irq's, the
>> DMAM2M0 interrupt gets handled first. When it's done it must be clearing
>> the DMAM2M1 interrupt automatically for some reason. So, when it's
>> handled there is no interrupt pending so we get the message.
>> 
>> The "cleanest" fix I can think of is the following patch.  This is against
>> linux-next.
>
> I can't figure out any better fix unless we move ep93xx back to use
> !MULTI_IRQ_HANDLER which obviously is not good thing to do.

Mika,

I wonder if the spurious interrupts were the real problem behind what
was causing the Oops that prompted your patch:

dma/ep93xx_dma: prevent ep93xx_dma_tasklet() to reference an empty list

???

Hartley

RE: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Wednesday, March 21, 2012 12:39 PM, Mika Westerberg wrote:
> On Tue, Mar 20, 2012 at 09:09:26AM +0100, Rafal Prylowski wrote:
>> Implement double buffering for M2M DMA channels.
>> 
>> Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>
>
> The subject line prefix should probably be 'dmaengine/ep93xx_dma: ' but
> otherwise this looks good.
>
> Acked-by: Mika Westerberg <mika.westerberg@iki.fi>

Mika,

Did you test this patch or just review it?

On my system it doesn't work. I think it has something to do with the
changes to m2m_hw_interrupt but I haven't tracked it down yet.

It looks like what's missing is a:

	edmac->edma->hw_submit(edmac);

But, maybe that's not needed with double buffering?

Regards,
Hartley

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Wednesday, March 21, 2012 12:39 PM, Mika Westerberg wrote:
> On Tue, Mar 20, 2012 at 09:09:26AM +0100, Rafal Prylowski wrote:
>> Implement double buffering for M2M DMA channels.
>> 
>> Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>
>
> The subject line prefix should probably be 'dmaengine/ep93xx_dma: ' but
> otherwise this looks good.
>
> Acked-by: Mika Westerberg <mika.westerberg@iki.fi>

Mika,

Did you test this patch or just review it?

On my system it doesn't work. I think it has something to do with the
changes to m2m_hw_interrupt but I haven't tracked it down yet.

It looks like what's missing is a:

	edmac->edma->hw_submit(edmac);

But, maybe that's not needed with double buffering?

Regards,
Hartley

Re: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Mika Westerberg]

On Thu, Mar 22, 2012 at 06:56:30PM -0500, H Hartley Sweeten wrote:

> I wonder if the spurious interrupts were the real problem behind what
> was causing the Oops that prompted your patch:
> 
> dma/ep93xx_dma: prevent ep93xx_dma_tasklet() to reference an empty list

No, it was a different problem spotted by Rafal. It was related to calling
dma_terminate_all() in a interrupt context or something like that.

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Mika Westerberg]

On Thu, Mar 22, 2012 at 06:56:30PM -0500, H Hartley Sweeten wrote:

> I wonder if the spurious interrupts were the real problem behind what
> was causing the Oops that prompted your patch:
> 
> dma/ep93xx_dma: prevent ep93xx_dma_tasklet() to reference an empty list

No, it was a different problem spotted by Rafal. It was related to calling
dma_terminate_all() in a interrupt context or something like that.

Re: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Mika Westerberg]

On Thu, Mar 22, 2012 at 09:19:10PM -0500, H Hartley Sweeten wrote:

> Did you test this patch or just review it?

I tested and reviewed it.

> On my system it doesn't work. I think it has something to do with the
> changes to m2m_hw_interrupt but I haven't tracked it down yet.
> 
> It looks like what's missing is a:
> 
> 	edmac->edma->hw_submit(edmac);
> 
> But, maybe that's not needed with double buffering?

Did you try without your patch and adding my VIC hack? That's what I did when
I first got this patch and I saw no problems. I tested with audio (M2P),
mmc_spi (M2M) and dmatest (M2M).

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Mika Westerberg]

On Thu, Mar 22, 2012 at 09:19:10PM -0500, H Hartley Sweeten wrote:

> Did you test this patch or just review it?

I tested and reviewed it.

> On my system it doesn't work. I think it has something to do with the
> changes to m2m_hw_interrupt but I haven't tracked it down yet.
> 
> It looks like what's missing is a:
> 
> 	edmac->edma->hw_submit(edmac);
> 
> But, maybe that's not needed with double buffering?

Did you try without your patch and adding my VIC hack? That's what I did when
I first got this patch and I saw no problems. I tested with audio (M2P),
mmc_spi (M2M) and dmatest (M2M).

RE: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Friday, March 23, 2012 12:05 AM, Mika Westerberg wrote:
> On Thu, Mar 22, 2012 at 09:19:10PM -0500, H Hartley Sweeten wrote:
>
>> Did you test this patch or just review it?
>
> I tested and reviewed it.

OK. What kernel? My tests so far have been with 3.3.

>> On my system it doesn't work. I think it has something to do with the
>> changes to m2m_hw_interrupt but I haven't tracked it down yet.
>> 
>> It looks like what's missing is a:
>> 
>> 	edmac->edma->hw_submit(edmac);
>> 
>> But, maybe that's not needed with double buffering?
>
> Did you try without your patch and adding my VIC hack? That's what I did when
> I first got this patch and I saw no problems. I tested with audio (M2P),
> mmc_spi (M2M) and dmatest (M2M).

No, I have not applied your VIC hack.

Until we get a working solution, without any hacks, I don't want this to go in.

Regards,
Hartley

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Friday, March 23, 2012 12:05 AM, Mika Westerberg wrote:
> On Thu, Mar 22, 2012 at 09:19:10PM -0500, H Hartley Sweeten wrote:
>
>> Did you test this patch or just review it?
>
> I tested and reviewed it.

OK. What kernel? My tests so far have been with 3.3.

>> On my system it doesn't work. I think it has something to do with the
>> changes to m2m_hw_interrupt but I haven't tracked it down yet.
>> 
>> It looks like what's missing is a:
>> 
>> 	edmac->edma->hw_submit(edmac);
>> 
>> But, maybe that's not needed with double buffering?
>
> Did you try without your patch and adding my VIC hack? That's what I did when
> I first got this patch and I saw no problems. I tested with audio (M2P),
> mmc_spi (M2M) and dmatest (M2M).

No, I have not applied your VIC hack.

Until we get a working solution, without any hacks, I don't want this to go in.

Regards,
Hartley

Re: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Mika Westerberg]

On Fri, Mar 23, 2012 at 11:09:50AM -0500, H Hartley Sweeten wrote:
> On Friday, March 23, 2012 12:05 AM, Mika Westerberg wrote:
> > On Thu, Mar 22, 2012 at 09:19:10PM -0500, H Hartley Sweeten wrote:
> >
> >> Did you test this patch or just review it?
> >
> > I tested and reviewed it.
> 
> OK. What kernel? My tests so far have been with 3.3.

3.3 and -next.

> >> On my system it doesn't work. I think it has something to do with the
> >> changes to m2m_hw_interrupt but I haven't tracked it down yet.
> >> 
> >> It looks like what's missing is a:
> >> 
> >> 	edmac->edma->hw_submit(edmac);
> >> 
> >> But, maybe that's not needed with double buffering?
> >
> > Did you try without your patch and adding my VIC hack? That's what I did when
> > I first got this patch and I saw no problems. I tested with audio (M2P),
> > mmc_spi (M2M) and dmatest (M2M).
> 
> No, I have not applied your VIC hack.

Please try it. The point there was that we have a problem in ep93xx VIC/DMA
code but it is totally different thing and not related to Rafal's patch. We
can sort the VIC thing out separately IMHO. ep93xx dma in 3.3 is broken
regardless this patch.

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Mika Westerberg]

On Fri, Mar 23, 2012 at 11:09:50AM -0500, H Hartley Sweeten wrote:
> On Friday, March 23, 2012 12:05 AM, Mika Westerberg wrote:
> > On Thu, Mar 22, 2012 at 09:19:10PM -0500, H Hartley Sweeten wrote:
> >
> >> Did you test this patch or just review it?
> >
> > I tested and reviewed it.
> 
> OK. What kernel? My tests so far have been with 3.3.

3.3 and -next.

> >> On my system it doesn't work. I think it has something to do with the
> >> changes to m2m_hw_interrupt but I haven't tracked it down yet.
> >> 
> >> It looks like what's missing is a:
> >> 
> >> 	edmac->edma->hw_submit(edmac);
> >> 
> >> But, maybe that's not needed with double buffering?
> >
> > Did you try without your patch and adding my VIC hack? That's what I did when
> > I first got this patch and I saw no problems. I tested with audio (M2P),
> > mmc_spi (M2M) and dmatest (M2M).
> 
> No, I have not applied your VIC hack.

Please try it. The point there was that we have a problem in ep93xx VIC/DMA
code but it is totally different thing and not related to Rafal's patch. We
can sort the VIC thing out separately IMHO. ep93xx dma in 3.3 is broken
regardless this patch.

Re: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Rafal Prylowski]

On 2012-03-24 08:32, Mika Westerberg wrote:
> Please try it. The point there was that we have a problem in ep93xx VIC/DMA
> code but it is totally different thing and not related to Rafal's patch. We
> can sort the VIC thing out separately IMHO. ep93xx dma in 3.3 is broken
> regardless this patch.

I think, that we can't be 100% sure that these two problems are not related
at all. If some changes to VIC/ep93xx interrupt handling etc will be send
to list, I'll test them in my configuration without my patch. If it will
help (to fix the problem I'm observing also in 3.2), then double buffering
for M2M could be much simplier.

Now, I'm trying to test mmc_spi in ep93xx and let you know what the results
are.

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Rafal Prylowski]

On 2012-03-24 08:32, Mika Westerberg wrote:
> Please try it. The point there was that we have a problem in ep93xx VIC/DMA
> code but it is totally different thing and not related to Rafal's patch. We
> can sort the VIC thing out separately IMHO. ep93xx dma in 3.3 is broken
> regardless this patch.

I think, that we can't be 100% sure that these two problems are not related
at all. If some changes to VIC/ep93xx interrupt handling etc will be send
to list, I'll test them in my configuration without my patch. If it will
help (to fix the problem I'm observing also in 3.2), then double buffering
for M2M could be much simplier.

Now, I'm trying to test mmc_spi in ep93xx and let you know what the results
are.

RE: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Saturday, March 24, 2012 12:32 AM, Mika Westerberg wrote:
> On Fri, Mar 23, 2012 at 11:09:50AM -0500, H Hartley Sweeten wrote:
>> No, I have not applied your VIC hack.
>
> Please try it. The point there was that we have a problem in ep93xx VIC/DMA
> code but it is totally different thing and not related to Rafal's patch. We
> can sort the VIC thing out separately IMHO. ep93xx dma in 3.3 is broken
> regardless this patch.

Mika,

I tried you VIC hack. It does seem to work.

I tried doing a bit more debugging with the handle_one_vic function. It
appears that the timer tick is what's causing the spi dma interrupts grief.
I'm just not sure how it's happening or how to fix it...

I modified handle_one_vic to output a message when multiple interrupts
are detected in the stat. Then, if multiple interrupts were detected, to output
a message with the new calculated stat and the actual stat. These "should"
occur one right after the other when multiple interrupts are detected. But
that's not what I'm getting. Here's a sample trace with comments:

handle_one_vic: stat:0x00060000 - handling irq:17 now
	stat shows interrupts 17 and 18
handle_one_vic: stat:0x00040010 - handling irq:4 now
	stat shows interrupts 4 and 18, 17 was handled
handle_one_vic: next stat:0x00040000 - actual stat:0x00040000
	next stat shows interrupt 18, 4 was handled, 18 is pending
handle_one_vic: stat:0x00040000 - handling irq:18 now
	stat shows interrupt 18
handle_one_vic: next stat:0x00000000 - actual stat:0x00000010
	next stat shows no interrupts, 18 was handled, 4 is pending
handle_one_vic: next stat:0x00040000 - actual stat:0x00000000
	next stat shows interrupt 18, it was already handled, none are pending
handle_one_vic: stat:0x00040000 - handling irq:18 now
	stat shows interrupt 18 (which was already handled)
dma dma1chan1: spurious interrupt: status=00002180
	bang... spurious interrupt

It looks like the timer interrupt (4) is causing vic_handle_irq to start
iterating over the VIC's while an iteration is already in progress.  One
of the iterations is handling interrupt 18 correctly but, since the stat
is only read once, the second iteration also tries to handle it.

Any ideas?

Regards,
Hartley

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Saturday, March 24, 2012 12:32 AM, Mika Westerberg wrote:
> On Fri, Mar 23, 2012 at 11:09:50AM -0500, H Hartley Sweeten wrote:
>> No, I have not applied your VIC hack.
>
> Please try it. The point there was that we have a problem in ep93xx VIC/DMA
> code but it is totally different thing and not related to Rafal's patch. We
> can sort the VIC thing out separately IMHO. ep93xx dma in 3.3 is broken
> regardless this patch.

Mika,

I tried you VIC hack. It does seem to work.

I tried doing a bit more debugging with the handle_one_vic function. It
appears that the timer tick is what's causing the spi dma interrupts grief.
I'm just not sure how it's happening or how to fix it...

I modified handle_one_vic to output a message when multiple interrupts
are detected in the stat. Then, if multiple interrupts were detected, to output
a message with the new calculated stat and the actual stat. These "should"
occur one right after the other when multiple interrupts are detected. But
that's not what I'm getting. Here's a sample trace with comments:

handle_one_vic: stat:0x00060000 - handling irq:17 now
	stat shows interrupts 17 and 18
handle_one_vic: stat:0x00040010 - handling irq:4 now
	stat shows interrupts 4 and 18, 17 was handled
handle_one_vic: next stat:0x00040000 - actual stat:0x00040000
	next stat shows interrupt 18, 4 was handled, 18 is pending
handle_one_vic: stat:0x00040000 - handling irq:18 now
	stat shows interrupt 18
handle_one_vic: next stat:0x00000000 - actual stat:0x00000010
	next stat shows no interrupts, 18 was handled, 4 is pending
handle_one_vic: next stat:0x00040000 - actual stat:0x00000000
	next stat shows interrupt 18, it was already handled, none are pending
handle_one_vic: stat:0x00040000 - handling irq:18 now
	stat shows interrupt 18 (which was already handled)
dma dma1chan1: spurious interrupt: status=00002180
	bang... spurious interrupt

It looks like the timer interrupt (4) is causing vic_handle_irq to start
iterating over the VIC's while an iteration is already in progress.  One
of the iterations is handling interrupt 18 correctly but, since the stat
is only read once, the second iteration also tries to handle it.

Any ideas?

Regards,
Hartley

Re: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Mika Westerberg]

On Thu, Mar 29, 2012 at 05:33:49PM -0500, H Hartley Sweeten wrote:

> I tried doing a bit more debugging with the handle_one_vic function. It
> appears that the timer tick is what's causing the spi dma interrupts grief.
> I'm just not sure how it's happening or how to fix it...
> 
> I modified handle_one_vic to output a message when multiple interrupts
> are detected in the stat. Then, if multiple interrupts were detected, to output
> a message with the new calculated stat and the actual stat. These "should"
> occur one right after the other when multiple interrupts are detected. But
> that's not what I'm getting. Here's a sample trace with comments:
> 
> handle_one_vic: stat:0x00060000 - handling irq:17 now
> 	stat shows interrupts 17 and 18
> handle_one_vic: stat:0x00040010 - handling irq:4 now
> 	stat shows interrupts 4 and 18, 17 was handled
> handle_one_vic: next stat:0x00040000 - actual stat:0x00040000
> 	next stat shows interrupt 18, 4 was handled, 18 is pending
> handle_one_vic: stat:0x00040000 - handling irq:18 now
> 	stat shows interrupt 18
> handle_one_vic: next stat:0x00000000 - actual stat:0x00000010
> 	next stat shows no interrupts, 18 was handled, 4 is pending
> handle_one_vic: next stat:0x00040000 - actual stat:0x00000000
> 	next stat shows interrupt 18, it was already handled, none are pending
> handle_one_vic: stat:0x00040000 - handling irq:18 now
> 	stat shows interrupt 18 (which was already handled)
> dma dma1chan1: spurious interrupt: status=00002180
> 	bang... spurious interrupt
> 
> It looks like the timer interrupt (4) is causing vic_handle_irq to start
> iterating over the VIC's while an iteration is already in progress.  One
> of the iterations is handling interrupt 18 correctly but, since the stat
> is only read once, the second iteration also tries to handle it.
>
> Any ideas?

Unfortunately no :-/ I've been investigating this also and so far haven't
found anything which could explain this behaviour. It is good that you found
that the timer interrupt might have something to do with this. I'm going to
add some more debugging code and see if that helps to identify the reason for
this.

It might also be that the ep93xx_dma driver is doing something wrong in its
interrupt handler which causes the DONE bit to stay asserted even though the
first thing it does is to write 0 to M2M_INTERRUPT register which is supposed
to clear the interrupt..

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Mika Westerberg]

On Thu, Mar 29, 2012 at 05:33:49PM -0500, H Hartley Sweeten wrote:

> I tried doing a bit more debugging with the handle_one_vic function. It
> appears that the timer tick is what's causing the spi dma interrupts grief.
> I'm just not sure how it's happening or how to fix it...
> 
> I modified handle_one_vic to output a message when multiple interrupts
> are detected in the stat. Then, if multiple interrupts were detected, to output
> a message with the new calculated stat and the actual stat. These "should"
> occur one right after the other when multiple interrupts are detected. But
> that's not what I'm getting. Here's a sample trace with comments:
> 
> handle_one_vic: stat:0x00060000 - handling irq:17 now
> 	stat shows interrupts 17 and 18
> handle_one_vic: stat:0x00040010 - handling irq:4 now
> 	stat shows interrupts 4 and 18, 17 was handled
> handle_one_vic: next stat:0x00040000 - actual stat:0x00040000
> 	next stat shows interrupt 18, 4 was handled, 18 is pending
> handle_one_vic: stat:0x00040000 - handling irq:18 now
> 	stat shows interrupt 18
> handle_one_vic: next stat:0x00000000 - actual stat:0x00000010
> 	next stat shows no interrupts, 18 was handled, 4 is pending
> handle_one_vic: next stat:0x00040000 - actual stat:0x00000000
> 	next stat shows interrupt 18, it was already handled, none are pending
> handle_one_vic: stat:0x00040000 - handling irq:18 now
> 	stat shows interrupt 18 (which was already handled)
> dma dma1chan1: spurious interrupt: status=00002180
> 	bang... spurious interrupt
> 
> It looks like the timer interrupt (4) is causing vic_handle_irq to start
> iterating over the VIC's while an iteration is already in progress.  One
> of the iterations is handling interrupt 18 correctly but, since the stat
> is only read once, the second iteration also tries to handle it.
>
> Any ideas?

Unfortunately no :-/ I've been investigating this also and so far haven't
found anything which could explain this behaviour. It is good that you found
that the timer interrupt might have something to do with this. I'm going to
add some more debugging code and see if that helps to identify the reason for
this.

It might also be that the ep93xx_dma driver is doing something wrong in its
interrupt handler which causes the DONE bit to stay asserted even though the
first thing it does is to write 0 to M2M_INTERRUPT register which is supposed
to clear the interrupt..

Re: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Mika Westerberg]

On Fri, Mar 23, 2012 at 11:09:50AM -0500, H Hartley Sweeten wrote:
> On Friday, March 23, 2012 12:05 AM, Mika Westerberg wrote:
> > On Thu, Mar 22, 2012 at 09:19:10PM -0500, H Hartley Sweeten wrote:
> >
> >> Did you test this patch or just review it?
> >
> > I tested and reviewed it.
> 
> OK. What kernel? My tests so far have been with 3.3.
> 
> >> On my system it doesn't work. I think it has something to do with the
> >> changes to m2m_hw_interrupt but I haven't tracked it down yet.
> >> 
> >> It looks like what's missing is a:
> >> 
> >> 	edmac->edma->hw_submit(edmac);
> >> 
> >> But, maybe that's not needed with double buffering?
> >
> > Did you try without your patch and adding my VIC hack? That's what I did when
> > I first got this patch and I saw no problems. I tested with audio (M2P),
> > mmc_spi (M2M) and dmatest (M2M).
> 
> No, I have not applied your VIC hack.
> 
> Until we get a working solution, without any hacks, I don't want this to go in.

Now that the spurious interrupts thing with VIC has been sorted out, should we
revisit this patch? Hartley, do you have any objections merging this?

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Mika Westerberg]

On Fri, Mar 23, 2012 at 11:09:50AM -0500, H Hartley Sweeten wrote:
> On Friday, March 23, 2012 12:05 AM, Mika Westerberg wrote:
> > On Thu, Mar 22, 2012 at 09:19:10PM -0500, H Hartley Sweeten wrote:
> >
> >> Did you test this patch or just review it?
> >
> > I tested and reviewed it.
> 
> OK. What kernel? My tests so far have been with 3.3.
> 
> >> On my system it doesn't work. I think it has something to do with the
> >> changes to m2m_hw_interrupt but I haven't tracked it down yet.
> >> 
> >> It looks like what's missing is a:
> >> 
> >> 	edmac->edma->hw_submit(edmac);
> >> 
> >> But, maybe that's not needed with double buffering?
> >
> > Did you try without your patch and adding my VIC hack? That's what I did when
> > I first got this patch and I saw no problems. I tested with audio (M2P),
> > mmc_spi (M2M) and dmatest (M2M).
> 
> No, I have not applied your VIC hack.
> 
> Until we get a working solution, without any hacks, I don't want this to go in.

Now that the spurious interrupts thing with VIC has been sorted out, should we
revisit this patch? Hartley, do you have any objections merging this?

RE: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Tuesday, April 10, 2012 10:29 AM, Mika Westerberg wrote:
> On Fri, Mar 23, 2012 at 11:09:50AM -0500, H Hartley Sweeten wrote:
>> On Friday, March 23, 2012 12:05 AM, Mika Westerberg wrote:
>>> On Thu, Mar 22, 2012 at 09:19:10PM -0500, H Hartley Sweeten wrote:
>>>
>>>> Did you test this patch or just review it?
>>>
>>> I tested and reviewed it.
>> 
>> OK. What kernel? My tests so far have been with 3.3.
>> 
>>>> On my system it doesn't work. I think it has something to do with the
>>>> changes to m2m_hw_interrupt but I haven't tracked it down yet.
>>>> 
>>>> It looks like what's missing is a:
>>>> 
>>>> 	edmac->edma->hw_submit(edmac);
>>>> 
>>>> But, maybe that's not needed with double buffering?
>>>
>>> Did you try without your patch and adding my VIC hack? That's what I did when
>>> I first got this patch and I saw no problems. I tested with audio (M2P),
>>> mmc_spi (M2M) and dmatest (M2M).
>> 
>> No, I have not applied your VIC hack.
>> 
>> Until we get a working solution, without any hacks, I don't want this to go in.
>
> Now that the spurious interrupts thing with VIC has been sorted out, should we
> revisit this patch? Hartley, do you have any objections merging this?

I have not had a chance to look at this with the VIC issue fixed.

Last time I tried testing the double buffering patch my system would not boot
correctly. I'll try to test this again shortly.

Rafal,

Could you repost the latest version of the patch?

Regards,
Hartley

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Tuesday, April 10, 2012 10:29 AM, Mika Westerberg wrote:
> On Fri, Mar 23, 2012 at 11:09:50AM -0500, H Hartley Sweeten wrote:
>> On Friday, March 23, 2012 12:05 AM, Mika Westerberg wrote:
>>> On Thu, Mar 22, 2012 at 09:19:10PM -0500, H Hartley Sweeten wrote:
>>>
>>>> Did you test this patch or just review it?
>>>
>>> I tested and reviewed it.
>> 
>> OK. What kernel? My tests so far have been with 3.3.
>> 
>>>> On my system it doesn't work. I think it has something to do with the
>>>> changes to m2m_hw_interrupt but I haven't tracked it down yet.
>>>> 
>>>> It looks like what's missing is a:
>>>> 
>>>> 	edmac->edma->hw_submit(edmac);
>>>> 
>>>> But, maybe that's not needed with double buffering?
>>>
>>> Did you try without your patch and adding my VIC hack? That's what I did when
>>> I first got this patch and I saw no problems. I tested with audio (M2P),
>>> mmc_spi (M2M) and dmatest (M2M).
>> 
>> No, I have not applied your VIC hack.
>> 
>> Until we get a working solution, without any hacks, I don't want this to go in.
>
> Now that the spurious interrupts thing with VIC has been sorted out, should we
> revisit this patch? Hartley, do you have any objections merging this?

I have not had a chance to look at this with the VIC issue fixed.

Last time I tried testing the double buffering patch my system would not boot
correctly. I'll try to test this again shortly.

Rafal,

Could you repost the latest version of the patch?

Regards,
Hartley

Re: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Rafal Prylowski]

On 2012-04-10 19:55, H Hartley Sweeten wrote:
> On Tuesday, April 10, 2012 10:29 AM, Mika Westerberg wrote:
>> On Fri, Mar 23, 2012 at 11:09:50AM -0500, H Hartley Sweeten wrote:
>>
>> Now that the spurious interrupts thing with VIC has been sorted out, should we
>> revisit this patch? Hartley, do you have any objections merging this?
> 
> I have not had a chance to look at this with the VIC issue fixed.
> 
> Last time I tried testing the double buffering patch my system would not boot
> correctly. I'll try to test this again shortly.
> 
> Rafal,
> 
> Could you repost the latest version of the patch?
> 

Hi,

Here is the patch. It applies to current mainline (3.4rc2). The only thing that
has changed is additional call to ep93xx_dma_advance_active(..) before disabling
the channel.


Implement double buffering for M2M DMA channels.

Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>

Index: linux-2.6/drivers/dma/ep93xx_dma.c
===================================================================
--- linux-2.6.orig/drivers/dma/ep93xx_dma.c
+++ linux-2.6/drivers/dma/ep93xx_dma.c
@@ -71,6 +71,7 @@
 #define M2M_CONTROL_TM_SHIFT		13
 #define M2M_CONTROL_TM_TX		(1 << M2M_CONTROL_TM_SHIFT)
 #define M2M_CONTROL_TM_RX		(2 << M2M_CONTROL_TM_SHIFT)
+#define M2M_CONTROL_NFBINT		BIT(21)
 #define M2M_CONTROL_RSS_SHIFT		22
 #define M2M_CONTROL_RSS_SSPRX		(1 << M2M_CONTROL_RSS_SHIFT)
 #define M2M_CONTROL_RSS_SSPTX		(2 << M2M_CONTROL_RSS_SHIFT)
@@ -79,7 +80,23 @@
 #define M2M_CONTROL_PWSC_SHIFT		25
 
 #define M2M_INTERRUPT			0x0004
-#define M2M_INTERRUPT_DONEINT		BIT(1)
+#define M2M_INTERRUPT_MASK		6
+
+#define M2M_STATUS			0x000c
+#define M2M_STATUS_CTL_SHIFT		1
+#define M2M_STATUS_CTL_IDLE		(0 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_STALL		(1 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_MEMRD		(2 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_MEMWR		(3 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_BWCWAIT		(4 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_MASK		(7 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_BUF_SHIFT		4
+#define M2M_STATUS_BUF_NO		(0 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_BUF_ON		(1 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_BUF_NEXT		(2 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_BUF_MASK		(3 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_DONE			BIT(6)
+
 
 #define M2M_BCR0			0x0010
 #define M2M_BCR1			0x0014
@@ -426,15 +443,6 @@ static int m2p_hw_interrupt(struct ep93x
 
 /*
  * M2M DMA implementation
- *
- * For the M2M transfers we don't use NFB at all. This is because it simply
- * doesn't work well with memcpy transfers. When you submit both buffers it is
- * extremely unlikely that you get an NFB interrupt, but it instead reports
- * DONE interrupt and both buffers are already transferred which means that we
- * weren't able to update the next buffer.
- *
- * So for now we "simulate" NFB by just submitting buffer after buffer
- * without double buffering.
  */
 
 static int m2m_hw_setup(struct ep93xx_dma_chan *edmac)
@@ -543,6 +551,11 @@ static void m2m_hw_submit(struct ep93xx_
 	m2m_fill_desc(edmac);
 	control |= M2M_CONTROL_DONEINT;
 
+	if (ep93xx_dma_advance_active(edmac)) {
+		m2m_fill_desc(edmac);
+		control |= M2M_CONTROL_NFBINT;
+	}
+
 	/*
 	 * Now we can finally enable the channel. For M2M channel this must be
 	 * done _after_ the BCRx registers are programmed.
@@ -560,32 +573,86 @@ static void m2m_hw_submit(struct ep93xx_
 	}
 }
 
+/*
+ * According to EP93xx User's Guide, we should receive DONE interrupt when all
+ * M2M DMA controller transactions complete normally. This is not always the
+ * case - sometimes EP93xx M2M DMA asserts DONE interrupt when the DMA channel
+ * is still running (channel Buffer FSM in DMA_BUF_ON state, and channel
+ * Control FSM in DMA_MEM_RD state, observed at least in IDE-DMA operation).
+ * In effect, disabling the channel when only DONE bit is set could stop
+ * currently running DMA transfer. To avoid this, we use Buffer FSM and
+ * Control FSM to check current state of DMA channel.
+ */
 static int m2m_hw_interrupt(struct ep93xx_dma_chan *edmac)
 {
+	u32 status = readl(edmac->regs + M2M_STATUS);
+	u32 ctl_fsm = status & M2M_STATUS_CTL_MASK;
+	u32 buf_fsm = status & M2M_STATUS_BUF_MASK;
+	bool done = status & M2M_STATUS_DONE;
+	bool last;
 	u32 control;
 
-	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_DONEINT))
+	/* Accept only DONE and NFB interrupts */
+	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_MASK))
 		return INTERRUPT_UNKNOWN;
 
-	/* Clear the DONE bit */
-	writel(0, edmac->regs + M2M_INTERRUPT);
+	if (done)
+		/* Clear the DONE bit */
+		writel(0, edmac->regs + M2M_INTERRUPT);
 
-	/* Disable interrupts and the channel */
-	control = readl(edmac->regs + M2M_CONTROL);
-	control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_ENABLE);
-	writel(control, edmac->regs + M2M_CONTROL);
+	/*
+	 * Check whether we are done with descriptors or not. This, together
+	 * with DMA channel state, determines action to take in interrupt.
+	 */
+	last = list_first_entry(edmac->active.next,
+		struct ep93xx_dma_desc, node)->txd.cookie;
 
 	/*
-	 * Since we only get DONE interrupt we have to find out ourselves
-	 * whether there still is something to process. So we try to advance
-	 * the chain an see whether it succeeds.
+	 * Use M2M DMA Buffer FSM and Control FSM to check current state of
+	 * DMA channel. Using DONE and NFB bits from channel status register
+	 * or bits from channel interrupt register was proven not to be
+	 * reliable.
 	 */
-	if (ep93xx_dma_advance_active(edmac)) {
-		edmac->edma->hw_submit(edmac);
+	if (!last &&
+	    (buf_fsm == M2M_STATUS_BUF_NO ||
+	     buf_fsm == M2M_STATUS_BUF_ON)) {
+		/*
+		 * Two buffers are ready for update when Buffer FSM is in
+		 * DMA_NO_BUF state. Only one buffer can be prepared without
+		 * disabling the channel, or polling the DONE bit.
+		 * To simplify things, always prepare only one buffer.
+		 */
+		ep93xx_dma_advance_active(edmac);
+		m2m_fill_desc(edmac);
+		if (done && !edmac->chan.private) {
+			/* Software trigger for memcpy channel */
+			control = readl(edmac->regs + M2M_CONTROL);
+			control |= M2M_CONTROL_START;
+			writel(control, edmac->regs + M2M_CONTROL);
+		}
 		return INTERRUPT_NEXT_BUFFER;
 	}
 
-	return INTERRUPT_DONE;
+	/*
+	 * Disable the channel only when Buffer FSM is in DMA_NO_BUF state
+	 * and Control FSM is in DMA_STALL state.
+	 */
+	if (last &&
+	    buf_fsm == M2M_STATUS_BUF_NO &&
+	    ctl_fsm == M2M_STATUS_CTL_STALL) {
+		ep93xx_dma_advance_active(edmac);
+		/* Disable interrupts and the channel */
+		control = readl(edmac->regs + M2M_CONTROL);
+		control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_NFBINT
+			    | M2M_CONTROL_ENABLE);
+		writel(control, edmac->regs + M2M_CONTROL);
+		return INTERRUPT_DONE;
+	}
+
+	/*
+	 * Nothing to do this time.
+	 */
+	return INTERRUPT_NEXT_BUFFER;
 }
 
 /*

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Rafal Prylowski]

On 2012-04-10 19:55, H Hartley Sweeten wrote:
> On Tuesday, April 10, 2012 10:29 AM, Mika Westerberg wrote:
>> On Fri, Mar 23, 2012 at 11:09:50AM -0500, H Hartley Sweeten wrote:
>>
>> Now that the spurious interrupts thing with VIC has been sorted out, should we
>> revisit this patch? Hartley, do you have any objections merging this?
> 
> I have not had a chance to look at this with the VIC issue fixed.
> 
> Last time I tried testing the double buffering patch my system would not boot
> correctly. I'll try to test this again shortly.
> 
> Rafal,
> 
> Could you repost the latest version of the patch?
> 

Hi,

Here is the patch. It applies to current mainline (3.4rc2). The only thing that
has changed is additional call to ep93xx_dma_advance_active(..) before disabling
the channel.


Implement double buffering for M2M DMA channels.

Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>

Index: linux-2.6/drivers/dma/ep93xx_dma.c
===================================================================
--- linux-2.6.orig/drivers/dma/ep93xx_dma.c
+++ linux-2.6/drivers/dma/ep93xx_dma.c
@@ -71,6 +71,7 @@
 #define M2M_CONTROL_TM_SHIFT		13
 #define M2M_CONTROL_TM_TX		(1 << M2M_CONTROL_TM_SHIFT)
 #define M2M_CONTROL_TM_RX		(2 << M2M_CONTROL_TM_SHIFT)
+#define M2M_CONTROL_NFBINT		BIT(21)
 #define M2M_CONTROL_RSS_SHIFT		22
 #define M2M_CONTROL_RSS_SSPRX		(1 << M2M_CONTROL_RSS_SHIFT)
 #define M2M_CONTROL_RSS_SSPTX		(2 << M2M_CONTROL_RSS_SHIFT)
@@ -79,7 +80,23 @@
 #define M2M_CONTROL_PWSC_SHIFT		25
 
 #define M2M_INTERRUPT			0x0004
-#define M2M_INTERRUPT_DONEINT		BIT(1)
+#define M2M_INTERRUPT_MASK		6
+
+#define M2M_STATUS			0x000c
+#define M2M_STATUS_CTL_SHIFT		1
+#define M2M_STATUS_CTL_IDLE		(0 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_STALL		(1 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_MEMRD		(2 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_MEMWR		(3 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_BWCWAIT		(4 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_MASK		(7 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_BUF_SHIFT		4
+#define M2M_STATUS_BUF_NO		(0 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_BUF_ON		(1 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_BUF_NEXT		(2 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_BUF_MASK		(3 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_DONE			BIT(6)
+
 
 #define M2M_BCR0			0x0010
 #define M2M_BCR1			0x0014
@@ -426,15 +443,6 @@ static int m2p_hw_interrupt(struct ep93x
 
 /*
  * M2M DMA implementation
- *
- * For the M2M transfers we don't use NFB at all. This is because it simply
- * doesn't work well with memcpy transfers. When you submit both buffers it is
- * extremely unlikely that you get an NFB interrupt, but it instead reports
- * DONE interrupt and both buffers are already transferred which means that we
- * weren't able to update the next buffer.
- *
- * So for now we "simulate" NFB by just submitting buffer after buffer
- * without double buffering.
  */
 
 static int m2m_hw_setup(struct ep93xx_dma_chan *edmac)
@@ -543,6 +551,11 @@ static void m2m_hw_submit(struct ep93xx_
 	m2m_fill_desc(edmac);
 	control |= M2M_CONTROL_DONEINT;
 
+	if (ep93xx_dma_advance_active(edmac)) {
+		m2m_fill_desc(edmac);
+		control |= M2M_CONTROL_NFBINT;
+	}
+
 	/*
 	 * Now we can finally enable the channel. For M2M channel this must be
 	 * done _after_ the BCRx registers are programmed.
@@ -560,32 +573,86 @@ static void m2m_hw_submit(struct ep93xx_
 	}
 }
 
+/*
+ * According to EP93xx User's Guide, we should receive DONE interrupt when all
+ * M2M DMA controller transactions complete normally. This is not always the
+ * case - sometimes EP93xx M2M DMA asserts DONE interrupt when the DMA channel
+ * is still running (channel Buffer FSM in DMA_BUF_ON state, and channel
+ * Control FSM in DMA_MEM_RD state, observed@least in IDE-DMA operation).
+ * In effect, disabling the channel when only DONE bit is set could stop
+ * currently running DMA transfer. To avoid this, we use Buffer FSM and
+ * Control FSM to check current state of DMA channel.
+ */
 static int m2m_hw_interrupt(struct ep93xx_dma_chan *edmac)
 {
+	u32 status = readl(edmac->regs + M2M_STATUS);
+	u32 ctl_fsm = status & M2M_STATUS_CTL_MASK;
+	u32 buf_fsm = status & M2M_STATUS_BUF_MASK;
+	bool done = status & M2M_STATUS_DONE;
+	bool last;
 	u32 control;
 
-	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_DONEINT))
+	/* Accept only DONE and NFB interrupts */
+	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_MASK))
 		return INTERRUPT_UNKNOWN;
 
-	/* Clear the DONE bit */
-	writel(0, edmac->regs + M2M_INTERRUPT);
+	if (done)
+		/* Clear the DONE bit */
+		writel(0, edmac->regs + M2M_INTERRUPT);
 
-	/* Disable interrupts and the channel */
-	control = readl(edmac->regs + M2M_CONTROL);
-	control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_ENABLE);
-	writel(control, edmac->regs + M2M_CONTROL);
+	/*
+	 * Check whether we are done with descriptors or not. This, together
+	 * with DMA channel state, determines action to take in interrupt.
+	 */
+	last = list_first_entry(edmac->active.next,
+		struct ep93xx_dma_desc, node)->txd.cookie;
 
 	/*
-	 * Since we only get DONE interrupt we have to find out ourselves
-	 * whether there still is something to process. So we try to advance
-	 * the chain an see whether it succeeds.
+	 * Use M2M DMA Buffer FSM and Control FSM to check current state of
+	 * DMA channel. Using DONE and NFB bits from channel status register
+	 * or bits from channel interrupt register was proven not to be
+	 * reliable.
 	 */
-	if (ep93xx_dma_advance_active(edmac)) {
-		edmac->edma->hw_submit(edmac);
+	if (!last &&
+	    (buf_fsm == M2M_STATUS_BUF_NO ||
+	     buf_fsm == M2M_STATUS_BUF_ON)) {
+		/*
+		 * Two buffers are ready for update when Buffer FSM is in
+		 * DMA_NO_BUF state. Only one buffer can be prepared without
+		 * disabling the channel, or polling the DONE bit.
+		 * To simplify things, always prepare only one buffer.
+		 */
+		ep93xx_dma_advance_active(edmac);
+		m2m_fill_desc(edmac);
+		if (done && !edmac->chan.private) {
+			/* Software trigger for memcpy channel */
+			control = readl(edmac->regs + M2M_CONTROL);
+			control |= M2M_CONTROL_START;
+			writel(control, edmac->regs + M2M_CONTROL);
+		}
 		return INTERRUPT_NEXT_BUFFER;
 	}
 
-	return INTERRUPT_DONE;
+	/*
+	 * Disable the channel only when Buffer FSM is in DMA_NO_BUF state
+	 * and Control FSM is in DMA_STALL state.
+	 */
+	if (last &&
+	    buf_fsm == M2M_STATUS_BUF_NO &&
+	    ctl_fsm == M2M_STATUS_CTL_STALL) {
+		ep93xx_dma_advance_active(edmac);
+		/* Disable interrupts and the channel */
+		control = readl(edmac->regs + M2M_CONTROL);
+		control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_NFBINT
+			    | M2M_CONTROL_ENABLE);
+		writel(control, edmac->regs + M2M_CONTROL);
+		return INTERRUPT_DONE;
+	}
+
+	/*
+	 * Nothing to do this time.
+	 */
+	return INTERRUPT_NEXT_BUFFER;
 }
 
 /*

RE: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Wednesday, April 11, 2012 12:19 AM, Rafal Prylowski wrote:
> On 2012-04-10 19:55, H Hartley Sweeten wrote:
>> On Tuesday, April 10, 2012 10:29 AM, Mika Westerberg wrote:
>>> On Fri, Mar 23, 2012 at 11:09:50AM -0500, H Hartley Sweeten wrote:
>>>
>>> Now that the spurious interrupts thing with VIC has been sorted out, should we
>>> revisit this patch? Hartley, do you have any objections merging this?
> >
>> I have not had a chance to look at this with the VIC issue fixed.
>> 
>> Last time I tried testing the double buffering patch my system would not boot
>> correctly. I'll try to test this again shortly.
>> 
>> Rafal,
>> 
>> Could you repost the latest version of the patch?
>> 
>
> Hi,
>
> Here is the patch. It applies to current mainline (3.4rc2). The only thing that
> has changed is additional call to ep93xx_dma_advance_active(..) before disabling
> the channel.

Rafal,

This patch still doesn't work on my ep93xx system.

FWIW, I have two devices on my SPI bus:

static struct spi_board_info vision_spi_board_info[] __initdata = {
	{
		.modalias		= "sst25l",
		.platform_data		= &vision_spi_flash_data,
		.controller_data	= &vision_spi_flash_hw,
		.max_speed_hz		= 20000000,
		.bus_num		= 0,
		.chip_select		= 0,
		.mode			= SPI_MODE_3,
	}, {
		.modalias		= "mmc_spi",
		.platform_data		= &vision_spi_mmc_data,
		.controller_data	= &vision_spi_mmc_hw,
		.max_speed_hz		= 20000000,
		.bus_num		= 0,
		.chip_select		= 1,
		.mode			= SPI_MODE_3,
	},
};

static struct ep93xx_spi_info vision_spi_master __initdata = {
	.num_chipselect		= ARRAY_SIZE(vision_spi_board_info),
	.use_dma		= true,
};

With your patch applied I get these relevant messages during boot:

sst25l spi0.0: sst25lf040a (512 KiB)
Creating 3 MTD partitions on "SPI Flash":
0x000000000000-0x000000001000 : "SPI bootstrap"
0x000000001000-0x000000002000 : "Bootstrap config"
0x000000002000-0x000000080000 : "System config"
ep93xx-spi ep93xx-spi.0: EP93xx SPI Controller at 0x808a0000 irq 53
mmc_spi spi0.1: SD/MMC host mmc0, no poweroff

And /proc/interrupts shows this right after boot:

# cat /proc/interrupts
           CPU0
  4:       4519       VIC  ep93xx timer
 17:    1268725       VIC  ep93xx-spi-rx
 18:    1277244       VIC  ep93xx-spi-tx
 29:          0       VIC  ep93xx-keypad
 33:          0       VIC  rtc-isl1208
 39:        176       VIC  eth0
 52:        112       VIC  uart-pl010
 53:          1       VIC  ep93xx-spi
 56:          0       VIC  ohci_hcd:usb1
 79:          0      GPIO  mmc_spi:cd
 87:          0      GPIO  0-0074

Without your patch I get this:

sst25l spi0.0: sst25lf040a (512 KiB)
Creating 3 MTD partitions on "SPI Flash":
0x000000000000-0x000000001000 : "SPI bootstrap"
0x000000001000-0x000000002000 : "Bootstrap config"
0x000000002000-0x000000080000 : "System config"
ep93xx-spi ep93xx-spi.0: EP93xx SPI Controller at 0x808a0000 irq 53
mmc_spi spi0.1: SD/MMC host mmc0, no poweroff
mmc0: new SD card on SPI
mmcblk0: mmc0:0000 S064B 60.6 MiB
mmcblk0: p1

# cat /proc/interrupts
           CPU0
  4:       1794       VIC  ep93xx timer
 17:        141       VIC  ep93xx-spi-rx
 18:        141       VIC  ep93xx-spi-tx
 29:          0       VIC  ep93xx-keypad
 33:          0       VIC  rtc-isl1208
 39:        100       VIC  eth0
 52:        109       VIC  uart-pl010
 53:        418       VIC  ep93xx-spi
 56:          0       VIC  ohci_hcd:usb1
 79:          0      GPIO  mmc_spi:cd
 87:          0      GPIO  0-0074
Err:          0

It appears your patch is causing an interrupt storm on my system.

Regards,
Hartley

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Wednesday, April 11, 2012 12:19 AM, Rafal Prylowski wrote:
> On 2012-04-10 19:55, H Hartley Sweeten wrote:
>> On Tuesday, April 10, 2012 10:29 AM, Mika Westerberg wrote:
>>> On Fri, Mar 23, 2012 at 11:09:50AM -0500, H Hartley Sweeten wrote:
>>>
>>> Now that the spurious interrupts thing with VIC has been sorted out, should we
>>> revisit this patch? Hartley, do you have any objections merging this?
> >
>> I have not had a chance to look at this with the VIC issue fixed.
>> 
>> Last time I tried testing the double buffering patch my system would not boot
>> correctly. I'll try to test this again shortly.
>> 
>> Rafal,
>> 
>> Could you repost the latest version of the patch?
>> 
>
> Hi,
>
> Here is the patch. It applies to current mainline (3.4rc2). The only thing that
> has changed is additional call to ep93xx_dma_advance_active(..) before disabling
> the channel.

Rafal,

This patch still doesn't work on my ep93xx system.

FWIW, I have two devices on my SPI bus:

static struct spi_board_info vision_spi_board_info[] __initdata = {
	{
		.modalias		= "sst25l",
		.platform_data		= &vision_spi_flash_data,
		.controller_data	= &vision_spi_flash_hw,
		.max_speed_hz		= 20000000,
		.bus_num		= 0,
		.chip_select		= 0,
		.mode			= SPI_MODE_3,
	}, {
		.modalias		= "mmc_spi",
		.platform_data		= &vision_spi_mmc_data,
		.controller_data	= &vision_spi_mmc_hw,
		.max_speed_hz		= 20000000,
		.bus_num		= 0,
		.chip_select		= 1,
		.mode			= SPI_MODE_3,
	},
};

static struct ep93xx_spi_info vision_spi_master __initdata = {
	.num_chipselect		= ARRAY_SIZE(vision_spi_board_info),
	.use_dma		= true,
};

With your patch applied I get these relevant messages during boot:

sst25l spi0.0: sst25lf040a (512 KiB)
Creating 3 MTD partitions on "SPI Flash":
0x000000000000-0x000000001000 : "SPI bootstrap"
0x000000001000-0x000000002000 : "Bootstrap config"
0x000000002000-0x000000080000 : "System config"
ep93xx-spi ep93xx-spi.0: EP93xx SPI Controller at 0x808a0000 irq 53
mmc_spi spi0.1: SD/MMC host mmc0, no poweroff

And /proc/interrupts shows this right after boot:

# cat /proc/interrupts
           CPU0
  4:       4519       VIC  ep93xx timer
 17:    1268725       VIC  ep93xx-spi-rx
 18:    1277244       VIC  ep93xx-spi-tx
 29:          0       VIC  ep93xx-keypad
 33:          0       VIC  rtc-isl1208
 39:        176       VIC  eth0
 52:        112       VIC  uart-pl010
 53:          1       VIC  ep93xx-spi
 56:          0       VIC  ohci_hcd:usb1
 79:          0      GPIO  mmc_spi:cd
 87:          0      GPIO  0-0074

Without your patch I get this:

sst25l spi0.0: sst25lf040a (512 KiB)
Creating 3 MTD partitions on "SPI Flash":
0x000000000000-0x000000001000 : "SPI bootstrap"
0x000000001000-0x000000002000 : "Bootstrap config"
0x000000002000-0x000000080000 : "System config"
ep93xx-spi ep93xx-spi.0: EP93xx SPI Controller at 0x808a0000 irq 53
mmc_spi spi0.1: SD/MMC host mmc0, no poweroff
mmc0: new SD card on SPI
mmcblk0: mmc0:0000 S064B 60.6 MiB
mmcblk0: p1

# cat /proc/interrupts
           CPU0
  4:       1794       VIC  ep93xx timer
 17:        141       VIC  ep93xx-spi-rx
 18:        141       VIC  ep93xx-spi-tx
 29:          0       VIC  ep93xx-keypad
 33:          0       VIC  rtc-isl1208
 39:        100       VIC  eth0
 52:        109       VIC  uart-pl010
 53:        418       VIC  ep93xx-spi
 56:          0       VIC  ohci_hcd:usb1
 79:          0      GPIO  mmc_spi:cd
 87:          0      GPIO  0-0074
Err:          0

It appears your patch is causing an interrupt storm on my system.

Regards,
Hartley

Re: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Rafal Prylowski]

On 2012-04-16 20:59, H Hartley Sweeten wrote:
> On Wednesday, April 11, 2012 12:19 AM, Rafal Prylowski wrote:
> 
> Rafal,
> 
> This patch still doesn't work on my ep93xx system.

<snip>

> It appears your patch is causing an interrupt storm on my system.
> 

Could you please apply the following patch on top of double buffering
patch? I would like to know the state of dma channel when you get
that interrupt storm.

Thanks,
RP

Index: linux-2.6/drivers/dma/ep93xx_dma.c
===================================================================
--- linux-2.6.orig/drivers/dma/ep93xx_dma.c
+++ linux-2.6/drivers/dma/ep93xx_dma.c
@@ -606,6 +606,7 @@ static int m2m_hw_interrupt(struct ep93x
 	 */
 	last = list_first_entry(edmac->active.next,
 		struct ep93xx_dma_desc, node)->txd.cookie;
+	printk("M2M: %x %s\n", status, last ? "last" : "");
 
 	/*
 	 * Use M2M DMA Buffer FSM and Control FSM to check current state of

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Rafal Prylowski]

On 2012-04-16 20:59, H Hartley Sweeten wrote:
> On Wednesday, April 11, 2012 12:19 AM, Rafal Prylowski wrote:
> 
> Rafal,
> 
> This patch still doesn't work on my ep93xx system.

<snip>

> It appears your patch is causing an interrupt storm on my system.
> 

Could you please apply the following patch on top of double buffering
patch? I would like to know the state of dma channel when you get
that interrupt storm.

Thanks,
RP

Index: linux-2.6/drivers/dma/ep93xx_dma.c
===================================================================
--- linux-2.6.orig/drivers/dma/ep93xx_dma.c
+++ linux-2.6/drivers/dma/ep93xx_dma.c
@@ -606,6 +606,7 @@ static int m2m_hw_interrupt(struct ep93x
 	 */
 	last = list_first_entry(edmac->active.next,
 		struct ep93xx_dma_desc, node)->txd.cookie;
+	printk("M2M: %x %s\n", status, last ? "last" : "");
 
 	/*
 	 * Use M2M DMA Buffer FSM and Control FSM to check current state of

RE: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Tuesday, April 17, 2012 12:16 AM, Rafal Prylowski wrote:
> On 2012-04-16 20:59, H Hartley Sweeten wrote:
>> It appears your patch is causing an interrupt storm on my system.
>> 
>
> Could you please apply the following patch on top of double buffering
> patch? I would like to know the state of dma channel when you get
> that interrupt storm.

Rafal,

Here's the output:

mmc_spi spi0.1: SD/MMC host mmc0, no poweroff
M2M: 20c3
M2M: 20c3
M2M: 21c3
M2M: 21c3
M2M: 21c3
M2M: 21c3
M2M: 21c3
M2M: 21c3

The "M2M: 21c3" keeps getting output until the system is turned off.

Regards,
Hartley

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Tuesday, April 17, 2012 12:16 AM, Rafal Prylowski wrote:
> On 2012-04-16 20:59, H Hartley Sweeten wrote:
>> It appears your patch is causing an interrupt storm on my system.
>> 
>
> Could you please apply the following patch on top of double buffering
> patch? I would like to know the state of dma channel when you get
> that interrupt storm.

Rafal,

Here's the output:

mmc_spi spi0.1: SD/MMC host mmc0, no poweroff
M2M: 20c3
M2M: 20c3
M2M: 21c3
M2M: 21c3
M2M: 21c3
M2M: 21c3
M2M: 21c3
M2M: 21c3

The "M2M: 21c3" keeps getting output until the system is turned off.

Regards,
Hartley

RE: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Tuesday, April 17, 2012 8:46 AM, H Hartley Sweeten wrote:
> On Tuesday, April 17, 2012 12:16 AM, Rafal Prylowski wrote:
>> On 2012-04-16 20:59, H Hartley Sweeten wrote:
>>> It appears your patch is causing an interrupt storm on my system.
>>> 
>>
>> Could you please apply the following patch on top of double buffering
>> patch? I would like to know the state of dma channel when you get
>> that interrupt storm.
>
> Rafal,
>
> Here's the output:
>
> mmc_spi spi0.1: SD/MMC host mmc0, no poweroff
> M2M: 20c3
> M2M: 20c3
> M2M: 21c3
> M2M: 21c3
> M2M: 21c3
> M2M: 21c3
> M2M: 21c3
> M2M: 21c3
>
> The "M2M: 21c3" keeps getting output until the system is turned off.

Rafal,

It appears that the M2M_CONTROL_NFBINT bit is never getting set when
dma is used with the spi-ep93xx.c and mmc_spi.c drivers.

I added a prink in msm_hw_submit():

	if (ep93xx_dma_advance_active(edmac)) {
		m2m_fill_desc(edmac);
		control |= M2M_CONTROL_NFBINT;
		printk("%s: NFB enabled\n", __func__);
	}

This message is never displayed.

And for some reason the txd.cookie is not getting set correctly to detect
the last entry in m2m_hw_interrupt.

	/*
	 * Check whether we are done with descriptors or not. This, together
	 * with DMA channel state, determines action to take in interrupt.
	 */
	last = list_first_entry(edmac->active.next,
		struct ep93xx_dma_desc, node)->txd.cookie;

This is causing the code for the INTERRUPT_NEXT_BUFFER to be executed
even though a "next" buffer does not exist.

I think your handling logic in m2m_hw_interrupt needs a bit more work.

I hacked it to test and this appears to work but it's not optimal...

Here's the whole function:


static int m2m_hw_interrupt(struct ep93xx_dma_chan *edmac)
{
	u32 status = readl(edmac->regs + M2M_STATUS);
	u32 ctl_fsm = status & M2M_STATUS_CTL_MASK;
	u32 buf_fsm = status & M2M_STATUS_BUF_MASK;
	bool done = status & M2M_STATUS_DONE;
	bool last;
	u32 control;

	/* Accept only DONE and NFB interrupts */
	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_MASK))
		return INTERRUPT_UNKNOWN;

	if (done)
		/* Clear the DONE bit */
		writel(0, edmac->regs + M2M_INTERRUPT);

	/*
	 * Check whether we are done with descriptors or not. This, together
	 * with DMA channel state, determines action to take in interrupt.
	 */
	last = list_first_entry(edmac->active.next,
		struct ep93xx_dma_desc, node)->txd.cookie;

	/*
	 * Use M2M DMA Buffer FSM and Control FSM to check current state of
	 * DMA channel. Using DONE and NFB bits from channel status register
	 * or bits from channel interrupt register was proven not to be
	 * reliable.
	 */
	if (!last &&
	    (buf_fsm == M2M_STATUS_BUF_NO ||
	     buf_fsm == M2M_STATUS_BUF_ON)) {
		/*
		 * Two buffers are ready for update when Buffer FSM is in
		 * DMA_NO_BUF state. Only one buffer can be prepared without
		 * disabling the channel, or polling the DONE bit.
		 * To simplify things, always prepare only one buffer.
		 */
		if (ep93xx_dma_advance_active(edmac)) {
			m2m_fill_desc(edmac);
			if (done && !edmac->chan.private) {
				/* Software trigger for memcpy channel */
				control = readl(edmac->regs + M2M_CONTROL);
				control |= M2M_CONTROL_START;
				writel(control, edmac->regs + M2M_CONTROL);
			}
			return INTERRUPT_NEXT_BUFFER;
		} else {
			/*
			 * HACK: We don't have another buffer to prepare.
			 * Just disable the channel.
			 */
			control = readl(edmac->regs + M2M_CONTROL);
			control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_NFBINT
				| M2M_CONTROL_ENABLE);
			writel(control, edmac->regs + M2M_CONTROL);
			return INTERRUPT_DONE;
		}
	}

	/*
	 * Disable the channel only when Buffer FSM is in DMA_NO_BUF state
	 * and Control FSM is in DMA_STALL state.
	 */
	if (last &&
	    buf_fsm == M2M_STATUS_BUF_NO &&
	    ctl_fsm == M2M_STATUS_CTL_STALL) {
		ep93xx_dma_advance_active(edmac);
		/* Disable interrupts and the channel */
		control = readl(edmac->regs + M2M_CONTROL);
		control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_NFBINT
			    | M2M_CONTROL_ENABLE);
		writel(control, edmac->regs + M2M_CONTROL);
		return INTERRUPT_DONE;
	}

	/*
	 * Nothing to do this time.
	 */
	return INTERRUPT_NEXT_BUFFER;
}


With this I am able to boot my system and use the mmc card.

Regards,
Hartley

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Tuesday, April 17, 2012 8:46 AM, H Hartley Sweeten wrote:
> On Tuesday, April 17, 2012 12:16 AM, Rafal Prylowski wrote:
>> On 2012-04-16 20:59, H Hartley Sweeten wrote:
>>> It appears your patch is causing an interrupt storm on my system.
>>> 
>>
>> Could you please apply the following patch on top of double buffering
>> patch? I would like to know the state of dma channel when you get
>> that interrupt storm.
>
> Rafal,
>
> Here's the output:
>
> mmc_spi spi0.1: SD/MMC host mmc0, no poweroff
> M2M: 20c3
> M2M: 20c3
> M2M: 21c3
> M2M: 21c3
> M2M: 21c3
> M2M: 21c3
> M2M: 21c3
> M2M: 21c3
>
> The "M2M: 21c3" keeps getting output until the system is turned off.

Rafal,

It appears that the M2M_CONTROL_NFBINT bit is never getting set when
dma is used with the spi-ep93xx.c and mmc_spi.c drivers.

I added a prink in msm_hw_submit():

	if (ep93xx_dma_advance_active(edmac)) {
		m2m_fill_desc(edmac);
		control |= M2M_CONTROL_NFBINT;
		printk("%s: NFB enabled\n", __func__);
	}

This message is never displayed.

And for some reason the txd.cookie is not getting set correctly to detect
the last entry in m2m_hw_interrupt.

	/*
	 * Check whether we are done with descriptors or not. This, together
	 * with DMA channel state, determines action to take in interrupt.
	 */
	last = list_first_entry(edmac->active.next,
		struct ep93xx_dma_desc, node)->txd.cookie;

This is causing the code for the INTERRUPT_NEXT_BUFFER to be executed
even though a "next" buffer does not exist.

I think your handling logic in m2m_hw_interrupt needs a bit more work.

I hacked it to test and this appears to work but it's not optimal...

Here's the whole function:


static int m2m_hw_interrupt(struct ep93xx_dma_chan *edmac)
{
	u32 status = readl(edmac->regs + M2M_STATUS);
	u32 ctl_fsm = status & M2M_STATUS_CTL_MASK;
	u32 buf_fsm = status & M2M_STATUS_BUF_MASK;
	bool done = status & M2M_STATUS_DONE;
	bool last;
	u32 control;

	/* Accept only DONE and NFB interrupts */
	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_MASK))
		return INTERRUPT_UNKNOWN;

	if (done)
		/* Clear the DONE bit */
		writel(0, edmac->regs + M2M_INTERRUPT);

	/*
	 * Check whether we are done with descriptors or not. This, together
	 * with DMA channel state, determines action to take in interrupt.
	 */
	last = list_first_entry(edmac->active.next,
		struct ep93xx_dma_desc, node)->txd.cookie;

	/*
	 * Use M2M DMA Buffer FSM and Control FSM to check current state of
	 * DMA channel. Using DONE and NFB bits from channel status register
	 * or bits from channel interrupt register was proven not to be
	 * reliable.
	 */
	if (!last &&
	    (buf_fsm == M2M_STATUS_BUF_NO ||
	     buf_fsm == M2M_STATUS_BUF_ON)) {
		/*
		 * Two buffers are ready for update when Buffer FSM is in
		 * DMA_NO_BUF state. Only one buffer can be prepared without
		 * disabling the channel, or polling the DONE bit.
		 * To simplify things, always prepare only one buffer.
		 */
		if (ep93xx_dma_advance_active(edmac)) {
			m2m_fill_desc(edmac);
			if (done && !edmac->chan.private) {
				/* Software trigger for memcpy channel */
				control = readl(edmac->regs + M2M_CONTROL);
				control |= M2M_CONTROL_START;
				writel(control, edmac->regs + M2M_CONTROL);
			}
			return INTERRUPT_NEXT_BUFFER;
		} else {
			/*
			 * HACK: We don't have another buffer to prepare.
			 * Just disable the channel.
			 */
			control = readl(edmac->regs + M2M_CONTROL);
			control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_NFBINT
				| M2M_CONTROL_ENABLE);
			writel(control, edmac->regs + M2M_CONTROL);
			return INTERRUPT_DONE;
		}
	}

	/*
	 * Disable the channel only when Buffer FSM is in DMA_NO_BUF state
	 * and Control FSM is in DMA_STALL state.
	 */
	if (last &&
	    buf_fsm == M2M_STATUS_BUF_NO &&
	    ctl_fsm == M2M_STATUS_CTL_STALL) {
		ep93xx_dma_advance_active(edmac);
		/* Disable interrupts and the channel */
		control = readl(edmac->regs + M2M_CONTROL);
		control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_NFBINT
			    | M2M_CONTROL_ENABLE);
		writel(control, edmac->regs + M2M_CONTROL);
		return INTERRUPT_DONE;
	}

	/*
	 * Nothing to do this time.
	 */
	return INTERRUPT_NEXT_BUFFER;
}


With this I am able to boot my system and use the mmc card.

Regards,
Hartley

Re: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Rafal Prylowski]

On 2012-04-17 22:51, H Hartley Sweeten wrote:
> On Tuesday, April 17, 2012 8:46 AM, H Hartley Sweeten wrote:
>>
>> Rafal,
>>
>> Here's the output:
>>
>> mmc_spi spi0.1: SD/MMC host mmc0, no poweroff
>> M2M: 20c3
>> M2M: 20c3
>> M2M: 21c3
>> M2M: 21c3
>> M2M: 21c3
>> M2M: 21c3
>> M2M: 21c3
>> M2M: 21c3
>>
>> The "M2M: 21c3" keeps getting output until the system is turned off.
> 
> Rafal,
> 
> It appears that the M2M_CONTROL_NFBINT bit is never getting set when
> dma is used with the spi-ep93xx.c and mmc_spi.c drivers.
> 
> I added a prink in msm_hw_submit():
> 
> 	if (ep93xx_dma_advance_active(edmac)) {
> 		m2m_fill_desc(edmac);
> 		control |= M2M_CONTROL_NFBINT;
> 		printk("%s: NFB enabled\n", __func__);
> 	}
> 
> This message is never displayed.
> 
> And for some reason the txd.cookie is not getting set correctly to detect
> the last entry in m2m_hw_interrupt.
> 
> 	/*
> 	 * Check whether we are done with descriptors or not. This, together
> 	 * with DMA channel state, determines action to take in interrupt.
> 	 */
> 	last = list_first_entry(edmac->active.next,
> 		struct ep93xx_dma_desc, node)->txd.cookie;
> 
> This is causing the code for the INTERRUPT_NEXT_BUFFER to be executed
> even though a "next" buffer does not exist.
> 

Well, I thought this is correct even if we have only one descriptor in active
queue (when NFBINT bit is not set). edmac->active.next should point to the same
element as edmac->active, and txd.cookie should be set. But maybe I'm wrong with
this.

Following is a double buffering patch, which should solve your issues with interrupt
storm. It's similar to your hack, but hopefully more optimal, and (the most
important thing for me) it never tries to disable the channel without ensuring
that it is in proper state. If it happens that we are done with descriptors
but Buffer and Control FSM informs that the channel is still operating, we just
wait for another interrupt from EP93xx dma (it's quite interesting, that EP93xx dma
always issues next interrupt, and sets DONE bit again).
Patch applies to current mainline.


Implement double buffering for M2M DMA channels.

Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>

Index: linux-2.6/drivers/dma/ep93xx_dma.c
===================================================================
--- linux-2.6.orig/drivers/dma/ep93xx_dma.c
+++ linux-2.6/drivers/dma/ep93xx_dma.c
@@ -71,6 +71,7 @@
 #define M2M_CONTROL_TM_SHIFT		13
 #define M2M_CONTROL_TM_TX		(1 << M2M_CONTROL_TM_SHIFT)
 #define M2M_CONTROL_TM_RX		(2 << M2M_CONTROL_TM_SHIFT)
+#define M2M_CONTROL_NFBINT		BIT(21)
 #define M2M_CONTROL_RSS_SHIFT		22
 #define M2M_CONTROL_RSS_SSPRX		(1 << M2M_CONTROL_RSS_SHIFT)
 #define M2M_CONTROL_RSS_SSPTX		(2 << M2M_CONTROL_RSS_SHIFT)
@@ -79,7 +80,23 @@
 #define M2M_CONTROL_PWSC_SHIFT		25
 
 #define M2M_INTERRUPT			0x0004
-#define M2M_INTERRUPT_DONEINT		BIT(1)
+#define M2M_INTERRUPT_MASK		6
+
+#define M2M_STATUS			0x000c
+#define M2M_STATUS_CTL_SHIFT		1
+#define M2M_STATUS_CTL_IDLE		(0 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_STALL		(1 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_MEMRD		(2 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_MEMWR		(3 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_BWCWAIT		(4 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_MASK		(7 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_BUF_SHIFT		4
+#define M2M_STATUS_BUF_NO		(0 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_BUF_ON		(1 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_BUF_NEXT		(2 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_BUF_MASK		(3 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_DONE			BIT(6)
+
 
 #define M2M_BCR0			0x0010
 #define M2M_BCR1			0x0014
@@ -426,15 +443,6 @@ static int m2p_hw_interrupt(struct ep93x
 
 /*
  * M2M DMA implementation
- *
- * For the M2M transfers we don't use NFB at all. This is because it simply
- * doesn't work well with memcpy transfers. When you submit both buffers it is
- * extremely unlikely that you get an NFB interrupt, but it instead reports
- * DONE interrupt and both buffers are already transferred which means that we
- * weren't able to update the next buffer.
- *
- * So for now we "simulate" NFB by just submitting buffer after buffer
- * without double buffering.
  */
 
 static int m2m_hw_setup(struct ep93xx_dma_chan *edmac)
@@ -543,6 +551,11 @@ static void m2m_hw_submit(struct ep93xx_
 	m2m_fill_desc(edmac);
 	control |= M2M_CONTROL_DONEINT;
 
+	if (ep93xx_dma_advance_active(edmac)) {
+		m2m_fill_desc(edmac);
+		control |= M2M_CONTROL_NFBINT;
+	}
+
 	/*
 	 * Now we can finally enable the channel. For M2M channel this must be
 	 * done _after_ the BCRx registers are programmed.
@@ -560,32 +573,87 @@ static void m2m_hw_submit(struct ep93xx_
 	}
 }
 
+/*
+ * According to EP93xx User's Guide, we should receive DONE interrupt when all
+ * M2M DMA controller transactions complete normally. This is not always the
+ * case - sometimes EP93xx M2M DMA asserts DONE interrupt when the DMA channel
+ * is still running (channel Buffer FSM in DMA_BUF_ON state, and channel
+ * Control FSM in DMA_MEM_RD state, observed at least in IDE-DMA operation).
+ * In effect, disabling the channel when only DONE bit is set could stop
+ * currently running DMA transfer. To avoid this, we use Buffer FSM and
+ * Control FSM to check current state of DMA channel.
+ */
 static int m2m_hw_interrupt(struct ep93xx_dma_chan *edmac)
 {
+	u32 status = readl(edmac->regs + M2M_STATUS);
+	u32 ctl_fsm = status & M2M_STATUS_CTL_MASK;
+	u32 buf_fsm = status & M2M_STATUS_BUF_MASK;
+	bool done = status & M2M_STATUS_DONE;
+	bool last_done;
 	u32 control;
+	struct ep93xx_dma_desc *desc;
 
-	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_DONEINT))
+	/* Accept only DONE and NFB interrupts */
+	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_MASK))
 		return INTERRUPT_UNKNOWN;
 
-	/* Clear the DONE bit */
-	writel(0, edmac->regs + M2M_INTERRUPT);
+	if (done)
+		/* Clear the DONE bit */
+		writel(0, edmac->regs + M2M_INTERRUPT);
 
-	/* Disable interrupts and the channel */
-	control = readl(edmac->regs + M2M_CONTROL);
-	control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_ENABLE);
-	writel(control, edmac->regs + M2M_CONTROL);
+	/*
+	 * Check whether we are done with descriptors or not. This, together
+	 * with DMA channel state, determines action to take in interrupt.
+	 */
+	desc = ep93xx_dma_get_active(edmac);
+	last_done = !desc || desc->txd.cookie;
 
 	/*
-	 * Since we only get DONE interrupt we have to find out ourselves
-	 * whether there still is something to process. So we try to advance
-	 * the chain an see whether it succeeds.
+	 * Use M2M DMA Buffer FSM and Control FSM to check current state of
+	 * DMA channel. Using DONE and NFB bits from channel status register
+	 * or bits from channel interrupt register is not reliable.
 	 */
-	if (ep93xx_dma_advance_active(edmac)) {
-		edmac->edma->hw_submit(edmac);
-		return INTERRUPT_NEXT_BUFFER;
+	if (!last_done &&
+	    (buf_fsm == M2M_STATUS_BUF_NO ||
+	     buf_fsm == M2M_STATUS_BUF_ON)) {
+		/*
+		 * Two buffers are ready for update when Buffer FSM is in
+		 * DMA_NO_BUF state. Only one buffer can be prepared without
+		 * disabling the channel or polling the DONE bit.
+		 * To simplify things, always prepare only one buffer.
+		 */
+		if (ep93xx_dma_advance_active(edmac)) {
+			m2m_fill_desc(edmac);
+			if (done && !edmac->chan.private) {
+				/* Software trigger for memcpy channel */
+				control = readl(edmac->regs + M2M_CONTROL);
+				control |= M2M_CONTROL_START;
+				writel(control, edmac->regs + M2M_CONTROL);
+			}
+			return INTERRUPT_NEXT_BUFFER;
+		} else
+			last_done = true;
+	}
+
+	/*
+	 * Disable the channel only when Buffer FSM is in DMA_NO_BUF state
+	 * and Control FSM is in DMA_STALL state.
+	 */
+	if (last_done &&
+	    buf_fsm == M2M_STATUS_BUF_NO &&
+	    ctl_fsm == M2M_STATUS_CTL_STALL) {
+		/* Disable interrupts and the channel */
+		control = readl(edmac->regs + M2M_CONTROL);
+		control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_NFBINT
+			    | M2M_CONTROL_ENABLE);
+		writel(control, edmac->regs + M2M_CONTROL);
+		return INTERRUPT_DONE;
 	}
 
-	return INTERRUPT_DONE;
+	/*
+	 * Nothing to do this time.
+	 */
+	return INTERRUPT_NEXT_BUFFER;
 }
 
 /*

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[Rafal Prylowski]

On 2012-04-17 22:51, H Hartley Sweeten wrote:
> On Tuesday, April 17, 2012 8:46 AM, H Hartley Sweeten wrote:
>>
>> Rafal,
>>
>> Here's the output:
>>
>> mmc_spi spi0.1: SD/MMC host mmc0, no poweroff
>> M2M: 20c3
>> M2M: 20c3
>> M2M: 21c3
>> M2M: 21c3
>> M2M: 21c3
>> M2M: 21c3
>> M2M: 21c3
>> M2M: 21c3
>>
>> The "M2M: 21c3" keeps getting output until the system is turned off.
> 
> Rafal,
> 
> It appears that the M2M_CONTROL_NFBINT bit is never getting set when
> dma is used with the spi-ep93xx.c and mmc_spi.c drivers.
> 
> I added a prink in msm_hw_submit():
> 
> 	if (ep93xx_dma_advance_active(edmac)) {
> 		m2m_fill_desc(edmac);
> 		control |= M2M_CONTROL_NFBINT;
> 		printk("%s: NFB enabled\n", __func__);
> 	}
> 
> This message is never displayed.
> 
> And for some reason the txd.cookie is not getting set correctly to detect
> the last entry in m2m_hw_interrupt.
> 
> 	/*
> 	 * Check whether we are done with descriptors or not. This, together
> 	 * with DMA channel state, determines action to take in interrupt.
> 	 */
> 	last = list_first_entry(edmac->active.next,
> 		struct ep93xx_dma_desc, node)->txd.cookie;
> 
> This is causing the code for the INTERRUPT_NEXT_BUFFER to be executed
> even though a "next" buffer does not exist.
> 

Well, I thought this is correct even if we have only one descriptor in active
queue (when NFBINT bit is not set). edmac->active.next should point to the same
element as edmac->active, and txd.cookie should be set. But maybe I'm wrong with
this.

Following is a double buffering patch, which should solve your issues with interrupt
storm. It's similar to your hack, but hopefully more optimal, and (the most
important thing for me) it never tries to disable the channel without ensuring
that it is in proper state. If it happens that we are done with descriptors
but Buffer and Control FSM informs that the channel is still operating, we just
wait for another interrupt from EP93xx dma (it's quite interesting, that EP93xx dma
always issues next interrupt, and sets DONE bit again).
Patch applies to current mainline.


Implement double buffering for M2M DMA channels.

Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>

Index: linux-2.6/drivers/dma/ep93xx_dma.c
===================================================================
--- linux-2.6.orig/drivers/dma/ep93xx_dma.c
+++ linux-2.6/drivers/dma/ep93xx_dma.c
@@ -71,6 +71,7 @@
 #define M2M_CONTROL_TM_SHIFT		13
 #define M2M_CONTROL_TM_TX		(1 << M2M_CONTROL_TM_SHIFT)
 #define M2M_CONTROL_TM_RX		(2 << M2M_CONTROL_TM_SHIFT)
+#define M2M_CONTROL_NFBINT		BIT(21)
 #define M2M_CONTROL_RSS_SHIFT		22
 #define M2M_CONTROL_RSS_SSPRX		(1 << M2M_CONTROL_RSS_SHIFT)
 #define M2M_CONTROL_RSS_SSPTX		(2 << M2M_CONTROL_RSS_SHIFT)
@@ -79,7 +80,23 @@
 #define M2M_CONTROL_PWSC_SHIFT		25
 
 #define M2M_INTERRUPT			0x0004
-#define M2M_INTERRUPT_DONEINT		BIT(1)
+#define M2M_INTERRUPT_MASK		6
+
+#define M2M_STATUS			0x000c
+#define M2M_STATUS_CTL_SHIFT		1
+#define M2M_STATUS_CTL_IDLE		(0 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_STALL		(1 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_MEMRD		(2 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_MEMWR		(3 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_BWCWAIT		(4 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_CTL_MASK		(7 << M2M_STATUS_CTL_SHIFT)
+#define M2M_STATUS_BUF_SHIFT		4
+#define M2M_STATUS_BUF_NO		(0 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_BUF_ON		(1 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_BUF_NEXT		(2 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_BUF_MASK		(3 << M2M_STATUS_BUF_SHIFT)
+#define M2M_STATUS_DONE			BIT(6)
+
 
 #define M2M_BCR0			0x0010
 #define M2M_BCR1			0x0014
@@ -426,15 +443,6 @@ static int m2p_hw_interrupt(struct ep93x
 
 /*
  * M2M DMA implementation
- *
- * For the M2M transfers we don't use NFB at all. This is because it simply
- * doesn't work well with memcpy transfers. When you submit both buffers it is
- * extremely unlikely that you get an NFB interrupt, but it instead reports
- * DONE interrupt and both buffers are already transferred which means that we
- * weren't able to update the next buffer.
- *
- * So for now we "simulate" NFB by just submitting buffer after buffer
- * without double buffering.
  */
 
 static int m2m_hw_setup(struct ep93xx_dma_chan *edmac)
@@ -543,6 +551,11 @@ static void m2m_hw_submit(struct ep93xx_
 	m2m_fill_desc(edmac);
 	control |= M2M_CONTROL_DONEINT;
 
+	if (ep93xx_dma_advance_active(edmac)) {
+		m2m_fill_desc(edmac);
+		control |= M2M_CONTROL_NFBINT;
+	}
+
 	/*
 	 * Now we can finally enable the channel. For M2M channel this must be
 	 * done _after_ the BCRx registers are programmed.
@@ -560,32 +573,87 @@ static void m2m_hw_submit(struct ep93xx_
 	}
 }
 
+/*
+ * According to EP93xx User's Guide, we should receive DONE interrupt when all
+ * M2M DMA controller transactions complete normally. This is not always the
+ * case - sometimes EP93xx M2M DMA asserts DONE interrupt when the DMA channel
+ * is still running (channel Buffer FSM in DMA_BUF_ON state, and channel
+ * Control FSM in DMA_MEM_RD state, observed@least in IDE-DMA operation).
+ * In effect, disabling the channel when only DONE bit is set could stop
+ * currently running DMA transfer. To avoid this, we use Buffer FSM and
+ * Control FSM to check current state of DMA channel.
+ */
 static int m2m_hw_interrupt(struct ep93xx_dma_chan *edmac)
 {
+	u32 status = readl(edmac->regs + M2M_STATUS);
+	u32 ctl_fsm = status & M2M_STATUS_CTL_MASK;
+	u32 buf_fsm = status & M2M_STATUS_BUF_MASK;
+	bool done = status & M2M_STATUS_DONE;
+	bool last_done;
 	u32 control;
+	struct ep93xx_dma_desc *desc;
 
-	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_DONEINT))
+	/* Accept only DONE and NFB interrupts */
+	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_MASK))
 		return INTERRUPT_UNKNOWN;
 
-	/* Clear the DONE bit */
-	writel(0, edmac->regs + M2M_INTERRUPT);
+	if (done)
+		/* Clear the DONE bit */
+		writel(0, edmac->regs + M2M_INTERRUPT);
 
-	/* Disable interrupts and the channel */
-	control = readl(edmac->regs + M2M_CONTROL);
-	control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_ENABLE);
-	writel(control, edmac->regs + M2M_CONTROL);
+	/*
+	 * Check whether we are done with descriptors or not. This, together
+	 * with DMA channel state, determines action to take in interrupt.
+	 */
+	desc = ep93xx_dma_get_active(edmac);
+	last_done = !desc || desc->txd.cookie;
 
 	/*
-	 * Since we only get DONE interrupt we have to find out ourselves
-	 * whether there still is something to process. So we try to advance
-	 * the chain an see whether it succeeds.
+	 * Use M2M DMA Buffer FSM and Control FSM to check current state of
+	 * DMA channel. Using DONE and NFB bits from channel status register
+	 * or bits from channel interrupt register is not reliable.
 	 */
-	if (ep93xx_dma_advance_active(edmac)) {
-		edmac->edma->hw_submit(edmac);
-		return INTERRUPT_NEXT_BUFFER;
+	if (!last_done &&
+	    (buf_fsm == M2M_STATUS_BUF_NO ||
+	     buf_fsm == M2M_STATUS_BUF_ON)) {
+		/*
+		 * Two buffers are ready for update when Buffer FSM is in
+		 * DMA_NO_BUF state. Only one buffer can be prepared without
+		 * disabling the channel or polling the DONE bit.
+		 * To simplify things, always prepare only one buffer.
+		 */
+		if (ep93xx_dma_advance_active(edmac)) {
+			m2m_fill_desc(edmac);
+			if (done && !edmac->chan.private) {
+				/* Software trigger for memcpy channel */
+				control = readl(edmac->regs + M2M_CONTROL);
+				control |= M2M_CONTROL_START;
+				writel(control, edmac->regs + M2M_CONTROL);
+			}
+			return INTERRUPT_NEXT_BUFFER;
+		} else
+			last_done = true;
+	}
+
+	/*
+	 * Disable the channel only when Buffer FSM is in DMA_NO_BUF state
+	 * and Control FSM is in DMA_STALL state.
+	 */
+	if (last_done &&
+	    buf_fsm == M2M_STATUS_BUF_NO &&
+	    ctl_fsm == M2M_STATUS_CTL_STALL) {
+		/* Disable interrupts and the channel */
+		control = readl(edmac->regs + M2M_CONTROL);
+		control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_NFBINT
+			    | M2M_CONTROL_ENABLE);
+		writel(control, edmac->regs + M2M_CONTROL);
+		return INTERRUPT_DONE;
 	}
 
-	return INTERRUPT_DONE;
+	/*
+	 * Nothing to do this time.
+	 */
+	return INTERRUPT_NEXT_BUFFER;
 }
 
 /*

RE: [PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Wednesday, April 18, 2012 9:41 AM, Rafal Prylowski wrote:
> On 2012-04-17 22:51, H Hartley Sweeten wrote:
>> On Tuesday, April 17, 2012 8:46 AM, H Hartley Sweeten wrote:
>>>
>>> Rafal,
>>>
>>> Here's the output:
>>>
>>> mmc_spi spi0.1: SD/MMC host mmc0, no poweroff
>>> M2M: 20c3
>>> M2M: 20c3
>>> M2M: 21c3
>>> M2M: 21c3
>>> M2M: 21c3
>>> M2M: 21c3
>>> M2M: 21c3
>>> M2M: 21c3
>>>
>>> The "M2M: 21c3" keeps getting output until the system is turned off.
>> 
>> Rafal,
>> 
>> It appears that the M2M_CONTROL_NFBINT bit is never getting set when
>> dma is used with the spi-ep93xx.c and mmc_spi.c drivers.
>> 
>> I added a prink in msm_hw_submit():
>> 
>> 	if (ep93xx_dma_advance_active(edmac)) {
>> 		m2m_fill_desc(edmac);
>> 		control |= M2M_CONTROL_NFBINT;
>> 		printk("%s: NFB enabled\n", __func__);
>> 	}
>> 
>> This message is never displayed.
>> 
>> And for some reason the txd.cookie is not getting set correctly to detect
>> the last entry in m2m_hw_interrupt.
>> 
>> 	/*
>> 	 * Check whether we are done with descriptors or not. This, together
>> 	 * with DMA channel state, determines action to take in interrupt.
>> 	 */
>> 	last = list_first_entry(edmac->active.next,
>> 		struct ep93xx_dma_desc, node)->txd.cookie;
>> 
>> This is causing the code for the INTERRUPT_NEXT_BUFFER to be executed
>> even though a "next" buffer does not exist.
>> 
>
> Well, I thought this is correct even if we have only one descriptor in active
> queue (when NFBINT bit is not set). edmac->active.next should point to the same
> element as edmac->active, and txd.cookie should be set. But maybe I'm wrong with
> this.
>
> Following is a double buffering patch, which should solve your issues with interrupt
> storm. It's similar to your hack, but hopefully more optimal, and (the most
> important thing for me) it never tries to disable the channel without ensuring
> that it is in proper state. If it happens that we are done with descriptors
> but Buffer and Control FSM informs that the channel is still operating, we just
> wait for another interrupt from EP93xx dma (it's quite interesting, that EP93xx dma
> always issues next interrupt, and sets DONE bit again).
> Patch applies to current mainline.
>
>
> Implement double buffering for M2M DMA channels.
>
>  Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>
>
> Index: linux-2.6/drivers/dma/ep93xx_dma.c
> ===================================================================
> --- linux-2.6.orig/drivers/dma/ep93xx_dma.c
> +++ linux-2.6/drivers/dma/ep93xx_dma.c
> @@ -71,6 +71,7 @@
>  #define M2M_CONTROL_TM_SHIFT		13
>  #define M2M_CONTROL_TM_TX		(1 << M2M_CONTROL_TM_SHIFT)
>  #define M2M_CONTROL_TM_RX		(2 << M2M_CONTROL_TM_SHIFT)
> +#define M2M_CONTROL_NFBINT		BIT(21)
>  #define M2M_CONTROL_RSS_SHIFT		22
>  #define M2M_CONTROL_RSS_SSPRX		(1 << M2M_CONTROL_RSS_SHIFT)
>  #define M2M_CONTROL_RSS_SSPTX		(2 << M2M_CONTROL_RSS_SHIFT)
> @@ -79,7 +80,23 @@
>  #define M2M_CONTROL_PWSC_SHIFT		25
>  
>  #define M2M_INTERRUPT			0x0004
> -#define M2M_INTERRUPT_DONEINT		BIT(1)
> +#define M2M_INTERRUPT_MASK		6
> +
> +#define M2M_STATUS			0x000c
> +#define M2M_STATUS_CTL_SHIFT		1
> +#define M2M_STATUS_CTL_IDLE		(0 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_STALL		(1 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_MEMRD		(2 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_MEMWR		(3 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_BWCWAIT		(4 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_MASK		(7 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_BUF_SHIFT		4
> +#define M2M_STATUS_BUF_NO		(0 << M2M_STATUS_BUF_SHIFT)
> +#define M2M_STATUS_BUF_ON		(1 << M2M_STATUS_BUF_SHIFT)
> +#define M2M_STATUS_BUF_NEXT		(2 << M2M_STATUS_BUF_SHIFT)
> +#define M2M_STATUS_BUF_MASK		(3 << M2M_STATUS_BUF_SHIFT)
> +#define M2M_STATUS_DONE			BIT(6)
> +

Nitpick.. Unnecessary whitespace.

>  
>  #define M2M_BCR0			0x0010
>  #define M2M_BCR1			0x0014
> @@ -426,15 +443,6 @@ static int m2p_hw_interrupt(struct ep93x
>  
>  /*
>   * M2M DMA implementation
> - *
> - * For the M2M transfers we don't use NFB at all. This is because it simply
> - * doesn't work well with memcpy transfers. When you submit both buffers it is
> - * extremely unlikely that you get an NFB interrupt, but it instead reports
> - * DONE interrupt and both buffers are already transferred which means that we
> - * weren't able to update the next buffer.
> - *
> - * So for now we "simulate" NFB by just submitting buffer after buffer
> - * without double buffering.
>   */
>  
>  static int m2m_hw_setup(struct ep93xx_dma_chan *edmac)
> @@ -543,6 +551,11 @@ static void m2m_hw_submit(struct ep93xx_
>  	m2m_fill_desc(edmac);
>  	control |= M2M_CONTROL_DONEINT;
>  
> +	if (ep93xx_dma_advance_active(edmac)) {
> +		m2m_fill_desc(edmac);
> +		control |= M2M_CONTROL_NFBINT;
> +	}
> +
>  	/*
>  	 * Now we can finally enable the channel. For M2M channel this must be
>  	 * done _after_ the BCRx registers are programmed.
> @@ -560,32 +573,87 @@ static void m2m_hw_submit(struct ep93xx_
>  	}
>  }
>  
> +/*
> + * According to EP93xx User's Guide, we should receive DONE interrupt when all
> + * M2M DMA controller transactions complete normally. This is not always the
> + * case - sometimes EP93xx M2M DMA asserts DONE interrupt when the DMA channel
> + * is still running (channel Buffer FSM in DMA_BUF_ON state, and channel
> + * Control FSM in DMA_MEM_RD state, observed at least in IDE-DMA operation).
> + * In effect, disabling the channel when only DONE bit is set could stop
> + * currently running DMA transfer. To avoid this, we use Buffer FSM and
> + * Control FSM to check current state of DMA channel.
> + */
>  static int m2m_hw_interrupt(struct ep93xx_dma_chan *edmac)
>  {
> +	u32 status = readl(edmac->regs + M2M_STATUS);
> +	u32 ctl_fsm = status & M2M_STATUS_CTL_MASK;
> +	u32 buf_fsm = status & M2M_STATUS_BUF_MASK;
> +	bool done = status & M2M_STATUS_DONE;
> +	bool last_done;
>  	u32 control;
> +	struct ep93xx_dma_desc *desc;
>  
> -	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_DONEINT))
> +	/* Accept only DONE and NFB interrupts */
> +	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_MASK))
>  		return INTERRUPT_UNKNOWN;
>  
> -	/* Clear the DONE bit */
> -	writel(0, edmac->regs + M2M_INTERRUPT);
> +	if (done)
> +		/* Clear the DONE bit */
> +		writel(0, edmac->regs + M2M_INTERRUPT);

Another nitpick. Either move the comment above the if (done) line or enclose
the two lines in { }.

>  
> -	/* Disable interrupts and the channel */
> -	control = readl(edmac->regs + M2M_CONTROL);
> -	control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_ENABLE);
> -	writel(control, edmac->regs + M2M_CONTROL);
> +	/*
> +	 * Check whether we are done with descriptors or not. This, together
> +	 * with DMA channel state, determines action to take in interrupt.
> +	 */
> +	desc = ep93xx_dma_get_active(edmac);
> +	last_done = !desc || desc->txd.cookie;
>  
>  	/*
> -	 * Since we only get DONE interrupt we have to find out ourselves
> -	 * whether there still is something to process. So we try to advance
> -	 * the chain an see whether it succeeds.
> +	 * Use M2M DMA Buffer FSM and Control FSM to check current state of
> +	 * DMA channel. Using DONE and NFB bits from channel status register
> +	 * or bits from channel interrupt register is not reliable.
>  	 */
> -	if (ep93xx_dma_advance_active(edmac)) {
> -		edmac->edma->hw_submit(edmac);
> -		return INTERRUPT_NEXT_BUFFER;
> +	if (!last_done &&
> +	    (buf_fsm == M2M_STATUS_BUF_NO ||
> +	     buf_fsm == M2M_STATUS_BUF_ON)) {
> +		/*
> +		 * Two buffers are ready for update when Buffer FSM is in
> +		 * DMA_NO_BUF state. Only one buffer can be prepared without
> +		 * disabling the channel or polling the DONE bit.
> +		 * To simplify things, always prepare only one buffer.
> +		 */
> +		if (ep93xx_dma_advance_active(edmac)) {
> +			m2m_fill_desc(edmac);
> +			if (done && !edmac->chan.private) {
> +				/* Software trigger for memcpy channel */
> +				control = readl(edmac->regs + M2M_CONTROL);
> +				control |= M2M_CONTROL_START;
> +				writel(control, edmac->regs + M2M_CONTROL);
> +			}
> +			return INTERRUPT_NEXT_BUFFER;
> +		} else
> +			last_done = true;

One more nitpick. The else branch should also be enclosed with { }. See CodingStyle.

> +	}
> +
> +	/*
> +	 * Disable the channel only when Buffer FSM is in DMA_NO_BUF state
> +	 * and Control FSM is in DMA_STALL state.
> +	 */
> +	if (last_done &&
> +	    buf_fsm == M2M_STATUS_BUF_NO &&
> +	    ctl_fsm == M2M_STATUS_CTL_STALL) {
> +		/* Disable interrupts and the channel */
> +		control = readl(edmac->regs + M2M_CONTROL);
> +		control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_NFBINT
> +			    | M2M_CONTROL_ENABLE);
> +		writel(control, edmac->regs + M2M_CONTROL);
> +		return INTERRUPT_DONE;
>  	}
>  
> -	return INTERRUPT_DONE;
> +	/*
> +	 * Nothing to do this time.
> +	 */
> +	return INTERRUPT_NEXT_BUFFER;
>  }
>  
>  /*

Other than the nitpicks, I'm now happy with this patch. It works fine on
my ep93xx board with the mmc_spi and sst25l drivers.

Tested-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Acked-by: H Hartley Sweeten <hsweeten@visionengravers.com>

[PATCH] ep93xx: Implement double buffering for M2M DMA channels

[H Hartley Sweeten]

On Wednesday, April 18, 2012 9:41 AM, Rafal Prylowski wrote:
> On 2012-04-17 22:51, H Hartley Sweeten wrote:
>> On Tuesday, April 17, 2012 8:46 AM, H Hartley Sweeten wrote:
>>>
>>> Rafal,
>>>
>>> Here's the output:
>>>
>>> mmc_spi spi0.1: SD/MMC host mmc0, no poweroff
>>> M2M: 20c3
>>> M2M: 20c3
>>> M2M: 21c3
>>> M2M: 21c3
>>> M2M: 21c3
>>> M2M: 21c3
>>> M2M: 21c3
>>> M2M: 21c3
>>>
>>> The "M2M: 21c3" keeps getting output until the system is turned off.
>> 
>> Rafal,
>> 
>> It appears that the M2M_CONTROL_NFBINT bit is never getting set when
>> dma is used with the spi-ep93xx.c and mmc_spi.c drivers.
>> 
>> I added a prink in msm_hw_submit():
>> 
>> 	if (ep93xx_dma_advance_active(edmac)) {
>> 		m2m_fill_desc(edmac);
>> 		control |= M2M_CONTROL_NFBINT;
>> 		printk("%s: NFB enabled\n", __func__);
>> 	}
>> 
>> This message is never displayed.
>> 
>> And for some reason the txd.cookie is not getting set correctly to detect
>> the last entry in m2m_hw_interrupt.
>> 
>> 	/*
>> 	 * Check whether we are done with descriptors or not. This, together
>> 	 * with DMA channel state, determines action to take in interrupt.
>> 	 */
>> 	last = list_first_entry(edmac->active.next,
>> 		struct ep93xx_dma_desc, node)->txd.cookie;
>> 
>> This is causing the code for the INTERRUPT_NEXT_BUFFER to be executed
>> even though a "next" buffer does not exist.
>> 
>
> Well, I thought this is correct even if we have only one descriptor in active
> queue (when NFBINT bit is not set). edmac->active.next should point to the same
> element as edmac->active, and txd.cookie should be set. But maybe I'm wrong with
> this.
>
> Following is a double buffering patch, which should solve your issues with interrupt
> storm. It's similar to your hack, but hopefully more optimal, and (the most
> important thing for me) it never tries to disable the channel without ensuring
> that it is in proper state. If it happens that we are done with descriptors
> but Buffer and Control FSM informs that the channel is still operating, we just
> wait for another interrupt from EP93xx dma (it's quite interesting, that EP93xx dma
> always issues next interrupt, and sets DONE bit again).
> Patch applies to current mainline.
>
>
> Implement double buffering for M2M DMA channels.
>
>  Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>
>
> Index: linux-2.6/drivers/dma/ep93xx_dma.c
> ===================================================================
> --- linux-2.6.orig/drivers/dma/ep93xx_dma.c
> +++ linux-2.6/drivers/dma/ep93xx_dma.c
> @@ -71,6 +71,7 @@
>  #define M2M_CONTROL_TM_SHIFT		13
>  #define M2M_CONTROL_TM_TX		(1 << M2M_CONTROL_TM_SHIFT)
>  #define M2M_CONTROL_TM_RX		(2 << M2M_CONTROL_TM_SHIFT)
> +#define M2M_CONTROL_NFBINT		BIT(21)
>  #define M2M_CONTROL_RSS_SHIFT		22
>  #define M2M_CONTROL_RSS_SSPRX		(1 << M2M_CONTROL_RSS_SHIFT)
>  #define M2M_CONTROL_RSS_SSPTX		(2 << M2M_CONTROL_RSS_SHIFT)
> @@ -79,7 +80,23 @@
>  #define M2M_CONTROL_PWSC_SHIFT		25
>  
>  #define M2M_INTERRUPT			0x0004
> -#define M2M_INTERRUPT_DONEINT		BIT(1)
> +#define M2M_INTERRUPT_MASK		6
> +
> +#define M2M_STATUS			0x000c
> +#define M2M_STATUS_CTL_SHIFT		1
> +#define M2M_STATUS_CTL_IDLE		(0 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_STALL		(1 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_MEMRD		(2 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_MEMWR		(3 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_BWCWAIT		(4 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_CTL_MASK		(7 << M2M_STATUS_CTL_SHIFT)
> +#define M2M_STATUS_BUF_SHIFT		4
> +#define M2M_STATUS_BUF_NO		(0 << M2M_STATUS_BUF_SHIFT)
> +#define M2M_STATUS_BUF_ON		(1 << M2M_STATUS_BUF_SHIFT)
> +#define M2M_STATUS_BUF_NEXT		(2 << M2M_STATUS_BUF_SHIFT)
> +#define M2M_STATUS_BUF_MASK		(3 << M2M_STATUS_BUF_SHIFT)
> +#define M2M_STATUS_DONE			BIT(6)
> +

Nitpick.. Unnecessary whitespace.

>  
>  #define M2M_BCR0			0x0010
>  #define M2M_BCR1			0x0014
> @@ -426,15 +443,6 @@ static int m2p_hw_interrupt(struct ep93x
>  
>  /*
>   * M2M DMA implementation
> - *
> - * For the M2M transfers we don't use NFB at all. This is because it simply
> - * doesn't work well with memcpy transfers. When you submit both buffers it is
> - * extremely unlikely that you get an NFB interrupt, but it instead reports
> - * DONE interrupt and both buffers are already transferred which means that we
> - * weren't able to update the next buffer.
> - *
> - * So for now we "simulate" NFB by just submitting buffer after buffer
> - * without double buffering.
>   */
>  
>  static int m2m_hw_setup(struct ep93xx_dma_chan *edmac)
> @@ -543,6 +551,11 @@ static void m2m_hw_submit(struct ep93xx_
>  	m2m_fill_desc(edmac);
>  	control |= M2M_CONTROL_DONEINT;
>  
> +	if (ep93xx_dma_advance_active(edmac)) {
> +		m2m_fill_desc(edmac);
> +		control |= M2M_CONTROL_NFBINT;
> +	}
> +
>  	/*
>  	 * Now we can finally enable the channel. For M2M channel this must be
>  	 * done _after_ the BCRx registers are programmed.
> @@ -560,32 +573,87 @@ static void m2m_hw_submit(struct ep93xx_
>  	}
>  }
>  
> +/*
> + * According to EP93xx User's Guide, we should receive DONE interrupt when all
> + * M2M DMA controller transactions complete normally. This is not always the
> + * case - sometimes EP93xx M2M DMA asserts DONE interrupt when the DMA channel
> + * is still running (channel Buffer FSM in DMA_BUF_ON state, and channel
> + * Control FSM in DMA_MEM_RD state, observed at least in IDE-DMA operation).
> + * In effect, disabling the channel when only DONE bit is set could stop
> + * currently running DMA transfer. To avoid this, we use Buffer FSM and
> + * Control FSM to check current state of DMA channel.
> + */
>  static int m2m_hw_interrupt(struct ep93xx_dma_chan *edmac)
>  {
> +	u32 status = readl(edmac->regs + M2M_STATUS);
> +	u32 ctl_fsm = status & M2M_STATUS_CTL_MASK;
> +	u32 buf_fsm = status & M2M_STATUS_BUF_MASK;
> +	bool done = status & M2M_STATUS_DONE;
> +	bool last_done;
>  	u32 control;
> +	struct ep93xx_dma_desc *desc;
>  
> -	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_DONEINT))
> +	/* Accept only DONE and NFB interrupts */
> +	if (!(readl(edmac->regs + M2M_INTERRUPT) & M2M_INTERRUPT_MASK))
>  		return INTERRUPT_UNKNOWN;
>  
> -	/* Clear the DONE bit */
> -	writel(0, edmac->regs + M2M_INTERRUPT);
> +	if (done)
> +		/* Clear the DONE bit */
> +		writel(0, edmac->regs + M2M_INTERRUPT);

Another nitpick. Either move the comment above the if (done) line or enclose
the two lines in { }.

>  
> -	/* Disable interrupts and the channel */
> -	control = readl(edmac->regs + M2M_CONTROL);
> -	control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_ENABLE);
> -	writel(control, edmac->regs + M2M_CONTROL);
> +	/*
> +	 * Check whether we are done with descriptors or not. This, together
> +	 * with DMA channel state, determines action to take in interrupt.
> +	 */
> +	desc = ep93xx_dma_get_active(edmac);
> +	last_done = !desc || desc->txd.cookie;
>  
>  	/*
> -	 * Since we only get DONE interrupt we have to find out ourselves
> -	 * whether there still is something to process. So we try to advance
> -	 * the chain an see whether it succeeds.
> +	 * Use M2M DMA Buffer FSM and Control FSM to check current state of
> +	 * DMA channel. Using DONE and NFB bits from channel status register
> +	 * or bits from channel interrupt register is not reliable.
>  	 */
> -	if (ep93xx_dma_advance_active(edmac)) {
> -		edmac->edma->hw_submit(edmac);
> -		return INTERRUPT_NEXT_BUFFER;
> +	if (!last_done &&
> +	    (buf_fsm == M2M_STATUS_BUF_NO ||
> +	     buf_fsm == M2M_STATUS_BUF_ON)) {
> +		/*
> +		 * Two buffers are ready for update when Buffer FSM is in
> +		 * DMA_NO_BUF state. Only one buffer can be prepared without
> +		 * disabling the channel or polling the DONE bit.
> +		 * To simplify things, always prepare only one buffer.
> +		 */
> +		if (ep93xx_dma_advance_active(edmac)) {
> +			m2m_fill_desc(edmac);
> +			if (done && !edmac->chan.private) {
> +				/* Software trigger for memcpy channel */
> +				control = readl(edmac->regs + M2M_CONTROL);
> +				control |= M2M_CONTROL_START;
> +				writel(control, edmac->regs + M2M_CONTROL);
> +			}
> +			return INTERRUPT_NEXT_BUFFER;
> +		} else
> +			last_done = true;

One more nitpick. The else branch should also be enclosed with { }. See CodingStyle.

> +	}
> +
> +	/*
> +	 * Disable the channel only when Buffer FSM is in DMA_NO_BUF state
> +	 * and Control FSM is in DMA_STALL state.
> +	 */
> +	if (last_done &&
> +	    buf_fsm == M2M_STATUS_BUF_NO &&
> +	    ctl_fsm == M2M_STATUS_CTL_STALL) {
> +		/* Disable interrupts and the channel */
> +		control = readl(edmac->regs + M2M_CONTROL);
> +		control &= ~(M2M_CONTROL_DONEINT | M2M_CONTROL_NFBINT
> +			    | M2M_CONTROL_ENABLE);
> +		writel(control, edmac->regs + M2M_CONTROL);
> +		return INTERRUPT_DONE;
>  	}
>  
> -	return INTERRUPT_DONE;
> +	/*
> +	 * Nothing to do this time.
> +	 */
> +	return INTERRUPT_NEXT_BUFFER;
>  }
>  
>  /*

Other than the nitpicks, I'm now happy with this patch. It works fine on
my ep93xx board with the mmc_spi and sst25l drivers.

Tested-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Acked-by: H Hartley Sweeten <hsweeten@visionengravers.com>