[PATCH v2 0/3] dma: mxs-dma bugfixes and cleanup

[PATCH v2 0/3] dma: mxs-dma bugfixes and cleanup

[Markus Pargmann]

Hi,

I am using the mxs-dma driver with the generic soc pcm dmaengine and found some
sound issues. The patches are based on v3.12-rc2.

v2 contains a new patch which reports correct residues for cyclic dma.
I removed "dma: mxs-dma: Track number of irqs for callback" as it is not
necessary for pcm dmaengine.

Regards,

Markus Pargmann


Markus Pargmann (3):
      dma: mxs-dma: Cleanup interrupt handler
      dma: mxs-dma: Pause channel while prep_dma_cyclic
      dma: mxs-dma: Report correct residue for cyclic DMA

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

[PATCH v2 1/3] 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 | 95 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 59 insertions(+), 36 deletions(-)

diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index ccd13df..bfca8dc 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -272,58 +272,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 v2 2/3] dma: mxs-dma: Pause channel while prep_dma_cyclic

[Markus Pargmann]

Using mxs-dma with pcm-dmaengine with small period-length can in some
cases lead to strange audio effects. This patch pauses the DMA channel
while preparing cyclic DMA commands. I tested with mx28 and
period-length of 160.

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

diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index bfca8dc..a696438 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -544,8 +544,8 @@ static struct dma_async_tx_descriptor *mxs_dma_prep_dma_cyclic(
 	if (mxs_chan->status == DMA_IN_PROGRESS)
 		return NULL;
 
-	mxs_chan->status = DMA_IN_PROGRESS;
-	mxs_chan->flags |= MXS_DMA_SG_LOOP;
+	mxs_dma_pause_chan(mxs_chan);
+	mxs_dma_reset_chan(mxs_chan);
 
 	if (num_periods > NUM_CCW) {
 		dev_err(mxs_dma->dma_device.dev,
@@ -585,6 +585,11 @@ static struct dma_async_tx_descriptor *mxs_dma_prep_dma_cyclic(
 
 		i++;
 	}
+	mxs_dma_resume_chan(mxs_chan);
+
+	mxs_chan->status = DMA_IN_PROGRESS;
+	mxs_chan->flags |= MXS_DMA_SG_LOOP;
+
 	mxs_chan->desc_count = i;
 
 	return &mxs_chan->desc;
-- 
1.8.4.rc3

[PATCH v2 3/3] 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.
---
 drivers/dma/mxs-dma.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index a696438..f99498b 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -57,6 +57,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
@@ -627,8 +629,23 @@ 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 v2 1/3] dma: mxs-dma: Cleanup interrupt handler

[Lothar Waßmann]

Hi,

Markus Pargmann writes:
> 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 | 95 ++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 59 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> index ccd13df..bfca8dc 100644
> --- a/drivers/dma/mxs-dma.c
> +++ b/drivers/dma/mxs-dma.c
> @@ -272,58 +272,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;
>
You might use a linked list for all active channels so that you don't
have to check all unused channels when trying to find the channel
number for an irq.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH v2 1/3] dma: mxs-dma: Cleanup interrupt handler

[Marc Kleine-Budde]

On 09/30/2013 03:54 PM, Lothar Wa?mann wrote:
> Hi,
> 
> Markus Pargmann writes:
>> 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 | 95 ++++++++++++++++++++++++++++++++-------------------
>>  1 file changed, 59 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
>> index ccd13df..bfca8dc 100644
>> --- a/drivers/dma/mxs-dma.c
>> +++ b/drivers/dma/mxs-dma.c
>> @@ -272,58 +272,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;
>>
> You might use a linked list for all active channels so that you don't
> have to check all unused channels when trying to find the channel
> number for an irq.

Or an array of mxs_dma->nr_channels length for direct mapping.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 259 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130930/33c1b90f/attachment.sig>

[PATCH v2 1/3] dma: mxs-dma: Cleanup interrupt handler

[Markus Pargmann]

Hi,

On Mon, Sep 30, 2013 at 03:59:10PM +0200, Marc Kleine-Budde wrote:
> On 09/30/2013 03:54 PM, Lothar Wa?mann wrote:
> > Hi,
> > 
> > Markus Pargmann writes:
> >> 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 | 95 ++++++++++++++++++++++++++++++++-------------------
> >>  1 file changed, 59 insertions(+), 36 deletions(-)
> >>
> >> diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> >> index ccd13df..bfca8dc 100644
> >> --- a/drivers/dma/mxs-dma.c
> >> +++ b/drivers/dma/mxs-dma.c
> >> @@ -272,58 +272,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;
> >>
> > You might use a linked list for all active channels so that you don't
> > have to check all unused channels when trying to find the channel
> > number for an irq.
> 
> Or an array of mxs_dma->nr_channels length for direct mapping.

We map irq to channel so an array of nr_channels won't work here.

I will use a linked list.

Thanks

Markus


-- 
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 |