[PATCH v2] dw_dmac: allocate dma descriptors from DMA_COHERENT memory

[PATCH v2] dw_dmac: allocate dma descriptors from DMA_COHERENT memory

[Andy Shevchenko]

Currently descriptors are allocated from normal cacheable memory and that slows
down filling the descriptors, as we need to call cache_coherency routines
afterwards. It would be better to allocate memory for these descriptors from
DMA_COHERENT memory. This would make code much cleaner too.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Since v1:
 - change commit log message as Viresh suggested
 - update indentation accordingly to Viresh comments

 drivers/dma/dw_dmac.c      |   81 ++++++++++----------------------------------
 drivers/dma/dw_dmac_regs.h |    1 +
 2 files changed, 18 insertions(+), 64 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 561bb9a..6915520 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -14,6 +14,7 @@
 #include <linux/delay.h>
 #include <linux/dmaengine.h>
 #include <linux/dma-mapping.h>
+#include <linux/dmapool.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
@@ -98,14 +99,6 @@ static inline unsigned int dwc_get_data_width(struct dma_chan *chan, int master)
 
 /*----------------------------------------------------------------------*/
 
-/*
- * Because we're not relying on writeback from the controller (it may not
- * even be configured into the core!) we don't need to use dma_pool.  These
- * descriptors -- and associated data -- are cacheable.  We do need to make
- * sure their dcache entries are written back before handing them off to
- * the controller, though.
- */
-
 static struct device *chan2dev(struct dma_chan *chan)
 {
 	return &chan->dev->device;
@@ -144,19 +137,6 @@ static struct dw_desc *dwc_desc_get(struct dw_dma_chan *dwc)
 	return ret;
 }
 
-static void dwc_sync_desc_for_cpu(struct dw_dma_chan *dwc, struct dw_desc *desc)
-{
-	struct dw_desc	*child;
-
-	list_for_each_entry(child, &desc->tx_list, desc_node)
-		dma_sync_single_for_cpu(chan2parent(&dwc->chan),
-				child->txd.phys, sizeof(child->lli),
-				DMA_TO_DEVICE);
-	dma_sync_single_for_cpu(chan2parent(&dwc->chan),
-			desc->txd.phys, sizeof(desc->lli),
-			DMA_TO_DEVICE);
-}
-
 /*
  * Move a descriptor, including any children, to the free list.
  * `desc' must not be on any lists.
@@ -168,8 +148,6 @@ static void dwc_desc_put(struct dw_dma_chan *dwc, struct dw_desc *desc)
 	if (desc) {
 		struct dw_desc *child;
 
-		dwc_sync_desc_for_cpu(dwc, desc);
-
 		spin_lock_irqsave(&dwc->lock, flags);
 		list_for_each_entry(child, &desc->tx_list, desc_node)
 			dev_vdbg(chan2dev(&dwc->chan),
@@ -342,8 +320,6 @@ dwc_descriptor_complete(struct dw_dma_chan *dwc, struct dw_desc *desc,
 		param = txd->callback_param;
 	}
 
-	dwc_sync_desc_for_cpu(dwc, desc);
-
 	/* async_tx_ack */
 	list_for_each_entry(child, &desc->tx_list, desc_node)
 		async_tx_ack(&child->txd);
@@ -777,25 +753,17 @@ dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
 			first = desc;
 		} else {
 			prev->lli.llp = desc->txd.phys;
-			dma_sync_single_for_device(chan2parent(chan),
-					prev->txd.phys, sizeof(prev->lli),
-					DMA_TO_DEVICE);
 			list_add_tail(&desc->desc_node,
 					&first->tx_list);
 		}
 		prev = desc;
 	}
 
-
 	if (flags & DMA_PREP_INTERRUPT)
 		/* Trigger interrupt after last block */
 		prev->lli.ctllo |= DWC_CTLL_INT_EN;
 
 	prev->lli.llp = 0;
-	dma_sync_single_for_device(chan2parent(chan),
-			prev->txd.phys, sizeof(prev->lli),
-			DMA_TO_DEVICE);
-
 	first->txd.flags = flags;
 	first->len = len;
 
@@ -883,10 +851,6 @@ slave_sg_todev_fill_desc:
 				first = desc;
 			} else {
 				prev->lli.llp = desc->txd.phys;
-				dma_sync_single_for_device(chan2parent(chan),
-						prev->txd.phys,
-						sizeof(prev->lli),
-						DMA_TO_DEVICE);
 				list_add_tail(&desc->desc_node,
 						&first->tx_list);
 			}
@@ -945,10 +909,6 @@ slave_sg_fromdev_fill_desc:
 				first = desc;
 			} else {
 				prev->lli.llp = desc->txd.phys;
-				dma_sync_single_for_device(chan2parent(chan),
-						prev->txd.phys,
-						sizeof(prev->lli),
-						DMA_TO_DEVICE);
 				list_add_tail(&desc->desc_node,
 						&first->tx_list);
 			}
