[PATCH v2 0/3] dmaengine: add per channel capabilities api

[PATCH v2 0/3] dmaengine: add per channel capabilities api

[Matt Porter]

Changes since v1:
	- Use the existing dma_transaction_type enums instead of
	  adding the mostly duplicated dmaengine_apis enums

This series adds a new dmaengine api, dma_get_channels_caps(), which
may be used by a driver to get channel-specific capabilities. This is
based on a starting point suggested by Vinod Koul, but only implements
the minimal sets of channel capabilities to fulfill the needs of the
EDMA DMA Engine driver at this time.

Specifically, this implementation supports reporting of a set of
transfer type operations, maximum number of SG segments, and the
maximum size of an SG segment that a channel can support.

The call is implemented as follows:

	struct dmaengine_chan_caps
	*dma_get_channel_caps(struct dma_chan *chan,
			      enum dma_transfer_direction dir);

The dma transfer direction parameter may appear a bit out of place
but it is necessary since the direction field in struct
dma_slave_config was deprecated. In some cases, EDMA for one, it
is necessary for the dmaengine driver to have the burst and address
width slave configuration parameters available in order to compute
the maximum segment size that can be handle. Due to this requirement,
the calling order of this api is as follows:

1. Allocate a DMA slave channel
1a. [Optionally] Get channel capabilities
2. Set slave and controller specific parameters
3. Get a descriptor for transaction
4. Submit the transaction
5. Issue pending requests and wait for callback notification

Along with the API implementation, this series implements the
backend device_channel_caps() in the EDMA DMA Engine driver and
converts the davinci_mmc driver to use dma_get_channel_caps() to
replace hardcoded limits.

This is tested on the AM1808-EVM.

Matt Porter (3):
  dmaengine: add dma_get_channel_caps()
  dma: edma: add device_channel_caps() support
  mmc: davinci: get SG segment limits with dma_get_channel_caps()

 drivers/dma/edma.c                        |   27 ++++++++++++
 drivers/mmc/host/davinci_mmc.c            |   66 +++++++++--------------------
 include/linux/dmaengine.h                 |   40 +++++++++++++++++
 include/linux/platform_data/mmc-davinci.h |    3 --
 4 files changed, 88 insertions(+), 48 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/3] dmaengine: add dma_get_channel_caps()

[Matt Porter]

Add a dmaengine API to retrieve per channel capabilities.
Currently, only channel ops and SG segment limitations are
implemented caps.

The API is optionally implemented by drivers and when
unimplemented will return a NULL pointer. It is intended
to be executed after a channel has been requested and, if
the channel is intended to be used with slave SG transfers,
then it may only be called after dmaengine_slave_config()
has executed. The slave driver provides parameters such as
burst size and address width which may be necessary for
the dmaengine driver to use in order to properly return SG
segment limit caps.

Suggested-by: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Matt Porter <mporter@ti.com>
---
 include/linux/dmaengine.h |   40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index c88f302..9fd0c5b 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -371,6 +371,26 @@ struct dma_slave_config {
 	unsigned int slave_id;
 };
 
