[PATCH v4 0/5] dma: mxs-dma bugfixes and cleanup

[PATCH v4 0/5] dma: mxs-dma bugfixes and cleanup

[Markus Pargmann]

Hi,

v4 adds another patch to increase sound stability. Patch 3 ("dma: mxs-dma: Fix
channel reset hardware bug") decreases the number of channel stalls, but after
hours of repeated playback the bug still appears. The new patch ("dma: mxs: Use
semaphores for cyclic DMA") replaces reset channel usage with hardware
semaphore counter mechanism.

Regards,

Markus Pargmann


Markus Pargmann (5):
      dma: mxs-dma: Cleanup interrupt handler
      dma: mxs-dma: Report correct residue for cyclic DMA
      dma: mxs-dma: Fix channel reset hardware bug
      dma: mxs-dma: Update state after channel reset
      dma: mxs: Use semaphores for cyclic DMA

 drivers/dma/mxs-dma.c | 176 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 137 insertions(+), 39 deletions(-)

[PATCH v4 1/5] dma: mxs-dma: Cleanup interrupt handler

[Markus Pargmann]

The DMA interrupt handler uses its controll registers to handle all
available channel interrupts it can find.

This patch changes it to handle only one interrupt by directly mapping
irq number to channel. It also includes a cleanup of the ctrl-register
usage.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/dma/mxs-dma.c | 96 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 60 insertions(+), 36 deletions(-)

diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index ccd13df..13198e5 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -27,6 +27,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_dma.h>
+#include <linux/list.h>
 
 #include <asm/irq.h>
 
@@ -272,58 +273,81 @@ static void mxs_dma_tasklet(unsigned long data)
 		mxs_chan->desc.callback(mxs_chan->desc.callback_param);
 }
 
+static int mxs_dma_irq_to_chan(struct mxs_dma_engine *mxs_dma, int irq)
+{
+	int i;
+
+	for (i = 0; i != mxs_dma->nr_channels; ++i)
+		if (mxs_dma->mxs_chans[i].chan_irq == irq)
+			return i;
+
+	return -EINVAL;
+}
+
 static irqreturn_t mxs_dma_int_handler(int irq, void *dev_id)
 {
 	struct mxs_dma_engine *mxs_dma = dev_id;
-	u32 stat1, stat2;
+	struct mxs_dma_chan *mxs_chan;
+	u32 completed;
+	u32 err;
+	int chan = mxs_dma_irq_to_chan(mxs_dma, irq);
+
+	if (chan < 0)
+		return IRQ_NONE;
 
 	/* completion status */
-	stat1 = readl(mxs_dma->base + HW_APBHX_CTRL1);
-	stat1 &= MXS_DMA_CHANNELS_MASK;
-	writel(stat1, mxs_dma->base + HW_APBHX_CTRL1 + STMP_OFFSET_REG_CLR);
+	completed = readl(mxs_dma->base + HW_APBHX_CTRL1);
+	completed = (completed >> chan) & 0x1;
+
+	/* Clear interrupt */
+	writel((1 << chan),
+			mxs_dma->base + HW_APBHX_CTRL1 + STMP_OFFSET_REG_CLR);
 
 	/* error status */
-	stat2 = readl(mxs_dma->base + HW_APBHX_CTRL2);
-	writel(stat2, mxs_dma->base + HW_APBHX_CTRL2 + STMP_OFFSET_REG_CLR);
+	err = readl(mxs_dma->base + HW_APBHX_CTRL2);
+	err &= (1 << (MXS_DMA_CHANNELS + chan)) | (1 << chan);
+
+	/*
+	 * error status bit is in the upper 16 bits, error irq bit in the lower
+	 * 16 bits. We transform it into a simpler error code:
+	 * err: 0x00 = no error, 0x01 = TERMINATION, 0x02 = BUS_ERROR
+	 */
+	err = (err >> (MXS_DMA_CHANNELS + chan)) + (err >> chan);
+
+	/* Clear error irq */
+	writel((1 << chan),
+			mxs_dma->base + HW_APBHX_CTRL2 + STMP_OFFSET_REG_CLR);
 
 	/*
 	 * When both completion and error of termination bits set at the
 	 * same time, we do not take it as an error.  IOW, it only becomes
-	 * an error we need to handle here in case of either it's (1) a bus
-	 * error or (2) a termination error with no completion.
+	 * an error we need to handle here in case of either it's a bus
+	 * error or a termination error with no completion. 0x01 is termination
+	 * error, so we can subtract err & completed to get the real error case.
 	 */
-	stat2 = ((stat2 >> MXS_DMA_CHANNELS) & stat2) | /* (1) */
-		(~(stat2 >> MXS_DMA_CHANNELS) & stat2 & ~stat1); /* (2) */
-
-	/* combine error and completion status for checking */
-	stat1 = (stat2 << MXS_DMA_CHANNELS) | stat1;
-	while (stat1) {
-		int channel = fls(stat1) - 1;
-		struct mxs_dma_chan *mxs_chan =
-			&mxs_dma->mxs_chans[channel % MXS_DMA_CHANNELS];
-
-		if (channel >= MXS_DMA_CHANNELS) {
-			dev_dbg(mxs_dma->dma_device.dev,
-				"%s: error in channel %d\n", __func__,
-				channel - MXS_DMA_CHANNELS);
-			mxs_chan->status = DMA_ERROR;
-			mxs_dma_reset_chan(mxs_chan);
-		} else {
-			if (mxs_chan->flags & MXS_DMA_SG_LOOP)
-				mxs_chan->status = DMA_IN_PROGRESS;
-			else
-				mxs_chan->status = DMA_SUCCESS;
-		}
+	err -= err & completed;
 
-		stat1 &= ~(1 << channel);
+	mxs_chan = &mxs_dma->mxs_chans[chan];
 
-		if (mxs_chan->status == DMA_SUCCESS)
-			dma_cookie_complete(&mxs_chan->desc);
-
-		/* schedule tasklet on this channel */
-		tasklet_schedule(&mxs_chan->tasklet);
+	if (err) {
+		dev_dbg(mxs_dma->dma_device.dev,
+			"%s: error in channel %d\n", __func__,
+			chan);
+		mxs_chan->status = DMA_ERROR;
+		mxs_dma_reset_chan(mxs_chan);
+	} else {
+		if (mxs_chan->flags & MXS_DMA_SG_LOOP)
+			mxs_chan->status = DMA_IN_PROGRESS;
+		else
+			mxs_chan->status = DMA_SUCCESS;
 	}
 
+	if (mxs_chan->status == DMA_SUCCESS)
+		dma_cookie_complete(&mxs_chan->desc);
+
+	/* schedule tasklet on this channel */
+	tasklet_schedule(&mxs_chan->tasklet);
+
 	return IRQ_HANDLED;
 }
 
-- 
1.8.4.rc3

[PATCH v4 2/5] dma: mxs-dma: Report correct residue for cyclic DMA

[Markus Pargmann]

Use the channel's buffer address register to calculate correct residue
value for tx_status.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/dma/mxs-dma.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index 13198e5..52282b0 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -58,6 +58,8 @@
 	(((dma_is_apbh(d) && apbh_is_old(d)) ? 0x050 : 0x110) + (n) * 0x70)
 #define HW_APBHX_CHn_SEMA(d, n) \
 	(((dma_is_apbh(d) && apbh_is_old(d)) ? 0x080 : 0x140) + (n) * 0x70)
+#define HW_APBHX_CHn_BAR(d, n) \
+	(((dma_is_apbh(d) && apbh_is_old(d)) ? 0x070 : 0x130) + (n) * 0x70)
 
 /*
  * ccw bits definitions
@@ -623,8 +625,24 @@ static enum dma_status mxs_dma_tx_status(struct dma_chan *chan,
 			dma_cookie_t cookie, struct dma_tx_state *txstate)
 {
 	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
+	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
+	u32 residue = 0;
+
+	if (mxs_chan->status == DMA_IN_PROGRESS &&
+			mxs_chan->flags & MXS_DMA_SG_LOOP) {
+		struct mxs_dma_ccw *last_ccw;
+		u32 bar;
+
+		last_ccw = &mxs_chan->ccw[mxs_chan->desc_count - 1];
+		residue = last_ccw->xfer_bytes + last_ccw->bufaddr;
+
+		bar = readl(mxs_dma->base +
+				HW_APBHX_CHn_BAR(mxs_dma, chan->chan_id));
+		residue -= bar;
+	}
 
-	dma_set_tx_state(txstate, chan->completed_cookie, chan->cookie, 0);
+	dma_set_tx_state(txstate, chan->completed_cookie, chan->cookie,
+			residue);
 
 	return mxs_chan->status;
 }
-- 
1.8.4.rc3

[PATCH v4 3/5] dma: mxs-dma: Fix channel reset hardware bug

[Markus Pargmann]

This is no official errata, but I noticed that the channel reset may
stop working if the DMA state engine is in the READ_FLUSH state.

This patch uses the channel debug1 register to wait for the DMA
statemachine to leave the READ_FLUSH state. After that we can continue
to reset the channel.

Tested on i.MX28.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/dma/mxs-dma.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index 52282b0..42b634a 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -60,6 +60,7 @@
 	(((dma_is_apbh(d) && apbh_is_old(d)) ? 0x080 : 0x140) + (n) * 0x70)
 #define HW_APBHX_CHn_BAR(d, n) \
 	(((dma_is_apbh(d) && apbh_is_old(d)) ? 0x070 : 0x130) + (n) * 0x70)
+#define HW_APBX_CHn_DEBUG1(d, n) (0x150 + (n) * 0x70)
 
 /*
  * ccw bits definitions
@@ -204,12 +205,36 @@ static void mxs_dma_reset_chan(struct mxs_dma_chan *mxs_chan)
 	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
 	int chan_id = mxs_chan->chan.chan_id;
 
-	if (dma_is_apbh(mxs_dma) && apbh_is_old(mxs_dma))
+	if (dma_is_apbh(mxs_dma) && apbh_is_old(mxs_dma)) {
 		writel(1 << (chan_id + BP_APBH_CTRL0_RESET_CHANNEL),
 			mxs_dma->base + HW_APBHX_CTRL0 + STMP_OFFSET_REG_SET);
-	else
+	} else {
+		unsigned long elapsed = 0;
+		const unsigned long max_wait = 50000; /* 50ms */
+		void __iomem *reg_dbg1 = mxs_dma->base +
+				HW_APBX_CHn_DEBUG1(mxs_dma, chan_id);
+
+		/*
+		 * On i.MX28 APBX, the DMA channel can stop working if we reset
+		 * the channel while it is in READ_FLUSH (0x08) state.
+		 * We wait here until we leave the state. Then we trigger the
+		 * reset. Waiting a maximum of 50ms, the kernel shouldn't crash
+		 * because of this.
+		 */
+		while ((readl(reg_dbg1) & 0xf) == 0x8 && elapsed < max_wait) {
+			udelay(100);
+			elapsed += 100;
+		}
+
+		if (elapsed >= max_wait)
+			dev_err(&mxs_chan->mxs_dma->pdev->dev,
+					"Failed waiting for the DMA channel %d to leave state READ_FLUSH, trying to reset channel in READ_FLUSH state now\n",
+					chan_id);
+
+
 		writel(1 << (chan_id + BP_APBHX_CHANNEL_CTRL_RESET_CHANNEL),
 			mxs_dma->base + HW_APBHX_CHANNEL_CTRL + STMP_OFFSET_REG_SET);
+	}
 }
 
 static void mxs_dma_enable_chan(struct mxs_dma_chan *mxs_chan)
