Hi Haavard,
Some (delayed) comments.
On Nov 23, 2007 5:20 AM, Haavard Skinnemoen <[email protected]> wrote:
> Add a new struct dma_slave_descriptor which extends the standard
> dma_async_tx_descriptor with a few members that are needed for doing
> DMA from/to peripherals with hardware handshaking (aka slave DMA.)
>
> Add new operations to struct dma_device for creating such descriptors,
> for setting up the controller to do slave DMA for a given device, and
> for terminating all pending transfers. The latter is needed because
> there may be errors outside the scope of the DMA Engine framework that
> requires DMA operations to be terminated prematurely.
>
> Signed-off-by: Haavard Skinnemoen <[email protected]>
> ---
> drivers/dma/dmaengine.c | 6 +++++
> include/linux/dmaengine.h | 55 ++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 60 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index ec7e871..3d17918 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -362,6 +362,12 @@ int dma_async_device_register(struct dma_device *device)
> !device->device_prep_dma_memset);
> BUG_ON(dma_has_cap(DMA_ZERO_SUM, device->cap_mask) &&
> !device->device_prep_dma_interrupt);
> + BUG_ON(dma_has_cap(DMA_SLAVE, device->cap_mask) &&
> + !device->device_set_slave);
> + BUG_ON(dma_has_cap(DMA_SLAVE, device->cap_mask) &&
> + !device->device_prep_slave);
> + BUG_ON(dma_has_cap(DMA_SLAVE, device->cap_mask) &&
> + !device->device_terminate_all);
>
> BUG_ON(!device->device_alloc_chan_resources);
> BUG_ON(!device->device_free_chan_resources);
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 55c9a69..e81189f 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
A few questions:
The one change that seems to be missing, at least in my mind, is
extending struct dma_client to include details about the slave device.
Whereas it is assumed that a 'dmaengine' can access kernel memory,
certain device memory regions may be out of bounds for a given
channel.
> @@ -89,10 +89,33 @@ enum dma_transaction_type {
> DMA_MEMSET,
> DMA_MEMCPY_CRC32C,
> DMA_INTERRUPT,
> + DMA_SLAVE,
> };
>
> /* last transaction type for creation of the capabilities mask */
> -#define DMA_TX_TYPE_END (DMA_INTERRUPT + 1)
> +#define DMA_TX_TYPE_END (DMA_SLAVE + 1)
> +
> +/**
> + * enum dma_slave_direction - direction of a DMA slave transfer
> + * @DMA_SLAVE_TO_MEMORY: Transfer data from peripheral to memory
> + * @DMA_SLAVE_FROM_MEMORY: Transfer data from memory to peripheral
> + */
> +enum dma_slave_direction {
> + DMA_SLAVE_TO_MEMORY,
> + DMA_SLAVE_FROM_MEMORY,
> +};
> +
> +/**
> + * enum dma_slave_width - DMA slave register access width.
> + * @DMA_SLAVE_WIDTH_8BIT: Do 8-bit slave register accesses
> + * @DMA_SLAVE_WIDTH_16BIT: Do 16-bit slave register accesses
> + * @DMA_SLAVE_WIDTH_32BIT: Do 32-bit slave register accesses
> + */
> +enum dma_slave_width {
> + DMA_SLAVE_WIDTH_8BIT,
> + DMA_SLAVE_WIDTH_16BIT,
> + DMA_SLAVE_WIDTH_32BIT,
> +};
>
> /**
> * dma_cap_mask_t - capabilities bitmap modeled after cpumask_t.
> @@ -240,6 +263,25 @@ struct dma_async_tx_descriptor {
> };
>
> /**
> + * struct dma_slave_descriptor - extended DMA descriptor for slave DMA
> + * @async_tx: async transaction descriptor
> + * @slave_set_direction: set the direction of the slave DMA
> + * transaction in the hardware descriptor
> + * @slave_set_width: set the slave register access width in the
> + * hardware descriptor
> + * @client_node: for use by the client, for example when operating on
> + * scatterlists.
> + */
> +struct dma_slave_descriptor {
> + struct dma_async_tx_descriptor txd;
> + void (*slave_set_direction)(struct dma_slave_descriptor *desc,
> + enum dma_slave_direction direction);
I have come to the conclusion that the flexibility provided by
'tx_set_src' and 'tx_set_dest' does not really buy anything and adds
unnecessary indirect branch overhead. I am developing a patch to just
add the parameters to the 'device_prep_dma_*' routines. I assume the
same can be said for 'slave_set_direction' unless you can think of a
reason why it should be separate from 'device_prep_slave'?
> + void (*slave_set_width)(struct dma_slave_descriptor *desc,
> + enum dma_slave_width width);
> + struct list_head client_node;
> +};
'slave_set_width' appears to be something that can be done once at
channel allocation time and not per each operation. It seems
channel-slave associations are exclusive since you only call
'device_set_slave' once and do not expect those values to change. So
perhaps moving this all to dmaengine common code makes sense,
something like:
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 55c9a69..71d4ac2 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -114,10 +114,18 @@ struct dma_chan_percpu {
unsigned long bytes_transferred;
};
+struct dma_slave_data {
+ struct device *slave;
+ dma_addr_t tx_reg;
+ dma_addr_t rx_reg;
+ enum dma_slave_width width;
+};
+
/**
* struct dma_chan - devices supply DMA channels, clients use them
* @device: ptr to the dma device who supplies this channel, always !%NULL
* @cookie: last cookie value returned to client
+ * @slave_data: data for preparing slave transfers
* @chan_id: channel ID for sysfs
* @class_dev: class device for sysfs
* @refcount: kref, used in "bigref" slow-mode
@@ -129,6 +137,7 @@ struct dma_chan_percpu {
struct dma_chan {
struct dma_device *device;
dma_cookie_t cookie;
+ struct dma_slave_data slave_data;
/* sysfs */
int chan_id;
@@ -193,6 +202,7 @@ typedef enum dma_state_client
(*dma_event_callback) (struct dma_client *client,
struct dma_client {
dma_event_callback event_callback;
dma_cap_mask_t cap_mask;
+ struct device *slave;
struct list_head global_node;
};
If dma_client.slave is non-NULL the client is requesting a channel
with the ability to talk to the given device. If
dma_chan.slave_data.slave is non-NULL this channel has been
exclusively claimed by the given device.
> +
> +/**
> * struct dma_device - info on the entity supplying DMA services
> * @chancnt: how many DMA channels are supported
> * @channels: the list of struct dma_chan
> @@ -258,6 +300,10 @@ struct dma_async_tx_descriptor {
> * @device_prep_dma_zero_sum: prepares a zero_sum operation
> * @device_prep_dma_memset: prepares a memset operation
> * @device_prep_dma_interrupt: prepares an end of chain interrupt operation
> + * @device_set_slave: set up a channel to do slave DMA for a given
> + * peripheral
> + * @device_prep_slave: prepares a slave dma operation
> + * @device_terminate_all: terminate all pending operations
> * @device_dependency_added: async_tx notifies the channel about new deps
> * @device_issue_pending: push pending transactions to hardware
> */
> @@ -291,6 +337,13 @@ struct dma_device {
> struct dma_async_tx_descriptor *(*device_prep_dma_interrupt)(
> struct dma_chan *chan);
>
> + void (*device_set_slave)(struct dma_chan *chan,
> + dma_addr_t rx_reg, unsigned int rx_hs_id,
> + dma_addr_t tx_reg, unsigned int tx_hs_id);
What is the significance of rx_hs_is and tx_hs_id?
> + struct dma_slave_descriptor *(*device_prep_slave)(
> + struct dma_chan *chan, size_t len, int int_en);
> + void (*device_terminate_all)(struct dma_chan *chan);
> +
> void (*device_dependency_added)(struct dma_chan *chan);
> enum dma_status (*device_is_tx_complete)(struct dma_chan *chan,
> dma_cookie_t cookie, dma_cookie_t *last,
> --
Regards,
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
