[PATCH] fsldma: move DMA_SLAVE support functions to the driver

[PATCH] fsldma: move DMA_SLAVE support functions to the driver

[Ira W. Snyder]

The DMA_SLAVE support functions all existed as static inlines in the
driver specific header arch/powerpc/include/asm/fsldma.h. Move the body
of the functions to the driver itself, and EXPORT_SYMBOL_GPL() them.

At the same time, add the missing linux/list.h header.

Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
---

Hello AKPM, Dan,

At AKPM's request, I moved these static inline'd functions into the driver
code itself, and then EXPORT_SYMBOL_GPL()'d them. I also added a gfp
parameter to the fsl_dma_slave_alloc() function. And included the
linux/list.h header.

If you'd like three separate patches, I can do that. The changes were so
trivial that I lumped all of them into a single patch.

Thanks,
Ira

 arch/powerpc/include/asm/fsldma.h |   70 +++------------------------------
 drivers/dma/fsldma.c              |   77 +++++++++++++++++++++++++++++++++++++
 2 files changed, 83 insertions(+), 64 deletions(-)

diff --git a/arch/powerpc/include/asm/fsldma.h b/arch/powerpc/include/asm/fsldma.h
index debc5ed..34ec00b 100644
--- a/arch/powerpc/include/asm/fsldma.h
+++ b/arch/powerpc/include/asm/fsldma.h
@@ -1,7 +1,7 @@
 /*
  * Freescale MPC83XX / MPC85XX DMA Controller
  *
- * Copyright (c) 2009 Ira W. Snyder <iws@ovro.caltech.edu>
+ * Copyright (c) 2009-2010 Ira W. Snyder <iws@ovro.caltech.edu>
  *
  * This file is licensed under the terms of the GNU General Public License
  * version 2. This program is licensed "as is" without any warranty of any
@@ -11,6 +11,7 @@
 #ifndef __ARCH_POWERPC_ASM_FSLDMA_H__
 #define __ARCH_POWERPC_ASM_FSLDMA_H__
 
+#include <linux/list.h>
 #include <linux/slab.h>
 #include <linux/dmaengine.h>
 
@@ -69,69 +70,10 @@ struct fsl_dma_slave {
 	bool external_pause;
 };
 
-/**
- * fsl_dma_slave_append - add an address/length pair to a struct fsl_dma_slave
- * @slave: the &struct fsl_dma_slave to add to
- * @address: the hardware address to add
- * @length: the length of bytes to transfer from @address
- *
- * Add a hardware address/length pair to a struct fsl_dma_slave. Returns 0 on
- * success, -ERRNO otherwise.
- */
-static inline int fsl_dma_slave_append(struct fsl_dma_slave *slave,
-				       dma_addr_t address, size_t length)
-{
-	struct fsl_dma_hw_addr *addr;
-
-	addr = kzalloc(sizeof(*addr), GFP_ATOMIC);
-	if (!addr)
-		return -ENOMEM;
-
-	INIT_LIST_HEAD(&addr->entry);
-	addr->address = address;
-	addr->length = length;
-
-	list_add_tail(&addr->entry, &slave->addresses);
-	return 0;
-}
-
-/**
- * fsl_dma_slave_free - free a struct fsl_dma_slave
- * @slave: the struct fsl_dma_slave to free
- *
- * Free a struct fsl_dma_slave and all associated address/length pairs
- */
-static inline void fsl_dma_slave_free(struct fsl_dma_slave *slave)
-{
-	struct fsl_dma_hw_addr *addr, *tmp;
-
-	if (slave) {
-		list_for_each_entry_safe(addr, tmp, &slave->addresses, entry) {
-			list_del(&addr->entry);
-			kfree(addr);
-		}
-
-		kfree(slave);
-	}
-}
-
-/**
- * fsl_dma_slave_alloc - allocate a struct fsl_dma_slave
- * @gfp: the flags to pass to kmalloc when allocating this structure
- *
- * Allocate a struct fsl_dma_slave for use by the DMA_SLAVE API. Returns a new
- * struct fsl_dma_slave on success, or NULL on failure.
- */
-static inline struct fsl_dma_slave *fsl_dma_slave_alloc(gfp_t gfp)
-{
-	struct fsl_dma_slave *slave;
-
-	slave = kzalloc(sizeof(*slave), gfp);
-	if (!slave)
-		return NULL;
+struct fsl_dma_slave *fsl_dma_slave_alloc(gfp_t gfp);
+void fsl_dma_slave_free(struct fsl_dma_slave *slave);
 
-	INIT_LIST_HEAD(&slave->addresses);
-	return slave;
-}
+int fsl_dma_slave_append(struct fsl_dma_slave *slave, dma_addr_t address,
+			 size_t length, gfp_t gfp);
 
 #endif /* __ARCH_POWERPC_ASM_FSLDMA_H__ */
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index cea08be..f436ca4 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -38,6 +38,83 @@
 #include <asm/fsldma.h>
 #include "fsldma.h"
 