-- 
1.8.4.rc3

[PATCH v4 4/5] dma: mxs-dma: Update state after channel reset

[Markus Pargmann]

After a channel reset, the channel stops running automatically. The
state update was missing so that a channel perperation right after a
channel reset failed.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/dma/mxs-dma.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index 42b634a..9e136e8 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -235,6 +235,8 @@ static void mxs_dma_reset_chan(struct mxs_dma_chan *mxs_chan)
 		writel(1 << (chan_id + BP_APBHX_CHANNEL_CTRL_RESET_CHANNEL),
 			mxs_dma->base + HW_APBHX_CHANNEL_CTRL + STMP_OFFSET_REG_SET);
 	}
+
+	mxs_chan->status = DMA_SUCCESS;
 }
 
 static void mxs_dma_enable_chan(struct mxs_dma_chan *mxs_chan)
@@ -362,7 +364,7 @@ static irqreturn_t mxs_dma_int_handler(int irq, void *dev_id)
 			chan);
 		mxs_chan->status = DMA_ERROR;
 		mxs_dma_reset_chan(mxs_chan);
-	} else {
+	} else if (mxs_chan->status != DMA_SUCCESS) {
 		if (mxs_chan->flags & MXS_DMA_SG_LOOP)
 			mxs_chan->status = DMA_IN_PROGRESS;
 		else
-- 
1.8.4.rc3

[PATCH v4 5/5] dma: mxs: Use semaphores for cyclic DMA

[Markus Pargmann]

mxs dma channel hardware reset command is not reliable and can cause
a channel stall. The only way to fix the channel stall is a DMA engine
reset.

To avoid channel resets we use the hardware semaphore counter. For each
transmitted segment, the DMA channel will decrease the counter by one.
To use this mechanism with cyclic DMA, we need to increase the semaphore
counter with each completed DMA command in the interrupt handler. To
avoid any interruptions between the DMA transfers, the semaphore counter
is initialized with 2. This way the counter can be increased in the
interrupt handler without an influence on the transfer of the DMA
engine.

When disabling the channel, we stop increasing the semaphore counter in
the interrupt handler.

This patch was tested on i.MX28 with the SAIF DMA channel.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/dma/mxs-dma.c | 41 +++++++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index 9e136e8..417b7cc 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -119,7 +119,9 @@ struct mxs_dma_chan {
 	int				desc_count;
 	enum dma_status			status;
 	unsigned int			flags;
+	unsigned bool			reset;
 #define MXS_DMA_SG_LOOP			(1 << 0)
+#define MXS_DMA_USE_SEMAPHORE		(1 << 1)
 };
 
 #define MXS_DMA_CHANNELS		16
@@ -205,7 +207,17 @@ static void mxs_dma_reset_chan(struct mxs_dma_chan *mxs_chan)
 	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
 	int chan_id = mxs_chan->chan.chan_id;
 
-	if (dma_is_apbh(mxs_dma) && apbh_is_old(mxs_dma)) {
+	/*
+	 * mxs dma channel resets can cause a channel stall. To recover from a
+	 * channel stall, we have to reset the whole DMA engine. To avoid this,
+	 * we use cyclic DMA with semaphores, that are enhanced in
+	 * mxs_dma_int_handler. To reset the channel, we can simply stop writing
+	 * into the semaphore counter.
+	 */
+	if (mxs_chan->flags & MXS_DMA_USE_SEMAPHORE &&
+			mxs_chan->flags & MXS_DMA_SG_LOOP) {
+		mxs_chan->reset = true;
+	} else if (dma_is_apbh(mxs_dma) && apbh_is_old(mxs_dma)) {
 		writel(1 << (chan_id + BP_APBH_CTRL0_RESET_CHANNEL),
 			mxs_dma->base + HW_APBHX_CTRL0 + STMP_OFFSET_REG_SET);
 	} else {
@@ -231,7 +243,6 @@ static void mxs_dma_reset_chan(struct mxs_dma_chan *mxs_chan)
 					"Failed waiting for the DMA channel %d to leave state READ_FLUSH, trying to reset channel in READ_FLUSH state now\n",
 					chan_id);
 
-
 		writel(1 << (chan_id + BP_APBHX_CHANNEL_CTRL_RESET_CHANNEL),
 			mxs_dma->base + HW_APBHX_CHANNEL_CTRL + STMP_OFFSET_REG_SET);
 	}
@@ -249,7 +260,16 @@ static void mxs_dma_enable_chan(struct mxs_dma_chan *mxs_chan)
 		mxs_dma->base + HW_APBHX_CHn_NXTCMDAR(mxs_dma, chan_id));
 
 	/* write 1 to SEMA to kick off the channel */
