[PATCH] DMAENGINE: runtime config for slave channels

[PATCH] DMAENGINE: runtime config for slave channels

[Linus Walleij]

This adds an interface to the DMAengine to make it possible to
reconfigure a channel at runtime. We add a few foreseen config
parameters to the passed struct, with a void * pointer for custom
per-device or per-platform data.

Cc: Viresh Kumar <viresh.kumar@st.com>
Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
OK at night I realized that maybe what we do for runtime
re-configuration of DMA for the AMBA PrimeCells is actually generic
and should maybe be lifted into the DMAengine instead.

This conclusion came from Viresh stating that the SPEAr platform
will need runtime reconfiguration for other things than PrimeCells,
(on the PL08x driver) and actually we will likely need it ourselves
too.

So before starting to abuse that PrimeCell interface for things
not PrimeCell, this way I suggest making this generic from day 1.

Doing so makes it possible for me to merge the runtime
re-configuration support for DMA40, COH 901 318 and the current
iteration of the PL08x driver as well, and break that out of the
PrimeCell series and drop the include/linux/amba/dma.h entirely
while still getting some real nice and generic functionality
in place.

The PrimeCell patchset can then be nicely wrapped around this.

More genric run-time configurations can be added to the struct if
they are really generic like these, else the private_config can
be used for local obscurities.

