pasemi_dma: Driver for PA Semi PWRficient on-chip DMA engine

Hi Olof,

Just one thing I noticed ...

drivers/dma/pasemi_dma.c line 394

Quoted 2 lines · On Thu, 6 Mar 2008 17:39:00 -0600 Olof Johansson <[email protected]> wrote:

drivers/dma/pasemi_dma.c new file +1 -0

+	dma_dev->device_dependency_added = pasemi_dma_dependency_added;

device_dependency_added is going away in 2.6.26 ...

pasemi_dma: Driver for PA Semi PWRficient on-chip DMA engine

First cut at a dma copy offload driver for PA Semi PWRficient. It uses the platform-specific functions to allocate channels, etc.

Quoted 4 lines · On Thu, 6 Mar 2008 17:39:00 -0600 Olof Johansson <[email protected]> wrote:

pasemi_dma: Driver for PA Semi PWRficient on-chip DMA engine

First cut at a dma copy offload driver for PA Semi PWRficient. It uses the platform-specific functions to allocate channels, etc.

Applied this on Paul's latest and powerpc allmodconfig goes boom.

drivers/dma/pasemi_dma.c: In function `pasemi_dma_alloc_chan_resources':
drivers/dma/pasemi_dma.c:152: error: `PAS_DMA_TXCHAN_CFG_TY_COPY' undeclared (first use in this function) drivers/dma/pasemi_dma.c:152: error: (Each undeclared identifier is reported only once drivers/dma/pasemi_dma.c:152: error: for each function it appears in.) drivers/dma/pasemi_dma.c:154: error: PAS_DMA_TXCHAN_CFG_LPDQ' undeclared (first use in this function) drivers/dma/pasemi_dma.c:155: error: PAS_DMA_TXCHAN_CFG_LPSQ' undeclared (first use in this function) drivers/dma/pasemi_dma.c: In function `pasemi_dma_probe':
drivers/dma/pasemi_dma.c:394: error: structure has no member named `device_dependency_added'

Also this driver from git-md-accel is pretty sick:

drivers/dma/fsldma.c:439: warning: comparison of distinct pointer types lacks a cast drivers/dma/fsldma.c: In function `fsl_chan_xfer_ld_queue':
drivers/dma/fsldma.c:584: warning: long long unsigned int format, dma_addr_t arg (arg 4) drivers/dma/fsldma.c: In function `fsl_dma_chan_do_interrupt':
drivers/dma/fsldma.c:661: warning: unsigned int format, different type arg (arg 5) drivers/dma/fsldma.c:677: warning: long long unsigned int format, dma_addr_t arg (arg 4) drivers/dma/fsldma.c:677: warning: long long unsigned int format, dma_addr_t arg (arg 5) drivers/dma/fsldma.c:694: warning: unsigned int format, different type arg (arg 4) drivers/dma/fsldma.c: In function `fsl_dma_self_test':
drivers/dma/fsldma.c:833: warning: int format, different type arg (arg 5) drivers/dma/fsldma.c: In function `of_fsl_dma_probe':
drivers/dma/fsldma.c:1003: warning: unsigned int format, different type arg (arg 5) drivers/dma/fsldma.c: At top level:
drivers/dma/fsldma.c:723: warning: 'fsl_dma_callback_test' defined but not used

Quoted 4 lines · Andrew Morton

Quoted 2 lines · On Thu, 6 Mar 2008 17:39:00 -0600 Olof Johansson <[email protected]> wrote:

pasemi_dma: Driver for PA Semi PWRficient on-chip DMA engine

Quoted 4 lines

Quoted 2 lines

First cut at a dma copy offload driver for PA Semi PWRficient. It uses the platform-specific functions to allocate channels, etc.

Applied this on Paul's latest and powerpc allmodconfig goes boom.

It's dependent on my latest pull request of pasemi.git for-2.6.26 that Paul hasn't pulled/pushed yet.

Quoted 8 lines

drivers/dma/pasemi_dma.c: In function `pasemi_dma_alloc_chan_resources':
drivers/dma/pasemi_dma.c:152: error: `PAS_DMA_TXCHAN_CFG_TY_COPY' undeclared (first use in this function) drivers/dma/pasemi_dma.c:152: error: (Each undeclared identifier is reported only once drivers/dma/pasemi_dma.c:152: error: for each function it appears in.) drivers/dma/pasemi_dma.c:154: error: PAS_DMA_TXCHAN_CFG_LPDQ' undeclared (first use in this function) drivers/dma/pasemi_dma.c:155: error: PAS_DMA_TXCHAN_CFG_LPSQ' undeclared (first use in this function) drivers/dma/pasemi_dma.c: In function `pasemi_dma_probe':
drivers/dma/pasemi_dma.c:394: error: structure has no member named `device_dependency_added'

.. and that one is caused by recent changes in async_tx.git. I was waiting on other review comments from the DMA maintainers before resubmitting; timer has expired though and I'll do it anyway today.

Quoted 17 lines

Also this driver from git-md-accel is pretty sick:

drivers/dma/fsldma.c:439: warning: comparison of distinct pointer types lacks a cast drivers/dma/fsldma.c: In function `fsl_chan_xfer_ld_queue':
drivers/dma/fsldma.c:584: warning: long long unsigned int format, dma_addr_t arg (arg 4) drivers/dma/fsldma.c: In function `fsl_dma_chan_do_interrupt':
drivers/dma/fsldma.c:661: warning: unsigned int format, different type arg (arg 5) drivers/dma/fsldma.c:677: warning: long long unsigned int format, dma_addr_t arg (arg 4) drivers/dma/fsldma.c:677: warning: long long unsigned int format, dma_addr_t arg (arg 5) drivers/dma/fsldma.c:694: warning: unsigned int format, different type arg (arg 4) drivers/dma/fsldma.c: In function `fsl_dma_self_test':
drivers/dma/fsldma.c:833: warning: int format, different type arg (arg 5) drivers/dma/fsldma.c: In function `of_fsl_dma_probe':
drivers/dma/fsldma.c:1003: warning: unsigned int format, different type arg (arg 5) drivers/dma/fsldma.c: At top level:
drivers/dma/fsldma.c:723: warning: 'fsl_dma_callback_test' defined but not used

Yeah, Zhang Wei posted a patch for that on lkml yesterday.

Quoted 5 lines · On Tue, 11 Mar 2008 09:25:45 -0500 Olof Johansson <[email protected]> wrote:

Quoted 4 lines · Andrew Morton

Quoted 2 lines · On Thu, 6 Mar 2008 17:39:00 -0600 Olof Johansson <[email protected]> wrote:

pasemi_dma: Driver for PA Semi PWRficient on-chip DMA engine

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

First cut at a dma copy offload driver for PA Semi PWRficient. It uses the platform-specific functions to allocate channels, etc.

Applied this on Paul's latest and powerpc allmodconfig goes boom.

It's dependent on my latest pull request of pasemi.git for-2.6.26 that Paul hasn't pulled/pushed yet.

Maybe we should get that tree into -mm and/or linux-next.

People do test -mm on powerpc.

Quoted 9 lines

Quoted 8 lines

drivers/dma/pasemi_dma.c: In function `pasemi_dma_alloc_chan_resources':
drivers/dma/pasemi_dma.c:152: error: `PAS_DMA_TXCHAN_CFG_TY_COPY' undeclared (first use in this function) drivers/dma/pasemi_dma.c:152: error: (Each undeclared identifier is reported only once drivers/dma/pasemi_dma.c:152: error: for each function it appears in.) drivers/dma/pasemi_dma.c:154: error: PAS_DMA_TXCHAN_CFG_LPDQ' undeclared (first use in this function) drivers/dma/pasemi_dma.c:155: error: PAS_DMA_TXCHAN_CFG_LPSQ' undeclared (first use in this function) drivers/dma/pasemi_dma.c: In function `pasemi_dma_probe':
drivers/dma/pasemi_dma.c:394: error: structure has no member named `device_dependency_added'

Quoted 3 lines

.. and that one is caused by recent changes in async_tx.git. I was waiting on other review comments from the DMA maintainers before resubmitting; timer has expired though and I'll do it anyway today.

What is async_tx.git?

Quoted 18 lines

Quoted 17 lines

Also this driver from git-md-accel is pretty sick:

drivers/dma/fsldma.c:439: warning: comparison of distinct pointer types lacks a cast drivers/dma/fsldma.c: In function `fsl_chan_xfer_ld_queue':
drivers/dma/fsldma.c:584: warning: long long unsigned int format, dma_addr_t arg (arg 4) drivers/dma/fsldma.c: In function `fsl_dma_chan_do_interrupt':
drivers/dma/fsldma.c:661: warning: unsigned int format, different type arg (arg 5) drivers/dma/fsldma.c:677: warning: long long unsigned int format, dma_addr_t arg (arg 4) drivers/dma/fsldma.c:677: warning: long long unsigned int format, dma_addr_t arg (arg 5) drivers/dma/fsldma.c:694: warning: unsigned int format, different type arg (arg 4) drivers/dma/fsldma.c: In function `fsl_dma_self_test':
drivers/dma/fsldma.c:833: warning: int format, different type arg (arg 5) drivers/dma/fsldma.c: In function `of_fsl_dma_probe':
drivers/dma/fsldma.c:1003: warning: unsigned int format, different type arg (arg 5) drivers/dma/fsldma.c: At top level:
drivers/dma/fsldma.c:723: warning: 'fsl_dma_callback_test' defined but not used

Quoted 1 line

Yeah, Zhang Wei posted a patch for that on lkml yesterday.

OK.

Quoted 18 lines

Quoted 18 lines

Quoted 17 lines

Also this driver from git-md-accel is pretty sick:

drivers/dma/fsldma.c:439: warning: comparison of distinct pointer types lacks a cast drivers/dma/fsldma.c: In function `fsl_chan_xfer_ld_queue':
drivers/dma/fsldma.c:584: warning: long long unsigned int format, dma_addr_t arg (arg 4) drivers/dma/fsldma.c: In function `fsl_dma_chan_do_interrupt':
drivers/dma/fsldma.c:661: warning: unsigned int format, different type arg (arg 5) drivers/dma/fsldma.c:677: warning: long long unsigned int format, dma_addr_t arg (arg 4) drivers/dma/fsldma.c:677: warning: long long unsigned int format, dma_addr_t arg (arg 5) drivers/dma/fsldma.c:694: warning: unsigned int format, different type arg (arg 4) drivers/dma/fsldma.c: In function `fsl_dma_self_test':
drivers/dma/fsldma.c:833: warning: int format, different type arg (arg 5) drivers/dma/fsldma.c: In function `of_fsl_dma_probe':
drivers/dma/fsldma.c:1003: warning: unsigned int format, different type arg (arg 5) drivers/dma/fsldma.c: At top level:
drivers/dma/fsldma.c:723: warning: 'fsl_dma_callback_test' defined but not used

Quoted 3 lines

Quoted 1 line

Yeah, Zhang Wei posted a patch for that on lkml yesterday.

OK.

My fault for not pushing out this cleanup to the old url while the git-md-accel changeover was pending.

Quoted 2 lines

My fault for not pushing out this cleanup to the old url while the git-md-accel changeover was pending.

Any reason not to push Zhang's fixes for 2.6.25?

I also have a patch I want to push for 2.6.25 to deal with the powerpc device tree. I was going to handle this via the powerpc tree's since that seems to be the model we have used for netdev and other drivers that play with arch/powerpc. I hope that's not an issue (I'll CC you on the patch).

pasemi_dma: Driver for PA Semi PWRficient on-chip DMA engine

First cut at a dma copy offload driver for PA Semi PWRficient. It uses the platform-specific functions to allocate channels, etc.

Signed-off-by Olof Johansson <[email protected]>

---

This has some dependencies on other patches currently queued up in the powerpc git trees for 2.6.26. I'd appreciate reviews and acked-bys, but it might be easiest to just merge it up the powerpc path due to the dependencies.

I notice that the driver does not handle callbacks in its descriptor cleanup path. This could be ok if your intent is only to support the net_dma style polled operations, but this will not work for the raid-offload async_tx case. I think the solution is for async_tx to ignore channels without the DMA_INTERRUPT capability.

drivers/dma/pasemi_dma.c line 80

Quoted 6 lines

drivers/dma/pasemi_dma.c new file +6 -0

+static void pasemi_dma_clean(struct pasemi_dma_chan *chan)
+{
+       int old, new, i;
+       unsigned long flags;
+       struct pasemi_dma_desc *desc;
+       spin_lock_irqsave(&chan->desc_lock, flags);

Is spin_lock_bh() insufficient here?

...that's all that jumps out at the moment.

Quoted 5 lines · Dan Williams

I notice that the driver does not handle callbacks in its descriptor cleanup path. This could be ok if your intent is only to support the net_dma style polled operations, but this will not work for the raid-offload async_tx case. I think the solution is for async_tx to ignore channels without the DMA_INTERRUPT capability.

Good point, and correct - I have mostly been testing this with the NET_DMA offload. async_tx doesn't make use of just memcpy, does it?

Also, how is DMA_INTERRUPT supposed to work? I see there's a separate "prep_dma_interrupt" function, but that doesn't make sense to me. Don't you want the interrupt associated with a specific transaction instead of added as a separate (empty) transaction?

Once I add the descriptor to the ring, I can't change it to set the interrupt request bit on it. I suppose I could just add a dummy descriptor to the ring, but that doesn't seem quite right either.

Quoted 10 lines

drivers/dma/pasemi_dma.c line 80

Quoted 7 lines

drivers/dma/pasemi_dma.c new file +6 -0

+static void pasemi_dma_clean(struct pasemi_dma_chan *chan)
+{
+       int old, new, i;
+       unsigned long flags;
+       struct pasemi_dma_desc *desc;
+       spin_lock_irqsave(&chan->desc_lock, flags);

Is spin_lock_bh() insufficient here?

Quoted 1 line

...that's all that jumps out at the moment.

Can't do that if it's called both from the polling as well as the IRQ context, it'd need to hold off irqs. I.e. once I add the DMA_INTERRUPT support it will be needed.

-Olof

Quoted 8 lines · Olof Johansson

Quoted 5 lines · Dan Williams

I notice that the driver does not handle callbacks in its descriptor cleanup path. This could be ok if your intent is only to support the net_dma style polled operations, but this will not work for the raid-offload async_tx case. I think the solution is for async_tx to ignore channels without the DMA_INTERRUPT capability.

Quoted 3 lines

Good point, and correct - I have mostly been testing this with the NET_DMA offload. async_tx doesn't make use of just memcpy, does it?

Right, it makes use of any capability it can get its hands on, otherwise fall back to software.

Quoted 5 lines

Also, how is DMA_INTERRUPT supposed to work? I see there's a separate "prep_dma_interrupt" function, but that doesn't make sense to me. Don't you want the interrupt associated with a specific transaction instead of added as a separate (empty) transaction?

Yes, and that is what happens in most cases. The additional prep_dma_interrupt method is for two special cases:
1/ Locations where code is submitting an indeterminate number of operations and wants an interrupt (callback) at the completion of the chain. Someting like:
list_for_each_entry()
async_memcpy()
async_trigger_callback()

2/ For supporting channel switching on, for instance, a memcpy->xor chain where chan1 supports memcpy and chan2 supports xor. When async_tx sees this it injects an interrupt for chan1. The xor operation gets kicked off by the bottom half of the chan1 interrupt handler. However, if chan1 can do both operations no interrupt is needed.

Quoted 4 lines

Once I add the descriptor to the ring, I can't change it to set the interrupt request bit on it. I suppose I could just add a dummy descriptor to the ring, but that doesn't seem quite right either.

Dummy descriptors that do nothing but cause an interrupt is the intent.

Quoted 9 lines

Quoted 9 lines

drivers/dma/pasemi_dma.c line 80

Quoted 6 lines

drivers/dma/pasemi_dma.c new file +6 -0

+static void pasemi_dma_clean(struct pasemi_dma_chan *chan)
+{
+       int old, new, i;
+       unsigned long flags;
+       struct pasemi_dma_desc *desc;
+       spin_lock_irqsave(&chan->desc_lock, flags);

Is spin_lock_bh() insufficient here?

Quoted 6 lines

Quoted 1 line

...that's all that jumps out at the moment.

Can't do that if it's called both from the polling as well as the IRQ context, it'd need to hold off irqs. I.e. once I add the DMA_INTERRUPT support it will be needed.

...do it in a tasklet.

Quoted 1 line

Dummy descriptors that do nothing but cause an interrupt is the intent.

Well, it'd be slightly more efficient to do add the interrupt attribute to the last issued descriptor when it's known in advance. If the underlying driver doesn't support it, adding a separate descriptor would be a good fallback.

Anyway, this isn't likely to be a performance bottleneck. If it turns out to be, I'll refactor it and submit patches.

Quoted 6 lines

Quoted 4 lines

Can't do that if it's called both from the polling as well as the IRQ context, it'd need to hold off irqs. I.e. once I add the DMA_INTERRUPT support it will be needed.

...do it in a tasklet.

Why? That just adds overhead and latency.

pasemi_dma: Driver for PA Semi PWRficient on-chip DMA engine

DMA copy offload driver for PA Semi PWRficient. It uses the

platform-specific functions to allocate channels, etc.

Signed-off-by Olof Johansson <[email protected]>

---

Changes since last post:

* Add DMA_INTERRUPT support and handling of the interrupt flag
* Fix interrupt handler for above and add tasklet
* Switch to spin_lock_bh() where possible
* Remove empty dependency_added() function since it's no longer used in the framework.
* Fix bug in "ring full" estimation.

Note that this still needs to go on top of the powerpc.git tree due to the pasemi_dma.h updates that this driver depends on. I suggest merging this through pasemi.git->powerpc.git->linus with an Acked-by from the DMA guys.

-Olof

--- /dev/null
+++ b/drivers/dma/pasemi_dma.c

+static void pasemi_dma_clean(struct pasemi_dma_chan *chan) +{
+ int old, new, i;

(quoted diff) +2 -0

+       struct pasemi_dma_desc *desc;
 +       dma_async_tx_callback callback;
 +
 +restart:
 +       spin_lock_bh(&chan->desc_lock);
 
+
 +       old = chan->next_to_clean;
 +
 +       new = *chan->chan.status & PAS_STATUS_PCNT_M;
 +       new <<= 2;
 +       new &= (RING_SZ-1);
 +
 +       if (old > new)
 +               new += RING_SZ;
 +
 +       for (i = old; i < new; i += 4) {
 +               if (unlikely(chan->chan.ring_virt[i & (RING_SZ-1)] & XCT_COPY_O))
 +                       break;
 +               desc = chan->ring_info[i & (RING_SZ-1)];
 +
 +               callback = desc->async_tx.callback;
 +               if (callback) {

Looks good, makes me want to go back and cleanup iop-adma a bit. A few fyi's below, but no other review comments.

Quoted 3 lines

Note that this still needs to go on top of the powerpc.git tree due to the pasemi_dma.h updates that this driver depends on. I suggest merging this through pasemi.git->powerpc.git->linus with an Acked-by from the DMA guys.

Ok, it still may not compile in mainline until after 2.6.26-rc1 due to additional dmaengine cleanups like the ack-to-flags change I posted earlier. Any better way to handle this? Go through -mm?

Acked-by Dan Williams <[email protected]>

From: Olof Johansson [mailto:[email protected]]=20
Sent: Sunday, March 16, 2008 2:30 PM

pasemi_dma: Driver for PA Semi PWRficient on-chip DMA engine

DMA copy offload driver for PA Semi PWRficient. It uses the platform-specific functions to allocate channels, etc.

Signed-off-by Olof Johansson <[email protected]>

---

Changes since last post:

* Add DMA_INTERRUPT support and handling of the interrupt flag
* Fix interrupt handler for above and add tasklet
* Switch to spin_lock_bh() where possible
* Remove empty dependency_added() function since it's no longer used in the framework.
* Fix bug in "ring full" estimation.

Note that this still needs to go on top of the powerpc.git=20 tree due to the
pasemi_dma.h updates that this driver depends on. I suggest=20 merging this
through pasemi.git->powerpc.git->linus with an Acked-by from=20 the DMA guys.

-Olof

[...]
=20

drivers/dma/pasemi_dma.c new file +4 -0

+
+static unsigned int channels =3D 4;
+module_param(channels, uint, S_IRUGO);
+MODULE_PARM_DESC(channels, "Number of channels for copy=20

(default: 2)");

In the future please copy Maciej as one of "the DMA guys" as he has taken over ioatdma for me. Beyond that, one little picky comment below...

Quoted 4 lines

drivers/dma/pasemi_dma.c new file +3 -0

+static unsigned int channels = 4;
+module_param(channels, uint, S_IRUGO);
+MODULE_PARM_DESC(channels, "Number of channels for copy 

(default: 2)");

Is the number of channels defaulting to 2 or 4?