Patchwork [01/11] dma: imx-sdma: make channel0 operations atomic

login
register
mail settings
Submitter Richard Zhao
Date April 27, 2012, 7:02 a.m.
Message ID <1335510185-7906-2-git-send-email-richard.zhao@freescale.com>
Download mbox | patch
Permalink /patch/155391/
State New
Headers show

Comments

Richard Zhao - April 27, 2012, 7:02 a.m.
device_prep_dma_cyclic may be call in audio trigger function which is
atomic context, so we make it atomic too.

 - change channel0 lock to spinlock.
 - Use polling to wait for channel0 finish running.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
---
 drivers/dma/imx-sdma.c |   57 +++++++++++++++++++++++++++--------------------
 1 files changed, 33 insertions(+), 24 deletions(-)
Sascha Hauer - April 27, 2012, 7:55 a.m.
On Fri, Apr 27, 2012 at 03:02:55PM +0800, Richard Zhao wrote:
> device_prep_dma_cyclic may be call in audio trigger function which is
> atomic context, so we make it atomic too.
> 
>  - change channel0 lock to spinlock.
>  - Use polling to wait for channel0 finish running.
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> ---
>  drivers/dma/imx-sdma.c |   57 +++++++++++++++++++++++++++--------------------
>  1 files changed, 33 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index fddccae..fc49ffa 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -24,7 +24,7 @@
>  #include <linux/mm.h>
>  #include <linux/interrupt.h>
>  #include <linux/clk.h>
> -#include <linux/wait.h>
> +#include <linux/delay.h>
>  #include <linux/sched.h>
>  #include <linux/semaphore.h>
>  #include <linux/spinlock.h>
> @@ -324,7 +324,7 @@ struct sdma_engine {
>  	struct dma_device		dma_device;
>  	struct clk			*clk_ipg;
>  	struct clk			*clk_ahb;
> -	struct mutex			channel_0_lock;
> +	spinlock_t		channel_0_lock;
>  	struct sdma_script_start_addrs	*script_addrs;
>  };
>  
> @@ -402,19 +402,31 @@ static void sdma_enable_channel(struct sdma_engine *sdma, int channel)
>  }
>  
>  /*
> - * sdma_run_channel - run a channel and wait till it's done
> + * sdma_run_channel0 - run a channel and wait till it's done
>   */
> -static int sdma_run_channel(struct sdma_channel *sdmac)
> +static int sdma_run_channel0(struct sdma_channel *sdmac)

Renaming this to sdma_run_channel0 is fine, but then the argument should
be changed to struct sdma_engine. It makes no sense to say in the
function name that this function is channel 0 only and at the same time
allow to pass in an arbitrary other channel.

Sascha
Vinod Koul - April 27, 2012, 8:21 a.m.
On Fri, 2012-04-27 at 15:02 +0800, Richard Zhao wrote:
> device_prep_dma_cyclic may be call in audio trigger function which is
> atomic context, so we make it atomic too.
No this is wrong behavior. You should not call dma prepare functions in
any of the sound trigger calls. It would make sense to move this in
sound prepare callback.
Richard Zhao - April 27, 2012, 8:41 a.m.
On Fri, Apr 27, 2012 at 01:51:40PM +0530, Vinod Koul wrote:
> On Fri, 2012-04-27 at 15:02 +0800, Richard Zhao wrote:
> > device_prep_dma_cyclic may be call in audio trigger function which is
> > atomic context, so we make it atomic too.
> No this is wrong behavior. You should not call dma prepare functions in
> any of the sound trigger calls. It would make sense to move this in
> sound prepare callback.
Then, could you please doc it somewhere? I think I'm not the only one
confused.

