To use DMA generic APIs for Samsung DMA

Signed-off-by Boojin Kim <[email protected]>

---
 drivers/dma/pl330.c |   75 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 73 insertions(+), 2 deletions(-)

drivers/dma/pl330.c +70 -2

index 6abe1ec..b7ecf47 100644
@@ -17,6 +17,7 @@
 #include <linux/interrupt.h>
 #include <linux/amba/bus.h>
 #include <linux/amba/pl330.h>
+#include <linux/pm_runtime.h>
 
 #define NR_DEFAULT_DESC	16
 

@@ -83,6 +84,8 @@ struct dma_pl330_dmac {
 
 	/* Peripheral channels connected to this DMAC */
 	struct dma_pl330_chan peripherals[0]; /* keep at end */
+
+	struct clk *clk;
 };
 
 struct dma_pl330_desc {

@@ -696,6 +699,30 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 		goto probe_err1;
 	}
 
+	pdmac->clk = clk_get(&adev->dev, "dma");
+	if (IS_ERR(pdmac->clk)) {
+		dev_err(&adev->dev, "Cannot get operation clock.\n");
+		ret = -EINVAL;
+		goto probe_err1;
+	}
+
+	amba_set_drvdata(adev, pdmac);
+
+#ifdef CONFIG_PM_RUNTIME
+	/* to use the runtime PM helper functions */
+	pm_runtime_enable(&adev->dev);
+
+	/* enable the power domain */
+	if (pm_runtime_get_sync(&adev->dev)) {
+		dev_err(&adev->dev, "failed to get runtime pm\n");
+		ret = -ENODEV;
+		goto probe_err1;
+	}
+#else
+	/* enable dma clk */
+	clk_enable(pdmac->clk);
+#endif
+
 	irq = adev->irq[0];
 	ret = request_irq(irq, pl330_irq_handler, 0,
 			dev_name(&adev->dev), pi);

@@ -763,8 +790,6 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 		goto probe_err4;
 	}
 
