[PATCH] DMAEngine: sirf: let the users be able to pause and resume specific buffer

[PATCH] DMAEngine: sirf: let the users be able to pause and resume specific buffer

[Barry Song]

From: Qipan Li <Qipan.Li@csr.com>

this patch adds a buffer_index in pause and resume entries, then users
can pause and resume a buffer they want, but don't pause the whole dma.

a typical application scenerios is Ping-Pang in two buffers:
at the beginning, we enable buf1 and buf2 to receive dma data, after
buf1 is full, we pause buf1 and handle the data in this buffer to avoid
overflow in buf1. but at the same time, dma is still tranferring in buf2.
once we have finished data process in buf1, we enable buf1 again.
this will maximize the chance of dma transferring. users pause buf1 by:
dmaengine_device_control(sirfport->rx_dma_chan, DMA_PAUSE, 1);
users pause buf2 by:
dmaengine_device_control(sirfport->rx_dma_chan, DMA_PAUSE, 2);
users can still pause the whole dma transferring by dmaengine_pause().

Signed-off-by: Qipan Li <Qipan.Li@csr.com>
Signed-off-by: Barry Song <Baohua.Song@csr.com>
---
 drivers/dma/sirf-dma.c | 102 ++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 80 insertions(+), 22 deletions(-)

diff --git a/drivers/dma/sirf-dma.c b/drivers/dma/sirf-dma.c
index 1d627e2..7d500d2 100644
--- a/drivers/dma/sirf-dma.c
+++ b/drivers/dma/sirf-dma.c
@@ -315,43 +315,101 @@ static int sirfsoc_dma_terminate_all(struct sirfsoc_dma_chan *schan)
 	return 0;
 }
 
-static int sirfsoc_dma_pause_chan(struct sirfsoc_dma_chan *schan)
+static int sirfsoc_dma_pause_chan(struct sirfsoc_dma_chan *schan,
+						unsigned long buf_index)
 {
 	struct sirfsoc_dma *sdma = dma_chan_to_sirfsoc_dma(&schan->chan);
 	int cid = schan->chan.chan_id;
 	unsigned long flags;
+	unsigned long loop_ctrl_val;
 
 	spin_lock_irqsave(&schan->lock, flags);
-
-	if (!sdma->is_marco)
-		writel_relaxed(readl_relaxed(sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL)
-			& ~((1 << cid) | 1 << (cid + 16)),
-			sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL);
-	else
-		writel_relaxed((1 << cid) | 1 << (cid + 16),
-			sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL_CLR);
-
+	if (!sdma->is_marco) {
+		loop_ctrl_val = readl_relaxed(sdma->base +
+				SIRFSOC_DMA_CH_LOOP_CTRL);
+		switch (buf_index) {
+		case 1:
+			writel_relaxed(loop_ctrl_val & ~(1 << cid),
+					sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL);
+			break;
+		case 2:
+			writel_relaxed(loop_ctrl_val & ~(1 << (cid + 16)),
+					sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL);
+			break;
+		case 0:
+		default:
+			writel_relaxed(loop_ctrl_val &
+					~((1 << cid) | 1 << (cid + 16)),
+					sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL);
+			break;
+		}
+	} else {
+		switch (buf_index) {
+		case 1:
+			writel_relaxed((1 << cid), sdma->base +
+						SIRFSOC_DMA_CH_LOOP_CTRL_CLR);
+			break;
+		case 2:
+			writel_relaxed(1 << (cid + 16), sdma->base +
+						SIRFSOC_DMA_CH_LOOP_CTRL_CLR);
+			break;
+		case 0:
+		default:
+			writel_relaxed((1 << cid) | 1 << (cid + 16),
+				sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL_CLR);
+			break;
+		}
+	}
 	spin_unlock_irqrestore(&schan->lock, flags);
 
 	return 0;
 }
 
