* [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() @ 2012-02-01 15:26 Guennadi Liakhovetski 2012-03-02 13:21 ` Guennadi Liakhovetski 0 siblings, 1 reply; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-02-01 15:26 UTC (permalink / raw) To: linux-kernel; +Cc: Vinod Koul When performing slame DMA some dmaengine drivers need additional data from client drivers to find out, whether they can support that specific client and to configure the DMA channel for it. This additional data has to be supplied by client drivers during channel allocation, i.e., with the __dma_request_channel() function. This patch adds a new struct dma_slave_desc with some basic data in it, further this struct can be embedded in hardware-specific types to supply any auxiliary configuration. Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> --- Vinod, this is the patch I told you about. It also requires changing all dmaengine drivers by adding one parameter to their .device_alloc_chan_resources() methods, which they would simply continue to ignore. Since this patch doesn't include that modification, it is marked as an RFC. drivers/dma/dmaengine.c | 14 ++++++++------ include/linux/dmaengine.h | 16 ++++++++++++---- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c index 139d418..0eb03b8 100644 --- a/drivers/dma/dmaengine.c +++ b/drivers/dma/dmaengine.c @@ -204,10 +204,11 @@ static void balance_ref_count(struct dma_chan *chan) /** * dma_chan_get - try to grab a dma channel's parent driver module * @chan - channel to grab + * @slave - optional DMA slave specification * * Must be called under dma_list_mutex */ -static int dma_chan_get(struct dma_chan *chan) +static int dma_chan_get(struct dma_chan *chan, struct dma_slave_desc *slave) { struct module *owner = dma_chan_to_owner(chan); @@ -220,7 +221,7 @@ static int dma_chan_get(struct dma_chan *chan) /* allocate upon first client reference */ if (chan->client_count == 1) { - int desc_cnt = chan->device->device_alloc_chan_resources(chan); + int desc_cnt = chan->device->device_alloc_chan_resources(chan, slave); if (desc_cnt < 0) { chan->client_count = 0; @@ -482,7 +483,8 @@ static struct dma_chan *private_candidate(dma_cap_mask_t *mask, struct dma_devic * @fn: optional callback to disposition available channels * @fn_param: opaque parameter to pass to dma_filter_fn */ -struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param) +struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param, + struct dma_slave_desc *slave) { struct dma_device *device, *_d; struct dma_chan *chan = NULL; @@ -500,7 +502,7 @@ struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, v */ dma_cap_set(DMA_PRIVATE, device->cap_mask); device->privatecnt++; - err = dma_chan_get(chan); + err = dma_chan_get(chan, slave); if (err == -ENODEV) { pr_debug("%s: %s module removed\n", __func__, @@ -555,7 +557,7 @@ void dmaengine_get(void) if (dma_has_cap(DMA_PRIVATE, device->cap_mask)) continue; list_for_each_entry(chan, &device->channels, device_node) { - err = dma_chan_get(chan); + err = dma_chan_get(chan, NULL); if (err == -ENODEV) { /* module removed before we could use it */ list_del_rcu(&device->global_node); @@ -762,7 +764,7 @@ int dma_async_device_register(struct dma_device *device) /* if clients are already waiting for channels we need * to take references on their behalf */ - if (dma_chan_get(chan) == -ENODEV) { + if (dma_chan_get(chan, NULL) == -ENODEV) { /* note we can only get here for the first * channel as the remaining channels are * guaranteed to get a reference diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index 679b349..8164007 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -412,6 +412,10 @@ struct dma_async_tx_descriptor { #endif }; +struct dma_slave_desc { + unsigned int id; +}; + #ifndef CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH static inline void txd_lock(struct dma_async_tx_descriptor *txd) { @@ -541,7 +545,8 @@ struct dma_device { int dev_id; struct device *dev; - int (*device_alloc_chan_resources)(struct dma_chan *chan); + int (*device_alloc_chan_resources)(struct dma_chan *chan, + struct dma_slave_desc *slave); void (*device_free_chan_resources)(struct dma_chan *chan); struct dma_async_tx_descriptor *(*device_prep_dma_memcpy)( @@ -922,7 +927,8 @@ enum dma_status dma_sync_wait(struct dma_chan *chan, dma_cookie_t cookie); #ifdef CONFIG_DMA_ENGINE enum dma_status dma_wait_for_async_tx(struct dma_async_tx_descriptor *tx); void dma_issue_pending_all(void); -struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param); +struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param, + struct dma_slave_desc *slave); void dma_release_channel(struct dma_chan *chan); #else static inline enum dma_status dma_wait_for_async_tx(struct dma_async_tx_descriptor *tx) @@ -933,7 +939,7 @@ static inline void dma_issue_pending_all(void) { } static inline struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, - dma_filter_fn fn, void *fn_param) + dma_filter_fn fn, void *fn_param, struct dma_slave_desc *slave) { return NULL; } @@ -948,7 +954,9 @@ int dma_async_device_register(struct dma_device *device); void dma_async_device_unregister(struct dma_device *device); void dma_run_dependencies(struct dma_async_tx_descriptor *tx); struct dma_chan *dma_find_channel(enum dma_transaction_type tx_type); -#define dma_request_channel(mask, x, y) __dma_request_channel(&(mask), x, y) +#define dma_request_channel(mask, x, y) __dma_request_channel(&(mask), x, y, NULL) +#define dma_request_slave_channel(mask, x, y, id) __dma_request_channel(&(mask), \ + x, y, id) /* --- Helper iov-locking functions --- */ -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-02-01 15:26 [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() Guennadi Liakhovetski @ 2012-03-02 13:21 ` Guennadi Liakhovetski 2012-03-06 8:30 ` Vinod Koul 0 siblings, 1 reply; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-02 13:21 UTC (permalink / raw) To: linux-kernel; +Cc: Vinod Koul Hi Vinod On Wed, 1 Feb 2012, Guennadi Liakhovetski wrote: > When performing slame DMA some dmaengine drivers need additional data from > client drivers to find out, whether they can support that specific client > and to configure the DMA channel for it. This additional data has to be > supplied by client drivers during channel allocation, i.e., with the > __dma_request_channel() function. This patch adds a new > struct dma_slave_desc with some basic data in it, further this struct can > be embedded in hardware-specific types to supply any auxiliary > configuration. > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> What do you think about this patch? It would be important for me to know your opinion, resp., to get it approved by you, then I could base my other shdma / "simple" dma work on top of it. It would also give me a "natural" way to eliminate the use of the .priv pointer in shdma. Thanks Guennadi > --- > > Vinod, this is the patch I told you about. It also requires changing all > dmaengine drivers by adding one parameter to their > .device_alloc_chan_resources() methods, which they would simply continue > to ignore. Since this patch doesn't include that modification, it is > marked as an RFC. > > drivers/dma/dmaengine.c | 14 ++++++++------ > include/linux/dmaengine.h | 16 ++++++++++++---- > 2 files changed, 20 insertions(+), 10 deletions(-) > > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c > index 139d418..0eb03b8 100644 > --- a/drivers/dma/dmaengine.c > +++ b/drivers/dma/dmaengine.c > @@ -204,10 +204,11 @@ static void balance_ref_count(struct dma_chan *chan) > /** > * dma_chan_get - try to grab a dma channel's parent driver module > * @chan - channel to grab > + * @slave - optional DMA slave specification > * > * Must be called under dma_list_mutex > */ > -static int dma_chan_get(struct dma_chan *chan) > +static int dma_chan_get(struct dma_chan *chan, struct dma_slave_desc *slave) > { > struct module *owner = dma_chan_to_owner(chan); > > @@ -220,7 +221,7 @@ static int dma_chan_get(struct dma_chan *chan) > > /* allocate upon first client reference */ > if (chan->client_count == 1) { > - int desc_cnt = chan->device->device_alloc_chan_resources(chan); > + int desc_cnt = chan->device->device_alloc_chan_resources(chan, slave); > > if (desc_cnt < 0) { > chan->client_count = 0; > @@ -482,7 +483,8 @@ static struct dma_chan *private_candidate(dma_cap_mask_t *mask, struct dma_devic > * @fn: optional callback to disposition available channels > * @fn_param: opaque parameter to pass to dma_filter_fn > */ > -struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param) > +struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param, > + struct dma_slave_desc *slave) > { > struct dma_device *device, *_d; > struct dma_chan *chan = NULL; > @@ -500,7 +502,7 @@ struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, v > */ > dma_cap_set(DMA_PRIVATE, device->cap_mask); > device->privatecnt++; > - err = dma_chan_get(chan); > + err = dma_chan_get(chan, slave); > > if (err == -ENODEV) { > pr_debug("%s: %s module removed\n", __func__, > @@ -555,7 +557,7 @@ void dmaengine_get(void) > if (dma_has_cap(DMA_PRIVATE, device->cap_mask)) > continue; > list_for_each_entry(chan, &device->channels, device_node) { > - err = dma_chan_get(chan); > + err = dma_chan_get(chan, NULL); > if (err == -ENODEV) { > /* module removed before we could use it */ > list_del_rcu(&device->global_node); > @@ -762,7 +764,7 @@ int dma_async_device_register(struct dma_device *device) > /* if clients are already waiting for channels we need > * to take references on their behalf > */ > - if (dma_chan_get(chan) == -ENODEV) { > + if (dma_chan_get(chan, NULL) == -ENODEV) { > /* note we can only get here for the first > * channel as the remaining channels are > * guaranteed to get a reference > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > index 679b349..8164007 100644 > --- a/include/linux/dmaengine.h > +++ b/include/linux/dmaengine.h > @@ -412,6 +412,10 @@ struct dma_async_tx_descriptor { > #endif > }; > > +struct dma_slave_desc { > + unsigned int id; > +}; > + > #ifndef CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH > static inline void txd_lock(struct dma_async_tx_descriptor *txd) > { > @@ -541,7 +545,8 @@ struct dma_device { > int dev_id; > struct device *dev; > > - int (*device_alloc_chan_resources)(struct dma_chan *chan); > + int (*device_alloc_chan_resources)(struct dma_chan *chan, > + struct dma_slave_desc *slave); > void (*device_free_chan_resources)(struct dma_chan *chan); > > struct dma_async_tx_descriptor *(*device_prep_dma_memcpy)( > @@ -922,7 +927,8 @@ enum dma_status dma_sync_wait(struct dma_chan *chan, dma_cookie_t cookie); > #ifdef CONFIG_DMA_ENGINE > enum dma_status dma_wait_for_async_tx(struct dma_async_tx_descriptor *tx); > void dma_issue_pending_all(void); > -struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param); > +struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param, > + struct dma_slave_desc *slave); > void dma_release_channel(struct dma_chan *chan); > #else > static inline enum dma_status dma_wait_for_async_tx(struct dma_async_tx_descriptor *tx) > @@ -933,7 +939,7 @@ static inline void dma_issue_pending_all(void) > { > } > static inline struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, > - dma_filter_fn fn, void *fn_param) > + dma_filter_fn fn, void *fn_param, struct dma_slave_desc *slave) > { > return NULL; > } > @@ -948,7 +954,9 @@ int dma_async_device_register(struct dma_device *device); > void dma_async_device_unregister(struct dma_device *device); > void dma_run_dependencies(struct dma_async_tx_descriptor *tx); > struct dma_chan *dma_find_channel(enum dma_transaction_type tx_type); > -#define dma_request_channel(mask, x, y) __dma_request_channel(&(mask), x, y) > +#define dma_request_channel(mask, x, y) __dma_request_channel(&(mask), x, y, NULL) > +#define dma_request_slave_channel(mask, x, y, id) __dma_request_channel(&(mask), \ > + x, y, id) > > /* --- Helper iov-locking functions --- */ > > -- > 1.7.2.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-02 13:21 ` Guennadi Liakhovetski @ 2012-03-06 8:30 ` Vinod Koul 2012-03-06 8:53 ` Guennadi Liakhovetski 0 siblings, 1 reply; 70+ messages in thread From: Vinod Koul @ 2012-03-06 8:30 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: linux-kernel On Fri, 2012-03-02 at 14:21 +0100, Guennadi Liakhovetski wrote: > Hi Vinod > > On Wed, 1 Feb 2012, Guennadi Liakhovetski wrote: sorry I thought I had replied, but looks like it got missed! > > > When performing slame DMA some dmaengine drivers need additional data from typo ^^^^^^^^^ > > client drivers to find out, whether they can support that specific client > > and to configure the DMA channel for it. This additional data has to be > > supplied by client drivers during channel allocation, i.e., with the > > __dma_request_channel() function. This patch adds a new > > struct dma_slave_desc with some basic data in it, further this struct can > > be embedded in hardware-specific types to supply any auxiliary > > configuration. counter arguing shouldn't the client drivers find out of the channel requested is capable or not, that can be alternate approach as well. That way people implement this in the filer functions and find if this is the channel we need rather than dmac finding out if it can service the client or not. Frankly I prefer former model, as that way dmacs will present channel capabilities, and clients can use as they deem fit. > > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > What do you think about this patch? It would be important for me to know > your opinion, resp., to get it approved by you, then I could base my other > shdma / "simple" dma work on top of it. It would also give me a "natural" > way to eliminate the use of the .priv pointer in shdma. > > Thanks > Guennadi > > > --- > > > > Vinod, this is the patch I told you about. It also requires changing all > > dmaengine drivers by adding one parameter to their > > .device_alloc_chan_resources() methods, which they would simply continue > > to ignore. Since this patch doesn't include that modification, it is > > marked as an RFC. > > > > drivers/dma/dmaengine.c | 14 ++++++++------ > > include/linux/dmaengine.h | 16 ++++++++++++---- > > 2 files changed, 20 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c > > index 139d418..0eb03b8 100644 > > --- a/drivers/dma/dmaengine.c > > +++ b/drivers/dma/dmaengine.c > > @@ -204,10 +204,11 @@ static void balance_ref_count(struct dma_chan *chan) > > /** > > * dma_chan_get - try to grab a dma channel's parent driver module > > * @chan - channel to grab > > + * @slave - optional DMA slave specification > > * > > * Must be called under dma_list_mutex > > */ > > -static int dma_chan_get(struct dma_chan *chan) > > +static int dma_chan_get(struct dma_chan *chan, struct dma_slave_desc *slave) > > { > > struct module *owner = dma_chan_to_owner(chan); > > > > @@ -220,7 +221,7 @@ static int dma_chan_get(struct dma_chan *chan) > > > > /* allocate upon first client reference */ > > if (chan->client_count == 1) { > > - int desc_cnt = chan->device->device_alloc_chan_resources(chan); > > + int desc_cnt = chan->device->device_alloc_chan_resources(chan, slave); > > > > if (desc_cnt < 0) { > > chan->client_count = 0; > > @@ -482,7 +483,8 @@ static struct dma_chan *private_candidate(dma_cap_mask_t *mask, struct dma_devic > > * @fn: optional callback to disposition available channels > > * @fn_param: opaque parameter to pass to dma_filter_fn > > */ > > -struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param) > > +struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param, > > + struct dma_slave_desc *slave) > > { > > struct dma_device *device, *_d; > > struct dma_chan *chan = NULL; > > @@ -500,7 +502,7 @@ struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, v > > */ > > dma_cap_set(DMA_PRIVATE, device->cap_mask); > > device->privatecnt++; > > - err = dma_chan_get(chan); > > + err = dma_chan_get(chan, slave); > > > > if (err == -ENODEV) { > > pr_debug("%s: %s module removed\n", __func__, > > @@ -555,7 +557,7 @@ void dmaengine_get(void) > > if (dma_has_cap(DMA_PRIVATE, device->cap_mask)) > > continue; > > list_for_each_entry(chan, &device->channels, device_node) { > > - err = dma_chan_get(chan); > > + err = dma_chan_get(chan, NULL); > > if (err == -ENODEV) { > > /* module removed before we could use it */ > > list_del_rcu(&device->global_node); > > @@ -762,7 +764,7 @@ int dma_async_device_register(struct dma_device *device) > > /* if clients are already waiting for channels we need > > * to take references on their behalf > > */ > > - if (dma_chan_get(chan) == -ENODEV) { > > + if (dma_chan_get(chan, NULL) == -ENODEV) { > > /* note we can only get here for the first > > * channel as the remaining channels are > > * guaranteed to get a reference > > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > > index 679b349..8164007 100644 > > --- a/include/linux/dmaengine.h > > +++ b/include/linux/dmaengine.h > > @@ -412,6 +412,10 @@ struct dma_async_tx_descriptor { > > #endif > > }; > > > > +struct dma_slave_desc { > > + unsigned int id; > > +}; > > + > > #ifndef CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH > > static inline void txd_lock(struct dma_async_tx_descriptor *txd) > > { > > @@ -541,7 +545,8 @@ struct dma_device { > > int dev_id; > > struct device *dev; > > > > - int (*device_alloc_chan_resources)(struct dma_chan *chan); > > + int (*device_alloc_chan_resources)(struct dma_chan *chan, > > + struct dma_slave_desc *slave); > > void (*device_free_chan_resources)(struct dma_chan *chan); > > > > struct dma_async_tx_descriptor *(*device_prep_dma_memcpy)( > > @@ -922,7 +927,8 @@ enum dma_status dma_sync_wait(struct dma_chan *chan, dma_cookie_t cookie); > > #ifdef CONFIG_DMA_ENGINE > > enum dma_status dma_wait_for_async_tx(struct dma_async_tx_descriptor *tx); > > void dma_issue_pending_all(void); > > -struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param); > > +struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param, > > + struct dma_slave_desc *slave); > > void dma_release_channel(struct dma_chan *chan); > > #else > > static inline enum dma_status dma_wait_for_async_tx(struct dma_async_tx_descriptor *tx) > > @@ -933,7 +939,7 @@ static inline void dma_issue_pending_all(void) > > { > > } > > static inline struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, > > - dma_filter_fn fn, void *fn_param) > > + dma_filter_fn fn, void *fn_param, struct dma_slave_desc *slave) > > { > > return NULL; > > } > > @@ -948,7 +954,9 @@ int dma_async_device_register(struct dma_device *device); > > void dma_async_device_unregister(struct dma_device *device); > > void dma_run_dependencies(struct dma_async_tx_descriptor *tx); > > struct dma_chan *dma_find_channel(enum dma_transaction_type tx_type); > > -#define dma_request_channel(mask, x, y) __dma_request_channel(&(mask), x, y) > > +#define dma_request_channel(mask, x, y) __dma_request_channel(&(mask), x, y, NULL) > > +#define dma_request_slave_channel(mask, x, y, id) __dma_request_channel(&(mask), \ > > + x, y, id) > > > > /* --- Helper iov-locking functions --- */ > > So what are we supposed to do with the slave argument. Is the expectation that dmacs will parse the parameters in dma_slave_desc and based on these return the status of .device_alloc_chan_resources? -- ~Vinod ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-06 8:30 ` Vinod Koul @ 2012-03-06 8:53 ` Guennadi Liakhovetski 2012-03-06 12:08 ` Vinod Koul 0 siblings, 1 reply; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-06 8:53 UTC (permalink / raw) To: Vinod Koul; +Cc: linux-kernel Hi Vinod Thanks for your review. On Tue, 6 Mar 2012, Vinod Koul wrote: > On Fri, 2012-03-02 at 14:21 +0100, Guennadi Liakhovetski wrote: > > Hi Vinod > > > > On Wed, 1 Feb 2012, Guennadi Liakhovetski wrote: > sorry I thought I had replied, but looks like it got missed! > > > > > When performing slame DMA some dmaengine drivers need additional data from > typo ^^^^^^^^^ > > > client drivers to find out, whether they can support that specific client > > > and to configure the DMA channel for it. This additional data has to be > > > supplied by client drivers during channel allocation, i.e., with the > > > __dma_request_channel() function. This patch adds a new > > > struct dma_slave_desc with some basic data in it, further this struct can > > > be embedded in hardware-specific types to supply any auxiliary > > > configuration. > counter arguing shouldn't the client drivers find out of the channel > requested is capable or not, that can be alternate approach as well. > That way people implement this in the filer functions and find if this > is the channel we need rather than dmac finding out if it can service > the client or not. How shall clients find this out? This is system- and DMAC-specific, this has nothing to do with the client functionality. The proposed approach is: * a client driver (MMC, USB, anything else) is capable to use DMA uses the standard dmaengine API to transfer the data * if the platform, where it's running, is supplying any auxiliary data, that it has to pass to the DMAC driver, it can do so, without getting involved in the details, just passing a pointer * the most natural location to do this is IMHO when requesting a DMA channel Now, on sh-mobile platforms you can realistically have around 5 DMAC instances with 2 or 6 channels each, of which, say, 3 controllers are suitable for MMC and 2 are not. How shall the filter function find this out? Call some ugly platform callback? Traverse some platform-specific lists? Or use a fixed channel, thus significantly reducing flexibility? Sorry, none of these options seems very attractive to me. > Frankly I prefer former model, as that way dmacs will present channel > capabilities, and clients can use as they deem fit. > > > > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> [snip] > > > @@ -948,7 +954,9 @@ int dma_async_device_register(struct dma_device *device); > > > void dma_async_device_unregister(struct dma_device *device); > > > void dma_run_dependencies(struct dma_async_tx_descriptor *tx); > > > struct dma_chan *dma_find_channel(enum dma_transaction_type tx_type); > > > -#define dma_request_channel(mask, x, y) __dma_request_channel(&(mask), x, y) > > > +#define dma_request_channel(mask, x, y) __dma_request_channel(&(mask), x, y, NULL) > > > +#define dma_request_slave_channel(mask, x, y, id) __dma_request_channel(&(mask), \ > > > + x, y, id) > > > > > > /* --- Helper iov-locking functions --- */ > > > > So what are we supposed to do with the slave argument. Is the > expectation that dmacs will parse the parameters in dma_slave_desc and > based on these return the status of .device_alloc_chan_resources? Exactly. If the channel, handed in to the DMAC driver by the dmaengine core for allocation / configuration is suitable for this slave, actions will be performed, otherwise an error will be returned. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-06 8:53 ` Guennadi Liakhovetski @ 2012-03-06 12:08 ` Vinod Koul 2012-03-06 13:03 ` Guennadi Liakhovetski 0 siblings, 1 reply; 70+ messages in thread From: Vinod Koul @ 2012-03-06 12:08 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: linux-kernel On Tue, 2012-03-06 at 09:53 +0100, Guennadi Liakhovetski wrote: > Hi Vinod > > Thanks for your review. > > On Tue, 6 Mar 2012, Vinod Koul wrote: > > > On Fri, 2012-03-02 at 14:21 +0100, Guennadi Liakhovetski wrote: > > > Hi Vinod > > > > > > On Wed, 1 Feb 2012, Guennadi Liakhovetski wrote: > > sorry I thought I had replied, but looks like it got missed! > > > > > > > When performing slame DMA some dmaengine drivers need additional data from > > typo ^^^^^^^^^ > > > > client drivers to find out, whether they can support that specific client > > > > and to configure the DMA channel for it. This additional data has to be > > > > supplied by client drivers during channel allocation, i.e., with the > > > > __dma_request_channel() function. This patch adds a new > > > > struct dma_slave_desc with some basic data in it, further this struct can > > > > be embedded in hardware-specific types to supply any auxiliary > > > > configuration. > > counter arguing shouldn't the client drivers find out of the channel > > requested is capable or not, that can be alternate approach as well. > > That way people implement this in the filer functions and find if this > > is the channel we need rather than dmac finding out if it can service > > the client or not. > > How shall clients find this out? This is system- and DMAC-specific, this > has nothing to do with the client functionality. The proposed approach is: > > * a client driver (MMC, USB, anything else) is capable to use DMA uses the > standard dmaengine API to transfer the data > > * if the platform, where it's running, is supplying any auxiliary data, > that it has to pass to the DMAC driver, it can do so, without getting > involved in the details, just passing a pointer > > * the most natural location to do this is IMHO when requesting a DMA > channel and in that case why do you need the new parameters to be passed back in filter function. What is the role of filter in this case ? > > Now, on sh-mobile platforms you can realistically have around 5 DMAC > instances with 2 or 6 channels each, of which, say, 3 controllers are > suitable for MMC and 2 are not. How shall the filter function find this > out? Call some ugly platform callback? Traverse some platform-specific > lists? Or use a fixed channel, thus significantly reducing flexibility? > Sorry, none of these options seems very attractive to me. well you can counter argue that dmac does not have this information either. Bigger question is who knows about this mapping and how do we incorporate this mapping into channel allocation > > > Frankly I prefer former model, as that way dmacs will present channel > > capabilities, and clients can use as they deem fit. > > > > > > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > [snip] -- ~Vinod ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-06 12:08 ` Vinod Koul @ 2012-03-06 13:03 ` Guennadi Liakhovetski 2012-03-07 6:28 ` Vinod Koul 0 siblings, 1 reply; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-06 13:03 UTC (permalink / raw) To: Vinod Koul; +Cc: linux-kernel On Tue, 6 Mar 2012, Vinod Koul wrote: > On Tue, 2012-03-06 at 09:53 +0100, Guennadi Liakhovetski wrote: > > Hi Vinod > > > > Thanks for your review. > > > > On Tue, 6 Mar 2012, Vinod Koul wrote: > > > > > On Fri, 2012-03-02 at 14:21 +0100, Guennadi Liakhovetski wrote: > > > > Hi Vinod > > > > > > > > On Wed, 1 Feb 2012, Guennadi Liakhovetski wrote: > > > sorry I thought I had replied, but looks like it got missed! > > > > > > > > > When performing slame DMA some dmaengine drivers need additional data from > > > typo ^^^^^^^^^ > > > > > client drivers to find out, whether they can support that specific client > > > > > and to configure the DMA channel for it. This additional data has to be > > > > > supplied by client drivers during channel allocation, i.e., with the > > > > > __dma_request_channel() function. This patch adds a new > > > > > struct dma_slave_desc with some basic data in it, further this struct can > > > > > be embedded in hardware-specific types to supply any auxiliary > > > > > configuration. > > > counter arguing shouldn't the client drivers find out of the channel > > > requested is capable or not, that can be alternate approach as well. > > > That way people implement this in the filer functions and find if this > > > is the channel we need rather than dmac finding out if it can service > > > the client or not. > > > > How shall clients find this out? This is system- and DMAC-specific, this > > has nothing to do with the client functionality. The proposed approach is: > > > > * a client driver (MMC, USB, anything else) is capable to use DMA uses the > > standard dmaengine API to transfer the data > > > > * if the platform, where it's running, is supplying any auxiliary data, > > that it has to pass to the DMAC driver, it can do so, without getting > > involved in the details, just passing a pointer > > > > * the most natural location to do this is IMHO when requesting a DMA > > channel > and in that case why do you need the new parameters to be passed back in > filter function. What is the role of filter in this case ? Sorry, who said I needed them? No changes are required to the filter function. As for its role - don't know, I didn't design it:-) In my case the filter will essentially become a "return true" dummy, respectively, it can simply be omitted. In general, I can hardly imagine a situation, where, say, an MMC driver would have internal knowledge about DMA channels on the system, allowing it to select a suitable one... So, I'm really not sure what it is for. Good, that it is at least optional. Maybe it can be deprecated with time. > > Now, on sh-mobile platforms you can realistically have around 5 DMAC > > instances with 2 or 6 channels each, of which, say, 3 controllers are > > suitable for MMC and 2 are not. How shall the filter function find this > > out? Call some ugly platform callback? Traverse some platform-specific > > lists? Or use a fixed channel, thus significantly reducing flexibility? > > Sorry, none of these options seems very attractive to me. > well you can counter argue that dmac does not have this information > either. But the DMAC is certainly a better match for making channel-selection decisions. > Bigger question is who knows about this mapping and how do we > incorporate this mapping into channel allocation The platform does. And this knowledge has to be passed to the relevant driver. But I think it's the DMAC driver, that is relevant, not the client driver. The platform would supply information like DMAC #1 channel #1 (can be used for) device #1 device #2 ... channel #2 ... ... And I don't think, it would be reasonable to let every slave driver use this information. These lists can also be optimised for specific platforms. E.g., on some sh-mobile SoCs you have two DMAC types. One of them can serve devices from list A on any channel, the other one - from list B. So, all you have to do, is to reference either A or B from your DMAC platform data. Whereas doing a reverse mapping: for each (potential) DMA user reference a list of channels, that it can use - would be really clumsy. > > > Frankly I prefer former model, as that way dmacs will present channel > > > capabilities, and clients can use as they deem fit. > > > > > > > > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-06 13:03 ` Guennadi Liakhovetski @ 2012-03-07 6:28 ` Vinod Koul 2012-03-07 9:18 ` Guennadi Liakhovetski 0 siblings, 1 reply; 70+ messages in thread From: Vinod Koul @ 2012-03-07 6:28 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: linux-kernel, 'Jassi Brar', Linus Walleij, rmk On Tue, 2012-03-06 at 14:03 +0100, Guennadi Liakhovetski wrote: <adding few folks to thread, who i may be intrested in this discussion> > But the DMAC is certainly a better match for making channel-selection > decisions. I am not sure about that as well... > > > Bigger question is who knows about this mapping and how do we > > incorporate this mapping into channel allocation > > The platform does. And this knowledge has to be passed to the relevant > driver. But I think it's the DMAC driver, that is relevant, not the client > driver. The platform would supply information like > > DMAC #1 > channel #1 > (can be used for) device #1 > device #2 > ... > channel #2 > ... > ... right :-) and we need to ensure that somehow this information is presented to dmaengine and dmaengine uses this information to filter the channel requests. In past we had good discussion [1], [2], [3] on this topic but unfortunately nothing came out of it. I like the approach outlined by Linus W [1], where we can get the information from platform (DT, FW,....) and its presented to dmaengine. I think we need to solve this _now_. There are two aspects a) to ensure dmaengine understand channel-client mapping. For this we can start with idea in [1] and see if this suits everyones needs b) how to ensure the platform gives this information in variety of arch we have (arm, x86, sh-....) Thoughts...? > And I don't think, it would be reasonable to let every slave driver use > this information. These lists can also be optimised for specific > platforms. E.g., on some sh-mobile SoCs you have two DMAC types. One of > them can serve devices from list A on any channel, the other one - from > list B. So, all you have to do, is to reference either A or B from your > DMAC platform data. Whereas doing a reverse mapping: for each (potential) > DMA user reference a list of channels, that it can use - would be really > clumsy. > [1]: http://lists.infradead.org/pipermail/linux-arm-kernel/2011-August/060717.html [2]: http://lists.infradead.org/pipermail/linux-arm-kernel/2011-July/059212.html [3]: http://lists.infradead.org/pipermail/linux-arm-kernel/2011-July/059217.html -- ~Vinod ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-07 6:28 ` Vinod Koul @ 2012-03-07 9:18 ` Guennadi Liakhovetski 2012-03-07 9:30 ` Russell King - ARM Linux 2012-03-07 9:46 ` Linus Walleij 0 siblings, 2 replies; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-07 9:18 UTC (permalink / raw) To: Vinod Koul Cc: linux-kernel, 'Jassi Brar', Linus Walleij, Russell King (changed Russell's address to the one from MAINTAINERS) On Wed, 7 Mar 2012, Vinod Koul wrote: > On Tue, 2012-03-06 at 14:03 +0100, Guennadi Liakhovetski wrote: > <adding few folks to thread, who i may be intrested in this discussion> > > But the DMAC is certainly a better match for making channel-selection > > decisions. > I am not sure about that as well... > > > > > Bigger question is who knows about this mapping and how do we > > > incorporate this mapping into channel allocation > > > > The platform does. And this knowledge has to be passed to the relevant > > driver. But I think it's the DMAC driver, that is relevant, not the client > > driver. The platform would supply information like > > > > DMAC #1 > > channel #1 > > (can be used for) device #1 > > device #2 > > ... > > channel #2 > > ... > > ... > right :-) > and we need to ensure that somehow this information is presented to > dmaengine and dmaengine uses this information to filter the channel > requests. In past we had good discussion [1], [2], [3] on this topic but > unfortunately nothing came out of it. > > I like the approach outlined by Linus W [1], where we can get the > information from platform (DT, FW,....) and its presented to dmaengine. I still don't see an answer to the very same question, that we've been discussing over multiple threads and mails now: how do we use that, if it's not a 1-to-1 mapping? I.e., many channels on many controllers can be run-time configured for use with different client devices. Also the above idea from Linus W doesn't directly address this. Following his clock / regulator analogy. There the correspondence is clearly fixed: fixed clocks and power supplies are used with every specific device. Whereas in our DMAC case it's not. I'm trying to think of an analogy, where you have several pools of resources, and each consumer can use any of the resources on some of those pools, but nothing pops up. Interrupts, GPIOs, clocks, regulators - they all are fixed to their consumers. Also notice, this can become even worse, if we ever get controllers with channels with different capabilities on them. So, I really would let the DMAC driver do the mapping and not try to put it in the core. Thanks Guennadi > I think we need to solve this _now_. There are two aspects > a) to ensure dmaengine understand channel-client mapping. For this we > can start with idea in [1] and see if this suits everyones needs > b) how to ensure the platform gives this information in variety of arch > we have (arm, x86, sh-....) > > Thoughts...? > > And I don't think, it would be reasonable to let every slave driver use > > this information. These lists can also be optimised for specific > > platforms. E.g., on some sh-mobile SoCs you have two DMAC types. One of > > them can serve devices from list A on any channel, the other one - from > > list B. So, all you have to do, is to reference either A or B from your > > DMAC platform data. Whereas doing a reverse mapping: for each (potential) > > DMA user reference a list of channels, that it can use - would be really > > clumsy. > > > [1]: > http://lists.infradead.org/pipermail/linux-arm-kernel/2011-August/060717.html > [2]: > http://lists.infradead.org/pipermail/linux-arm-kernel/2011-July/059212.html > [3]: > http://lists.infradead.org/pipermail/linux-arm-kernel/2011-July/059217.html > > > -- > ~Vinod > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-07 9:18 ` Guennadi Liakhovetski @ 2012-03-07 9:30 ` Russell King - ARM Linux 2012-03-07 9:55 ` Linus Walleij 2012-03-07 10:02 ` Guennadi Liakhovetski 2012-03-07 9:46 ` Linus Walleij 1 sibling, 2 replies; 70+ messages in thread From: Russell King - ARM Linux @ 2012-03-07 9:30 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Vinod Koul, linux-kernel, 'Jassi Brar', Linus Walleij On Wed, Mar 07, 2012 at 10:18:42AM +0100, Guennadi Liakhovetski wrote: > I still don't see an answer to the very same question, that we've been > discussing over multiple threads and mails now: how do we use that, if > it's not a 1-to-1 mapping? I.e., many channels on many controllers can be > run-time configured for use with different client devices. Also the above > idea from Linus W doesn't directly address this. > > Following his clock / regulator analogy. There the correspondence is > clearly fixed: fixed clocks and power supplies are used with every > specific device. Whereas in our DMAC case it's not. > > I'm trying to think of an analogy, where you have several pools of > resources, and each consumer can use any of the resources on some of those > pools, but nothing pops up. Interrupts, GPIOs, clocks, regulators - they > all are fixed to their consumers. > > Also notice, this can become even worse, if we ever get controllers with > channels with different capabilities on them. So, I really would let the > DMAC driver do the mapping and not try to put it in the core. Well, I am quite convinced (being on my third or fourth DMA engine driver) that DMA engine does stuff quite wrongly when it comes to slave stuff. Why? The code required to support slave device transfers is very much the same for each driver (that's partly because I'm writing my drivers in the same way.) Essentially, slave channels aren't physical channels themselves, but virtual channels which get assigned to physical channels at some point. I would really like to see some common infrastructure for handling these virtual channels so I don't have to write yet another version of that code, and that's something I'll be working on. My main critera for selecting a virtual channel is the DMA request signal into the DMA controller itself, and nothing more. Most other non-specific configuration information (data register, data register width, burst size etc) comes from the peripheral driver. Any other data relevant to the DMA engine needs to come from the platform in some way, that being DT or platform data or whatever. For example, with a DMA engine which has two bus interfaces, where it matters which bus interface is used, that needs to be specified by the platform and not by the DMA engine itself nor the peripheral driver. That would also include, if relevant, which physical channels a virtual channel (dma request) could be routed to. Note that we've been omitting that from the PL08x driver so far - memory- to-memory requests are supposed to only be handled by a couple of channels which are designed for that purpose (so they don't flood the bus) but we currently allocate them to any channel... ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-07 9:30 ` Russell King - ARM Linux @ 2012-03-07 9:55 ` Linus Walleij 2012-03-07 10:02 ` Guennadi Liakhovetski 1 sibling, 0 replies; 70+ messages in thread From: Linus Walleij @ 2012-03-07 9:55 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Guennadi Liakhovetski, Vinod Koul, linux-kernel, Jassi Brar, Narayanan G, Per Forlin On Wed, Mar 7, 2012 at 10:30 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > The code required to support slave device transfers is very much the same > for each driver (that's partly because I'm writing my drivers in the same > way.) Essentially, slave channels aren't physical channels themselves, > but virtual channels which get assigned to physical channels at some point. Actually in the COH901318 driver each device has a dedicated physical channel. It looks like some hardware engineer took the PL080 VHDL code and just copy/pasted as many instances of the channel engine as was needed to provide a unique channel for each device. I've been scratching my head over that when I've thought about merging the two drivers, because the register set and LLI is actually very similar. > I would really like to see some common infrastructure for handling these > virtual channels so I don't have to write yet another version of that code, > and that's something I'll be working on. Please go ahead with this! We have the same problem in the DMA40 driver basically, but in this thing the hardware actually takes care of the virtual-over-physical channel arbitration if you want to, which makes the "virtual" channels have physical registers and confusing things a bit. (However in the DMA40 case it may be some point in actually trying to just use the physical channels in that driver and have the dmaengine core arbitrate these under some circumstances, we already have some special flags to lock down certain physical channels.) Yours, Linus Walleij ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-07 9:30 ` Russell King - ARM Linux 2012-03-07 9:55 ` Linus Walleij @ 2012-03-07 10:02 ` Guennadi Liakhovetski 2012-03-07 10:31 ` Russell King - ARM Linux 1 sibling, 1 reply; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-07 10:02 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Vinod Koul, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Wed, 7 Mar 2012, Russell King - ARM Linux wrote: > On Wed, Mar 07, 2012 at 10:18:42AM +0100, Guennadi Liakhovetski wrote: > > I still don't see an answer to the very same question, that we've been > > discussing over multiple threads and mails now: how do we use that, if > > it's not a 1-to-1 mapping? I.e., many channels on many controllers can be > > run-time configured for use with different client devices. Also the above > > idea from Linus W doesn't directly address this. > > > > Following his clock / regulator analogy. There the correspondence is > > clearly fixed: fixed clocks and power supplies are used with every > > specific device. Whereas in our DMAC case it's not. > > > > I'm trying to think of an analogy, where you have several pools of > > resources, and each consumer can use any of the resources on some of those > > pools, but nothing pops up. Interrupts, GPIOs, clocks, regulators - they > > all are fixed to their consumers. > > > > Also notice, this can become even worse, if we ever get controllers with > > channels with different capabilities on them. So, I really would let the > > DMAC driver do the mapping and not try to put it in the core. > > Well, I am quite convinced (being on my third or fourth DMA engine driver) > that DMA engine does stuff quite wrongly when it comes to slave stuff. > Why? > > The code required to support slave device transfers is very much the same > for each driver (that's partly because I'm writing my drivers in the same > way.) Essentially, slave channels aren't physical channels themselves, > but virtual channels which get assigned to physical channels at some point. > I would really like to see some common infrastructure for handling these > virtual channels so I don't have to write yet another version of that code, > and that's something I'll be working on. Right, I thought about virtual DMA channels, but I really hoped we wouldn't have to do that;-) Would we then also have to use virtual DMAC instances? Given that the core operates in terms of DMA-controller devices and channels and the hardware is also built around those concepts, it seems a natural choice to use a 1-to-1 correspondence. In the sh-mobile case, AFAIK, until now we can have N DMA controllers of M types. Within each type the controllers are identical, meaning also, that all channels on them can be configured to work with all the same devices. Currently that's also what we present to the dmaengine core and to clients: N controllers. Whereas it has been suggested a couple of times, that neither the core nore clients should be bothered with specific DMA controller instances, so, we might as well just present M virtual controllers to the system - 1 of each type, each with a unique capability set? Thanks Guennadi > My main critera for selecting a virtual channel is the DMA request signal > into the DMA controller itself, and nothing more. Most other non-specific > configuration information (data register, data register width, burst size > etc) comes from the peripheral driver. > > Any other data relevant to the DMA engine needs to come from the platform > in some way, that being DT or platform data or whatever. For example, > with a DMA engine which has two bus interfaces, where it matters which > bus interface is used, that needs to be specified by the platform and not > by the DMA engine itself nor the peripheral driver. > > That would also include, if relevant, which physical channels a virtual > channel (dma request) could be routed to. > > Note that we've been omitting that from the PL08x driver so far - memory- > to-memory requests are supposed to only be handled by a couple of channels > which are designed for that purpose (so they don't flood the bus) but > we currently allocate them to any channel... > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-07 10:02 ` Guennadi Liakhovetski @ 2012-03-07 10:31 ` Russell King - ARM Linux 2012-03-07 12:30 ` Guennadi Liakhovetski 2012-03-07 16:20 ` Linus Walleij 0 siblings, 2 replies; 70+ messages in thread From: Russell King - ARM Linux @ 2012-03-07 10:31 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Vinod Koul, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Wed, Mar 07, 2012 at 11:02:46AM +0100, Guennadi Liakhovetski wrote: > Right, I thought about virtual DMA channels, but I really hoped we > wouldn't have to do that;-) We have to do it because on many systems, we have N request signals and M channels, where M << N. If we don't do that, then we run into problems with tying up DMA channels when they're not being used. For example, the SA11x0 IrDA driver uses two virtual channels, one for receive and one for transmit. The SA11x0 has a total of five DMA channels. To waste two of them on IrDA when it's half-duplex is just silly. Doing the whole 'request+free' thing is also silly because switching between tx and rx mode is timing-critical. Plus, it's a lot easier for driver writers to request their DMA channels at the start, and hold them until they've completed - less complicated error checking, less latency, more throughput etc. > Would we then also have to use virtual DMAC > instances? Given that the core operates in terms of DMA-controller devices > and channels and the hardware is also built around those concepts, it > seems a natural choice to use a 1-to-1 correspondence. That depends what you want from virtual DMA channels. At the moment, most setups are about virtual DMA channels within a DMA device itself. So we expose just the virtual DMA channels and hide the physical channels within the depths of the driver. > In the sh-mobile case, AFAIK, until now we can have N DMA controllers of M > types. Within each type the controllers are identical, meaning also, that > all channels on them can be configured to work with all the same devices. > Currently that's also what we present to the dmaengine core and to > clients: N controllers. Whereas it has been suggested a couple of times, > that neither the core nore clients should be bothered with specific DMA > controller instances, so, we might as well just present M virtual > controllers to the system - 1 of each type, each with a unique capability > set? I am aware of these kinds of setup, but at the moment I think there's enough of a big problem to solve with dmaengine without having it made even bigger. It's already a big enough headache trying to work out sane ways to provide library based functionality which can be shared between the DMA engine drivers... I've been chipping away at this problem since Jan 2011 and so far all I've managed to produce sanely is the consolidation of cookie handling. I have two versions of virtual channels, one based on the PL08x code and one based on my new SA11x0 code which will probably be shared with an OMAP DMA engine driver I'm working on, but I haven't looked at making all these drivers work with one set of virtual channel code yet (mainly due to crippled PL08x hardware.) ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-07 10:31 ` Russell King - ARM Linux @ 2012-03-07 12:30 ` Guennadi Liakhovetski 2012-03-07 12:45 ` Guennadi Liakhovetski 2012-03-07 12:46 ` Russell King - ARM Linux 2012-03-07 16:20 ` Linus Walleij 1 sibling, 2 replies; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-07 12:30 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Vinod Koul, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Wed, 7 Mar 2012, Russell King - ARM Linux wrote: > On Wed, Mar 07, 2012 at 11:02:46AM +0100, Guennadi Liakhovetski wrote: > > Right, I thought about virtual DMA channels, but I really hoped we > > wouldn't have to do that;-) > > We have to do it because on many systems, we have N request signals and > M channels, where M << N. If we don't do that, then we run into problems > with tying up DMA channels when they're not being used. > > For example, the SA11x0 IrDA driver uses two virtual channels, one for > receive and one for transmit. The SA11x0 has a total of five DMA > channels. Huh, that certainly justifies channel virtualisation, yes. > To waste two of them on IrDA when it's half-duplex is just > silly. Doing the whole 'request+free' thing is also silly because > switching between tx and rx mode is timing-critical. > > Plus, it's a lot easier for driver writers to request their DMA channels > at the start, and hold them until they've completed - less complicated > error checking, less latency, more throughput etc. > > > Would we then also have to use virtual DMAC > > instances? Given that the core operates in terms of DMA-controller devices > > and channels and the hardware is also built around those concepts, it > > seems a natural choice to use a 1-to-1 correspondence. > > That depends what you want from virtual DMA channels. At the moment, most > setups are about virtual DMA channels within a DMA device itself. So we > expose just the virtual DMA channels and hide the physical channels within > the depths of the driver. > > > In the sh-mobile case, AFAIK, until now we can have N DMA controllers of M > > types. Within each type the controllers are identical, meaning also, that > > all channels on them can be configured to work with all the same devices. > > Currently that's also what we present to the dmaengine core and to > > clients: N controllers. Whereas it has been suggested a couple of times, > > that neither the core nore clients should be bothered with specific DMA > > controller instances, so, we might as well just present M virtual > > controllers to the system - 1 of each type, each with a unique capability > > set? > > I am aware of these kinds of setup, but at the moment I think there's > enough of a big problem to solve with dmaengine without having it made > even bigger. It's already a big enough headache trying to work out > sane ways to provide library based functionality which can be shared > between the DMA engine drivers... > > I've been chipping away at this problem since Jan 2011 and so far all > I've managed to produce sanely is the consolidation of cookie handling. > I have two versions of virtual channels, one based on the PL08x code > and one based on my new SA11x0 code which will probably be shared with > an OMAP DMA engine driver I'm working on, but I haven't looked at making > all these drivers work with one set of virtual channel code yet (mainly > due to crippled PL08x hardware.) Ok, let me try to begin a yet another attempt to describe possible DMA channel allocation and configuration possibilites. Please, add any missing ones, then we can hopefully select the best one. 1. The current scheme is: (a) client issues dma_request_channel() with an optional filter function as parameter (b) the core picks up a suitable from its PoV DMA controller device and a channel on it and calls the filter function with that channel as an argument (c) the filter function can verify, whether that channel is suitable or not (*) (d) the client driver then can call dmaengine_slave_config() to provide any additional channel configuration information to the DMA controller driver (**) (e) if the filter has rejected this channel, the core jumps to the next DMA controller instance (***) 2. (goal: eliminate filter function look-ups) proposed by Linus W (a) client issues dma_request_slave_channel(dev, "MMC-RX") (b) the dmaengine core scans a platform-provided list of channel mappings and picks up _the_ correct channel (****) 3. Jassi's idea with capabilities has been rejected by Russell 4. (goal: simplify the allocation and configuration procedure) proposed by myself (a) as in (1) client issues dma_request_channel() with an additional slave configuration parameter (b) the core picks up a suitable from its PoV DMA controller device and a channel on it, (optionally) calls the filter (c) the core calls DMA controller driver's .device_alloc_chan_resources() method, which verifies, whether the channel can be configured for the requesting slave, if not, an error is returned and the next DMA controller instance is checked by the core Naturally, my preference goes for (4) because (a) I think, it is the DMA controller driver, that has to decide, whether the channel is suitable for a specific slave, (b) changes to the core are minimal, simple and trivially backwards-compatible, (c) the core is not cluttered with hw-specific channel mappings, (d) the additional call to dmaengine_slave_config() can be eliminated. Thanks Guennadi (*) in sh-mobile case this would translate to walking a list of DMAC instances, provided by the platform, and verifying, whether this channel is suitable for this client. E.g., on sh7372 we would have (at least, possibly more in the future) 2 such lists: type-A - for most clients, and type-B - for USB controllers, then each device, willing to use DMA would need a pointer to one of those two lists in their platform data... (**) on sh-mobile struct dma_slave_config provides no useful information for this configuration task, so, it would have to be embedded in a larger hw-specific struct, with the actual struct dma_slave_config almost completely wasted... (***) this is actually a "lucky" decision for sh-mobile, because once one channel turned out to be unsuitable for a client, all other channels on this controller will be unsuitable too. (****) two comments here: (1) I'm not sure it's good enough to let the core select a channel, I'd rather DMA controller driver do that, (2) to make it work with non 1-to-1 mappings virtual channels _and_ controller instances have to be used. --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-07 12:30 ` Guennadi Liakhovetski @ 2012-03-07 12:45 ` Guennadi Liakhovetski 2012-03-07 12:46 ` Russell King - ARM Linux 1 sibling, 0 replies; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-07 12:45 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Vinod Koul, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Wed, 7 Mar 2012, Guennadi Liakhovetski wrote: > 2. (goal: eliminate filter function look-ups) proposed by Linus W > > (a) client issues > dma_request_slave_channel(dev, "MMC-RX") > (b) the dmaengine core scans a platform-provided list of channel mappings > and picks up _the_ correct channel (****) I think, this could work well with Russell's idea to implement virtual DMA channels centrally in dmaengine core. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-07 12:30 ` Guennadi Liakhovetski 2012-03-07 12:45 ` Guennadi Liakhovetski @ 2012-03-07 12:46 ` Russell King - ARM Linux 2012-03-07 13:49 ` Guennadi Liakhovetski 2012-03-07 16:31 ` Linus Walleij 1 sibling, 2 replies; 70+ messages in thread From: Russell King - ARM Linux @ 2012-03-07 12:46 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Vinod Koul, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Wed, Mar 07, 2012 at 01:30:23PM +0100, Guennadi Liakhovetski wrote: > 1. The current scheme is: > > (a) client issues > dma_request_channel() > with an optional filter function as parameter > (b) the core picks up a suitable from its PoV DMA controller device and a > channel on it and calls the filter function with that channel as an > argument > (c) the filter function can verify, whether that channel is suitable or > not (*) > (d) the client driver then can call > dmaengine_slave_config() > to provide any additional channel configuration information to the DMA > controller driver (**) > (e) if the filter has rejected this channel, the core jumps to the next > DMA controller instance (***) No - if the filter function rejects the first free channel, the next free channel on the same controller will be tried. When all channels have been tried, the next DMA controller is checked. > 2. (goal: eliminate filter function look-ups) proposed by Linus W > > (a) client issues > dma_request_slave_channel(dev, "MMC-RX") > (b) the dmaengine core scans a platform-provided list of channel mappings > and picks up _the_ correct channel (****) That doesn't work if you have multiple DMA controllers supporting the same client. > 3. Jassi's idea with capabilities has been rejected by Russell > > 4. (goal: simplify the allocation and configuration procedure) proposed by > myself > > (a) as in (1) client issues > dma_request_channel() > with an additional slave configuration parameter > (b) the core picks up a suitable from its PoV DMA controller device and a > channel on it, (optionally) calls the filter How can it work out what's a suitable DMA controller device? Even knowing where the DMA register is, the burst size and width doesn't really narrow down the selection of the DMA controller. > (c) the core calls DMA controller driver's > .device_alloc_chan_resources() > method, which verifies, whether the channel can be configured for the > requesting slave, if not, an error is returned and the next DMA > controller instance is checked by the core And this effectively prevents a channel being reconfigured to target a different burst size or different transfer width without freeing and re-requesting it. > Naturally, my preference goes for (4) because (a) I think, it is the DMA > controller driver, that has to decide, whether the channel is suitable for > a specific slave, We already effectively do that with many of the DMA engine drivers. The DMA engine drivers export their filter function which should be used when requesting a channel (if you care about the channel you end up with.) > (b) changes to the core are minimal, simple and > trivially backwards-compatible, (c) the core is not cluttered with > hw-specific channel mappings, (d) the additional call to > dmaengine_slave_config() can be eliminated. The call to dmaengine_slave_config() actually simplifies the DMA engine support for some drivers though, so eliminating it doesn't help. What would be useful is to have a helper function along these lines: struct dma_chan *dma_request_channel_config(mask, fn, data, config) { struct dma_chan *c = dma_request_channel(mask, fn, data); if (c) { if (dmaengine_slave_config(c, config)) { dma_release_channel(c); c = NULL; } } return c; } which would simplify some of the DMA engine users. There'll still be some though which would want to call dmaengine_slave_config() to change the channels configuration when the mode of the device switches. However, I don't see anything in struct dma_slave_config which could be used to select an appropriate channel. ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-07 12:46 ` Russell King - ARM Linux @ 2012-03-07 13:49 ` Guennadi Liakhovetski 2012-03-07 14:26 ` Russell King - ARM Linux 2012-03-07 16:31 ` Linus Walleij 1 sibling, 1 reply; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-07 13:49 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Vinod Koul, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Wed, 7 Mar 2012, Russell King - ARM Linux wrote: > On Wed, Mar 07, 2012 at 01:30:23PM +0100, Guennadi Liakhovetski wrote: > > 1. The current scheme is: > > > > (a) client issues > > dma_request_channel() > > with an optional filter function as parameter > > (b) the core picks up a suitable from its PoV DMA controller device and a > > channel on it and calls the filter function with that channel as an > > argument > > (c) the filter function can verify, whether that channel is suitable or > > not (*) > > (d) the client driver then can call > > dmaengine_slave_config() > > to provide any additional channel configuration information to the DMA > > controller driver (**) > > (e) if the filter has rejected this channel, the core jumps to the next > > DMA controller instance (***) > > No - if the filter function rejects the first free channel, the next free > channel on the same controller will be tried. When all channels have > been tried, the next DMA controller is checked. Right, sorry, I confused it with an error, returned by DMA driver's .device_alloc_chan_resources() > > 2. (goal: eliminate filter function look-ups) proposed by Linus W > > > > (a) client issues > > dma_request_slave_channel(dev, "MMC-RX") > > (b) the dmaengine core scans a platform-provided list of channel mappings > > and picks up _the_ correct channel (****) > > That doesn't work if you have multiple DMA controllers supporting the > same client. Right, that's why I was against it, but it would work with virtual channels (and virtual devices)? > > 3. Jassi's idea with capabilities has been rejected by Russell > > > > 4. (goal: simplify the allocation and configuration procedure) proposed by > > myself > > > > (a) as in (1) client issues > > dma_request_channel() > > with an additional slave configuration parameter > > (b) the core picks up a suitable from its PoV DMA controller device and a > > channel on it, (optionally) calls the filter > > How can it work out what's a suitable DMA controller device? It doesn't, it will have to probe all DMA devices, until .device_alloc_chan_resources() succeeds in (c) below. > Even knowing > where the DMA register is, the burst size and width doesn't really narrow > down the selection of the DMA controller. > > > (c) the core calls DMA controller driver's > > .device_alloc_chan_resources() > > method, which verifies, whether the channel can be configured for the > > requesting slave, if not, an error is returned and the next DMA > > controller instance is checked by the core > > And this effectively prevents a channel being reconfigured to target a > different burst size or different transfer width without freeing and > re-requesting it. Cannot dmaengine_slave_config() be used for that? > > Naturally, my preference goes for (4) because (a) I think, it is the DMA > > controller driver, that has to decide, whether the channel is suitable for > > a specific slave, > > We already effectively do that with many of the DMA engine drivers. The > DMA engine drivers export their filter function which should be used when > requesting a channel (if you care about the channel you end up with.) This is one of the things I'd like to avoid - having to extend the standard API with hardware-specific methods... It's already bad enough, that client drivers often have to use DMA-controller specific types to configure transfers. Ideally I'd prefer to have 0 DMA device knowledge in client drivers. If needed, they should just pass DMA device data from platform code to the DMA controller driver as opaque handles. > > (b) changes to the core are minimal, simple and > > trivially backwards-compatible, (c) the core is not cluttered with > > hw-specific channel mappings, (d) the additional call to > > dmaengine_slave_config() can be eliminated. > > The call to dmaengine_slave_config() actually simplifies the DMA engine > support for some drivers though, so eliminating it doesn't help. Right, sorry, I didn't mean, that I'd like to get rid of it completely. I just meant, that being forced to use it for every slave channel allocation isn't very nice. > What > would be useful is to have a helper function along these lines: > > struct dma_chan *dma_request_channel_config(mask, fn, data, config) > { > struct dma_chan *c = dma_request_channel(mask, fn, data); > > if (c) { > if (dmaengine_slave_config(c, config)) { > dma_release_channel(c); > c = NULL; > } > } > return c; > } Hm, yeah... That seems like an over-complication to me: to "just" allocae a channel you cann dma_request_channel(), which scans your devices and channels on them, calls your filter, calls the DMA controller driver's allocation method, only to eventually call dmaengine_slave_config() and see it fail, after which you release the channel and start anew... Ah, there's the problem actually: you cannot try to find another channel, if dmaengine_slave_config() fails - the scan will restart from the beginning and you end up with the same failure again. So, we cannot rely on dmaengine_slave_config() to be the first instance, where the DMA controller driver actually gets the channel configuration and has a chance to verify its suitability. > which would simplify some of the DMA engine users. There'll still be > some though which would want to call dmaengine_slave_config() to change > the channels configuration when the mode of the device switches. > > However, I don't see anything in struct dma_slave_config which could be > used to select an appropriate channel. That's also my problem with it, and the reason, why I suggested, that it has to be embedded in a hardware-specific channel configuration type. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-07 13:49 ` Guennadi Liakhovetski @ 2012-03-07 14:26 ` Russell King - ARM Linux 2012-03-07 15:44 ` Guennadi Liakhovetski 0 siblings, 1 reply; 70+ messages in thread From: Russell King - ARM Linux @ 2012-03-07 14:26 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Vinod Koul, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Wed, Mar 07, 2012 at 02:49:25PM +0100, Guennadi Liakhovetski wrote: > On Wed, 7 Mar 2012, Russell King - ARM Linux wrote: > > What > > would be useful is to have a helper function along these lines: > > > > struct dma_chan *dma_request_channel_config(mask, fn, data, config) > > { > > struct dma_chan *c = dma_request_channel(mask, fn, data); > > > > if (c) { > > if (dmaengine_slave_config(c, config)) { > > dma_release_channel(c); > > c = NULL; > > } > > } > > return c; > > } > > Hm, yeah... That seems like an over-complication to me: to "just" allocae > a channel you cann dma_request_channel(), which scans your devices and > channels on them, calls your filter, calls the DMA controller driver's > allocation method, only to eventually call dmaengine_slave_config() and > see it fail, after which you release the channel and start anew... The point is _not_ that this uses dmaengine_slave_config() to find a channel at all. As I already said, there's nothing in dma_slave_config which _could_ be used to decide whether a channel is suitable or not. So to try to filter on the slave configuration is a pure red herring. None of the DMA controllers I have access to (whether they be working or not) could make any kind of decision about whether a particular channel is suitable for the peripheral by looking at the dma_slave_config structure - it's very much the case that the decision would be that 'any channel will do', which in reality it won't because there's other information required to make the decision about which virtual channel should be used. Specifically, the request signal information. Even more specifically, the request signal information may not be _just_ the request signal on the DMA controller but also incorporate an external MUX like on the Realview boards (which pl08x handles itself.) I do not see any milage in trying to select a channel based on "I want a DMA engine to access register X, width Y, burst size Z." As far as I can see, the common situation is that there's nothing in that set of information which would be useful to chose a specific channel on a DMA engine. ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-07 14:26 ` Russell King - ARM Linux @ 2012-03-07 15:44 ` Guennadi Liakhovetski 2012-03-07 16:27 ` Russell King - ARM Linux 0 siblings, 1 reply; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-07 15:44 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Vinod Koul, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Wed, 7 Mar 2012, Russell King - ARM Linux wrote: > On Wed, Mar 07, 2012 at 02:49:25PM +0100, Guennadi Liakhovetski wrote: > > On Wed, 7 Mar 2012, Russell King - ARM Linux wrote: > > > What > > > would be useful is to have a helper function along these lines: > > > > > > struct dma_chan *dma_request_channel_config(mask, fn, data, config) > > > { > > > struct dma_chan *c = dma_request_channel(mask, fn, data); > > > > > > if (c) { > > > if (dmaengine_slave_config(c, config)) { > > > dma_release_channel(c); > > > c = NULL; > > > } > > > } > > > return c; > > > } > > > > Hm, yeah... That seems like an over-complication to me: to "just" allocae > > a channel you cann dma_request_channel(), which scans your devices and > > channels on them, calls your filter, calls the DMA controller driver's > > allocation method, only to eventually call dmaengine_slave_config() and > > see it fail, after which you release the channel and start anew... > > The point is _not_ that this uses dmaengine_slave_config() to find a > channel at all. As I already said, there's nothing in dma_slave_config > which _could_ be used to decide whether a channel is suitable or not. I completely agree, that's why I wrote in my previous mail: > > > However, I don't see anything in struct dma_slave_config which could be > > > used to select an appropriate channel. > > > > That's also my problem with it, and the reason, why I suggested, that it > > has to be embedded in a hardware-specific channel configuration type. So, the question remains: which way should we go? If we don't come up with a generic solution, I'd be inclined to just do something as silly as arch/arm/mach-shmobile/board-*.c static const struct device *group1_dma_dev[] = { &dma0.device, &dma1.device, }; static const struct device *group2_dma_dev[] = { &dma2.device, &dma3.device, }; static struct my_platform_data = { .dma_dev_list = group1_dma_dev, .dma_dev_list_num = ARRAY_SIZE(group1_dma_dev), }; drivers/.../sh_*.c static bool filter(struct dma_chan *chan, void *arg) { struct device *dev = chan->dev->device.parent; struct my_platform_data *pdata = arg; for (i = 0; i < pdata->dma_dev_list_num; i++) if (pdata->dma_dev_list[i] == dev) return true; return false; } even though I find it silly to have to do this on every platform and in every driver. Thanks Guennadi > So to try to filter on the slave configuration is a pure red herring. > None of the DMA controllers I have access to (whether they be working > or not) could make any kind of decision about whether a particular > channel is suitable for the peripheral by looking at the dma_slave_config > structure - it's very much the case that the decision would be that > 'any channel will do', which in reality it won't because there's other > information required to make the decision about which virtual channel > should be used. Specifically, the request signal information. > > Even more specifically, the request signal information may not be _just_ > the request signal on the DMA controller but also incorporate an > external MUX like on the Realview boards (which pl08x handles itself.) > > I do not see any milage in trying to select a channel based on "I want > a DMA engine to access register X, width Y, burst size Z." As far as > I can see, the common situation is that there's nothing in that set of > information which would be useful to chose a specific channel on a > DMA engine. --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-07 15:44 ` Guennadi Liakhovetski @ 2012-03-07 16:27 ` Russell King - ARM Linux 2012-03-07 18:21 ` Guennadi Liakhovetski 0 siblings, 1 reply; 70+ messages in thread From: Russell King - ARM Linux @ 2012-03-07 16:27 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Vinod Koul, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Wed, Mar 07, 2012 at 04:44:12PM +0100, Guennadi Liakhovetski wrote: > So, the question remains: which way should we go? If we don't come up with > a generic solution, I'd be inclined to just do something as silly as > > arch/arm/mach-shmobile/board-*.c > > static const struct device *group1_dma_dev[] = { > &dma0.device, > &dma1.device, > }; > > static const struct device *group2_dma_dev[] = { > &dma2.device, > &dma3.device, > }; > > static struct my_platform_data = { > .dma_dev_list = group1_dma_dev, > .dma_dev_list_num = ARRAY_SIZE(group1_dma_dev), > }; > > drivers/.../sh_*.c > > static bool filter(struct dma_chan *chan, void *arg) > { > struct device *dev = chan->dev->device.parent; > struct my_platform_data *pdata = arg; > > for (i = 0; i < pdata->dma_dev_list_num; i++) > if (pdata->dma_dev_list[i] == dev) > return true; > > return false; > } > > even though I find it silly to have to do this on every platform and in > every driver. Why are you thinking that the filter function implementation has to be provided by the peripheral driver? That's just wrong and broken. Think about it - how does the peripheral driver know what kind of dma channel its filter function has been passed - to give an example, if you built into your kernel support for the PL08x DMA engine, and lets say you had PL08x DMA engine hardware, how would your filter function decide whether it was one of your per-device channels or whether it was a PL08x DMA engine channel? And in the case of a PL08x DMA engine channel (which for slaves are all virtual channels depending on the request signal desired), how would your peripheral driver know which channel would be the right one? This is why DMA engine drivers such as PL08x and SA11x0 export their own filter functions. The filter function will only match against a channel owned by that exact driver - essentially this: bool xxx_filter_id(struct dma_chan *chan, void *chan_id) { /* Reject channels for devices not bound to this driver */ if (chan->device->dev->driver != &xxx_driver) return false; ... xxx device specific channel matching ... return false; } This ensures that when the filter function needs to dereference the driver private parts of struct dma_chan, it can be sure that the channel really is being driven by this driver, and the underlying structure behind struct dma_chan is well known at that point. Anything which isn't driven by this driver is (as far as the filter function is concerned) an unknown. Once you know that the channel is owned by the right driver you can then take whatever action is necessary to check - with type safety - whether the channel is correct or not. You get type safety through two facts: 1. As described above, the filter function will reject any channel not driven by this driver. 2. You need to pass the drivers filter function and data together into the same API. Because both are together you have locality of reference. And... moreover, matching struct devices just would not fit for any DMA engine I've worked with - that being PL08x, SA11x0 and OMAP. Neither does, as I've said, using the dma_slave_config struct. Let me put it another way: 1. using struct device would be wrong for these DMA engines because a single DMA engine _device_ has multiple physical DMA channels, and not only do these physical channels share registers and interrupts, but they also share DMA requests. 2. the dma_slave_config struct can't be used to select a DMA engine or a DMA channel because it does not contain information about whether the DMA engine can access the peripheral, whether there's a link for the device request signals to the DMA engine channel. I will give you that channel matching in DMA engine stuff is far from perfect, but what I'm seeing are attempts to optimize the channel allocation for a different set of DMA devices at the expense of penalising those already there. We could pass a struct device corresponding with the desired DMA engine and DMA request number in, but that still won't fit the PL08x driver, where there's a platform specific hardware mux in front of a subset of the PL08x's request signals. So, what this is all telling me is that the _right_ interface is the one we currently have, that being the filter function and an opaque set of data to be interpreted by the supplied filter function. If you need to match by a list of struct device, then the interface allows it. If you need to match by string against a channel driven by a specific DMA engine driver, the interface allows it. If you need to do something more complicated, like one of N drivers, the interface allows it. Hell, even with the PL08x and SA11x0 filter functions, lets say for arguments sake that I had hardware with both these in, I could select a channel like this: bool pl08x_or_sa11x0_filter(struct dma_chan *chan, void *chan_id) { if (pl08x_filter_id(chan, chan_id)) return true; return sa11x0_filter_id(chan, chan_id); } where both treat 'chan_id' as a string. ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-07 16:27 ` Russell King - ARM Linux @ 2012-03-07 18:21 ` Guennadi Liakhovetski 2012-03-08 6:30 ` Vinod Koul 0 siblings, 1 reply; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-07 18:21 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Vinod Koul, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt Thanks for a detailed explanation! On Wed, 7 Mar 2012, Russell King - ARM Linux wrote: > On Wed, Mar 07, 2012 at 04:44:12PM +0100, Guennadi Liakhovetski wrote: > > So, the question remains: which way should we go? If we don't come up with > > a generic solution, I'd be inclined to just do something as silly as > > > > arch/arm/mach-shmobile/board-*.c > > > > static const struct device *group1_dma_dev[] = { > > &dma0.device, > > &dma1.device, > > }; > > > > static const struct device *group2_dma_dev[] = { > > &dma2.device, > > &dma3.device, > > }; > > > > static struct my_platform_data = { > > .dma_dev_list = group1_dma_dev, > > .dma_dev_list_num = ARRAY_SIZE(group1_dma_dev), > > }; > > > > drivers/.../sh_*.c > > > > static bool filter(struct dma_chan *chan, void *arg) > > { > > struct device *dev = chan->dev->device.parent; > > struct my_platform_data *pdata = arg; > > > > for (i = 0; i < pdata->dma_dev_list_num; i++) > > if (pdata->dma_dev_list[i] == dev) > > return true; > > > > return false; > > } > > > > even though I find it silly to have to do this on every platform and in > > every driver. > > Why are you thinking that the filter function implementation has to be > provided by the peripheral driver? That's just wrong and broken. Again: because I don't like adding private APIs to a generic one. > Think about it - how does the peripheral driver know what kind of dma > channel its filter function has been passed - to give an example, if > you built into your kernel support for the PL08x DMA engine, and lets > say you had PL08x DMA engine hardware, how would your filter function > decide whether it was one of your per-device channels or whether it > was a PL08x DMA engine channel? Sorry, there must be a confusion here: I was not proposing the above implementation for all hardware types, I don't have a good overview of all possible DMA engine scenarious and, fortunately, I don't have to implement anything that generic:-) Even though I did write above "arch/arm/mach-shmobile/board-*.c" it probably wasn't clear enough: I was only talking about the shdma DMA engine driver and its clients. And so far on all sh-mobile hardware, that I'm aware of, we haven't been mixing DMA engine types on the same hardware. This is going to change soon, as soon as we get USBHS?-DMAC support in the kernel, but even then, those controllers will not be interchangeable: only USBHS devices will be served by USBHS-DMAC controllers, the rest can be served by any other controller. So, matching on a DMA controller device would perfectly suffice. Of course, client drivers have no access to those device objects, that's why those lists have to be provided to them by the platform code. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-07 18:21 ` Guennadi Liakhovetski @ 2012-03-08 6:30 ` Vinod Koul 2012-03-08 10:16 ` Guennadi Liakhovetski 0 siblings, 1 reply; 70+ messages in thread From: Vinod Koul @ 2012-03-08 6:30 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Wed, 2012-03-07 at 19:21 +0100, Guennadi Liakhovetski wrote: > > > > Why are you thinking that the filter function implementation has to be > > provided by the peripheral driver? That's just wrong and broken. > > Again: because I don't like adding private APIs to a generic one. > > > Think about it - how does the peripheral driver know what kind of dma > > channel its filter function has been passed - to give an example, if > > you built into your kernel support for the PL08x DMA engine, and lets > > say you had PL08x DMA engine hardware, how would your filter function > > decide whether it was one of your per-device channels or whether it > > was a PL08x DMA engine channel? > > Sorry, there must be a confusion here: I was not proposing the above > implementation for all hardware types, I don't have a good overview of all > possible DMA engine scenarious and, fortunately, I don't have to implement > anything that generic:-) > > Even though I did write above "arch/arm/mach-shmobile/board-*.c" it > probably wasn't clear enough: I was only talking about the shdma DMA > engine driver and its clients. And so far on all sh-mobile hardware, that > I'm aware of, we haven't been mixing DMA engine types on the same > hardware. This is going to change soon, as soon as we get USBHS?-DMAC > support in the kernel, but even then, those controllers will not be > interchangeable: only USBHS devices will be served by USBHS-DMAC > controllers, the rest can be served by any other controller. So, matching > on a DMA controller device would perfectly suffice. > > Of course, client drivers have no access to those device objects, that's > why those lists have to be provided to them by the platform code. We are trying to solve this problem by making it a client or dmac problem rather than a platform problem. We *miss* the point here in discussion that platform *knows* the channel mapping and *not* dmac or client, so any solution not based on this would not work, so let the platform provide this to dmaengine. We can have the map as* [*with due credit to Linus Walleij, whose idea I have extended a small bit to have multiple channel and 1 to many mapping] struct dmaengine_map { char *ch_name; char *client_name; char *dmac_name; unsigned int ch; }; struct dmaengine_map[] = { { .name = "MMC-RX", .client_name = "mmc.0", .dmac_name = "pl08x.0", .ch = 0; }, /* mmc.0 device can use pl08x.0 controller ch 0 */ { .name = "MMC-TX", .client_name = "mmc.0", .dmac_name = "pl08x.0", .ch = 1; }, /* mmc.0 device can use pl08x.0 controller ch 1 */ { .name = "SSP-TX", .client_name = "pl022.0", .dmac_name = "pl022.0", .ch = 1; }, /* SSP-TX device can use pl022.0 controller ch 1 */ { .name = "SSP-RX", .client_name = "pl022.0", .dmac_name = "pl022.0", .ch = 2; }, /* SSP-TX device can use pl022.0 controller ch 2 */ { .name = "MMC-TX", .client_name = "pl022.0", .dmac_name = "pl022.0", .ch = 2; }, /* BTW I ahve ultra spl hardware where * SSP-TX device can also use pl022.0 controller ch 2 */ ... }; This also takes care care of many to 1 mapping where a channel can talk to multiple clients and dmaengine choose first in list. If we do virtual channels (which I would advise) then we can have 1-1 mapping, even otherwise dmaengine can pick first channel, and client has right to refuse (filter fn ofcourse!) So we can add int dmaengine_add_channel_map(struct dmaengine_map *map, unsigned int num_entries) { /* store this map into dmaengine and use for channle allocation */ } This map can be given by device tree, board files, etc based on each what the respective arch deems the best way. And based on yesterday discussion, I like Russell's idea of hiding dma_slave_config, so: struct dma_chan *dma_request_channel_config(mask, fn, data, config) { struct dma_chan *c = dma_request_channel(mask, fn, data); if (c) { if (dmaengine_slave_config(c, config)) { dma_release_channel(c); c = NULL; } } return c; } where struct dma_chan *dma_request_channel(mask, fn, data) { for_each_match_in_map(c, map) { if (fn && ! fn(c, data)) continue; return chan; } return NULL; } At this point the client has the channel it needs to use .prepare_xxx API without the need of anything else... Does this model fit all? -- ~Vinod ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-08 6:30 ` Vinod Koul @ 2012-03-08 10:16 ` Guennadi Liakhovetski 2012-03-08 10:55 ` Vinod Koul 2012-03-08 11:46 ` Linus Walleij 0 siblings, 2 replies; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-08 10:16 UTC (permalink / raw) To: Vinod Koul Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Thu, 8 Mar 2012, Vinod Koul wrote: > On Wed, 2012-03-07 at 19:21 +0100, Guennadi Liakhovetski wrote: > > > > > > Why are you thinking that the filter function implementation has to be > > > provided by the peripheral driver? That's just wrong and broken. > > > > Again: because I don't like adding private APIs to a generic one. > > > > > Think about it - how does the peripheral driver know what kind of dma > > > channel its filter function has been passed - to give an example, if > > > you built into your kernel support for the PL08x DMA engine, and lets > > > say you had PL08x DMA engine hardware, how would your filter function > > > decide whether it was one of your per-device channels or whether it > > > was a PL08x DMA engine channel? > > > > Sorry, there must be a confusion here: I was not proposing the above > > implementation for all hardware types, I don't have a good overview of all > > possible DMA engine scenarious and, fortunately, I don't have to implement > > anything that generic:-) > > > > Even though I did write above "arch/arm/mach-shmobile/board-*.c" it > > probably wasn't clear enough: I was only talking about the shdma DMA > > engine driver and its clients. And so far on all sh-mobile hardware, that > > I'm aware of, we haven't been mixing DMA engine types on the same > > hardware. This is going to change soon, as soon as we get USBHS?-DMAC > > support in the kernel, but even then, those controllers will not be > > interchangeable: only USBHS devices will be served by USBHS-DMAC > > controllers, the rest can be served by any other controller. So, matching > > on a DMA controller device would perfectly suffice. > > > > Of course, client drivers have no access to those device objects, that's > > why those lists have to be provided to them by the platform code. > We are trying to solve this problem by making it a client or dmac > problem rather than a platform problem. We *miss* the point here in > discussion that platform *knows* the channel mapping and *not* dmac or > client, so any solution not based on this would not work, so let the > platform provide this to dmaengine. I still have the impression, that my specific use-case (sh-mobile), where channels can be freely configured for use by _ANY_ client on one of _SEVERAL_ DMAC instances, is not fully understood or taken into account. For this driver any kind of fixed mapping means, that we'd have to use both virtual channels and controllers, adding _a lot_ of complexity to the DMAC driver and making the dmaengine core just an "obfuscation layer." Yes, I remember Russell proposing core helpers for this. They would help, but (1) when would they be available, (2) how well would they be suitable for us, (3) they'd take the coding / maintainance burden away, but wouldn't reduce complexity and run-time overhead. Whereas on the other hand our case can be handled _very_ easily: 1. a client requests a channel of a specific type 2. one of channels of that type, residing on one of compatible controllers, is allocated, configured and handed in to the client That's it. No filtering, no post-allocation configuration - at least so far. And penalising such a simple case by forcing it to use virtual channels and filter through some unnatural mappings doesn't seem very productive to me. Thanks Guennadi > We can have the map as* > [*with due credit to Linus Walleij, whose idea I have extended a small > bit to have multiple channel and 1 to many mapping] > > struct dmaengine_map { > char *ch_name; > char *client_name; > char *dmac_name; > unsigned int ch; > }; > > struct dmaengine_map[] = { > { > .name = "MMC-RX", > .client_name = "mmc.0", > .dmac_name = "pl08x.0", > .ch = 0; > }, > /* mmc.0 device can use pl08x.0 controller ch 0 */ > { > .name = "MMC-TX", > .client_name = "mmc.0", > .dmac_name = "pl08x.0", > .ch = 1; > }, > /* mmc.0 device can use pl08x.0 controller ch 1 */ > { > .name = "SSP-TX", > .client_name = "pl022.0", > .dmac_name = "pl022.0", > .ch = 1; > }, > /* SSP-TX device can use pl022.0 controller ch 1 */ > { > .name = "SSP-RX", > .client_name = "pl022.0", > .dmac_name = "pl022.0", > .ch = 2; > }, > /* SSP-TX device can use pl022.0 controller ch 2 */ > { > .name = "MMC-TX", > .client_name = "pl022.0", > .dmac_name = "pl022.0", > .ch = 2; > }, > /* BTW I ahve ultra spl hardware where > * SSP-TX device can also use pl022.0 controller ch 2 */ > ... > }; > This also takes care care of many to 1 mapping where a channel can talk > to multiple clients and dmaengine choose first in list. > > If we do virtual channels (which I would advise) then we can have 1-1 > mapping, even otherwise dmaengine can pick first channel, and client has > right to refuse (filter fn ofcourse!) > > So we can add > int dmaengine_add_channel_map(struct dmaengine_map *map, unsigned int num_entries) > { > /* store this map into dmaengine and use for channle allocation */ > } > This map can be given by device tree, board files, etc based on each > what the respective arch deems the best way. > > And based on yesterday discussion, I like Russell's idea of hiding > dma_slave_config, so: > > struct dma_chan *dma_request_channel_config(mask, fn, data, config) > { > struct dma_chan *c = dma_request_channel(mask, fn, data); > > if (c) { > if (dmaengine_slave_config(c, config)) { > dma_release_channel(c); > c = NULL; > } > } > return c; > } > > where > struct dma_chan *dma_request_channel(mask, fn, data) > { > for_each_match_in_map(c, map) { > if (fn && ! fn(c, data)) > continue; > return chan; > } > return NULL; > } > > At this point the client has the channel it needs to use .prepare_xxx > API without the need of anything else... > > Does this model fit all? > > -- > ~Vinod > > > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-08 10:16 ` Guennadi Liakhovetski @ 2012-03-08 10:55 ` Vinod Koul 2012-03-08 11:22 ` Guennadi Liakhovetski 2012-03-08 11:46 ` Linus Walleij 1 sibling, 1 reply; 70+ messages in thread From: Vinod Koul @ 2012-03-08 10:55 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Thu, 2012-03-08 at 11:16 +0100, Guennadi Liakhovetski wrote: > I still have the impression, that my specific use-case (sh-mobile), where > channels can be freely configured for use by _ANY_ client on one of > _SEVERAL_ DMAC instances, is not fully understood or taken into account. > For this driver any kind of fixed mapping means, that we'd have to use > both virtual channels and controllers, adding _a lot_ of complexity to the > DMAC driver and making the dmaengine core just an "obfuscation layer." > Yes, I remember Russell proposing core helpers for this. They would help, > but (1) when would they be available, (2) how well would they be suitable > for us, (3) they'd take the coding / maintainance burden away, but > wouldn't reduce complexity and run-time overhead. Lets try to address you case as well. On a typical platform 1) how many dma controllers you have? 2) how many clients you have 3) which client can use what controller channel? How is mapping decided, do you have a mux, is it hard wired by soc designers,....? Can you pls give a description so that we ensure all models fit in the final solution? -- ~Vinod ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-08 10:55 ` Vinod Koul @ 2012-03-08 11:22 ` Guennadi Liakhovetski 2012-03-08 11:34 ` Vinod Koul 0 siblings, 1 reply; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-08 11:22 UTC (permalink / raw) To: Vinod Koul Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Thu, 8 Mar 2012, Vinod Koul wrote: > On Thu, 2012-03-08 at 11:16 +0100, Guennadi Liakhovetski wrote: > > I still have the impression, that my specific use-case (sh-mobile), where > > channels can be freely configured for use by _ANY_ client on one of > > _SEVERAL_ DMAC instances, is not fully understood or taken into account. > > For this driver any kind of fixed mapping means, that we'd have to use > > both virtual channels and controllers, adding _a lot_ of complexity to the > > DMAC driver and making the dmaengine core just an "obfuscation layer." > > Yes, I remember Russell proposing core helpers for this. They would help, > > but (1) when would they be available, (2) how well would they be suitable > > for us, (3) they'd take the coding / maintainance burden away, but > > wouldn't reduce complexity and run-time overhead. > Lets try to address you case as well. > On a typical platform Let's take the mackerel board with the sh7372 SoC. it's not the state of the art, but that's what I'm currently working with and it should give us a good enough idea > 1) how many dma controllers you have? currently supported 5 of 3 types (3 of type A, 1 of each of the types B and C), all handled by the same driver > 2) how many clients you have huh... many. Maybe like 20 or more, and more, that are not yet supported, using type A, and 1 for each of types B and C > 3) which client can use what controller channel? How is mapping decided, > do you have a mux, is it hard wired by soc designers,....? In general - with all the current sh-mobile hardware, that I'm aware of - there can be several controller instances on an SoC of each controller type. Inside each type all instances and all channels are freely configurable. So, of 20 Type A clients they can use any channels on any one of the 3 type A controllers. Types B and C are "degenerate" cases, there clients are practically hard-wired to a specific DMA controller. So, we don't have to decide on mappings for type A. We just pick up any free channels on any controller and configure them accordingly. Whether there's a mux somewhere - you can say so, but it's all inside the SoC, and it's configured automatically ones you configure a physical channel to serve a specific client. > Can you pls give a description so that we ensure all models fit in the > final solution? That's what I've been trying to do since several days now... I've been saying "multiple controllers with multiple channels all freely configurable for any device from a list" again and again... Seems I'm speaking some strange language, that noone understands. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-08 11:22 ` Guennadi Liakhovetski @ 2012-03-08 11:34 ` Vinod Koul 2012-03-08 12:58 ` Vinod Koul 0 siblings, 1 reply; 70+ messages in thread From: Vinod Koul @ 2012-03-08 11:34 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: vinod.koul, Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Thu, 2012-03-08 at 12:22 +0100, Guennadi Liakhovetski wrote: > On Thu, 8 Mar 2012, Vinod Koul wrote: > > > On Thu, 2012-03-08 at 11:16 +0100, Guennadi Liakhovetski wrote: > > > I still have the impression, that my specific use-case (sh-mobile), where > > > channels can be freely configured for use by _ANY_ client on one of > > > _SEVERAL_ DMAC instances, is not fully understood or taken into account. > > > For this driver any kind of fixed mapping means, that we'd have to use > > > both virtual channels and controllers, adding _a lot_ of complexity to the > > > DMAC driver and making the dmaengine core just an "obfuscation layer." > > > Yes, I remember Russell proposing core helpers for this. They would help, > > > but (1) when would they be available, (2) how well would they be suitable > > > for us, (3) they'd take the coding / maintainance burden away, but > > > wouldn't reduce complexity and run-time overhead. > > Lets try to address you case as well. > > On a typical platform > > Let's take the mackerel board with the sh7372 SoC. it's not the state of > the art, but that's what I'm currently working with and it should give us > a good enough idea > > > 1) how many dma controllers you have? > > currently supported 5 of 3 types (3 of type A, 1 of each of the types B > and C), all handled by the same driver > > > 2) how many clients you have > > huh... many. Maybe like 20 or more, and more, that are not yet supported, > using type A, and 1 for each of types B and C > > > 3) which client can use what controller channel? How is mapping decided, > > do you have a mux, is it hard wired by soc designers,....? > > In general - with all the current sh-mobile hardware, that I'm aware of - > there can be several controller instances on an SoC of each controller > type. Inside each type all instances and all channels are freely > configurable. So, of 20 Type A clients they can use any channels on any > one of the 3 type A controllers. Types B and C are "degenerate" cases, > there clients are practically hard-wired to a specific DMA controller. > > So, we don't have to decide on mappings for type A. We just pick up any > free channels on any controller and configure them accordingly. Whether > there's a mux somewhere - you can say so, but it's all inside the SoC, and > it's configured automatically ones you configure a physical channel to > serve a specific client. > > > Can you pls give a description so that we ensure all models fit in the > > final solution? > > That's what I've been trying to do since several days now... I've been > saying "multiple controllers with multiple channels all freely > configurable for any device from a list" again and again... Seems I'm > speaking some strange language, that noone understands. Okay. One more question before I can tell you how it can work for you without you sweating it out :-) So you have: case A: Here you have N dmacs and M controllers, any controller can use any channel, No constraints on channel assignments, right? case B: Some hardwired controllers P which can only be used by a set clients Q? Anything else I missed in your description? ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-08 11:34 ` Vinod Koul @ 2012-03-08 12:58 ` Vinod Koul 2012-03-08 13:18 ` Guennadi Liakhovetski 0 siblings, 1 reply; 70+ messages in thread From: Vinod Koul @ 2012-03-08 12:58 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Thu, 2012-03-08 at 17:04 +0530, Vinod Koul wrote: > On Thu, 2012-03-08 at 12:22 +0100, Guennadi Liakhovetski wrote: > > On Thu, 8 Mar 2012, Vinod Koul wrote: > > > > > On Thu, 2012-03-08 at 11:16 +0100, Guennadi Liakhovetski wrote: > > > > I still have the impression, that my specific use-case (sh-mobile), where > > > > channels can be freely configured for use by _ANY_ client on one of > > > > _SEVERAL_ DMAC instances, is not fully understood or taken into account. > > > > For this driver any kind of fixed mapping means, that we'd have to use > > > > both virtual channels and controllers, adding _a lot_ of complexity to the > > > > DMAC driver and making the dmaengine core just an "obfuscation layer." > > > > Yes, I remember Russell proposing core helpers for this. They would help, > > > > but (1) when would they be available, (2) how well would they be suitable > > > > for us, (3) they'd take the coding / maintainance burden away, but > > > > wouldn't reduce complexity and run-time overhead. > > > Lets try to address you case as well. > > > On a typical platform > > > > Let's take the mackerel board with the sh7372 SoC. it's not the state of > > the art, but that's what I'm currently working with and it should give us > > a good enough idea > > > > > 1) how many dma controllers you have? > > > > currently supported 5 of 3 types (3 of type A, 1 of each of the types B > > and C), all handled by the same driver > > > > > 2) how many clients you have > > > > huh... many. Maybe like 20 or more, and more, that are not yet supported, > > using type A, and 1 for each of types B and C > > > > > 3) which client can use what controller channel? How is mapping decided, > > > do you have a mux, is it hard wired by soc designers,....? > > > > In general - with all the current sh-mobile hardware, that I'm aware of - > > there can be several controller instances on an SoC of each controller > > type. Inside each type all instances and all channels are freely > > configurable. So, of 20 Type A clients they can use any channels on any > > one of the 3 type A controllers. Types B and C are "degenerate" cases, > > there clients are practically hard-wired to a specific DMA controller. > > > > So, we don't have to decide on mappings for type A. We just pick up any > > free channels on any controller and configure them accordingly. Whether > > there's a mux somewhere - you can say so, but it's all inside the SoC, and > > it's configured automatically ones you configure a physical channel to > > serve a specific client. > > > > > Can you pls give a description so that we ensure all models fit in the > > > final solution? > > > > That's what I've been trying to do since several days now... I've been > > saying "multiple controllers with multiple channels all freely > > configurable for any device from a list" again and again... Seems I'm > > speaking some strange language, that noone understands. > Okay. One more question before I can tell you how it can work for you > without you sweating it out :-) > > So you have: > case A: Here you have N dmacs and M controllers, any controller can use > any channel, No constraints on channel assignments, right? > case B: Some hardwired controllers P which can only be used by a set > clients Q? > > Anything else I missed in your description? Assuming I didn't miss... The case B can be handled without sweat by platforms channel mapping information. Case A where we don't find that devices exist in map, thus being treated as generic DMA channels and can be handled easily in sequence. So when someone in Q request a channel it would get first channel in Ps This way we handle both of them in a transparent manner to both clients and controllers. Perhaps we can also add capability to know that if channel is to be searched in map or not - would be anyway required for non slave cases. -- ~Vinod ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-08 12:58 ` Vinod Koul @ 2012-03-08 13:18 ` Guennadi Liakhovetski 2012-03-09 9:21 ` Vinod Koul 0 siblings, 1 reply; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-08 13:18 UTC (permalink / raw) To: Vinod Koul Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Thu, 8 Mar 2012, Vinod Koul wrote: > On Thu, 2012-03-08 at 17:04 +0530, Vinod Koul wrote: > > On Thu, 2012-03-08 at 12:22 +0100, Guennadi Liakhovetski wrote: > > > On Thu, 8 Mar 2012, Vinod Koul wrote: > > > > > > > On Thu, 2012-03-08 at 11:16 +0100, Guennadi Liakhovetski wrote: > > > > > I still have the impression, that my specific use-case (sh-mobile), where > > > > > channels can be freely configured for use by _ANY_ client on one of > > > > > _SEVERAL_ DMAC instances, is not fully understood or taken into account. > > > > > For this driver any kind of fixed mapping means, that we'd have to use > > > > > both virtual channels and controllers, adding _a lot_ of complexity to the > > > > > DMAC driver and making the dmaengine core just an "obfuscation layer." > > > > > Yes, I remember Russell proposing core helpers for this. They would help, > > > > > but (1) when would they be available, (2) how well would they be suitable > > > > > for us, (3) they'd take the coding / maintainance burden away, but > > > > > wouldn't reduce complexity and run-time overhead. > > > > Lets try to address you case as well. > > > > On a typical platform > > > > > > Let's take the mackerel board with the sh7372 SoC. it's not the state of > > > the art, but that's what I'm currently working with and it should give us > > > a good enough idea > > > > > > > 1) how many dma controllers you have? > > > > > > currently supported 5 of 3 types (3 of type A, 1 of each of the types B > > > and C), all handled by the same driver > > > > > > > 2) how many clients you have > > > > > > huh... many. Maybe like 20 or more, and more, that are not yet supported, > > > using type A, and 1 for each of types B and C > > > > > > > 3) which client can use what controller channel? How is mapping decided, > > > > do you have a mux, is it hard wired by soc designers,....? > > > > > > In general - with all the current sh-mobile hardware, that I'm aware of - > > > there can be several controller instances on an SoC of each controller > > > type. Inside each type all instances and all channels are freely > > > configurable. So, of 20 Type A clients they can use any channels on any > > > one of the 3 type A controllers. Types B and C are "degenerate" cases, > > > there clients are practically hard-wired to a specific DMA controller. > > > > > > So, we don't have to decide on mappings for type A. We just pick up any > > > free channels on any controller and configure them accordingly. Whether > > > there's a mux somewhere - you can say so, but it's all inside the SoC, and > > > it's configured automatically ones you configure a physical channel to > > > serve a specific client. > > > > > > > Can you pls give a description so that we ensure all models fit in the > > > > final solution? > > > > > > That's what I've been trying to do since several days now... I've been > > > saying "multiple controllers with multiple channels all freely > > > configurable for any device from a list" again and again... Seems I'm > > > speaking some strange language, that noone understands. > > Okay. One more question before I can tell you how it can work for you > > without you sweating it out :-) > > > > So you have: > > case A: Here you have N dmacs and M controllers, any controller can use > > any channel, No constraints on channel assignments, right? > > case B: Some hardwired controllers P which can only be used by a set > > clients Q? > > > > Anything else I missed in your description? > Assuming I didn't miss... > > The case B can be handled without sweat by platforms channel mapping > information. > > Case A where we don't find that devices exist in map, thus being treated > as generic DMA channels and can be handled easily in sequence. So when > someone in Q request a channel it would get first channel in Ps > > This way we handle both of them in a transparent manner to both clients > and controllers. > > Perhaps we can also add capability to know that if channel is to be > searched in map or not - would be anyway required for non slave cases. Right, but I don't understand then what this gives us. You propose some channel maps, that will not be used for your "case A." Which means, for "case A" nothing changes. So, the reason for this whole thread hasn't been addressed: how to pass channel configuration to the DMA controller driver. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-08 13:18 ` Guennadi Liakhovetski @ 2012-03-09 9:21 ` Vinod Koul 2012-03-09 9:24 ` Guennadi Liakhovetski 0 siblings, 1 reply; 70+ messages in thread From: Vinod Koul @ 2012-03-09 9:21 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Thu, 2012-03-08 at 14:18 +0100, Guennadi Liakhovetski wrote: > > Assuming I didn't miss... > > > > The case B can be handled without sweat by platforms channel mapping > > information. > > > > Case A where we don't find that devices exist in map, thus being treated > > as generic DMA channels and can be handled easily in sequence. So when > > someone in Q request a channel it would get first channel in Ps > > > > This way we handle both of them in a transparent manner to both clients > > and controllers. > > > > Perhaps we can also add capability to know that if channel is to be > > searched in map or not - would be anyway required for non slave cases. > > Right, but I don't understand then what this gives us. You propose some > channel maps, that will not be used for your "case A." Which means, for > "case A" nothing changes. So, the reason for this whole thread hasn't been > addressed: how to pass channel configuration to the DMA controller driver. For "Case A" there should be no filtering or any issues even now. You have controller requesting a channel and as long as they get a channel for respective pool, it should work. Or is there anything else which is required in this case? -- ~Vinod ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-09 9:21 ` Vinod Koul @ 2012-03-09 9:24 ` Guennadi Liakhovetski 2012-03-09 9:39 ` Vinod Koul 0 siblings, 1 reply; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-09 9:24 UTC (permalink / raw) To: Vinod Koul Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Fri, 9 Mar 2012, Vinod Koul wrote: > On Thu, 2012-03-08 at 14:18 +0100, Guennadi Liakhovetski wrote: > > > Assuming I didn't miss... > > > > > > The case B can be handled without sweat by platforms channel mapping > > > information. > > > > > > Case A where we don't find that devices exist in map, thus being treated > > > as generic DMA channels and can be handled easily in sequence. So when > > > someone in Q request a channel it would get first channel in Ps > > > > > > This way we handle both of them in a transparent manner to both clients > > > and controllers. > > > > > > Perhaps we can also add capability to know that if channel is to be > > > searched in map or not - would be anyway required for non slave cases. > > > > Right, but I don't understand then what this gives us. You propose some > > channel maps, that will not be used for your "case A." Which means, for > > "case A" nothing changes. So, the reason for this whole thread hasn't been > > addressed: how to pass channel configuration to the DMA controller driver. > For "Case A" there should be no filtering or any issues even now. You > have controller requesting a channel and as long as they get a channel > for respective pool, it should work. > > Or is there anything else which is required in this case? Yes, there is. You keep complaining about my use of .priv pointer, which I use in the filter to pass channel configuration to the DMA controller driver. That is the original reason for this whole discussion. If you're fine with that, I'll just carry that .priv use over in the "simple" DMA engine driver and stay happy forever:-) Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-09 9:24 ` Guennadi Liakhovetski @ 2012-03-09 9:39 ` Vinod Koul 2012-03-09 12:20 ` Guennadi Liakhovetski 0 siblings, 1 reply; 70+ messages in thread From: Vinod Koul @ 2012-03-09 9:39 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Fri, 2012-03-09 at 10:24 +0100, Guennadi Liakhovetski wrote: > On Fri, 9 Mar 2012, Vinod Koul wrote: > > > On Thu, 2012-03-08 at 14:18 +0100, Guennadi Liakhovetski wrote: > > > > Assuming I didn't miss... > > > > > > > > The case B can be handled without sweat by platforms channel mapping > > > > information. > > > > > > > > Case A where we don't find that devices exist in map, thus being treated > > > > as generic DMA channels and can be handled easily in sequence. So when > > > > someone in Q request a channel it would get first channel in Ps > > > > > > > > This way we handle both of them in a transparent manner to both clients > > > > and controllers. > > > > > > > > Perhaps we can also add capability to know that if channel is to be > > > > searched in map or not - would be anyway required for non slave cases. > > > > > > Right, but I don't understand then what this gives us. You propose some > > > channel maps, that will not be used for your "case A." Which means, for > > > "case A" nothing changes. So, the reason for this whole thread hasn't been > > > addressed: how to pass channel configuration to the DMA controller driver. > > For "Case A" there should be no filtering or any issues even now. You > > have controller requesting a channel and as long as they get a channel > > for respective pool, it should work. > > > > Or is there anything else which is required in this case? > > Yes, there is. You keep complaining about my use of .priv pointer, which I > use in the filter to pass channel configuration to the DMA controller > driver. That is the original reason for this whole discussion. If you're > fine with that, I'll just carry that .priv use over in the "simple" DMA > engine driver and stay happy forever:-) Channel configuration != filtering & channel allocation The discussion is focused on getting request_channel fixed so that we take care of oddities in respective platforms for channel maps which exist. What the heck does it have to on how you configure you channel?? Once you have channel allocated correctly which can work for you, you need to configure you channel. Since this is slave case you to tell what your client FIFO depth, burst size, width etc parameters. There is dma_slave_config for that purpose ONLY. Is there anything else required for _configuring_ if yes, we can add those. But then again that doesn't have anything to do with how you get a channel. -- ~Vinod ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-09 9:39 ` Vinod Koul @ 2012-03-09 12:20 ` Guennadi Liakhovetski 2012-03-09 14:07 ` Russell King - ARM Linux 2012-03-12 2:47 ` Vinod Koul 0 siblings, 2 replies; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-09 12:20 UTC (permalink / raw) To: Vinod Koul Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Fri, 9 Mar 2012, Vinod Koul wrote: > On Fri, 2012-03-09 at 10:24 +0100, Guennadi Liakhovetski wrote: > > On Fri, 9 Mar 2012, Vinod Koul wrote: > > > > > On Thu, 2012-03-08 at 14:18 +0100, Guennadi Liakhovetski wrote: > > > > > Assuming I didn't miss... > > > > > > > > > > The case B can be handled without sweat by platforms channel mapping > > > > > information. > > > > > > > > > > Case A where we don't find that devices exist in map, thus being treated > > > > > as generic DMA channels and can be handled easily in sequence. So when > > > > > someone in Q request a channel it would get first channel in Ps > > > > > > > > > > This way we handle both of them in a transparent manner to both clients > > > > > and controllers. > > > > > > > > > > Perhaps we can also add capability to know that if channel is to be > > > > > searched in map or not - would be anyway required for non slave cases. > > > > > > > > Right, but I don't understand then what this gives us. You propose some > > > > channel maps, that will not be used for your "case A." Which means, for > > > > "case A" nothing changes. So, the reason for this whole thread hasn't been > > > > addressed: how to pass channel configuration to the DMA controller driver. > > > For "Case A" there should be no filtering or any issues even now. You > > > have controller requesting a channel and as long as they get a channel > > > for respective pool, it should work. > > > > > > Or is there anything else which is required in this case? > > > > Yes, there is. You keep complaining about my use of .priv pointer, which I > > use in the filter to pass channel configuration to the DMA controller > > driver. That is the original reason for this whole discussion. If you're > > fine with that, I'll just carry that .priv use over in the "simple" DMA > > engine driver and stay happy forever:-) > Channel configuration != filtering & channel allocation > > The discussion is focused on getting request_channel fixed so that we > take care of oddities in respective platforms for channel maps which > exist. > What the heck does it have to on how you configure you channel?? > > Once you have channel allocated correctly which can work for you, It can be made to work as long as there's only one DMAC group with configurable channels and all other DMACs are dedicated to specific peripherals, yes. I don't know whether there are already now or are approaching any platforms with multiple reconfigurable groups. > you > need to configure you channel. Since this is slave case you to tell what > your client FIFO depth, burst size, width etc parameters. There is > dma_slave_config for that purpose ONLY. Is there anything else required > for _configuring_ if yes, we can add those. But then again that doesn't > have anything to do with how you get a channel. As Russell mentioned, struct dma_slave_config isn't suitable for channel configuration. Currently we use struct sh_dmae_slave_config to configure channels. As you can see, apart from the client's address, it also contains two fields with register values, that have to be used with this client. We could use dmaengine_slave_config() and embed struct dma_slave_config in our private type, but out of 7 fields of that struct effectively only one would be used, which makes this approach both clumsy and inefficient. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-09 12:20 ` Guennadi Liakhovetski @ 2012-03-09 14:07 ` Russell King - ARM Linux 2012-03-09 14:15 ` Guennadi Liakhovetski 2012-03-12 2:47 ` Vinod Koul 1 sibling, 1 reply; 70+ messages in thread From: Russell King - ARM Linux @ 2012-03-09 14:07 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Vinod Koul, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Fri, Mar 09, 2012 at 01:20:08PM +0100, Guennadi Liakhovetski wrote: > As Russell mentioned, struct dma_slave_config isn't suitable for channel > configuration. Actually, I said the exact reverse of what you've just stated. I also stated (on more than one occasion) that dma_slave_config contains nothing relevant to channel selection. ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-09 14:07 ` Russell King - ARM Linux @ 2012-03-09 14:15 ` Guennadi Liakhovetski 0 siblings, 0 replies; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-09 14:15 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Vinod Koul, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Fri, 9 Mar 2012, Russell King - ARM Linux wrote: > On Fri, Mar 09, 2012 at 01:20:08PM +0100, Guennadi Liakhovetski wrote: > > As Russell mentioned, struct dma_slave_config isn't suitable for channel > > configuration. > > Actually, I said the exact reverse of what you've just stated. > > I also stated (on more than one occasion) that dma_slave_config contains > nothing relevant to channel selection. Oops, ok, sorry, should have re-read. In any case, it is not suitable to configure sh-mobile DMAC channels. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-09 12:20 ` Guennadi Liakhovetski 2012-03-09 14:07 ` Russell King - ARM Linux @ 2012-03-12 2:47 ` Vinod Koul 2012-03-12 19:47 ` Linus Walleij 2012-03-16 9:36 ` Guennadi Liakhovetski 1 sibling, 2 replies; 70+ messages in thread From: Vinod Koul @ 2012-03-12 2:47 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Fri, 2012-03-09 at 13:20 +0100, Guennadi Liakhovetski wrote: > It can be made to work as long as there's only one DMAC group with > configurable channels and all other DMACs are dedicated to specific > peripherals, yes. I don't know whether there are already now or are > approaching any platforms with multiple reconfigurable groups. And that is what I am talking about. Having specific channel mapping given by platform for all channels which are to be used dedicated. And a pool of channels which can be used by anyone (if they can be) on a platform. Does this proposal sound good for others as well. I think we can target this for next merge cycle, we are too late for the current one. -- ~Vinod ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-12 2:47 ` Vinod Koul @ 2012-03-12 19:47 ` Linus Walleij 2012-03-16 9:36 ` Guennadi Liakhovetski 1 sibling, 0 replies; 70+ messages in thread From: Linus Walleij @ 2012-03-12 19:47 UTC (permalink / raw) To: Vinod Koul Cc: Guennadi Liakhovetski, Russell King - ARM Linux, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt On Mon, Mar 12, 2012 at 3:47 AM, Vinod Koul <vinod.koul@intel.com> wrote: > On Fri, 2012-03-09 at 13:20 +0100, Guennadi Liakhovetski wrote: >> It can be made to work as long as there's only one DMAC group with >> configurable channels and all other DMACs are dedicated to specific >> peripherals, yes. I don't know whether there are already now or are >> approaching any platforms with multiple reconfigurable groups. > > And that is what I am talking about. > > Having specific channel mapping given by platform for all channels which > are to be used dedicated. And a pool of channels which can be used by > anyone (if they can be) on a platform. > > Does this proposal sound good for others as well. It sounds good to me. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-12 2:47 ` Vinod Koul 2012-03-12 19:47 ` Linus Walleij @ 2012-03-16 9:36 ` Guennadi Liakhovetski 2012-03-16 10:16 ` Linus Walleij 2012-03-19 11:37 ` Vinod Koul 1 sibling, 2 replies; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-16 9:36 UTC (permalink / raw) To: Vinod Koul Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Mon, 12 Mar 2012, Vinod Koul wrote: > On Fri, 2012-03-09 at 13:20 +0100, Guennadi Liakhovetski wrote: > > It can be made to work as long as there's only one DMAC group with > > configurable channels and all other DMACs are dedicated to specific > > peripherals, yes. I don't know whether there are already now or are > > approaching any platforms with multiple reconfigurable groups. > And that is what I am talking about. > > Having specific channel mapping given by platform for all channels which > are to be used dedicated. And a pool of channels which can be used by > anyone (if they can be) on a platform. > > Does this proposal sound good for others as well. I think we can target > this for next merge cycle, we are too late for the current one. Ok, let me try to summarise, what this would mean for sh-mobile: 1. this proposal introduces a new special case: with or without a mapping, that will have to be handled in affected client and DMA controller drivers. E.g., on sh-mobile some devices might on some systems use channels from "general purpose" DMA controllers (no mapping), on other systems it will be a dedicated controller (fixed mapping). 2. this will break, if we get more than 1 "general purpose" type with different supported client sets. So, we develop a new API with a pre-programmed limitation. 3. this will mean a substantial driver and platform code modification. Nothing super-complex, but still some. 4. we'll need a 3-stage channel allocation / configuration: request, filter, config. Whereas with my configuration-parameter proposal it's just one stage: allocate-and-configure. So, yes, this would be doable, but it doesn't look like a very good solution to me. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-16 9:36 ` Guennadi Liakhovetski @ 2012-03-16 10:16 ` Linus Walleij 2012-03-16 10:31 ` Russell King - ARM Linux ` (2 more replies) 2012-03-19 11:37 ` Vinod Koul 1 sibling, 3 replies; 70+ messages in thread From: Linus Walleij @ 2012-03-16 10:16 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Vinod Koul, Russell King - ARM Linux, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt On Fri, Mar 16, 2012 at 10:36 AM, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > Ok, let me try to summarise, what this would mean for sh-mobile: > > 1. this proposal introduces a new special case: with or without a mapping, > that will have to be handled in affected client and DMA controller > drivers. E.g., on sh-mobile some devices might on some systems use > channels from "general purpose" DMA controllers (no mapping), on other > systems it will be a dedicated controller (fixed mapping). > > 2. this will break, if we get more than 1 "general purpose" type with > different supported client sets. So, we develop a new API with a > pre-programmed limitation. I fail to see why this would not be solved by a one-to-many mapping? Flag for each device which channels it may use in a mapping table in platform data or device tree, I don't see the problem. You don't even have to specify that on a per-channel basis if you can come up with something more clever in the mapping table, such as "this device can use any channel on this DMAC, and channels 1-7 on that DMAC" - no problem? > 3. this will mean a substantial driver and platform code modification. > Nothing super-complex, but still some. Big deal. Refactoring is fun... ;-) > 4. we'll need a 3-stage channel allocation / configuration: request, > filter, config. In my world: channel request with *NO* filter function. Filter functions are part of the problem. So we refactor these away as part of this change. That's the whole point... The core gathers information from the platform and the DMAC driver(s) to build up the constraints necessary to hand out workling channels to each device that request one. And Russell IIRC already suggested a request-and-config channel inline for the simple cases, and if you still need to explicitly runtime-reconfigure then that's for a good reason. > Whereas with my configuration-parameter proposal it's just > one stage: allocate-and-configure. For one specific hardware, yes. For DMAengine at large and the majority of the drivers, no. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-16 10:16 ` Linus Walleij @ 2012-03-16 10:31 ` Russell King - ARM Linux 2012-03-16 11:09 ` Guennadi Liakhovetski 2012-03-19 11:39 ` Vinod Koul 2 siblings, 0 replies; 70+ messages in thread From: Russell King - ARM Linux @ 2012-03-16 10:31 UTC (permalink / raw) To: Linus Walleij Cc: Guennadi Liakhovetski, Vinod Koul, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt On Fri, Mar 16, 2012 at 11:16:14AM +0100, Linus Walleij wrote: > And Russell IIRC already suggested a request-and-config > channel inline for the simple cases, and if you still need to > explicitly runtime-reconfigure then that's for a good > reason. Here's a patch which does request-and-configure - untested at the moment apart from build-testing for my Assabet platform. Haven't converted any drivers to use it yet either. drivers/dma/dmaengine.c | 14 ++++++++++++-- include/linux/dmaengine.h | 13 +++++++++---- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c index a6c6051..9c920a6 100644 --- a/drivers/dma/dmaengine.c +++ b/drivers/dma/dmaengine.c @@ -485,7 +485,8 @@ static struct dma_chan *private_candidate(dma_cap_mask_t *mask, struct dma_devic * @fn: optional callback to disposition available channels * @fn_param: opaque parameter to pass to dma_filter_fn */ -struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param) +struct dma_chan *__dma_request_config_channel(dma_cap_mask_t *mask, + dma_filter_fn fn, void *fn_param, struct dma_slave_config *cfg) { struct dma_device *device, *_d; struct dma_chan *chan = NULL; @@ -521,12 +522,21 @@ struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, v } mutex_unlock(&dma_list_mutex); + if (chan && cfg) { + int ret = device->device_control(chan, DMA_SLAVE_CONFIG, + (unsigned long)cfg); + if (ret) { + dma_release_channel(chan); + chan = NULL; + } + } + pr_debug("%s: %s (%s)\n", __func__, chan ? "success" : "fail", chan ? dma_chan_name(chan) : NULL); return chan; } -EXPORT_SYMBOL_GPL(__dma_request_channel); +EXPORT_SYMBOL_GPL(__dma_request_config_channel); void dma_release_channel(struct dma_chan *chan) { diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index 4b17ca8..9a4e9e9 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -924,7 +924,8 @@ enum dma_status dma_sync_wait(struct dma_chan *chan, dma_cookie_t cookie); #ifdef CONFIG_DMA_ENGINE enum dma_status dma_wait_for_async_tx(struct dma_async_tx_descriptor *tx); void dma_issue_pending_all(void); -struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param); +struct dma_chan *__dma_request_config_channel(dma_cap_mask_t *mask, + dma_filter_fn fn, void *fn_param, struct dma_slave_config *cfg); void dma_release_channel(struct dma_chan *chan); #else static inline enum dma_status dma_wait_for_async_tx(struct dma_async_tx_descriptor *tx) @@ -934,8 +935,9 @@ static inline enum dma_status dma_wait_for_async_tx(struct dma_async_tx_descript static inline void dma_issue_pending_all(void) { } -static inline struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, - dma_filter_fn fn, void *fn_param) +static inline struct dma_chan *__dma_request_config_channel( + dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param, + struct dma_slave_config *cfg) { return NULL; } @@ -950,7 +952,10 @@ int dma_async_device_register(struct dma_device *device); void dma_async_device_unregister(struct dma_device *device); void dma_run_dependencies(struct dma_async_tx_descriptor *tx); struct dma_chan *dma_find_channel(enum dma_transaction_type tx_type); -#define dma_request_channel(mask, x, y) __dma_request_channel(&(mask), x, y) +#define dma_request_channel(mask, x, y) \ + __dma_request_config_channel(&(mask), x, y, NULL) +#define dma_request_config_channel(mask, x, y, cfg) \ + __dma_request_config_channel(&(mask), x, y, cfg) /* --- Helper iov-locking functions --- */ ^ permalink raw reply related [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-16 10:16 ` Linus Walleij 2012-03-16 10:31 ` Russell King - ARM Linux @ 2012-03-16 11:09 ` Guennadi Liakhovetski 2012-03-16 14:11 ` Linus Walleij 2012-03-30 10:25 ` Russell King - ARM Linux 2012-03-19 11:39 ` Vinod Koul 2 siblings, 2 replies; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-16 11:09 UTC (permalink / raw) To: Linus Walleij Cc: Vinod Koul, Russell King - ARM Linux, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt On Fri, 16 Mar 2012, Linus Walleij wrote: > On Fri, Mar 16, 2012 at 10:36 AM, Guennadi Liakhovetski > <g.liakhovetski@gmx.de> wrote: > > > Ok, let me try to summarise, what this would mean for sh-mobile: > > > > 1. this proposal introduces a new special case: with or without a mapping, > > that will have to be handled in affected client and DMA controller > > drivers. E.g., on sh-mobile some devices might on some systems use > > channels from "general purpose" DMA controllers (no mapping), on other > > systems it will be a dedicated controller (fixed mapping). > > > > 2. this will break, if we get more than 1 "general purpose" type with > > different supported client sets. So, we develop a new API with a > > pre-programmed limitation. > > I fail to see why this would not be solved by a one-to-many mapping? > > Flag for each device which channels it may use in a mapping > table in platform data or device tree, I don't see the problem. > > You don't even have to specify that on a per-channel basis if > you can come up with something more clever in the mapping > table, such as "this device can use any channel on this DMAC, > and channels 1-7 on that DMAC" - no problem? Sure, everything is possible. So, would something like this make you happy: struct dma_channel_range { const char *dma_device; int channel_start; int channel_end; }; struct dma_map { const char *name; const char *client; const struct dma_channel_range *chan_range; int chan_range_num; }; You really want to do this?... And the least important question: who and when will implement the core support for this? > > 3. this will mean a substantial driver and platform code modification. > > Nothing super-complex, but still some. > > Big deal. Refactoring is fun... ;-) > > > 4. we'll need a 3-stage channel allocation / configuration: request, > > filter, config. > > In my world: channel request with *NO* filter function. How??? Again: 1. the client issues a dma_request_channel() with _just_ a capability mask and a filter and its argument as parameters - _nothing_ about channel restrictions. 2. you propose to eliminate a filter - the core has no way to know, which channel to pick up... 3. the wrapper, proposed by Russell, now calls dmaengine_slave_config(), which fails, because that's a wrong channel (hope I get this right this time - configuration has nothing to do with selection:-)) 4. that's it, if you start again - the dmaengine core will enumerate the same channels again and propose the same unsuitable channel to you - there's no way to continue to the next channel / device. What am I missing? How is the mapping going to be used, if you eliminate the filter function? > Filter functions are part of the problem. So we refactor these > away as part of this change. That's the whole point... > > The core gathers information from the platform and the > DMAC driver(s) to build up the constraints necessary to > hand out workling channels to each device that request > one. > > And Russell IIRC already suggested a request-and-config > channel inline for the simple cases, and if you still need to > explicitly runtime-reconfigure then that's for a good > reason. > > > Whereas with my configuration-parameter proposal it's just > > one stage: allocate-and-configure. > > For one specific hardware, yes. For DMAengine at large > and the majority of the drivers, no. Sorry, why? I don't think I saw an answer to it apart from - maintenance burden... You can use that parameter to actually pass information to be used by the core to scan your mapping tables, I really don't see how you want to use those tables with the existing dmaengine channel-allocation API. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-16 11:09 ` Guennadi Liakhovetski @ 2012-03-16 14:11 ` Linus Walleij 2012-03-16 14:28 ` Guennadi Liakhovetski 2012-03-19 11:58 ` Vinod Koul 2012-03-30 10:25 ` Russell King - ARM Linux 1 sibling, 2 replies; 70+ messages in thread From: Linus Walleij @ 2012-03-16 14:11 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Vinod Koul, Russell King - ARM Linux, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt On Fri, Mar 16, 2012 at 12:09 PM, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > And the least important question: who and when will implement the core > support for this? I'm trying to call the kernel HR department to hire a consultant for me but they just put me on the phone queue all the time, I don't know what I might be doing wrong ... :-) If the question is whether we need more people writing complicated core patches for the dmaengine I think the answer is "yes"? > 1. the client issues a dma_request_channel() with _just_ a capability mask > and a filter and its argument as parameters - _nothing_ about channel > restrictions. > > 2. you propose to eliminate a filter - the core has no way to know, which > channel to pick up... Nah, thinking about it... Eliminate the external filter, make it internal. We already have the problem that these filter functions need to be passed around too much in platform data e.g. so we need to do away with it. The filter functions seem to come from the DMA drivers themselves mostly. (Help me with the complete picture here...) For example: amba-pl08x.c:bool pl08x_filter_id(struct dma_chan *chan, void *chan_id) coh901318.c:bool coh901318_filter_id(struct dma_chan *chan, void *chan_id) pl330.c:bool pl330_filter(struct dma_chan *chan, void *param) sirf-dma.c:bool sirfsoc_dma_filter_id(struct dma_chan *chan, void *chan_id) ste_dma40.c:bool stedma40_filter(struct dma_chan *chan, void *data) So delete the typedef for dma_filter_fn remove these filters from external header files. And stop that thing from being passed around and into struct dma_device so the dmaengine core can still filter or process channels, but nothing on the outside need to know about it. That way we can centralize it to drivers/dma and not spread it out throughout the kernel. > 3. the wrapper, proposed by Russell, now calls dmaengine_slave_config(), > which fails, because that's a wrong channel (hope I get this right this > time - configuration has nothing to do with selection:-)) Oh I was not thinking of relying on config to sort out channels. I was thinking of internalizing the dma_filter_fn and make it an (optional, maybe?) part of dmaengine. > 4. that's it, if you start again - the dmaengine core will enumerate the > same channels again and propose the same unsuitable channel to you - > there's no way to continue to the next channel / device. > > What am I missing? How is the mapping going to be used, if you eliminate > the filter function? As above, I guess factoring away the filter functions would be the first real hard problem. Thanks, Linus Walleij ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-16 14:11 ` Linus Walleij @ 2012-03-16 14:28 ` Guennadi Liakhovetski 2012-03-30 5:44 ` Linus Walleij 2012-03-30 10:29 ` Russell King - ARM Linux 2012-03-19 11:58 ` Vinod Koul 1 sibling, 2 replies; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-16 14:28 UTC (permalink / raw) To: Linus Walleij Cc: Vinod Koul, Russell King - ARM Linux, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt On Fri, 16 Mar 2012, Linus Walleij wrote: > On Fri, Mar 16, 2012 at 12:09 PM, Guennadi Liakhovetski > <g.liakhovetski@gmx.de> wrote: > > > And the least important question: who and when will implement the core > > support for this? > > I'm trying to call the kernel HR department to hire a consultant for me but they > just put me on the phone queue all the time, I don't know what I might be > doing wrong ... :-) > > If the question is whether we need more people writing complicated core > patches for the dmaengine I think the answer is "yes"? > > > 1. the client issues a dma_request_channel() with _just_ a capability mask > > and a filter and its argument as parameters - _nothing_ about channel > > restrictions. > > > > 2. you propose to eliminate a filter - the core has no way to know, which > > channel to pick up... > > Nah, thinking about it... > > Eliminate the external filter, make it internal. We already have the > problem that these filter functions need to be passed around too much > in platform data e.g. so we need to do away with it. > > The filter functions seem to come from the DMA drivers > themselves mostly. (Help me with the complete picture here...) > For example: > > amba-pl08x.c:bool pl08x_filter_id(struct dma_chan *chan, void *chan_id) > coh901318.c:bool coh901318_filter_id(struct dma_chan *chan, void *chan_id) > pl330.c:bool pl330_filter(struct dma_chan *chan, void *param) > sirf-dma.c:bool sirfsoc_dma_filter_id(struct dma_chan *chan, void *chan_id) > ste_dma40.c:bool stedma40_filter(struct dma_chan *chan, void *data) > > So delete the typedef for dma_filter_fn remove these filters from > external header files. > > And stop that thing from being passed around and into > struct dma_device so the dmaengine core can still filter or process > channels, but nothing on the outside need to know about it. That way > we can centralize it to drivers/dma and not spread it out throughout > the kernel. > > > 3. the wrapper, proposed by Russell, now calls dmaengine_slave_config(), > > which fails, because that's a wrong channel (hope I get this right this > > time - configuration has nothing to do with selection:-)) > > Oh I was not thinking of relying on config to sort out channels. > > I was thinking of internalizing the dma_filter_fn and make it an > (optional, maybe?) part of dmaengine. Yessss!!! Let's do that! :-D Now, you're proposing exactly the same, as what I was proposing! :-) Now you just have to remove the filter function parameter from dma_request_channel() - it is anyway the same for all and implemented in the dmaengine core - and you get dma_request_channel(mask, slave_desc) which is exactly what I was proposing! :-) Ok, I didn't remove the filter function, instead I added one more parameter, but in essence - it is the same! But since you yourself say, that this isn't easy - to remove the filter function from all drivers at once, maybe my variant - add a parameter and transition gradually - is easier! ;-) Thanks Guennadi > > 4. that's it, if you start again - the dmaengine core will enumerate the > > same channels again and propose the same unsuitable channel to you - > > there's no way to continue to the next channel / device. > > > > What am I missing? How is the mapping going to be used, if you eliminate > > the filter function? > > As above, I guess factoring away the filter functions would be > the first real hard problem. > > Thanks, > Linus Walleij > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-16 14:28 ` Guennadi Liakhovetski @ 2012-03-30 5:44 ` Linus Walleij 2012-03-30 6:40 ` Guennadi Liakhovetski 2012-03-30 10:38 ` Russell King - ARM Linux 2012-03-30 10:29 ` Russell King - ARM Linux 1 sibling, 2 replies; 70+ messages in thread From: Linus Walleij @ 2012-03-30 5:44 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Vinod Koul, Russell King - ARM Linux, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt On Fri, Mar 16, 2012 at 3:28 PM, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > On Fri, 16 Mar 2012, Linus Walleij wrote: >> Oh I was not thinking of relying on config to sort out channels. >> >> I was thinking of internalizing the dma_filter_fn and make it an >> (optional, maybe?) part of dmaengine. > > Yessss!!! Let's do that! :-D Now, you're proposing exactly the same, as > what I was proposing! :-) No. > Now you just have to remove the filter function > parameter from dma_request_channel() - it is anyway the same for all and > implemented in the dmaengine core - and you get > > dma_request_channel(mask, slave_desc) What is the point of mask on slave channels? I was more thinking introduce a new call: dma_request_slave_channel(struct device *dev); >From this the core looks up a suitable channel for that device. However you're right (in some later mail) that we need to distinguish between RX/TX channels at this point, so I can agree we need some additional parameter, but that should be very abstract, not containing any custom stuff or any void * or something like that. If the device and direction is really all we need to distinguish a suitable channel (which I imagine) the signature may very well be: dma_request_slave_channel(struct device *dev, enum dma_transfer_direction dir); But I'm not sure. (Keep beating me about it... but at this point I think code speaks more than words.) > which is exactly what I was proposing! :-) Sorry, not at all AFAICT. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-30 5:44 ` Linus Walleij @ 2012-03-30 6:40 ` Guennadi Liakhovetski 2012-03-30 10:38 ` Russell King - ARM Linux 1 sibling, 0 replies; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-30 6:40 UTC (permalink / raw) To: Linus Walleij Cc: Vinod Koul, Russell King - ARM Linux, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt Hi Linus On Fri, 30 Mar 2012, Linus Walleij wrote: > On Fri, Mar 16, 2012 at 3:28 PM, Guennadi Liakhovetski > <g.liakhovetski@gmx.de> wrote: > > On Fri, 16 Mar 2012, Linus Walleij wrote: > > >> Oh I was not thinking of relying on config to sort out channels. > >> > >> I was thinking of internalizing the dma_filter_fn and make it an > >> (optional, maybe?) part of dmaengine. > > > > Yessss!!! Let's do that! :-D Now, you're proposing exactly the same, as > > what I was proposing! :-) > > No. Well, ok, not _exactly_, but in essence. Maybe I didn't explain my proposal well enough, so, the analogy is not that clear. My main requirement was: we need a way for DMA slaves to pass information about the required DMA channel to the dmaengine framework already with the request_channel() call, because just requesting _a_ channel and then looking at it and either accepting or rejecting it is not good enough. My proposal was more generic - pass an opaque parameter to be matched against. You are proposing a very specific channel description, but see more below. > > Now you just have to remove the filter function > > parameter from dma_request_channel() - it is anyway the same for all and > > implemented in the dmaengine core - and you get > > > > dma_request_channel(mask, slave_desc) > > What is the point of mask on slave channels? > I was more thinking introduce a new call: > > dma_request_slave_channel(struct device *dev); > > >From this the core looks up a suitable channel for that device. > > However you're right (in some later mail) that we need to distinguish > between RX/TX channels at this point, so I can agree we need some > additional parameter, but that should be very abstract, not containing > any custom stuff or any void * or something like that. > > If the device and direction is really all we need to distinguish a suitable > channel (which I imagine) the signature may very well be: > > dma_request_slave_channel(struct device *dev, enum dma_transfer_direction dir); > > But I'm not sure. (Keep beating me about it... but at this point > I think code speaks more than words.) Ok, I looked through a datasheet of one of SoCs, that we're working with and most devices require no more than two channels. But then I hit a weird example: the sh7372 SoC has 3 SDHI (SD-card) controller instances. SDHI1 and SDHI2 each can use 2 DMA channels - Tx and Rx, this is also what is currently used. However, SDHI0 has 4 (!) DMA channels listed for it in the datasheet: 2 Rx and 2 Tx. I have no idea whether it is a bug in the documentation (not very likely, this is later confirmed at a different location), or whether those channels are completely identical (what would be the point then???), or whether we'll ever support that in software (ATM we only use two of them), but - a precedent is there... So, we can either pretend, that we don't know about it or decide, that we'll never use it and go ahead with just device/direction, or we can make it more future-proof immediately. But, well, this is a kernel internal API, so, we can change it at any time in the future. Thanks Guennadi > > which is exactly what I was proposing! :-) > > Sorry, not at all AFAICT. > > Yours, > Linus Walleij > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-30 5:44 ` Linus Walleij 2012-03-30 6:40 ` Guennadi Liakhovetski @ 2012-03-30 10:38 ` Russell King - ARM Linux 2012-04-03 20:36 ` Linus Walleij 2012-04-03 20:44 ` Linus Walleij 1 sibling, 2 replies; 70+ messages in thread From: Russell King - ARM Linux @ 2012-03-30 10:38 UTC (permalink / raw) To: Linus Walleij Cc: Guennadi Liakhovetski, Vinod Koul, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt On Fri, Mar 30, 2012 at 07:44:09AM +0200, Linus Walleij wrote: > However you're right (in some later mail) that we need to distinguish > between RX/TX channels at this point, so I can agree we need some > additional parameter, but that should be very abstract, not containing > any custom stuff or any void * or something like that. No you don't. DMA engines can generally deal with transfers operating in either direction on a single channel. Where many of the restrictions come from is the way virtual channels are bound to request signals. Take for instance MMC, where that is strictly half-duplex. You're either transferring data to the card, or from the card. Why would you want to claim two channels _if_ there was some way to properly configure the DMA engine between those two modes? That's something which is lacking in the current DMA engine API, and the reason is that the data required to do that is somewhat platform specific and therefore nebulous. Eg, it could require a different DMA handshake signal, different external MUX selection, or something like that. As I'm looking at converting OMAP to DMA engine, if we persist with our current approach, I need 128 virtual DMA channels, one per request signal. It would be better to either create these on the fly or sort out some way that channels can be _properly_ reconfigured between different handshake lines. Actually, I think that's the key thing: the handshake lines should be the data involved in channel selection and nothing else - though as I've pointed out already, there's the complication for external MUXing between the DMA engine and the peripheral which makes that non-trivial. ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-30 10:38 ` Russell King - ARM Linux @ 2012-04-03 20:36 ` Linus Walleij 2012-04-03 20:44 ` Linus Walleij 1 sibling, 0 replies; 70+ messages in thread From: Linus Walleij @ 2012-04-03 20:36 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Guennadi Liakhovetski, Vinod Koul, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt On Fri, Mar 30, 2012 at 12:38 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Fri, Mar 30, 2012 at 07:44:09AM +0200, Linus Walleij wrote: >> However you're right (in some later mail) that we need to distinguish >> between RX/TX channels at this point, so I can agree we need some >> additional parameter, but that should be very abstract, not containing >> any custom stuff or any void * or something like that. > > No you don't. DMA engines can generally deal with transfers operating in > either direction on a single channel. You're right, and I came to think of it out of reach from a computer. The MMC on the RealViews and U300 indeed works like that. And there has to be one way of doing this instead of hooking it in at channel selection time, config-time is it. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-30 10:38 ` Russell King - ARM Linux 2012-04-03 20:36 ` Linus Walleij @ 2012-04-03 20:44 ` Linus Walleij 2012-04-12 21:33 ` Guennadi Liakhovetski 1 sibling, 1 reply; 70+ messages in thread From: Linus Walleij @ 2012-04-03 20:44 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Guennadi Liakhovetski, Vinod Koul, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt On Fri, Mar 30, 2012 at 12:38 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > Actually, I think that's the key thing: the handshake lines should be > the data involved in channel selection and nothing else - though as I've > pointed out already, there's the complication for external MUXing between > the DMA engine and the peripheral which makes that non-trivial. I think you're onto something here, and it sounds it could be made elegant to me. As for muxed request signals, we would need to hook in some cascading framework if we want it all generic. On the Nomadik 8815 (which I now have up and running) the system has two PL080 instances, each with 16 channels. However the same request lines are *partly* routed to *both* PL080 instances. This would be one of the things we could solve with a line-oriented approach, say we have this set of DMA lines, and then defined a group of lines per DMAC instance, then by stating that this device has this request line we can infer the suitable DMA slave engines and select one with a free channel to run the transfer. It does require some upfront code, but I'm sure it can be made to work. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-04-03 20:44 ` Linus Walleij @ 2012-04-12 21:33 ` Guennadi Liakhovetski 2012-04-12 23:48 ` Russell King - ARM Linux 0 siblings, 1 reply; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-04-12 21:33 UTC (permalink / raw) To: Linus Walleij Cc: Russell King - ARM Linux, Vinod Koul, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt On Tue, 3 Apr 2012, Linus Walleij wrote: > On Fri, Mar 30, 2012 at 12:38 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > > Actually, I think that's the key thing: the handshake lines should be > > the data involved in channel selection and nothing else - though as I've > > pointed out already, there's the complication for external MUXing between > > the DMA engine and the peripheral which makes that non-trivial. > > I think you're onto something here, and it sounds it could be made > elegant to me. Now, how do we proceed from here? Do I understand it right, that Russell would like to implement his ideas? Any idea when this is likely to happen? If not too soon, maybe we could / should work out an intermediate solution? How about, for example: I resubmit my patch, but don't claim it for general use, rather keep it sh-own (even though the generic part will have no hardware-specific code in it). Then we can implement both sh driver on top of that. Of course, the dubious use of the struct dma_chan::private field will remain. But it is there now anyway, so, this will not introduce any new uses of .private or extend the existing one. And once we have a generic way to request specific DMA channels, we'll switch the shdma driver(s) to it too. Thanks Guennadi > > As for muxed request signals, we would need to hook in some > cascading framework if we want it all generic. > > On the Nomadik 8815 (which I now have up and running) > the system has two PL080 instances, each with 16 channels. > However the same request lines are *partly* routed to *both* > PL080 instances. This would be one of the things we could > solve with a line-oriented approach, say we have this set > of DMA lines, and then defined a group of lines per > DMAC instance, then by stating that this device has this > request line we can infer the suitable DMA slave engines > and select one with a free channel to run the transfer. > > It does require some upfront code, but I'm sure it can be > made to work. > > Yours, > Linus Walleij > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-04-12 21:33 ` Guennadi Liakhovetski @ 2012-04-12 23:48 ` Russell King - ARM Linux 0 siblings, 0 replies; 70+ messages in thread From: Russell King - ARM Linux @ 2012-04-12 23:48 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Linus Walleij, Vinod Koul, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt On Thu, Apr 12, 2012 at 11:33:28PM +0200, Guennadi Liakhovetski wrote: > Now, how do we proceed from here? Do I understand it right, that Russell > would like to implement his ideas? Any idea when this is likely to happen? Well, I'm messing around with OMAP at the moment, converting its DMA support to be DMA engine based. As I think I've already said, I've not taken _that_ much interest in dealing with how channels actually get allocated there (though I do have something which works for the time being so I can run and test this stuff.) Note that the sa11x0 DMA engine driver uses strings to match channels, and the strings are essentially identifiers for DMA request signals, and this avoids either passing around DMA request numbers or having them in some central header file. ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-16 14:28 ` Guennadi Liakhovetski 2012-03-30 5:44 ` Linus Walleij @ 2012-03-30 10:29 ` Russell King - ARM Linux 2012-03-30 10:40 ` Guennadi Liakhovetski 1 sibling, 1 reply; 70+ messages in thread From: Russell King - ARM Linux @ 2012-03-30 10:29 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Linus Walleij, Vinod Koul, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt On Fri, Mar 16, 2012 at 03:28:39PM +0100, Guennadi Liakhovetski wrote: > Yessss!!! Let's do that! :-D Now, you're proposing exactly the same, as > what I was proposing! :-) Now you just have to remove the filter function > parameter from dma_request_channel() - it is anyway the same for all and > implemented in the dmaengine core - and you get > > dma_request_channel(mask, slave_desc) > > which is exactly what I was proposing! :-) Bollocks it is. You're wanting to use the peripheral address, width and burst size to try to determine what channel to use. That's a totally crackpot idea, and as I've already said several times it won't work in many real life cases we have already. ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-30 10:29 ` Russell King - ARM Linux @ 2012-03-30 10:40 ` Guennadi Liakhovetski 2012-03-30 10:43 ` Russell King - ARM Linux 0 siblings, 1 reply; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-30 10:40 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Linus Walleij, Vinod Koul, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt On Fri, 30 Mar 2012, Russell King - ARM Linux wrote: > On Fri, Mar 16, 2012 at 03:28:39PM +0100, Guennadi Liakhovetski wrote: > > Yessss!!! Let's do that! :-D Now, you're proposing exactly the same, as > > what I was proposing! :-) Now you just have to remove the filter function > > parameter from dma_request_channel() - it is anyway the same for all and > > implemented in the dmaengine core - and you get > > > > dma_request_channel(mask, slave_desc) > > > > which is exactly what I was proposing! :-) > > Bollocks it is. You're wanting to use the peripheral address, width > and burst size to try to determine what channel to use. That's a > totally crackpot idea, and as I've already said several times it won't > work in many real life cases we have already. I'm afraid I'm just somehow failing to explain my thoughts to you, Russell. I don't think I ever said about using addresses, widths etc. They are not even used _now_ by the shdma.c driver. Why should I propose that then? What I was proposing is to use some kind of an opaque slave ID for that. Which is, I think, pretty much the same as using a device pointer and DMA direction. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-30 10:40 ` Guennadi Liakhovetski @ 2012-03-30 10:43 ` Russell King - ARM Linux 0 siblings, 0 replies; 70+ messages in thread From: Russell King - ARM Linux @ 2012-03-30 10:43 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Linus Walleij, Vinod Koul, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt On Fri, Mar 30, 2012 at 12:40:12PM +0200, Guennadi Liakhovetski wrote: > On Fri, 30 Mar 2012, Russell King - ARM Linux wrote: > > > On Fri, Mar 16, 2012 at 03:28:39PM +0100, Guennadi Liakhovetski wrote: > > > Yessss!!! Let's do that! :-D Now, you're proposing exactly the same, as > > > what I was proposing! :-) Now you just have to remove the filter function > > > parameter from dma_request_channel() - it is anyway the same for all and > > > implemented in the dmaengine core - and you get > > > > > > dma_request_channel(mask, slave_desc) > > > > > > which is exactly what I was proposing! :-) > > > > Bollocks it is. You're wanting to use the peripheral address, width > > and burst size to try to determine what channel to use. That's a > > totally crackpot idea, and as I've already said several times it won't > > work in many real life cases we have already. > > I'm afraid I'm just somehow failing to explain my thoughts to you, > Russell. I don't think I ever said about using addresses, widths etc. Not directly - what you said was about combining the request functionality along with the slave_config functionality. The slave_config functionality takes exactly what I said above: the addresses, widths, burst size of the peripheral. Therefore, you did _by implication of what the functions you have been referring to do_ been talking exactly about using addresses, widths and burst sizes to select the channel. ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-16 14:11 ` Linus Walleij 2012-03-16 14:28 ` Guennadi Liakhovetski @ 2012-03-19 11:58 ` Vinod Koul 1 sibling, 0 replies; 70+ messages in thread From: Vinod Koul @ 2012-03-19 11:58 UTC (permalink / raw) To: Linus Walleij Cc: Guennadi Liakhovetski, Russell King - ARM Linux, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt On Fri, 2012-03-16 at 15:11 +0100, Linus Walleij wrote: > On Fri, Mar 16, 2012 at 12:09 PM, Guennadi Liakhovetski > <g.liakhovetski@gmx.de> wrote: > > > And the least important question: who and when will implement the core > > support for this? I will try to do the dmaengine work for 3.4. The code for each platform needs to come from you guys :-) > > I'm trying to call the kernel HR department to hire a consultant for me but they > just put me on the phone queue all the time, I don't know what I might be > doing wrong ... :-) > > If the question is whether we need more people writing complicated core > patches for the dmaengine I think the answer is "yes"? > > > 1. the client issues a dma_request_channel() with _just_ a capability mask > > and a filter and its argument as parameters - _nothing_ about channel > > restrictions. > > > > 2. you propose to eliminate a filter - the core has no way to know, which > > channel to pick up... > > Nah, thinking about it... > > Eliminate the external filter, make it internal. We already have the > problem that these filter functions need to be passed around too much > in platform data e.g. so we need to do away with it. > > The filter functions seem to come from the DMA drivers > themselves mostly. (Help me with the complete picture here...) > For example: > > amba-pl08x.c:bool pl08x_filter_id(struct dma_chan *chan, void *chan_id) > coh901318.c:bool coh901318_filter_id(struct dma_chan *chan, void *chan_id) > pl330.c:bool pl330_filter(struct dma_chan *chan, void *param) > sirf-dma.c:bool sirfsoc_dma_filter_id(struct dma_chan *chan, void *chan_id) > ste_dma40.c:bool stedma40_filter(struct dma_chan *chan, void *data) > > So delete the typedef for dma_filter_fn remove these filters from > external header files. > > And stop that thing from being passed around and into > struct dma_device so the dmaengine core can still filter or process > channels, but nothing on the outside need to know about it. That way > we can centralize it to drivers/dma and not spread it out throughout > the kernel. The _only_ reason why we issue with filters is due to simple reason of having no way to map channels. So everyone decided to do it as they deemed fit. With platform giving you channel map, and dmaengine doing filtering, do you need filter?? > > > 3. the wrapper, proposed by Russell, now calls dmaengine_slave_config(), > > which fails, because that's a wrong channel (hope I get this right this > > time - configuration has nothing to do with selection:-)) > > Oh I was not thinking of relying on config to sort out channels. NO. That maybe true for sh-mobile or for you, but generically that is not the case. Channel config and selection/mapping are orthogonal concepts so lets keep them so. > > I was thinking of internalizing the dma_filter_fn and make it an > (optional, maybe?) part of dmaengine. > > > 4. that's it, if you start again - the dmaengine core will enumerate the > > same channels again and propose the same unsuitable channel to you - > > there's no way to continue to the next channel / device. > > > > What am I missing? How is the mapping going to be used, if you eliminate > > the filter function? > > As above, I guess factoring away the filter functions would be > the first real hard problem. One we have good mapping solution, they would go away on its own, as people just call dma_request_channel() -- ~Vinod ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-16 11:09 ` Guennadi Liakhovetski 2012-03-16 14:11 ` Linus Walleij @ 2012-03-30 10:25 ` Russell King - ARM Linux 1 sibling, 0 replies; 70+ messages in thread From: Russell King - ARM Linux @ 2012-03-30 10:25 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Linus Walleij, Vinod Koul, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt On Fri, Mar 16, 2012 at 12:09:55PM +0100, Guennadi Liakhovetski wrote: > 3. the wrapper, proposed by Russell, now calls dmaengine_slave_config(), > which fails, because that's a wrong channel (hope I get this right this > time - configuration has nothing to do with selection:-)) I keep saying this. Slave configuration has nothing to do with channel selection. All it does is tell the DMA engine about the essential details about the peripheral it's being asked to transfer data with. If a DMA engine itself doesn't support the peripheral, then none of the channels on that DMA engine would support it - eg, if the peripheral had a byte sized register but your DMA engine only supported word size, that would be a valid reason to have slave_config() fail. However, if that's the case why would you have wired the peripheral up to such a DMA engine? And that's the hint here: you can't use any peripheral with any DMA engine - there has to be physical handshaking signals connecting the two together. And that's what actually controls which DMA engines and DMA channels are suitable. Not what the physical address or width or burst size required by the peripheral. It's all to do with physical electrical signal routing. The sooner you get this silly idea that dmaengine_slave_config() should somehow be involved with DMA channel selection the better it will be for everyone. ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-16 10:16 ` Linus Walleij 2012-03-16 10:31 ` Russell King - ARM Linux 2012-03-16 11:09 ` Guennadi Liakhovetski @ 2012-03-19 11:39 ` Vinod Koul 2 siblings, 0 replies; 70+ messages in thread From: Vinod Koul @ 2012-03-19 11:39 UTC (permalink / raw) To: Linus Walleij Cc: Guennadi Liakhovetski, Russell King - ARM Linux, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt On Fri, 2012-03-16 at 11:16 +0100, Linus Walleij wrote: > On Fri, Mar 16, 2012 at 10:36 AM, Guennadi Liakhovetski > <g.liakhovetski@gmx.de> wrote: > > > Ok, let me try to summarise, what this would mean for sh-mobile: > > > > 1. this proposal introduces a new special case: with or without a mapping, > > that will have to be handled in affected client and DMA controller > > drivers. E.g., on sh-mobile some devices might on some systems use > > channels from "general purpose" DMA controllers (no mapping), on other > > systems it will be a dedicated controller (fixed mapping). > > > > 2. this will break, if we get more than 1 "general purpose" type with > > different supported client sets. So, we develop a new API with a > > pre-programmed limitation. > > I fail to see why this would not be solved by a one-to-many mapping? > > Flag for each device which channels it may use in a mapping > table in platform data or device tree, I don't see the problem. > > You don't even have to specify that on a per-channel basis if > you can come up with something more clever in the mapping > table, such as "this device can use any channel on this DMAC, > and channels 1-7 on that DMAC" - no problem? Thats why added channel number to your proposal :) > > > 3. this will mean a substantial driver and platform code modification. > > Nothing super-complex, but still some. > > Big deal. Refactoring is fun... ;-) > > > 4. we'll need a 3-stage channel allocation / configuration: request, > > filter, config. > > In my world: channel request with *NO* filter function. > > Filter functions are part of the problem. So we refactor these > away as part of this change. That's the whole point... > > The core gathers information from the platform and the > DMAC driver(s) to build up the constraints necessary to > hand out workling channels to each device that request > one. > > And Russell IIRC already suggested a request-and-config > channel inline for the simple cases, and if you still need to > explicitly runtime-reconfigure then that's for a good > reason. > > > Whereas with my configuration-parameter proposal it's just > > one stage: allocate-and-configure. > > For one specific hardware, yes. For DMAengine at large > and the majority of the drivers, no. right, and that is my main concern. > > Yours, > Linus Walleij > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- ~Vinod ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-16 9:36 ` Guennadi Liakhovetski 2012-03-16 10:16 ` Linus Walleij @ 2012-03-19 11:37 ` Vinod Koul 2012-03-19 11:47 ` Guennadi Liakhovetski 1 sibling, 1 reply; 70+ messages in thread From: Vinod Koul @ 2012-03-19 11:37 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Fri, 2012-03-16 at 10:36 +0100, Guennadi Liakhovetski wrote: > On Mon, 12 Mar 2012, Vinod Koul wrote: > > > On Fri, 2012-03-09 at 13:20 +0100, Guennadi Liakhovetski wrote: > > > It can be made to work as long as there's only one DMAC group with > > > configurable channels and all other DMACs are dedicated to specific > > > peripherals, yes. I don't know whether there are already now or are > > > approaching any platforms with multiple reconfigurable groups. > > And that is what I am talking about. > > > > Having specific channel mapping given by platform for all channels which > > are to be used dedicated. And a pool of channels which can be used by > > anyone (if they can be) on a platform. > > > > Does this proposal sound good for others as well. I think we can target > > this for next merge cycle, we are too late for the current one. > > Ok, let me try to summarise, what this would mean for sh-mobile: > > 1. this proposal introduces a new special case: with or without a mapping, > that will have to be handled in affected client and DMA controller > drivers. E.g., on sh-mobile some devices might on some systems use > channels from "general purpose" DMA controllers (no mapping), on other > systems it will be a dedicated controller (fixed mapping). that should work. The mapping is platform specific, so I expect the board handling code for that one to tell dmaengine the mapping. On device A: controller P can be generic but on some other device it can be dedicated. > > 2. this will break, if we get more than 1 "general purpose" type with > different supported client sets. So, we develop a new API with a > pre-programmed limitation. No, see above > > 3. this will mean a substantial driver and platform code modification. > Nothing super-complex, but still some. Again No to driver, Yes to platform mapping part, which is again device specfic > > 4. we'll need a 3-stage channel allocation / configuration: request, > filter, config. Whereas with my configuration-parameter proposal it's just > one stage: allocate-and-configure. Its not about stages, it about doing the right thing. Which happens to make dmaengine aware of the mapping which exists for a certain device and give you the channels based on how hardware is mapped. If we get this right then we dont need to worry about filtering as well, that can go away. With Russell's approach it just request_config one single step to get channel and get it configured for slave. > > So, yes, this would be doable, but it doesn't look like a very good > solution to me. -- ~Vinod ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-19 11:37 ` Vinod Koul @ 2012-03-19 11:47 ` Guennadi Liakhovetski 2012-03-19 13:34 ` Vinod Koul 0 siblings, 1 reply; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-19 11:47 UTC (permalink / raw) To: Vinod Koul Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Mon, 19 Mar 2012, Vinod Koul wrote: > On Fri, 2012-03-16 at 10:36 +0100, Guennadi Liakhovetski wrote: > > On Mon, 12 Mar 2012, Vinod Koul wrote: > > > > > On Fri, 2012-03-09 at 13:20 +0100, Guennadi Liakhovetski wrote: > > > > It can be made to work as long as there's only one DMAC group with > > > > configurable channels and all other DMACs are dedicated to specific > > > > peripherals, yes. I don't know whether there are already now or are > > > > approaching any platforms with multiple reconfigurable groups. > > > And that is what I am talking about. > > > > > > Having specific channel mapping given by platform for all channels which > > > are to be used dedicated. And a pool of channels which can be used by > > > anyone (if they can be) on a platform. > > > > > > Does this proposal sound good for others as well. I think we can target > > > this for next merge cycle, we are too late for the current one. > > > > Ok, let me try to summarise, what this would mean for sh-mobile: > > > > 1. this proposal introduces a new special case: with or without a mapping, > > that will have to be handled in affected client and DMA controller > > drivers. E.g., on sh-mobile some devices might on some systems use > > channels from "general purpose" DMA controllers (no mapping), on other > > systems it will be a dedicated controller (fixed mapping). > that should work. The mapping is platform specific, so I expect the > board handling code for that one to tell dmaengine the mapping. > On device A: controller P can be generic but on some other device it can > be dedicated. As I wrote in a reply to Linus W - you need to pass information about the requesting client to the dmaengine core to let it match it against mapping tables. You have to pass this information with the dma_request_channel() function. So, either you need to add a parameter or you have to reuse one of existing ones, e.g., deprecate the filter and use its argument for this purpose. If you do this and as long as you pass that parameter further on to the dmaengine device (controller) driver after whatever matching you like to do in the core - I'm fine with that, that fits well with my initial proposal. Thanks Guennadi > > > > 2. this will break, if we get more than 1 "general purpose" type with > > different supported client sets. So, we develop a new API with a > > pre-programmed limitation. > No, see above > > > > 3. this will mean a substantial driver and platform code modification. > > Nothing super-complex, but still some. > Again No to driver, Yes to platform mapping part, which is again device > specfic > > > > 4. we'll need a 3-stage channel allocation / configuration: request, > > filter, config. Whereas with my configuration-parameter proposal it's just > > one stage: allocate-and-configure. > Its not about stages, it about doing the right thing. Which happens to > make dmaengine aware of the mapping which exists for a certain device > and give you the channels based on how hardware is mapped. > > If we get this right then we dont need to worry about filtering as well, > that can go away. With Russell's approach it just request_config one > single step to get channel and get it configured for slave. > > > > So, yes, this would be doable, but it doesn't look like a very good > > solution to me. > > > -- > ~Vinod > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-19 11:47 ` Guennadi Liakhovetski @ 2012-03-19 13:34 ` Vinod Koul 2012-03-19 13:38 ` Guennadi Liakhovetski 0 siblings, 1 reply; 70+ messages in thread From: Vinod Koul @ 2012-03-19 13:34 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Mon, 2012-03-19 at 12:47 +0100, Guennadi Liakhovetski wrote: > As I wrote in a reply to Linus W - you need to pass information about the > requesting client to the dmaengine core to let it match it against mapping > tables. NO. The client only needs to say that he needs a channel for DMA_SLAVE DMAengine will know for this client, the platform channel map (already given to it by platform) says that we can give it DMAC X, channel 4 only. So see if it free, if so allocate it and give to client (while doing usual stuff) > You have to pass this information with the dma_request_channel() > function. So, either you need to add a parameter or you have to reuse one > of existing ones, e.g., deprecate the filter and use its argument for this > purpose. If you do this and as long as you pass that parameter further on > to the dmaengine device (controller) driver after whatever matching you > like to do in the core - I'm fine with that, that fits well with my > initial proposal. I don't care about filter, it can go away if it is not required. Passing slave_config is *enhancement* so for (hopefully) last time a) it has *nothing* to do with getting a channel, no role to play in generic scheme of things b) it allows client to call one api for get+configure thats all! -- ~Vinod ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-19 13:34 ` Vinod Koul @ 2012-03-19 13:38 ` Guennadi Liakhovetski 2012-03-19 14:00 ` Vinod Koul 0 siblings, 1 reply; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-19 13:38 UTC (permalink / raw) To: Vinod Koul Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Mon, 19 Mar 2012, Vinod Koul wrote: > On Mon, 2012-03-19 at 12:47 +0100, Guennadi Liakhovetski wrote: > > As I wrote in a reply to Linus W - you need to pass information about the > > requesting client to the dmaengine core to let it match it against mapping > > tables. > NO. > The client only needs to say that he needs a channel for DMA_SLAVE How? dma_request_channel(mask, filter_fn, filter_arg) Where shall the client pass to dmaengine its identity info? > DMAengine will know for this client, the platform channel map (already > given to it by platform) says that we can give it DMAC X, channel 4 > only. Some clients need multiple channels - Tx, Rx,... Thanks Guennadi > So see if it free, if so allocate it and give to client (while > doing usual stuff) > > You have to pass this information with the dma_request_channel() > > function. So, either you need to add a parameter or you have to reuse one > > of existing ones, e.g., deprecate the filter and use its argument for this > > purpose. If you do this and as long as you pass that parameter further on > > to the dmaengine device (controller) driver after whatever matching you > > like to do in the core - I'm fine with that, that fits well with my > > initial proposal. > I don't care about filter, it can go away if it is not required. > > Passing slave_config is *enhancement* so for (hopefully) last time > a) it has *nothing* to do with getting a channel, no role to play in > generic scheme of things > b) it allows client to call one api for get+configure thats all! > > -- > ~Vinod > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-19 13:38 ` Guennadi Liakhovetski @ 2012-03-19 14:00 ` Vinod Koul 2012-03-19 14:09 ` Guennadi Liakhovetski 0 siblings, 1 reply; 70+ messages in thread From: Vinod Koul @ 2012-03-19 14:00 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Mon, 2012-03-19 at 14:38 +0100, Guennadi Liakhovetski wrote: > On Mon, 19 Mar 2012, Vinod Koul wrote: > > > On Mon, 2012-03-19 at 12:47 +0100, Guennadi Liakhovetski wrote: > > > As I wrote in a reply to Linus W - you need to pass information about the > > > requesting client to the dmaengine core to let it match it against mapping > > > tables. > > NO. > > The client only needs to say that he needs a channel for DMA_SLAVE > > How? Did you miss our earlier discussion on https://lkml.org/lkml/2012/3/8/26 Perhaps, I can enlighten you will below excerpt where we proposed to add: int dmaengine_add_channel_map(struct dmaengine_map *map, unsigned int num_entries) { /* store this map into dmaengine and use for channle allocation */ } So dmaengine knows client A can use DMAC P, channel 2 and 3. So first request will receive ch 2 for A and second one will yield 3. > > dma_request_channel(mask, filter_fn, filter_arg) > > Where shall the client pass to dmaengine its identity info? Only change I see for above to tell which client is requesting, so we may have to add device pointer of client while requesting. > > > DMAengine will know for this client, the platform channel map (already > > given to it by platform) says that we can give it DMAC X, channel 4 > > only. > > Some clients need multiple channels - Tx, Rx,... > > Thanks > Guennadi > > > So see if it free, if so allocate it and give to client (while > > doing usual stuff) > > > You have to pass this information with the dma_request_channel() > > > function. So, either you need to add a parameter or you have to reuse one > > > of existing ones, e.g., deprecate the filter and use its argument for this > > > purpose. If you do this and as long as you pass that parameter further on > > > to the dmaengine device (controller) driver after whatever matching you > > > like to do in the core - I'm fine with that, that fits well with my > > > initial proposal. > > I don't care about filter, it can go away if it is not required. > > > > Passing slave_config is *enhancement* so for (hopefully) last time > > a) it has *nothing* to do with getting a channel, no role to play in > > generic scheme of things > > b) it allows client to call one api for get+configure thats all! > > > > -- > > ~Vinod > > > > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer > http://www.open-technology.de/ > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- ~Vinod ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-19 14:00 ` Vinod Koul @ 2012-03-19 14:09 ` Guennadi Liakhovetski 2012-03-19 14:22 ` Vinod Koul 0 siblings, 1 reply; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-19 14:09 UTC (permalink / raw) To: Vinod Koul Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Mon, 19 Mar 2012, Vinod Koul wrote: > On Mon, 2012-03-19 at 14:38 +0100, Guennadi Liakhovetski wrote: > > On Mon, 19 Mar 2012, Vinod Koul wrote: > > > > > On Mon, 2012-03-19 at 12:47 +0100, Guennadi Liakhovetski wrote: > > > > As I wrote in a reply to Linus W - you need to pass information about the > > > > requesting client to the dmaengine core to let it match it against mapping > > > > tables. > > > NO. > > > The client only needs to say that he needs a channel for DMA_SLAVE > > > > How? > Did you miss our earlier discussion on No, I did not. > https://lkml.org/lkml/2012/3/8/26 > > Perhaps, I can enlighten you will below excerpt where we proposed to > add: > > int dmaengine_add_channel_map(struct dmaengine_map *map, unsigned int num_entries) > { > /* store this map into dmaengine and use for channle allocation */ > } > > So dmaengine knows client A can use DMAC P, channel 2 and 3. I understand this. > So first request will receive ch 2 for A and second one will yield 3. > > > > > dma_request_channel(mask, filter_fn, filter_arg) > > > > Where shall the client pass to dmaengine its identity info? > Only change I see for above to tell which client is requesting, so we > may have to add device pointer of client while requesting. YES! This is exactly what I am talking about! We need an additional parameter to dma_request_channel(). Whereas in the discussion, that you pointed me to, it still had the same 3 parameters, as now. (Maybe this has already been decided upon before - to add an additional parameter, not sure anymore, this thread has become too long and too slow... My apologies in this case) So, this can be a device pointer or some specialised slave ID. Device pointer is nice, I agree. And the next change, that I'd like to request is pass this parameter further on to DMA device driver's .device_alloc_chan_resources() method. Thanks Guennadi > > > > > DMAengine will know for this client, the platform channel map (already > > > given to it by platform) says that we can give it DMAC X, channel 4 > > > only. > > > > Some clients need multiple channels - Tx, Rx,... > > > > Thanks > > Guennadi > > > > > So see if it free, if so allocate it and give to client (while > > > doing usual stuff) > > > > You have to pass this information with the dma_request_channel() > > > > function. So, either you need to add a parameter or you have to reuse one > > > > of existing ones, e.g., deprecate the filter and use its argument for this > > > > purpose. If you do this and as long as you pass that parameter further on > > > > to the dmaengine device (controller) driver after whatever matching you > > > > like to do in the core - I'm fine with that, that fits well with my > > > > initial proposal. > > > I don't care about filter, it can go away if it is not required. > > > > > > Passing slave_config is *enhancement* so for (hopefully) last time > > > a) it has *nothing* to do with getting a channel, no role to play in > > > generic scheme of things > > > b) it allows client to call one api for get+configure thats all! > > > > > > -- > > > ~Vinod > > > > > > > --- > > Guennadi Liakhovetski, Ph.D. > > Freelance Open-Source Software Developer > > http://www.open-technology.de/ > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ > > > -- > ~Vinod > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-19 14:09 ` Guennadi Liakhovetski @ 2012-03-19 14:22 ` Vinod Koul 2012-03-19 14:45 ` Guennadi Liakhovetski 0 siblings, 1 reply; 70+ messages in thread From: Vinod Koul @ 2012-03-19 14:22 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Mon, 2012-03-19 at 15:09 +0100, Guennadi Liakhovetski wrote: > YES! This is exactly what I am talking about! We need an additional > parameter to dma_request_channel(). Whereas in the discussion, that you > pointed me to, it still had the same 3 parameters, as now. (Maybe this has > already been decided upon before - to add an additional parameter, not > sure anymore, this thread has become too long and too slow... My apologies > in this case) So, this can be a device pointer or some specialised slave > ID. Device pointer is nice, I agree. And the next change, that I'd like to > request is pass this parameter further on to DMA device driver's > .device_alloc_chan_resources() method. Sorry my bad. I confused your request for additional parameter as request for adding slave_config etc stuff into request API. Yes this API would need to be modified for telling dmaengine about client. Now Given that this is more slave stuff, I have leaning to adding a slave specific request api, something like dmaengine_request_slave_channel() which is used for our purpose while keeping the original API intact for async_tx usages. Btw why should .device_alloc_chan_resources() need this, I so no reason, unless we are working around some other issue. -- ~Vinod ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-19 14:22 ` Vinod Koul @ 2012-03-19 14:45 ` Guennadi Liakhovetski 2012-03-19 16:20 ` Vinod Koul 0 siblings, 1 reply; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-19 14:45 UTC (permalink / raw) To: Vinod Koul Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Mon, 19 Mar 2012, Vinod Koul wrote: > On Mon, 2012-03-19 at 15:09 +0100, Guennadi Liakhovetski wrote: > > YES! This is exactly what I am talking about! We need an additional > > parameter to dma_request_channel(). Whereas in the discussion, that you > > pointed me to, it still had the same 3 parameters, as now. (Maybe this has > > already been decided upon before - to add an additional parameter, not > > sure anymore, this thread has become too long and too slow... My apologies > > in this case) So, this can be a device pointer or some specialised slave > > ID. Device pointer is nice, I agree. And the next change, that I'd like to > > request is pass this parameter further on to DMA device driver's > > .device_alloc_chan_resources() method. > Sorry my bad. I confused your request for additional parameter as > request for adding slave_config etc stuff into request API. > > Yes this API would need to be modified for telling dmaengine about > client. Now Given that this is more slave stuff, I have leaning to > adding a slave specific request api, something like > dmaengine_request_slave_channel() which is used for our purpose while > keeping the original API intact for async_tx usages. > > Btw why should .device_alloc_chan_resources() need this, I so no reason, > unless we are working around some other issue. The reason is the following: with the proposed modifications to the existing API we want to achieve: 1. client requests a channel dmaengine_request_slave_channel(dev) 2. the core looks through platform-specific mapping tables and finds a suitable channel 3. the core calls device driver's .device_alloc_chan_resources() method for that channel 4. the client uses dmaengine_slave_config(chan, config) to configure the channel Now, that last channel configuration can have two aspects: (1) static: routing, multiplexing, peripheral address. This needs to be set only once per each such channel allocation to a specific client, (2) (potentially) dynamic: any burst sizes etc. We can pass both these configuration types together to the DMA device driver at step 4. For that the slave would have to embed struct dma_slave_config into another hardware-specific type with additional routing parameters. Otherwise we could pass static configuration, supplied by the platform, from the client to the DMA device driver already at step 1. This way we would avoid having to embed struct dma_slave_config and pass it around even if the driver doesn't actually use it. In some cases step 4 would be then dropped completely. But I can live with either solution. Another reason is, that in the future it can happen, that we get further restrictions on channel selection, that will not fit into the standard mapping scheme. In this case, having access to slave's identification, the DMA device driver will have another chance to check, whether the proposed channel is indeed suitable. BTW, if we only pass a device pointer and return to the client channels one-by-one, how will the client know, which channel it just has got back - Tx or Rx? Will it have to try to configure it for each role and see, which one succeeds?... Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-19 14:45 ` Guennadi Liakhovetski @ 2012-03-19 16:20 ` Vinod Koul 2012-03-19 16:32 ` Guennadi Liakhovetski 0 siblings, 1 reply; 70+ messages in thread From: Vinod Koul @ 2012-03-19 16:20 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Mon, 2012-03-19 at 15:45 +0100, Guennadi Liakhovetski wrote: > > Btw why should .device_alloc_chan_resources() need this, I so no reason, > > unless we are working around some other issue. > > The reason is the following: with the proposed modifications to the > existing API we want to achieve: > > 1. client requests a channel > dmaengine_request_slave_channel(dev) > 2. the core looks through platform-specific mapping tables and finds a > suitable channel > 3. the core calls device driver's .device_alloc_chan_resources() method > for that channel > 4. the client uses > dmaengine_slave_config(chan, config) > to configure the channel > > Now, that last channel configuration can have two aspects: (1) static: > routing, multiplexing, peripheral address. This needs to be set only once > per each such channel allocation to a specific client, correct > (2) (potentially) > dynamic: any burst sizes etc. I dont think burst-size would be dynamic, are they on sh- ? > We can pass both these configuration types > together to the DMA device driver at step 4. For that the slave would have > to embed struct dma_slave_config into another hardware-specific type with > additional routing parameters. I think most of generic parameters are already there. As long as you dont have abnormal controller current dma_slave_config should suffice. Again, if you have specific need pls let us know, this structure can & should be expanded if dmacs needs something for slave configuration. > Otherwise we could pass static > configuration, supplied by the platform, platform, where is that coming into configuration discussion. Platforms role is only for *mapping* and not for configuration. > from the client to the DMA device > driver already at step 1. This way we would avoid having to embed struct > dma_slave_config and pass it around even if the driver doesn't actually > use it. In some cases step 4 would be then dropped completely. But I can > live with either solution. > > Another reason is, that in the future it can happen, that we get further > restrictions on channel selection, that will not fit into the standard > mapping scheme. In this case, having access to slave's identification, the > DMA device driver will have another chance to check, whether the proposed > channel is indeed suitable. DMAC do not and should not check that. It is again job of client to check that. I know sh- doesnt do it this way, but after we sort of mapping, I expect this to be gone. DMACs should not know anything about clients. > > BTW, if we only pass a device pointer and return to the client channels > one-by-one, how will the client know, which channel it just has got back - > Tx or Rx? Will it have to try to configure it for each role and see, which > one succeeds?... There are few options and considerations: a) if platform map forces you to use specific channel for TX and specific one for RX, then you should have mapping explicitly tell you about RX and TX channels. b) if not, then you should get a channel which can work with a client. And then use direction to find about TX and RX for a descriptor. I know some controller can use same channel for both RX and TX. -- ~Vinod ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-19 16:20 ` Vinod Koul @ 2012-03-19 16:32 ` Guennadi Liakhovetski 2012-03-20 7:11 ` Vinod Koul 0 siblings, 1 reply; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-19 16:32 UTC (permalink / raw) To: Vinod Koul Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Mon, 19 Mar 2012, Vinod Koul wrote: > On Mon, 2012-03-19 at 15:45 +0100, Guennadi Liakhovetski wrote: > > > Btw why should .device_alloc_chan_resources() need this, I so no reason, > > > unless we are working around some other issue. > > > > The reason is the following: with the proposed modifications to the > > existing API we want to achieve: > > > > 1. client requests a channel > > dmaengine_request_slave_channel(dev) > > 2. the core looks through platform-specific mapping tables and finds a > > suitable channel > > 3. the core calls device driver's .device_alloc_chan_resources() method > > for that channel > > 4. the client uses > > dmaengine_slave_config(chan, config) > > to configure the channel > > > > Now, that last channel configuration can have two aspects: (1) static: > > routing, multiplexing, peripheral address. This needs to be set only once > > per each such channel allocation to a specific client, > correct > > > (2) (potentially) > > dynamic: any burst sizes etc. > I dont think burst-size would be dynamic, are they on sh- ? Not atm, I just thought some other platforms / clients wanted to configure it dynamically. I may be wrong, though. > > We can pass both these configuration types > > together to the DMA device driver at step 4. For that the slave would have > > to embed struct dma_slave_config into another hardware-specific type with > > additional routing parameters. > I think most of generic parameters are already there. As long as you > dont have abnormal controller current dma_slave_config should suffice. > Again, if you have specific need pls let us know, this structure can & > should be expanded if dmacs needs something for slave configuration. Sure. In the datasheet we have: to configure a channel for client 1 write value X to register A and value Y to register B. That's it. One of them is really just a magic slave ID. Another one has several fields with values like bus width and transfer size. They could be passed "properly," but since those fields also vary between DMAC versions, it is easier to just fix the whole register value per client and pass it from the platform data. In any case, at least the slave ID is really just a constant, that cannot be calculated and has to be passed from platform data. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-19 16:32 ` Guennadi Liakhovetski @ 2012-03-20 7:11 ` Vinod Koul 0 siblings, 0 replies; 70+ messages in thread From: Vinod Koul @ 2012-03-20 7:11 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Russell King - ARM Linux, linux-kernel, 'Jassi Brar', Linus Walleij, Magnus Damm, Paul Mundt On Mon, 2012-03-19 at 17:32 +0100, Guennadi Liakhovetski wrote: > Sure. In the datasheet we have: to configure a channel for client 1 write > value X to register A and value Y to register B. That's it. One of them is > really just a magic slave ID. Another one has several fields with values > like bus width and transfer size. They could be passed "properly," but > since those fields also vary between DMAC versions, it is easier to just > fix the whole register value per client and pass it from the platform > data. In any case, at least the slave ID is really just a constant, that > cannot be calculated and has to be passed from platform data. I think I like the idea of having the these slave IDs coming from platform map. That way drivers can be agnostic... -- ~Vinod ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-08 10:16 ` Guennadi Liakhovetski 2012-03-08 10:55 ` Vinod Koul @ 2012-03-08 11:46 ` Linus Walleij 2012-03-08 12:36 ` Guennadi Liakhovetski 1 sibling, 1 reply; 70+ messages in thread From: Linus Walleij @ 2012-03-08 11:46 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Vinod Koul, Russell King - ARM Linux, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt On Thu, Mar 8, 2012 at 11:16 AM, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: >(...) our case can be handled _very_ easily: > > 1. a client requests a channel of a specific type > 2. one of channels of that type, residing on one of compatible > controllers, is allocated, configured and handed in to the client > > That's it. No filtering, no post-allocation configuration - at least so > far. And penalising such a simple case by forcing it to use virtual > channels and filter through some unnatural mappings doesn't seem very > productive to me. But you do realize that this increases the complexity of the dmaengine and means more maintenance burden for the subsystem maintainer that will after this have to think in two different sematic ways about channel retrieveal - yeah this one passes that as parameter and this one has a config struct provided, then this one use a filter function still - etc. That is, of course, unless you convert all the existing DMA engines to do it the same way, then it's redesigning proper. In that case I am much more positive to the change, even if it doesn't take us all the way to the new channel mappings we want to have. You haven't stated whether you will go in and rewrite the other drivers to use this scheme or whether you will just add this one kludge to handle this one DMA controller, then just update all others to ignore the parameter. (You'd obviously have to do that to get this to even compile...) So the *proper* way to refactor to using this scheme would be to introduce this new scheme *and* remove the filter function from all the other drivers and DMA engine at large, so it's not needed anymore. Which means a bit of refactoring. Currently drivers have to pass a filter function from platform data to filter out relevant channels, and with the new style (this patch plus removing all filter functions) it will be passing something else instead. That's all fine, and actually an improvement, because passing around a filter function is not as good as passing some struct with data. So does RFC patch mean you will fix this up for everyone or does it mean something else? If you're not also planning to get rid of the filter functions for all other drivers I don't see this going anywhere right now. It is not beneficial for dmaengine, the only benefit is one more kludgy driver to maintain. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-08 11:46 ` Linus Walleij @ 2012-03-08 12:36 ` Guennadi Liakhovetski 0 siblings, 0 replies; 70+ messages in thread From: Guennadi Liakhovetski @ 2012-03-08 12:36 UTC (permalink / raw) To: Linus Walleij Cc: Vinod Koul, Russell King - ARM Linux, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt On Thu, 8 Mar 2012, Linus Walleij wrote: > On Thu, Mar 8, 2012 at 11:16 AM, Guennadi Liakhovetski > <g.liakhovetski@gmx.de> wrote: > > >(...) our case can be handled _very_ easily: > > > > 1. a client requests a channel of a specific type > > 2. one of channels of that type, residing on one of compatible > > controllers, is allocated, configured and handed in to the client > > > > That's it. No filtering, no post-allocation configuration - at least so > > far. And penalising such a simple case by forcing it to use virtual > > channels and filter through some unnatural mappings doesn't seem very > > productive to me. > > But you do realize that this increases the complexity of the > dmaengine and means more maintenance burden for the > subsystem maintainer that will after this have to think in two > different sematic ways about channel retrieveal - yeah this one > passes that as parameter and this one has a config struct > provided, then this one use a filter function still - etc. Yes, I do. This is why I've marked it an RFC - I'm open to a discussion, what's the best solution to suit us all. > That is, of course, unless you convert all the existing DMA > engines to do it the same way, then it's redesigning proper. No, don't think it would be reasonable or maybe even possible to completely remove the filter. At least this wasn't an (immediate) goal of this patch. However, if we decide, that in principle such an API extension should make filters redundant, we can gradually over time look at various drivers and get rid of the filters. > In that case I am much more positive to the change, even > if it doesn't take us all the way to the new channel mappings > we want to have. > > You haven't stated whether you will go in and rewrite the other > drivers to use this scheme or whether you will just add this one > kludge to handle this one DMA controller, then just update > all others to ignore the parameter. (You'd obviously have to > do that to get this to even compile...) > > So the *proper* way to refactor to using this scheme would > be to introduce this new scheme *and* remove the filter > function from all the other drivers and DMA engine at large, > so it's not needed anymore. Which means a bit of refactoring. > > Currently drivers have to pass a filter function from > platform data to filter out relevant channels, and with > the new style (this patch plus removing all filter functions) > it will be passing something else instead. That's all > fine, and actually an improvement, because passing around > a filter function is not as good as passing some struct > with data. Agree in principle, but I don't think I can claim, that I'm sufficiently certain, that all drivers can be reasonably converted. At least, thinking again about Russell's approach to implementing filters in DMA device drivers themselves, it seems to me, it should indeed be possible to quite easily just pass the same argument, that's currently passed to the filter function, to the allocation request instead and call the filter internally in .device_alloc_chan_resources() in the very beginning instead. > So does RFC patch mean you will fix this up for everyone > or does it mean something else? I currently count almost 40 calls to dma_request_channel() and around 20-25 DMA drivers in the kernel... So, I'm not sure whether it's reasonable to try to pull such a change globally over one release cycle. Thanks Guennadi > If you're not also planning to get rid of the filter functions > for all other drivers I don't see this going anywhere right > now. It is not beneficial for dmaengine, the only benefit > is one more kludgy driver to maintain. > > Yours, > Linus Walleij --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-07 12:46 ` Russell King - ARM Linux 2012-03-07 13:49 ` Guennadi Liakhovetski @ 2012-03-07 16:31 ` Linus Walleij 1 sibling, 0 replies; 70+ messages in thread From: Linus Walleij @ 2012-03-07 16:31 UTC (permalink / raw) To: Russell King - ARM Linux, Guennadi Liakhovetski Cc: Vinod Koul, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt On Wed, Mar 7, 2012 at 1:46 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > The call to dmaengine_slave_config() actually simplifies the DMA engine > support for some drivers though, so eliminating it doesn't help. What > would be useful is to have a helper function along these lines: > > struct dma_chan *dma_request_channel_config(mask, fn, data, config) > { > struct dma_chan *c = dma_request_channel(mask, fn, data); > > if (c) { > if (dmaengine_slave_config(c, config)) { > dma_release_channel(c); > c = NULL; > } > } > return c; > } > > which would simplify some of the DMA engine users. Yes that will cut down some overhead from some drivers. [Russell] > There'll still be > some though which would want to call dmaengine_slave_config() to change > the channels configuration when the mode of the device switches. [Guennadi] > Cannot dmaengine_slave_config() be used for that? It is already used for that. The PL022 SPI driver does this using different word sizes on the wire, so if the same controller talks to chips with different word width it reconfigures the size of unit picked from memory. Basically the width (not depth) of the FIFO is adjustable run-time. The words have to be fetched from memory with the same width. drivers/spi/amba-pl022.c Yours, Linus Walleij ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-07 10:31 ` Russell King - ARM Linux 2012-03-07 12:30 ` Guennadi Liakhovetski @ 2012-03-07 16:20 ` Linus Walleij 1 sibling, 0 replies; 70+ messages in thread From: Linus Walleij @ 2012-03-07 16:20 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Guennadi Liakhovetski, Vinod Koul, linux-kernel, Jassi Brar, Magnus Damm, Paul Mundt On Wed, Mar 7, 2012 at 11:31 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > For example, the SA11x0 IrDA driver uses two virtual channels, one for > receive and one for transmit. The SA11x0 has a total of five DMA > channels. To waste two of them on IrDA when it's half-duplex is just > silly. Doing the whole 'request+free' thing is also silly because > switching between tx and rx mode is timing-critical. Oh. that's luxury! The ARM PB11MPcore has two physical DMA channels. If I lock one up for say continous UART RX I have only one left, and that need to do everything else. So multiplexing the physical engines it is a must. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 70+ messages in thread
* Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() 2012-03-07 9:18 ` Guennadi Liakhovetski 2012-03-07 9:30 ` Russell King - ARM Linux @ 2012-03-07 9:46 ` Linus Walleij 1 sibling, 0 replies; 70+ messages in thread From: Linus Walleij @ 2012-03-07 9:46 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: Vinod Koul, linux-kernel, Jassi Brar, Russell King On Wed, Mar 7, 2012 at 10:18 AM, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > [Vinod] >> I like the approach outlined by Linus W [1], where we can get the >> information from platform (DT, FW,....) and its presented to dmaengine. > > I still don't see an answer to the very same question, that we've been > discussing over multiple threads and mails now: how do we use that, if > it's not a 1-to-1 mapping? I.e., many channels on many controllers can be > run-time configured for use with different client devices. Also the above > idea from Linus W doesn't directly address this. True, it's a new problem space. However I see nothing wrong in the basic idea that the platform data and/or device tree should supply a number of mappings with constraints to dmaengine, that eventually helps it to select and enable a proper channel. For example in the regulator framework we have voltage constraints on the rails, and the subsystem infers the voltage from these constraints. Constraints in platform data are nice. So the way forward in my simple opinion is to get the core dmaengine to be aware of the applicable constraints and hand out DMA channels or NACK channel requests if these constraints cannot be satisfied. So when you write: > Whereas doing a reverse mapping: for each (potential) > DMA user reference a list of channels, that it can use - would be really > clumsy. I basically disagree. I think the knowledge of available channels and their characteristics should be known to the dmaengine core, and the core shall select what channel to use. Else I fear we end up with a lot of logic distributed all over the place with no consolidation in sight, it will just grow everywhere with each new DMA controller. However it may need to have a different form given what Russell says: many drivers tend to have a number of arbitrable channels and the constraints is really about which event line (burst/single request line) to mux in for that one channel. So the platform data may need to take a form that better reflects this, which in turn necessitates that the dmaengine core and channel request interface be refactored to be aware of this kind of DMAC slaves. It requires a bit of upfront code but I think this is the way forward. Besides, Russell says he's already working on refactoring one part of the problem (virtual channels / request lines) if I understand him correctly. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 70+ messages in thread
end of thread, other threads:[~2012-04-12 23:48 UTC | newest] Thread overview: 70+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-02-01 15:26 [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel() Guennadi Liakhovetski 2012-03-02 13:21 ` Guennadi Liakhovetski 2012-03-06 8:30 ` Vinod Koul 2012-03-06 8:53 ` Guennadi Liakhovetski 2012-03-06 12:08 ` Vinod Koul 2012-03-06 13:03 ` Guennadi Liakhovetski 2012-03-07 6:28 ` Vinod Koul 2012-03-07 9:18 ` Guennadi Liakhovetski 2012-03-07 9:30 ` Russell King - ARM Linux 2012-03-07 9:55 ` Linus Walleij 2012-03-07 10:02 ` Guennadi Liakhovetski 2012-03-07 10:31 ` Russell King - ARM Linux 2012-03-07 12:30 ` Guennadi Liakhovetski 2012-03-07 12:45 ` Guennadi Liakhovetski 2012-03-07 12:46 ` Russell King - ARM Linux 2012-03-07 13:49 ` Guennadi Liakhovetski 2012-03-07 14:26 ` Russell King - ARM Linux 2012-03-07 15:44 ` Guennadi Liakhovetski 2012-03-07 16:27 ` Russell King - ARM Linux 2012-03-07 18:21 ` Guennadi Liakhovetski 2012-03-08 6:30 ` Vinod Koul 2012-03-08 10:16 ` Guennadi Liakhovetski 2012-03-08 10:55 ` Vinod Koul 2012-03-08 11:22 ` Guennadi Liakhovetski 2012-03-08 11:34 ` Vinod Koul 2012-03-08 12:58 ` Vinod Koul 2012-03-08 13:18 ` Guennadi Liakhovetski 2012-03-09 9:21 ` Vinod Koul 2012-03-09 9:24 ` Guennadi Liakhovetski 2012-03-09 9:39 ` Vinod Koul 2012-03-09 12:20 ` Guennadi Liakhovetski 2012-03-09 14:07 ` Russell King - ARM Linux 2012-03-09 14:15 ` Guennadi Liakhovetski 2012-03-12 2:47 ` Vinod Koul 2012-03-12 19:47 ` Linus Walleij 2012-03-16 9:36 ` Guennadi Liakhovetski 2012-03-16 10:16 ` Linus Walleij 2012-03-16 10:31 ` Russell King - ARM Linux 2012-03-16 11:09 ` Guennadi Liakhovetski 2012-03-16 14:11 ` Linus Walleij 2012-03-16 14:28 ` Guennadi Liakhovetski 2012-03-30 5:44 ` Linus Walleij 2012-03-30 6:40 ` Guennadi Liakhovetski 2012-03-30 10:38 ` Russell King - ARM Linux 2012-04-03 20:36 ` Linus Walleij 2012-04-03 20:44 ` Linus Walleij 2012-04-12 21:33 ` Guennadi Liakhovetski 2012-04-12 23:48 ` Russell King - ARM Linux 2012-03-30 10:29 ` Russell King - ARM Linux 2012-03-30 10:40 ` Guennadi Liakhovetski 2012-03-30 10:43 ` Russell King - ARM Linux 2012-03-19 11:58 ` Vinod Koul 2012-03-30 10:25 ` Russell King - ARM Linux 2012-03-19 11:39 ` Vinod Koul 2012-03-19 11:37 ` Vinod Koul 2012-03-19 11:47 ` Guennadi Liakhovetski 2012-03-19 13:34 ` Vinod Koul 2012-03-19 13:38 ` Guennadi Liakhovetski 2012-03-19 14:00 ` Vinod Koul 2012-03-19 14:09 ` Guennadi Liakhovetski 2012-03-19 14:22 ` Vinod Koul 2012-03-19 14:45 ` Guennadi Liakhovetski 2012-03-19 16:20 ` Vinod Koul 2012-03-19 16:32 ` Guennadi Liakhovetski 2012-03-20 7:11 ` Vinod Koul 2012-03-08 11:46 ` Linus Walleij 2012-03-08 12:36 ` Guennadi Liakhovetski 2012-03-07 16:31 ` Linus Walleij 2012-03-07 16:20 ` Linus Walleij 2012-03-07 9:46 ` Linus Walleij
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).