-	writel(1, mxs_dma->base + HW_APBHX_CHn_SEMA(mxs_dma, chan_id));
+	if (mxs_chan->flags & MXS_DMA_USE_SEMAPHORE &&
+			mxs_chan->flags & MXS_DMA_SG_LOOP) {
+		/* A cyclic DMA consists of at least 2 segments, so initialize
+		 * the semaphore with 2 so we have enough time to add 1 to the
+		 * semaphore if we need to */
+		writel(2, mxs_dma->base + HW_APBHX_CHn_SEMA(mxs_dma, chan_id));
+	} else {
+		writel(1, mxs_dma->base + HW_APBHX_CHn_SEMA(mxs_dma, chan_id));
+	}
+	mxs_chan->reset = false;
 }
 
 static void mxs_dma_disable_chan(struct mxs_dma_chan *mxs_chan)
@@ -365,14 +385,21 @@ static irqreturn_t mxs_dma_int_handler(int irq, void *dev_id)
 		mxs_chan->status = DMA_ERROR;
 		mxs_dma_reset_chan(mxs_chan);
 	} else if (mxs_chan->status != DMA_SUCCESS) {
-		if (mxs_chan->flags & MXS_DMA_SG_LOOP)
+		if (mxs_chan->flags & MXS_DMA_SG_LOOP) {
 			mxs_chan->status = DMA_IN_PROGRESS;
-		else
+			if (mxs_chan->flags & MXS_DMA_USE_SEMAPHORE)
+				writel(1, mxs_dma->base +
+					HW_APBHX_CHn_SEMA(mxs_dma, chan));
+		} else {
 			mxs_chan->status = DMA_SUCCESS;
+		}
 	}
 