@@ -968,10 +928,6 @@ slave_sg_fromdev_fill_desc:
 		prev->lli.ctllo |= DWC_CTLL_INT_EN;
 
 	prev->lli.llp = 0;
-	dma_sync_single_for_device(chan2parent(chan),
-			prev->txd.phys, sizeof(prev->lli),
-			DMA_TO_DEVICE);
-
 	first->len = total_len;
 
 	return &first->txd;
@@ -1125,7 +1081,6 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
 	struct dw_desc		*desc;
 	int			i;
 	unsigned long		flags;
-	int			ret;
 
 	dev_vdbg(chan2dev(chan), "%s\n", __func__);
 
@@ -1146,21 +1101,21 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
 	spin_lock_irqsave(&dwc->lock, flags);
 	i = dwc->descs_allocated;
 	while (dwc->descs_allocated < NR_DESCS_PER_CHANNEL) {
+		dma_addr_t phys;
+
 		spin_unlock_irqrestore(&dwc->lock, flags);
 
-		desc = kzalloc(sizeof(struct dw_desc), GFP_KERNEL);
+		desc = dma_pool_alloc(dw->desc_pool, GFP_ATOMIC, &phys);
 		if (!desc)
 			goto err_desc_alloc;
 
+		memset(desc, 0, sizeof(struct dw_desc));
+
 		INIT_LIST_HEAD(&desc->tx_list);
 		dma_async_tx_descriptor_init(&desc->txd, chan);
 		desc->txd.tx_submit = dwc_tx_submit;
 		desc->txd.flags = DMA_CTRL_ACK;
-		desc->txd.phys = dma_map_single(chan2parent(chan), &desc->lli,
-				sizeof(desc->lli), DMA_TO_DEVICE);
-		ret = dma_mapping_error(chan2parent(chan), desc->txd.phys);
-		if (ret)
-			goto err_desc_alloc;
+		desc->txd.phys = phys;
 
 		dwc_desc_put(dwc, desc);
 
@@ -1175,8 +1130,6 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
 	return i;
 
 err_desc_alloc:
-	kfree(desc);
-
 	dev_info(chan2dev(chan), "only allocated %d descriptors\n", i);
 
 	return i;
@@ -1211,9 +1164,7 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
 
 	list_for_each_entry_safe(desc, _desc, &list, desc_node) {
 		dev_vdbg(chan2dev(chan), "  freeing descriptor %p\n", desc);
-		dma_unmap_single(chan2parent(chan), desc->txd.phys,
-				sizeof(desc->lli), DMA_TO_DEVICE);
-		kfree(desc);
+		dma_pool_free(dw->desc_pool, desc, desc->txd.phys);
 	}
 
 	dev_vdbg(chan2dev(chan), "%s: done\n", __func__);
@@ -1458,20 +1409,14 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
 		desc->lli.ctlhi = (period_len >> reg_width);
 		cdesc->desc[i] = desc;
 
-		if (last) {
+		if (last)
 			last->lli.llp = desc->txd.phys;
-			dma_sync_single_for_device(chan2parent(chan),
-					last->txd.phys, sizeof(last->lli),
-					DMA_TO_DEVICE);
-		}
 
 		last = desc;
 	}
 
 	/* lets make a cyclic list */
 	last->lli.llp = cdesc->desc[0]->txd.phys;
-	dma_sync_single_for_device(chan2parent(chan), last->txd.phys,
-			sizeof(last->lli), DMA_TO_DEVICE);
 
 	dev_dbg(chan2dev(&dwc->chan), "cyclic prepared buf 0x%llx len %zu "
 			"period %zu periods %d\n", (unsigned long long)buf_addr,
@@ -1729,6 +1674,14 @@ static int dw_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, dw);
 
+	/* create a pool of consistent memory blocks for hardware descriptors */
+	dw->desc_pool = dmam_pool_create("dw_dmac_desc_pool", &pdev->dev,
+					 sizeof(struct dw_desc), 4, 0);
+	if (!dw->desc_pool) {
+		dev_err(&pdev->dev, "No memory for descriptors dma pool\n");
+		return -ENOMEM;
+	}
+
 	tasklet_init(&dw->tasklet, dw_dma_tasklet, (unsigned long)dw);
 
 	INIT_LIST_HEAD(&dw->dma.channels);
diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw_dmac_regs.h
index 577f2dd..fef296d 100644
--- a/drivers/dma/dw_dmac_regs.h
+++ b/drivers/dma/dw_dmac_regs.h
@@ -235,6 +235,7 @@ static inline struct dw_dma_chan *to_dw_dma_chan(struct dma_chan *chan)
 struct dw_dma {
 	struct dma_device	dma;
 	void __iomem		*regs;
+	struct dma_pool		*desc_pool;
 	struct tasklet_struct	tasklet;
 	struct clk		*clk;
 
-- 
1.7.10.4

[PATCH v3] dw_dmac: don't exceed AHB master number in dwc_get_data_width

[Andy Shevchenko]

The driver assumes that hardware has two AHB masters which might not be always
true. In such cases we must not exceed number of the AHB masters present in the
hardware. In the proposed scheme in this patch, we would choose the master with
highest possible number whenever we exceed max AHB masters.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Since v2:
 - update commit message accordingly to Viresh suggestion
Since v1:
 - use proper value of master in the CTL lo register as well

 drivers/dma/dw_dmac.c |   33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 30ff2a4..561bb9a 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -46,13 +46,29 @@ static inline unsigned int dwc_get_sms(struct dw_dma_slave *slave)
 	return slave ? slave->src_master : 1;
 }
 