-static int sirfsoc_dma_resume_chan(struct sirfsoc_dma_chan *schan)
+static int sirfsoc_dma_resume_chan(struct sirfsoc_dma_chan *schan,
+						unsigned long buf_index)
 {
 	struct sirfsoc_dma *sdma = dma_chan_to_sirfsoc_dma(&schan->chan);
 	int cid = schan->chan.chan_id;
 	unsigned long flags;
+	unsigned long loop_ctrl_val;
 
 	spin_lock_irqsave(&schan->lock, flags);
-
-	if (!sdma->is_marco)
-		writel_relaxed(readl_relaxed(sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL)
-			| ((1 << cid) | 1 << (cid + 16)),
-			sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL);
-	else
-		writel_relaxed((1 << cid) | 1 << (cid + 16),
-			sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL);
-
+	if (!sdma->is_marco) {
+		loop_ctrl_val = readl_relaxed(sdma->base +
+				SIRFSOC_DMA_CH_LOOP_CTRL);
+		switch (buf_index) {
+		case 1:
+			writel_relaxed(loop_ctrl_val | (1 << cid),
+					sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL);
+			break;
+		case 2:
+			writel_relaxed(loop_ctrl_val | 1 << (cid + 16),
+					sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL);
+			break;
+		case 0:
+		default:
+			writel_relaxed(loop_ctrl_val | (1 << cid) |
+					1 << (cid + 16),
+					sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL);
+			break;
+		}
+	} else {
+		switch (buf_index) {
+		case 1:
+			writel_relaxed((1 << cid),
+					sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL);
+			break;
+		case 2:
+			writel_relaxed((1 << (cid + 16)),
+					sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL);
+			break;
+		case 0:
+		default:
+			writel_relaxed((1 << cid) | 1 << (cid + 16),
+					sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL);
+			break;
+		}
+	}
 	spin_unlock_irqrestore(&schan->lock, flags);
 
 	return 0;
@@ -365,9 +423,9 @@ static int sirfsoc_dma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 
 	switch (cmd) {
 	case DMA_PAUSE:
-		return sirfsoc_dma_pause_chan(schan);
+		return sirfsoc_dma_pause_chan(schan, arg);
 	case DMA_RESUME:
-		return sirfsoc_dma_resume_chan(schan);
+		return sirfsoc_dma_resume_chan(schan, arg);
 	case DMA_TERMINATE_ALL:
 		return sirfsoc_dma_terminate_all(schan);
 	case DMA_SLAVE_CONFIG:
-- 
1.8.2.3



Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog

[PATCH] DMAEngine: sirf: let the users be able to pause and resume specific buffer

[Barry Song]

2013/7/4 Barry Song <Baohua.Song@csr.com>
>
> From: Qipan Li <Qipan.Li@csr.com>
>
> this patch adds a buffer_index in pause and resume entries, then users
> can pause and resume a buffer they want, but don't pause the whole dma.
>
> a typical application scenerios is Ping-Pang in two buffers:
> at the beginning, we enable buf1 and buf2 to receive dma data, after
> buf1 is full, we pause buf1 and handle the data in this buffer to avoid
> overflow in buf1. but at the same time, dma is still tranferring in buf2.
> once we have finished data process in buf1, we enable buf1 again.
> this will maximize the chance of dma transferring. users pause buf1 by:
> dmaengine_device_control(sirfport->rx_dma_chan, DMA_PAUSE, 1);
> users pause buf2 by:
> dmaengine_device_control(sirfport->rx_dma_chan, DMA_PAUSE, 2);
> users can still pause the whole dma transferring by dmaengine_pause().
>
> Signed-off-by: Qipan Li <Qipan.Li@csr.com>
> Signed-off-by: Barry Song <Baohua.Song@csr.com>
> ---
>  drivers/dma/sirf-dma.c | 102
> ++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 80 insertions(+), 22 deletions(-)

Hi Vinod, as we still have some other patches of dma clients depending
on this, would you give some comments about it?

>
> diff --git a/drivers/dma/sirf-dma.c b/drivers/dma/sirf-dma.c
> index 1d627e2..7d500d2 100644
> --- a/drivers/dma/sirf-dma.c
> +++ b/drivers/dma/sirf-dma.c
> @@ -315,43 +315,101 @@ static int sirfsoc_dma_terminate_all(struct
> sirfsoc_dma_chan *schan)
>         return 0;
>  }
>
> -static int sirfsoc_dma_pause_chan(struct sirfsoc_dma_chan *schan)
> +static int sirfsoc_dma_pause_chan(struct sirfsoc_dma_chan *schan,
> +                                               unsigned long buf_index)
>  {
>         struct sirfsoc_dma *sdma = dma_chan_to_sirfsoc_dma(&schan->chan);
>         int cid = schan->chan.chan_id;
>         unsigned long flags;
> +       unsigned long loop_ctrl_val;
>
>         spin_lock_irqsave(&schan->lock, flags);
> -
> -       if (!sdma->is_marco)
> -               writel_relaxed(readl_relaxed(sdma->base +
> SIRFSOC_DMA_CH_LOOP_CTRL)
> -                       & ~((1 << cid) | 1 << (cid + 16)),
> -                       sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL);
> -       else
> -               writel_relaxed((1 << cid) | 1 << (cid + 16),
> -                       sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL_CLR);
> -
> +       if (!sdma->is_marco) {
> +               loop_ctrl_val = readl_relaxed(sdma->base +
> +                               SIRFSOC_DMA_CH_LOOP_CTRL);
> +               switch (buf_index) {
> +               case 1:
> +                       writel_relaxed(loop_ctrl_val & ~(1 << cid),
> +                                       sdma->base +
> SIRFSOC_DMA_CH_LOOP_CTRL);
> +                       break;
> +               case 2:
> +                       writel_relaxed(loop_ctrl_val & ~(1 << (cid + 16)),
> +                                       sdma->base +
> SIRFSOC_DMA_CH_LOOP_CTRL);
> +                       break;
> +               case 0:
> +               default:
> +                       writel_relaxed(loop_ctrl_val &
> +                                       ~((1 << cid) | 1 << (cid + 16)),
> +                                       sdma->base +
> SIRFSOC_DMA_CH_LOOP_CTRL);
> +                       break;
> +               }
> +       } else {
> +               switch (buf_index) {
> +               case 1:
> +                       writel_relaxed((1 << cid), sdma->base +
> +
> SIRFSOC_DMA_CH_LOOP_CTRL_CLR);
> +                       break;
> +               case 2:
> +                       writel_relaxed(1 << (cid + 16), sdma->base +
> +
> SIRFSOC_DMA_CH_LOOP_CTRL_CLR);
> +                       break;
> +               case 0:
> +               default:
> +                       writel_relaxed((1 << cid) | 1 << (cid + 16),
> +                               sdma->base +
> SIRFSOC_DMA_CH_LOOP_CTRL_CLR);
> +                       break;
> +               }
> +       }
>         spin_unlock_irqrestore(&schan->lock, flags);
>
>         return 0;
>  }
>
> -static int sirfsoc_dma_resume_chan(struct sirfsoc_dma_chan *schan)
> +static int sirfsoc_dma_resume_chan(struct sirfsoc_dma_chan *schan,
> +                                               unsigned long buf_index)
>  {
>         struct sirfsoc_dma *sdma = dma_chan_to_sirfsoc_dma(&schan->chan);
>         int cid = schan->chan.chan_id;
>         unsigned long flags;
> +       unsigned long loop_ctrl_val;
>
>         spin_lock_irqsave(&schan->lock, flags);
> -
> -       if (!sdma->is_marco)
> -               writel_relaxed(readl_relaxed(sdma->base +
> SIRFSOC_DMA_CH_LOOP_CTRL)
> -                       | ((1 << cid) | 1 << (cid + 16)),
> -                       sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL);
> -       else
> -               writel_relaxed((1 << cid) | 1 << (cid + 16),
> -                       sdma->base + SIRFSOC_DMA_CH_LOOP_CTRL);
> -
> +       if (!sdma->is_marco) {
> +               loop_ctrl_val = readl_relaxed(sdma->base +
> +                               SIRFSOC_DMA_CH_LOOP_CTRL);
> +               switch (buf_index) {
> +               case 1:
> +                       writel_relaxed(loop_ctrl_val | (1 << cid),
> +                                       sdma->base +
> SIRFSOC_DMA_CH_LOOP_CTRL);
> +                       break;
> +               case 2:
> +                       writel_relaxed(loop_ctrl_val | 1 << (cid + 16),
> +                                       sdma->base +
> SIRFSOC_DMA_CH_LOOP_CTRL);
> +                       break;
> +               case 0:
> +               default:
> +                       writel_relaxed(loop_ctrl_val | (1 << cid) |
> +                                       1 << (cid + 16),
> +                                       sdma->base +
> SIRFSOC_DMA_CH_LOOP_CTRL);
> +                       break;
> +               }
> +       } else {
> +               switch (buf_index) {
> +               case 1:
> +                       writel_relaxed((1 << cid),
> +                                       sdma->base +
> SIRFSOC_DMA_CH_LOOP_CTRL);
> +                       break;
> +               case 2:
> +                       writel_relaxed((1 << (cid + 16)),
> +                                       sdma->base +
> SIRFSOC_DMA_CH_LOOP_CTRL);
> +                       break;
> +               case 0:
> +               default:
> +                       writel_relaxed((1 << cid) | 1 << (cid + 16),
> +                                       sdma->base +
> SIRFSOC_DMA_CH_LOOP_CTRL);
> +                       break;
> +               }
> +       }
>         spin_unlock_irqrestore(&schan->lock, flags);
>
>         return 0;
> @@ -365,9 +423,9 @@ static int sirfsoc_dma_control(struct dma_chan *chan,
> enum dma_ctrl_cmd cmd,
>
>         switch (cmd) {
>         case DMA_PAUSE:
> -               return sirfsoc_dma_pause_chan(schan);
> +               return sirfsoc_dma_pause_chan(schan, arg);
>         case DMA_RESUME:
> -               return sirfsoc_dma_resume_chan(schan);
> +               return sirfsoc_dma_resume_chan(schan, arg);
>         case DMA_TERMINATE_ALL:
>                 return sirfsoc_dma_terminate_all(schan);
>         case DMA_SLAVE_CONFIG:
> --
> 1.8.2.3


-barry

[PATCH] DMAEngine: sirf: let the users be able to pause and resume specific buffer

[Vinod Koul]

On Mon, Jul 29, 2013 at 10:14:24AM +0800, Barry Song wrote:
> 2013/7/4 Barry Song <Baohua.Song@csr.com>
well the submitter didn't cc me. So this wasnt in my INBOX!

> >
> > From: Qipan Li <Qipan.Li@csr.com>
> >
> > this patch adds a buffer_index in pause and resume entries, then users
> > can pause and resume a buffer they want, but don't pause the whole dma.
> >
> > a typical application scenerios is Ping-Pang in two buffers:
typo, pong	          		^^^^^^^^^
> > at the beginning, we enable buf1 and buf2 to receive dma data, after
> > buf1 is full, we pause buf1 and handle the data in this buffer to avoid
> > overflow in buf1. but at the same time, dma is still tranferring in buf2.
> > once we have finished data process in buf1, we enable buf1 again.
> > this will maximize the chance of dma transferring. users pause buf1 by:
> > dmaengine_device_control(sirfport->rx_dma_chan, DMA_PAUSE, 1);
> > users pause buf2 by:
> > dmaengine_device_control(sirfport->rx_dma_chan, DMA_PAUSE, 2);
> > users can still pause the whole dma transferring by dmaengine_pause().
Well this is odd, the second arg is not supposed to be valid for PAUSE op. And
PAUSE means you pause the whole channel.

The above can be done by 
a) have two buffers:
	- prepare first and submit, and start
	- while first is ongoing, prepare second and start
	- first is completed, so you start processing that while second is
	  getting DMAed
	- when processing is completed, submit first again
	- when second is completed, start processing that and submit that once
	  proocessed

b) have multiple buffers, A, B, C & D and redo above schema.

Since patch is not the right approach and doesnt use API correctly NAK from me on that

~Vinod

[PATCH] DMAEngine: sirf: let the users be able to pause and resume specific buffer

[Barry Song]

2013/7/29 Vinod Koul <vinod.koul@intel.com>:
> On Mon, Jul 29, 2013 at 10:14:24AM +0800, Barry Song wrote:
>> 2013/7/4 Barry Song <Baohua.Song@csr.com>
> well the submitter didn't cc me. So this wasnt in my INBOX!

Vinod, i must did some misoperation. sorry for that.

>
>> >
>> > From: Qipan Li <Qipan.Li@csr.com>
>> >
>> > this patch adds a buffer_index in pause and resume entries, then users
>> > can pause and resume a buffer they want, but don't pause the whole dma.
>> >
>> > a typical application scenerios is Ping-Pang in two buffers:
> typo, pong                              ^^^^^^^^^
>> > at the beginning, we enable buf1 and buf2 to receive dma data, after
>> > buf1 is full, we pause buf1 and handle the data in this buffer to avoid
>> > overflow in buf1. but at the same time, dma is still tranferring in buf2.
>> > once we have finished data process in buf1, we enable buf1 again.
>> > this will maximize the chance of dma transferring. users pause buf1 by:
>> > dmaengine_device_control(sirfport->rx_dma_chan, DMA_PAUSE, 1);
>> > users pause buf2 by:
>> > dmaengine_device_control(sirfport->rx_dma_chan, DMA_PAUSE, 2);
>> > users can still pause the whole dma transferring by dmaengine_pause().
> Well this is odd, the second arg is not supposed to be valid for PAUSE op. And
> PAUSE means you pause the whole channel.
>
> The above can be done by
> a) have two buffers:
>         - prepare first and submit, and start
>         - while first is ongoing, prepare second and start
>         - first is completed, so you start processing that while second is
>           getting DMAed
>         - when processing is completed, submit first again
>         - when second is completed, start processing that and submit that once
>           proocessed
>
> b) have multiple buffers, A, B, C & D and redo above schema.