Need your help on how to proceed on this one Dan.
---
 include/linux/dmaengine.h |   39 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 5204f01..18f536c 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -114,11 +114,17 @@ enum dma_ctrl_flags {
  * @DMA_TERMINATE_ALL: terminate all ongoing transfers
  * @DMA_PAUSE: pause ongoing transfers
  * @DMA_RESUME: resume paused transfer
+ * @DMA_SLAVE_CONFIG: this command is only implemented by DMA controllers
+ * that need to runtime reconfigure the slave channels (as opposed to passing
+ * configuration data in statically from the platform). An additional
+ * argument of struct dma_channel_config must be passed in with this
+ * command.
  */
 enum dma_ctrl_cmd {
 	DMA_TERMINATE_ALL,
 	DMA_PAUSE,
 	DMA_RESUME,
+	DMA_SLAVE_CONFIG,
 };
 
 /**
@@ -199,6 +205,39 @@ struct dma_chan_dev {
 	atomic_t *idr_ref;
 };
 
+/**
+ * struct dma_slave_channel_config - dma slave channel runtime config
+ * @addr: this is the physical address where DMA data should be
+ * read (RX) or written (TX)
+ * @addr_width: this is the width of the source (RX) or target
+ * (TX) register where DMA data shall be read/written, in bytes.
+ * legal values: 1, 2, 4, 8.
+ * @direction: whether the data goes in or out on this slave channel,
+ * right now.
+ * @maxburst: the maximum number of words (note: words, not bytes)
+ * that can be sent in one burst to the device. Typically something
+ * like half the FIFO depth on I/O peripherals so you don't
+ * overflow it.
+ * @private_config: if you need to pass in specialized configuration
+ * at runtime, apart from the generic things supported in this
+ * struct, you provide it in this pointer and dereference it inside
+ * your dmaengine driver to get the proper configuration bits out.
+ *
+ * This struct is passed in as configuration data to a DMA engine
+ * in order to set up a certain channel for DMA transport at runtime.
+ * The DMA device/engine has to provide support for an additional
+ * command in the channel config interface, DMA_SLAVE_CONFIG
+ * and this struct will then be passed in as an argument to the
+ * DMA engine device_control() function.
+ */
+struct dma_slave_channel_config {
+	dma_addr_t addr;
+	u8 addr_width:4;
+	enum dma_data_direction direction;
+	int maxburst;
+	void *private_config;
+};
+
 static inline const char *dma_chan_name(struct dma_chan *chan)
 {
 	return dev_name(&chan->dev->device);
-- 
1.6.3.3

Re: [PATCH] DMAENGINE: runtime config for slave channels

[Dan Williams]

On Sun, Jun 20, 2010 at 3:19 PM, Linus Walleij
<linus.walleij@stericsson.com> wrote:
> This adds an interface to the DMAengine to make it possible to
> reconfigure a channel at runtime. We add a few foreseen config
> parameters to the passed struct, with a void * pointer for custom
> per-device or per-platform data.
>
> Cc: Viresh Kumar <viresh.kumar@st.com>
> Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
> ---
> OK at night I realized that maybe what we do for runtime
> re-configuration of DMA for the AMBA PrimeCells is actually generic
> and should maybe be lifted into the DMAengine instead.
>
> This conclusion came from Viresh stating that the SPEAr platform
> will need runtime reconfiguration for other things than PrimeCells,
> (on the PL08x driver) and actually we will likely need it ourselves
> too.
>
> So before starting to abuse that PrimeCell interface for things
> not PrimeCell, this way I suggest making this generic from day 1.
>
> Doing so makes it possible for me to merge the runtime
> re-configuration support for DMA40, COH 901 318 and the current
> iteration of the PL08x driver as well, and break that out of the
> PrimeCell series and drop the include/linux/amba/dma.h entirely
> while still getting some real nice and generic functionality
> in place.
>
> The PrimeCell patchset can then be nicely wrapped around this.
>
> More genric run-time configurations can be added to the struct if
> they are really generic like these, else the private_config can
> be used for local obscurities.
>
> Need your help on how to proceed on this one Dan.

Is there a guarantee that all devices that implement this command will
support all the fields?  The presence of private_config concerns me
because that still seems to imply some hard coded knowledge about the
specific dma engine from the client.

...which is fine, but only if this is a common way to express
arch-specific extensions.  In other words it's not clear where the
line is drawn between "this is a common operation that different dma
drivers can use / extend incompatibly" versus "this truly a generic
api that a client driver can depend on to run unmodified across
architectures that have a supporting dma driver".  Clarifying that
would help end point device driver authors to know what they can rely
on... to date all the slave implementations have been written by the
same author who wrote the driver, so the information hiding has been
blurred.  It would be nice to define / document what is and is not
expected to work for this command across implementations.

--
Dan

RE: [PATCH] DMAENGINE: runtime config for slave channels

[Linus WALLEIJ]

[Dan Williams]

> Is there a guarantee that all devices that implement this command will
> support all the fields?

No. They may be ignored on some controllers.

> The presence of private_config concerns me
> because that still seems to imply some hard coded knowledge about the
> specific dma engine from the client.

Yes that is the idea.

> ...which is fine, but only if this is a common way to express
> arch-specific extensions. In other words it's not clear where the
> line is drawn between "this is a common operation that different dma
> drivers can use / extend incompatibly" versus "this truly a generic
> api that a client driver can depend on to run unmodified across
> architectures that have a supporting dma driver".  Clarifying that
> would help end point device driver authors to know what they can rely
> on... to date all the slave implementations have been written by the
> same author who wrote the driver, so the information hiding has been
> blurred.  It would be nice to define / document what is and is not
> expected to work for this command across implementations.

OK the idea is to have the generic stuff in
struct dma_slave_channel_config, and the only criteria I have for
deciding what is generic is what COH901318, DMA40 and PL08x need
to runtime configure to handle things like UART RX/TX and SPI 
DMA with varying wordwidth which is currently my most complex
usecases.

There may be DMA controllers that cannot specify the word width
for example, but most can. Those that cannot do this will ignore
that field, and as a consequence cannot be used to feed drivers
that make use of it with DMA.

This is a hardware restriction though: if your DMA controller cannot
control the word width you cannot use the PL022 driver with DMA
for example.

The private_config is intended for stuff that can never be
expressed in a generic way. For example Viresh told me that they
may need to runtime-control which bus master the PL080 is using.
Then the driver needs to take platform-specific actions, and
there will be some struct pl08x_slave_config passed in this
opaque pointer so the PL08x can be set up properly.

Since you don't seem to be totally against this approach I
will attempt to submit a more elaborate patch set.

Yours,
Linus Walleij