Thanks
Richard
> 
> -- 
> ~Vinod
> 
>
Richard Zhao - April 27, 2012, 9:17 a.m.
On Fri, Apr 27, 2012 at 09:55:44AM +0200, Sascha Hauer wrote:
> On Fri, Apr 27, 2012 at 03:02:55PM +0800, Richard Zhao wrote:
> > device_prep_dma_cyclic may be call in audio trigger function which is
> > atomic context, so we make it atomic too.
> > 
> >  - change channel0 lock to spinlock.
> >  - Use polling to wait for channel0 finish running.
> > 
> > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
> > ---
> >  drivers/dma/imx-sdma.c |   57 +++++++++++++++++++++++++++--------------------
> >  1 files changed, 33 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > index fddccae..fc49ffa 100644
> > --- a/drivers/dma/imx-sdma.c
> > +++ b/drivers/dma/imx-sdma.c
> > @@ -24,7 +24,7 @@
> >  #include <linux/mm.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/clk.h>
> > -#include <linux/wait.h>
> > +#include <linux/delay.h>
> >  #include <linux/sched.h>
> >  #include <linux/semaphore.h>
> >  #include <linux/spinlock.h>
> > @@ -324,7 +324,7 @@ struct sdma_engine {
> >  	struct dma_device		dma_device;
> >  	struct clk			*clk_ipg;
> >  	struct clk			*clk_ahb;
> > -	struct mutex			channel_0_lock;
> > +	spinlock_t		channel_0_lock;
> >  	struct sdma_script_start_addrs	*script_addrs;
> >  };
> >  
> > @@ -402,19 +402,31 @@ static void sdma_enable_channel(struct sdma_engine *sdma, int channel)
> >  }
> >  
> >  /*
> > - * sdma_run_channel - run a channel and wait till it's done
> > + * sdma_run_channel0 - run a channel and wait till it's done
> >   */
> > -static int sdma_run_channel(struct sdma_channel *sdmac)
> > +static int sdma_run_channel0(struct sdma_channel *sdmac)
> 
> Renaming this to sdma_run_channel0 is fine, but then the argument should
> be changed to struct sdma_engine. It makes no sense to say in the
> function name that this function is channel 0 only and at the same time
> allow to pass in an arbitrary other channel.
Correct. Thanks.

Richard
> 
> Sascha
> 
> 
> -- 
> 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 |
>
Lothar Waßmann - April 27, 2012, 9:18 a.m.
Hi,

Richard Zhao writes:
> device_prep_dma_cyclic may be call in audio trigger function which is
> atomic context, so we make it atomic too.
> 
>  - change channel0 lock to spinlock.
>  - Use polling to wait for channel0 finish running.
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
>
Actually I didn't sign off the patch that I posted, because I wanted
to wait for more comments first.


Lothar Waßmann
Vinod Koul - April 27, 2012, 10:22 a.m.
On Fri, 2012-04-27 at 16:41 +0800, Richard Zhao wrote:
> On Fri, Apr 27, 2012 at 01:51:40PM +0530, Vinod Koul wrote:
> > On Fri, 2012-04-27 at 15:02 +0800, Richard Zhao wrote:
> > > device_prep_dma_cyclic may be call in audio trigger function which is
> > > atomic context, so we make it atomic too.
> > No this is wrong behavior. You should not call dma prepare functions in
> > any of the sound trigger calls. It would make sense to move this in
> > sound prepare callback.
> Then, could you please doc it somewhere? I think I'm not the only one
> confused.
See the soc-dmaengine.c for correct behavior!
I can document only dmaengine behavior (which is already there) and not
how each subsystem should use this. People is subsystems need to see how
to use the dmaengine APIs sanely
Mark Brown - April 27, 2012, 11:20 a.m.
On Fri, Apr 27, 2012 at 03:52:10PM +0530, Vinod Koul wrote:
> On Fri, 2012-04-27 at 16:41 +0800, Richard Zhao wrote:

> > Then, could you please doc it somewhere? I think I'm not the only one
> > confused.

> See the soc-dmaengine.c for correct behavior!

Better yet, convert your code to use it if it's not doing so already!
Richard Zhao - April 27, 2012, 11:26 a.m.
On Fri, Apr 27, 2012 at 03:52:10PM +0530, Vinod Koul wrote:
> On Fri, 2012-04-27 at 16:41 +0800, Richard Zhao wrote:
> > On Fri, Apr 27, 2012 at 01:51:40PM +0530, Vinod Koul wrote:
> > > On Fri, 2012-04-27 at 15:02 +0800, Richard Zhao wrote:
> > > > device_prep_dma_cyclic may be call in audio trigger function which is
> > > > atomic context, so we make it atomic too.
> > > No this is wrong behavior. You should not call dma prepare functions in
> > > any of the sound trigger calls. It would make sense to move this in
> > > sound prepare callback.
> > Then, could you please doc it somewhere? I think I'm not the only one
> > confused.
> See the soc-dmaengine.c for correct behavior!
Do you mean sound/soc/soc-dmaengine-pcm.c ? It's where it calls dma
prepare in trigger function.
snd_dmaengine_pcm_trigger --> dmaengine_pcm_prepare_and_submit -->
dmaengine_prep_dma_cyclic