+#define SRC_MASTER	0
+#define DST_MASTER	1
+
+static inline unsigned int dwc_get_master(struct dma_chan *chan, int master)
+{
+	struct dw_dma *dw = to_dw_dma(chan->device);
+	struct dw_dma_slave *dws = chan->private;
+	unsigned int m = 0;
+
+	if (master == SRC_MASTER)
+		m = dwc_get_sms(dws);
+	else if (master == DST_MASTER)
+		m = dwc_get_dms(dws);
+
+	return min_t(unsigned int, dw->nr_masters - 1, m);
+}
+
 #define DWC_DEFAULT_CTLLO(_chan) ({				\
-		struct dw_dma_slave *__slave = (_chan->private);	\
 		struct dw_dma_chan *_dwc = to_dw_dma_chan(_chan);	\
 		struct dma_slave_config	*_sconfig = &_dwc->dma_sconfig;	\
 		bool _is_slave = is_slave_direction(_dwc->direction);	\
-		int _dms = dwc_get_dms(__slave);		\
-		int _sms = dwc_get_sms(__slave);		\
+		int _dms = dwc_get_master(_chan, DST_MASTER);		\
+		int _sms = dwc_get_master(_chan, SRC_MASTER);		\
 		u8 _smsize = _is_slave ? _sconfig->src_maxburst :	\
 			DW_DMA_MSIZE_16;			\
 		u8 _dmsize = _is_slave ? _sconfig->dst_maxburst :	\
@@ -73,20 +89,11 @@ static inline unsigned int dwc_get_sms(struct dw_dma_slave *slave)
  */
 #define NR_DESCS_PER_CHANNEL	64
 
-#define SRC_MASTER	0
-#define DST_MASTER	1
-
 static inline unsigned int dwc_get_data_width(struct dma_chan *chan, int master)
 {
 	struct dw_dma *dw = to_dw_dma(chan->device);
-	struct dw_dma_slave *dws = chan->private;
 
-	if (master == SRC_MASTER)
-		return dw->data_width[dwc_get_sms(dws)];
-	else if (master == DST_MASTER)
-		return dw->data_width[dwc_get_dms(dws)];
-
-	return 0;
+	return dw->data_width[dwc_get_master(chan, master)];
 }
 
 /*----------------------------------------------------------------------*/
-- 
1.7.10.4

Re: [PATCH v3] dw_dmac: don't exceed AHB master number in dwc_get_data_width

[Viresh Kumar]

On Wed, Jan 16, 2013 at 7:18 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> +static inline unsigned int dwc_get_master(struct dma_chan *chan, int master)
> +{
> +       struct dw_dma *dw = to_dw_dma(chan->device);
> +       struct dw_dma_slave *dws = chan->private;
> +       unsigned int m = 0;

:(

Re: [PATCH v3] dw_dmac: don't exceed AHB master number in dwc_get_data_width

[Andy Shevchenko]

On Wed, 2013-01-16 at 19:37 +0530, Viresh Kumar wrote: 
> On Wed, Jan 16, 2013 at 7:18 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > +static inline unsigned int dwc_get_master(struct dma_chan *chan, int master)
> > +{
> > +       struct dw_dma *dw = to_dw_dma(chan->device);
> > +       struct dw_dma_slave *dws = chan->private;
> > +       unsigned int m = 0;
> 
> :(

There a protection of wrong master parameter.

In the code I did
if ()
m = ...
else if ()
m = ...

Otherwise it would be 0.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3] dw_dmac: don't exceed AHB master number in dwc_get_data_width

[Viresh Kumar]

On Wed, Jan 16, 2013 at 8:32 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> There a protection of wrong master parameter.
>
> In the code I did
> if ()
> m = ...
> else if ()
> m = ...
>
> Otherwise it would be 0.

That's what. I don't want the else if to be there. Make it else only.
We aren't supposed to pass master other than 0 or 1.

Re: [PATCH v2] dw_dmac: allocate dma descriptors from DMA_COHERENT memory

[Vinod Koul]

On Wed, Jan 16, 2013 at 03:48:50PM +0200, Andy Shevchenko wrote:
> Currently descriptors are allocated from normal cacheable memory and that slows
> down filling the descriptors, as we need to call cache_coherency routines
> afterwards. It would be better to allocate memory for these descriptors from
> DMA_COHERENT memory. This would make code much cleaner too.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Applied Thanks

--
~Vinod