+/*
+ * External API
+ */
+
+/**
+ * fsl_dma_slave_append - add an address/length pair to a struct fsl_dma_slave
+ * @slave: the &struct fsl_dma_slave to add to
+ * @address: the hardware address to add
+ * @length: the length of bytes to transfer from @address
+ * @gfp: the flags to pass to kmalloc when allocating memory
+ *
+ * Add a hardware address/length pair to a struct fsl_dma_slave. Returns 0 on
+ * success, -ERRNO otherwise.
+ */
+int fsl_dma_slave_append(struct fsl_dma_slave *slave, dma_addr_t address,
+			 size_t length, gfp_t gfp)
+{
+	struct fsl_dma_hw_addr *addr;
+
+	addr = kzalloc(sizeof(*addr), gfp);
+	if (!addr)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&addr->entry);
+	addr->address = address;
+	addr->length = length;
+
+	list_add_tail(&addr->entry, &slave->addresses);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fsl_dma_slave_append);
+
+/**
+ * fsl_dma_slave_free - free a struct fsl_dma_slave
+ * @slave: the struct fsl_dma_slave to free
+ *
+ * Free a struct fsl_dma_slave and all associated address/length pairs
+ */
+void fsl_dma_slave_free(struct fsl_dma_slave *slave)
+{
+	struct fsl_dma_hw_addr *addr, *tmp;
+
+	if (slave) {
+		list_for_each_entry_safe(addr, tmp, &slave->addresses, entry) {
+			list_del(&addr->entry);
+			kfree(addr);
+		}
+
+		kfree(slave);
+	}
+}
+EXPORT_SYMBOL_GPL(fsl_dma_slave_free);
+
+/**
+ * fsl_dma_slave_alloc - allocate a struct fsl_dma_slave
+ * @gfp: the flags to pass to kmalloc when allocating this structure
+ *
+ * Allocate a struct fsl_dma_slave for use by the DMA_SLAVE API. Returns a new
+ * struct fsl_dma_slave on success, or NULL on failure.
+ */
+struct fsl_dma_slave *fsl_dma_slave_alloc(gfp_t gfp)
+{
+	struct fsl_dma_slave *slave;
+
+	slave = kzalloc(sizeof(*slave), gfp);
+	if (!slave)
+		return NULL;
+
+	INIT_LIST_HEAD(&slave->addresses);
+	return slave;
+}
+EXPORT_SYMBOL_GPL(fsl_dma_slave_alloc);
+
+/*
+ * Driver Code
+ */
+
 static void dma_init(struct fsldma_chan *chan)
 {
 	/* Reset the channel */
-- 
1.7.1

Re: [PATCH] fsldma: move DMA_SLAVE support functions to the driver

[Dan Williams]

On Tue, Sep 7, 2010 at 1:45 PM, Ira W. Snyder <iws@ovro.caltech.edu> wrote:
> The DMA_SLAVE support functions all existed as static inlines in the
> driver specific header arch/powerpc/include/asm/fsldma.h. Move the body
> of the functions to the driver itself, and EXPORT_SYMBOL_GPL() them.
>
> At the same time, add the missing linux/list.h header.
>
> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> ---
>
> Hello AKPM, Dan,

Hi, I'm back from paternity leave.

>
> At AKPM's request, I moved these static inline'd functions into the driver
> code itself, and then EXPORT_SYMBOL_GPL()'d them. I also added a gfp
> parameter to the fsl_dma_slave_alloc() function. And included the
> linux/list.h header.
>
> If you'd like three separate patches, I can do that. The changes were so
> trivial that I lumped all of them into a single patch.
>
> Thanks,
> Ira
>
>  arch/powerpc/include/asm/fsldma.h |   70 +++------------------------------
>  drivers/dma/fsldma.c              |   77 +++++++++++++++++++++++++++++++++++++

Remind me why we need this header file and these routines again?  git
grep says nothing in-tree is using these?

I also do not want to proliferate driver specific interfaces.  It
defeats the purpose of having a common dmaengine structure.  Along
these lines Linus W. added the new device_control() method (first
added in commit c3635c78, later tweaked in 05827630).  Could that be
massaged for your needs?

--
Dan

Re: [PATCH] fsldma: move DMA_SLAVE support functions to the driver

[Ira W. Snyder]

On Thu, Sep 23, 2010 at 11:31:11AM -0700, Dan Williams wrote:
> On Tue, Sep 7, 2010 at 1:45 PM, Ira W. Snyder <iws@ovro.caltech.edu> wrote:
> > The DMA_SLAVE support functions all existed as static inlines in the
> > driver specific header arch/powerpc/include/asm/fsldma.h. Move the body
> > of the functions to the driver itself, and EXPORT_SYMBOL_GPL() them.
> >
> > At the same time, add the missing linux/list.h header.
> >
> > Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> > ---
> >
> > Hello AKPM, Dan,
> 
> Hi, I'm back from paternity leave.
> 
> >
> > At AKPM's request, I moved these static inline'd functions into the driver
> > code itself, and then EXPORT_SYMBOL_GPL()'d them. I also added a gfp
> > parameter to the fsl_dma_slave_alloc() function. And included the
> > linux/list.h header.
> >
> > If you'd like three separate patches, I can do that. The changes were so
> > trivial that I lumped all of them into a single patch.
> >
> > Thanks,
> > Ira
> >
> >  arch/powerpc/include/asm/fsldma.h |   70 +++------------------------------
> >  drivers/dma/fsldma.c              |   77 +++++++++++++++++++++++++++++++++++++
> 
> Remind me why we need this header file and these routines again?  git
> grep says nothing in-tree is using these?
> 
> I also do not want to proliferate driver specific interfaces.  It
> defeats the purpose of having a common dmaengine structure.  Along
> these lines Linus W. added the new device_control() method (first
> added in commit c3635c78, later tweaked in 05827630).  Could that be
> massaged for your needs?
> 

These routines are used by a (currently out of tree) driver that I
maintain. Please see the LKML and linuxppc-dev ML post titled:
[PATCH RFCv2 0/5] CARMA Board Support

The FPGA programming driver uses these interfaces. Past a few initial
comments (which I promptly fixed!), it seems that nobody wants to look
at this driver. I don't know how to move it towards mainline other than
spam people, which I would prefer to avoid doing.

The new device_control() method doesn't have enough information for this
purpose. I also don't know of any other DMA controllers that have the
feature I'm using (described below).

The fsldma controller has the option of being controlled by an external
device, via some pins. This is leveraged by our system controller to
properly program some big data processing FPGAs.

Basically, the fsldma controller must be told:
1) descriptors (src, dst, len tuples)
2) use the external-start feature (external control)
3) go