> I can document only dmaengine behavior (which is already there)
Sure, I mean, can you doc in include/linux/dmaengine.h that
dmaengine_prep_xxx may sleep?

> and not
> how each subsystem should use this. People is subsystems need to see how
> to use the dmaengine APIs sanely

Thanks
Richard
> 
> 
> -- 
> ~Vinod
> 
>
Russell King - ARM Linux - April 27, 2012, 11:33 a.m.
On Fri, Apr 27, 2012 at 07:26:56PM +0800, Richard Zhao wrote:
> Sure, I mean, can you doc in include/linux/dmaengine.h that
> dmaengine_prep_xxx may sleep?

Incorrect.  They may _not_ sleep.
Laxman Dewangan - April 27, 2012, 1:01 p.m.
On Friday 27 April 2012 05:03 PM, Russell King - ARM Linux wrote:
> On Fri, Apr 27, 2012 at 07:26:56PM +0800, Richard Zhao wrote:
>> Sure, I mean, can you doc in include/linux/dmaengine.h that
>> dmaengine_prep_xxx may sleep?
> Incorrect.  They may _not_ sleep.

But I have seen that we are using the kzalloc in the dmaengine_prep_xxx 
and kzalloc is sleeping call.
Russell King - ARM Linux - April 27, 2012, 1:10 p.m.
On Fri, Apr 27, 2012 at 06:31:13PM +0530, Laxman Dewangan wrote:
> On Friday 27 April 2012 05:03 PM, Russell King - ARM Linux wrote:
>> On Fri, Apr 27, 2012 at 07:26:56PM +0800, Richard Zhao wrote:
>>> Sure, I mean, can you doc in include/linux/dmaengine.h that
>>> dmaengine_prep_xxx may sleep?
>> Incorrect.  They may _not_ sleep.
>
> But I have seen that we are using the kzalloc in the dmaengine_prep_xxx  
> and kzalloc is sleeping call.

Not with GFP_ATOMIC it isn't.
Laxman Dewangan - April 27, 2012, 1:17 p.m.
On Friday 27 April 2012 06:40 PM, Russell King - ARM Linux wrote:
> On Fri, Apr 27, 2012 at 06:31:13PM +0530, Laxman Dewangan wrote:
>> On Friday 27 April 2012 05:03 PM, Russell King - ARM Linux wrote:
>>> On Fri, Apr 27, 2012 at 07:26:56PM +0800, Richard Zhao wrote:
>>>> Sure, I mean, can you doc in include/linux/dmaengine.h that
>>>> dmaengine_prep_xxx may sleep?
>>> Incorrect.  They may _not_ sleep.
>> But I have seen that we are using the kzalloc in the dmaengine_prep_xxx
>> and kzalloc is sleeping call.
> Not with GFP_ATOMIC it isn't.

Yes, with GFP_ATOMIC , it will not be a sleeping calls.
I will fix my dma driver accordingly.
Richard Zhao - April 27, 2012, 1:19 p.m.
On Fri, Apr 27, 2012 at 02:10:10PM +0100, Russell King - ARM Linux wrote:
> On Fri, Apr 27, 2012 at 06:31:13PM +0530, Laxman Dewangan wrote:
> > On Friday 27 April 2012 05:03 PM, Russell King - ARM Linux wrote:
> >> On Fri, Apr 27, 2012 at 07:26:56PM +0800, Richard Zhao wrote:
> >>> Sure, I mean, can you doc in include/linux/dmaengine.h that
> >>> dmaengine_prep_xxx may sleep?
> >> Incorrect.  They may _not_ sleep.
Vinod, do you agree? Any way, better doc it.

Thanks
Richard
> >
> > But I have seen that we are using the kzalloc in the dmaengine_prep_xxx  
> > and kzalloc is sleeping call.
> 
> Not with GFP_ATOMIC it isn't.
>
Richard Zhao - April 27, 2012, 1:25 p.m.
On Fri, Apr 27, 2012 at 11:18:31AM +0200, Lothar Waßmann wrote:
> Hi,
> 
> Richard Zhao writes:
> > device_prep_dma_cyclic may be call in audio trigger function which is
> > atomic context, so we make it atomic too.
> > 
> >  - change channel0 lock to spinlock.
> >  - Use polling to wait for channel0 finish running.
> > 
> > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> >
> Actually I didn't sign off the patch that I posted, because I wanted
> to wait for more comments first.
I send it out with slight modifications because the series highly
depend on it. Will you take it over or let me put it in next version?
Both are ok to me.

