[PATCH v4 0/3] dmaengine: add slave sg transfer limits api

[PATCH v4 0/3] dmaengine: add slave sg transfer limits api

[Matt Porter]

Changes since v3:
	- Change api name to dma_get_slave_sg_limits() to avoid
	  confusion with h/w caps which are static.

Changes since v2:
	- Change to a separate slave sg specific api. Drop the
	  generic per-channel capabilities api that is not used.

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_slave_sg_limits(), which
may be used by a client driver to get slave SG transfer limits for a
particular channel. At this time, these include the max number of
segments and max length of a segment that a channel can handle for a
SG transfer.

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

This is tested on the AM1808-EVM.

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

 drivers/dma/edma.c                        |   17 +++++++++++++
 drivers/mmc/host/davinci_mmc.c            |   37 ++++++++-------------------
 include/linux/dmaengine.h                 |   39 +++++++++++++++++++++++++++++
 include/linux/platform_data/mmc-davinci.h |    3 ---
 4 files changed, 66 insertions(+), 30 deletions(-)

-- 
1.7.9.5

[PATCH v4 1/3] dmaengine: add dma_get_slave_sg_limits()

[Matt Porter]

Add a dmaengine API to retrieve slave SG transfer limits.

The API is optionally implemented by dmaengine drivers and when
unimplemented will return a NULL pointer. A client driver using
this API provides the required dma channel, address width, and
burst size of the transfer. dma_get_slave_sg_limits() returns an
SG limits structure with the maximum number and size of SG segments
that the given channel can handle.

Signed-off-by: Matt Porter <mporter@ti.com>
---
 include/linux/dmaengine.h |   39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 91ac8da..a4a6aac 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -371,6 +371,18 @@ struct dma_slave_config {
 	unsigned int slave_id;
 };
 