Now #3's "go" doesn't actually start the transfer. It just starts
listening on the external pins for the correct sequence for each
transaction.

Also, the descriptors must be set up such that they form a single chain,
without interruptions between segments. Back at the beginning of the
year, I did a lot of work on the fsldma driver. It *should* be able to
chain DMA transfers appropriately for this mode using the existing
device_prep_dma_memcpy() routine.

This means that it would be possible to move some of the functionality
of fsldma's device_prep_slave_sg() into core DMAEngine code, and
implement something like dma_memcpy_sg_to_sg(). This would take two
scatterlists and call device_prep_slave_sg() enough times to setup a DMA
operation that will move all data appropriately.

This is exactly what fsldma's device_prep_slave_sg() actually does. It
was non-trivial to get the triply nested loops correct.

As an example from my driver, I have this scenario:
1) a scatterlist of kernel memory pages (this is my data payload)
2) a fixed set of non-incrementing physical addresses I need to send that data to

Other than the fsldma device_prep_slave_sg(), this is difficult to do.

I hope that helps to explain the problem.

Ira

Re: [PATCH] fsldma: move DMA_SLAVE support functions to the driver

[Dan Williams]

On Thu, Sep 23, 2010 at 1:12 PM, Ira W. Snyder <iws@ovro.caltech.edu> wrote:
> On Thu, Sep 23, 2010 at 11:31:11AM -0700, Dan Williams wrote:
>> On Tue, Sep 7, 2010 at 1:45 PM, Ira W. Snyder <iws@ovro.caltech.edu> wrote:
>> > The DMA_SLAVE support functions all existed as static inlines in the
>> > driver specific header arch/powerpc/include/asm/fsldma.h. Move the body
>> > of the functions to the driver itself, and EXPORT_SYMBOL_GPL() them.
>> >
>> > At the same time, add the missing linux/list.h header.
>> >
>> > Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
>> > ---
>> >
>> > Hello AKPM, Dan,
>>
>> Hi, I'm back from paternity leave.
>>
>> >
>> > At AKPM's request, I moved these static inline'd functions into the driver
>> > code itself, and then EXPORT_SYMBOL_GPL()'d them. I also added a gfp
>> > parameter to the fsl_dma_slave_alloc() function. And included the
>> > linux/list.h header.
>> >
>> > If you'd like three separate patches, I can do that. The changes were so
>> > trivial that I lumped all of them into a single patch.
>> >
>> > Thanks,
>> > Ira
>> >
>> >  arch/powerpc/include/asm/fsldma.h |   70 +++------------------------------
>> >  drivers/dma/fsldma.c              |   77 +++++++++++++++++++++++++++++++++++++
>>
>> Remind me why we need this header file and these routines again?  git
>> grep says nothing in-tree is using these?
>>
>> I also do not want to proliferate driver specific interfaces.  It
>> defeats the purpose of having a common dmaengine structure.  Along
>> these lines Linus W. added the new device_control() method (first
>> added in commit c3635c78, later tweaked in 05827630).  Could that be
>> massaged for your needs?
>>
>
> These routines are used by a (currently out of tree) driver that I
> maintain. Please see the LKML and linuxppc-dev ML post titled:
> [PATCH RFCv2 0/5] CARMA Board Support
>
> The FPGA programming driver uses these interfaces. Past a few initial
> comments (which I promptly fixed!), it seems that nobody wants to look
> at this driver. I don't know how to move it towards mainline other than
> spam people, which I would prefer to avoid doing.
>
> The new device_control() method doesn't have enough information for this
> purpose. I also don't know of any other DMA controllers that have the
> feature I'm using (described below).
>
> The fsldma controller has the option of being controlled by an external
> device, via some pins. This is leveraged by our system controller to
> properly program some big data processing FPGAs.
>
> Basically, the fsldma controller must be told:
> 1) descriptors (src, dst, len tuples)
> 2) use the external-start feature (external control)
> 3) go
>
> Now #3's "go" doesn't actually start the transfer. It just starts
> listening on the external pins for the correct sequence for each
> transaction.
>
> Also, the descriptors must be set up such that they form a single chain,
> without interruptions between segments. Back at the beginning of the
> year, I did a lot of work on the fsldma driver. It *should* be able to
> chain DMA transfers appropriately for this mode using the existing
> device_prep_dma_memcpy() routine.
>
> This means that it would be possible to move some of the functionality
> of fsldma's device_prep_slave_sg() into core DMAEngine code, and
> implement something like dma_memcpy_sg_to_sg(). This would take two
> scatterlists and call device_prep_slave_sg() enough times to setup a DMA
> operation that will move all data appropriately.
>
> This is exactly what fsldma's device_prep_slave_sg() actually does. It
> was non-trivial to get the triply nested loops correct.
>
> As an example from my driver, I have this scenario:
> 1) a scatterlist of kernel memory pages (this is my data payload)
> 2) a fixed set of non-incrementing physical addresses I need to send that data to
>
> Other than the fsldma device_prep_slave_sg(), this is difficult to do.
>
> I hope that helps to explain the problem.