yes, that is the original idea for multiple buffer management for DMA.

the reason which pushed us to the ping-pong pause is that we don't
need to issue software dma descriptor every time for every buffer as
hardware can do that.
so we went to a pause for buffers which have been issued by hardware
and stop buffer overflow.

>
> Since patch is not the right approach and doesnt use API correctly NAK from me on that

the original idea is actually extending the DMA pause API insteading
of using current PAUSE pf the whole link. i got a little worried about
the patch too, guys have a long history for ping-pong pause even
before using DMAEngine framework.

if your answer is definite no, let's move to your way which makes more senses.

>
> ~Vinod

-barry

[PATCH] DMAEngine: sirf: let the users be able to pause and resume specific buffer

[Russell King - ARM Linux]

On Mon, Jul 29, 2013 at 03:20:31PM +0800, Barry Song wrote:
> 2013/7/29 Vinod Koul <vinod.koul@intel.com>:
> > The above can be done by
> > a) have two buffers:
> >         - prepare first and submit, and start
> >         - while first is ongoing, prepare second and start
> >         - first is completed, so you start processing that while second is
> >           getting DMAed
> >         - when processing is completed, submit first again
> >         - when second is completed, start processing that and submit that once
> >           proocessed
> >
> > b) have multiple buffers, A, B, C & D and redo above schema.
> 
> yes, that is the original idea for multiple buffer management for DMA.
> 
> the reason which pushed us to the ping-pong pause is that we don't
> need to issue software dma descriptor every time for every buffer as
> hardware can do that.