-	if (mxs_chan->status == DMA_SUCCESS)
+	if (mxs_chan->status == DMA_SUCCESS) {
+		if (mxs_chan->reset)
+			return IRQ_HANDLED;
 		dma_cookie_complete(&mxs_chan->desc);
+	}
 
 	/* schedule tasklet on this channel */
 	tasklet_schedule(&mxs_chan->tasklet);
@@ -576,6 +603,7 @@ static struct dma_async_tx_descriptor *mxs_dma_prep_dma_cyclic(
 
 	mxs_chan->status = DMA_IN_PROGRESS;
 	mxs_chan->flags |= MXS_DMA_SG_LOOP;
+	mxs_chan->flags |= MXS_DMA_USE_SEMAPHORE;
 
 	if (num_periods > NUM_CCW) {
 		dev_err(mxs_dma->dma_device.dev,
@@ -607,6 +635,7 @@ static struct dma_async_tx_descriptor *mxs_dma_prep_dma_cyclic(
 		ccw->bits |= CCW_IRQ;
 		ccw->bits |= CCW_HALT_ON_TERM;
 		ccw->bits |= CCW_TERM_FLUSH;
+		ccw->bits |= CCW_DEC_SEM;
 		ccw->bits |= BF_CCW(direction == DMA_DEV_TO_MEM ?
 				MXS_DMA_CMD_WRITE : MXS_DMA_CMD_READ, COMMAND);
 
-- 
1.8.4.rc3

[PATCH v4 0/5] dma: mxs-dma bugfixes and cleanup

[Fabio Estevam]

Hi Markus,

[Also including alsa-devel and some other folks involved with mxs audio]

On Fri, Oct 11, 2013 at 9:12 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> Hi,
>
> v4 adds another patch to increase sound stability. Patch 3 ("dma: mxs-dma: Fix
> channel reset hardware bug") decreases the number of channel stalls, but after
> hours of repeated playback the bug still appears. The new patch ("dma: mxs: Use
> semaphores for cyclic DMA") replaces reset channel usage with hardware
> semaphore counter mechanism.

How is the bug you are trying to solve manifest and how to reproduce it?

I have been investigating an issue about sound stability on mx28 as well.

The usecase I am doing is:

while true
do
    arecord -D hw:0,1 -d 10 -f S16_LE -r 44100 -c 2 | aplay
    sleep 2
done

(Record 10 seconds, sleep and keep in this loop forever).

The problem I see is that randomly the captured audio becomes
extremely noisy and loud (throughout the whole 10 seconds) and then in
the subsequent loop it becomes normal. This is very random and it
happens at a 1 or 2% failure rate.

I tested your v3, but it did not help to fix this particular issue.

I also tried experimenting with the SAIF IOMUX settings:

index cda19c8..463fa92 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -545,9 +545,9 @@
                         MX28_PAD_SAIF0_BITCLK__SAIF0_BITCLK
                         MX28_PAD_SAIF0_SDATA0__SAIF0_SDATA0
                     >;
-                    fsl,drive-strength = <MXS_DRIVE_12mA>;
+                    fsl,drive-strength = <MXS_DRIVE_4mA>;
                     fsl,voltage = <MXS_VOLTAGE_HIGH>;
-                    fsl,pull-up = <MXS_PULL_ENABLE>;
+                    fsl,pull-up = <MXS_PULL_DISABLE>;
                 };

                 saif0_pins_b: saif0 at 1 {
@@ -557,9 +557,9 @@
                         MX28_PAD_SAIF0_BITCLK__SAIF0_BITCLK
                         MX28_PAD_SAIF0_SDATA0__SAIF0_SDATA0
                     >;
-                    fsl,drive-strength = <MXS_DRIVE_12mA>;
+                    fsl,drive-strength = <MXS_DRIVE_4mA>;
                     fsl,voltage = <MXS_VOLTAGE_HIGH>;
-                    fsl,pull-up = <MXS_PULL_ENABLE>;
+                    fsl,pull-up = <MXS_PULL_DISABLE>;
                 };

                 saif1_pins_a: saif1 at 0 {
@@ -567,9 +567,9 @@
                     fsl,pinmux-ids = <
                         MX28_PAD_SAIF1_SDATA0__SAIF1_SDATA0
                     >;
-                    fsl,drive-strength = <MXS_DRIVE_12mA>;
+                    fsl,drive-strength = <MXS_DRIVE_4mA>;
                     fsl,voltage = <MXS_VOLTAGE_HIGH>;
-                    fsl,pull-up = <MXS_PULL_ENABLE>;
+                    fsl,pull-up = <MXS_PULL_DISABLE>;
                 };

and my tests are showing a much better stability. Not able to get this
problem after several hours.

Regards,

Fabio Estevam

[PATCH v4 0/5] dma: mxs-dma bugfixes and cleanup

[Markus Pargmann]

Hi Fabio,

On Fri, Oct 11, 2013 at 11:01:29AM -0300, Fabio Estevam wrote:
> Hi Markus,
> 
> [Also including alsa-devel and some other folks involved with mxs audio]
> 
> On Fri, Oct 11, 2013 at 9:12 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> > Hi,
> >
> > v4 adds another patch to increase sound stability. Patch 3 ("dma: mxs-dma: Fix
> > channel reset hardware bug") decreases the number of channel stalls, but after
> > hours of repeated playback the bug still appears. The new patch ("dma: mxs: Use
> > semaphores for cyclic DMA") replaces reset channel usage with hardware
> > semaphore counter mechanism.
> 
> How is the bug you are trying to solve manifest and how to reproduce it?

My testcase is the following:

/bin/sh -c "while true; do ifconfig $net down; ifconfig $net up; done" &
while $(timeout 10 aplay --period-size=160 --buffer-size=320 $audio_file); do ; done

$audio_file is a 1 second testfile: Signed 16 bit Little Endian, Rate 16000 Hz, Mono

The network interface up/down loop creates enough load that aplay is not
able to deliver enough audio data into the DMA buffer resulting in many
playback interruptions. The interruptions are triggered by the pcm_lib
as soon as there is not enough data in the DMA buffer.

To stop the playback a DMA channel reset is executed. Sometimes this
can lead to a channel stall. There is no audio playback possible until
the next reboot or DMA engine reset. I fixed it by replacing channel
reset with hardware semaphore counter.

The second issue was a sound quality problem. With one of those
interruptions in playback, it was possible that the DMA buffer position
pointer gets wrong due to missed interrupt calls. The result is a bad
sound quality because the DMA engine is reading the same buffer segment
as aplay is writing to. Fixed by using dma residue reporting.

> 
> I have been investigating an issue about sound stability on mx28 as well.
> 
> The usecase I am doing is:
> 
> while true
> do
>     arecord -D hw:0,1 -d 10 -f S16_LE -r 44100 -c 2 | aplay
>     sleep 2
> done
> 
> (Record 10 seconds, sleep and keep in this loop forever).
> 
> The problem I see is that randomly the captured audio becomes
> extremely noisy and loud (throughout the whole 10 seconds) and then in
> the subsequent loop it becomes normal. This is very random and it
> happens at a 1 or 2% failure rate.
> 
> I tested your v3, but it did not help to fix this particular issue.

I think that is another problem independent of the other two issues I
observed. You could try to remove the NO_RESIDUE flag from mxs-pcm which
will enable the use of the residue reporting introduced in this series.
But I don't think it will help.

> 
> I also tried experimenting with the SAIF IOMUX settings:
> 
> index cda19c8..463fa92 100644
> --- a/arch/arm/boot/dts/imx28.dtsi
> +++ b/arch/arm/boot/dts/imx28.dtsi
> @@ -545,9 +545,9 @@
>                          MX28_PAD_SAIF0_BITCLK__SAIF0_BITCLK
>                          MX28_PAD_SAIF0_SDATA0__SAIF0_SDATA0
>                      >;
> -                    fsl,drive-strength = <MXS_DRIVE_12mA>;
> +                    fsl,drive-strength = <MXS_DRIVE_4mA>;
>                      fsl,voltage = <MXS_VOLTAGE_HIGH>;
> -                    fsl,pull-up = <MXS_PULL_ENABLE>;
> +                    fsl,pull-up = <MXS_PULL_DISABLE>;
>                  };
> 
>                  saif0_pins_b: saif0 at 1 {
> @@ -557,9 +557,9 @@
>                          MX28_PAD_SAIF0_BITCLK__SAIF0_BITCLK
>                          MX28_PAD_SAIF0_SDATA0__SAIF0_SDATA0
>                      >;
> -                    fsl,drive-strength = <MXS_DRIVE_12mA>;
> +                    fsl,drive-strength = <MXS_DRIVE_4mA>;
>                      fsl,voltage = <MXS_VOLTAGE_HIGH>;
> -                    fsl,pull-up = <MXS_PULL_ENABLE>;
> +                    fsl,pull-up = <MXS_PULL_DISABLE>;
>                  };
> 
>                  saif1_pins_a: saif1 at 0 {
> @@ -567,9 +567,9 @@
>                      fsl,pinmux-ids = <
>                          MX28_PAD_SAIF1_SDATA0__SAIF1_SDATA0
>                      >;
> -                    fsl,drive-strength = <MXS_DRIVE_12mA>;
> +                    fsl,drive-strength = <MXS_DRIVE_4mA>;
>                      fsl,voltage = <MXS_VOLTAGE_HIGH>;
> -                    fsl,pull-up = <MXS_PULL_ENABLE>;
> +                    fsl,pull-up = <MXS_PULL_DISABLE>;
>                  };
> 
> and my tests are showing a much better stability. Not able to get this
> problem after several hours.
> 
> Regards,
> 
> Fabio Estevam
> 

Regards,

Markus Pargmann

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v4 0/5] dma: mxs-dma bugfixes and cleanup

[Mark Brown]

On Fri, Oct 11, 2013 at 11:01:29AM -0300, Fabio Estevam wrote:

> The problem I see is that randomly the captured audio becomes
> extremely noisy and loud (throughout the whole 10 seconds) and then in
> the subsequent loop it becomes normal. This is very random and it
> happens at a 1 or 2% failure rate.

This sounds like there's a problem with L/R sync or noise on the bit
clock and the samples aren't being well aligned with the clock.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131011/416eef1c/attachment.sig>