Yes, thanks now I recall, and I remember saying something about how
this passes the "Voyager test".  That said it seems odd to carry
symbol exports for something like this.  Instead of exporting can we
exploit the fact that the fpga driver already knows it has a
fsldma_device pointer and do something like:

struct fsldma_device *fdev = container_of(dma, typeof(fdev), common);

fdev->slave_alloc();

This keeps it contained to the fsldma driver.

--
Dan

Re: [PATCH] fsldma: move DMA_SLAVE support functions to the driver

[Ira W. Snyder]

On Thu, Sep 23, 2010 at 02:58:06PM -0700, Dan Williams wrote:
> On Thu, Sep 23, 2010 at 1:12 PM, Ira W. Snyder <iws@ovro.caltech.edu> wrote:
> > On Thu, Sep 23, 2010 at 11:31:11AM -0700, Dan Williams wrote:
> >> On Tue, Sep 7, 2010 at 1:45 PM, Ira W. Snyder <iws@ovro.caltech.edu> wrote:
> >> > The DMA_SLAVE support functions all existed as static inlines in the
> >> > driver specific header arch/powerpc/include/asm/fsldma.h. Move the body
> >> > of the functions to the driver itself, and EXPORT_SYMBOL_GPL() them.
> >> >
> >> > At the same time, add the missing linux/list.h header.
> >> >
> >> > Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> >> > ---
> >> >
> >> > Hello AKPM, Dan,
> >>
> >> Hi, I'm back from paternity leave.
> >>
> >> >
> >> > At AKPM's request, I moved these static inline'd functions into the driver
> >> > code itself, and then EXPORT_SYMBOL_GPL()'d them. I also added a gfp
> >> > parameter to the fsl_dma_slave_alloc() function. And included the
> >> > linux/list.h header.
> >> >
> >> > If you'd like three separate patches, I can do that. The changes were so
> >> > trivial that I lumped all of them into a single patch.
> >> >
> >> > Thanks,
> >> > Ira
> >> >
> >> >  arch/powerpc/include/asm/fsldma.h |   70 +++------------------------------
> >> >  drivers/dma/fsldma.c              |   77 +++++++++++++++++++++++++++++++++++++
> >>
> >> Remind me why we need this header file and these routines again?  git
> >> grep says nothing in-tree is using these?
> >>
> >> I also do not want to proliferate driver specific interfaces.  It
> >> defeats the purpose of having a common dmaengine structure.  Along
> >> these lines Linus W. added the new device_control() method (first
> >> added in commit c3635c78, later tweaked in 05827630).  Could that be
> >> massaged for your needs?
> >>
> >
> > These routines are used by a (currently out of tree) driver that I
> > maintain. Please see the LKML and linuxppc-dev ML post titled:
> > [PATCH RFCv2 0/5] CARMA Board Support
> >
> > The FPGA programming driver uses these interfaces. Past a few initial
> > comments (which I promptly fixed!), it seems that nobody wants to look
> > at this driver. I don't know how to move it towards mainline other than
> > spam people, which I would prefer to avoid doing.
> >
> > The new device_control() method doesn't have enough information for this
> > purpose. I also don't know of any other DMA controllers that have the
> > feature I'm using (described below).
> >
> > The fsldma controller has the option of being controlled by an external
> > device, via some pins. This is leveraged by our system controller to
> > properly program some big data processing FPGAs.
> >
> > Basically, the fsldma controller must be told:
> > 1) descriptors (src, dst, len tuples)
> > 2) use the external-start feature (external control)
> > 3) go
> >
> > Now #3's "go" doesn't actually start the transfer. It just starts
> > listening on the external pins for the correct sequence for each
> > transaction.
> >
> > Also, the descriptors must be set up such that they form a single chain,
> > without interruptions between segments. Back at the beginning of the
> > year, I did a lot of work on the fsldma driver. It *should* be able to
> > chain DMA transfers appropriately for this mode using the existing
> > device_prep_dma_memcpy() routine.
> >
> > This means that it would be possible to move some of the functionality
> > of fsldma's device_prep_slave_sg() into core DMAEngine code, and
> > implement something like dma_memcpy_sg_to_sg(). This would take two
> > scatterlists and call device_prep_slave_sg() enough times to setup a DMA
> > operation that will move all data appropriately.
> >
> > This is exactly what fsldma's device_prep_slave_sg() actually does. It
> > was non-trivial to get the triply nested loops correct.
> >
> > As an example from my driver, I have this scenario:
> > 1) a scatterlist of kernel memory pages (this is my data payload)
> > 2) a fixed set of non-incrementing physical addresses I need to send that data to
> >
> > Other than the fsldma device_prep_slave_sg(), this is difficult to do.
> >
> > I hope that helps to explain the problem.
> 
> Yes, thanks now I recall, and I remember saying something about how
> this passes the "Voyager test".  That said it seems odd to carry
> symbol exports for something like this.  Instead of exporting can we
> exploit the fact that the fpga driver already knows it has a
> fsldma_device pointer and do something like:
> 
> struct fsldma_device *fdev = container_of(dma, typeof(fdev), common);
> 
> fdev->slave_alloc();
> 
> This keeps it contained to the fsldma driver.
> 