Thanks
Richard
> 
> 
> 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@karo-electronics.de
> ___________________________________________________________
>
Lothar Waßmann - April 27, 2012, 3:13 p.m.
Hi,

Richard Zhao writes:
> On Fri, Apr 27, 2012 at 11:18:31AM +0200, Lothar Waßmann wrote:
> > Hi,
> > 
> > Richard Zhao writes:
> > > device_prep_dma_cyclic may be call in audio trigger function which is
> > > atomic context, so we make it atomic too.
> > > 
> > >  - change channel0 lock to spinlock.
> > >  - Use polling to wait for channel0 finish running.
> > > 
> > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > >
> > Actually I didn't sign off the patch that I posted, because I wanted
> > to wait for more comments first.
> I send it out with slight modifications because the series highly
> depend on it. Will you take it over or let me put it in next version?
> Both are ok to me.
> 
I think you should keep it as part of your sound patches and I will
test your final version on our hardware.


Lothar Waßmann
Huang Shijie - April 27, 2012, 3:27 p.m.
On Fri, Apr 27, 2012 at 11:13 AM, Lothar Waßmann <LW@karo-electronics.de> wrote:
> Hi,
>
> Richard Zhao writes:
>> On Fri, Apr 27, 2012 at 11:18:31AM +0200, Lothar Waßmann wrote:
>> > Hi,
>> >
>> > Richard Zhao writes:
>> > > device_prep_dma_cyclic may be call in audio trigger function which is
>> > > atomic context, so we make it atomic too.
>> > >
>> > >  - change channel0 lock to spinlock.
>> > >  - Use polling to wait for channel0 finish running.
>> > >
>> > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
>> > >
>> > Actually I didn't sign off the patch that I posted, because I wanted
>> > to wait for more comments first.
>> I send it out with slight modifications because the series highly
>> depend on it. Will you take it over or let me put it in next version?
>> Both are ok to me.
>>
> I think you should keep it as part of your sound patches and I will
> test your final version on our hardware.
>
I hope we can get a conclusion that the prep_slave_sg() can be called
in atomic context or not.
My patch "add DMA support to UART" heavily depends on it.

Huang Shijie

>
> 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@karo-electronics.de
> ___________________________________________________________
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Patch

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index fddccae..fc49ffa 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -24,7 +24,7 @@ 
 #include <linux/mm.h>
 #include <linux/interrupt.h>
 #include <linux/clk.h>
-#include <linux/wait.h>
+#include <linux/delay.h>
 #include <linux/sched.h>
 #include <linux/semaphore.h>
 #include <linux/spinlock.h>
@@ -324,7 +324,7 @@  struct sdma_engine {
 	struct dma_device		dma_device;
 	struct clk			*clk_ipg;
 	struct clk			*clk_ahb;
-	struct mutex			channel_0_lock;
+	spinlock_t		channel_0_lock;
 	struct sdma_script_start_addrs	*script_addrs;
 };
 
@@ -402,19 +402,31 @@  static void sdma_enable_channel(struct sdma_engine *sdma, int channel)
 }
 
 /*
- * sdma_run_channel - run a channel and wait till it's done
+ * sdma_run_channel0 - run a channel and wait till it's done
  */
-static int sdma_run_channel(struct sdma_channel *sdmac)
+static int sdma_run_channel0(struct sdma_channel *sdmac)
 {
 	struct sdma_engine *sdma = sdmac->sdma;
 	int channel = sdmac->channel;
 	int ret;
+	unsigned long timeout = 500;
 
-	init_completion(&sdmac->done);
-
+	if (channel)
+		return -EINVAL;
 	sdma_enable_channel(sdma, channel);
 
-	ret = wait_for_completion_timeout(&sdmac->done, HZ);
+	while (!(ret = readl_relaxed(sdma->regs + SDMA_H_INTR) & 1)) {
+		if (timeout-- <= 0)
+			break;
+		udelay(1);
+	}
+
+	if (ret) {
+		/* Clear the interrupt status */
+		writel_relaxed(ret, sdma->regs + SDMA_H_INTR);
+	} else {
+		dev_err(sdma->dev, "Timeout waiting for CH0 ready\n");
+	}
 
 	return ret ? 0 : -ETIMEDOUT;
 }