-	amba_set_drvdata(adev, pdmac);
-
 	dev_info(&adev->dev,
 		"Loaded driver for PL330 DMAC-%d\n", adev->periphid);
 	dev_info(&adev->dev,

@@ -825,6 +850,13 @@ static int __devexit pl330_remove(struct amba_device *adev)
 	res = &adev->res;
 	release_mem_region(res->start, resource_size(res));
 
+#ifdef CONFIG_PM_RUNTIME
+	pm_runtime_put(&adev->dev);
+	pm_runtime_disable(&adev->dev);
+#else
+	clk_disable(pdmac->clk);
+#endif
+
 	kfree(pdmac);
 
 	return 0;

@@ -838,10 +870,49 @@ static struct amba_id pl330_ids[] = {
 	{ 0, 0 },
 };
 
+#ifdef CONFIG_PM_RUNTIME
+static int pl330_runtime_suspend(struct device *dev)
+{
+	struct dma_pl330_dmac *pdmac = dev_get_drvdata(dev);
+
+	if (!pdmac) {
+		dev_err(dev, "failed to get dmac\n");
+		return -ENODEV;
+	}
+
+	clk_disable(pdmac->clk);
+
+	return 0;
+}
+
+static int pl330_runtime_resume(struct device *dev)
+{
+	struct dma_pl330_dmac *pdmac = dev_get_drvdata(dev);
+
+	if (!pdmac) {
+		dev_err(dev, "failed to get dmac\n");
+		return -ENODEV;
+	}
+
+	clk_enable(pdmac->clk);
+
+	return 0;
+}
+#else
+#define pl330_runtime_suspend	NULL
+#define pl330_runtime_resume	NULL
+#endif /* CONFIG_PM_RUNTIME */
+
+static const struct dev_pm_ops pl330_pm_ops = {
+	.runtime_suspend = pl330_runtime_suspend,
+	.runtime_resume = pl330_runtime_resume,

drivers/dma/pl330.c +4 -11

@@ -748,17 +747,19 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 ? ? ? ? ? ? ? ?struct dma_pl330_peri *peri = &pdat->peri[i];
 ? ? ? ? ? ? ? ?pch = &pdmac->peripherals[i];
 
- ? ? ? ? ? ? ? switch (peri->rqtype) {
- ? ? ? ? ? ? ? case MEMTOMEM:
- ? ? ? ? ? ? ? ? ? ? ? dma_cap_set(DMA_MEMCPY, pd->cap_mask);
- ? ? ? ? ? ? ? ? ? ? ? break;
- ? ? ? ? ? ? ? case MEMTODEV:
- ? ? ? ? ? ? ? case DEVTOMEM:
- ? ? ? ? ? ? ? ? ? ? ? dma_cap_set(DMA_SLAVE, pd->cap_mask);
- ? ? ? ? ? ? ? ? ? ? ? break;
- ? ? ? ? ? ? ? default:
- ? ? ? ? ? ? ? ? ? ? ? dev_err(&adev->dev, "DEVTODEV Not Supported\n");
- ? ? ? ? ? ? ? ? ? ? ? continue;
+ ? ? ? ? ? ? ? if (peri) {
+ ? ? ? ? ? ? ? ? ? ? ? switch (peri->rqtype) {
+ ? ? ? ? ? ? ? ? ? ? ? case MEMTOMEM:
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? dma_cap_set(DMA_MEMCPY, pd->cap_mask);

-----Original Message-----
From: Jassi Brar [mailto:jassisinghbrar at gmail.com]
Sent: Monday, July 25, 2011 5:58 PM
To: Boojin Kim
Cc: linux-arm-kernel at lists.infradead.org; linux-samsung-soc at vger.kernel.org; Vinod Koul; Dan Williams; Kukjin Kim; Grant Likely; Mark Brown
Subject: Re: [PATCH V4 02/14] DMA: PL330: Update PL330 DMA API driver

drivers/dma/pl330.c static struct dma_pl330_desc *pl330_get_desc(struct

Quoted 5 lines · Boojin Kim

drivers/dma/pl330.c +0 -0

index b7ecf47..586ab39 100644
@@ -455,7 +455,7 @@ static struct dma_pl330_desc *pl330_get_desc(struct

dma_pl330_chan *pch)

drivers/dma/pl330.c static struct dma_pl330_desc *pl330_get_desc(struct dma_pl330_chan *pch) · line 458

Quoted 5 lines

drivers/dma/pl330.c +1 -1

       async_tx_ack(&desc->txd);
 
       desc->req.rqtype = peri->rqtype;
-       desc->req.peri = peri->peri_id;
+       desc->req.peri = pch->chan.chan_id;

Btw, I plan to revert this if my patch is accepted that proposes to free chan_id solely for use by DMAC drivers(which we should employ to pass paltform assigned channel ID to client drivers)

drivers/dma/pl330.c pl330_probe(struct amba_device *adev, const struct

Quoted 1 line

drivers/dma/pl330.c +0 -0

@@ -748,17 +747,19 @@ pl330_probe(struct amba_device *adev, const struct

amba_id *id)

Quoted 15 lines

drivers/dma/pl330.c +1 -11

               struct dma_pl330_peri *peri = &pdat->peri[i];
               pch = &pdmac->peripherals[i];
 
-               switch (peri->rqtype) {
-               case MEMTOMEM:
-                       dma_cap_set(DMA_MEMCPY, pd->cap_mask);
-                       break;
-               case MEMTODEV:
-               case DEVTOMEM:
-                       dma_cap_set(DMA_SLAVE, pd->cap_mask);
-                       break;
-               default:
-                       dev_err(&adev->dev, "DEVTODEV Not Supported\n");
-                       continue;
+               if (peri) {

drivers/dma/pl330.c pl330_probe(struct amba_device *adev, const struct amba_id *id)

Quoted 10 lines

drivers/dma/pl330.c +10 -0

+                       switch (peri->rqtype) {
+                       case MEMTOMEM:
+                               dma_cap_set(DMA_MEMCPY, pd->cap_mask);
+                               break;
+                       case MEMTODEV:
+                       case DEVTOMEM:
+                               dma_cap_set(DMA_SLAVE, pd->cap_mask);
+                               break;
+                       default:
+                               dev_err(&adev->dev, "DEVTODEV Not

Supported\n");

drivers/dma/pl330.c pl330_probe(struct amba_device *adev, const struct amba_id *id) · line 763

Quoted 3 lines

drivers/dma/pl330.c +2 -0

+                               continue;
+                       }
               }

If you pass platform data correctly, you don't need to check against NULL peri.

This patch adds support DMA_SLAVE_CONFIG command.

Signed-off-by Boojin Kim <[email protected]>

---
 drivers/dma/pl330.c |   53 +++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 39 insertions(+), 14 deletions(-)

drivers/dma/pl330.c +18 -14

index 586ab39..880f010 100644
@@ -256,25 +256,50 @@ static int pl330_alloc_chan_resources(struct dma_chan *chan)
 static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned long arg)
 {
 	struct dma_pl330_chan *pch = to_pchan(chan);
-	struct dma_pl330_desc *desc;
+	struct dma_pl330_desc *desc, *_dt;
 	unsigned long flags;
+	struct dma_pl330_dmac *pdmac = pch->dmac;
+	struct dma_slave_config *slave_config;
+	struct dma_pl330_peri *peri;
+	LIST_HEAD(list);
 
-	/* Only supports DMA_TERMINATE_ALL */
-	if (cmd != DMA_TERMINATE_ALL)
-		return -ENXIO;
-
-	spin_lock_irqsave(&pch->lock, flags);
-
-	/* FLUSH the PL330 Channel thread */
-	pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
+	switch (cmd) {
+	case DMA_TERMINATE_ALL:
+		spin_lock_irqsave(&pch->lock, flags);
 
-	/* Mark all desc done */
-	list_for_each_entry(desc, &pch->work_list, node)
-		desc->status = DONE;
+		/* FLUSH the PL330 Channel thread */
+		pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
 
-	spin_unlock_irqrestore(&pch->lock, flags);
+		/* Mark all desc done */
+		list_for_each_entry_safe(desc, _dt, &pch->work_list , node) {
+			desc->status = DONE;
+			pch->completed = desc->txd.cookie;
+			list_move_tail(&desc->node, &list);
+		}
 
-	pl330_tasklet((unsigned long) pch);
+		list_splice_tail_init(&list, &pdmac->desc_pool);
+		spin_unlock_irqrestore(&pch->lock, flags);

drivers/dma/pl330.c +6 -0

+		}
+		break;
+	default:
+		dev_err(pch->dmac->pif.dev, "Not supported command.\n");
+		return -ENXIO;
+	}
 
 	return 0;
 }

Sent: Monday, July 25, 2011 7:27 PM
To: Boojin Kim
Cc: vinod.koul at intel.com; linux-arm-kernel at lists.infradead.org; linux- samsung-soc at vger.kernel.org; Kukjin Kim; Jassi Brar; Grant Likely; Mark Brown; Dan Williams
Subject: Re: [PATCH V4 03/14] DMA: PL330: Support DMA_SLAVE_CONFIG command

Quoted 5 lines · Boojin Kim

This patch adds support DMA_SLAVE_CONFIG command.

Signed-off-by Boojin Kim <[email protected]>

---
 drivers/dma/pl330.c |   53 +++++++++++++++++++++++++++++++++++++----

---------

drivers/dma/pl330.c static int pl330_alloc_chan_resources(struct

Quoted 7 lines

 1 files changed, 39 insertions(+), 14 deletions(-)

drivers/dma/pl330.c +0 -0

index 586ab39..880f010 100644
@@ -256,25 +256,50 @@ static int pl330_alloc_chan_resources(struct

dma_chan *chan)

drivers/dma/pl330.c static int pl330_alloc_chan_resources(struct · line 256

Quoted 1 line

drivers/dma/pl330.c +0 -0

 static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd

cmd, unsigned long arg)

Quoted 21 lines

drivers/dma/pl330.c +8 -9

 {
 	struct dma_pl330_chan *pch = to_pchan(chan);
-	struct dma_pl330_desc *desc;
+	struct dma_pl330_desc *desc, *_dt;
 	unsigned long flags;
+	struct dma_pl330_dmac *pdmac = pch->dmac;
+	struct dma_slave_config *slave_config;
+	struct dma_pl330_peri *peri;
+	LIST_HEAD(list);
 
-	/* Only supports DMA_TERMINATE_ALL */
-	if (cmd != DMA_TERMINATE_ALL)
-		return -ENXIO;
-
-	spin_lock_irqsave(&pch->lock, flags);
-
-	/* FLUSH the PL330 Channel thread */
-	pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
+	switch (cmd) {
+	case DMA_TERMINATE_ALL:
+		spin_lock_irqsave(&pch->lock, flags);

drivers/dma/pl330.c static int pl330_alloc_chan_resources(struct dma_chan *chan)

Quoted 10 lines

drivers/dma/pl330.c +4 -4

 
-	/* Mark all desc done */
-	list_for_each_entry(desc, &pch->work_list, node)
-		desc->status = DONE;
+		/* FLUSH the PL330 Channel thread */
+		pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
 
-	spin_unlock_irqrestore(&pch->lock, flags);
+		/* Mark all desc done */
+		list_for_each_entry_safe(desc, _dt, &pch->work_list , node)

{

Quoted 5 lines

drivers/dma/pl330.c +4 -0

+			desc->status = DONE;
+			pch->completed = desc->txd.cookie;
+			list_move_tail(&desc->node, &list);
+		}
 

Quoted 4 lines

drivers/dma/pl330.c +3 -1

-	pl330_tasklet((unsigned long) pch);
+		list_splice_tail_init(&list, &pdmac->desc_pool);
+		spin_unlock_irqrestore(&pch->lock, flags);
+		break;

drivers/dma/pl330.c static int pl330_alloc_chan_resources(struct dma_chan *chan)

Quoted 10 lines

drivers/dma/pl330.c +9 -0

+	case DMA_SLAVE_CONFIG:
+		slave_config = (struct dma_slave_config *)arg;
+		peri = pch->chan.private;
+
+		if (slave_config->direction == DMA_TO_DEVICE) {
+			if (slave_config->dst_addr)
+				peri->fifo_addr = slave_config->dst_addr;
+			if (slave_config->dst_addr_width)
+				peri->burst_sz = __ffs(slave_config-
 dst_addr_width);

This doesn't sound right, why can't you use xxx_maxburst field instead?

drivers/dma/pl330.c static int pl330_alloc_chan_resources(struct dma_chan *chan)

Quoted 7 lines

drivers/dma/pl330.c +5 -0

+		} else if (slave_config->direction == DMA_FROM_DEVICE) {
+			if (slave_config->src_addr)
+				peri->fifo_addr = slave_config->src_addr;
+			if (slave_config->src_addr_width)
+				peri->burst_sz = __ffs(slave_config-
 src_addr_width);

ditto...

Quoted 9 lines

drivers/dma/pl330.c +6 -0

+		}
+		break;
+	default:
+		dev_err(pch->dmac->pif.dev, "Not supported command.\n");
+		return -ENXIO;
+	}
 

return 0;
}

--
~Vinod Koul
Intel Corp.

Vinod Koul Wrote:

Quoted 62 lines

Sent: Monday, July 25, 2011 7:27 PM
To: Boojin Kim
Cc: vinod.koul at intel.com; linux-arm-kernel at lists.infradead.org; linux- samsung-soc at vger.kernel.org; Kukjin Kim; Jassi Brar; Grant Likely; Mark Brown; Dan Williams
Subject: Re: [PATCH V4 03/14] DMA: PL330: Support DMA_SLAVE_CONFIG command

Quoted 5 lines · Boojin Kim

This patch adds support DMA_SLAVE_CONFIG command.

Signed-off-by Boojin Kim <[email protected]>

---
 drivers/dma/pl330.c |   53 +++++++++++++++++++++++++++++++++++++----

---------

drivers/dma/pl330.c static int pl330_alloc_chan_resources(struct

Quoted 7 lines

 1 files changed, 39 insertions(+), 14 deletions(-)

drivers/dma/pl330.c +0 -0

index 586ab39..880f010 100644
@@ -256,25 +256,50 @@ static int pl330_alloc_chan_resources(struct

dma_chan *chan)

drivers/dma/pl330.c static int pl330_alloc_chan_resources(struct · line 256

Quoted 1 line

drivers/dma/pl330.c +0 -0

 static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd

cmd, unsigned long arg)

Quoted 21 lines

drivers/dma/pl330.c +8 -9

 {
 	struct dma_pl330_chan *pch = to_pchan(chan);
-	struct dma_pl330_desc *desc;
+	struct dma_pl330_desc *desc, *_dt;
 	unsigned long flags;
+	struct dma_pl330_dmac *pdmac = pch->dmac;
+	struct dma_slave_config *slave_config;
+	struct dma_pl330_peri *peri;
+	LIST_HEAD(list);
 
-	/* Only supports DMA_TERMINATE_ALL */
-	if (cmd != DMA_TERMINATE_ALL)
-		return -ENXIO;
-
-	spin_lock_irqsave(&pch->lock, flags);
-
-	/* FLUSH the PL330 Channel thread */
-	pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
+	switch (cmd) {
+	case DMA_TERMINATE_ALL:
+		spin_lock_irqsave(&pch->lock, flags);

drivers/dma/pl330.c static int pl330_alloc_chan_resources(struct dma_chan *chan)

Quoted 10 lines

drivers/dma/pl330.c +4 -4

 
-	/* Mark all desc done */
-	list_for_each_entry(desc, &pch->work_list, node)
-		desc->status = DONE;
+		/* FLUSH the PL330 Channel thread */
+		pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
 
-	spin_unlock_irqrestore(&pch->lock, flags);
+		/* Mark all desc done */
+		list_for_each_entry_safe(desc, _dt, &pch->work_list , node)

{

Quoted 5 lines

drivers/dma/pl330.c +4 -0

+			desc->status = DONE;
+			pch->completed = desc->txd.cookie;
+			list_move_tail(&desc->node, &list);
+		}
 

Quoted 15 lines

Quoted 4 lines

drivers/dma/pl330.c +3 -1

-	pl330_tasklet((unsigned long) pch);
+		list_splice_tail_init(&list, &pdmac->desc_pool);
+		spin_unlock_irqrestore(&pch->lock, flags);
+		break;

drivers/dma/pl330.c static int pl330_alloc_chan_resources(struct dma_chan *chan)

Quoted 10 lines

drivers/dma/pl330.c +9 -0

+	case DMA_SLAVE_CONFIG:
+		slave_config = (struct dma_slave_config *)arg;
+		peri = pch->chan.private;
+
+		if (slave_config->direction == DMA_TO_DEVICE) {
+			if (slave_config->dst_addr)
+				peri->fifo_addr = slave_config->dst_addr;
+			if (slave_config->dst_addr_width)
+				peri->burst_sz = __ffs(slave_config-
 dst_addr_width);

This doesn't sound right, why can't you use xxx_maxburst field instead?

Can you explain the problem in more detail?
Do you mean that 'xxx_maxburst' takes place of 'xxx_addr_width' or __ffs() isn't proper here?

Additionally, I will modify this code to consider '_maxburst' field.

Sent: Monday, July 25, 2011 10:48 PM
To: Boojin Kim
Cc: vinod.koul at intel.com; linux-arm-kernel at lists.infradead.org; linux- samsung-soc at vger.kernel.org; 'Kukjin Kim'; 'Jassi Brar'; 'Grant Likely'; 'Mark Brown'; 'Dan Williams'
Subject: RE: [PATCH V4 03/14] DMA: PL330: Support DMA_SLAVE_CONFIG command

Quoted 4 lines · Boojin Kim

Vinod Koul Wrote:

Quoted 3 lines

Sent: Monday, July 25, 2011 7:27 PM
To: Boojin Kim
Cc: vinod.koul at intel.com; linux-arm-kernel at lists.infradead.org;

linux-

Quoted 11 lines

Quoted 11 lines

samsung-soc at vger.kernel.org; Kukjin Kim; Jassi Brar; Grant Likely; Mark Brown; Dan Williams
Subject: Re: [PATCH V4 03/14] DMA: PL330: Support DMA_SLAVE_CONFIG command

Quoted 5 lines · Boojin Kim

This patch adds support DMA_SLAVE_CONFIG command.

Signed-off-by Boojin Kim <[email protected]>

---
 drivers/dma/pl330.c |   53

+++++++++++++++++++++++++++++++++++++----

Quoted 8 lines

Quoted 8 lines

---------

drivers/dma/pl330.c static int

Quoted 7 lines

 1 files changed, 39 insertions(+), 14 deletions(-)

drivers/dma/pl330.c +0 -0

index 586ab39..880f010 100644
@@ -256,25 +256,50 @@ static int

pl330_alloc_chan_resources(struct

Quoted 2 lines

Quoted 2 lines

dma_chan *chan)

Quoted 1 line

drivers/dma/pl330.c +0 -0

 static int pl330_control(struct dma_chan *chan, enum

dma_ctrl_cmd

Quoted 39 lines

Quoted 29 lines

cmd, unsigned long arg)

Quoted 28 lines

drivers/dma/pl330.c +10 -12

 {
 	struct dma_pl330_chan *pch = to_pchan(chan);
-	struct dma_pl330_desc *desc;
+	struct dma_pl330_desc *desc, *_dt;
 	unsigned long flags;
+	struct dma_pl330_dmac *pdmac = pch->dmac;
+	struct dma_slave_config *slave_config;
+	struct dma_pl330_peri *peri;
+	LIST_HEAD(list);
 
-	/* Only supports DMA_TERMINATE_ALL */
-	if (cmd != DMA_TERMINATE_ALL)
-		return -ENXIO;
-
-	spin_lock_irqsave(&pch->lock, flags);
-
-	/* FLUSH the PL330 Channel thread */
-	pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
+	switch (cmd) {
+	case DMA_TERMINATE_ALL:
+		spin_lock_irqsave(&pch->lock, flags);
 
-	/* Mark all desc done */
-	list_for_each_entry(desc, &pch->work_list, node)
-		desc->status = DONE;
+		/* FLUSH the PL330 Channel thread */
+		pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
 

Quoted 3 lines

drivers/dma/pl330.c static int pl330_alloc_chan_resources(struct dma_chan *chan)

Quoted 3 lines

drivers/dma/pl330.c +2 -1

-	spin_unlock_irqrestore(&pch->lock, flags);
+		/* Mark all desc done */
+		list_for_each_entry_safe(desc, _dt, &pch-

work_list , node)

Quoted 6 lines

{

Quoted 5 lines

drivers/dma/pl330.c +4 -0

+			desc->status = DONE;
+			pch->completed = desc->txd.cookie;
+			list_move_tail(&desc->node, &list);
+		}
 

Quoted 16 lines

Quoted 11 lines

drivers/dma/pl330.c static int pl330_alloc_chan_resources(struct dma_chan *chan)

Quoted 11 lines

drivers/dma/pl330.c +10 -1

-	pl330_tasklet((unsigned long) pch);
+		list_splice_tail_init(&list, &pdmac->desc_pool);
+		spin_unlock_irqrestore(&pch->lock, flags);
+		break;
+	case DMA_SLAVE_CONFIG:
+		slave_config = (struct dma_slave_config *)arg;
+		peri = pch->chan.private;
+
+		if (slave_config->direction == DMA_TO_DEVICE) {
+			if (slave_config->dst_addr)
+				peri->fifo_addr = slave_config-

dst_addr;

Quoted 4 lines

drivers/dma/pl330.c static int · line 292

Quoted 3 lines

drivers/dma/pl330.c +2 -0

+			if (slave_config->dst_addr_width)
+				peri->burst_sz = __ffs(slave_config-
 dst_addr_width);

This doesn't sound right, why can't you use xxx_maxburst field

instead?

Quoted 2 lines

Can you explain the problem in more detail?
Do you mean that 'xxx_maxburst' takes place of 'xxx_addr_width' or

__ffs()

Quoted 3 lines

isn't proper here?

Additionally, I will modify this code to consider '_maxburst' field.

__ffs is okay and sensible.
I meant you should use proper fields for passing info, which in this case would be dst_maxburst for specifying destination burst sizes

--
~Vinod Koul
Intel Corp.

Vinod Koul Wrote:

Quoted 2 lines

I meant you should use proper fields for passing info, which in this case would be dst_maxburst for specifying destination burst sizes

I think the naming seems to make confusion.
'Peri->burst_sz' on this code means the width of fifo in bytes of the peri. It's same with the meaning 'xxx_addr_width'. So, I think it's proper usage. 'peri->burst_sz' is passed to PL330 HW. PL330 HW wants to get the power of 2 based the fifo width for it.
For example, If fifo width is 8 bytes, PL330 HW should get '3' for it. And I will modify code to consider xxx_maxburst.

drivers/dma/pl330.c +8 -0

+static void pl330_tasklet_cyclic(unsigned long data)
+{
+	struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data;
+	struct dma_pl330_desc *desc, *_dt;
+	unsigned long flags;
+	LIST_HEAD(list);
+
+	spin_lock_irqsave(&pch->lock, flags);

+

drivers/dma/pl330.c +5 -0

+		}
+
+	spin_unlock_irqrestore(&pch->lock, flags);
+}
+
 static void pl330_tasklet(unsigned long data)
 {
 	struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data;

drivers/dma/pl330.c +12 -0

 }
 

@@ -316,6 +359,15 @@ static void pl330_free_chan_resources(struct dma_chan *chan)
 	pl330_release_channel(pch->pl330_chid);
 	pch->pl330_chid = NULL;
 
+	if (pch->cyclic) {
+		pch->cyclic = false;
+		list_splice_tail_init(&pch->work_list, &pch->dmac->desc_pool);
+		if (pch->cyclic_task) {
+			tasklet_kill(pch->cyclic_task);
+			pch->cyclic_task = NULL;
+		}
+	}
+
 	spin_unlock_irqrestore(&pch->lock, flags);
 }
 

@@ -547,6 +599,63 @@ static inline int get_burst_len(struct dma_pl330_desc *desc, size_t len)
 	return burst_len;
 }
 
+static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic(
+		struct dma_chan *chan, dma_addr_t dma_addr, size_t len,
+		size_t period_len, enum dma_data_direction direction)

drivers/dma/pl330.c +25 -0

+
+	desc = pl330_get_desc(pch);
+	if (!desc) {
+		dev_err(pch->dmac->pif.dev, "%s:%d Unable to fetch desc\n",
+			__func__, __LINE__);
+		return NULL;
+	}
+
+	switch (direction) {
+	case DMA_TO_DEVICE:
+		desc->rqcfg.src_inc = 1;
+		desc->rqcfg.dst_inc = 0;
+		src = dma_addr;
+		dst = peri->fifo_addr;
+		break;
+	case DMA_FROM_DEVICE:
+		desc->rqcfg.src_inc = 0;
+		desc->rqcfg.dst_inc = 1;
+		src = peri->fifo_addr;
+		dst = dma_addr;
+		break;
+	default:
+		dev_err(pch->dmac->pif.dev, "%s:%d Invalid dma direction\n",
+		__func__, __LINE__);
+		return NULL;

Sent: Monday, July 25, 2011 6:28 PM
To: Boojin Kim
Cc: linux-arm-kernel at lists.infradead.org;

Kukjin Kim; Vinod Koul; Jassi Brar; Grant Likely; Mark Brown; Dan Williams
Subject: Re: [PATCH V4 04/14] DMA: PL330: Add DMA_CYCLIC capability

Quoted 8 lines · Boojin Kim

drivers/dma/pl330.c +8 -0

+static void pl330_tasklet_cyclic(unsigned long data)
+{
+	struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data;
+	struct dma_pl330_desc *desc, *_dt;
+	unsigned long flags;
+	LIST_HEAD(list);
+
+	spin_lock_irqsave(&pch->lock, flags);

...

drivers/dma/pl330.c static inline void fill_queue(struct dma_pl330_chan *pch) · line 220

Quoted 3 lines

drivers/dma/pl330.c +3 -0

+			callback = desc->txd.callback;
+			if (callback)
+				callback(desc->txd.callback_param);

On this again - what if the callback wants to terminate the DMA activity because there's no more audio data to be sent/received from the device?

Quoted 2 lines

+

Useless blank line.

drivers/dma/pl330.c static void dma_pl330_rqcb(void *token, enum

Quoted 10 lines

drivers/dma/pl330.c +5 -0

+		}
+
+	spin_unlock_irqrestore(&pch->lock, flags);
+}
+
 static void pl330_tasklet(unsigned long data)
 {
 	struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data;

@@ -227,6 +267,9 @@ static void dma_pl330_rqcb(void *token, enum

pl330_op_err err)

Quoted 1 line

drivers/dma/pl330.c static void dma_pl330_rqcb(void *token, enum pl330_op_err err) · line 273

Quoted 6 lines

drivers/dma/pl330.c +3 -0

 	spin_unlock_irqrestore(&pch->lock, flags);
 
+	if (pch->cyclic_task)
+		tasklet_schedule(pch->cyclic_task);
+	else
 	tasklet_schedule(&pch->task);

Indentation error.

Review static void pl330_free_chan_resources(struct

Quoted 4 lines

}

(quoted diff) +0 -0

@@ -316,6 +359,15 @@ static void pl330_free_chan_resources(struct

*chan)

drivers/dma/pl330.c static void pl330_free_chan_resources(struct dma_chan *chan)

Quoted 6 lines

drivers/dma/pl330.c +3 -0

 	pl330_release_channel(pch->pl330_chid);
 	pch->pl330_chid = NULL;
 
+	if (pch->cyclic) {
+		pch->cyclic = false;
+		list_splice_tail_init(&pch->work_list,

drivers/dma/pl330.c static inline int get_burst_len(struct

Quoted 10 lines

drivers/dma/pl330.c +6 -0

+		if (pch->cyclic_task) {
+			tasklet_kill(pch->cyclic_task);
+			pch->cyclic_task = NULL;
+		}
+	}
+
 	spin_unlock_irqrestore(&pch->lock, flags);
 }
 

@@ -547,6 +599,63 @@ static inline int get_burst_len(struct

*desc, size_t len)

Quoted 12 lines

drivers/dma/pl330.c +9 -0

 	return burst_len;
 }
 
+static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic(
+		struct dma_chan *chan, dma_addr_t dma_addr, size_t len,
+		size_t period_len, enum dma_data_direction direction)
+{
+	struct dma_pl330_desc *desc;
+	struct dma_pl330_chan *pch = to_pchan(chan);
+	struct dma_pl330_peri *peri = chan->private;
+	dma_addr_t dst;
+	dma_addr_t src;

drivers/dma/pl330.c static inline int get_burst_len(struct dma_pl330_desc *desc, size_t len) · line 614

Quoted 4 lines

drivers/dma/pl330.c +4 -0

+
+	pch = to_pchan(chan);
+	if (!pch)
+		return NULL;

You've already done the to_pchan() thing when declaring pch. You've already dereferenced 'chan', so there's no way that pch could be NULL here.

Quoted 33 lines

drivers/dma/pl330.c +32 -0

+
+	desc = pl330_get_desc(pch);
+	if (!desc) {
+		dev_err(pch->dmac->pif.dev, "%s:%d Unable to fetch desc\n",
+			__func__, __LINE__);
+		return NULL;
+	}
+
+	switch (direction) {
+	case DMA_TO_DEVICE:
+		desc->rqcfg.src_inc = 1;
+		desc->rqcfg.dst_inc = 0;
+		src = dma_addr;
+		dst = peri->fifo_addr;
+		break;
+	case DMA_FROM_DEVICE:
+		desc->rqcfg.src_inc = 0;
+		desc->rqcfg.dst_inc = 1;
+		src = peri->fifo_addr;
+		dst = dma_addr;
+		break;
+	default:
+		dev_err(pch->dmac->pif.dev, "%s:%d Invalid dma direction\n",
+		__func__, __LINE__);
+		return NULL;
+	}
+
+	desc->rqcfg.brst_size = peri->burst_sz;
+	desc->rqcfg.brst_len = 1;
+
+	if (!pch->cyclic_task) {
+		pch->cyclic_task =

drivers/dma/pl330.c static inline int get_burst_len(struct dma_pl330_desc *desc, size_t len) · line 649

Quoted 3 lines

drivers/dma/pl330.c +3 -0

+			kmalloc(sizeof(struct tasklet_struct), GFP_KERNEL);
+		tasklet_init(pch->cyclic_task,
+			pl330_tasklet_cyclic, (unsigned int)pch);

Here you allocate memory for the cyclic task. Above you set this pointer to NULL. That sounds like a memory leak to me. Why are you kmallocing this memory - why can't it be part of the dma_pl330_chan structure? It's only 28 bytes.

Quoted 10 lines · Boojin Kim

Quoted 8 lines

drivers/dma/pl330.c +8 -0

+static void pl330_tasklet_cyclic(unsigned long data)
+{
+	struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data;
+	struct dma_pl330_desc *desc, *_dt;
+	unsigned long flags;
+	LIST_HEAD(list);
+
+	spin_lock_irqsave(&pch->lock, flags);

...

Quoted 6 lines

drivers/dma/pl330.c static inline void fill_queue(struct dma_pl330_chan *pch) · line 220

Quoted 3 lines

drivers/dma/pl330.c +3 -0

+			callback = desc->txd.callback;
+			if (callback)
+				callback(desc->txd.callback_param);

On this again - what if the callback wants to terminate the DMA activity because there's no more audio data to be sent/received from the device?

Do you mean what is happened if the callback() is called after channel is terminated ?
Or What is happened if Callback() calls 'dma_release_channel()' to terminate DMA?

Quoted 10 lines

drivers/dma/pl330.c static inline int get_burst_len(struct dma_pl330_desc *desc, size_t len) · line 649

Quoted 5 lines

drivers/dma/pl330.c +5 -0

+	if (!pch->cyclic_task) {
+		pch->cyclic_task =
+			kmalloc(sizeof(struct tasklet_struct), GFP_KERNEL);
+		tasklet_init(pch->cyclic_task,
+			pl330_tasklet_cyclic, (unsigned int)pch);

Here you allocate memory for the cyclic task. Above you set this pointer to NULL. That sounds like a memory leak to me. Why are you kmallocing this memory - why can't it be part of the dma_pl330_chan structure? It's only 28 bytes.

It's my mistake. I should have been free of the memory.

And the reason why I use kmalloc for 'cyclic_task' is following. This memory size for 'cyclic_tasklet' is the 896 bytes ( = the number of channel * sizeof(struct tasklet_struct)= 32*28) for each DMAC. And This memory size is increased according to the number of DMAC.
And Samsung has the DMAC that is dedicated for Mem-to-Mem operation. If I make 'cyclic_task' be part of dma_pl330_chan, this DMAC that is dedicated for Mem-to-Mem operation should hold unused data.
So, I think it's loss that all dma channels hold own 'cyclic_task'.

Quoted 17 lines · Boojin Kim

Quoted 16 lines · Boojin Kim

Quoted 8 lines

drivers/dma/pl330.c +8 -0

+static void pl330_tasklet_cyclic(unsigned long data)
+{
+	struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data;
+	struct dma_pl330_desc *desc, *_dt;
+	unsigned long flags;
+	LIST_HEAD(list);
+
+	spin_lock_irqsave(&pch->lock, flags);

...

drivers/dma/pl330.c static inline void fill_queue(struct dma_pl330_chan *pch) · line 220

Quoted 3 lines

drivers/dma/pl330.c +3 -0

+			callback = desc->txd.callback;
+			if (callback)
+				callback(desc->txd.callback_param);

On this again - what if the callback wants to terminate the DMA activity because there's no more audio data to be sent/received from the device?

Quoted 4 lines

Do you mean what is happened if the callback() is called after channel is terminated ?
Or What is happened if Callback() calls 'dma_release_channel()' to terminate DMA?

No. I mean what if the callback wants to call dmaengine_terminate_all().

Quoted 14 lines

Quoted 11 lines

drivers/dma/pl330.c static inline int get_burst_len(struct dma_pl330_desc *desc, size_t len) · line 649

Quoted 6 lines

drivers/dma/pl330.c +5 -0

+	if (!pch->cyclic_task) {
+		pch->cyclic_task =
+			kmalloc(sizeof(struct tasklet_struct), GFP_KERNEL);
+		tasklet_init(pch->cyclic_task,
+			pl330_tasklet_cyclic, (unsigned int)pch);

Here you allocate memory for the cyclic task. Above you set this pointer to NULL. That sounds like a memory leak to me. Why are you kmallocing this memory - why can't it be part of the dma_pl330_chan structure? It's only 28 bytes.

It's my mistake. I should have been free of the memory.

Quoted 8 lines

And the reason why I use kmalloc for 'cyclic_task' is following. This memory size for 'cyclic_tasklet' is the 896 bytes ( = the number of channel * sizeof(struct tasklet_struct)= 32*28) for each DMAC. And This memory size is increased according to the number of DMAC.
And Samsung has the DMAC that is dedicated for Mem-to-Mem operation. If I make 'cyclic_task' be part of dma_pl330_chan, this DMAC that is dedicated for Mem-to-Mem operation should hold unused data.
So, I think it's loss that all dma channels hold own 'cyclic_task'.

Could you re-use the tasklet that already exists?

Quoted 23 lines · Russell King - ARM Linux

Quoted 21 lines · Boojin Kim

Quoted 10 lines · Boojin Kim

Quoted 8 lines

drivers/dma/pl330.c +8 -0

+static void pl330_tasklet_cyclic(unsigned long data)
+{
+	struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data;
+	struct dma_pl330_desc *desc, *_dt;
+	unsigned long flags;
+	LIST_HEAD(list);
+
+	spin_lock_irqsave(&pch->lock, flags);

...

Quoted 6 lines

drivers/dma/pl330.c static inline void fill_queue(struct dma_pl330_chan *pch) · line 220

Quoted 3 lines

drivers/dma/pl330.c +3 -0

+			callback = desc->txd.callback;
+			if (callback)
+				callback(desc->txd.callback_param);

On this again - what if the callback wants to terminate the DMA activity because there's no more audio data to be sent/received from the device?

Do you mean what is happened if the callback() is called after channel is terminated ?
Or What is happened if Callback() calls 'dma_release_channel()' to terminate DMA?

Quoted 1 line

No. I mean what if the callback wants to call dmaengine_terminate_all().

you are supposed to drop the lock here, that way callback can call any DMA API, otherwise it will result in deadlock.
This make me wonder you haven't read the documentation at all, please ensure you have read Documentation/dmaengine.txt before next posting

Quoted 25 lines · Vinod Koul

Quoted 24 lines · Russell King - ARM Linux

Quoted 17 lines · Boojin Kim

Quoted 10 lines · Boojin Kim

Quoted 8 lines

drivers/dma/pl330.c +8 -0

+static void pl330_tasklet_cyclic(unsigned long data)
+{
+	struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data;
+	struct dma_pl330_desc *desc, *_dt;
+	unsigned long flags;
+	LIST_HEAD(list);
+
+	spin_lock_irqsave(&pch->lock, flags);

...

Quoted 6 lines

drivers/dma/pl330.c static inline void fill_queue(struct dma_pl330_chan *pch) · line 220

Quoted 3 lines

drivers/dma/pl330.c +3 -0

+			callback = desc->txd.callback;
+			if (callback)
+				callback(desc->txd.callback_param);

On this again - what if the callback wants to terminate the DMA activity because there's no more audio data to be sent/received from the device?

Quoted 4 lines

Do you mean what is happened if the callback() is called after channel is terminated ?
Or What is happened if Callback() calls 'dma_release_channel()' to terminate DMA?

No. I mean what if the callback wants to call dmaengine_terminate_all().

Quoted 4 lines

you are supposed to drop the lock here, that way callback can call any DMA API, otherwise it will result in deadlock.
This make me wonder you haven't read the documentation at all, please ensure you have read Documentation/dmaengine.txt before next posting

I know that very well thank you. Please look at my previous post in the previous round of patches, and the response from Boojin Kim to see why I used this as an *EXAMPLE* to get this fixed.

Quoted 3 lines

I know that very well thank you. Please look at my previous post in the previous round of patches, and the response from Boojin Kim to see why I used this as an *EXAMPLE* to get this fixed.

Russell,
Sorry that was not intended for you but for the author of patch Boojin Kim... Agree on your EXAMPLE, just wanted to ensure authors have read it as going thru this patch made it clear that they haven't.

Quoted 7 lines · Vinod Koul

Quoted 3 lines

I know that very well thank you. Please look at my previous post in the previous round of patches, and the response from Boojin Kim to see why I used this as an *EXAMPLE* to get this fixed.

Russell,
Sorry that was not intended for you but for the author of patch Boojin Kim... Agree on your EXAMPLE, just wanted to ensure authors have read it as going thru this patch made it clear that they haven't.

That wasn't obvious because of the To: and the use of 'you' in your reply.

In any case, let's fix up the documentation to be a little more complete and expansive about these conditions, and improve its formatting so it's easier to read.

A few points:

1. This really should describe the dma_slave_config structure in detail (or refer to the documentation in dmaengine.h) especially describing what is mandatory there.

2. Note the filter function thing - I think this should be mandatory for slave and cyclic channels. These typically have a DMA request signal associated with each channel, and so 'any random channel' isn't really acceptable like it is for the async_tx APIs.

3. I think this should specify that dmaengine_slave_config() should be specified as mandatory after obtaining the DMA engine channel.

Any comments on this before I prepare this as a proper patch?

Documentation/dmaengine.txt +72 -42

index 5a0cb1e..8ba8d3c 100644
@@ -10,87 +10,163 @@
 Below is a guide to device driver writers on how to use the Slave-DMA API of the
 DMA Engine. This is applicable only for slave DMA usage only.
 
-The slave DMA usage consists of following steps
+The slave DMA usage consists of following steps:
 1. Allocate a DMA slave channel
 2. Set slave and controller specific parameters
 3. Get a descriptor for transaction
 4. Submit the transaction and wait for callback notification
+5. Issue pending requests
 
 1. Allocate a DMA slave channel
-Channel allocation is slightly different in the slave DMA context, client
-drivers typically need a channel from a particular DMA controller only and even
-in some cases a specific channel is desired. To request a channel
-dma_request_channel() API is used.
-
-Interface:
-struct dma_chan *dma_request_channel(dma_cap_mask_t mask,
-		dma_filter_fn filter_fn,
-		void *filter_param);
-where dma_filter_fn is defined as:
-typedef bool (*dma_filter_fn)(struct dma_chan *chan, void *filter_param);
-
-When the optional 'filter_fn' parameter is set to NULL dma_request_channel
-simply returns the first channel that satisfies the capability mask.  Otherwise,
-when the mask parameter is insufficient for specifying the necessary channel,
-the filter_fn routine can be used to disposition the available channels in the
-system. The filter_fn routine is called once for each free channel in the
-system.  Upon seeing a suitable channel filter_fn returns DMA_ACK which flags
-that channel to be the return value from dma_request_channel.  A channel
-allocated via this interface is exclusive to the caller, until
-dma_release_channel() is called.
+
+   Channel allocation is slightly different in the slave DMA context,
+   client drivers typically need a channel from a particular DMA
+   controller only and even in some cases a specific channel is desired.
+   To request a channel dma_request_channel() API is used.
+
+   Interface:
+	struct dma_chan *dma_request_channel(dma_cap_mask_t mask,
+			dma_filter_fn filter_fn,
+			void *filter_param);
+   where dma_filter_fn is defined as:
+	typedef bool (*dma_filter_fn)(struct dma_chan *chan, void *filter_param);
+
+   The 'filter_fn' parameter is optional, but highly recommended for
+   slave and cyclic channels as they typically need to obtain a specific
+   DMA channel.
+
+   When the optional 'filter_fn' parameter is NULL, dma_request_channel()
+   simply returns the first channel that satisfies the capability mask.
+
+   Otherwise, the 'filter_fn' routine will be called once for each free
+   channel which has a capability in 'mask'.  'filter_fn' is expected to
+   return 'true' when the desired DMA channel is found.
+
+   A channel allocated via this interface is exclusive to the caller,
+   until dma_release_channel() is called.
 
 2. Set slave and controller specific parameters
-Next step is always to pass some specific information to the DMA driver. Most of
-the generic information which a slave DMA can use is in struct dma_slave_config.
-It allows the clients to specify DMA direction, DMA addresses, bus widths, DMA
-burst lengths etc. If some DMA controllers have more parameters to be sent then
-they should try to embed struct dma_slave_config in their controller specific
-structure. That gives flexibility to client to pass more parameters, if
-required.
-
-Interface:
-int dmaengine_slave_config(struct dma_chan *chan,
-					  struct dma_slave_config *config)
+
+   Next step is always to pass some specific information to the DMA
+   driver.  Most of the generic information which a slave DMA can use
+   is in struct dma_slave_config.  This allows the clients to specify
+   DMA direction, DMA addresses, bus widths, DMA burst lengths etc
+   for the peripheral.
+
+   If some DMA controllers have more parameters to be sent then they
+   should try to embed struct dma_slave_config in their controller
+   specific structure. That gives flexibility to client to pass more
+   parameters, if required.
+
+   Interface:
+	int dmaengine_slave_config(struct dma_chan *chan,
+				  struct dma_slave_config *config)
 
 3. Get a descriptor for transaction
-For slave usage the various modes of slave transfers supported by the
-DMA-engine are:
-slave_sg	- DMA a list of scatter gather buffers from/to a peripheral
-dma_cyclic	- Perform a cyclic DMA operation from/to a peripheral till the
+
+   For slave usage the various modes of slave transfers supported by the
+   DMA-engine are:
+
+   slave_sg	- DMA a list of scatter gather buffers from/to a peripheral
+   dma_cyclic	- Perform a cyclic DMA operation from/to a peripheral till the
 		  operation is explicitly stopped.
-The non NULL return of this transfer API represents a "descriptor" for the given
-transaction.
 
-Interface:
-struct dma_async_tx_descriptor *(*chan->device->device_prep_dma_sg)(
+   A non-NULL return of this transfer API represents a "descriptor" for
+   the given transaction.
+
+   Interface:
+	struct dma_async_tx_descriptor *(*chan->device->device_prep_dma_sg)(
 		struct dma_chan *chan,
 		struct scatterlist *dst_sg, unsigned int dst_nents,
 		struct scatterlist *src_sg, unsigned int src_nents,
 		unsigned long flags);
-struct dma_async_tx_descriptor *(*chan->device->device_prep_dma_cyclic)(
+
+	struct dma_async_tx_descriptor *(*chan->device->device_prep_dma_cyclic)(
 		struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
 		size_t period_len, enum dma_data_direction direction);
 
+   Once a descriptor has been obtained, the callback information can be
+   added and must then be submitted.  Some DMA engine drivers may hold a
+   spinlock between a successful preparation and submission so it is
+   important that these two operations are closely paired.
+
+   Note:
+	Although the async_tx API specifies that completion callback
+	routines cannot submit any new operations, this is not the
+	case for slave/cyclic DMA.
+
+	For slave DMA, the subsequent transaction may not be available
+	for submission prior to callback function being invoked, so
+	slave DMA callbacks are permitted to prepare and submit a new
+	transaction.
+
+	For cyclic DMA, a callback function may wish to terminate the

Documentation/dmaengine.txt +4 -22

-To schedule the transaction to be scheduled by dma device, the "descriptor"
-returned in above (3) needs to be submitted.
-To tell the dma driver that a transaction is ready to be serviced, the
-descriptor->submit() callback needs to be invoked. This chains the descriptor to
-the pending queue.
-The transactions in the pending queue can be activated by calling the
-issue_pending API. If channel is idle then the first transaction in queue is
-started and subsequent ones queued up.
-On completion of the DMA operation the next in queue is submitted and a tasklet
-triggered. The tasklet would then call the client driver completion callback
-routine for notification, if set.
-Interface:
-void dma_async_issue_pending(struct dma_chan *chan);
-
-==============================================================================
-
-Additional usage notes for dma driver writers
-1/ Although DMA engine specifies that completion callback routines cannot submit
-any new operations, but typically for slave DMA subsequent transaction may not
-be available for submit prior to callback routine being called. This requirement
-is not a requirement for DMA-slave devices. But they should take care to drop
-the spin-lock they might be holding before calling the callback routine
+
+   Once the descriptor has been prepared and the callback information
+   added, it must be placed on the DMA engine drivers pending queue.
+

Documentation/dmaengine.txt +45 -0

+
+   The transactions in the pending queue can be activated by calling the
+   issue_pending API. If channel is idle then the first transaction in
+   queue is started and subsequent ones queued up.
+
+   On completion of each DMA operation, the next in queue is started and
+   a tasklet triggered. The tasklet will then call the client driver
+   completion callback routine for notification, if set.
+
+   Interface:
+	void dma_async_issue_pending(struct dma_chan *chan);
+
+Further APIs:
+
+1. int dmaengine_terminate_all(struct dma_chan *chan)
+
+   This causes all activity for the DMA channel to be stopped, and may
+   discard data in the DMA FIFO which hasn't been fully transferred.
+   No callback functions will be called for any incomplete transfers.
+
+2. int dmaengine_pause(struct dma_chan *chan)
+
+   This pauses activity on the DMA channel without data loss.
+
+3. int dmaengine_resume(struct dma_chan *chan)
+
+   Resume a previously paused DMA channel.  It is invalid to resume a
+   channel which is not currently paused.
+
+4. enum dma_status dma_async_is_tx_complete(struct dma_chan *chan,
+        dma_cookie_t cookie, dma_cookie_t *last, dma_cookie_t *used)
+
+   This can be used to check the status of the channel.  Please see
+   the documentation in include/linux/dmaengine.h for a more complete
+   description of this API.
+
+   This can be used in conjunction with dma_async_is_complete() and
+   the cookie returned from 'descriptor->submit()' to check for
+   completion of a specific DMA transaction.
+
+   Note:
+	Not all DMA engine drivers can return reliable information for
+	a running DMA channel.  It is recommended that DMA engine users
+	pause or stop (via dmaengine_terminate_all) the channel before
+	using this API.

Here's an updated patch.

8<----------
From: Russell King <[email protected]>
DMAEngine: Improve slave/cyclic documentation

Improve the documentation for the slave and cyclic DMA engine support reformatting it for easier reading, adding further APIs, splitting it into five steps, and including references to the documentation in dmaengine.h.

Signed-off-by Russell King <[email protected]>

---
 Documentation/dmaengine.txt |  211 ++++++++++++++++++++++++++++++-------------
 1 files changed, 146 insertions(+), 65 deletions(-)

Documentation/dmaengine.txt +0 -0

index 5a0cb1e..8c2e888 100644

Documentation/dmaengine.txt +144 -64

 1. Allocate a DMA slave channel
-Channel allocation is slightly different in the slave DMA context, client
-drivers typically need a channel from a particular DMA controller only and even
-in some cases a specific channel is desired. To request a channel
-dma_request_channel() API is used.
-
-Interface:
-struct dma_chan *dma_request_channel(dma_cap_mask_t mask,
-		dma_filter_fn filter_fn,
-		void *filter_param);
-where dma_filter_fn is defined as:
-typedef bool (*dma_filter_fn)(struct dma_chan *chan, void *filter_param);
-
-When the optional 'filter_fn' parameter is set to NULL dma_request_channel
-simply returns the first channel that satisfies the capability mask.  Otherwise,
-when the mask parameter is insufficient for specifying the necessary channel,
-the filter_fn routine can be used to disposition the available channels in the
-system. The filter_fn routine is called once for each free channel in the
-system.  Upon seeing a suitable channel filter_fn returns DMA_ACK which flags
-that channel to be the return value from dma_request_channel.  A channel
-allocated via this interface is exclusive to the caller, until
-dma_release_channel() is called.
+
+   Channel allocation is slightly different in the slave DMA context,
+   client drivers typically need a channel from a particular DMA
+   controller only and even in some cases a specific channel is desired.
+   To request a channel dma_request_channel() API is used.
+
+   Interface:
+	struct dma_chan *dma_request_channel(dma_cap_mask_t mask,
+			dma_filter_fn filter_fn,
+			void *filter_param);
+   where dma_filter_fn is defined as:
+	typedef bool (*dma_filter_fn)(struct dma_chan *chan, void *filter_param);
+
+   The 'filter_fn' parameter is optional, but highly recommended for
+   slave and cyclic channels as they typically need to obtain a specific
+   DMA channel.
+
+   When the optional 'filter_fn' parameter is NULL, dma_request_channel()
+   simply returns the first channel that satisfies the capability mask.
+
+   Otherwise, the 'filter_fn' routine will be called once for each free
+   channel which has a capability in 'mask'.  'filter_fn' is expected to
+   return 'true' when the desired DMA channel is found.
+
+   A channel allocated via this interface is exclusive to the caller,
+   until dma_release_channel() is called.
 
 2. Set slave and controller specific parameters
-Next step is always to pass some specific information to the DMA driver. Most of
-the generic information which a slave DMA can use is in struct dma_slave_config.
-It allows the clients to specify DMA direction, DMA addresses, bus widths, DMA
-burst lengths etc. If some DMA controllers have more parameters to be sent then
-they should try to embed struct dma_slave_config in their controller specific
-structure. That gives flexibility to client to pass more parameters, if
-required.
-
-Interface:
-int dmaengine_slave_config(struct dma_chan *chan,
-					  struct dma_slave_config *config)
+
+   Next step is always to pass some specific information to the DMA
+   driver.  Most of the generic information which a slave DMA can use
+   is in struct dma_slave_config.  This allows the clients to specify
+   DMA direction, DMA addresses, bus widths, DMA burst lengths etc
+   for the peripheral.
+
+   If some DMA controllers have more parameters to be sent then they
+   should try to embed struct dma_slave_config in their controller
+   specific structure. That gives flexibility to client to pass more
+   parameters, if required.
+
+   Interface:
+	int dmaengine_slave_config(struct dma_chan *chan,
+				  struct dma_slave_config *config)
+
+   Please see the dma_slave_config structure definition in dmaengine.h
+   for a detailed explaination of the struct members.  Please note
+   that the 'direction' member will be going away as it duplicates the
+   direction given in the prepare call.
 
 3. Get a descriptor for transaction
-For slave usage the various modes of slave transfers supported by the
-DMA-engine are:
-slave_sg	- DMA a list of scatter gather buffers from/to a peripheral
-dma_cyclic	- Perform a cyclic DMA operation from/to a peripheral till the
+
+   For slave usage the various modes of slave transfers supported by the
+   DMA-engine are:
+
+   slave_sg	- DMA a list of scatter gather buffers from/to a peripheral
+   dma_cyclic	- Perform a cyclic DMA operation from/to a peripheral till the
 		  operation is explicitly stopped.
-The non NULL return of this transfer API represents a "descriptor" for the given
-transaction.
 
-Interface:
-struct dma_async_tx_descriptor *(*chan->device->device_prep_dma_sg)(
+   A non-NULL return of this transfer API represents a "descriptor" for
+   the given transaction.
+
+   Interface:
+	struct dma_async_tx_descriptor *(*chan->device->device_prep_dma_sg)(
 		struct dma_chan *chan,
 		struct scatterlist *dst_sg, unsigned int dst_nents,
 		struct scatterlist *src_sg, unsigned int src_nents,
 		unsigned long flags);
-struct dma_async_tx_descriptor *(*chan->device->device_prep_dma_cyclic)(
+
+	struct dma_async_tx_descriptor *(*chan->device->device_prep_dma_cyclic)(
 		struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
 		size_t period_len, enum dma_data_direction direction);
 
-4. Submit the transaction and wait for callback notification
-To schedule the transaction to be scheduled by dma device, the "descriptor"
-returned in above (3) needs to be submitted.
-To tell the dma driver that a transaction is ready to be serviced, the
-descriptor->submit() callback needs to be invoked. This chains the descriptor to
-the pending queue.
-The transactions in the pending queue can be activated by calling the
-issue_pending API. If channel is idle then the first transaction in queue is
-started and subsequent ones queued up.
-On completion of the DMA operation the next in queue is submitted and a tasklet
-triggered. The tasklet would then call the client driver completion callback
-routine for notification, if set.
-Interface:
-void dma_async_issue_pending(struct dma_chan *chan);
-
-==============================================================================
-
-Additional usage notes for dma driver writers
-1/ Although DMA engine specifies that completion callback routines cannot submit
-any new operations, but typically for slave DMA subsequent transaction may not
-be available for submit prior to callback routine being called. This requirement
-is not a requirement for DMA-slave devices. But they should take care to drop
-the spin-lock they might be holding before calling the callback routine
+   Once a descriptor has been obtained, the callback information can be
+   added and must then be submitted.  Some DMA engine drivers may hold a
+   spinlock between a successful preparation and submission so it is
+   important that these two operations are closely paired.
+
+   Note:
+	Although the async_tx API specifies that completion callback
+	routines cannot submit any new operations, this is not the
+	case for slave/cyclic DMA.
+
+	For slave DMA, the subsequent transaction may not be available
+	for submission prior to callback function being invoked, so
+	slave DMA callbacks are permitted to prepare and submit a new
+	transaction.
+
+	For cyclic DMA, a callback function may wish to terminate the
+	DMA via dmaengine_terminate_all().
+
+	Therefore, it is important that DMA engine drivers drop any
+	locks before calling the callback function which may cause a
+	deadlock.
+
+	Note that callbacks will always be invoked from the DMA
+	engines tasklet, never from interrupt context.
+
+4. Submit the transaction
+
+   Once the descriptor has been prepared and the callback information
+   added, it must be placed on the DMA engine drivers pending queue.
+
+   Interface:
+	dma_cookie_t dmaengine_submit(struct dma_async_tx_descriptor *desc)
+
+   This returns a cookie can be used to check the progress of DMA engine
+   activity via other DMA engine calls not covered in this document.
+
+   dmaengine_submit() will not start the DMA operation, it merely adds
+   it to the pending queue.  For this, see step 5, dma_async_issue_pending.
+
+5. Issue pending DMA requests and wait for callback notification
+
+   The transactions in the pending queue can be activated by calling the
+   issue_pending API. If channel is idle then the first transaction in
+   queue is started and subsequent ones queued up.
+
+   On completion of each DMA operation, the next in queue is started and
+   a tasklet triggered. The tasklet will then call the client driver
+   completion callback routine for notification, if set.
+
+   Interface:
+	void dma_async_issue_pending(struct dma_chan *chan);
+
+Further APIs:
+
+1. int dmaengine_terminate_all(struct dma_chan *chan)
+
+   This causes all activity for the DMA channel to be stopped, and may
+   discard data in the DMA FIFO which hasn't been fully transferred.
+   No callback functions will be called for any incomplete transfers.
+
+2. int dmaengine_pause(struct dma_chan *chan)
+
+   This pauses activity on the DMA channel without data loss.
+
+3. int dmaengine_resume(struct dma_chan *chan)
+
+   Resume a previously paused DMA channel.  It is invalid to resume a
+   channel which is not currently paused.
+
+4. enum dma_status dma_async_is_tx_complete(struct dma_chan *chan,
+        dma_cookie_t cookie, dma_cookie_t *last, dma_cookie_t *used)
+
+   This can be used to check the status of the channel.  Please see
+   the documentation in include/linux/dmaengine.h for a more complete
+   description of this API.
+
+   This can be used in conjunction with dma_async_is_complete() and
+   the cookie returned from 'descriptor->submit()' to check for
+   completion of a specific DMA transaction.
+
+   Note:
+	Not all DMA engine drivers can return reliable information for
+	a running DMA channel.  It is recommended that DMA engine users
+	pause or stop (via dmaengine_terminate_all) the channel before
+	using this API.
 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 26 lines · Russell King - ARM Linux

Here's an updated patch.

8<----------
From: Russell King <[email protected]>
DMAEngine: Improve slave/cyclic documentation

Improve the documentation for the slave and cyclic DMA engine support reformatting it for easier reading, adding further APIs, splitting it into five steps, and including references to the documentation in dmaengine.h.

Signed-off-by Russell King <[email protected]>

---
?Documentation/dmaengine.txt | ?211 ++++++++++++++++++++++++++++++------------- ?1 files changed, 146 insertions(+), 65 deletions(-)

Documentation/dmaengine.txt +0 -0

index 5a0cb1e..8c2e888 100644
@@ -10,87 +10,168 @@

?Below is a guide to device driver writers on how to use the Slave-DMA API of the ?DMA Engine. This is applicable only for slave DMA usage only.

Documentation/dmaengine.txt +1 -1

-The slave DMA usage consists of following steps
+The slave DMA usage consists of following steps:

Quoted 5 lines

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

Thanks Russell,

Applied with change to 4 above. Moved "and wait for callback notification" to 5.

Dear Vinod,

Since it came from the RMK, most probably it'll be the best.

But applying patches upon personal timeout seems very dangerous.

People not responding doesn't mean only either people agree completely or they don't care. Some might be interested but too busy with current tasks that they need time to check... please make some policy for such cases.

It already happened with the patch from Rob, which you probably have to revert.

IMHO, if nobody replied, maybe you could first ack the patch and wait for, say a week, before applying?
That way people will know they have to hurry if they care otherwise the patch is going upstream as such.

Quoted 5 lines · Jassi Brar

Dear Vinod,

?Since it came from the RMK, most probably it'll be the best.

But applying patches upon personal timeout seems very dangerous.

Ehh what? ?Is there any contention over this documentation patch?

Quoted 8 lines

People not responding doesn't mean only either people agree completely or they don't care. Some might be interested but too busy with current tasks that they need time to check... please make some policy for such cases.

It already happened with the patch from Rob, which you probably have to revert.

Quoted 4 lines

IMHO, if nobody replied, maybe you could first ack the patch and wait for, say a week, before applying?
That way people will know they have to hurry if they care otherwise the patch is going upstream as such.

A week is far too long. ?That's how patches get lost and missed.

Quoted 7 lines · Russell King - ARM Linux

Quoted 5 lines · Jassi Brar

Dear Vinod,

?Since it came from the RMK, most probably it'll be the best.

But applying patches upon personal timeout seems very dangerous.

Quoted 1 line

Ehh what? ?Is there any contention over this documentation patch?

I haven't yet read it... even after reading I would object only if I found your patch disturbing enough to disrupt my bowel movements. Which I don't think would be the case.

I just observed it is second time that Vinod applied a patch without any ack or prior alert.

Quoted 7 lines · Russell King - ARM Linux

Quoted 5 lines · Jassi Brar

Dear Vinod,

?Since it came from the RMK, most probably it'll be the best.

But applying patches upon personal timeout seems very dangerous.

Quoted 1 line

Ehh what? ?Is there any contention over this documentation patch?

I haven't yet read it... even after reading I would object only if I found your patch disturbing enough to disrupt my bowel movements. Which I don't think would be the case.

I just observed it is second time that Vinod applied a patch without any ack or prior alert.

Quoted 3 lines · Russell King - ARM Linux

Quoted 2 lines · Jassi Brar

Dear Vinod,

Quoted 5 lines

Quoted 3 lines

Since it came from the RMK, most probably it'll be the best.

But applying patches upon personal timeout seems very dangerous.

Ehh what? Is there any contention over this documentation patch?

I haven't yet read it... even after reading I would object only if I found your patch disturbing enough to disrupt my bowel movements. Which I don't think would be the case.

I just observed it is second time that Vinod applied a patch without any ack or prior alert.

Quoted 9 lines

Quoted 9 lines

People not responding doesn't mean only either people agree completely or they don't care. Some might be interested but too busy with current tasks that they need time to check... please make some policy for such cases.

It already happened with the patch from Rob, which you probably have to revert.

Quoted 7 lines

Quoted 4 lines

IMHO, if nobody replied, maybe you could first ack the patch and wait for, say a week, before applying?
That way people will know they have to hurry if they care otherwise the patch is going upstream as such.

A week is far too long. That's how patches get lost and missed.

He may decide to wait shorter, but imho a week after the first ack isn't that long.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 21 lines · Russell King - ARM Linux

Here's an updated patch.

8<----------
From: Russell King <[email protected]>
DMAEngine: Improve slave/cyclic documentation

Improve the documentation for the slave and cyclic DMA engine support reformatting it for easier reading, adding further APIs, splitting it into five steps, and including references to the documentation in dmaengine.h.

Signed-off-by Russell King <[email protected]>

---
 Documentation/dmaengine.txt |  211 ++++++++++++++++++++++++++++++-------------
 1 files changed, 146 insertions(+), 65 deletions(-)

Documentation/dmaengine.txt +0 -0

index 5a0cb1e..8c2e888 100644

Documentation/dmaengine.txt line 18

Quoted 10 lines

Documentation/dmaengine.txt +2 -1

@@ -10,87 +10,168 @@
 Below is a guide to device driver writers on how to use the Slave-DMA API of the
 DMA Engine. This is applicable only for slave DMA usage only.
 
-The slave DMA usage consists of following steps
+The slave DMA usage consists of following steps:
 1. Allocate a DMA slave channel
 2. Set slave and controller specific parameters
 3. Get a descriptor for transaction
 4. Submit the transaction and wait for callback notification
+5. Issue pending requests

Thanks Russell,

Applied with change to 4 above. Moved "and wait for callback notification" to 5.

Quoted 34 lines · Vinod Koul

Quoted 21 lines · Russell King - ARM Linux

Here's an updated patch.

8<----------
From: Russell King <[email protected]>
DMAEngine: Improve slave/cyclic documentation

Improve the documentation for the slave and cyclic DMA engine support reformatting it for easier reading, adding further APIs, splitting it into five steps, and including references to the documentation in dmaengine.h.

Signed-off-by Russell King <[email protected]>

---
 Documentation/dmaengine.txt |  211 ++++++++++++++++++++++++++++++-------------
 1 files changed, 146 insertions(+), 65 deletions(-)

Documentation/dmaengine.txt +0 -0

index 5a0cb1e..8c2e888 100644

Documentation/dmaengine.txt line 18

Quoted 10 lines

Documentation/dmaengine.txt +2 -1

@@ -10,87 +10,168 @@
 Below is a guide to device driver writers on how to use the Slave-DMA API of the
 DMA Engine. This is applicable only for slave DMA usage only.
 
-The slave DMA usage consists of following steps
+The slave DMA usage consists of following steps:
 1. Allocate a DMA slave channel
 2. Set slave and controller specific parameters
 3. Get a descriptor for transaction
 4. Submit the transaction and wait for callback notification
+5. Issue pending requests

Thanks Russell,

Quoted 2 lines

Applied with change to 4 above. Moved "and wait for callback notification" to 5.

BTW, did you apply this one or the later one with the improved documentation for dma mapping and fixed interface for slave transfers etc. ?

Here's an updated patch.

Sent: Monday, July 25, 2011 7:48 PM
To: Russell King - ARM Linux
Cc: vinod.koul at intel.com; Boojin Kim; 'Kukjin Kim'; 'Jassi Brar'; 'Grant Likely'; linux-samsung-soc at vger.kernel.org; 'Mark Brown'; 'Dan Williams'; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH V4 04/14] DMA: PL330: Add DMA_CYCLIC capability

Quoted 11 lines · Russell King - ARM Linux

Quoted 10 lines · Boojin Kim

Quoted 10 lines · Boojin Kim

Quoted 8 lines

drivers/dma/pl330.c +8 -0

+static void pl330_tasklet_cyclic(unsigned long data)
+{
+	struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data;
+	struct dma_pl330_desc *desc, *_dt;
+	unsigned long flags;
+	LIST_HEAD(list);
+
+	spin_lock_irqsave(&pch->lock, flags);

...

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

drivers/dma/pl330.c static inline void fill_queue(struct dma_pl330_chan *pch) · line 220

Quoted 3 lines

drivers/dma/pl330.c +3 -0

+			callback = desc->txd.callback;
+			if (callback)
+				callback(desc->txd.callback_param);

On this again - what if the callback wants to terminate the DMA

activity

Quoted 1 line

Quoted 1 line

Quoted 1 line

because there's no more audio data to be sent/received from the

device?

Quoted 2 lines

Quoted 2 lines

Do you mean what is happened if the callback() is called after

channel is

Quoted 2 lines

Quoted 2 lines

terminated ?
Or What is happened if Callback() calls 'dma_release_channel()' to

terminate

Quoted 3 lines

Quoted 1 line

DMA?

No. I mean what if the callback wants to call

dmaengine_terminate_all().
you are supposed to drop the lock here, that way callback can call any DMA API, otherwise it will result in deadlock.
This make me wonder you haven't read the documentation at all, please ensure you have read Documentation/dmaengine.txt before next posting

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

drivers/dma/pl330.c static inline int get_burst_len(struct dma_pl330_desc *desc, size_t len)

Quoted 4 lines

drivers/dma/pl330.c +3 -0

+	if (!pch->cyclic_task) {
+		pch->cyclic_task =
+			kmalloc(sizeof(struct tasklet_struct),

GFP_KERNEL);

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

drivers/dma/pl330.c static inline int get_burst_len(struct dma_pl330_desc *desc, size_t len) · line 649

Quoted 2 lines

drivers/dma/pl330.c +2 -0

+		tasklet_init(pch->cyclic_task,
+			pl330_tasklet_cyclic, (unsigned int)pch);

Here you allocate memory for the cyclic task. Above you set this

pointer

Quoted 1 line

Quoted 1 line

Quoted 1 line

to NULL. That sounds like a memory leak to me. Why are you

kmallocing

Quoted 1 line

Quoted 1 line

Quoted 1 line

this memory - why can't it be part of the dma_pl330_chan

structure? It's

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

only 28 bytes.

It's my mistake. I should have been free of the memory.

And the reason why I use kmalloc for 'cyclic_task' is following. This memory size for 'cyclic_tasklet' is the 896 bytes ( = the

number of

Quoted 1 line

Quoted 1 line

channel * sizeof(struct tasklet_struct)= 32*28) for each DMAC. And

This

Quoted 2 lines

Quoted 2 lines

memory size is increased according to the number of DMAC. And Samsung has the DMAC that is dedicated for Mem-to-Mem

operation. If I

Quoted 1 line

Quoted 1 line

make 'cyclic_task' be part of dma_pl330_chan, this DMAC that is

dedicated

Quoted 4 lines

Quoted 2 lines

for Mem-to-Mem operation should hold unused data.
So, I think it's loss that all dma channels hold own 'cyclic_task'.

Could you re-use the tasklet that already exists?

--
~Vinod Koul
Intel Corp.

Sent: Monday, July 25, 2011 7:36 PM
To: Boojin Kim
Cc: linux-arm-kernel at lists.infradead.org; linux-samsung- soc at vger.kernel.org; 'Kukjin Kim'; 'Vinod Koul'; 'Jassi Brar'; 'Grant Likely'; 'Mark Brown'; 'Dan Williams'
Subject: Re: [PATCH V4 04/14] DMA: PL330: Add DMA_CYCLIC capability

Quoted 10 lines · Boojin Kim

Quoted 10 lines · Boojin Kim

Quoted 8 lines

drivers/dma/pl330.c +8 -0

+static void pl330_tasklet_cyclic(unsigned long data)
+{
+	struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data;
+	struct dma_pl330_desc *desc, *_dt;
+	unsigned long flags;
+	LIST_HEAD(list);
+
+	spin_lock_irqsave(&pch->lock, flags);

...

Quoted 5 lines

Quoted 5 lines

drivers/dma/pl330.c static inline void fill_queue(struct dma_pl330_chan *pch) · line 220

Quoted 3 lines

drivers/dma/pl330.c +3 -0

+			callback = desc->txd.callback;
+			if (callback)
+				callback(desc->txd.callback_param);

On this again - what if the callback wants to terminate the DMA

activity

Quoted 1 line

Quoted 1 line

because there's no more audio data to be sent/received from the

device?

Quoted 2 lines

Do you mean what is happened if the callback() is called after

channel is

Quoted 2 lines

terminated ?
Or What is happened if Callback() calls 'dma_release_channel()' to

terminate

Quoted 1 line

DMA?

No. I mean what if the callback wants to call
dmaengine_terminate_all().

Quoted 4 lines

Quoted 4 lines

drivers/dma/pl330.c static inline int get_burst_len(struct dma_pl330_desc *desc, size_t len)

Quoted 4 lines

drivers/dma/pl330.c +3 -0

+	if (!pch->cyclic_task) {
+		pch->cyclic_task =
+			kmalloc(sizeof(struct tasklet_struct),

GFP_KERNEL);

Quoted 4 lines

Quoted 4 lines

drivers/dma/pl330.c static inline int get_burst_len(struct dma_pl330_desc *desc, size_t len) · line 649

Quoted 2 lines

drivers/dma/pl330.c +2 -0

+		tasklet_init(pch->cyclic_task,
+			pl330_tasklet_cyclic, (unsigned int)pch);

Here you allocate memory for the cyclic task. Above you set this

pointer

Quoted 1 line

Quoted 1 line

to NULL. That sounds like a memory leak to me. Why are you

kmallocing

Quoted 1 line

Quoted 1 line

this memory - why can't it be part of the dma_pl330_chan structure?

It's

Quoted 6 lines

Quoted 1 line

only 28 bytes.

It's my mistake. I should have been free of the memory.

And the reason why I use kmalloc for 'cyclic_task' is following. This memory size for 'cyclic_tasklet' is the 896 bytes ( = the

number of

Quoted 1 line

channel * sizeof(struct tasklet_struct)= 32*28) for each DMAC. And

This

Quoted 2 lines

memory size is increased according to the number of DMAC. And Samsung has the DMAC that is dedicated for Mem-to-Mem operation.

If I

Quoted 1 line

make 'cyclic_task' be part of dma_pl330_chan, this DMAC that is

dedicated

Quoted 2 lines

for Mem-to-Mem operation should hold unused data.
So, I think it's loss that all dma channels hold own 'cyclic_task'.

Could you re-use the tasklet that already exists?

This patch adds DMA_CYCLIC capability that is used for audio driver. DMA driver with DMA_CYCLIC capability reuses the dma requests that were submitted through tx_submit().

Signed-off-by Boojin Kim <[email protected]>

---
?drivers/dma/pl330.c | ?111 +++++++++++++++++++++++++++++++++++++++++++++++++++ ?1 files changed, 111 insertions(+), 0 deletions(-)

drivers/dma/pl330.c +0 -0

index 880f010..121c75a 100644
@@ -69,6 +69,11 @@ struct dma_pl330_chan {

? ? ? ? * NULL if the channel is available to be acquired. ? ? ? ? */
? ? ? ?void *pl330_chid;

drivers/dma/pl330.c +5 -0

+
+ ? ? ? /* taks for cyclic capability */
+ ? ? ? struct tasklet_struct *cyclic_task;
+
+ ? ? ? bool cyclic;

?};

drivers/dma/pl330.c +24 -0

+static void pl330_tasklet_cyclic(unsigned long data)
+{
+ ? ? ? struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data;
+ ? ? ? struct dma_pl330_desc *desc, *_dt;
+ ? ? ? unsigned long flags;
+ ? ? ? LIST_HEAD(list);
+
+ ? ? ? spin_lock_irqsave(&pch->lock, flags);
+
+ ? ? ? /* Pick up ripe tomatoes */
+ ? ? ? list_for_each_entry_safe(desc, _dt, &pch->work_list, node)
+ ? ? ? ? ? ? ? if (desc->status == DONE) {
+ ? ? ? ? ? ? ? ? ? ? ? dma_async_tx_callback callback;
+
+ ? ? ? ? ? ? ? ? ? ? ? list_move_tail(&desc->node, &pch->work_list);
+ ? ? ? ? ? ? ? ? ? ? ? pch->completed = desc->txd.cookie;
+
+ ? ? ? ? ? ? ? ? ? ? ? desc->status = PREP;
+
+ ? ? ? ? ? ? ? ? ? ? ? /* Try to submit a req imm.
+ ? ? ? ? ? ? ? ? ? ? ? next to the last completed cookie */
+ ? ? ? ? ? ? ? ? ? ? ? fill_queue(pch);
+
+ ? ? ? ? ? ? ? ? ? ? ? /* Make sure the PL330 Channel thread is active */

drivers/dma/pl330.c +1 -0

@@ -316,6 +359,15 @@ static void pl330_free_chan_resources(struct dma_chan *chan)
 ? ? ? ?pl330_release_channel(pch->pl330_chid);
 ? ? ? ?pch->pl330_chid = NULL;
 
+ ? ? ? if (pch->cyclic) {

Sent: Monday, July 25, 2011 8:24 PM
To: Boojin Kim
Cc: linux-arm-kernel at lists.infradead.org; linux-samsung- soc at vger.kernel.org; Vinod Koul; Dan Williams; Kukjin Kim; Grant Likely; Mark Brown
Subject: Re: [PATCH V4 04/14] DMA: PL330: Add DMA_CYCLIC capability

Quoted 7 lines · Boojin Kim

This patch adds DMA_CYCLIC capability that is used for audio driver. DMA driver with DMA_CYCLIC capability reuses the dma requests that were submitted through tx_submit().

Signed-off-by Boojin Kim <[email protected]>

---
 drivers/dma/pl330.c |  111

+++++++++++++++++++++++++++++++++++++++++++++++++++

Quoted 11 lines

 1 files changed, 111 insertions(+), 0 deletions(-)

drivers/dma/pl330.c +1 -0

index 880f010..121c75a 100644
@@ -69,6 +69,11 @@ struct dma_pl330_chan {
        * NULL if the channel is available to be acquired.
        */
       void *pl330_chid;
+

drivers/dma/pl330.c struct dma_pl330_chan { · line 77

Quoted 5 lines

drivers/dma/pl330.c +4 -0

+       /* taks for cyclic capability */
+       struct tasklet_struct *cyclic_task;
+
+       bool cyclic;
 };

We already have a tasklet_struct member here.
This 'cyclic' flag can indicate if we are doing cyclic or serial transfers. So, this cyclic_task seems unnecessary.

Quoted 15 lines

drivers/dma/pl330.c +14 -0

+static void pl330_tasklet_cyclic(unsigned long data)
+{
+       struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data;
+       struct dma_pl330_desc *desc, *_dt;
+       unsigned long flags;
+       LIST_HEAD(list);
+
+       spin_lock_irqsave(&pch->lock, flags);
+
+       /* Pick up ripe tomatoes */
+       list_for_each_entry_safe(desc, _dt, &pch->work_list, node)
+               if (desc->status == DONE) {
+                       dma_async_tx_callback callback;
+

drivers/dma/pl330.c static inline void fill_queue(struct dma_pl330_chan *pch)

Quoted 10 lines

drivers/dma/pl330.c +10 -0

+                       list_move_tail(&desc->node, &pch->work_list);
+                       pch->completed = desc->txd.cookie;
+
+                       desc->status = PREP;
+
+                       /* Try to submit a req imm.
+                       next to the last completed cookie */
+                       fill_queue(pch);
+
+                       /* Make sure the PL330 Channel thread is active

*/

Quoted 1 line

+ pl330_chan_ctrl(pch->pl330_chid,

PL330_OP_START);

drivers/dma/pl330.c static inline void fill_queue(struct dma_pl330_chan *pch) · line 225

Quoted 9 lines

drivers/dma/pl330.c +9 -0

+
+                       callback = desc->txd.callback;
+                       if (callback)
+                               callback(desc->txd.callback_param);
+
+               }
+
+       spin_unlock_irqrestore(&pch->lock, flags);
+}

Please merge this with pl330_tasklet and use 'cyclic' flag to differentiate.

drivers/dma/pl330.c static void dma_pl330_rqcb(void *token, enum

Quoted 1 line

drivers/dma/pl330.c +0 -0

@@ -227,6 +267,9 @@ static void dma_pl330_rqcb(void *token, enum

pl330_op_err err)

drivers/dma/pl330.c static void dma_pl330_rqcb(void *token, enum pl330_op_err err) · line 273

Quoted 7 lines

drivers/dma/pl330.c +3 -0

       spin_unlock_irqrestore(&pch->lock, flags);
 
+       if (pch->cyclic_task)
+               tasklet_schedule(pch->cyclic_task);
+       else
       tasklet_schedule(&pch->task);

A tab here please, and check for 'cyclic' flag.

drivers/dma/pl330.c static void pl330_free_chan_resources(struct

Quoted 1 line

drivers/dma/pl330.c +0 -0

@@ -316,6 +359,15 @@ static void pl330_free_chan_resources(struct

dma_chan *chan)

Quoted 5 lines

drivers/dma/pl330.c +2 -0

       pl330_release_channel(pch->pl330_chid);
       pch->pl330_chid = NULL;
 
+       if (pch->cyclic) {
+               pch->cyclic = false;

drivers/dma/pl330.c static void pl330_free_chan_resources(struct dma_chan *chan) · line 372

Quoted 10 lines

drivers/dma/pl330.c +7 -0

+               list_splice_tail_init(&pch->work_list, &pch->dmac-
 desc_pool);
+               if (pch->cyclic_task) {
+                       tasklet_kill(pch->cyclic_task);
+                       pch->cyclic_task = NULL;
+               }
+       }
+
       spin_unlock_irqrestore(&pch->lock, flags);
 }

To be explicit, please initialize 'cyclic' flag to false in pl330_alloc_chan_resources

This patch adds to support PL330-DMA driver on DMADEVICE for S5P SoCs.

Signed-off-by Boojin Kim <[email protected]>

Signed-off-by Kukjin Kim <[email protected]>

---
 arch/arm/mach-exynos4/include/mach/dma.h           |    4 +-
 arch/arm/mach-s5p64x0/include/mach/dma.h           |    2 +-
 arch/arm/mach-s5pc100/include/mach/dma.h           |    2 +-
 arch/arm/mach-s5pv210/include/mach/dma.h           |    2 +-
 arch/arm/plat-samsung/Kconfig                      |    8 +++++++
 .../include/plat/{s3c-dma-pl330.h => dma-pl330.h}  |   21 +++++++++++++------
 .../plat-samsung/include/plat/s3c-pl330-pdata.h    |    2 +-
 7 files changed, 28 insertions(+), 13 deletions(-)
 rename arch/arm/plat-samsung/include/plat/{s3c-dma-pl330.h => dma-pl330.h} (84%)

arch/arm/mach-exynos4/include/mach/dma.h +2 -2

index 81209eb..201842a 100644
@@ -20,7 +20,7 @@
 #ifndef __MACH_DMA_H
 #define __MACH_DMA_H
 
-/* This platform uses the common S3C DMA API driver for PL330 */
-#include <plat/s3c-dma-pl330.h>
+/* This platform uses the common DMA API driver for PL330 */
+#include <plat/dma-pl330.h>
 
 #endif /* __MACH_DMA_H */

arch/arm/mach-s5p64x0/include/mach/dma.h +1 -1

index 81209eb..1beae2c 100644
@@ -21,6 +21,6 @@
 #define __MACH_DMA_H
 
 /* This platform uses the common S3C DMA API driver for PL330 */
-#include <plat/s3c-dma-pl330.h>
+#include <plat/dma-pl330.h>
 
 #endif /* __MACH_DMA_H */

arch/arm/mach-s5pc100/include/mach/dma.h +1 -1

index 81209eb..1beae2c 100644
@@ -21,6 +21,6 @@
 #define __MACH_DMA_H
 
 /* This platform uses the common S3C DMA API driver for PL330 */
-#include <plat/s3c-dma-pl330.h>
+#include <plat/dma-pl330.h>
 
 #endif /* __MACH_DMA_H */

arch/arm/mach-s5pv210/include/mach/dma.h +1 -1

index 81209eb..1beae2c 100644
@@ -21,6 +21,6 @@
 #define __MACH_DMA_H
 
 /* This platform uses the common S3C DMA API driver for PL330 */
-#include <plat/s3c-dma-pl330.h>
+#include <plat/dma-pl330.h>
 
 #endif /* __MACH_DMA_H */

arch/arm/plat-samsung/Kconfig +8 -0

index 4d79519..cb170a6 100644
@@ -300,6 +300,14 @@ config S3C_PL330_DMA
 	help
 	  S3C DMA API Driver for PL330 DMAC.
 
+config DMADEV_PL330
+	bool
+	select DMADEVICES
+	select PL330_DMA
+	select ARM_AMBA
+	help
+	  Use DMA device engine for PL330 DMAC.
+
 comment "Power management"
 
 config SAMSUNG_PM_DEBUG

arch/arm/plat-samsung/include/plat/dma-pl330.h renamed from arch/arm/plat-samsung/include/plat/s3c-dma-pl330.h +0 -0

similarity index 84%
rename from arch/arm/plat-samsung/include/plat/s3c-dma-pl330.h
rename to arch/arm/plat-samsung/include/plat/dma-pl330.h
index 8107442..c402719 100644
@@ -8,19 +8,18 @@
  * (at your option) any later version.
  */
 

arch/arm/plat-samsung/include/plat/dma-pl330.h renamed from arch/arm/plat-samsung/include/plat/s3c-dma-pl330.h +10 -2

 /*
  * PL330 can assign any channel to communicate with
  * any of the peripherals attched to the DMAC.
  * For the sake of consistency across client drivers,
  * We keep the channel names unchanged and only add
  * missing peripherals are added.
- * Order is not important since S3C PL330 API driver
+ * Order is not important since DMA PL330 API driver
  * use these just as IDs.
  */
 enum dma_ch {

@@ -84,6 +83,14 @@ enum dma_ch {
 	DMACH_SLIMBUS4_TX,
 	DMACH_SLIMBUS5_RX,
 	DMACH_SLIMBUS5_TX,
+	DMACH_MTOM_0,
+	DMACH_MTOM_1,
+	DMACH_MTOM_2,
+	DMACH_MTOM_3,
+	DMACH_MTOM_4,
+	DMACH_MTOM_5,
+	DMACH_MTOM_6,
+	DMACH_MTOM_7,
 	/* END Marker, also used to denote a reserved channel */
 	DMACH_MAX,
 };

@@ -95,4 +102,4 @@ static inline bool s3c_dma_has_circular(void)
 
 #include <plat/dma.h>
 
-#endif	/* __S3C_DMA_PL330_H_ */
+#endif	/* __DMA_PL330_H_ */

arch/arm/plat-samsung/include/plat/s3c-pl330-pdata.h +1 -1

index bf5e2a9..64fdf66 100644
@@ -12,7 +12,7 @@
 #ifndef __S3C_PL330_PDATA_H
 #define __S3C_PL330_PDATA_H
 
-#include <plat/s3c-dma-pl330.h>
+#include <plat/dma-pl330.h>
 
 /*
  * Every PL330 DMAC has max 32 peripheral interfaces,

arch/arm/plat-samsung/Kconfig +0 -0

index 4d79519..cb170a6 100644
@@ -300,6 +300,14 @@ config S3C_PL330_DMA

? ? ? ?help
? ? ? ? ?S3C DMA API Driver for PL330 DMAC.

arch/arm/plat-samsung/include/plat/dma-pl330.h renamed from arch/arm/plat-samsung/include/plat/s3c-dma-pl330.h +0 -0

similarity index 84%
rename from arch/arm/plat-samsung/include/plat/s3c-dma-pl330.h
rename to arch/arm/plat-samsung/include/plat/dma-pl330.h
index 8107442..c402719 100644
@@ -8,19 +8,18 @@

?* (at your option) any later version.
?*/

arch/arm/plat-samsung/include/plat/dma-pl330.h renamed from arch/arm/plat-samsung/include/plat/s3c-dma-pl330.h +8 -0

@@ -84,6 +83,14 @@ enum dma_ch {
 ? ? ? ?DMACH_SLIMBUS4_TX,
 ? ? ? ?DMACH_SLIMBUS5_RX,
 ? ? ? ?DMACH_SLIMBUS5_TX,
+ ? ? ? DMACH_MTOM_0,
+ ? ? ? DMACH_MTOM_1,
+ ? ? ? DMACH_MTOM_2,
+ ? ? ? DMACH_MTOM_3,
+ ? ? ? DMACH_MTOM_4,
+ ? ? ? DMACH_MTOM_5,
+ ? ? ? DMACH_MTOM_6,
+ ? ? ? DMACH_MTOM_7,

? ? ? ?/* END Marker, also used to denote a reserved channel */ ? ? ? ?DMACH_MAX,
?};