I agree that it sucks to have exports for this. I'm happy with the
static inlines in a header, but AKPM didn't like them.

I think the solution above would actually need to be a struct
fsldma_chan instead of a struct fsldma_device, but otherwise would work.

The caveat with your solution above is that (parts of)
drivers/dma/fsldma.h need to be copied into
arch/powerpc/include/asm/fsldma.h for the type information. It seems a
bit awkward as well.

I'd like to propose another possible solution:
1) add an external_control member to struct dma_slave_config
2) remove the list_head from struct fsl_dma_slave
3) remove all of the functions from arch/powerpc/include/asm/fsldma.h
4) add dma_async_memcpy_sg_to_sg()

dma_cookie_t dma_async_memcpy_sg_to_sg(struct dma_chan *chan,
				       struct *dst_sg, int dst_nents,
				       struct *src_sg, int src_nents,
				       dma_async_tx_callback cb,
				       void *cb_param);

This function would do most of the current work that the current fsldma
device_prep_slave_sg() does: setting up a DMA chain to transfer between
two scatterlists.

I'm willing to do an ugly hack to make a fake scatterlist with my fixed
non-incrementing hardware addresses for the dst_sg. Like this:

sg_dma_address(sg) = MY_FIXED_ADDRESS_1;
sg_dma_len(sg) = MY_FIXED_LENGTH_1;
sg = sg_next(sg);
sg_dma_address(sg) = MY_FIXED_ADDRESS_2;
sg_dma_len(sg) = MY_FIXED_LENGTH_2;