@@ -426,17 +438,17 @@  static int sdma_load_script(struct sdma_engine *sdma, void *buf, int size,
 	void *buf_virt;
 	dma_addr_t buf_phys;
 	int ret;
-
-	mutex_lock(&sdma->channel_0_lock);
+	unsigned long flags;
 
 	buf_virt = dma_alloc_coherent(NULL,
 			size,
 			&buf_phys, GFP_KERNEL);
 	if (!buf_virt) {
-		ret = -ENOMEM;
-		goto err_out;
+		return -ENOMEM;
 	}
 
+	spin_lock_irqsave(&sdma->channel_0_lock, flags);
+
 	bd0->mode.command = C0_SETPM;
 	bd0->mode.status = BD_DONE | BD_INTR | BD_WRAP | BD_EXTD;
 	bd0->mode.count = size / 2;
@@ -445,12 +457,11 @@  static int sdma_load_script(struct sdma_engine *sdma, void *buf, int size,
 
 	memcpy(buf_virt, buf, size);
 
-	ret = sdma_run_channel(&sdma->channel[0]);
+	ret = sdma_run_channel0(&sdma->channel[0]);
 
-	dma_free_coherent(NULL, size, buf_virt, buf_phys);
+	spin_unlock_irqrestore(&sdma->channel_0_lock, flags);
 
-err_out:
-	mutex_unlock(&sdma->channel_0_lock);
+	dma_free_coherent(NULL, size, buf_virt, buf_phys);
 
 	return ret;
 }
@@ -539,10 +550,6 @@  static void mxc_sdma_handle_channel(struct sdma_channel *sdmac)
 {
 	complete(&sdmac->done);
 
-	/* not interested in channel 0 interrupts */
-	if (sdmac->channel == 0)
-		return;
-
 	if (sdmac->flags & IMX_DMA_SG_LOOP)
 		sdma_handle_channel_loop(sdmac);
 	else
@@ -555,6 +562,8 @@  static irqreturn_t sdma_int_handler(int irq, void *dev_id)
 	unsigned long stat;
 
 	stat = readl_relaxed(sdma->regs + SDMA_H_INTR);
+	/* not interested in channel 0 interrupts */
+	stat &= ~1;
 	writel_relaxed(stat, sdma->regs + SDMA_H_INTR);
 
 	while (stat) {
@@ -660,6 +669,7 @@  static int sdma_load_context(struct sdma_channel *sdmac)
 	struct sdma_context_data *context = sdma->context;
 	struct sdma_buffer_descriptor *bd0 = sdma->channel[0].bd;
 	int ret;
+	unsigned long flags;
 
 	if (sdmac->direction == DMA_DEV_TO_MEM) {
 		load_address = sdmac->pc_from_device;
@@ -677,7 +687,7 @@  static int sdma_load_context(struct sdma_channel *sdmac)
 	dev_dbg(sdma->dev, "event_mask0 = 0x%08x\n", (u32)sdmac->event_mask[0]);
 	dev_dbg(sdma->dev, "event_mask1 = 0x%08x\n", (u32)sdmac->event_mask[1]);
 
-	mutex_lock(&sdma->channel_0_lock);
+	spin_lock_irqsave(&sdma->channel_0_lock, flags);
 
 	memset(context, 0, sizeof(*context));
 	context->channel_state.pc = load_address;
@@ -696,10 +706,9 @@  static int sdma_load_context(struct sdma_channel *sdmac)
 	bd0->mode.count = sizeof(*context) / 4;
 	bd0->buffer_addr = sdma->context_phys;
 	bd0->ext_buffer_addr = 2048 + (sizeof(*context) / 4) * channel;
+	ret = sdma_run_channel0(&sdma->channel[0]);
 
-	ret = sdma_run_channel(&sdma->channel[0]);
-
-	mutex_unlock(&sdma->channel_0_lock);
+	spin_unlock_irqrestore(&sdma->channel_0_lock, flags);
 
 	return ret;
 }
@@ -1305,7 +1314,7 @@  static int __init sdma_probe(struct platform_device *pdev)
 	if (!sdma)
 		return -ENOMEM;
 
-	mutex_init(&sdma->channel_0_lock);
+	spin_lock_init(&sdma->channel_0_lock);
 
 	sdma->dev = &pdev->dev;