+/* struct dma_slave_sg_limits - expose SG transfer limits of a channel
+ *
+ * @max_seg_nr: maximum number of SG segments supported on a SG/SLAVE
+ *	    channel (0 for no maximum or not a SG/SLAVE channel)
+ * @max_seg_len: maximum length of SG segments supported on a SG/SLAVE
+ *	     channel (0 for no maximum or not a SG/SLAVE channel)
+ */
+struct dma_slave_sg_limits {
+	u32 max_seg_nr;
+	u32 max_seg_len;
+};
+
 static inline const char *dma_chan_name(struct dma_chan *chan)
 {
 	return dev_name(&chan->dev->device);
@@ -534,6 +546,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_slave_sg_limits: return the slave SG capabilities
  */
 struct dma_device {
 
@@ -602,6 +615,9 @@ struct dma_device {
 					    dma_cookie_t cookie,
 					    struct dma_tx_state *txstate);
 	void (*device_issue_pending)(struct dma_chan *chan);
+	struct dma_slave_sg_limits *(*device_slave_sg_limits)(
+		struct dma_chan *chan, enum dma_slave_buswidth addr_width,
+		u32 maxburst);
 };
 
 static inline int dmaengine_device_control(struct dma_chan *chan,
@@ -963,6 +979,29 @@ dma_set_tx_state(struct dma_tx_state *st, dma_cookie_t last, dma_cookie_t used,
 	}
 }
 
+/**
+ * dma_get_slave_sg_limits - get DMAC SG transfer capabilities
+ * @chan: target DMA channel
+ * @addr_width: address width of the DMA transfer
+ * @maxburst: maximum DMA transfer burst size
+ *
+ * Get SG transfer capabilities for a specified channel. If the dmaengine
+ * driver does not implement SG transfer capabilities then NULL is
+ * returned.
+ */
+static inline struct dma_slave_sg_limits
+*dma_get_slave_sg_limits(struct dma_chan *chan,
+		       enum dma_slave_buswidth addr_width,
+		       u32 maxburst)
+{
+	if (chan->device->device_slave_sg_limits)
+		return chan->device->device_slave_sg_limits(chan,
+							  addr_width,
+							  maxburst);
+
+	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

Re: [PATCH v4 1/3] dmaengine: add dma_get_slave_sg_limits()

[Lars-Peter Clausen]

[...]
>   *	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_slave_sg_limits: return the slave SG capabilities
>   */
>  struct dma_device {
>  
> @@ -602,6 +615,9 @@ struct dma_device {
>  					    dma_cookie_t cookie,
>  					    struct dma_tx_state *txstate);
>  	void (*device_issue_pending)(struct dma_chan *chan);
> +	struct dma_slave_sg_limits *(*device_slave_sg_limits)(
> +		struct dma_chan *chan, enum dma_slave_buswidth addr_width,
> +		u32 maxburst);

In my opinion it is better to pass in a pointer to a dma_slave_sg_limits
struct and let the driver fill it. Instead of passing back a pointer to an
internal structure. It is kind of problematic because you never really know
when the user is done using the struct and you don't know when it is safe to
free or reuse it. E.g. in your implementation for the edma driver if the
function is called with different parameters for the same channel, the
previous result will also be overwritten.

- Lars

Re: [PATCH v4 1/3] dmaengine: add dma_get_slave_sg_limits()

[Vinod Koul]

On Wed, Mar 06, 2013 at 02:56:05PM -0500, Matt Porter wrote:
> Add a dmaengine API to retrieve slave SG transfer limits.
> 
> The API is optionally implemented by dmaengine drivers and when
> unimplemented will return a NULL pointer. A client driver using
> this API provides the required dma channel, address width, and
> burst size of the transfer. dma_get_slave_sg_limits() returns an
> SG limits structure with the maximum number and size of SG segments
> that the given channel can handle.
> 
> Signed-off-by: Matt Porter <mporter@ti.com>
> ---
> +static inline struct dma_slave_sg_limits
> +*dma_get_slave_sg_limits(struct dma_chan *chan,
> +		       enum dma_slave_buswidth addr_width,
> +		       u32 maxburst)
> +{
> +	if (chan->device->device_slave_sg_limits)
> +		return chan->device->device_slave_sg_limits(chan,
> +							  addr_width,
> +							  maxburst);
Hi Matt,

Sorry for delayed reply, this series was sent just before i went for vacation :)

I agree with Lars, that returning pointer maynot be good idea. So this bit needs
work. Also the readblity of above is bad by complying to 80char limit. I would
make it easier to read

--
~Vinod
> +
> +	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);
> 

Re: [PATCH v4 1/3] dmaengine: add dma_get_slave_sg_limits()

[Lars-Peter Clausen]

On 03/06/2013 08:56 PM, Matt Porter wrote:
> Add a dmaengine API to retrieve slave SG transfer limits.
> 
> The API is optionally implemented by dmaengine drivers and when
> unimplemented will return a NULL pointer. A client driver using
> this API provides the required dma channel, address width, and
> burst size of the transfer. dma_get_slave_sg_limits() returns an
> SG limits structure with the maximum number and size of SG segments
> that the given channel can handle.


Hi Matt,

Are you still working on this patchset? Or do you mind if I pick it up, make
the discussed changes and resubmit it?

Thanks,
- Lars

[PATCH v4 2/3] dma: edma: add device_slave_sg_limits() support

[Matt Porter]

Implement device_slave_sg_limits().

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 an SG segment is limited by the addr_width
and maxburst of a given transfer request. These values are
provided by the client driver and used to calculate and return
the maximum segment length.

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

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index cd7e328..42373bf 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 dma_slave_sg_limits	sg_limits;
 };
 
 struct edma_cc {
@@ -462,6 +463,20 @@ static void edma_issue_pending(struct dma_chan *chan)
 	spin_unlock_irqrestore(&echan->vchan.lock, flags);
 }
 
+static struct dma_slave_sg_limits
+*edma_get_slave_sg_limits(struct dma_chan *chan,
+			enum dma_slave_buswidth addr_width,
+			u32 maxburst)
+{
+	struct edma_chan *echan;
+
+	echan = to_edma_chan(chan);
+	echan->sg_limits.max_seg_len =
+		(SZ_64K - 1) * addr_width * maxburst;
+
+	return &echan->sg_limits;
+}
+
 static size_t edma_desc_size(struct edma_desc *edesc)
 {
 	int i;
@@ -521,6 +536,7 @@ 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;
+		echan->sg_limits.max_seg_nr = MAX_NR_SG;
 
 		vchan_init(&echan->vchan, dma);
 
@@ -537,6 +553,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_slave_sg_limits = edma_get_slave_sg_limits;
 	dma->device_tx_status = edma_tx_status;
 	dma->device_control = edma_control;
 	dma->dev = dev;
-- 
1.7.9.5

[PATCH v4 3/3] mmc: davinci: get SG segment limits with dma_get_slave_sg_limits()

[Matt Porter]

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

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

diff --git a/drivers/mmc/host/davinci_mmc.c b/drivers/mmc/host/davinci_mmc.c
index 2063677..a98b5bc 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
@@ -1165,6 +1151,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 dma_slave_sg_limits *dma_sg_limits;
 
 	/* REVISIT:  when we're fully converted, fail if pdata is NULL */
 
@@ -1214,12 +1201,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 +1229,16 @@ 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;
+	/* Just check one channel for the DMA SG limits */
+	dma_sg_limits = dma_get_slave_sg_limits(
+				host->dma_tx,
+				DMA_SLAVE_BUSWIDTH_4_BYTES,
+				rw_threshold / DMA_SLAVE_BUSWIDTH_4_BYTES);
 
-	/* EDMA limit per hw segment (one or two MBytes) */
-	mmc->max_seg_size	= MAX_CCNT * rw_threshold;
+	if (dma_sg_limits) {
+		mmc->max_segs = dma_sg_limits->max_seg_nr;
+		mmc->max_seg_size = dma_sg_limits->max_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 v4 0/3] dmaengine: add slave sg transfer limits api

[Vinod Koul]

On Wed, Mar 06, 2013 at 02:56:04PM -0500, Matt Porter wrote:
> Changes since v3:
> 	- Change api name to dma_get_slave_sg_limits() to avoid
> 	  confusion with h/w caps which are static.
> 
> Changes since v2:
> 	- Change to a separate slave sg specific api. Drop the
> 	  generic per-channel capabilities api that is not used.
> 
> 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_slave_sg_limits(), which
> may be used by a client driver to get slave SG transfer limits for a
> particular channel. At this time, these include the max number of
> segments and max length of a segment that a channel can handle for a
> SG transfer.
Looks fine, should be ready for merge once we fix the API.
Also I was under the impression that you will add another API to calculate the
limits, the stuff which you were doing in caps API earlier.

--
~Vinod
> 
> Along with the API implementation, this series implements the backend
> device_slave_sg_limits() in the EDMA DMA Engine driver and converts the
> davinci_mmc driver to use dma_get_slave_sg_limits() to replace hardcoded
> limits.
> 
> This is tested on the AM1808-EVM.
> 
> Matt Porter (3):
>   dmaengine: add dma_get_slave_sg_limits()
>   dma: edma: add device_slave_sg_limits() support
>   mmc: davinci: get SG segment limits with dma_get_slave_sg_limits()
> 
>  drivers/dma/edma.c                        |   17 +++++++++++++
>  drivers/mmc/host/davinci_mmc.c            |   37 ++++++++-------------------
>  include/linux/dmaengine.h                 |   39 +++++++++++++++++++++++++++++
>  include/linux/platform_data/mmc-davinci.h |    3 ---
>  4 files changed, 66 insertions(+), 30 deletions(-)
> 
> -- 
> 1.7.9.5
>