Now my driver would look like this:

struct dma_slave_config dsc;
dsc.external_control = true;

/* allocate and fill my dst_sg with the fake data, like above */
/* DO NOT use dma_map_sg() on the fake scatterlist */
/* allocate and fill my src_sg with pages + data */
src_nents = dma_map_sg(..., src_sg, DMA_TO_DEVICE);

/* sets up the DMA transfer, but doesn't start it */
cookie = dma_async_memcpy_sg_to_sg(chan, dst_sg, dst_nents,
				   src_sg, src_nents,
				   my_cb, my_cb_param);

/* puts the DMA engine into external control mode */
ret = chan->device_control(chan, DMA_SLAVE_CONFIG, (unsigned long)dsc);

/* "start" the transfer */
dma_async_memcpy_issue_pending(chan);

/*
 * start the system controller FPGA programmer, which will toggle the
 * DMA engine's external control pins appropriately.
 *
 * Wait until my callback is called, which will wake up and continue
 * the function. Using wait_for_completion_timeout(), etc. Which is
 * exactly what I do now.
 */


The freescale DMA engine must have the external control bit set on every
new transaction. It is automatically cleared after a transaction
completes.

Ira

Re: [PATCH] fsldma: move DMA_SLAVE support functions to the driver

[Dan Williams]

[ copying Linus on this usage of slave_config ]

On Thu, Sep 23, 2010 at 4:20 PM, Ira W. Snyder <iws@ovro.caltech.edu> wrote:
> On Thu, Sep 23, 2010 at 02:58:06PM -0700, Dan Williams wrote:
[snip discussion of external slave driver]
>> Yes, thanks now I recall, and I remember saying something about how
>> this passes the "Voyager test".  That said it seems odd to carry
>> symbol exports for something like this.  Instead of exporting can we
>> exploit the fact that the fpga driver already knows it has a
>> fsldma_device pointer and do something like:
>>
>> struct fsldma_device *fdev = container_of(dma, typeof(fdev), common);
>>
>> fdev->slave_alloc();
>>
>> This keeps it contained to the fsldma driver.
>>
>
> I agree that it sucks to have exports for this. I'm happy with the
> static inlines in a header, but AKPM didn't like them.
>
> I think the solution above would actually need to be a struct
> fsldma_chan instead of a struct fsldma_device, but otherwise would work.
>
> The caveat with your solution above is that (parts of)
> drivers/dma/fsldma.h need to be copied into
> arch/powerpc/include/asm/fsldma.h for the type information. It seems a
> bit awkward as well.
>
> I'd like to propose another possible solution:
> 1) add an external_control member to struct dma_slave_config

Will you be using the other slave_config fields, or would it be better
to have a new dma_crtl_cmd?

> 2) remove the list_head from struct fsl_dma_slave
> 3) remove all of the functions from arch/powerpc/include/asm/fsldma.h
> 4) add dma_async_memcpy_sg_to_sg()
>
> dma_cookie_t dma_async_memcpy_sg_to_sg(struct dma_chan *chan,
>                                       struct *dst_sg, int dst_nents,
>                                       struct *src_sg, int src_nents,
>                                       dma_async_tx_callback cb,
>                                       void *cb_param);
>
> This function would do most of the current work that the current fsldma
> device_prep_slave_sg() does: setting up a DMA chain to transfer between
> two scatterlists.

