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

From: Qipan Li <[email protected]>

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 <[email protected]>

Signed-off-by Barry Song <[email protected]>

---
 drivers/dma/sirf-dma.c | 102
++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 80 insertions(+), 22 deletions(-)

drivers/dma/sirf-dma.c +49 -20

index 1d627e2..7d500d2 100644
@@ -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);

drivers/dma/sirf-dma.c +4 -0

+                       break;
+               case 2:
+                       writel_relaxed(loop_ctrl_val | 1 << (cid + 16),
+                                       sdma->base +

SIRFSOC_DMA_CH_LOOP_CTRL);

drivers/dma/sirf-dma.c +6 -0

+                       break;
+               case 0:
+               default:
+                       writel_relaxed(loop_ctrl_val | (1 << cid) |
+                                       1 << (cid + 16),
+                                       sdma->base +

SIRFSOC_DMA_CH_LOOP_CTRL);

drivers/dma/sirf-dma.c +7 -0

+                       break;
+               }
+       } else {
+               switch (buf_index) {
+               case 1:
+                       writel_relaxed((1 << cid),
+                                       sdma->base +

SIRFSOC_DMA_CH_LOOP_CTRL);

drivers/dma/sirf-dma.c +4 -0

+                       break;
+               case 2:
+                       writel_relaxed((1 << (cid + 16)),
+                                       sdma->base +

SIRFSOC_DMA_CH_LOOP_CTRL);

drivers/dma/sirf-dma.c +5 -0

+                       break;
+               case 0:
+               default:
+                       writel_relaxed((1 << cid) | 1 << (cid + 16),
+                                       sdma->base +

SIRFSOC_DMA_CH_LOOP_CTRL);

drivers/dma/sirf-dma.c +5 -2

+                       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

2013/7/4 Barry Song <[email protected]>

Quoted 7 lines

From: Qipan Li <[email protected]>

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:

Quoted 9 lines

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().

Quoted 1 line

2013/7/4 Barry Song <[email protected]>

well the submitter didn't cc me. So this wasnt in my INBOX!

Quoted 8 lines

Quoted 8 lines

From: Qipan Li <[email protected]>

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

Quoted 9 lines

Quoted 9 lines

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

2013/7/29 Vinod Koul <[email protected]>:

Quoted 10 lines

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

Quoted 1 line

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.

Quoted 11 lines

2013/7/29 Vinod Koul <[email protected]>:

Quoted 10 lines

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

Quoted 7 lines

Quoted 1 line

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

2013/7/4 Barry Song <[email protected]>

Quoted 17 lines

From: Qipan Li <[email protected]>

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().

Quoted 6 lines

Signed-off-by Qipan Li <[email protected]>

Signed-off-by Barry Song <[email protected]>

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

From the looks of this patch, it has nothing to do with any particular

Quoted 22 lines · Barry Song

2013/7/4 Barry Song <[email protected]>

Quoted 20 lines

From: Qipan Li <[email protected]>

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 <[email protected]> Signed-off-by: Barry Song

<[email protected]> --- drivers/dma/sirf-dma.c | 102 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 80 insertions(+), 22 deletions(-)

Quoted 2 lines

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.

Quoted 23 lines · Barry Song

Quoted 22 lines

2013/7/4 Barry Song <[email protected]>

Quoted 20 lines

From: Qipan Li <[email protected]>

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 <[email protected]> Signed-off-by: Barry Song

<[email protected]> --- drivers/dma/sirf-dma.c | 102 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 80 insertions(+), 22 deletions(-)

Quoted 6 lines

Quoted 2 lines

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.

Quoted 4 lines

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