If your DMA engine is correctly implemented (and this only works for
correctly implemented DMA engines), what you can do is:

- at the start of the transfer, prepare, submit and issue two buffers,
  each with a callback.
- when the first callback happens
  - that buffer has been transferred.
  - you can now do whatever you need with that data.
  - prepare, submit and issue the next buffer

This means the DMA engine always has a buffer available to it, so if
your CPU is fast enough, you can keep the transfer going without the
DMA channel ever stopping or having to be paused.  If your CPU isn't
fast enough to keep up, the DMA engine will stop at the end of the
last queued and issued buffer automatically.

Bonus points if you have implemented the "move to next buffer" correctly
too, so that the hardware can do a continuous transfer without having to
stop and wait between the buffers (provided of course that the next
transfer is compatible with the previous.)

[PATCH] DMAEngine: sirf: let the users be able to pause and resume specific buffer

[Barry Song]

2013/7/29 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Mon, Jul 29, 2013 at 03:20:31PM +0800, Barry Song wrote:
>> 2013/7/29 Vinod Koul <vinod.koul@intel.com>:
>> > The above can be done by
>> > a) have two buffers:
>> >         - prepare first and submit, and start
>> >         - while first is ongoing, prepare second and start
>> >         - first is completed, so you start processing that while second is
>> >           getting DMAed
>> >         - when processing is completed, submit first again
>> >         - when second is completed, start processing that and submit that once
>> >           proocessed
>> >
>> > b) have multiple buffers, A, B, C & D and redo above schema.
>>
>> yes, that is the original idea for multiple buffer management for DMA.
>>
>> the reason which pushed us to the ping-pong pause is that we don't
>> need to issue software dma descriptor every time for every buffer as
>> hardware can do that.
>
> If your DMA engine is correctly implemented (and this only works for
> correctly implemented DMA engines), what you can do is:
>
> - at the start of the transfer, prepare, submit and issue two buffers,
>   each with a callback.
> - when the first callback happens
>   - that buffer has been transferred.
>   - you can now do whatever you need with that data.
>   - prepare, submit and issue the next buffer
>
> This means the DMA engine always has a buffer available to it, so if
> your CPU is fast enough, you can keep the transfer going without the
> DMA channel ever stopping or having to be paused.  If your CPU isn't
> fast enough to keep up, the DMA engine will stop at the end of the
> last queued and issued buffer automatically.
>