Ok just wrap it in an ifdef CONFIG_DMAENGINE_SG_TO_SG so it compiles
away on platforms that don't care about it.  I carried an out-of-tree
config option CONFIG_ARCH_HAS_ASYNC_TX_FIND_CHANNEL for a driver that
eventually made it upstream, so there is precedent for such a thing.

I assume it will implement this as a series of calls to prep_slave_sg
or prep_memcpy and be compatible with other dma drivers?

>
> I'm willing to do an ugly hack to make a fake scatterlist with my fixed
> non-incrementing hardware addresses for the dst_sg. Like this:
>
> sg_dma_address(sg) = MY_FIXED_ADDRESS_1;
> sg_dma_len(sg) = MY_FIXED_LENGTH_1;
> sg = sg_next(sg);
> sg_dma_address(sg) = MY_FIXED_ADDRESS_2;
> sg_dma_len(sg) = MY_FIXED_LENGTH_2;
>
> Now my driver would look like this:
>
> struct dma_slave_config dsc;
> dsc.external_control = true;
>
> /* allocate and fill my dst_sg with the fake data, like above */
> /* DO NOT use dma_map_sg() on the fake scatterlist */
> /* allocate and fill my src_sg with pages + data */
> src_nents = dma_map_sg(..., src_sg, DMA_TO_DEVICE);
>
> /* sets up the DMA transfer, but doesn't start it */
> cookie = dma_async_memcpy_sg_to_sg(chan, dst_sg, dst_nents,
>                                   src_sg, src_nents,
>                                   my_cb, my_cb_param);
>
> /* puts the DMA engine into external control mode */
> ret = chan->device_control(chan, DMA_SLAVE_CONFIG, (unsigned long)dsc);
>
> /* "start" the transfer */
> dma_async_memcpy_issue_pending(chan);
>
> /*
>  * start the system controller FPGA programmer, which will toggle the
>  * DMA engine's external control pins appropriately.
>  *
>  * Wait until my callback is called, which will wake up and continue
>  * the function. Using wait_for_completion_timeout(), etc. Which is
>  * exactly what I do now.
>  */
>
>
> The freescale DMA engine must have the external control bit set on every
> new transaction. It is automatically cleared after a transaction
> completes.

I think this meets the spirit of upstream inclusion (documents and
provides a tested usage model for a piece of hardware), does so using
common (ish) interfaces, and has intrinsic value outside of enabling
an out-of-tree driver.

--
Dan

Re: [PATCH] fsldma: move DMA_SLAVE support functions to the driver

[Ira W. Snyder]

On Thu, Sep 23, 2010 at 05:19:27PM -0700, Dan Williams wrote:
> [ copying Linus on this usage of slave_config ]
> 
> On Thu, Sep 23, 2010 at 4:20 PM, Ira W. Snyder <iws@ovro.caltech.edu> wrote:
> > On Thu, Sep 23, 2010 at 02:58:06PM -0700, Dan Williams wrote:
> [snip discussion of external slave driver]
> >> Yes, thanks now I recall, and I remember saying something about how
> >> this passes the "Voyager test".  That said it seems odd to carry
> >> symbol exports for something like this.  Instead of exporting can we
> >> exploit the fact that the fpga driver already knows it has a
> >> fsldma_device pointer and do something like:
> >>
> >> struct fsldma_device *fdev = container_of(dma, typeof(fdev), common);
> >>
> >> fdev->slave_alloc();
> >>
> >> This keeps it contained to the fsldma driver.
> >>
> >
> > I agree that it sucks to have exports for this. I'm happy with the
> > static inlines in a header, but AKPM didn't like them.
> >
> > I think the solution above would actually need to be a struct
> > fsldma_chan instead of a struct fsldma_device, but otherwise would work.
> >
> > The caveat with your solution above is that (parts of)
> > drivers/dma/fsldma.h need to be copied into
> > arch/powerpc/include/asm/fsldma.h for the type information. It seems a
> > bit awkward as well.
> >
> > I'd like to propose another possible solution:
> > 1) add an external_control member to struct dma_slave_config
> 
> Will you be using the other slave_config fields, or would it be better
> to have a new dma_crtl_cmd?
> 