+/* struct dmaengine_chan_caps - expose capability of a channel
+ * Note: each channel can have same or different capabilities
+ *
+ * This primarily classifies capabilities into
+ * a) APIs/ops supported
+ * b) channel physical capabilities
+ *
+ * @cap_mask: api/ops capability (DMA_INTERRUPT and DMA_PRIVATE
+ *	       are invalid api/ops and will never be set)
+ * @seg_nr: maximum number of SG segments supported on a SG/SLAVE
+ *	    channel (0 for no maximum or not a SG/SLAVE channel)
+ * @seg_len: maximum length of SG segments supported on a SG/SLAVE
+ *	     channel (0 for no maximum or not a SG/SLAVE channel)
+ */
+struct dmaengine_chan_caps {
+	dma_cap_mask_t cap_mask;
+	int seg_nr;
+	int seg_len;
+};
+
 static inline const char *dma_chan_name(struct dma_chan *chan)
 {
 	return dev_name(&chan->dev->device);
@@ -534,6 +554,7 @@ struct dma_tx_state {
  *	struct with auxiliary transfer status information, otherwise the call
  *	will just return a simple status code
  * @device_issue_pending: push pending transactions to hardware
+ * @device_channel_caps: return the channel capabilities
  */
 struct dma_device {
 
@@ -602,6 +623,8 @@ struct dma_device {
 					    dma_cookie_t cookie,
 					    struct dma_tx_state *txstate);
 	void (*device_issue_pending)(struct dma_chan *chan);
+	struct dmaengine_chan_caps *(*device_channel_caps)(
+		struct dma_chan *chan, enum dma_transfer_direction direction);
 };
 
 static inline int dmaengine_device_control(struct dma_chan *chan,
@@ -969,6 +992,23 @@ dma_set_tx_state(struct dma_tx_state *st, dma_cookie_t last, dma_cookie_t used,
 	}
 }
 
+/**
+ * dma_get_channel_caps - flush pending transactions to HW
+ * @chan: target DMA channel
+ * @dir: direction of transfer
+ *
+ * Get the channel-specific capabilities. If the dmaengine
+ * driver does not implement per channel capbilities then
+ * NULL is returned.
+ */
+static inline struct dmaengine_chan_caps
+*dma_get_channel_caps(struct dma_chan *chan, enum dma_transfer_direction dir)
+{
+	if (chan->device->device_channel_caps)
+		return chan->device->device_channel_caps(chan, dir);
+	return NULL;
+}
+
 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);
-- 
1.7.9.5

[PATCH v2 2/3] dma: edma: add device_channel_caps() support

[Matt Porter]

Implement device_channel_caps().

EDMA has a finite set of PaRAM slots available for linking
a multi-segment SG transfer. In order to prevent any one
channel from consuming all PaRAM slots to fulfill a large SG
transfer, the driver reports a static per-channel max number
of SG segments it will handle.

The maximum size of SG segment is limited by the slave config
maxburst and addr_width for the channel in question. These values
are used from the current channel config to calculate and return
the max segment length cap.

Signed-off-by: Matt Porter <mporter@ti.com>
---
 drivers/dma/edma.c |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 82c8672..fc4b9db 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -70,6 +70,7 @@ struct edma_chan {
 	bool				alloced;
 	int				slot[EDMA_MAX_SLOTS];
 	struct dma_slave_config		cfg;
+	struct dmaengine_chan_caps	caps;
 };
 
 struct edma_cc {
@@ -462,6 +463,28 @@ static void edma_issue_pending(struct dma_chan *chan)
 	spin_unlock_irqrestore(&echan->vchan.lock, flags);
 }
 
+static struct dmaengine_chan_caps
+*edma_get_channel_caps(struct dma_chan *chan, enum dma_transfer_direction dir)
+{
+	struct edma_chan *echan;
+	enum dma_slave_buswidth width = 0;
+	u32 burst = 0;
+
+	if (chan) {
+		echan = to_edma_chan(chan);
+		if (dir == DMA_MEM_TO_DEV) {
+			width = echan->cfg.dst_addr_width;
+			burst = echan->cfg.dst_maxburst;
+		} else if (dir == DMA_DEV_TO_MEM) {
+			width = echan->cfg.src_addr_width;
+			burst = echan->cfg.src_maxburst;
+		}
+		echan->caps.seg_len = (SZ_64K - 1) * width * burst;
+		return &echan->caps;
+	}
+	return NULL;
+}
+
 static size_t edma_desc_size(struct edma_desc *edesc)
 {
 	int i;
@@ -521,6 +544,9 @@ static void __init edma_chan_init(struct edma_cc *ecc,
 		echan->ch_num = EDMA_CTLR_CHAN(ecc->ctlr, i);
 		echan->ecc = ecc;
 		echan->vchan.desc_free = edma_desc_free;
+		dma_cap_set(DMA_SLAVE, echan->caps.cap_mask);
+		dma_cap_set(DMA_SG, echan->caps.cap_mask);
+		echan->caps.seg_nr = MAX_NR_SG;
 
 		vchan_init(&echan->vchan, dma);
 
@@ -537,6 +563,7 @@ static void edma_dma_init(struct edma_cc *ecc, struct dma_device *dma,
 	dma->device_alloc_chan_resources = edma_alloc_chan_resources;
 	dma->device_free_chan_resources = edma_free_chan_resources;
 	dma->device_issue_pending = edma_issue_pending;
+	dma->device_channel_caps = edma_get_channel_caps;
 	dma->device_tx_status = edma_tx_status;
 	dma->device_control = edma_control;
 	dma->dev = dev;
-- 
1.7.9.5

[PATCH v2 3/3] mmc: davinci: get SG segment limits with dma_get_channel_caps()

[Matt Porter]

Replace the hardcoded values used to set max_segs/max_seg_size with
a dma_get_channel_caps() query to the dmaengine driver.

Signed-off-by: Matt Porter <mporter@ti.com>
---
 drivers/mmc/host/davinci_mmc.c            |   66 +++++++++--------------------
 include/linux/platform_data/mmc-davinci.h |    3 --
 2 files changed, 21 insertions(+), 48 deletions(-)

diff --git a/drivers/mmc/host/davinci_mmc.c b/drivers/mmc/host/davinci_mmc.c
index 2063677..17e186d 100644
--- a/drivers/mmc/host/davinci_mmc.c
+++ b/drivers/mmc/host/davinci_mmc.c
@@ -144,18 +144,6 @@
 /* MMCSD Init clock in Hz in opendrain mode */
 #define MMCSD_INIT_CLOCK		200000
 
-/*
- * One scatterlist dma "segment" is at most MAX_CCNT rw_threshold units,
- * and we handle up to MAX_NR_SG segments.  MMC_BLOCK_BOUNCE kicks in only
- * for drivers with max_segs == 1, making the segments bigger (64KB)
- * than the page or two that's otherwise typical. nr_sg (passed from
- * platform data) == 16 gives at least the same throughput boost, using
- * EDMA transfer linkage instead of spending CPU time copying pages.
- */
-#define MAX_CCNT	((1 << 16) - 1)
-
-#define MAX_NR_SG	16
-
 static unsigned rw_threshold = 32;
 module_param(rw_threshold, uint, S_IRUGO);
 MODULE_PARM_DESC(rw_threshold,
@@ -216,8 +204,6 @@ struct mmc_davinci_host {
 	u8 version;
 	/* for ns in one cycle calculation */
 	unsigned ns_in_one_cycle;
-	/* Number of sg segments */
-	u8 nr_sg;
 #ifdef CONFIG_CPU_FREQ
 	struct notifier_block	freq_transition;
 #endif
@@ -421,16 +407,7 @@ static int mmc_davinci_send_dma_request(struct mmc_davinci_host *host,
 	int ret = 0;
 
 	if (host->data_dir == DAVINCI_MMC_DATADIR_WRITE) {
-		struct dma_slave_config dma_tx_conf = {
-			.direction = DMA_MEM_TO_DEV,
-			.dst_addr = host->mem_res->start + DAVINCI_MMCDXR,
-			.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES,
-			.dst_maxburst =
-				rw_threshold / DMA_SLAVE_BUSWIDTH_4_BYTES,
-		};
 		chan = host->dma_tx;
-		dmaengine_slave_config(host->dma_tx, &dma_tx_conf);
-
 		desc = dmaengine_prep_slave_sg(host->dma_tx,
 				data->sg,
 				host->sg_len,
@@ -443,16 +420,7 @@ static int mmc_davinci_send_dma_request(struct mmc_davinci_host *host,
 			goto out;
 		}
 	} else {
-		struct dma_slave_config dma_rx_conf = {
-			.direction = DMA_DEV_TO_MEM,
-			.src_addr = host->mem_res->start + DAVINCI_MMCDRR,
-			.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES,
-			.src_maxburst =
-				rw_threshold / DMA_SLAVE_BUSWIDTH_4_BYTES,
-		};
 		chan = host->dma_rx;
-		dmaengine_slave_config(host->dma_rx, &dma_rx_conf);
-
 		desc = dmaengine_prep_slave_sg(host->dma_rx,
 				data->sg,
 				host->sg_len,
@@ -1165,6 +1133,7 @@ static int __init davinci_mmcsd_probe(struct platform_device *pdev)
 	struct resource *r, *mem = NULL;
 	int ret = 0, irq = 0;
 	size_t mem_size;
+	struct dmaengine_chan_caps *dma_chan_caps;
 
 	/* REVISIT:  when we're fully converted, fail if pdata is NULL */
 
@@ -1214,12 +1183,6 @@ static int __init davinci_mmcsd_probe(struct platform_device *pdev)
 
 	init_mmcsd_host(host);
 
-	if (pdata->nr_sg)
-		host->nr_sg = pdata->nr_sg - 1;
-
-	if (host->nr_sg > MAX_NR_SG || !host->nr_sg)
-		host->nr_sg = MAX_NR_SG;
-
 	host->use_dma = use_dma;
 	host->mmc_irq = irq;
 	host->sdio_irq = platform_get_irq(pdev, 1);
@@ -1248,14 +1211,27 @@ static int __init davinci_mmcsd_probe(struct platform_device *pdev)
 		mmc->caps |= pdata->caps;
 	mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
 
-	/* With no iommu coalescing pages, each phys_seg is a hw_seg.
-	 * Each hw_seg uses one EDMA parameter RAM slot, always one
-	 * channel and then usually some linked slots.
-	 */
-	mmc->max_segs		= MAX_NR_SG;
+	{
+		struct dma_slave_config dma_txrx_conf = {
+			.src_addr = host->mem_res->start + DAVINCI_MMCDRR,
+			.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES,
+			.src_maxburst =
+				rw_threshold / DMA_SLAVE_BUSWIDTH_4_BYTES,
+			.dst_addr = host->mem_res->start + DAVINCI_MMCDXR,
+			.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES,
+			.dst_maxburst =
+				rw_threshold / DMA_SLAVE_BUSWIDTH_4_BYTES,
+		};
+		dmaengine_slave_config(host->dma_tx, &dma_txrx_conf);
+		dmaengine_slave_config(host->dma_rx, &dma_txrx_conf);
+	}
 
-	/* EDMA limit per hw segment (one or two MBytes) */
-	mmc->max_seg_size	= MAX_CCNT * rw_threshold;
+	/* Just check one channel for the DMA SG limits */
+	dma_chan_caps = dma_get_channel_caps(host->dma_tx, DMA_MEM_TO_DEV);
+	if (dma_chan_caps) {
+		mmc->max_segs = dma_chan_caps->seg_nr;
+		mmc->max_seg_size = dma_chan_caps->seg_len;
+	}
 
 	/* MMC/SD controller limits for multiblock requests */
 	mmc->max_blk_size	= 4095;  /* BLEN is 12 bits */
diff --git a/include/linux/platform_data/mmc-davinci.h b/include/linux/platform_data/mmc-davinci.h
index 5ba6b22..6910209 100644
--- a/include/linux/platform_data/mmc-davinci.h
+++ b/include/linux/platform_data/mmc-davinci.h
@@ -25,9 +25,6 @@ struct davinci_mmc_config {
 
 	/* Version of the MMC/SD controller */
 	u8	version;
-
-	/* Number of sg segments */
-	u8	nr_sg;
 };
 void davinci_setup_mmc(int module, struct davinci_mmc_config *config);
 
-- 
1.7.9.5

Re: [PATCH v2 0/3] dmaengine: add per channel capabilities api

[Vinod Koul]

On Thu, Jan 10, 2013 at 02:07:03PM -0500, Matt Porter wrote:
> The call is implemented as follows:
> 
> 	struct dmaengine_chan_caps
> 	*dma_get_channel_caps(struct dma_chan *chan,
> 			      enum dma_transfer_direction dir);
> 
> The dma transfer direction parameter may appear a bit out of place
> but it is necessary since the direction field in struct
> dma_slave_config was deprecated. In some cases, EDMA for one, it
> is necessary for the dmaengine driver to have the burst and address
> width slave configuration parameters available in order to compute
> the maximum segment size that can be handle. Due to this requirement,
> the calling order of this api is as follows:
Well you are passing direction as argument so even in EDMA it doesn't seem to
help you as you seem to need burst and width!. So why do you even need the
direction to compute the capablities

--
~Vinod

Re: [PATCH v2 1/3] dmaengine: add dma_get_channel_caps()

[Vinod Koul]

On Thu, Jan 10, 2013 at 02:07:04PM -0500, Matt Porter wrote:
> +/* struct dmaengine_chan_caps - expose capability of a channel
> + * Note: each channel can have same or different capabilities
> + *
> + * This primarily classifies capabilities into
> + * a) APIs/ops supported
> + * b) channel physical capabilities
> + *
> + * @cap_mask: api/ops capability (DMA_INTERRUPT and DMA_PRIVATE
> + *	       are invalid api/ops and will never be set)
> + * @seg_nr: maximum number of SG segments supported on a SG/SLAVE
> + *	    channel (0 for no maximum or not a SG/SLAVE channel)
> + * @seg_len: maximum length of SG segments supported on a SG/SLAVE
> + *	     channel (0 for no maximum or not a SG/SLAVE channel)
> + */
> +struct dmaengine_chan_caps {
> +	dma_cap_mask_t cap_mask;
> +	int seg_nr;
> +	int seg_len;
> +};
Now am really unclear why we would need direction as argument.

Also, I would add the channel physical capablities like direction, widths,
lengths etc supported.
 
> +/**
> + * dma_get_channel_caps - flush pending transactions to HW
flush pending... ???

> + * driver does not implement per channel capbilities then
> + * NULL is returned.
> + */
> +static inline struct dmaengine_chan_caps
> +*dma_get_channel_caps(struct dma_chan *chan, enum dma_transfer_direction dir)
you need to add this for when CONFIG_DMA_ENGINE is not defined as well.
> +{
> +	if (chan->device->device_channel_caps)
> +		return chan->device->device_channel_caps(chan, dir);
> +	return NULL;
> +}
> +
>  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);
--
~Vinod

Re: [PATCH v2 2/3] dma: edma: add device_channel_caps() support

[Vinod Koul]

On Thu, Jan 10, 2013 at 02:07:05PM -0500, Matt Porter wrote:
> Implement device_channel_caps().
> 
> EDMA has a finite set of PaRAM slots available for linking
> a multi-segment SG transfer. In order to prevent any one
> channel from consuming all PaRAM slots to fulfill a large SG
> transfer, the driver reports a static per-channel max number
> of SG segments it will handle.
> 
> The maximum size of SG segment is limited by the slave config
> maxburst and addr_width for the channel in question. These values
> are used from the current channel config to calculate and return
> the max segment length cap.
> 
> Signed-off-by: Matt Porter <mporter@ti.com>
> ---
>  drivers/dma/edma.c |   27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
> index 82c8672..fc4b9db 100644
> --- a/drivers/dma/edma.c
> +++ b/drivers/dma/edma.c
> @@ -70,6 +70,7 @@ struct edma_chan {
>  	bool				alloced;
>  	int				slot[EDMA_MAX_SLOTS];
>  	struct dma_slave_config		cfg;
> +	struct dmaengine_chan_caps	caps;
>  };
>  
>  struct edma_cc {
> @@ -462,6 +463,28 @@ static void edma_issue_pending(struct dma_chan *chan)
>  	spin_unlock_irqrestore(&echan->vchan.lock, flags);
>  }
>  
> +static struct dmaengine_chan_caps
> +*edma_get_channel_caps(struct dma_chan *chan, enum dma_transfer_direction dir)
> +{
> +	struct edma_chan *echan;
> +	enum dma_slave_buswidth width = 0;
> +	u32 burst = 0;
> +
> +	if (chan) {
I think this check is redundant.
> +		echan = to_edma_chan(chan);
> +		if (dir == DMA_MEM_TO_DEV) {
> +			width = echan->cfg.dst_addr_width;
> +			burst = echan->cfg.dst_maxburst;
Per you API example burst and width is not set yet, so this doesn't make sense
> +		} else if (dir == DMA_DEV_TO_MEM) {
> +			width = echan->cfg.src_addr_width;
> +			burst = echan->cfg.src_maxburst;
> +		}
> +		echan->caps.seg_len = (SZ_64K - 1) * width * burst;
Also the defination of API is max, above computation doesn't make sense to me!

--
~Vinod

Re: [PATCH v2 0/3] dmaengine: add per channel capabilities api

[Matt Porter]

On Sun, Jan 20, 2013 at 12:37:34PM +0000, Vinod Koul wrote:
> On Thu, Jan 10, 2013 at 02:07:03PM -0500, Matt Porter wrote:
> > The call is implemented as follows:
> > 
> > 	struct dmaengine_chan_caps
> > 	*dma_get_channel_caps(struct dma_chan *chan,
> > 			      enum dma_transfer_direction dir);
> > 
> > The dma transfer direction parameter may appear a bit out of place
> > but it is necessary since the direction field in struct
> > dma_slave_config was deprecated. In some cases, EDMA for one, it
> > is necessary for the dmaengine driver to have the burst and address
> > width slave configuration parameters available in order to compute
> > the maximum segment size that can be handle. Due to this requirement,
> > the calling order of this api is as follows:
> Well you are passing direction as argument so even in EDMA it doesn't seem to
> help you as you seem to need burst and width!. So why do you even need the
> direction to compute the capablities

Yes, I need burst and width, but they are dependent on direction (dst vs
src, as stored in the slave channel config). Ok, so I think I know where
this is leading...the problem is probably that I made an implicit
dependency on burst and width here. The expectation in this
implementation is that dmaengine_slave_config() has already been called
and as a result, the dmaengine driver has either src_* or dst_*
attributes populated depending on the direction of the channel. Given
that, the call to dma_get_channel_caps() needs to provide the direction
in order for the driver to know which of src_*/dst_* attributes are
valid in order to do the max segment size calculation.

An alternative, since the slave driver is the one that provided the
info in the first place is:

	struct dmaengine_chan_caps
	*dma_get_channel_caps(struct dma_chan *chan,
			      enum dma_slave_buswidth addr_width,
			      u32 maxburst);

where the attributes required by the edma driver to find the max
segment size are now explicitly provided. This approach also removes
the ordering requirement of calling dmaengine_slave_config() first. Is
this what you had in mind?

-Matt

Re: [PATCH v2 1/3] dmaengine: add dma_get_channel_caps()

[Matt Porter]

On Sun, Jan 20, 2013 at 12:52:43PM +0000, Vinod Koul wrote:
> On Thu, Jan 10, 2013 at 02:07:04PM -0500, Matt Porter wrote:
> > +/* struct dmaengine_chan_caps - expose capability of a channel
> > + * Note: each channel can have same or different capabilities
> > + *
> > + * This primarily classifies capabilities into
> > + * a) APIs/ops supported
> > + * b) channel physical capabilities
> > + *
> > + * @cap_mask: api/ops capability (DMA_INTERRUPT and DMA_PRIVATE
> > + *	       are invalid api/ops and will never be set)
> > + * @seg_nr: maximum number of SG segments supported on a SG/SLAVE
> > + *	    channel (0 for no maximum or not a SG/SLAVE channel)
> > + * @seg_len: maximum length of SG segments supported on a SG/SLAVE
> > + *	     channel (0 for no maximum or not a SG/SLAVE channel)
> > + */
> > +struct dmaengine_chan_caps {
> > +	dma_cap_mask_t cap_mask;
> > +	int seg_nr;
> > +	int seg_len;
> > +};
> Now am really unclear why we would need direction as argument.

Best explanation is my reply to your comments on 0/3. In summary, the
direction allows the edma driver to select the src vs dst addr_width and
maxburst fields to be used to calculate the max segment size that can
be handled.

> Also, I would add the channel physical capablities like direction, widths,
> lengths etc supported.

Ok, I can take a stab at this...I didn't bother initially as I don't
have user for that info at this point. Though, I suppose we don't have
an immediate user for the cap_mask either.

> > +/**
> > + * dma_get_channel_caps - flush pending transactions to HW
> flush pending... ???

ugh, c&p fail...will fix.

> 
> > + * driver does not implement per channel capbilities then
> > + * NULL is returned.
> > + */
> > +static inline struct dmaengine_chan_caps
> > +*dma_get_channel_caps(struct dma_chan *chan, enum dma_transfer_direction dir)
> you need to add this for when CONFIG_DMA_ENGINE is not defined as well.

ok, will fix.

> > +{
> > +	if (chan->device->device_channel_caps)
> > +		return chan->device->device_channel_caps(chan, dir);
> > +	return NULL;
> > +}
> > +
> >  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);
> --
> ~Vinod

Re: [PATCH v2 2/3] dma: edma: add device_channel_caps() support

[Matt Porter]

On Sun, Jan 20, 2013 at 01:03:21PM +0000, Vinod Koul wrote:
> On Thu, Jan 10, 2013 at 02:07:05PM -0500, Matt Porter wrote:
> > Implement device_channel_caps().
> > 
> > EDMA has a finite set of PaRAM slots available for linking
> > a multi-segment SG transfer. In order to prevent any one
> > channel from consuming all PaRAM slots to fulfill a large SG
> > transfer, the driver reports a static per-channel max number
> > of SG segments it will handle.
> > 
> > The maximum size of SG segment is limited by the slave config
> > maxburst and addr_width for the channel in question. These values
> > are used from the current channel config to calculate and return
> > the max segment length cap.
> > 
> > Signed-off-by: Matt Porter <mporter@ti.com>
> > ---
> >  drivers/dma/edma.c |   27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
> > index 82c8672..fc4b9db 100644
> > --- a/drivers/dma/edma.c
> > +++ b/drivers/dma/edma.c
> > @@ -70,6 +70,7 @@ struct edma_chan {
> >  	bool				alloced;
> >  	int				slot[EDMA_MAX_SLOTS];
> >  	struct dma_slave_config		cfg;
> > +	struct dmaengine_chan_caps	caps;
> >  };
> >  
> >  struct edma_cc {
> > @@ -462,6 +463,28 @@ static void edma_issue_pending(struct dma_chan *chan)
> >  	spin_unlock_irqrestore(&echan->vchan.lock, flags);
> >  }
> >  
> > +static struct dmaengine_chan_caps
> > +*edma_get_channel_caps(struct dma_chan *chan, enum dma_transfer_direction dir)
> > +{
> > +	struct edma_chan *echan;
> > +	enum dma_slave_buswidth width = 0;
> > +	u32 burst = 0;
> > +
> > +	if (chan) {
> I think this check is redundant.

Yes, will remove.

> > +		echan = to_edma_chan(chan);
> > +		if (dir == DMA_MEM_TO_DEV) {
> > +			width = echan->cfg.dst_addr_width;
> > +			burst = echan->cfg.dst_maxburst;
> Per you API example burst and width is not set yet, so this doesn't make sense

The explanation in the cover letter mentions that dmaengine_slave_config() is
required to be called prior to dmaengine_get_channel_caps(). If we
switch to the alternative API, then that would go away including the
dependency on direction.

> > +		} else if (dir == DMA_DEV_TO_MEM) {
> > +			width = echan->cfg.src_addr_width;
> > +			burst = echan->cfg.src_maxburst;
> > +		}
> > +		echan->caps.seg_len = (SZ_64K - 1) * width * burst;
> Also the defination of API is max, above computation doesn't make sense to me!

Ok, so in this case, the slave driver has informed the dmaengine driver
that the max burst for the channel is foo. That's in units of
addr_width. On the EDMA DMAC, we program burst transfers by setting ACNT
to our per-transfer width (FIFO width in the slave SG case we are
covering here) then BCNT gets the maxburst setting. We then have a CCNT
field for EDMA that has a limit of SZ_64K-1 transfers. Thus, if a slave
driver tells us the maxburst for the channel is foo and our width is
bar, the maximum size of an SG segment in that configuration is:
(SZ_64K - 1) * bar * foo. 

-Matt

Re: [PATCH v2 0/3] dmaengine: add per channel capabilities api

[Vinod Koul]

On Sun, Jan 20, 2013 at 11:37:35AM -0500, Matt Porter wrote:
> On Sun, Jan 20, 2013 at 12:37:34PM +0000, Vinod Koul wrote:
> > On Thu, Jan 10, 2013 at 02:07:03PM -0500, Matt Porter wrote:
> > > The call is implemented as follows:
> > > 
> > > 	struct dmaengine_chan_caps
> > > 	*dma_get_channel_caps(struct dma_chan *chan,
> > > 			      enum dma_transfer_direction dir);
> > > 
> > > The dma transfer direction parameter may appear a bit out of place
> > > but it is necessary since the direction field in struct
> > > dma_slave_config was deprecated. In some cases, EDMA for one, it
> > > is necessary for the dmaengine driver to have the burst and address
> > > width slave configuration parameters available in order to compute
> > > the maximum segment size that can be handle. Due to this requirement,
> > > the calling order of this api is as follows:
> > Well you are passing direction as argument so even in EDMA it doesn't seem to
> > help you as you seem to need burst and width!. So why do you even need the
> > direction to compute the capablities
> 
> Yes, I need burst and width, but they are dependent on direction (dst vs
> src, as stored in the slave channel config). Ok, so I think I know where
> this is leading...the problem is probably that I made an implicit
> dependency on burst and width here. The expectation in this
And also due to wrong documentation. This is what you have put up the flow as:
Due to this requirement,
the calling order of this api is as follows:

1. Allocate a DMA slave channel
1a. [Optionally] Get channel capabilities
2. Set slave and controller specific parameters
3. Get a descriptor for transaction
4. Submit the transaction
5. Issue pending requests and wait for callback notification

Now when we query capablities, slave parameters _are_not_set_.
So seems like you have thought something and written something else!

Which brings me to the point on what are we trying to query:
a) API capability, dont need slave parameters for that
b) Sg segment length and numbers: Well these are capabilities, so it tells
you what is the maximum I can do. IMO it doesn't make sense to tie it down to
burst, width etc. For that configuration you are checking maximum. What this
needs to return is what is the maximum length it supports and maximum number of
sg list the h/w can use. Also if you return your burst and width capablity, then
any client can easily find out what is the length byte value it can hold.

If you feel this computaion if client specific, though looking at doesnt make me
think so, you can add a callback for this computaion given the parameters.

--
~Vinod

Re: [PATCH v2 2/3] dma: edma: add device_channel_caps() support

[Vinod Koul]

On Sun, Jan 20, 2013 at 11:51:08AM -0500, Matt Porter wrote:
> The explanation in the cover letter mentions that dmaengine_slave_config() is
> required to be called prior to dmaengine_get_channel_caps(). If we
> switch to the alternative API, then that would go away including the
> dependency on direction.
Nope you got that wrong!

--
~Vinod

Re: [PATCH v2 0/3] dmaengine: add per channel capabilities api

[Matt Porter]

On Mon, Jan 21, 2013 at 03:15:23AM +0000, Vinod Koul wrote:
> On Sun, Jan 20, 2013 at 11:37:35AM -0500, Matt Porter wrote:
> > On Sun, Jan 20, 2013 at 12:37:34PM +0000, Vinod Koul wrote:
> > > On Thu, Jan 10, 2013 at 02:07:03PM -0500, Matt Porter wrote:
> > > > The call is implemented as follows:
> > > > 
> > > > 	struct dmaengine_chan_caps
> > > > 	*dma_get_channel_caps(struct dma_chan *chan,
> > > > 			      enum dma_transfer_direction dir);
> > > > 
> > > > The dma transfer direction parameter may appear a bit out of place
> > > > but it is necessary since the direction field in struct
> > > > dma_slave_config was deprecated. In some cases, EDMA for one, it
> > > > is necessary for the dmaengine driver to have the burst and address
> > > > width slave configuration parameters available in order to compute
> > > > the maximum segment size that can be handle. Due to this requirement,
> > > > the calling order of this api is as follows:
> > > Well you are passing direction as argument so even in EDMA it doesn't seem to
> > > help you as you seem to need burst and width!. So why do you even need the
> > > direction to compute the capablities
> > 
> > Yes, I need burst and width, but they are dependent on direction (dst vs
> > src, as stored in the slave channel config). Ok, so I think I know where
> > this is leading...the problem is probably that I made an implicit
> > dependency on burst and width here. The expectation in this
> And also due to wrong documentation. This is what you have put up the flow as:
> Due to this requirement,
> the calling order of this api is as follows:
> 
> 1. Allocate a DMA slave channel
> 1a. [Optionally] Get channel capabilities
> 2. Set slave and controller specific parameters
> 3. Get a descriptor for transaction
> 4. Submit the transaction
> 5. Issue pending requests and wait for callback notification
>
> Now when we query capablities, slave parameters _are_not_set_.
> So seems like you have thought something and written something else!

Agreed. My documentation is incorrect. My bad, it should have been
ordered 1, 2, 1a, ... as you've noted.

> Which brings me to the point on what are we trying to query:
> a) API capability, dont need slave parameters for that

agreed

> b) Sg segment length and numbers: Well these are capabilities, so it tells
> you what is the maximum I can do. IMO it doesn't make sense to tie it down to
> burst, width etc. For that configuration you are checking maximum. What this
> needs to return is what is the maximum length it supports and maximum number of
> sg list the h/w can use. Also if you return your burst and width capablity, then
> any client can easily find out what is the length byte value it can hold.
 
Ok, so I could report the max burst size to the client, but on EDMA it's
going to be SZ_64K-1, as that's the hw limit. Max addr width would also
be the same or capped at the maximum enum the current API support of 8
bytes.

This information is not useful to the client in my case. Only the client
knows its FIFO size, and thus the actual burst size it needs to request
as that is a value specific to the IP the client driver controls. So it
knows actual burst size and the width of that fifo transfer, because we
already support telling the dmaengine driver this info in the current
API.

But, the client driver has no idea what DMAC it's using under the API.
So, whereas if the omap_hsmmc driver is running on omap-dma, it can
handle any SG segment size, if the driver happens to be running on edma,
it needs to know the actual allowed max segment size for the slave
channel parameters that it has set. The theoretical max segment size
on EDMA is far larger than the actual maximum for a configured
slave channel. This is why (yes, I screwed up and the documentation had
the ordering wrong) I had the intention of dma_get_channel_caps() only
being valid *after* dma_slave_config() was called. This is, in fact,
the only point at which the edma driver has enough information to be
able to report a valid max number of segments back to a client driver.
Consider that on another channel, with burst size configured by a client
driver to a high value (e.g. twice as big of a FIFO), now the edma
driver has to report a small max segment size that can be used. The
client driver has no idea it's on an edma controller so it needs some
help from the dmaengine API to choose the optimal constraints when it
builds SG lists. Too large and the EDMA driver has to reject it, too
small and we lose performance and also run into the constraint edma has
on the total number of segments we can handle in one sg list.

> If you feel this computaion if client specific, though looking at doesnt make me
> think so, you can add a callback for this computaion given the parameters.

It's client-specific in the sense that the client peripheral is what
provides the address width and burst size constraint (based on the FIFO
integrated with that peripheral instantiation), but the abstracted
dmaengine peripheral is what provides the segment size limit (no limit
in the omap-dma case but a variable limit in the edma case).

Sounds like you prefer a separate API from dma_get_channel_caps() to
handle the case I'm describing here. Possibly a separate
dma_get_max_sg_seg_size() and dma_get_max_sg_seg_nr()?

-Matt

Re: [PATCH v2 2/3] dma: edma: add device_channel_caps() support

[Matt Porter]

On Mon, Jan 21, 2013 at 03:16:32AM +0000, Vinod Koul wrote:
> On Sun, Jan 20, 2013 at 11:51:08AM -0500, Matt Porter wrote:
> > The explanation in the cover letter mentions that dmaengine_slave_config() is
> > required to be called prior to dmaengine_get_channel_caps(). If we
> > switch to the alternative API, then that would go away including the
> > dependency on direction.
> Nope you got that wrong!

:) Yes, dropped the ball there, should have been for the api to make
sense as implemented:

1. Allocate a DMA slave channel
2. Set slave and controller specific parameters
2a. [Optionally] Get channel capabilities
3. Get a descriptor for transaction
4. Submit the transaction
5. Issue pending requests and wait for callback notification

FWIW, the implementation example in the davinci mmc client driver shows
proper use as in the correct documentation above.

-Matt

Re: [PATCH v2 0/3] dmaengine: add per channel capabilities api

[Vinod Koul]

On Mon, Jan 21, 2013 at 01:19:23PM -0500, Matt Porter wrote:
> > b) Sg segment length and numbers: Well these are capabilities, so it tells
> > you what is the maximum I can do. IMO it doesn't make sense to tie it down to
> > burst, width etc. For that configuration you are checking maximum. What this
> > needs to return is what is the maximum length it supports and maximum number of
> > sg list the h/w can use. Also if you return your burst and width capablity, then
> > any client can easily find out what is the length byte value it can hold.
>  
> Ok, so I could report the max burst size to the client, but on EDMA it's
> going to be SZ_64K-1, as that's the hw limit. Max addr width would also
> be the same or capped at the maximum enum the current API support of 8
> bytes.
Yes IMO you should report the h/w limit and let client deduce what can be done
with that h/w limit given the other parameters (burst, length etc...)
> 
> This information is not useful to the client in my case. Only the client
> knows its FIFO size, and thus the actual burst size it needs to request
> as that is a value specific to the IP the client driver controls. So it
> knows actual burst size and the width of that fifo transfer, because we
> already support telling the dmaengine driver this info in the current
> API.
Your current API way is not very generic. We need to make sure it useful on
other systems too. That is why it would make sense to query the DMA H/W
capablities and then use it with above properties to get various parameters used
for initiating a transfer, that way you dont need to set slave parameters

--
~Vinod

Re: [PATCH v2 0/3] dmaengine: add per channel capabilities api

[Matt Porter]

On Mon, Jan 28, 2013 at 10:06:03AM +0000, Vinod Koul wrote:
> On Mon, Jan 21, 2013 at 01:19:23PM -0500, Matt Porter wrote:
> > > b) Sg segment length and numbers: Well these are capabilities, so it tells
> > > you what is the maximum I can do. IMO it doesn't make sense to tie it down to
> > > burst, width etc. For that configuration you are checking maximum. What this
> > > needs to return is what is the maximum length it supports and maximum number of
> > > sg list the h/w can use. Also if you return your burst and width capablity, then
> > > any client can easily find out what is the length byte value it can hold.
> >  
> > Ok, so I could report the max burst size to the client, but on EDMA it's
> > going to be SZ_64K-1, as that's the hw limit. Max addr width would also
> > be the same or capped at the maximum enum the current API support of 8
> > bytes.
> Yes IMO you should report the h/w limit and let client deduce what can be done
> with that h/w limit given the other parameters (burst, length etc...)

Hrm, in a driver that runs on two different DMACs, we don't know which
one we are running on in order to determine if the information returned
is relevant to do a calculation. If omap_hsmmc.c is running on SDMA the
absolute max_seg_len may be fixed. On EDMA, that max values can be
returned but is invalid for purposes of computing the actual length of
sg segments that can be absorbed by the driver.

The client driver does know the burst and length, and could compute this
in an EDMA specific way, but we're talking now about in the client
driver is:

if (i_am_edma)
	max_seg_len = (SZ_64K-1) * max_burst * addr_width;
else /* sdma */
	max_seg_len = MY_FIXED_VALUE_FROM_CHANNEL_CAPS;

There's no way to deduce what to do in the client driver without having
the knowledge of being on an EDMA DMAC or not. The only thing to do with
fixed h/w caps is to report back a safe value. That would be SZ_64K-1
but would be very inefficient when the slave config is set for a large
FIFO and 32-bit address width and the actual size that can be
transferred if much larger.

> > 
> > This information is not useful to the client in my case. Only the client
> > knows its FIFO size, and thus the actual burst size it needs to request
> > as that is a value specific to the IP the client driver controls. So it
> > knows actual burst size and the width of that fifo transfer, because we
> > already support telling the dmaengine driver this info in the current
> > API.
> Your current API way is not very generic. We need to make sure it useful on
> other systems too. That is why it would make sense to query the DMA H/W
> capablities and then use it with above properties to get various parameters used
> for initiating a transfer, that way you dont need to set slave parameters
 
Ok, you want something then that takes max_burst and addr_width as
arguments and returns max_seg_len. Easy enough. That's the minimum
information the driver needs to report the value and will avoid having
to set the slave config first.

-Matt