yes, suppose there are 4 buffers, we can issue all of buffer 0,1,2,3
at first. if buffer 0 has been read when buffer 3 is transferred,
buffer0 must have been issued again.
so that will keep buffers always alive if cpu is fast enough.

> Bonus points if you have implemented the "move to next buffer" correctly
> too, so that the hardware can do a continuous transfer without having to
> stop and wait between the buffers (provided of course that the next
> transfer is compatible with the previous.)

-barry

[PATCH] DMAEngine: sirf: let the users be able to pause and resume specific buffer

[Russell King - ARM Linux]

On Mon, Jul 29, 2013 at 10:14:24AM +0800, Barry Song wrote:
> 2013/7/4 Barry Song <Baohua.Song@csr.com>
> >
> > From: Qipan Li <Qipan.Li@csr.com>
> >
> > this patch adds a buffer_index in pause and resume entries, then users
> > can pause and resume a buffer they want, but don't pause the whole dma.
> >
> > a typical application scenerios is Ping-Pang in two buffers:
> > at the beginning, we enable buf1 and buf2 to receive dma data, after
> > buf1 is full, we pause buf1 and handle the data in this buffer to avoid
> > overflow in buf1. but at the same time, dma is still tranferring in buf2.
> > once we have finished data process in buf1, we enable buf1 again.
> > this will maximize the chance of dma transferring. users pause buf1 by:
> > dmaengine_device_control(sirfport->rx_dma_chan, DMA_PAUSE, 1);
> > users pause buf2 by:
> > dmaengine_device_control(sirfport->rx_dma_chan, DMA_PAUSE, 2);
> > users can still pause the whole dma transferring by dmaengine_pause().
> >
> > Signed-off-by: Qipan Li <Qipan.Li@csr.com>
> > Signed-off-by: Barry Song <Baohua.Song@csr.com>
> > ---
> >  drivers/dma/sirf-dma.c | 102
> > ++++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 80 insertions(+), 22 deletions(-)
> 
> Hi Vinod, as we still have some other patches of dma clients depending
> on this, would you give some comments about it?