I wouldn't need them, no. I'm only interested in the external_start and
request_count features. The Freescale DMA controller doesn't burst if
the "hold src/dst address constant" feature is used. Blame the hardware
guys.

I work around it by having a 4K fifo. All writes which hit the 4K of
address space go into the programmer's fifo. Two scatterlists fill this
requirement nicely: src scatterlist has X bytes of kernel memory, dst
scatterlist has X bytes of 4K entries pointing to the fifo.

Re-reading the description for the struct dma_slave_config (comments
above it), it says that controllers *may* have a custom structure which
they embed this structure inside. Like this:

struct fsldma_slave_config {
	struct dma_slave_config cfg;
	bool external_start;
	unsigned int request_count;
};

I guess that is why device_control() takes an unsigned long argument
instead of a struct dma_slave_config *. I'm happy to do that and not
require another command.

> > 2) remove the list_head from struct fsl_dma_slave
> > 3) remove all of the functions from arch/powerpc/include/asm/fsldma.h
> > 4) add dma_async_memcpy_sg_to_sg()
> >
> > dma_cookie_t dma_async_memcpy_sg_to_sg(struct dma_chan *chan,
> >                                       struct *dst_sg, int dst_nents,
> >                                       struct *src_sg, int src_nents,
> >                                       dma_async_tx_callback cb,
> >                                       void *cb_param);
> >
> > This function would do most of the current work that the current fsldma
> > device_prep_slave_sg() does: setting up a DMA chain to transfer between
> > two scatterlists.
> 
> Ok just wrap it in an ifdef CONFIG_DMAENGINE_SG_TO_SG so it compiles
> away on platforms that don't care about it.  I carried an out-of-tree
> config option CONFIG_ARCH_HAS_ASYNC_TX_FIND_CHANNEL for a driver that
> eventually made it upstream, so there is precedent for such a thing.
> 
> I assume it will implement this as a series of calls to prep_slave_sg
> or prep_memcpy and be compatible with other dma drivers?
> 

Yes, it will only use prep_memcpy(). The fsldma driver chains all calls
to prep_memcpy() together until dma_async_memcpy_issue_pending() is
called.

> >
> > I'm willing to do an ugly hack to make a fake scatterlist with my fixed
> > non-incrementing hardware addresses for the dst_sg. Like this:
> >
> > sg_dma_address(sg) = MY_FIXED_ADDRESS_1;
> > sg_dma_len(sg) = MY_FIXED_LENGTH_1;
> > sg = sg_next(sg);
> > sg_dma_address(sg) = MY_FIXED_ADDRESS_2;
> > sg_dma_len(sg) = MY_FIXED_LENGTH_2;
> >
> > Now my driver would look like this:
> >
> > struct dma_slave_config dsc;
> > dsc.external_control = true;
> >
> > /* allocate and fill my dst_sg with the fake data, like above */
> > /* DO NOT use dma_map_sg() on the fake scatterlist */
> > /* allocate and fill my src_sg with pages + data */
> > src_nents = dma_map_sg(..., src_sg, DMA_TO_DEVICE);
> >
> > /* sets up the DMA transfer, but doesn't start it */
> > cookie = dma_async_memcpy_sg_to_sg(chan, dst_sg, dst_nents,
> >                                   src_sg, src_nents,
> >                                   my_cb, my_cb_param);
> >
> > /* puts the DMA engine into external control mode */
> > ret = chan->device_control(chan, DMA_SLAVE_CONFIG, (unsigned long)dsc);
> >
> > /* "start" the transfer */
> > dma_async_memcpy_issue_pending(chan);
> >
> > /*
> >  * start the system controller FPGA programmer, which will toggle the
> >  * DMA engine's external control pins appropriately.
> >  *
> >  * Wait until my callback is called, which will wake up and continue
> >  * the function. Using wait_for_completion_timeout(), etc. Which is
> >  * exactly what I do now.
> >  */
> >
> >
> > The freescale DMA engine must have the external control bit set on every
> > new transaction. It is automatically cleared after a transaction
> > completes.
> 
> I think this meets the spirit of upstream inclusion (documents and
> provides a tested usage model for a piece of hardware), does so using
> common (ish) interfaces, and has intrinsic value outside of enabling
> an out-of-tree driver.
> 

Yep. The fake scatterlist thing is ugly, but I don't have any better
ideas. I'll work on implementing this tomorrow.

Thanks for the feedback,
Ira