Messages in this thread Patch in this message | | | Date | Mon, 3 Dec 2007 12:20:15 -0700 | From | "Dan Williams" <> | Subject | Re: [RFC 1/4] dmaengine: Add slave DMA interface |
| |
Hi Haavard,
Some (delayed) comments.
On Nov 23, 2007 5:20 AM, Haavard Skinnemoen <hskinnemoen@atmel.com> 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 <hskinnemoen@atmel.com> > --- > 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
| |