This adds an interface to the DMAengine to make it possible to
reconfigure a slave 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 runtime slave data.
Signed-off-by: Linus Walleij <[email protected]>
---
This version was revised after discussion, it splits the control
arguments in source/destination pairs to prepare for (among other
things) device to device transfers.
---
include/linux/dmaengine.h | 75 +++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 75 insertions(+), 0 deletions(-)
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 5204f01..406b820 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_slave_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,75 @@ struct dma_chan_dev {
atomic_t *idr_ref;
};
+/**
+ * enum dma_slave_buswidth - defines bus with of the DMA slave
+ * device, source or target busses
+ */
+enum dma_slave_buswidth {
+ DMA_SLAVE_BUSWIDTH_UNDEFINED = 0,
+ DMA_SLAVE_BUSWIDTH_1_BYTE = 1,
+ DMA_SLAVE_BUSWIDTH_2_BYTES = 2,
+ DMA_SLAVE_BUSWIDTH_4_BYTES = 4,
+ DMA_SLAVE_BUSWIDTH_8_BYTES = 8,
+};
+
+/**
+ * struct dma_slave_config - dma slave channel runtime config
+ * @direction: whether the data shall go in or out on this slave
+ * channel, right now. DMA_TO_DEVICE and DMA_FROM_DEVICE are
+ * legal values, DMA_BIDIRECTIONAL is not acceptable since we
+ * need to differentiate source and target addresses.
+ * @src_addr: this is the physical address where DMA slave data
+ * should be read (RX), if the source is memory this argument is
+ * ignored.
+ * @dst_addr: this is the physical address where DMA slave data
+ * should be written (TX), if the source is memory this argument
+ * is ignored.
+ * @src_addr_width: this is the width in bytes of the source (RX)
+ * register where DMA data shall be read. If the source
+ * is memory this may be ignored depending on architecture.
+ * Legal values: 1, 2, 4, 8.
+ * @dst_addr_width: same as src_addr_width but for destination
+ * target (TX) mutatis mutandis.
+ * @src_maxburst: the maximum number of words (note: words, as in
+ * units of the src_addr_width member, 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. This
+ * may or may not be applicable on memory sources.
+ * @dst_maxburst: same as src_maxburst but for destination target
+ * mutatus mutandis.
+ * @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.
+ *
+ * The rationale for adding configuration information to this struct
+ * is as follows: if it is likely that most DMA slave controllers in
+ * the world will support the configuration option, then make it
+ * generic. If not: if it is fixed so that it be sent in static from
+ * the platform data, then prefer to do that. Else, if it is neither
+ * fixed at runtime, nor generic enough (such as bus mastership on
+ * some CPU family and whatnot) then pass it in the private_config
+ * member and dereference it to some per-device struct in your driver.
+ */
+struct dma_slave_config {
+ enum dma_data_direction direction;
+ dma_addr_t src_addr;
+ dma_addr_t dst_addr;
+ enum dma_slave_buswidth src_addr_width;
+ enum dma_slave_buswidth dst_addr_width;
+ u32 src_maxburst;
+ u32 dst_maxburst;
+ void *private_config;
+};
+
static inline const char *dma_chan_name(struct dma_chan *chan)
{
return dev_name(&chan->dev->device);
--
1.7.1.1
2010/7/21 Linus Walleij <[email protected]>:
> This adds an interface to the DMAengine to make it possible to
> reconfigure a slave 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 runtime slave data.
BTW Vinod, if you're happy with this API, then please ACK it so
Dan has some indication whether it'll fit the Moorestown too.
Yours,
Linus Walleij
> 2010/7/22 Linus Walleij <[email protected]>:
>
> > This adds an interface to the DMAengine to make it possible to
> > reconfigure a slave 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 runtime slave data.
>
> BTW Vinod, if you're happy with this API, then please ACK it so
> Dan has some indication whether it'll fit the Moorestown too.
Shouldn't this patch remove the private member in dma_chan structure
Currently chan->private is used for sending slave or similar channel specific
information. Now if we want to add struct dma_slave_config, then IMHO it
would make sense to remove private variable and replace with dma_slave_config
struture. That way we can reuse this struture there as well and if someone wants
to add more stuff he can use the private_config.
Dan, what do you think about this?
Thanks
-Vinod
2010/7/22 Koul, Vinod <[email protected]>:
> Shouldn't this patch remove the private member in dma_chan structure
I don't think so.
> Currently chan->private is used for sending slave or similar channel specific
> information. Now if we want to add struct dma_slave_config, then IMHO it
> would make sense to remove private variable and replace with dma_slave_config
> struture. That way we can reuse this struture there as well and if someone wants
> to add more stuff he can use the private_config.
That member is described like this:
@private: private data for certain client-channel associations
chan->private is supposed to only be used inside the dmaengine
drivers themselves AFAICT.
Some drivers (like the txx9dmac.c) use this to hold the state of the
slave, whereas the void *private in the runtime config is supposed
to contain some custom configuration struct which is created
outside of the dmaengine framework and can be discarded
after use (if you so wish), so that's vastly different.
You could however argue (but this is another discussion altogether)
that using chan->private to hold any states is superfluous since
you could just have your struct dma_channel as a member of
your custom channel struct and use container_of to dereference
it (as we do in the coh901318 and ste_dma40 drivers) and then
stockpile any other struct members or substructs into your custom
struct but that's another issue, which would require major refactoring
of these drivers.
Yours,
Linus Walleij
On Thu, Jul 22, 2010 at 9:13 AM, Koul, Vinod <[email protected]> wrote:
>> 2010/7/22 Linus Walleij <[email protected]>:
>>
>> > This adds an interface to the DMAengine to make it possible to
>> > reconfigure a slave 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 runtime slave data.
>>
>> BTW Vinod, if you're happy with this API, then please ACK it so
>> Dan has some indication whether it'll fit the Moorestown too.
>
> Shouldn't this patch remove the private member in dma_chan structure
>
> Currently chan->private is used for sending slave or similar channel specific
> information. Now if we want to add struct dma_slave_config, then IMHO it
> would make sense to remove private variable and replace with dma_slave_config
> struture. That way we can reuse this struture there as well and if someone wants
> to add more stuff he can use the private_config.
>
> Dan, what do you think about this?
If you take a look at the current usages of chan->private I don't
think all of them are met by this interface.
We have:
struct at_dma_slave {
struct device *dma_dev;
dma_addr_t tx_reg;
dma_addr_t rx_reg;
enum at_dma_slave_width reg_width;
u32 cfg;
u32 ctrla;
};
struct dw_dma_slave {
struct device *dma_dev;
dma_addr_t tx_reg;
dma_addr_t rx_reg;
enum dw_dma_slave_width reg_width;
u32 cfg_hi;
u32 cfg_lo;
};
struct fsl_dma_slave {
/* List of hardware address/length pairs */
struct list_head addresses;
/* Support for extra controller features */
unsigned int request_count;
unsigned int src_loop_size;
unsigned int dst_loop_size;
bool external_start;
bool external_pause;
};
struct dma_pl330_peri {
/*
* Peri_Req i/f of the DMAC that is
* peripheral could be reached from.
*/
u8 peri_id; /* {0, 31} */
enum pl330_reqtype rqtype;
/* For M->D and D->M Channels */
int burst_sz; /* in power of 2 */
dma_addr_t fifo_addr;
};
struct sh_dmae_slave {
unsigned int slave_id; /* Set by the platform */
struct device *dma_dev; /* Set by the platform */
const struct sh_dmae_slave_config *config; /* Set by
the driver */
};
struct sh_dmae_slave_config {
unsigned int slave_id;
dma_addr_t addr;
u32 chcr;
char mid_rid;
};
struct txx9dmac_slave {
u64 tx_reg;
u64 rx_reg;
unsigned int reg_width;
};
...and I don't think this interface should try to meet all these
requirements because there will always be arch-specific quirks that
make things fall down. I think we should just start with an interface
that is minimally useful for the drivers that want to take advantage
of it. We could, since there is usually driver-specific knowledge
known by the client in the dma-slave case, just require that a
dma_slave_config be container_of() promoted to a driver specific
config. This lets the non-esoteric platform configurations use the
generic definition.
--
Dan
Hi Linus,
On Wed, Jul 21, 2010 at 12:22 PM, Linus Walleij
<[email protected]> wrote:
> This adds an interface to the DMAengine to make it possible to
> reconfigure a slave 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 runtime slave data.
>
> Signed-off-by: Linus Walleij <[email protected]>
> ---
> This version was revised after discussion, it splits the control
> arguments in source/destination pairs to prepare for (among other
> things) device to device transfers.
> ---
> ?include/linux/dmaengine.h | ? 75 +++++++++++++++++++++++++++++++++++++++++++++
> ?1 files changed, 75 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 5204f01..406b820 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_slave_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,75 @@ struct dma_chan_dev {
> ? ? ? ?atomic_t *idr_ref;
> ?};
>
> +/**
> + * enum dma_slave_buswidth - defines bus with of the DMA slave
> + * device, source or target busses
s/busses/buses/
> + */
> +enum dma_slave_buswidth {
> + ? ? ? DMA_SLAVE_BUSWIDTH_UNDEFINED = 0,
> + ? ? ? DMA_SLAVE_BUSWIDTH_1_BYTE = 1,
> + ? ? ? DMA_SLAVE_BUSWIDTH_2_BYTES = 2,
> + ? ? ? DMA_SLAVE_BUSWIDTH_4_BYTES = 4,
> + ? ? ? DMA_SLAVE_BUSWIDTH_8_BYTES = 8,
> +};
> +
> +/**
> + * struct dma_slave_config - dma slave channel runtime config
> + * @direction: whether the data shall go in or out on this slave
> + * channel, right now. DMA_TO_DEVICE and DMA_FROM_DEVICE are
> + * legal values, DMA_BIDIRECTIONAL is not acceptable since we
> + * need to differentiate source and target addresses.
> + * @src_addr: this is the physical address where DMA slave data
> + * should be read (RX), if the source is memory this argument is
> + * ignored.
> + * @dst_addr: this is the physical address where DMA slave data
> + * should be written (TX), if the source is memory this argument
> + * is ignored.
> + * @src_addr_width: this is the width in bytes of the source (RX)
> + * register where DMA data shall be read. If the source
> + * is memory this may be ignored depending on architecture.
> + * Legal values: 1, 2, 4, 8.
> + * @dst_addr_width: same as src_addr_width but for destination
> + * target (TX) mutatis mutandis.
/me wikipedias "mutatis mutandis", ah sort of latin for the s/// operator :-).
> + * @src_maxburst: the maximum number of words (note: words, as in
> + * units of the src_addr_width member, 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. This
> + * may or may not be applicable on memory sources.
> + * @dst_maxburst: same as src_maxburst but for destination target
> + * mutatus mutandis.
s/mutatus/mutatis/
> + * @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.
> + *
Will there be any users of this field initially, I'd just as soon
leave it out? Like I mentioned to Vinod I think requiring
dma_slave_config to be wrapped is a better way to extend this
structure.
--
Dan
> > + * @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.
> > + *
>
> Will there be any users of this field initially, I'd just as soon
> leave it out? Like I mentioned to Vinod I think requiring
> dma_slave_config to be wrapped is a better way to extend this
> structure.
>
I was thinking of moving some of structure fields into this, but yes wrapping
this into device specific will be a better way
-Vinod