You at least need to write up some documentation about this new
feature - you need to describe exactly how this argument relates to
a set of queued DMA descriptor(s) and its semantics with respect to
those queued descriptors.

>From the looks of this patch, it has nothing to do with any particular
queued descriptor, but you're exposing internal knowledge of the SiRF
DMA engine.

[PATCH] DMAEngine: sirf: let the users be able to pause and resume specific buffer

[Vinod Koul]

On Mon, Jul 29, 2013 at 01:08:30PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 29, 2013 at 10:14:24AM +0800, Barry Song wrote:
> > 2013/7/4 Barry Song <Baohua.Song@csr.com>
> > >
> > > From: Qipan Li <Qipan.Li@csr.com>
> > >
> > > this patch adds a buffer_index in pause and resume entries, then users can
> > > pause and resume a buffer they want, but don't pause the whole dma.
> > >
> > > a typical application scenerios is Ping-Pang in two buffers: at the
> > > beginning, we enable buf1 and buf2 to receive dma data, after buf1 is
> > > full, we pause buf1 and handle the data in this buffer to avoid overflow
> > > in buf1. but at the same time, dma is still tranferring in buf2.  once we
> > > have finished data process in buf1, we enable buf1 again.  this will
> > > maximize the chance of dma transferring. users pause buf1 by:
> > > dmaengine_device_control(sirfport->rx_dma_chan, DMA_PAUSE, 1); users pause
> > > buf2 by: dmaengine_device_control(sirfport->rx_dma_chan, DMA_PAUSE, 2);
> > > users can still pause the whole dma transferring by dmaengine_pause().
> > >
> > > Signed-off-by: Qipan Li <Qipan.Li@csr.com> Signed-off-by: Barry Song
> > > <Baohua.Song@csr.com> --- drivers/dma/sirf-dma.c | 102
> > > ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 80
> > > insertions(+), 22 deletions(-)
> > 
> > Hi Vinod, as we still have some other patches of dma clients depending on
> > this, would you give some comments about it?
> 
> You at least need to write up some documentation about this new feature - you
> need to describe exactly how this argument relates to a set of queued DMA
> descriptor(s) and its semantics with respect to those queued descriptors.
As I commented earlier today, I dont think we need this way. This can also be
achieved with submitting descriptors after the buffer has been processed.

