[PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Vinod Koul]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Vinod Koul]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Vinod Koul]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

(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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Russell King - ARM Linux]

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...

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Linus Walleij]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Russell King - ARM Linux]

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.)

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Russell King - ARM Linux]

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.

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Russell King - ARM Linux]

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.

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Russell King - ARM Linux]

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.

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Vinod Koul]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Vinod Koul]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Vinod Koul]

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?

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Vinod Koul]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Vinod Koul]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Vinod Koul]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Russell King - ARM Linux]

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.

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Vinod Koul]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Linus Walleij]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Linus Walleij]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Russell King - ARM Linux]

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 --- */
 

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Linus Walleij]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Linus Walleij]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Russell King - ARM Linux]

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.

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Linus Walleij]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Linus Walleij]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Russell King - ARM Linux]

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.

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Russell King - ARM Linux]

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.

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Russell King - ARM Linux]

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.

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Vinod Koul]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Russell King - ARM Linux]

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.

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Vinod Koul]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Vinod Koul]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Vinod Koul]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Vinod Koul]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Vinod Koul]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Vinod Koul]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Vinod Koul]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Linus Walleij]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Linus Walleij]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Linus Walleij]

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

Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

[Linus Walleij]

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