> From the looks of this patch, it has nothing to do with any particular
> queued descriptor, but you're exposing internal knowledge of the SiRF
> DMA engine.
More than dmaengine its the way of modelling here which seems to be the issue

~Vinod
-- 

[PATCH] DMAEngine: sirf: let the users be able to pause and resume specific buffer

[Barry Song]

2013/7/29 Vinod Koul <vinod.koul@intel.com>:
> On Mon, Jul 29, 2013 at 01:08:30PM +0100, Russell King - ARM Linux wrote:
>> On Mon, Jul 29, 2013 at 10:14:24AM +0800, Barry Song wrote:
>> > 2013/7/4 Barry Song <Baohua.Song@csr.com>
>> > >
>> > > From: Qipan Li <Qipan.Li@csr.com>
>> > >
>> > > this patch adds a buffer_index in pause and resume entries, then users can
>> > > pause and resume a buffer they want, but don't pause the whole dma.
>> > >
>> > > a typical application scenerios is Ping-Pang in two buffers: at the
>> > > beginning, we enable buf1 and buf2 to receive dma data, after buf1 is
>> > > full, we pause buf1 and handle the data in this buffer to avoid overflow
>> > > in buf1. but at the same time, dma is still tranferring in buf2.  once we
>> > > have finished data process in buf1, we enable buf1 again.  this will
>> > > maximize the chance of dma transferring. users pause buf1 by:
>> > > dmaengine_device_control(sirfport->rx_dma_chan, DMA_PAUSE, 1); users pause
>> > > buf2 by: dmaengine_device_control(sirfport->rx_dma_chan, DMA_PAUSE, 2);
>> > > users can still pause the whole dma transferring by dmaengine_pause().
>> > >
>> > > Signed-off-by: Qipan Li <Qipan.Li@csr.com> Signed-off-by: Barry Song
>> > > <Baohua.Song@csr.com> --- drivers/dma/sirf-dma.c | 102
>> > > ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 80
>> > > insertions(+), 22 deletions(-)
>> >
>> > Hi Vinod, as we still have some other patches of dma clients depending on
>> > this, would you give some comments about it?
>>
>> You at least need to write up some documentation about this new feature - you
>> need to describe exactly how this argument relates to a set of queued DMA
>> descriptor(s) and its semantics with respect to those queued descriptors.
> As I commented earlier today, I dont think we need this way. This can also be
> achieved with submitting descriptors after the buffer has been processed.

we submit all buffers together at the beginning, once one buffer is
right handled by cpu after dma transfers, we issue it again.

>
>> From the looks of this patch, it has nothing to do with any particular
>> queued descriptor, but you're exposing internal knowledge of the SiRF
>> DMA engine.
> More than dmaengine its the way of modelling here which seems to be the issue
>
> ~Vinod
> --
-barry