* [PATCH V4 00/13] To use DMA generic APIs for Samsung DMA
@ 2011-07-20 10:46 ` Boojin Kim
0 siblings, 0 replies; 52+ messages in thread
From: Boojin Kim @ 2011-07-20 10:46 UTC (permalink / raw)
To: linux-arm-kernel, linux-samsung-soc
Cc: Vinod Koul, Dan Williams, Jassi Brar, Kukjin Kim
This patchset adds support DMA generic APIs for Samsung DMA.
V4 doesn't has whole patch series and only has the patches differed from V3.
Changes from V3:
- Divided 03-patch into two patchs.
- First is 03-1-patch for DMA_SLAVE_CONFIG command.
- Another is 03-2-patch for DMA_CYCLIC capability.
- Code clean-up
- Remove unnecessary variable referance.
- Remove redunancy code
[PATCH V4 01/13] DMA: PL330: Add support runtime PM for PL330 DMAC
[PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
[PATCH V4 03-2/13] DMA: PL330: Add DMA_CYCLIC capability
[PATCH V4 05/13] ARM: SAMSUNG: Add common DMA operations
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 00/13] To use DMA generic APIs for Samsung DMA
@ 2011-07-20 10:46 ` Boojin Kim
0 siblings, 0 replies; 52+ messages in thread
From: Boojin Kim @ 2011-07-20 10:46 UTC (permalink / raw)
To: linux-arm-kernel
This patchset adds support DMA generic APIs for Samsung DMA.
V4 doesn't has whole patch series and only has the patches differed from V3.
Changes from V3:
- Divided 03-patch into two patchs.
- First is 03-1-patch for DMA_SLAVE_CONFIG command.
- Another is 03-2-patch for DMA_CYCLIC capability.
- Code clean-up
- Remove unnecessary variable referance.
- Remove redunancy code
[PATCH V4 01/13] DMA: PL330: Add support runtime PM for PL330 DMAC
[PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
[PATCH V4 03-2/13] DMA: PL330: Add DMA_CYCLIC capability
[PATCH V4 05/13] ARM: SAMSUNG: Add common DMA operations
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 01/13] DMA: PL330: Add support runtime PM for PL330 DMAC
2011-07-20 10:46 ` Boojin Kim
@ 2011-07-20 10:46 ` Boojin Kim
-1 siblings, 0 replies; 52+ messages in thread
From: Boojin Kim @ 2011-07-20 10:46 UTC (permalink / raw)
To: linux-arm-kernel, linux-samsung-soc
Cc: Vinod Koul, Dan Williams, Jassi Brar, Kukjin Kim, Boojin Kim
Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
Cc: Vinod Koul <vinod.koul@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
---
drivers/dma/pl330.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 73 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 6abe1ec..b7ecf47 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -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,
+};
+
static struct amba_driver pl330_driver = {
.drv = {
.owner = THIS_MODULE,
.name = "dma-pl330",
+ .pm = &pl330_pm_ops,
},
.id_table = pl330_ids,
.probe = pl330_probe,
--
1.7.1
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH V4 01/13] DMA: PL330: Add support runtime PM for PL330 DMAC
@ 2011-07-20 10:46 ` Boojin Kim
0 siblings, 0 replies; 52+ messages in thread
From: Boojin Kim @ 2011-07-20 10:46 UTC (permalink / raw)
To: linux-arm-kernel
Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
Cc: Vinod Koul <vinod.koul@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
---
drivers/dma/pl330.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 73 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 6abe1ec..b7ecf47 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -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,
+};
+
static struct amba_driver pl330_driver = {
.drv = {
.owner = THIS_MODULE,
.name = "dma-pl330",
+ .pm = &pl330_pm_ops,
},
.id_table = pl330_ids,
.probe = pl330_probe,
--
1.7.1
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
2011-07-20 10:46 ` Boojin Kim
@ 2011-07-20 10:46 ` Boojin Kim
-1 siblings, 0 replies; 52+ messages in thread
From: Boojin Kim @ 2011-07-20 10:46 UTC (permalink / raw)
To: linux-arm-kernel, linux-samsung-soc
Cc: Vinod Koul, Dan Williams, Jassi Brar, Kukjin Kim, Boojin Kim
Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
---
drivers/dma/pl330.c | 53 +++++++++++++++++++++++++++++++++++++-------------
1 files changed, 39 insertions(+), 14 deletions(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 586ab39..880f010 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -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);
+ 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;
+ if (slave_config->dst_addr_width)
+ peri->burst_sz = __ffs(slave_config->dst_addr_width);
+ } 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);
+ }
+ break;
+ default:
+ dev_err(pch->dmac->pif.dev, "Not supported command.\n");
+ return -ENXIO;
+ }
return 0;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
@ 2011-07-20 10:46 ` Boojin Kim
0 siblings, 0 replies; 52+ messages in thread
From: Boojin Kim @ 2011-07-20 10:46 UTC (permalink / raw)
To: linux-arm-kernel
Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
---
drivers/dma/pl330.c | 53 +++++++++++++++++++++++++++++++++++++-------------
1 files changed, 39 insertions(+), 14 deletions(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 586ab39..880f010 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -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);
+ 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;
+ if (slave_config->dst_addr_width)
+ peri->burst_sz = __ffs(slave_config->dst_addr_width);
+ } 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);
+ }
+ break;
+ default:
+ dev_err(pch->dmac->pif.dev, "Not supported command.\n");
+ return -ENXIO;
+ }
return 0;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH V4 03-2/13] DMA: PL330: Add DMA_CYCLIC capability
2011-07-20 10:46 ` Boojin Kim
@ 2011-07-20 10:46 ` Boojin Kim
-1 siblings, 0 replies; 52+ messages in thread
From: Boojin Kim @ 2011-07-20 10:46 UTC (permalink / raw)
To: linux-arm-kernel, linux-samsung-soc
Cc: Vinod Koul, Dan Williams, Jassi Brar, Kukjin Kim, Boojin Kim
This patch adds DMA_CYCLIC capability that is used for audio driver.
DMA driver activated with it reuses the dma requests that were submitted
through tx_submit().
Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
---
drivers/dma/pl330.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 111 insertions(+), 0 deletions(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 880f010..121c75a 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -69,6 +69,11 @@ struct dma_pl330_chan {
* NULL if the channel is available to be acquired.
*/
void *pl330_chid;
+
+ /* taks for cyclic capability */
+ struct tasklet_struct *cyclic_task;
+
+ bool cyclic;
};
struct dma_pl330_dmac {
@@ -184,6 +189,41 @@ static inline void fill_queue(struct dma_pl330_chan *pch)
}
}
+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 */
+ pl330_chan_ctrl(pch->pl330_chid, PL330_OP_START);
+
+ callback = desc->txd.callback;
+ if (callback)
+ callback(desc->txd.callback_param);
+
+ }
+
+ 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)
spin_unlock_irqrestore(&pch->lock, flags);
+ if (pch->cyclic_task)
+ tasklet_schedule(pch->cyclic_task);
+ else
tasklet_schedule(&pch->task);
}
@@ -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)
+{
+ 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;
+
+ pch = to_pchan(chan);
+ if (!pch)
+ return NULL;
+
+ 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 =
+ kmalloc(sizeof(struct tasklet_struct), GFP_KERNEL);
+ tasklet_init(pch->cyclic_task,
+ pl330_tasklet_cyclic, (unsigned int)pch);
+ }
+
+ pch->cyclic = true;
+
+ fill_px(&desc->px, dst, src, period_len);
+
+ return &desc->txd;
+}
+
static struct dma_async_tx_descriptor *
pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,
dma_addr_t src, size_t len, unsigned long flags)
@@ -780,6 +889,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
case MEMTODEV:
case DEVTOMEM:
dma_cap_set(DMA_SLAVE, pd->cap_mask);
+ dma_cap_set(DMA_CYCLIC, pd->cap_mask);
break;
default:
dev_err(&adev->dev, "DEVTODEV Not Supported\n");
@@ -805,6 +915,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
pd->device_alloc_chan_resources = pl330_alloc_chan_resources;
pd->device_free_chan_resources = pl330_free_chan_resources;
pd->device_prep_dma_memcpy = pl330_prep_dma_memcpy;
+ pd->device_prep_dma_cyclic = pl330_prep_dma_cyclic;
pd->device_tx_status = pl330_tx_status;
pd->device_prep_slave_sg = pl330_prep_slave_sg;
pd->device_control = pl330_control;
--
1.7.1
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH V4 03-2/13] DMA: PL330: Add DMA_CYCLIC capability
@ 2011-07-20 10:46 ` Boojin Kim
0 siblings, 0 replies; 52+ messages in thread
From: Boojin Kim @ 2011-07-20 10:46 UTC (permalink / raw)
To: linux-arm-kernel
This patch adds DMA_CYCLIC capability that is used for audio driver.
DMA driver activated with it reuses the dma requests that were submitted
through tx_submit().
Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
---
drivers/dma/pl330.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 111 insertions(+), 0 deletions(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 880f010..121c75a 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -69,6 +69,11 @@ struct dma_pl330_chan {
* NULL if the channel is available to be acquired.
*/
void *pl330_chid;
+
+ /* taks for cyclic capability */
+ struct tasklet_struct *cyclic_task;
+
+ bool cyclic;
};
struct dma_pl330_dmac {
@@ -184,6 +189,41 @@ static inline void fill_queue(struct dma_pl330_chan *pch)
}
}
+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 */
+ pl330_chan_ctrl(pch->pl330_chid, PL330_OP_START);
+
+ callback = desc->txd.callback;
+ if (callback)
+ callback(desc->txd.callback_param);
+
+ }
+
+ 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)
spin_unlock_irqrestore(&pch->lock, flags);
+ if (pch->cyclic_task)
+ tasklet_schedule(pch->cyclic_task);
+ else
tasklet_schedule(&pch->task);
}
@@ -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)
+{
+ 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;
+
+ pch = to_pchan(chan);
+ if (!pch)
+ return NULL;
+
+ 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 =
+ kmalloc(sizeof(struct tasklet_struct), GFP_KERNEL);
+ tasklet_init(pch->cyclic_task,
+ pl330_tasklet_cyclic, (unsigned int)pch);
+ }
+
+ pch->cyclic = true;
+
+ fill_px(&desc->px, dst, src, period_len);
+
+ return &desc->txd;
+}
+
static struct dma_async_tx_descriptor *
pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,
dma_addr_t src, size_t len, unsigned long flags)
@@ -780,6 +889,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
case MEMTODEV:
case DEVTOMEM:
dma_cap_set(DMA_SLAVE, pd->cap_mask);
+ dma_cap_set(DMA_CYCLIC, pd->cap_mask);
break;
default:
dev_err(&adev->dev, "DEVTODEV Not Supported\n");
@@ -805,6 +915,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
pd->device_alloc_chan_resources = pl330_alloc_chan_resources;
pd->device_free_chan_resources = pl330_free_chan_resources;
pd->device_prep_dma_memcpy = pl330_prep_dma_memcpy;
+ pd->device_prep_dma_cyclic = pl330_prep_dma_cyclic;
pd->device_tx_status = pl330_tx_status;
pd->device_prep_slave_sg = pl330_prep_slave_sg;
pd->device_control = pl330_control;
--
1.7.1
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH V4 05/13] ARM: SAMSUNG: Add common DMA operations
2011-07-20 10:46 ` Boojin Kim
@ 2011-07-20 10:46 ` Boojin Kim
-1 siblings, 0 replies; 52+ messages in thread
From: Boojin Kim @ 2011-07-20 10:46 UTC (permalink / raw)
To: linux-arm-kernel, linux-samsung-soc
Cc: Vinod Koul, Dan Williams, Jassi Brar, Kukjin Kim, Boojin Kim
This patch adds common DMA operations which are used for Samsung DMA
drivers. Currently there are two types of DMA driver for Samsung SoCs.
The one is S3C-DMA for S3C SoCs and the other is PL330-DMA for S5P SoCs.
This patch provides funcion pointers for common DMA operations to DMA
client driver like SPI and Audio. It makes DMA client drivers support
multi-platform.
In addition, this common DMA operations implement the shared actions
that are needed for DMA client driver. For example shared actions are
filter() function for dma_request_channel() and parameter passing for
device_prep_slave_sg().
Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
---
arch/arm/mach-s3c2410/include/mach/dma.h | 8 ++-
arch/arm/mach-s3c64xx/include/mach/dma.h | 4 +
arch/arm/plat-samsung/Makefile | 6 +-
arch/arm/plat-samsung/dma-ops.c | 133 ++++++++++++++++++++++++
arch/arm/plat-samsung/include/plat/dma-ops.h | 64 +++++++++++
arch/arm/plat-samsung/include/plat/dma-pl330.h | 4 +
arch/arm/plat-samsung/include/plat/dma.h | 1 +
arch/arm/plat-samsung/s3c-dma-ops.c | 133 ++++++++++++++++++++++++
8 files changed, 350 insertions(+), 3 deletions(-)
create mode 100644 arch/arm/plat-samsung/dma-ops.c
create mode 100644 arch/arm/plat-samsung/include/plat/dma-ops.h
create mode 100644 arch/arm/plat-samsung/s3c-dma-ops.c
diff --git a/arch/arm/mach-s3c2410/include/mach/dma.h b/arch/arm/mach-s3c2410/include/mach/dma.h
index b2b2a5b..e61a91f 100644
--- a/arch/arm/mach-s3c2410/include/mach/dma.h
+++ b/arch/arm/mach-s3c2410/include/mach/dma.h
@@ -13,7 +13,6 @@
#ifndef __ASM_ARCH_DMA_H
#define __ASM_ARCH_DMA_H __FILE__
-#include <plat/dma.h>
#include <linux/sysdev.h>
#define MAX_DMA_TRANSFER_SIZE 0x100000 /* Data Unit is half word */
@@ -51,6 +50,13 @@ enum dma_ch {
DMACH_MAX, /* the end entry */
};
+static inline bool samsung_dma_is_dmadev(void)
+{
+ return false;
+}
+
+#include <plat/dma.h>
+
#define DMACH_LOW_LEVEL (1<<28) /* use this to specifiy hardware ch no */
/* we have 4 dma channels */
diff --git a/arch/arm/mach-s3c64xx/include/mach/dma.h b/arch/arm/mach-s3c64xx/include/mach/dma.h
index 0a5d926..49c3a53 100644
--- a/arch/arm/mach-s3c64xx/include/mach/dma.h
+++ b/arch/arm/mach-s3c64xx/include/mach/dma.h
@@ -63,6 +63,10 @@ static __inline__ bool s3c_dma_has_circular(void)
return true;
}
+static inline bool samsung_dma_is_dmadev(void)
+{
+ return false;
+}
#define S3C2410_DMAF_CIRCULAR (1 << 0)
#include <plat/dma.h>
diff --git a/arch/arm/plat-samsung/Makefile b/arch/arm/plat-samsung/Makefile
index 53eb15b..9ecf2aa 100644
--- a/arch/arm/plat-samsung/Makefile
+++ b/arch/arm/plat-samsung/Makefile
@@ -62,9 +62,11 @@ obj-$(CONFIG_SAMSUNG_DEV_PWM) += dev-pwm.o
# DMA support
-obj-$(CONFIG_S3C_DMA) += dma.o
+obj-$(CONFIG_S3C_DMA) += dma.o s3c-dma-ops.o
-obj-$(CONFIG_S3C_PL330_DMA) += s3c-pl330.o
+obj-$(CONFIG_DMADEV_PL330) += dma-ops.o
+
+obj-$(CONFIG_S3C_PL330_DMA) += s3c-pl330.o s3c-dma-ops.o
# PM support
diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
new file mode 100644
index 0000000..9053433
--- /dev/null
+++ b/arch/arm/plat-samsung/dma-ops.c
@@ -0,0 +1,133 @@
+/* linux/arch/arm/plat-samsung/dma-ops.c
+ *
+ * Copyright (c) 2011 Samsung Electronics Co., Ltd.
+ * http://www.samsung.com
+ *
+ * Samsung DMA Operations
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/amba/pl330.h>
+
+#include <mach/dma.h>
+
+static bool pl330_filter(struct dma_chan *chan, void *param)
+{
+ struct dma_pl330_peri *peri = (struct dma_pl330_peri *)chan->private;
+ unsigned dma_ch = (unsigned)param;
+
+ if (peri->peri_id != dma_ch)
+ return false;
+
+ return true;
+}
+
+static unsigned samsung_dmadev_request(enum dma_ch dma_ch,
+ struct samsung_dma_info *info)
+{
+ struct dma_chan *chan;
+ dma_cap_mask_t mask;
+ struct dma_slave_config slave_config;
+
+ dma_cap_zero(mask);
+ dma_cap_set(info->cap, mask);
+
+ chan = dma_request_channel(mask, pl330_filter, (void *)dma_ch);
+
+ if (info->direction == DMA_FROM_DEVICE) {
+ memset(&slave_config, 0, sizeof(struct dma_slave_config));
+ slave_config.direction = info->direction;
+ slave_config.src_addr = info->fifo;
+ slave_config.src_addr_width = info->width;
+ dmaengine_slave_config(chan, &slave_config);
+ } else if (info->direction == DMA_TO_DEVICE) {
+ memset(&slave_config, 0, sizeof(struct dma_slave_config));
+ slave_config.direction = info->direction;
+ slave_config.dst_addr = info->fifo;
+ slave_config.dst_addr_width = info->width;
+ dmaengine_slave_config(chan, &slave_config);
+ }
+
+ return (unsigned)chan;
+}
+
+static int samsung_dmadev_release(unsigned ch,
+ struct s3c2410_dma_client *client)
+{
+ dma_release_channel((struct dma_chan *)ch);
+
+ return 0;
+}
+
+static int samsung_dmadev_prepare(unsigned ch,
+ struct samsung_dma_prep_info *info)
+{
+ struct scatterlist sg;
+ struct dma_chan *chan = (struct dma_chan *)ch;
+ struct dma_async_tx_descriptor *desc;
+
+ switch (info->cap) {
+ case DMA_SLAVE:
+ sg_init_table(&sg, 1);
+ sg_dma_len(&sg) = info->len;
+ sg_set_page(&sg, pfn_to_page(PFN_DOWN(info->buf)),
+ info->len, offset_in_page(info->buf));
+ sg_dma_address(&sg) = info->buf;
+
+ desc = chan->device->device_prep_slave_sg(chan,
+ &sg, 1, info->direction, DMA_PREP_INTERRUPT);
+ break;
+ case DMA_CYCLIC:
+ desc = chan->device->device_prep_dma_cyclic(chan,
+ info->buf, info->len, info->period, info->direction);
+ break;
+ default:
+ dev_err(&chan->dev->device, "unsupported format\n");
+ return -EFAULT;
+ }
+
+ if (!desc) {
+ dev_err(&chan->dev->device, "cannot prepare cyclic dma\n");
+ return -EFAULT;
+ }
+
+ desc->callback = info->fp;
+ desc->callback_param = info->fp_param;
+
+ dmaengine_submit((struct dma_async_tx_descriptor *)desc);
+
+ return 0;
+}
+
+static inline int samsung_dmadev_trigger(unsigned ch)
+{
+ dma_async_issue_pending((struct dma_chan *)ch);
+
+ return 0;
+}
+
+static inline int samsung_dmadev_flush(unsigned ch)
+{
+ return dmaengine_terminate_all((struct dma_chan *)ch);
+}
+
+struct samsung_dma_ops dmadev_ops = {
+ .request = samsung_dmadev_request,
+ .release = samsung_dmadev_release,
+ .prepare = samsung_dmadev_prepare,
+ .trigger = samsung_dmadev_trigger,
+ .started = NULL,
+ .flush = samsung_dmadev_flush,
+ .stop = samsung_dmadev_flush,
+};
+
+void *samsung_dmadev_get_ops(void)
+{
+ return &dmadev_ops;
+}
+EXPORT_SYMBOL(samsung_dmadev_get_ops);
diff --git a/arch/arm/plat-samsung/include/plat/dma-ops.h b/arch/arm/plat-samsung/include/plat/dma-ops.h
new file mode 100644
index 0000000..e5a68c0
--- /dev/null
+++ b/arch/arm/plat-samsung/include/plat/dma-ops.h
@@ -0,0 +1,64 @@
+/* arch/arm/plat-samsung/include/plat/dma-ops.h
+ *
+ * Copyright (c) 2011 Samsung Electronics Co., Ltd.
+ * http://www.samsung.com
+ *
+ * Samsung DMA support
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __SAMSUNG_DMA_OPS_H_
+#define __SAMSUNG_DMA_OPS_H_ __FILE__
+
+#include <linux/dmaengine.h>
+
+struct samsung_dma_prep_info {
+ enum dma_transaction_type cap;
+ enum dma_data_direction direction;
+ unsigned buf;
+ unsigned long period;
+ unsigned long len;
+ void (*fp)(void *data);
+ void *fp_param;
+};
+
+struct samsung_dma_info {
+ enum dma_transaction_type cap;
+ enum dma_data_direction direction;
+ enum dma_slave_buswidth width;
+ unsigned fifo;
+ struct s3c2410_dma_client *client;
+};
+
+struct samsung_dma_ops {
+ bool init;
+ unsigned (*request)(enum dma_ch ch, struct samsung_dma_info *info);
+ int (*release)(unsigned ch, struct s3c2410_dma_client *client);
+ int (*prepare)(unsigned ch, struct samsung_dma_prep_info *info);
+ int (*trigger)(unsigned ch);
+ int (*started)(unsigned ch);
+ int (*flush)(unsigned ch);
+ int (*stop)(unsigned ch);
+};
+
+extern void *samsung_dmadev_get_ops(void);
+extern void *s3c_dma_get_ops(void);
+
+static inline void *__samsung_dma_get_ops(void)
+{
+ if (samsung_dma_is_dmadev())
+ return samsung_dmadev_get_ops();
+ else
+ return s3c_dma_get_ops();
+}
+
+/*
+ * samsung_dma_get_ops
+ * get the set of samsung dma operations
+ */
+#define samsung_dma_get_ops() __samsung_dma_get_ops()
+
+#endif /* __SAMSUNG_DMA_OPS_H_ */
diff --git a/arch/arm/plat-samsung/include/plat/dma-pl330.h b/arch/arm/plat-samsung/include/plat/dma-pl330.h
index c402719..687489e 100644
--- a/arch/arm/plat-samsung/include/plat/dma-pl330.h
+++ b/arch/arm/plat-samsung/include/plat/dma-pl330.h
@@ -100,6 +100,10 @@ static inline bool s3c_dma_has_circular(void)
return true;
}
+static inline bool samsung_dma_is_dmadev(void)
+{
+ return true;
+}
#include <plat/dma.h>
#endif /* __DMA_PL330_H_ */
diff --git a/arch/arm/plat-samsung/include/plat/dma.h b/arch/arm/plat-samsung/include/plat/dma.h
index 2e8f8c6..e87c6be 100644
--- a/arch/arm/plat-samsung/include/plat/dma.h
+++ b/arch/arm/plat-samsung/include/plat/dma.h
@@ -125,3 +125,4 @@ extern int s3c2410_dma_set_opfn(unsigned int, s3c2410_dma_opfn_t rtn);
extern int s3c2410_dma_set_buffdone_fn(unsigned int, s3c2410_dma_cbfn_t rtn);
+#include <plat/dma-ops.h>
diff --git a/arch/arm/plat-samsung/s3c-dma-ops.c b/arch/arm/plat-samsung/s3c-dma-ops.c
new file mode 100644
index 0000000..33ab324
--- /dev/null
+++ b/arch/arm/plat-samsung/s3c-dma-ops.c
@@ -0,0 +1,133 @@
+/* linux/arch/arm/plat-samsung/s3c-dma-ops.c
+ *
+ * Copyright (c) 2011 Samsung Electronics Co., Ltd.
+ * http://www.samsung.com
+ *
+ * Samsung S3C-DMA Operations
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include <mach/dma.h>
+
+struct cb_data {
+ void (*fp) (void *);
+ void *fp_param;
+ unsigned ch;
+ struct list_head node;
+};
+
+static LIST_HEAD(dma_list);
+
+static void s3c_dma_cb(struct s3c2410_dma_chan *channel, void *param,
+ int size, enum s3c2410_dma_buffresult res)
+{
+ struct cb_data *data = param;
+
+ data->fp(data->fp_param);
+}
+
+static unsigned s3c_dma_request(enum dma_ch dma_ch,
+ struct samsung_dma_info *info)
+{
+ struct cb_data *data;
+
+ if (s3c2410_dma_request(dma_ch, info->client, NULL) < 0) {
+ s3c2410_dma_free(dma_ch, info->client);
+ return 0;
+ }
+
+ data = kzalloc(sizeof(struct cb_data), GFP_KERNEL);
+ data->ch = dma_ch;
+ list_add_tail(&data->node, &dma_list);
+
+ if (info->direction == DMA_FROM_DEVICE)
+ s3c2410_dma_devconfig(dma_ch, S3C2410_DMASRC_HW, info->fifo);
+ else
+ s3c2410_dma_devconfig(dma_ch, S3C2410_DMASRC_MEM, info->fifo);
+
+ if (info->cap == DMA_CYCLIC)
+ s3c2410_dma_setflags(dma_ch, S3C2410_DMAF_CIRCULAR);
+
+ s3c2410_dma_config(dma_ch, info->width);
+
+ return (unsigned)dma_ch;
+}
+
+static int s3c_dma_release(unsigned ch, struct s3c2410_dma_client *client)
+{
+ struct cb_data *data;
+
+ list_for_each_entry(data, &dma_list, node)
+ if (data->ch == ch)
+ break;
+ list_del(&data->node);
+
+ s3c2410_dma_free(ch, client);
+ kfree(data);
+
+ return 0;
+}
+
+static int s3c_dma_prepare(unsigned ch, struct samsung_dma_prep_info *info)
+{
+ struct cb_data *data;
+ int len = (info->cap == DMA_CYCLIC) ? info->period : info->len;
+
+ list_for_each_entry(data, &dma_list, node)
+ if (data->ch == ch)
+ break;
+
+ if (!data->fp) {
+ s3c2410_dma_set_buffdone_fn(ch, s3c_dma_cb);
+ data->fp = info->fp;
+ data->fp_param = info->fp_param;
+ }
+
+ s3c2410_dma_enqueue(ch, (void *)data, info->buf, len);
+
+ return 0;
+}
+
+static inline int s3c_dma_trigger(unsigned ch)
+{
+ return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_START);
+}
+
+static inline int s3c_dma_started(unsigned ch)
+{
+ return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STARTED);
+}
+
+static inline int s3c_dma_flush(unsigned ch)
+{
+ return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_FLUSH);
+}
+
+static inline int s3c_dma_stop(unsigned ch)
+{
+ return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STOP);
+}
+
+static struct samsung_dma_ops s3c_dma_ops = {
+ .request = s3c_dma_request,
+ .release = s3c_dma_release,
+ .prepare = s3c_dma_prepare,
+ .trigger = s3c_dma_trigger,
+ .started = s3c_dma_started,
+ .flush = s3c_dma_flush,
+ .stop = s3c_dma_stop,
+};
+
+void *s3c_dma_get_ops(void)
+{
+ return &s3c_dma_ops;
+}
+EXPORT_SYMBOL(s3c_dma_get_ops);
--
1.7.1
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH V4 05/13] ARM: SAMSUNG: Add common DMA operations
@ 2011-07-20 10:46 ` Boojin Kim
0 siblings, 0 replies; 52+ messages in thread
From: Boojin Kim @ 2011-07-20 10:46 UTC (permalink / raw)
To: linux-arm-kernel
This patch adds common DMA operations which are used for Samsung DMA
drivers. Currently there are two types of DMA driver for Samsung SoCs.
The one is S3C-DMA for S3C SoCs and the other is PL330-DMA for S5P SoCs.
This patch provides funcion pointers for common DMA operations to DMA
client driver like SPI and Audio. It makes DMA client drivers support
multi-platform.
In addition, this common DMA operations implement the shared actions
that are needed for DMA client driver. For example shared actions are
filter() function for dma_request_channel() and parameter passing for
device_prep_slave_sg().
Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
---
arch/arm/mach-s3c2410/include/mach/dma.h | 8 ++-
arch/arm/mach-s3c64xx/include/mach/dma.h | 4 +
arch/arm/plat-samsung/Makefile | 6 +-
arch/arm/plat-samsung/dma-ops.c | 133 ++++++++++++++++++++++++
arch/arm/plat-samsung/include/plat/dma-ops.h | 64 +++++++++++
arch/arm/plat-samsung/include/plat/dma-pl330.h | 4 +
arch/arm/plat-samsung/include/plat/dma.h | 1 +
arch/arm/plat-samsung/s3c-dma-ops.c | 133 ++++++++++++++++++++++++
8 files changed, 350 insertions(+), 3 deletions(-)
create mode 100644 arch/arm/plat-samsung/dma-ops.c
create mode 100644 arch/arm/plat-samsung/include/plat/dma-ops.h
create mode 100644 arch/arm/plat-samsung/s3c-dma-ops.c
diff --git a/arch/arm/mach-s3c2410/include/mach/dma.h b/arch/arm/mach-s3c2410/include/mach/dma.h
index b2b2a5b..e61a91f 100644
--- a/arch/arm/mach-s3c2410/include/mach/dma.h
+++ b/arch/arm/mach-s3c2410/include/mach/dma.h
@@ -13,7 +13,6 @@
#ifndef __ASM_ARCH_DMA_H
#define __ASM_ARCH_DMA_H __FILE__
-#include <plat/dma.h>
#include <linux/sysdev.h>
#define MAX_DMA_TRANSFER_SIZE 0x100000 /* Data Unit is half word */
@@ -51,6 +50,13 @@ enum dma_ch {
DMACH_MAX, /* the end entry */
};
+static inline bool samsung_dma_is_dmadev(void)
+{
+ return false;
+}
+
+#include <plat/dma.h>
+
#define DMACH_LOW_LEVEL (1<<28) /* use this to specifiy hardware ch no */
/* we have 4 dma channels */
diff --git a/arch/arm/mach-s3c64xx/include/mach/dma.h b/arch/arm/mach-s3c64xx/include/mach/dma.h
index 0a5d926..49c3a53 100644
--- a/arch/arm/mach-s3c64xx/include/mach/dma.h
+++ b/arch/arm/mach-s3c64xx/include/mach/dma.h
@@ -63,6 +63,10 @@ static __inline__ bool s3c_dma_has_circular(void)
return true;
}
+static inline bool samsung_dma_is_dmadev(void)
+{
+ return false;
+}
#define S3C2410_DMAF_CIRCULAR (1 << 0)
#include <plat/dma.h>
diff --git a/arch/arm/plat-samsung/Makefile b/arch/arm/plat-samsung/Makefile
index 53eb15b..9ecf2aa 100644
--- a/arch/arm/plat-samsung/Makefile
+++ b/arch/arm/plat-samsung/Makefile
@@ -62,9 +62,11 @@ obj-$(CONFIG_SAMSUNG_DEV_PWM) += dev-pwm.o
# DMA support
-obj-$(CONFIG_S3C_DMA) += dma.o
+obj-$(CONFIG_S3C_DMA) += dma.o s3c-dma-ops.o
-obj-$(CONFIG_S3C_PL330_DMA) += s3c-pl330.o
+obj-$(CONFIG_DMADEV_PL330) += dma-ops.o
+
+obj-$(CONFIG_S3C_PL330_DMA) += s3c-pl330.o s3c-dma-ops.o
# PM support
diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
new file mode 100644
index 0000000..9053433
--- /dev/null
+++ b/arch/arm/plat-samsung/dma-ops.c
@@ -0,0 +1,133 @@
+/* linux/arch/arm/plat-samsung/dma-ops.c
+ *
+ * Copyright (c) 2011 Samsung Electronics Co., Ltd.
+ * http://www.samsung.com
+ *
+ * Samsung DMA Operations
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/amba/pl330.h>
+
+#include <mach/dma.h>
+
+static bool pl330_filter(struct dma_chan *chan, void *param)
+{
+ struct dma_pl330_peri *peri = (struct dma_pl330_peri *)chan->private;
+ unsigned dma_ch = (unsigned)param;
+
+ if (peri->peri_id != dma_ch)
+ return false;
+
+ return true;
+}
+
+static unsigned samsung_dmadev_request(enum dma_ch dma_ch,
+ struct samsung_dma_info *info)
+{
+ struct dma_chan *chan;
+ dma_cap_mask_t mask;
+ struct dma_slave_config slave_config;
+
+ dma_cap_zero(mask);
+ dma_cap_set(info->cap, mask);
+
+ chan = dma_request_channel(mask, pl330_filter, (void *)dma_ch);
+
+ if (info->direction == DMA_FROM_DEVICE) {
+ memset(&slave_config, 0, sizeof(struct dma_slave_config));
+ slave_config.direction = info->direction;
+ slave_config.src_addr = info->fifo;
+ slave_config.src_addr_width = info->width;
+ dmaengine_slave_config(chan, &slave_config);
+ } else if (info->direction == DMA_TO_DEVICE) {
+ memset(&slave_config, 0, sizeof(struct dma_slave_config));
+ slave_config.direction = info->direction;
+ slave_config.dst_addr = info->fifo;
+ slave_config.dst_addr_width = info->width;
+ dmaengine_slave_config(chan, &slave_config);
+ }
+
+ return (unsigned)chan;
+}
+
+static int samsung_dmadev_release(unsigned ch,
+ struct s3c2410_dma_client *client)
+{
+ dma_release_channel((struct dma_chan *)ch);
+
+ return 0;
+}
+
+static int samsung_dmadev_prepare(unsigned ch,
+ struct samsung_dma_prep_info *info)
+{
+ struct scatterlist sg;
+ struct dma_chan *chan = (struct dma_chan *)ch;
+ struct dma_async_tx_descriptor *desc;
+
+ switch (info->cap) {
+ case DMA_SLAVE:
+ sg_init_table(&sg, 1);
+ sg_dma_len(&sg) = info->len;
+ sg_set_page(&sg, pfn_to_page(PFN_DOWN(info->buf)),
+ info->len, offset_in_page(info->buf));
+ sg_dma_address(&sg) = info->buf;
+
+ desc = chan->device->device_prep_slave_sg(chan,
+ &sg, 1, info->direction, DMA_PREP_INTERRUPT);
+ break;
+ case DMA_CYCLIC:
+ desc = chan->device->device_prep_dma_cyclic(chan,
+ info->buf, info->len, info->period, info->direction);
+ break;
+ default:
+ dev_err(&chan->dev->device, "unsupported format\n");
+ return -EFAULT;
+ }
+
+ if (!desc) {
+ dev_err(&chan->dev->device, "cannot prepare cyclic dma\n");
+ return -EFAULT;
+ }
+
+ desc->callback = info->fp;
+ desc->callback_param = info->fp_param;
+
+ dmaengine_submit((struct dma_async_tx_descriptor *)desc);
+
+ return 0;
+}
+
+static inline int samsung_dmadev_trigger(unsigned ch)
+{
+ dma_async_issue_pending((struct dma_chan *)ch);
+
+ return 0;
+}
+
+static inline int samsung_dmadev_flush(unsigned ch)
+{
+ return dmaengine_terminate_all((struct dma_chan *)ch);
+}
+
+struct samsung_dma_ops dmadev_ops = {
+ .request = samsung_dmadev_request,
+ .release = samsung_dmadev_release,
+ .prepare = samsung_dmadev_prepare,
+ .trigger = samsung_dmadev_trigger,
+ .started = NULL,
+ .flush = samsung_dmadev_flush,
+ .stop = samsung_dmadev_flush,
+};
+
+void *samsung_dmadev_get_ops(void)
+{
+ return &dmadev_ops;
+}
+EXPORT_SYMBOL(samsung_dmadev_get_ops);
diff --git a/arch/arm/plat-samsung/include/plat/dma-ops.h b/arch/arm/plat-samsung/include/plat/dma-ops.h
new file mode 100644
index 0000000..e5a68c0
--- /dev/null
+++ b/arch/arm/plat-samsung/include/plat/dma-ops.h
@@ -0,0 +1,64 @@
+/* arch/arm/plat-samsung/include/plat/dma-ops.h
+ *
+ * Copyright (c) 2011 Samsung Electronics Co., Ltd.
+ * http://www.samsung.com
+ *
+ * Samsung DMA support
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __SAMSUNG_DMA_OPS_H_
+#define __SAMSUNG_DMA_OPS_H_ __FILE__
+
+#include <linux/dmaengine.h>
+
+struct samsung_dma_prep_info {
+ enum dma_transaction_type cap;
+ enum dma_data_direction direction;
+ unsigned buf;
+ unsigned long period;
+ unsigned long len;
+ void (*fp)(void *data);
+ void *fp_param;
+};
+
+struct samsung_dma_info {
+ enum dma_transaction_type cap;
+ enum dma_data_direction direction;
+ enum dma_slave_buswidth width;
+ unsigned fifo;
+ struct s3c2410_dma_client *client;
+};
+
+struct samsung_dma_ops {
+ bool init;
+ unsigned (*request)(enum dma_ch ch, struct samsung_dma_info *info);
+ int (*release)(unsigned ch, struct s3c2410_dma_client *client);
+ int (*prepare)(unsigned ch, struct samsung_dma_prep_info *info);
+ int (*trigger)(unsigned ch);
+ int (*started)(unsigned ch);
+ int (*flush)(unsigned ch);
+ int (*stop)(unsigned ch);
+};
+
+extern void *samsung_dmadev_get_ops(void);
+extern void *s3c_dma_get_ops(void);
+
+static inline void *__samsung_dma_get_ops(void)
+{
+ if (samsung_dma_is_dmadev())
+ return samsung_dmadev_get_ops();
+ else
+ return s3c_dma_get_ops();
+}
+
+/*
+ * samsung_dma_get_ops
+ * get the set of samsung dma operations
+ */
+#define samsung_dma_get_ops() __samsung_dma_get_ops()
+
+#endif /* __SAMSUNG_DMA_OPS_H_ */
diff --git a/arch/arm/plat-samsung/include/plat/dma-pl330.h b/arch/arm/plat-samsung/include/plat/dma-pl330.h
index c402719..687489e 100644
--- a/arch/arm/plat-samsung/include/plat/dma-pl330.h
+++ b/arch/arm/plat-samsung/include/plat/dma-pl330.h
@@ -100,6 +100,10 @@ static inline bool s3c_dma_has_circular(void)
return true;
}
+static inline bool samsung_dma_is_dmadev(void)
+{
+ return true;
+}
#include <plat/dma.h>
#endif /* __DMA_PL330_H_ */
diff --git a/arch/arm/plat-samsung/include/plat/dma.h b/arch/arm/plat-samsung/include/plat/dma.h
index 2e8f8c6..e87c6be 100644
--- a/arch/arm/plat-samsung/include/plat/dma.h
+++ b/arch/arm/plat-samsung/include/plat/dma.h
@@ -125,3 +125,4 @@ extern int s3c2410_dma_set_opfn(unsigned int, s3c2410_dma_opfn_t rtn);
extern int s3c2410_dma_set_buffdone_fn(unsigned int, s3c2410_dma_cbfn_t rtn);
+#include <plat/dma-ops.h>
diff --git a/arch/arm/plat-samsung/s3c-dma-ops.c b/arch/arm/plat-samsung/s3c-dma-ops.c
new file mode 100644
index 0000000..33ab324
--- /dev/null
+++ b/arch/arm/plat-samsung/s3c-dma-ops.c
@@ -0,0 +1,133 @@
+/* linux/arch/arm/plat-samsung/s3c-dma-ops.c
+ *
+ * Copyright (c) 2011 Samsung Electronics Co., Ltd.
+ * http://www.samsung.com
+ *
+ * Samsung S3C-DMA Operations
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include <mach/dma.h>
+
+struct cb_data {
+ void (*fp) (void *);
+ void *fp_param;
+ unsigned ch;
+ struct list_head node;
+};
+
+static LIST_HEAD(dma_list);
+
+static void s3c_dma_cb(struct s3c2410_dma_chan *channel, void *param,
+ int size, enum s3c2410_dma_buffresult res)
+{
+ struct cb_data *data = param;
+
+ data->fp(data->fp_param);
+}
+
+static unsigned s3c_dma_request(enum dma_ch dma_ch,
+ struct samsung_dma_info *info)
+{
+ struct cb_data *data;
+
+ if (s3c2410_dma_request(dma_ch, info->client, NULL) < 0) {
+ s3c2410_dma_free(dma_ch, info->client);
+ return 0;
+ }
+
+ data = kzalloc(sizeof(struct cb_data), GFP_KERNEL);
+ data->ch = dma_ch;
+ list_add_tail(&data->node, &dma_list);
+
+ if (info->direction == DMA_FROM_DEVICE)
+ s3c2410_dma_devconfig(dma_ch, S3C2410_DMASRC_HW, info->fifo);
+ else
+ s3c2410_dma_devconfig(dma_ch, S3C2410_DMASRC_MEM, info->fifo);
+
+ if (info->cap == DMA_CYCLIC)
+ s3c2410_dma_setflags(dma_ch, S3C2410_DMAF_CIRCULAR);
+
+ s3c2410_dma_config(dma_ch, info->width);
+
+ return (unsigned)dma_ch;
+}
+
+static int s3c_dma_release(unsigned ch, struct s3c2410_dma_client *client)
+{
+ struct cb_data *data;
+
+ list_for_each_entry(data, &dma_list, node)
+ if (data->ch == ch)
+ break;
+ list_del(&data->node);
+
+ s3c2410_dma_free(ch, client);
+ kfree(data);
+
+ return 0;
+}
+
+static int s3c_dma_prepare(unsigned ch, struct samsung_dma_prep_info *info)
+{
+ struct cb_data *data;
+ int len = (info->cap == DMA_CYCLIC) ? info->period : info->len;
+
+ list_for_each_entry(data, &dma_list, node)
+ if (data->ch == ch)
+ break;
+
+ if (!data->fp) {
+ s3c2410_dma_set_buffdone_fn(ch, s3c_dma_cb);
+ data->fp = info->fp;
+ data->fp_param = info->fp_param;
+ }
+
+ s3c2410_dma_enqueue(ch, (void *)data, info->buf, len);
+
+ return 0;
+}
+
+static inline int s3c_dma_trigger(unsigned ch)
+{
+ return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_START);
+}
+
+static inline int s3c_dma_started(unsigned ch)
+{
+ return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STARTED);
+}
+
+static inline int s3c_dma_flush(unsigned ch)
+{
+ return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_FLUSH);
+}
+
+static inline int s3c_dma_stop(unsigned ch)
+{
+ return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STOP);
+}
+
+static struct samsung_dma_ops s3c_dma_ops = {
+ .request = s3c_dma_request,
+ .release = s3c_dma_release,
+ .prepare = s3c_dma_prepare,
+ .trigger = s3c_dma_trigger,
+ .started = s3c_dma_started,
+ .flush = s3c_dma_flush,
+ .stop = s3c_dma_stop,
+};
+
+void *s3c_dma_get_ops(void)
+{
+ return &s3c_dma_ops;
+}
+EXPORT_SYMBOL(s3c_dma_get_ops);
--
1.7.1
^ permalink raw reply related [flat|nested] 52+ messages in thread
* RE: [PATCH V4 00/13] To use DMA generic APIs for Samsung DMA
2011-07-20 10:46 ` Boojin Kim
@ 2011-07-20 16:04 ` Kukjin Kim
-1 siblings, 0 replies; 52+ messages in thread
From: Kukjin Kim @ 2011-07-20 16:04 UTC (permalink / raw)
To: 'Boojin Kim', linux-arm-kernel, linux-samsung-soc
Cc: 'Vinod Koul', 'Dan Williams', 'Jassi Brar'
Boojin Kim wrote:
>
> This patchset adds support DMA generic APIs for Samsung DMA.
> V4 doesn't has whole patch series and only has the patches differed from
V3.
>
> Changes from V3:
> - Divided 03-patch into two patchs.
> - First is 03-1-patch for DMA_SLAVE_CONFIG command.
> - Another is 03-2-patch for DMA_CYCLIC capability.
> - Code clean-up
> - Remove unnecessary variable referance.
> - Remove redunancy code
>
> [PATCH V4 01/13] DMA: PL330: Add support runtime PM for PL330 DMAC
> [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
> [PATCH V4 03-2/13] DMA: PL330: Add DMA_CYCLIC capability
> [PATCH V4 05/13] ARM: SAMSUNG: Add common DMA operations
Dear all,
Is there any comments on this?
As a note, this whole series are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git
next-samsung-dma
Boojin Kim (14):
DMA: PL330: Add support runtime PM for PL330 DMAC
DMA: PL330: Update PL330 DMA API driver
DMA: PL330: Support DMA_SLAVE_CONFIG command
DMA: PL330: Add DMA_CYCLIC capability
ARM: SAMSUNG: Update to use PL330-DMA driver
ARM: SAMSUNG: Add common DMA operations
ARM: EXYNOS4: Use generic DMA PL330 driver
ARM: S5PV210: Use generic DMA PL330 driver
ARM: S5PC100: Use generic DMA PL330 driver
ARM: S5P64X0: Use generic DMA PL330 driver
ARM: SAMSUNG: Remove S3C-PL330-DMA driver
spi/s3c64xx: Add support DMA engine API
ASoC: Samsung: Update DMA interface
ARM: SAMSUNG: Remove Samsung specific enum type for dma direction
arch/arm/mach-exynos4/Kconfig | 2 +-
arch/arm/mach-exynos4/clock.c | 16 +-
arch/arm/mach-exynos4/dma.c | 323 ++++--
arch/arm/mach-exynos4/include/mach/dma.h | 4 +-
arch/arm/mach-s3c2410/include/mach/dma.h | 20 +-
arch/arm/mach-s3c2412/dma.c | 4 +-
arch/arm/mach-s3c64xx/dma.c | 10 +-
arch/arm/mach-s3c64xx/include/mach/dma.h | 8 +-
arch/arm/mach-s5p64x0/Kconfig | 4 +-
arch/arm/mach-s5p64x0/clock-s5p6440.c | 9 +-
arch/arm/mach-s5p64x0/clock-s5p6450.c | 9 +-
arch/arm/mach-s5p64x0/dma.c | 317 ++++--
arch/arm/mach-s5p64x0/include/mach/dma.h | 2 +-
arch/arm/mach-s5pc100/Kconfig | 2 +-
arch/arm/mach-s5pc100/clock.c | 23 +-
arch/arm/mach-s5pc100/dma.c | 357 ++++--
arch/arm/mach-s5pc100/include/mach/dma.h | 2 +-
arch/arm/mach-s5pv210/Kconfig | 2 +-
arch/arm/mach-s5pv210/clock.c | 16 +-
arch/arm/mach-s5pv210/dma.c | 346 ++++--
arch/arm/mach-s5pv210/include/mach/dma.h | 2 +-
arch/arm/plat-s3c24xx/dma.c | 10 +-
arch/arm/plat-samsung/Kconfig | 8 +-
arch/arm/plat-samsung/Makefile | 4 +-
arch/arm/plat-samsung/dma-ops.c | 133 +++
arch/arm/plat-samsung/include/plat/dma-ops.h | 64 +
.../include/plat/{s3c-dma-pl330.h => dma-pl330.h} | 32 +-
arch/arm/plat-samsung/include/plat/dma-s3c24xx.h | 2 +-
arch/arm/plat-samsung/include/plat/dma.h | 10 +-
.../plat-samsung/include/plat/s3c-pl330-pdata.h | 32 -
arch/arm/plat-samsung/s3c-dma-ops.c | 130 ++
arch/arm/plat-samsung/s3c-pl330.c | 1244
--------------------
drivers/dma/Kconfig | 3 +-
drivers/dma/pl330.c | 270 ++++-
drivers/mmc/host/s3cmci.c | 6 +-
drivers/spi/spi_s3c64xx.c | 141 ++--
include/linux/amba/pl330.h | 2 +-
sound/soc/samsung/ac97.c | 10 +-
sound/soc/samsung/dma.c | 144 +--
sound/soc/samsung/dma.h | 4 +-
40 files changed, 1740 insertions(+), 1987 deletions(-)
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 00/13] To use DMA generic APIs for Samsung DMA
@ 2011-07-20 16:04 ` Kukjin Kim
0 siblings, 0 replies; 52+ messages in thread
From: Kukjin Kim @ 2011-07-20 16:04 UTC (permalink / raw)
To: linux-arm-kernel
Boojin Kim wrote:
>
> This patchset adds support DMA generic APIs for Samsung DMA.
> V4 doesn't has whole patch series and only has the patches differed from
V3.
>
> Changes from V3:
> - Divided 03-patch into two patchs.
> - First is 03-1-patch for DMA_SLAVE_CONFIG command.
> - Another is 03-2-patch for DMA_CYCLIC capability.
> - Code clean-up
> - Remove unnecessary variable referance.
> - Remove redunancy code
>
> [PATCH V4 01/13] DMA: PL330: Add support runtime PM for PL330 DMAC
> [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
> [PATCH V4 03-2/13] DMA: PL330: Add DMA_CYCLIC capability
> [PATCH V4 05/13] ARM: SAMSUNG: Add common DMA operations
Dear all,
Is there any comments on this?
As a note, this whole series are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git
next-samsung-dma
Boojin Kim (14):
DMA: PL330: Add support runtime PM for PL330 DMAC
DMA: PL330: Update PL330 DMA API driver
DMA: PL330: Support DMA_SLAVE_CONFIG command
DMA: PL330: Add DMA_CYCLIC capability
ARM: SAMSUNG: Update to use PL330-DMA driver
ARM: SAMSUNG: Add common DMA operations
ARM: EXYNOS4: Use generic DMA PL330 driver
ARM: S5PV210: Use generic DMA PL330 driver
ARM: S5PC100: Use generic DMA PL330 driver
ARM: S5P64X0: Use generic DMA PL330 driver
ARM: SAMSUNG: Remove S3C-PL330-DMA driver
spi/s3c64xx: Add support DMA engine API
ASoC: Samsung: Update DMA interface
ARM: SAMSUNG: Remove Samsung specific enum type for dma direction
arch/arm/mach-exynos4/Kconfig | 2 +-
arch/arm/mach-exynos4/clock.c | 16 +-
arch/arm/mach-exynos4/dma.c | 323 ++++--
arch/arm/mach-exynos4/include/mach/dma.h | 4 +-
arch/arm/mach-s3c2410/include/mach/dma.h | 20 +-
arch/arm/mach-s3c2412/dma.c | 4 +-
arch/arm/mach-s3c64xx/dma.c | 10 +-
arch/arm/mach-s3c64xx/include/mach/dma.h | 8 +-
arch/arm/mach-s5p64x0/Kconfig | 4 +-
arch/arm/mach-s5p64x0/clock-s5p6440.c | 9 +-
arch/arm/mach-s5p64x0/clock-s5p6450.c | 9 +-
arch/arm/mach-s5p64x0/dma.c | 317 ++++--
arch/arm/mach-s5p64x0/include/mach/dma.h | 2 +-
arch/arm/mach-s5pc100/Kconfig | 2 +-
arch/arm/mach-s5pc100/clock.c | 23 +-
arch/arm/mach-s5pc100/dma.c | 357 ++++--
arch/arm/mach-s5pc100/include/mach/dma.h | 2 +-
arch/arm/mach-s5pv210/Kconfig | 2 +-
arch/arm/mach-s5pv210/clock.c | 16 +-
arch/arm/mach-s5pv210/dma.c | 346 ++++--
arch/arm/mach-s5pv210/include/mach/dma.h | 2 +-
arch/arm/plat-s3c24xx/dma.c | 10 +-
arch/arm/plat-samsung/Kconfig | 8 +-
arch/arm/plat-samsung/Makefile | 4 +-
arch/arm/plat-samsung/dma-ops.c | 133 +++
arch/arm/plat-samsung/include/plat/dma-ops.h | 64 +
.../include/plat/{s3c-dma-pl330.h => dma-pl330.h} | 32 +-
arch/arm/plat-samsung/include/plat/dma-s3c24xx.h | 2 +-
arch/arm/plat-samsung/include/plat/dma.h | 10 +-
.../plat-samsung/include/plat/s3c-pl330-pdata.h | 32 -
arch/arm/plat-samsung/s3c-dma-ops.c | 130 ++
arch/arm/plat-samsung/s3c-pl330.c | 1244
--------------------
drivers/dma/Kconfig | 3 +-
drivers/dma/pl330.c | 270 ++++-
drivers/mmc/host/s3cmci.c | 6 +-
drivers/spi/spi_s3c64xx.c | 141 ++--
include/linux/amba/pl330.h | 2 +-
sound/soc/samsung/ac97.c | 10 +-
sound/soc/samsung/dma.c | 144 +--
sound/soc/samsung/dma.h | 4 +-
40 files changed, 1740 insertions(+), 1987 deletions(-)
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH V4 01/13] DMA: PL330: Add support runtime PM for PL330 DMAC
2011-07-20 10:46 ` Boojin Kim
@ 2011-07-20 18:35 ` Jassi Brar
-1 siblings, 0 replies; 52+ messages in thread
From: Jassi Brar @ 2011-07-20 18:35 UTC (permalink / raw)
To: Boojin Kim
Cc: linux-arm-kernel, linux-samsung-soc, Vinod Koul, Dan Williams,
Kukjin Kim
On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote:
> Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
> Cc: Vinod Koul <vinod.koul@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
Acked-by: Jassi Brar <jassisinghbrar@gmail.com>
> ---
> drivers/dma/pl330.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 73 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 6abe1ec..b7ecf47 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -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,
> +};
> +
> static struct amba_driver pl330_driver = {
> .drv = {
> .owner = THIS_MODULE,
> .name = "dma-pl330",
> + .pm = &pl330_pm_ops,
> },
> .id_table = pl330_ids,
> .probe = pl330_probe,
> --
> 1.7.1
>
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 01/13] DMA: PL330: Add support runtime PM for PL330 DMAC
@ 2011-07-20 18:35 ` Jassi Brar
0 siblings, 0 replies; 52+ messages in thread
From: Jassi Brar @ 2011-07-20 18:35 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote:
> Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
> Cc: Vinod Koul <vinod.koul@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
Acked-by: Jassi Brar <jassisinghbrar@gmail.com>
> ---
> ?drivers/dma/pl330.c | ? 75 +++++++++++++++++++++++++++++++++++++++++++++++++-
> ?1 files changed, 73 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 6abe1ec..b7ecf47 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -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,
> +};
> +
> ?static struct amba_driver pl330_driver = {
> ? ? ? ?.drv = {
> ? ? ? ? ? ? ? ?.owner = THIS_MODULE,
> ? ? ? ? ? ? ? ?.name = "dma-pl330",
> + ? ? ? ? ? ? ? .pm = &pl330_pm_ops,
> ? ? ? ?},
> ? ? ? ?.id_table = pl330_ids,
> ? ? ? ?.probe = pl330_probe,
> --
> 1.7.1
>
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
2011-07-20 10:46 ` Boojin Kim
@ 2011-07-20 19:17 ` Jassi Brar
-1 siblings, 0 replies; 52+ messages in thread
From: Jassi Brar @ 2011-07-20 19:17 UTC (permalink / raw)
To: Boojin Kim
Cc: linux-arm-kernel, linux-samsung-soc, Vinod Koul, Dan Williams,
Kukjin Kim
On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote:
> Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
> Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> ---
> drivers/dma/pl330.c | 53 +++++++++++++++++++++++++++++++++++++-------------
> 1 files changed, 39 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 586ab39..880f010 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -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);
> + break;
> + case DMA_SLAVE_CONFIG:
Please protect this section too using spin_lock.
> + 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);
> + } 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);
> + }
PL330 has fixed channels to peripherals.
So FIFO addresses(burst_sz too?) should already be set via platform data.
Client drivers shouldn't bother.
<will review remaining patches soon, gotta go now>
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
@ 2011-07-20 19:17 ` Jassi Brar
0 siblings, 0 replies; 52+ messages in thread
From: Jassi Brar @ 2011-07-20 19:17 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote:
> Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
> Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> ---
> ?drivers/dma/pl330.c | ? 53 +++++++++++++++++++++++++++++++++++++-------------
> ?1 files changed, 39 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 586ab39..880f010 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -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);
> + ? ? ? ? ? ? ? break;
> + ? ? ? case DMA_SLAVE_CONFIG:
Please protect this section too using spin_lock.
> + ? ? ? ? ? ? ? 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);
> + ? ? ? ? ? ? ? } 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);
> + ? ? ? ? ? ? ? }
PL330 has fixed channels to peripherals.
So FIFO addresses(burst_sz too?) should already be set via platform data.
Client drivers shouldn't bother.
<will review remaining patches soon, gotta go now>
^ permalink raw reply [flat|nested] 52+ messages in thread
* RE: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
2011-07-20 19:17 ` Jassi Brar
@ 2011-07-21 4:13 ` Boojin Kim
-1 siblings, 0 replies; 52+ messages in thread
From: Boojin Kim @ 2011-07-21 4:13 UTC (permalink / raw)
To: 'Jassi Brar'
Cc: 'Vinod Koul', 'Kukjin Kim',
'Dan Williams',
linux-samsung-soc, linux-arm-kernel
Jassi Brar wrote:
> Sent: Thursday, July 21, 2011 4:18 AM
> To: Boojin Kim
> Cc: linux-arm-kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org;
> Vinod Koul; Dan Williams; Kukjin Kim
> Subject: Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
>
> On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote:
> > Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
> > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> > ---
> > drivers/dma/pl330.c | 53
> > +++++++++++++++++++++++++++++++++++++----------
> ---
> > 1 files changed, 39 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> > index 586ab39..880f010 100644
> > --- a/drivers/dma/pl330.c
> > +++ b/drivers/dma/pl330.c
> > @@ -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);
> > + break;
> > + case DMA_SLAVE_CONFIG:
> Please protect this section too using spin_lock.
Why is spin_lock need here?
This code just sets configuration data into own channel structure.
>
>
> > + 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);
> > + } 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);
> > + }
> PL330 has fixed channels to peripherals.
> So FIFO addresses(burst_sz too?) should already be set via platform data.
> Client drivers shouldn't bother.
First, DMA machine code should know the FIFO address of all client drivers to
set via platform data.
In this case, Problem is that the definition of FIFO address is almost
declared to the header file of client driver.
For example, sound\soc\samsung\regs-i2s-v2.h only has the definition of fifo
address as following.
#define S3C2412_IISTXD (0x10)
#define S3C2412_IISRXD (0x14)
These definitions should be referred to DMA machine code to set fifo address
through platform data.
I think it's not good method.
And, SLAVE_CONFIG command exist to pass slave configuration data from client
driver to DMA driver.
So, I think that it's proper to pass fifo address through SLAVE_CONFIG
command.
Second, 'burst_sz' isn't fixed value. Namely, Client driver changes 'burst_sz'
according to its usecase.
For example, I2S driver sets 'burst_sz' to 4 in case of stereo
playback/recording. But in case of mono playback/recording, It changes
'burst_sz' to 2.
So, Client driver should use SLAVE_CONFIG command to set 'burst_sz' because
it's not fixed value.
>
> <will review remaining patches soon, gotta go now>
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
@ 2011-07-21 4:13 ` Boojin Kim
0 siblings, 0 replies; 52+ messages in thread
From: Boojin Kim @ 2011-07-21 4:13 UTC (permalink / raw)
To: linux-arm-kernel
Jassi Brar wrote:
> Sent: Thursday, July 21, 2011 4:18 AM
> To: Boojin Kim
> Cc: linux-arm-kernel at lists.infradead.org; linux-samsung-soc at vger.kernel.org;
> Vinod Koul; Dan Williams; Kukjin Kim
> Subject: Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
>
> On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote:
> > Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
> > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> > ---
> > drivers/dma/pl330.c | 53
> > +++++++++++++++++++++++++++++++++++++----------
> ---
> > 1 files changed, 39 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> > index 586ab39..880f010 100644
> > --- a/drivers/dma/pl330.c
> > +++ b/drivers/dma/pl330.c
> > @@ -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);
> > + break;
> > + case DMA_SLAVE_CONFIG:
> Please protect this section too using spin_lock.
Why is spin_lock need here?
This code just sets configuration data into own channel structure.
>
>
> > + 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);
> > + } 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);
> > + }
> PL330 has fixed channels to peripherals.
> So FIFO addresses(burst_sz too?) should already be set via platform data.
> Client drivers shouldn't bother.
First, DMA machine code should know the FIFO address of all client drivers to
set via platform data.
In this case, Problem is that the definition of FIFO address is almost
declared to the header file of client driver.
For example, sound\soc\samsung\regs-i2s-v2.h only has the definition of fifo
address as following.
#define S3C2412_IISTXD (0x10)
#define S3C2412_IISRXD (0x14)
These definitions should be referred to DMA machine code to set fifo address
through platform data.
I think it's not good method.
And, SLAVE_CONFIG command exist to pass slave configuration data from client
driver to DMA driver.
So, I think that it's proper to pass fifo address through SLAVE_CONFIG
command.
Second, 'burst_sz' isn't fixed value. Namely, Client driver changes 'burst_sz'
according to its usecase.
For example, I2S driver sets 'burst_sz' to 4 in case of stereo
playback/recording. But in case of mono playback/recording, It changes
'burst_sz' to 2.
So, Client driver should use SLAVE_CONFIG command to set 'burst_sz' because
it's not fixed value.
>
> <will review remaining patches soon, gotta go now>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
2011-07-21 4:13 ` Boojin Kim
@ 2011-07-21 5:00 ` Jassi Brar
-1 siblings, 0 replies; 52+ messages in thread
From: Jassi Brar @ 2011-07-21 5:00 UTC (permalink / raw)
To: Boojin Kim
Cc: linux-arm-kernel, linux-samsung-soc, Vinod Koul, Dan Williams,
Kukjin Kim
On Thu, Jul 21, 2011 at 9:43 AM, Boojin Kim <boojin.kim@samsung.com> wrote:
>> > - pl330_tasklet((unsigned long) pch);
>> > + list_splice_tail_init(&list, &pdmac->desc_pool);
>> > + spin_unlock_irqrestore(&pch->lock, flags);
>> > + break;
>> > + case DMA_SLAVE_CONFIG:
>> Please protect this section too using spin_lock.
> Why is spin_lock need here?
> This code just sets configuration data into own channel structure.
It does seem unncessary, but the configuration that is set here is read
in other parts of the driver. However unlikely but there is theoretical
possibility of race here - one shouldn't count upon goodness of client drivers.
>>
>> > + 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);
>> > + } 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);
>> > + }
>> PL330 has fixed channels to peripherals.
>> So FIFO addresses(burst_sz too?) should already be set via platform data.
>> Client drivers shouldn't bother.
>
> First, DMA machine code should know the FIFO address of all client drivers to
> set via platform data.
> In this case, Problem is that the definition of FIFO address is almost
> declared to the header file of client driver.
> For example, sound\soc\samsung\regs-i2s-v2.h only has the definition of fifo
> address as following.
>
> #define S3C2412_IISTXD (0x10)
> #define S3C2412_IISRXD (0x14)
>
> These definitions should be referred to DMA machine code to set fifo address
> through platform data.
> I think it's not good method.
Crap!
Client drivers don't conjure up the fifo address - if not hardcoded they
are gotten via platform_data already.
> And, SLAVE_CONFIG command exist to pass slave configuration data from client
> driver to DMA driver.
> So, I think that it's proper to pass fifo address through SLAVE_CONFIG
> command.
If it's still not clear, read the para above definition of 'struct
dma_slave_config'
in include/linux/dmaengine.h
>
> Second, 'burst_sz' isn't fixed value. Namely, Client driver changes 'burst_sz'
> according to its usecase.
> For example, I2S driver sets 'burst_sz' to 4 in case of stereo
> playback/recording. But in case of mono playback/recording, It changes
> 'burst_sz' to 2.
> So, Client driver should use SLAVE_CONFIG command to set 'burst_sz' because
> it's not fixed value.
>
Yeah yeah, ok, that's why I put that with a ? in bracket.
I just don't remember testing with fixed burst_sz with pl330.
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
@ 2011-07-21 5:00 ` Jassi Brar
0 siblings, 0 replies; 52+ messages in thread
From: Jassi Brar @ 2011-07-21 5:00 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jul 21, 2011 at 9:43 AM, Boojin Kim <boojin.kim@samsung.com> wrote:
>> > - ? ? ? pl330_tasklet((unsigned long) pch);
>> > + ? ? ? ? ? ? ? list_splice_tail_init(&list, &pdmac->desc_pool);
>> > + ? ? ? ? ? ? ? spin_unlock_irqrestore(&pch->lock, flags);
>> > + ? ? ? ? ? ? ? break;
>> > + ? ? ? case DMA_SLAVE_CONFIG:
>> Please protect this section too using spin_lock.
> Why is spin_lock need here?
> This code just sets configuration data into own channel structure.
It does seem unncessary, but the configuration that is set here is read
in other parts of the driver. However unlikely but there is theoretical
possibility of race here - one shouldn't count upon goodness of client drivers.
>>
>> > + ? ? ? ? ? ? ? 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);
>> > + ? ? ? ? ? ? ? } 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);
>> > + ? ? ? ? ? ? ? }
>> PL330 has fixed channels to peripherals.
>> So FIFO addresses(burst_sz too?) should already be set via platform data.
>> Client drivers shouldn't bother.
>
> First, DMA machine code should know the FIFO address of all client drivers to
> set via platform data.
> In this case, Problem is that the definition of FIFO address is almost
> declared to the header file of client driver.
> For example, sound\soc\samsung\regs-i2s-v2.h only has the definition of fifo
> address as following.
>
> #define S3C2412_IISTXD ? ? ? ? ? ? ? ? ?(0x10)
> #define S3C2412_IISRXD ? ? ? ? ? ? ? ? ?(0x14)
>
> These definitions should be referred to DMA machine code to set fifo address
> through platform data.
> I think it's not good method.
Crap!
Client drivers don't conjure up the fifo address - if not hardcoded they
are gotten via platform_data already.
> And, SLAVE_CONFIG command exist to pass slave configuration data from client
> driver to DMA driver.
> So, I think that it's proper to pass fifo address through SLAVE_CONFIG
> command.
If it's still not clear, read the para above definition of 'struct
dma_slave_config'
in include/linux/dmaengine.h
>
> Second, 'burst_sz' isn't fixed value. Namely, Client driver changes 'burst_sz'
> according to its usecase.
> For example, I2S driver sets 'burst_sz' to 4 in case of stereo
> playback/recording. But in case of mono playback/recording, It changes
> 'burst_sz' to 2.
> So, Client driver should use SLAVE_CONFIG command to set 'burst_sz' because
> it's not fixed value.
>
Yeah yeah, ok, that's why I put that with a ? in bracket.
I just don't remember testing with fixed burst_sz with pl330.
^ permalink raw reply [flat|nested] 52+ messages in thread
* RE: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
2011-07-21 5:00 ` Jassi Brar
@ 2011-07-21 6:34 ` Boojin Kim
-1 siblings, 0 replies; 52+ messages in thread
From: Boojin Kim @ 2011-07-21 6:34 UTC (permalink / raw)
To: 'Jassi Brar'
Cc: 'Vinod Koul', 'Kukjin Kim',
'Dan Williams',
linux-samsung-soc, linux-arm-kernel
Jassi Brar wrote:
> Sent: Thursday, July 21, 2011 2:00 PM
> To: Boojin Kim
> Cc: linux-arm-kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org;
> Vinod Koul; Dan Williams; Kukjin Kim
> Subject: Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
>
> On Thu, Jul 21, 2011 at 9:43 AM, Boojin Kim <boojin.kim@samsung.com> wrote:
>
> >> > - pl330_tasklet((unsigned long) pch);
> >> > + list_splice_tail_init(&list, &pdmac->desc_pool);
> >> > + spin_unlock_irqrestore(&pch->lock, flags);
> >> > + break;
> >> > + case DMA_SLAVE_CONFIG:
> >> Please protect this section too using spin_lock.
> > Why is spin_lock need here?
> > This code just sets configuration data into own channel structure.
> It does seem unncessary, but the configuration that is set here is read
> in other parts of the driver. However unlikely but there is theoretical
> possibility of race here - one shouldn't count upon goodness of client
> drivers.
Yes, Client driver doesn't afflict the configuration data again.
So, I think spin_lock isn't required here.
>
> >>
> >> > + 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);
> >> > + } 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);
> >> > + }
> >> PL330 has fixed channels to peripherals.
> >> So FIFO addresses(burst_sz too?) should already be set via platform data.
> >> Client drivers shouldn't bother.
> >
> > First, DMA machine code should know the FIFO address of all client drivers
> to
> > set via platform data.
> > In this case, Problem is that the definition of FIFO address is almost
> > declared to the header file of client driver.
> > For example, sound\soc\samsung\regs-i2s-v2.h only has the definition of
> fifo
> > address as following.
> >
> > #define S3C2412_IISTXD (0x10)
> > #define S3C2412_IISRXD (0x14)
> >
> > These definitions should be referred to DMA machine code to set fifo
> address
> > through platform data.
> > I think it's not good method.
> Crap!
> Client drivers don't conjure up the fifo address - if not hardcoded they
> are gotten via platform_data already.
For it, DMA machine code should include all header files of client driver that
has definition of FIFO address.
The number of header file would be more than 10. And I think it make the code
a little complicated.
>
> > And, SLAVE_CONFIG command exist to pass slave configuration data from
> client
> > driver to DMA driver.
> > So, I think that it's proper to pass fifo address through SLAVE_CONFIG
> > command.
> If it's still not clear, read the para above definition of 'struct
> dma_slave_config'
> in include/linux/dmaengine.h
Other DMA client drivers in mainline code already use SLAVE_CONFIG command to
pass fifo address as following.
Please refer to Sound/soc/imx/imx-pcm-dma/mx2.c, Driver/mmc/host/mmci.c,
Drivers/mmc/host/mxcmmc.c and so on.
And, Other DMA drivers also support to SLAVE_CONFIG command for it.
Please refer to amba-pl08x.c, coh901318.c, ste_dma40.c and so on in
'driver/dma' directory.
So, In my opinion, this is proper method to handle the client's fifo address.
>
> >
> > Second, 'burst_sz' isn't fixed value. Namely, Client driver changes
> 'burst_sz'
> > according to its usecase.
> > For example, I2S driver sets 'burst_sz' to 4 in case of stereo
> > playback/recording. But in case of mono playback/recording, It changes
> > 'burst_sz' to 2.
> > So, Client driver should use SLAVE_CONFIG command to set 'burst_sz'
> > because
> > it's not fixed value.
> >
> Yeah yeah, ok, that's why I put that with a ? in bracket.
> I just don't remember testing with fixed burst_sz with pl330.
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
@ 2011-07-21 6:34 ` Boojin Kim
0 siblings, 0 replies; 52+ messages in thread
From: Boojin Kim @ 2011-07-21 6:34 UTC (permalink / raw)
To: linux-arm-kernel
Jassi Brar wrote:
> Sent: Thursday, July 21, 2011 2:00 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
> Subject: Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
>
> On Thu, Jul 21, 2011 at 9:43 AM, Boojin Kim <boojin.kim@samsung.com> wrote:
>
> >> > - pl330_tasklet((unsigned long) pch);
> >> > + list_splice_tail_init(&list, &pdmac->desc_pool);
> >> > + spin_unlock_irqrestore(&pch->lock, flags);
> >> > + break;
> >> > + case DMA_SLAVE_CONFIG:
> >> Please protect this section too using spin_lock.
> > Why is spin_lock need here?
> > This code just sets configuration data into own channel structure.
> It does seem unncessary, but the configuration that is set here is read
> in other parts of the driver. However unlikely but there is theoretical
> possibility of race here - one shouldn't count upon goodness of client
> drivers.
Yes, Client driver doesn't afflict the configuration data again.
So, I think spin_lock isn't required here.
>
> >>
> >> > + 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);
> >> > + } 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);
> >> > + }
> >> PL330 has fixed channels to peripherals.
> >> So FIFO addresses(burst_sz too?) should already be set via platform data.
> >> Client drivers shouldn't bother.
> >
> > First, DMA machine code should know the FIFO address of all client drivers
> to
> > set via platform data.
> > In this case, Problem is that the definition of FIFO address is almost
> > declared to the header file of client driver.
> > For example, sound\soc\samsung\regs-i2s-v2.h only has the definition of
> fifo
> > address as following.
> >
> > #define S3C2412_IISTXD (0x10)
> > #define S3C2412_IISRXD (0x14)
> >
> > These definitions should be referred to DMA machine code to set fifo
> address
> > through platform data.
> > I think it's not good method.
> Crap!
> Client drivers don't conjure up the fifo address - if not hardcoded they
> are gotten via platform_data already.
For it, DMA machine code should include all header files of client driver that
has definition of FIFO address.
The number of header file would be more than 10. And I think it make the code
a little complicated.
>
> > And, SLAVE_CONFIG command exist to pass slave configuration data from
> client
> > driver to DMA driver.
> > So, I think that it's proper to pass fifo address through SLAVE_CONFIG
> > command.
> If it's still not clear, read the para above definition of 'struct
> dma_slave_config'
> in include/linux/dmaengine.h
Other DMA client drivers in mainline code already use SLAVE_CONFIG command to
pass fifo address as following.
Please refer to Sound/soc/imx/imx-pcm-dma/mx2.c, Driver/mmc/host/mmci.c,
Drivers/mmc/host/mxcmmc.c and so on.
And, Other DMA drivers also support to SLAVE_CONFIG command for it.
Please refer to amba-pl08x.c, coh901318.c, ste_dma40.c and so on in
'driver/dma' directory.
So, In my opinion, this is proper method to handle the client's fifo address.
>
> >
> > Second, 'burst_sz' isn't fixed value. Namely, Client driver changes
> 'burst_sz'
> > according to its usecase.
> > For example, I2S driver sets 'burst_sz' to 4 in case of stereo
> > playback/recording. But in case of mono playback/recording, It changes
> > 'burst_sz' to 2.
> > So, Client driver should use SLAVE_CONFIG command to set 'burst_sz'
> > because
> > it's not fixed value.
> >
> Yeah yeah, ok, that's why I put that with a ? in bracket.
> I just don't remember testing with fixed burst_sz with pl330.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
2011-07-21 6:34 ` Boojin Kim
@ 2011-07-21 7:31 ` Jassi Brar
-1 siblings, 0 replies; 52+ messages in thread
From: Jassi Brar @ 2011-07-21 7:31 UTC (permalink / raw)
To: Boojin Kim
Cc: Vinod Koul, linux-samsung-soc, Dan Williams, Kukjin Kim,
linux-arm-kernel
On Thu, Jul 21, 2011 at 12:04 PM, Boojin Kim <boojin.kim@samsung.com> wrote:
> Jassi Brar wrote:
>> Sent: Thursday, July 21, 2011 2:00 PM
>> To: Boojin Kim
>> Cc: linux-arm-kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org;
>> Vinod Koul; Dan Williams; Kukjin Kim
>> Subject: Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
>>
>> On Thu, Jul 21, 2011 at 9:43 AM, Boojin Kim <boojin.kim@samsung.com> wrote:
>>
>> >> > - pl330_tasklet((unsigned long) pch);
>> >> > + list_splice_tail_init(&list, &pdmac->desc_pool);
>> >> > + spin_unlock_irqrestore(&pch->lock, flags);
>> >> > + break;
>> >> > + case DMA_SLAVE_CONFIG:
>> >> Please protect this section too using spin_lock.
>> > Why is spin_lock need here?
>> > This code just sets configuration data into own channel structure.
>> It does seem unncessary, but the configuration that is set here is read
>> in other parts of the driver. However unlikely but there is theoretical
>> possibility of race here - one shouldn't count upon goodness of client
>> drivers.
> Yes, Client driver doesn't afflict the configuration data again.
> So, I think spin_lock isn't required here.
>
>>
>> >>
>> >> > + 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);
>> >> > + } 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);
>> >> > + }
>> >> PL330 has fixed channels to peripherals.
>> >> So FIFO addresses(burst_sz too?) should already be set via platform data.
>> >> Client drivers shouldn't bother.
>> >
>> > First, DMA machine code should know the FIFO address of all client drivers
>> to
>> > set via platform data.
>> > In this case, Problem is that the definition of FIFO address is almost
>> > declared to the header file of client driver.
>> > For example, sound\soc\samsung\regs-i2s-v2.h only has the definition of
>> fifo
>> > address as following.
>> >
>> > #define S3C2412_IISTXD (0x10)
>> > #define S3C2412_IISRXD (0x14)
>> >
>> > These definitions should be referred to DMA machine code to set fifo
>> address
>> > through platform data.
>> > I think it's not good method.
>> Crap!
>> Client drivers don't conjure up the fifo address - if not hardcoded they
>> are gotten via platform_data already.
>
> For it, DMA machine code should include all header files of client driver that
> has definition of FIFO address.
> The number of header file would be more than 10. And I think it make the code
> a little complicated.
>
>>
>> > And, SLAVE_CONFIG command exist to pass slave configuration data from
>> client
>> > driver to DMA driver.
>> > So, I think that it's proper to pass fifo address through SLAVE_CONFIG
>> > command.
>> If it's still not clear, read the para above definition of 'struct
>> dma_slave_config'
>> in include/linux/dmaengine.h
>
> Other DMA client drivers in mainline code already use SLAVE_CONFIG command to
> pass fifo address as following.
> Please refer to Sound/soc/imx/imx-pcm-dma/mx2.c, Driver/mmc/host/mmci.c,
> Drivers/mmc/host/mxcmmc.c and so on.
>
> And, Other DMA drivers also support to SLAVE_CONFIG command for it.
> Please refer to amba-pl08x.c, coh901318.c, ste_dma40.c and so on in
> 'driver/dma' directory.
> So, In my opinion, this is proper method to handle the client's fifo address.
>
NAK.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
@ 2011-07-21 7:31 ` Jassi Brar
0 siblings, 0 replies; 52+ messages in thread
From: Jassi Brar @ 2011-07-21 7:31 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jul 21, 2011 at 12:04 PM, Boojin Kim <boojin.kim@samsung.com> wrote:
> Jassi Brar wrote:
>> Sent: Thursday, July 21, 2011 2:00 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
>> Subject: Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
>>
>> On Thu, Jul 21, 2011 at 9:43 AM, Boojin Kim <boojin.kim@samsung.com> wrote:
>>
>> >> > - ? ? ? pl330_tasklet((unsigned long) pch);
>> >> > + ? ? ? ? ? ? ? list_splice_tail_init(&list, &pdmac->desc_pool);
>> >> > + ? ? ? ? ? ? ? spin_unlock_irqrestore(&pch->lock, flags);
>> >> > + ? ? ? ? ? ? ? break;
>> >> > + ? ? ? case DMA_SLAVE_CONFIG:
>> >> Please protect this section too using spin_lock.
>> > Why is spin_lock need here?
>> > This code just sets configuration data into own channel structure.
>> It does seem unncessary, but the configuration that is set here is read
>> in other parts of the driver. However unlikely but there is theoretical
>> possibility of race here - one shouldn't count upon goodness of client
>> drivers.
> Yes, Client driver doesn't afflict the configuration data again.
> So, I think spin_lock isn't required here.
>
>>
>> >>
>> >> > + ? ? ? ? ? ? ? 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);
>> >> > + ? ? ? ? ? ? ? } 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);
>> >> > + ? ? ? ? ? ? ? }
>> >> PL330 has fixed channels to peripherals.
>> >> So FIFO addresses(burst_sz too?) should already be set via platform data.
>> >> Client drivers shouldn't bother.
>> >
>> > First, DMA machine code should know the FIFO address of all client drivers
>> to
>> > set via platform data.
>> > In this case, Problem is that the definition of FIFO address is almost
>> > declared to the header file of client driver.
>> > For example, sound\soc\samsung\regs-i2s-v2.h only has the definition of
>> fifo
>> > address as following.
>> >
>> > #define S3C2412_IISTXD ? ? ? ? ? ? ? ? ?(0x10)
>> > #define S3C2412_IISRXD ? ? ? ? ? ? ? ? ?(0x14)
>> >
>> > These definitions should be referred to DMA machine code to set fifo
>> address
>> > through platform data.
>> > I think it's not good method.
>> Crap!
>> Client drivers don't conjure up the fifo address - if not hardcoded they
>> are gotten via platform_data already.
>
> For it, DMA machine code should include all header files of client driver that
> has definition of FIFO address.
> The number of header file would be more than 10. And I think it make the code
> a little complicated.
>
>>
>> > And, SLAVE_CONFIG command exist to pass slave configuration data from
>> client
>> > driver to DMA driver.
>> > So, I think that it's proper to pass fifo address through SLAVE_CONFIG
>> > command.
>> If it's still not clear, read the para above definition of 'struct
>> dma_slave_config'
>> in include/linux/dmaengine.h
>
> Other DMA client drivers in mainline code already use SLAVE_CONFIG command to
> pass fifo address as following.
> Please refer to Sound/soc/imx/imx-pcm-dma/mx2.c, Driver/mmc/host/mmci.c,
> Drivers/mmc/host/mxcmmc.c and so on.
>
> And, Other DMA drivers also support to SLAVE_CONFIG command for it.
> Please refer to amba-pl08x.c, coh901318.c, ste_dma40.c and so on in
> 'driver/dma' directory.
> So, In my opinion, this is proper method to handle the client's fifo address.
>
NAK.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
2011-07-20 19:17 ` Jassi Brar
@ 2011-07-21 8:11 ` Russell King - ARM Linux
-1 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2011-07-21 8:11 UTC (permalink / raw)
To: Jassi Brar
Cc: Boojin Kim, Vinod Koul, Kukjin Kim, Dan Williams,
linux-samsung-soc, linux-arm-kernel
On Thu, Jul 21, 2011 at 12:47:49AM +0530, Jassi Brar wrote:
> On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote:
> > + 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);
> > + } 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);
> > + }
> PL330 has fixed channels to peripherals.
> So FIFO addresses(burst_sz too?) should already be set via platform data.
> Client drivers shouldn't bother.
That's utter crap, and isn't what the DMA engine API is about.
The above looks correctly implemented. Slave DMA engine users are
supposed to supply the device DMA register address via this
DMA_SLAVE_CONFIG call. Doing this via platform data for the DMA
device is braindead.
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
@ 2011-07-21 8:11 ` Russell King - ARM Linux
0 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2011-07-21 8:11 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jul 21, 2011 at 12:47:49AM +0530, Jassi Brar wrote:
> On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote:
> > + ? ? ? ? ? ? ? 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);
> > + ? ? ? ? ? ? ? } 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);
> > + ? ? ? ? ? ? ? }
> PL330 has fixed channels to peripherals.
> So FIFO addresses(burst_sz too?) should already be set via platform data.
> Client drivers shouldn't bother.
That's utter crap, and isn't what the DMA engine API is about.
The above looks correctly implemented. Slave DMA engine users are
supposed to supply the device DMA register address via this
DMA_SLAVE_CONFIG call. Doing this via platform data for the DMA
device is braindead.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
2011-07-21 8:11 ` Russell King - ARM Linux
@ 2011-07-21 9:14 ` Jassi Brar
-1 siblings, 0 replies; 52+ messages in thread
From: Jassi Brar @ 2011-07-21 9:14 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: linux-samsung-soc, Boojin Kim, Vinod Koul, Kukjin Kim,
Dan Williams, linux-arm-kernel
On Thu, Jul 21, 2011 at 1:41 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Jul 21, 2011 at 12:47:49AM +0530, Jassi Brar wrote:
>> On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote:
>> > + 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);
>> > + } 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);
>> > + }
>> PL330 has fixed channels to peripherals.
>> So FIFO addresses(burst_sz too?) should already be set via platform data.
>> Client drivers shouldn't bother.
>
> That's utter crap, and isn't what the DMA engine API is about.
>
> The above looks correctly implemented. Slave DMA engine users are
> supposed to supply the device DMA register address via this
> DMA_SLAVE_CONFIG call. Doing this via platform data for the DMA
> device is braindead.
Rather than have 32 client drivers expect fifo_address from the
platform and then
provide to the DMAC, IMHO it is better for a single driver(DMAC) to
get 32 addresses
from the platform.
Esp when the DMAC driver already expect similar h/w parameter -- *direction*.
I don't understand why is 'fifo_address' a property much different
than 'direction' of the
channel ?
If a channel is flexible enough to change it's 'fifo_address', most probably it
could also change direction of transfers.
So, why do we want to take seriously 'fifo_address' provided by the
client drivers
and not the 'direction' ?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
@ 2011-07-21 9:14 ` Jassi Brar
0 siblings, 0 replies; 52+ messages in thread
From: Jassi Brar @ 2011-07-21 9:14 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jul 21, 2011 at 1:41 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Jul 21, 2011 at 12:47:49AM +0530, Jassi Brar wrote:
>> On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote:
>> > + ? ? ? ? ? ? ? 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);
>> > + ? ? ? ? ? ? ? } 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);
>> > + ? ? ? ? ? ? ? }
>> PL330 has fixed channels to peripherals.
>> So FIFO addresses(burst_sz too?) should already be set via platform data.
>> Client drivers shouldn't bother.
>
> That's utter crap, and isn't what the DMA engine API is about.
>
> The above looks correctly implemented. ?Slave DMA engine users are
> supposed to supply the device DMA register address via this
> DMA_SLAVE_CONFIG call. ?Doing this via platform data for the DMA
> device is braindead.
Rather than have 32 client drivers expect fifo_address from the
platform and then
provide to the DMAC, IMHO it is better for a single driver(DMAC) to
get 32 addresses
from the platform.
Esp when the DMAC driver already expect similar h/w parameter -- *direction*.
I don't understand why is 'fifo_address' a property much different
than 'direction' of the
channel ?
If a channel is flexible enough to change it's 'fifo_address', most probably it
could also change direction of transfers.
So, why do we want to take seriously 'fifo_address' provided by the
client drivers
and not the 'direction' ?
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
2011-07-21 9:14 ` Jassi Brar
@ 2011-07-21 10:23 ` Linus Walleij
-1 siblings, 0 replies; 52+ messages in thread
From: Linus Walleij @ 2011-07-21 10:23 UTC (permalink / raw)
To: Jassi Brar
Cc: Russell King - ARM Linux, linux-samsung-soc, Boojin Kim,
Vinod Koul, Kukjin Kim, Dan Williams, linux-arm-kernel
On Thu, Jul 21, 2011 at 11:14 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Thu, Jul 21, 2011 at 1:41 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Thu, Jul 21, 2011 at 12:47:49AM +0530, Jassi Brar wrote:
>>> PL330 has fixed channels to peripherals.
>>> So FIFO addresses(burst_sz too?) should already be set via platform data.
>>> Client drivers shouldn't bother.
>>
>> That's utter crap, and isn't what the DMA engine API is about.
>>
>> The above looks correctly implemented. Slave DMA engine users are
>> supposed to supply the device DMA register address via this
>> DMA_SLAVE_CONFIG call. Doing this via platform data for the DMA
>> device is braindead.
>
> Rather than have 32 client drivers expect fifo_address from the
> platform and then
> provide to the DMAC, IMHO it is better for a single driver(DMAC) to
> get 32 addresses
> from the platform.
My idea (when I implemented it) is that the device driver knows the
physical and virtual base and thus the address where DMA data is
destined. It knows all other registers, it remaps them, noone else should
be bothered with containing the knowledge of adress ranges for the
specific hardware block.
Passing this through platform data requires the machine to keep track
not only of the base adress of the peripheral (as is generally contained
in the machine or device tree) but also the offset of specific registers
in that memory region, which is too much.
Usually this means the offsets are defined in files like <mach/perfoo.h>
which means they can't be pushed down into drivers/foo/foo.c and
creates unnecessary bindings and de-encapsulation of the driver.
We want to get rid of such stuff. (I think?)
Therefore I introduced this to confine the different registers into
the driver itself and ask the DMA engine to transfer to a specific
address.
> Esp when the DMAC driver already expect similar h/w
> parameter -- *direction*.
>
> I don't understand why is 'fifo_address' a property much different
> than 'direction' of the
> channel ?
struct dma_slave_config {
enum dma_data_direction direction;
dma_addr_t src_addr;
dma_addr_t dst_addr;
enum dma_slave_buswidth src_addr_width;
enum dma_slave_buswidth dst_addr_width;
u32 src_maxburst;
u32 dst_maxburst;
};
We do support changing direction as well as src and dst address
(i.e. FIFO endpoint) at runtime.
Check drivers/mmc/host/mmci.c for an example of how we switch
a single channel from TX to RX in runtime using this one property.
However it has been noted by Russell that the direction member
is unnecessary since the device_prep_slave_sg() function in the
dmaengine vtable already takes a direction argument like this:
struct dma_async_tx_descriptor *(*device_prep_slave_sg)(
struct dma_chan *chan, struct scatterlist *sgl,
unsigned int sg_len, enum dma_data_direction direction,
unsigned long flags);
Therefore the direction setting needs to go from either the config
struct or the device_prep_slave_sg() call, as right now the channel
is being told about the direction twice which is not elegant and
confusing.
So you even have two ways of changing direction at runtime...
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
@ 2011-07-21 10:23 ` Linus Walleij
0 siblings, 0 replies; 52+ messages in thread
From: Linus Walleij @ 2011-07-21 10:23 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jul 21, 2011 at 11:14 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Thu, Jul 21, 2011 at 1:41 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Thu, Jul 21, 2011 at 12:47:49AM +0530, Jassi Brar wrote:
>>> PL330 has fixed channels to peripherals.
>>> So FIFO addresses(burst_sz too?) should already be set via platform data.
>>> Client drivers shouldn't bother.
>>
>> That's utter crap, and isn't what the DMA engine API is about.
>>
>> The above looks correctly implemented. ?Slave DMA engine users are
>> supposed to supply the device DMA register address via this
>> DMA_SLAVE_CONFIG call. ?Doing this via platform data for the DMA
>> device is braindead.
>
> Rather than have 32 client drivers expect fifo_address from the
> platform and then
> provide to the DMAC, IMHO it is better for a single driver(DMAC) to
> get 32 addresses
> from the platform.
My idea (when I implemented it) is that the device driver knows the
physical and virtual base and thus the address where DMA data is
destined. It knows all other registers, it remaps them, noone else should
be bothered with containing the knowledge of adress ranges for the
specific hardware block.
Passing this through platform data requires the machine to keep track
not only of the base adress of the peripheral (as is generally contained
in the machine or device tree) but also the offset of specific registers
in that memory region, which is too much.
Usually this means the offsets are defined in files like <mach/perfoo.h>
which means they can't be pushed down into drivers/foo/foo.c and
creates unnecessary bindings and de-encapsulation of the driver.
We want to get rid of such stuff. (I think?)
Therefore I introduced this to confine the different registers into
the driver itself and ask the DMA engine to transfer to a specific
address.
> Esp when the DMAC driver already expect similar h/w
> parameter -- *direction*.
>
> I don't understand why is 'fifo_address' a property much different
> than 'direction' of the
> channel ?
struct dma_slave_config {
enum dma_data_direction direction;
dma_addr_t src_addr;
dma_addr_t dst_addr;
enum dma_slave_buswidth src_addr_width;
enum dma_slave_buswidth dst_addr_width;
u32 src_maxburst;
u32 dst_maxburst;
};
We do support changing direction as well as src and dst address
(i.e. FIFO endpoint) at runtime.
Check drivers/mmc/host/mmci.c for an example of how we switch
a single channel from TX to RX in runtime using this one property.
However it has been noted by Russell that the direction member
is unnecessary since the device_prep_slave_sg() function in the
dmaengine vtable already takes a direction argument like this:
struct dma_async_tx_descriptor *(*device_prep_slave_sg)(
struct dma_chan *chan, struct scatterlist *sgl,
unsigned int sg_len, enum dma_data_direction direction,
unsigned long flags);
Therefore the direction setting needs to go from either the config
struct or the device_prep_slave_sg() call, as right now the channel
is being told about the direction twice which is not elegant and
confusing.
So you even have two ways of changing direction at runtime...
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
2011-07-21 9:14 ` Jassi Brar
@ 2011-07-21 11:29 ` Russell King - ARM Linux
-1 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2011-07-21 11:29 UTC (permalink / raw)
To: Jassi Brar
Cc: linux-samsung-soc, Boojin Kim, Vinod Koul, Kukjin Kim,
Dan Williams, linux-arm-kernel
On Thu, Jul 21, 2011 at 02:44:28PM +0530, Jassi Brar wrote:
> On Thu, Jul 21, 2011 at 1:41 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Thu, Jul 21, 2011 at 12:47:49AM +0530, Jassi Brar wrote:
> >> On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote:
> >> > + 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);
> >> > + } 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);
> >> > + }
> >> PL330 has fixed channels to peripherals.
> >> So FIFO addresses(burst_sz too?) should already be set via platform data.
> >> Client drivers shouldn't bother.
> >
> > That's utter crap, and isn't what the DMA engine API is about.
> >
> > The above looks correctly implemented. Slave DMA engine users are
> > supposed to supply the device DMA register address via this
> > DMA_SLAVE_CONFIG call. Doing this via platform data for the DMA
> > device is braindead.
>
> Rather than have 32 client drivers expect fifo_address from the
> platform and then
> provide to the DMAC, IMHO it is better for a single driver(DMAC) to
> get 32 addresses
> from the platform.
> Esp when the DMAC driver already expect similar h/w parameter -- *direction*.
Conversely, when a platform doesn't know where the FIFOs are because
they're located inside the actual devices, and the device driver does,
then it makes much more sense for the device driver to provide the
DMA engine with the bus address of that.
Does your hardware have a hardware block from the device itself containing
all the systems FIFOs ?
> I don't understand why is 'fifo_address' a property much different
> than 'direction' of the channel ?
Some channels can do memory-to-peripheral and peripheral-to-memory
transfers. Some peripherals provide a single set of DMA request/ack
lines to perform this function. Some peripherals have different
addresses for the TX and RX FIFOs within the device.
> If a channel is flexible enough to change it's 'fifo_address', most
> probably it could also change direction of transfers.
There are DMA engines and setups where that's true.
> So, why do we want to take seriously 'fifo_address' provided by the
> client drivers and not the 'direction' ?
The direction in the DMA slave config call I believe to be an annoying
overdesign which shouldn't be there - the DMA slave config call should
configure the DMA channel for the peripherals characteristics.
The actual channel direction should be setup at preparation time along
with the DMA buffer mapping etc.
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
@ 2011-07-21 11:29 ` Russell King - ARM Linux
0 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2011-07-21 11:29 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jul 21, 2011 at 02:44:28PM +0530, Jassi Brar wrote:
> On Thu, Jul 21, 2011 at 1:41 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Thu, Jul 21, 2011 at 12:47:49AM +0530, Jassi Brar wrote:
> >> On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote:
> >> > + ? ? ? ? ? ? ? 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);
> >> > + ? ? ? ? ? ? ? } 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);
> >> > + ? ? ? ? ? ? ? }
> >> PL330 has fixed channels to peripherals.
> >> So FIFO addresses(burst_sz too?) should already be set via platform data.
> >> Client drivers shouldn't bother.
> >
> > That's utter crap, and isn't what the DMA engine API is about.
> >
> > The above looks correctly implemented. ?Slave DMA engine users are
> > supposed to supply the device DMA register address via this
> > DMA_SLAVE_CONFIG call. ?Doing this via platform data for the DMA
> > device is braindead.
>
> Rather than have 32 client drivers expect fifo_address from the
> platform and then
> provide to the DMAC, IMHO it is better for a single driver(DMAC) to
> get 32 addresses
> from the platform.
> Esp when the DMAC driver already expect similar h/w parameter -- *direction*.
Conversely, when a platform doesn't know where the FIFOs are because
they're located inside the actual devices, and the device driver does,
then it makes much more sense for the device driver to provide the
DMA engine with the bus address of that.
Does your hardware have a hardware block from the device itself containing
all the systems FIFOs ?
> I don't understand why is 'fifo_address' a property much different
> than 'direction' of the channel ?
Some channels can do memory-to-peripheral and peripheral-to-memory
transfers. Some peripherals provide a single set of DMA request/ack
lines to perform this function. Some peripherals have different
addresses for the TX and RX FIFOs within the device.
> If a channel is flexible enough to change it's 'fifo_address', most
> probably it could also change direction of transfers.
There are DMA engines and setups where that's true.
> So, why do we want to take seriously 'fifo_address' provided by the
> client drivers and not the 'direction' ?
The direction in the DMA slave config call I believe to be an annoying
overdesign which shouldn't be there - the DMA slave config call should
configure the DMA channel for the peripherals characteristics.
The actual channel direction should be setup at preparation time along
with the DMA buffer mapping etc.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
2011-07-21 10:23 ` Linus Walleij
@ 2011-07-21 14:28 ` Jassi Brar
-1 siblings, 0 replies; 52+ messages in thread
From: Jassi Brar @ 2011-07-21 14:28 UTC (permalink / raw)
To: Linus Walleij
Cc: Kukjin Kim, Russell King - ARM Linux, Boojin Kim, Vinod Koul,
linux-samsung-soc, Dan Williams, linux-arm-kernel
On Thu, Jul 21, 2011 at 3:53 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Jul 21, 2011 at 11:14 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>> On Thu, Jul 21, 2011 at 1:41 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>> On Thu, Jul 21, 2011 at 12:47:49AM +0530, Jassi Brar wrote:
>>>> PL330 has fixed channels to peripherals.
>>>> So FIFO addresses(burst_sz too?) should already be set via platform data.
>>>> Client drivers shouldn't bother.
>>>
>>> That's utter crap, and isn't what the DMA engine API is about.
>>>
>>> The above looks correctly implemented. Slave DMA engine users are
>>> supposed to supply the device DMA register address via this
>>> DMA_SLAVE_CONFIG call. Doing this via platform data for the DMA
>>> device is braindead.
>>
>> Rather than have 32 client drivers expect fifo_address from the
>> platform and then
>> provide to the DMAC, IMHO it is better for a single driver(DMAC) to
>> get 32 addresses
>> from the platform.
>
> My idea (when I implemented it) is that the device driver knows the
> physical and virtual base and thus the address where DMA data is
> destined. It knows all other registers, it remaps them, noone else should
> be bothered with containing the knowledge of adress ranges for the
> specific hardware block.
>
> Passing this through platform data requires the machine to keep track
> not only of the base adress of the peripheral (as is generally contained
> in the machine or device tree) but also the offset of specific registers
> in that memory region, which is too much.
>
> Usually this means the offsets are defined in files like <mach/perfoo.h>
> which means they can't be pushed down into drivers/foo/foo.c and
> creates unnecessary bindings and de-encapsulation of the driver.
> We want to get rid of such stuff. (I think?)
>
> Therefore I introduced this to confine the different registers into
> the driver itself and ask the DMA engine to transfer to a specific
> address.
While that does make sense, it makes mandatory the ritual of calling
DMA_SLAVE_CONFIG mandatory for most of the cases.
And I think the effort to set fifo_addr for peripherals is overrated.
>
>> Esp when the DMAC driver already expect similar h/w
>> parameter -- *direction*.
>>
>> I don't understand why is 'fifo_address' a property much different
>> than 'direction' of the
>> channel ?
>
> struct dma_slave_config {
> enum dma_data_direction direction;
> dma_addr_t src_addr;
> dma_addr_t dst_addr;
> enum dma_slave_buswidth src_addr_width;
> enum dma_slave_buswidth dst_addr_width;
> u32 src_maxburst;
> u32 dst_maxburst;
> };
>
> We do support changing direction as well as src and dst address
> (i.e. FIFO endpoint) at runtime.
>
> Check drivers/mmc/host/mmci.c for an example of how we switch
> a single channel from TX to RX in runtime using this one property.
>
> However it has been noted by Russell that the direction member
> is unnecessary since the device_prep_slave_sg() function in the
> dmaengine vtable already takes a direction argument like this:
>
> struct dma_async_tx_descriptor *(*device_prep_slave_sg)(
> struct dma_chan *chan, struct scatterlist *sgl,
> unsigned int sg_len, enum dma_data_direction direction,
> unsigned long flags);
>
> Therefore the direction setting needs to go from either the config
> struct or the device_prep_slave_sg() call, as right now the channel
> is being told about the direction twice which is not elegant and
> confusing.
>
> So you even have two ways of changing direction at runtime...
Of course there are ways to set the direction... but whatever we do
it can't really be changed from what h/w can only do.
And that is my point. Let clients not bother trying to 'configure' what is
the only supported option by h/w.
And I don't suggest dropping the DMA_SLAVE_CONFIG callback - just
keep it for rarer situations when we have configurable channels.
And I assumed that was your original idea too.
-----------------------
* @DMA_SLAVE_CONFIG: this command is only implemented by DMA controllers
* that need to runtime reconfigure the slave channels (as opposed to passing
* configuration data in statically from the platform). An additional
* argument of struct dma_slave_config must be passed in with this
* command.
-----------------------
Regards,
-j
>
> Yours,
> Linus Walleij
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
@ 2011-07-21 14:28 ` Jassi Brar
0 siblings, 0 replies; 52+ messages in thread
From: Jassi Brar @ 2011-07-21 14:28 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jul 21, 2011 at 3:53 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Jul 21, 2011 at 11:14 AM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>> On Thu, Jul 21, 2011 at 1:41 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>> On Thu, Jul 21, 2011 at 12:47:49AM +0530, Jassi Brar wrote:
>>>> PL330 has fixed channels to peripherals.
>>>> So FIFO addresses(burst_sz too?) should already be set via platform data.
>>>> Client drivers shouldn't bother.
>>>
>>> That's utter crap, and isn't what the DMA engine API is about.
>>>
>>> The above looks correctly implemented. ?Slave DMA engine users are
>>> supposed to supply the device DMA register address via this
>>> DMA_SLAVE_CONFIG call. ?Doing this via platform data for the DMA
>>> device is braindead.
>>
>> Rather than have 32 client drivers expect fifo_address from the
>> platform and then
>> provide to the DMAC, IMHO it is better for a single driver(DMAC) to
>> get 32 addresses
>> from the platform.
>
> My idea (when I implemented it) is that the device driver knows the
> physical and virtual base and thus the address where DMA data is
> destined. It knows all other registers, it remaps them, noone else should
> be bothered with containing the knowledge of adress ranges for the
> specific hardware block.
>
> Passing this through platform data requires the machine to keep track
> not only of the base adress of the peripheral (as is generally contained
> in the machine or device tree) but also the offset of specific registers
> in that memory region, which is too much.
>
> Usually this means the offsets are defined in files like <mach/perfoo.h>
> which means they can't be pushed down into drivers/foo/foo.c and
> creates unnecessary bindings and de-encapsulation of the driver.
> We want to get rid of such stuff. (I think?)
>
> Therefore I introduced this to confine the different registers into
> the driver itself and ask the DMA engine to transfer to a specific
> address.
While that does make sense, it makes mandatory the ritual of calling
DMA_SLAVE_CONFIG mandatory for most of the cases.
And I think the effort to set fifo_addr for peripherals is overrated.
>
>> Esp when the DMAC driver already expect similar h/w
>> parameter -- *direction*.
>>
>> I don't understand why is 'fifo_address' a property much different
>> than 'direction' of the
>> channel ?
>
> struct dma_slave_config {
> ? ? ? ?enum dma_data_direction direction;
> ? ? ? ?dma_addr_t src_addr;
> ? ? ? ?dma_addr_t dst_addr;
> ? ? ? ?enum dma_slave_buswidth src_addr_width;
> ? ? ? ?enum dma_slave_buswidth dst_addr_width;
> ? ? ? ?u32 src_maxburst;
> ? ? ? ?u32 dst_maxburst;
> };
>
> We do support changing direction as well as src and dst address
> (i.e. FIFO endpoint) at runtime.
>
> Check drivers/mmc/host/mmci.c for an example of how we switch
> a single channel from TX to RX in runtime using this one property.
>
> However it has been noted by Russell that the direction member
> is unnecessary since the device_prep_slave_sg() function in the
> dmaengine vtable already takes a direction argument like this:
>
> ? ? ? ?struct dma_async_tx_descriptor *(*device_prep_slave_sg)(
> ? ? ? ? ? ? ? ?struct dma_chan *chan, struct scatterlist *sgl,
> ? ? ? ? ? ? ? ?unsigned int sg_len, enum dma_data_direction direction,
> ? ? ? ? ? ? ? ?unsigned long flags);
>
> Therefore the direction setting needs to go from either the config
> struct or the device_prep_slave_sg() call, as right now the channel
> is being told about the direction twice which is not elegant and
> confusing.
>
> So you even have two ways of changing direction at runtime...
Of course there are ways to set the direction... but whatever we do
it can't really be changed from what h/w can only do.
And that is my point. Let clients not bother trying to 'configure' what is
the only supported option by h/w.
And I don't suggest dropping the DMA_SLAVE_CONFIG callback - just
keep it for rarer situations when we have configurable channels.
And I assumed that was your original idea too.
-----------------------
* @DMA_SLAVE_CONFIG: this command is only implemented by DMA controllers
* that need to runtime reconfigure the slave channels (as opposed to passing
* configuration data in statically from the platform). An additional
* argument of struct dma_slave_config must be passed in with this
* command.
-----------------------
Regards,
-j
>
> Yours,
> Linus Walleij
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
2011-07-21 11:29 ` Russell King - ARM Linux
@ 2011-07-21 15:12 ` Jassi Brar
-1 siblings, 0 replies; 52+ messages in thread
From: Jassi Brar @ 2011-07-21 15:12 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: linux-samsung-soc, Boojin Kim, Vinod Koul, Kukjin Kim,
Dan Williams, linux-arm-kernel
On Thu, Jul 21, 2011 at 4:59 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
>> >> PL330 has fixed channels to peripherals.
>> >> So FIFO addresses(burst_sz too?) should already be set via platform data.
>> >> Client drivers shouldn't bother.
>> >
>> > That's utter crap, and isn't what the DMA engine API is about.
>> >
>> > The above looks correctly implemented. Slave DMA engine users are
>> > supposed to supply the device DMA register address via this
>> > DMA_SLAVE_CONFIG call. Doing this via platform data for the DMA
>> > device is braindead.
>>
>> Rather than have 32 client drivers expect fifo_address from the
>> platform and then
>> provide to the DMAC, IMHO it is better for a single driver(DMAC) to
>> get 32 addresses
>> from the platform.
>> Esp when the DMAC driver already expect similar h/w parameter -- *direction*.
>
> Conversely, when a platform doesn't know where the FIFOs are because
> they're located inside the actual devices, and the device driver does,
> then it makes much more sense for the device driver to provide the
> DMA engine with the bus address of that.
Yes, that is a case to consider.
Perhaps we already do something like that while setting i2c addresses
of attached devices in board files... and similarly it might be possible for the
developer to know what the fifo address is going to be after the device
is up and running esp when it is interfaced with an internal component
like DMA.
>
> Does your hardware have a hardware block from the device itself containing
> all the systems FIFOs ?
I am not sure what you ask, but let me say what I know.
In this case atleast, all PL330 DMA channels have fixed source/destination
address on the device side. So it's not like developer doesn't know
fifo_addr here.
>> I don't understand why is 'fifo_address' a property much different
>> than 'direction' of the channel ?
>
> Some channels can do memory-to-peripheral and peripheral-to-memory
> transfers. Some peripherals provide a single set of DMA request/ack
> lines to perform this function. Some peripherals have different
> addresses for the TX and RX FIFOs within the device.
Likewise we had something like that for I2S IP of S3C2440(or A0 ?)
a single fifo address shared by TX and RX channel.
Depending if it's full/half duplex capable we can have both or one
'virtual' channel active at a time.
>
>> If a channel is flexible enough to change it's 'fifo_address', most
>> probably it could also change direction of transfers.
>
> There are DMA engines and setups where that's true.
Of course, and I think this 'runtime' configuration should be done
only for such cases.
thnx
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
@ 2011-07-21 15:12 ` Jassi Brar
0 siblings, 0 replies; 52+ messages in thread
From: Jassi Brar @ 2011-07-21 15:12 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jul 21, 2011 at 4:59 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
>> >> PL330 has fixed channels to peripherals.
>> >> So FIFO addresses(burst_sz too?) should already be set via platform data.
>> >> Client drivers shouldn't bother.
>> >
>> > That's utter crap, and isn't what the DMA engine API is about.
>> >
>> > The above looks correctly implemented. ?Slave DMA engine users are
>> > supposed to supply the device DMA register address via this
>> > DMA_SLAVE_CONFIG call. ?Doing this via platform data for the DMA
>> > device is braindead.
>>
>> Rather than have 32 client drivers expect fifo_address from the
>> platform and then
>> provide to the DMAC, IMHO it is better for a single driver(DMAC) to
>> get 32 addresses
>> from the platform.
>> Esp when the DMAC driver already expect similar h/w parameter -- *direction*.
>
> Conversely, when a platform doesn't know where the FIFOs are because
> they're located inside the actual devices, and the device driver does,
> then it makes much more sense for the device driver to provide the
> DMA engine with the bus address of that.
Yes, that is a case to consider.
Perhaps we already do something like that while setting i2c addresses
of attached devices in board files... and similarly it might be possible for the
developer to know what the fifo address is going to be after the device
is up and running esp when it is interfaced with an internal component
like DMA.
>
> Does your hardware have a hardware block from the device itself containing
> all the systems FIFOs ?
I am not sure what you ask, but let me say what I know.
In this case atleast, all PL330 DMA channels have fixed source/destination
address on the device side. So it's not like developer doesn't know
fifo_addr here.
>> I don't understand why is 'fifo_address' a property much different
>> than 'direction' of the channel ?
>
> Some channels can do memory-to-peripheral and peripheral-to-memory
> transfers. ?Some peripherals provide a single set of DMA request/ack
> lines to perform this function. ?Some peripherals have different
> addresses for the TX and RX FIFOs within the device.
Likewise we had something like that for I2S IP of S3C2440(or A0 ?)
a single fifo address shared by TX and RX channel.
Depending if it's full/half duplex capable we can have both or one
'virtual' channel active at a time.
>
>> If a channel is flexible enough to change it's 'fifo_address', most
>> probably it could also change direction of transfers.
>
> There are DMA engines and setups where that's true.
Of course, and I think this 'runtime' configuration should be done
only for such cases.
thnx
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
2011-07-21 15:12 ` Jassi Brar
@ 2011-07-21 15:23 ` Russell King - ARM Linux
-1 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2011-07-21 15:23 UTC (permalink / raw)
To: Jassi Brar
Cc: linux-samsung-soc, Boojin Kim, Vinod Koul, Kukjin Kim,
Dan Williams, linux-arm-kernel
On Thu, Jul 21, 2011 at 08:42:40PM +0530, Jassi Brar wrote:
> On Thu, Jul 21, 2011 at 4:59 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > Does your hardware have a hardware block from the device itself containing
> > all the systems FIFOs ?
> I am not sure what you ask, but let me say what I know.
> In this case atleast, all PL330 DMA channels have fixed source/destination
> address on the device side. So it's not like developer doesn't know
> fifo_addr here.
Even so, your approach is _conceptually_ wrong. Think about it.
You declare your devices giving the bus address where they're located.
So, lets say for argument that your UART is located at 0x10001000.
Your UART driver knows that the FIFO register is at offset 0x20 from
the base address. Your platform data provides the UART driver with a
DMA match function and data specific for that match function. This
data encodes the specific DMA channel.
Now, why should you have to encode into the DMA drivers platform data
that DMA channel X has its FIFO at 0x10001020? Not only do you have
to declare the base address of the UART but also you have to know the
offset into the UART.
Why not just let the UART driver - which knows that the base address
is 0x10001000, and the FIFO is at offset 0x20 above that - tell the
DMA driver that's where the FIFO is located?
Finally, your specific SoC may have a PL330, which may have FIFOs tied
to specific DMA request signals. That's an _implementation_ _detail_.
That doesn't mean that's always going to be the case.
I've heard that ARM Ltd may start using the PL330 on their evaluation
boards. They have a habbit of muxing several DMA request signals to
several different peripherals. Will I need to rewrite all the Samsung
users of PL330 when that happens because they've decided they don't
like the DMA engine API and have gone off and done their own thing
instead?
So no, encoding the FIFO addresses into platform data for the DMA
controller is brain dead and totally the wrong thing to do. And when
you come to moving stuff over to DT, you'll see that's even more true
there.
So don't make the mistake now. Do things sensibly and follow the DMA
engine API. Arrange for all your drivers to call DMA_SLAVE_CONFIG
with the address of the FIFO.
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
@ 2011-07-21 15:23 ` Russell King - ARM Linux
0 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2011-07-21 15:23 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jul 21, 2011 at 08:42:40PM +0530, Jassi Brar wrote:
> On Thu, Jul 21, 2011 at 4:59 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > Does your hardware have a hardware block from the device itself containing
> > all the systems FIFOs ?
> I am not sure what you ask, but let me say what I know.
> In this case atleast, all PL330 DMA channels have fixed source/destination
> address on the device side. So it's not like developer doesn't know
> fifo_addr here.
Even so, your approach is _conceptually_ wrong. Think about it.
You declare your devices giving the bus address where they're located.
So, lets say for argument that your UART is located at 0x10001000.
Your UART driver knows that the FIFO register is at offset 0x20 from
the base address. Your platform data provides the UART driver with a
DMA match function and data specific for that match function. This
data encodes the specific DMA channel.
Now, why should you have to encode into the DMA drivers platform data
that DMA channel X has its FIFO at 0x10001020? Not only do you have
to declare the base address of the UART but also you have to know the
offset into the UART.
Why not just let the UART driver - which knows that the base address
is 0x10001000, and the FIFO is at offset 0x20 above that - tell the
DMA driver that's where the FIFO is located?
Finally, your specific SoC may have a PL330, which may have FIFOs tied
to specific DMA request signals. That's an _implementation_ _detail_.
That doesn't mean that's always going to be the case.
I've heard that ARM Ltd may start using the PL330 on their evaluation
boards. They have a habbit of muxing several DMA request signals to
several different peripherals. Will I need to rewrite all the Samsung
users of PL330 when that happens because they've decided they don't
like the DMA engine API and have gone off and done their own thing
instead?
So no, encoding the FIFO addresses into platform data for the DMA
controller is brain dead and totally the wrong thing to do. And when
you come to moving stuff over to DT, you'll see that's even more true
there.
So don't make the mistake now. Do things sensibly and follow the DMA
engine API. Arrange for all your drivers to call DMA_SLAVE_CONFIG
with the address of the FIFO.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
2011-07-21 14:28 ` Jassi Brar
@ 2011-07-21 15:28 ` Russell King - ARM Linux
-1 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2011-07-21 15:28 UTC (permalink / raw)
To: Jassi Brar
Cc: Kukjin Kim, Boojin Kim, Vinod Koul, Linus Walleij,
linux-samsung-soc, Dan Williams, linux-arm-kernel
On Thu, Jul 21, 2011 at 07:58:03PM +0530, Jassi Brar wrote:
> While that does make sense, it makes mandatory the ritual of calling
> DMA_SLAVE_CONFIG mandatory for most of the cases.
> And I think the effort to set fifo_addr for peripherals is overrated.
You have to do that anyway. You're dealing with DMA engine drivers which
you don't know the implementation details of.
Think about it for a moment, please. The point of the DMA engine
abstraction is to make DMA users independent of the DMA engine actually
being used.
That means all DMA engine drivers and all DMA engine users have to
provide a basic set of information to allow them to interact with each
other.
Encoding random bits of information into inappropriate places like FIFO
addresses into DMA engine platform data just means you might was well
stay with your private API, because that driver may never be able to
work with another DMA engine implementation.
So please, either do the job properly, or don't waste peoples time with
a half hearted mess of inappropriate platform data.
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
@ 2011-07-21 15:28 ` Russell King - ARM Linux
0 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2011-07-21 15:28 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jul 21, 2011 at 07:58:03PM +0530, Jassi Brar wrote:
> While that does make sense, it makes mandatory the ritual of calling
> DMA_SLAVE_CONFIG mandatory for most of the cases.
> And I think the effort to set fifo_addr for peripherals is overrated.
You have to do that anyway. You're dealing with DMA engine drivers which
you don't know the implementation details of.
Think about it for a moment, please. The point of the DMA engine
abstraction is to make DMA users independent of the DMA engine actually
being used.
That means all DMA engine drivers and all DMA engine users have to
provide a basic set of information to allow them to interact with each
other.
Encoding random bits of information into inappropriate places like FIFO
addresses into DMA engine platform data just means you might was well
stay with your private API, because that driver may never be able to
work with another DMA engine implementation.
So please, either do the job properly, or don't waste peoples time with
a half hearted mess of inappropriate platform data.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
2011-07-21 15:23 ` Russell King - ARM Linux
@ 2011-07-21 15:56 ` Jassi Brar
-1 siblings, 0 replies; 52+ messages in thread
From: Jassi Brar @ 2011-07-21 15:56 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: linux-samsung-soc, Boojin Kim, Vinod Koul, Kukjin Kim,
Dan Williams, linux-arm-kernel
On Thu, Jul 21, 2011 at 8:53 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Jul 21, 2011 at 08:42:40PM +0530, Jassi Brar wrote:
>> On Thu, Jul 21, 2011 at 4:59 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > Does your hardware have a hardware block from the device itself containing
>> > all the systems FIFOs ?
>> I am not sure what you ask, but let me say what I know.
>> In this case atleast, all PL330 DMA channels have fixed source/destination
>> address on the device side. So it's not like developer doesn't know
>> fifo_addr here.
>
> Even so, your approach is _conceptually_ wrong. Think about it.
>
> You declare your devices giving the bus address where they're located.
> So, lets say for argument that your UART is located at 0x10001000.
>
> Your UART driver knows that the FIFO register is at offset 0x20 from
> the base address. Your platform data provides the UART driver with a
> DMA match function and data specific for that match function. This
> data encodes the specific DMA channel.
>
> Now, why should you have to encode into the DMA drivers platform data
> that DMA channel X has its FIFO at 0x10001020? Not only do you have
> to declare the base address of the UART but also you have to know the
> offset into the UART.
>
> Why not just let the UART driver - which knows that the base address
> is 0x10001000, and the FIFO is at offset 0x20 above that - tell the
> DMA driver that's where the FIFO is located?
Yes I understand, the idea was to avoid optional DMA_SLAVE_CONFIG call.
Apparently we give different weightage to the pros and cons.
Anyways, I accept your opinion.
Though you might want to consider changing the DMA_SLAVE_CONFIG API from
optional to mandatory for Slave capable DMACs. Otherwise I don't see common
client drivers working over different DMACs.
Thanks.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
@ 2011-07-21 15:56 ` Jassi Brar
0 siblings, 0 replies; 52+ messages in thread
From: Jassi Brar @ 2011-07-21 15:56 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jul 21, 2011 at 8:53 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Jul 21, 2011 at 08:42:40PM +0530, Jassi Brar wrote:
>> On Thu, Jul 21, 2011 at 4:59 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > Does your hardware have a hardware block from the device itself containing
>> > all the systems FIFOs ?
>> I am not sure what you ask, but let me say what I know.
>> In this case atleast, all PL330 DMA channels have fixed source/destination
>> address on the device side. So it's not like developer doesn't know
>> fifo_addr here.
>
> Even so, your approach is _conceptually_ wrong. ?Think about it.
>
> You declare your devices giving the bus address where they're located.
> So, lets say for argument that your UART is located at 0x10001000.
>
> Your UART driver knows that the FIFO register is at offset 0x20 from
> the base address. ?Your platform data provides the UART driver with a
> DMA match function and data specific for that match function. ?This
> data encodes the specific DMA channel.
>
> Now, why should you have to encode into the DMA drivers platform data
> that DMA channel X has its FIFO at 0x10001020? ?Not only do you have
> to declare the base address of the UART but also you have to know the
> offset into the UART.
>
> Why not just let the UART driver - which knows that the base address
> is 0x10001000, and the FIFO is at offset 0x20 above that - tell the
> DMA driver that's where the FIFO is located?
Yes I understand, the idea was to avoid optional DMA_SLAVE_CONFIG call.
Apparently we give different weightage to the pros and cons.
Anyways, I accept your opinion.
Though you might want to consider changing the DMA_SLAVE_CONFIG API from
optional to mandatory for Slave capable DMACs. Otherwise I don't see common
client drivers working over different DMACs.
Thanks.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
2011-07-21 15:56 ` Jassi Brar
@ 2011-07-21 16:02 ` Russell King - ARM Linux
-1 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2011-07-21 16:02 UTC (permalink / raw)
To: Jassi Brar
Cc: linux-samsung-soc, Boojin Kim, Vinod Koul, Kukjin Kim,
Dan Williams, linux-arm-kernel
On Thu, Jul 21, 2011 at 09:26:22PM +0530, Jassi Brar wrote:
> Though you might want to consider changing the DMA_SLAVE_CONFIG API from
> optional to mandatory for Slave capable DMACs. Otherwise I don't see common
> client drivers working over different DMACs.
Yes, I think we will have to, otherwise we can't ensure we have cross-
compatibility between different DMA engine implementations.
We already have platforms where we have peripheral drivers making use of
several different DMA engine implementations, so nailing this down now,
before the number of DMA engine implementations increases still further
is definitely something that needs doing.
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
@ 2011-07-21 16:02 ` Russell King - ARM Linux
0 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2011-07-21 16:02 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jul 21, 2011 at 09:26:22PM +0530, Jassi Brar wrote:
> Though you might want to consider changing the DMA_SLAVE_CONFIG API from
> optional to mandatory for Slave capable DMACs. Otherwise I don't see common
> client drivers working over different DMACs.
Yes, I think we will have to, otherwise we can't ensure we have cross-
compatibility between different DMA engine implementations.
We already have platforms where we have peripheral drivers making use of
several different DMA engine implementations, so nailing this down now,
before the number of DMA engine implementations increases still further
is definitely something that needs doing.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
2011-07-20 10:46 ` Boojin Kim
@ 2011-07-22 5:42 ` Jassi Brar
-1 siblings, 0 replies; 52+ messages in thread
From: Jassi Brar @ 2011-07-22 5:42 UTC (permalink / raw)
To: Boojin Kim
Cc: linux-arm-kernel, linux-samsung-soc, Vinod Koul, Dan Williams,
Kukjin Kim
On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote:
> Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
> Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> ---
> drivers/dma/pl330.c | 53 +++++++++++++++++++++++++++++++++++++-------------
> 1 files changed, 39 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 586ab39..880f010 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -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);
> + 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;
> + if (slave_config->dst_addr_width)
> + peri->burst_sz = __ffs(slave_config->dst_addr_width);
> + } 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);
> + }
> + break;
As discussed yesterday, DMA_SLAVE_CONFIG is assumed to be madatory,
so please dismantle the 'struct dma_pl330_peri', move fifo_addr,
burst_sz and rqtype
to 'struct dma_pl330_chan' and poison them - that will force clients
to do DMA_SLAVE_CONFIG. Move 'peri_id' to 'struct dma_pl330_platdata'
And protect the changes to channel parameters with lock.
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
@ 2011-07-22 5:42 ` Jassi Brar
0 siblings, 0 replies; 52+ messages in thread
From: Jassi Brar @ 2011-07-22 5:42 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote:
> Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
> Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> ---
> ?drivers/dma/pl330.c | ? 53 +++++++++++++++++++++++++++++++++++++-------------
> ?1 files changed, 39 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 586ab39..880f010 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -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);
> + ? ? ? ? ? ? ? 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;
> + ? ? ? ? ? ? ? ? ? ? ? if (slave_config->dst_addr_width)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? peri->burst_sz = __ffs(slave_config->dst_addr_width);
> + ? ? ? ? ? ? ? } 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);
> + ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? break;
As discussed yesterday, DMA_SLAVE_CONFIG is assumed to be madatory,
so please dismantle the 'struct dma_pl330_peri', move fifo_addr,
burst_sz and rqtype
to 'struct dma_pl330_chan' and poison them - that will force clients
to do DMA_SLAVE_CONFIG. Move 'peri_id' to 'struct dma_pl330_platdata'
And protect the changes to channel parameters with lock.
^ permalink raw reply [flat|nested] 52+ messages in thread
* RE: [PATCH V4 00/13] To use DMA generic APIs for Samsung DMA
2011-07-20 10:46 ` Boojin Kim
@ 2011-07-22 12:07 ` Koul, Vinod
-1 siblings, 0 replies; 52+ messages in thread
From: Koul, Vinod @ 2011-07-22 12:07 UTC (permalink / raw)
To: Boojin Kim, linux-arm-kernel, linux-samsung-soc
Cc: Williams, Dan J, Jassi Brar, Kukjin Kim
>
> This patchset adds support DMA generic APIs for Samsung DMA.
> V4 doesn't has whole patch series and only has the patches differed from V3.
>
> Changes from V3:
> - Divided 03-patch into two patchs.
> - First is 03-1-patch for DMA_SLAVE_CONFIG command.
> - Another is 03-2-patch for DMA_CYCLIC capability.
> - Code clean-up
> - Remove unnecessary variable referance.
> - Remove redunancy code
>
> [PATCH V4 01/13] DMA: PL330: Add support runtime PM for PL330 DMAC
> [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
> [PATCH V4 03-2/13] DMA: PL330: Add DMA_CYCLIC capability
> [PATCH V4 05/13] ARM: SAMSUNG: Add common DMA operations
Can you pls send entire patchset again,
it makes things difficult to review patches
~Vinod
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 00/13] To use DMA generic APIs for Samsung DMA
@ 2011-07-22 12:07 ` Koul, Vinod
0 siblings, 0 replies; 52+ messages in thread
From: Koul, Vinod @ 2011-07-22 12:07 UTC (permalink / raw)
To: linux-arm-kernel
>
> This patchset adds support DMA generic APIs for Samsung DMA.
> V4 doesn't has whole patch series and only has the patches differed from V3.
>
> Changes from V3:
> - Divided 03-patch into two patchs.
> - First is 03-1-patch for DMA_SLAVE_CONFIG command.
> - Another is 03-2-patch for DMA_CYCLIC capability.
> - Code clean-up
> - Remove unnecessary variable referance.
> - Remove redunancy code
>
> [PATCH V4 01/13] DMA: PL330: Add support runtime PM for PL330 DMAC
> [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
> [PATCH V4 03-2/13] DMA: PL330: Add DMA_CYCLIC capability
> [PATCH V4 05/13] ARM: SAMSUNG: Add common DMA operations
Can you pls send entire patchset again,
it makes things difficult to review patches
~Vinod
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
2011-07-21 14:28 ` Jassi Brar
@ 2011-07-22 13:59 ` Linus Walleij
-1 siblings, 0 replies; 52+ messages in thread
From: Linus Walleij @ 2011-07-22 13:59 UTC (permalink / raw)
To: Jassi Brar
Cc: Russell King - ARM Linux, linux-samsung-soc, Boojin Kim,
Vinod Koul, Kukjin Kim, Dan Williams, linux-arm-kernel
On Thu, Jul 21, 2011 at 4:28 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> [Me]
>> Therefore I introduced this to confine the different registers into
>> the driver itself and ask the DMA engine to transfer to a specific
>> address.
>
> While that does make sense, it makes mandatory the ritual of calling
> DMA_SLAVE_CONFIG mandatory for most of the cases.
> And I think the effort to set fifo_addr for peripherals is overrated.
Oh well. I beg to differ, maybe I'm poisoned by stuff like this:
http://en.wikipedia.org/wiki/Encapsulation_%28object-oriented_programming%29
so let's say we agree to disagree on this then.
> Of course there are ways to set the direction... but whatever we do
> it can't really be changed from what h/w can only do.
> And that is my point. Let clients not bother trying to 'configure' what is
> the only supported option by h/w.
The interface is generic, and as is demonstrated in the U300 COH901318
and also I think ARM RealView and Versatile reference designs, the
DMA channel for the MMCI block is bidirectional, so you really change
the direction of the channel at runtime, it's not like I'm making it up
and introducing that possibility just for fun :-D
So the generic case is that you can request a direction for the channel,
and if the hardware doesn't support that, then NAK any tries to set it
to a direction which is illegal.
Yes, some abstraction but generalization does have a price, and I think
it's worth it.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
@ 2011-07-22 13:59 ` Linus Walleij
0 siblings, 0 replies; 52+ messages in thread
From: Linus Walleij @ 2011-07-22 13:59 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jul 21, 2011 at 4:28 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> [Me]
>> Therefore I introduced this to confine the different registers into
>> the driver itself and ask the DMA engine to transfer to a specific
>> address.
>
> While that does make sense, it makes mandatory the ritual of calling
> DMA_SLAVE_CONFIG mandatory for most of the cases.
> And I think the effort to set fifo_addr for peripherals is overrated.
Oh well. I beg to differ, maybe I'm poisoned by stuff like this:
http://en.wikipedia.org/wiki/Encapsulation_%28object-oriented_programming%29
so let's say we agree to disagree on this then.
> Of course there are ways to set the direction... but whatever we do
> it can't really be changed from what h/w can only do.
> And that is my point. ?Let clients not bother trying to 'configure' what is
> the only supported option by h/w.
The interface is generic, and as is demonstrated in the U300 COH901318
and also I think ARM RealView and Versatile reference designs, the
DMA channel for the MMCI block is bidirectional, so you really change
the direction of the channel at runtime, it's not like I'm making it up
and introducing that possibility just for fun :-D
So the generic case is that you can request a direction for the channel,
and if the hardware doesn't support that, then NAK any tries to set it
to a direction which is illegal.
Yes, some abstraction but generalization does have a price, and I think
it's worth it.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 52+ messages in thread
* RE: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
2011-07-22 5:42 ` Jassi Brar
@ 2011-07-25 2:10 ` Boojin Kim
-1 siblings, 0 replies; 52+ messages in thread
From: Boojin Kim @ 2011-07-25 2:10 UTC (permalink / raw)
To: 'Jassi Brar'
Cc: linux-arm-kernel, linux-samsung-soc, 'Vinod Koul',
'Dan Williams', 'Kukjin Kim'
Jassi Brar wrote:
> Sent: Friday, July 22, 2011 2:42 PM
> To: Boojin Kim
> Cc: linux-arm-kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org;
> Vinod Koul; Dan Williams; Kukjin Kim
> Subject: Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
>
> On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote:
> > Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
> > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> > ---
> > drivers/dma/pl330.c | 53
> > +++++++++++++++++++++++++++++++++++++----------
> ---
> > 1 files changed, 39 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> > index 586ab39..880f010 100644
> > --- a/drivers/dma/pl330.c
> > +++ b/drivers/dma/pl330.c
> > @@ -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);
> > + 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;
> > + if (slave_config->dst_addr_width)
> > + peri->burst_sz = __ffs(slave_config-
> >dst_addr_width);
> > + } 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);
> > + }
> > + break;
> As discussed yesterday, DMA_SLAVE_CONFIG is assumed to be madatory,
> so please dismantle the 'struct dma_pl330_peri', move fifo_addr,
> burst_sz and rqtype
> to 'struct dma_pl330_chan' and poison them - that will force clients
> to do DMA_SLAVE_CONFIG. Move 'peri_id' to 'struct dma_pl330_platdata'
> And protect the changes to channel parameters with lock.
It's fine that you understand my implementation.
I'm agree to remove unnecessary structure.
But, this item should bring the change of all DMA machine code and doesn't has
deeply relationship with the purpose of this patch series (samsung dma usage).
So, I'd like to make it to another patch and submit it.
How about my opinion ?
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
@ 2011-07-25 2:10 ` Boojin Kim
0 siblings, 0 replies; 52+ messages in thread
From: Boojin Kim @ 2011-07-25 2:10 UTC (permalink / raw)
To: linux-arm-kernel
Jassi Brar wrote:
> Sent: Friday, July 22, 2011 2:42 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
> Subject: Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
>
> On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim <boojin.kim@samsung.com> wrote:
> > Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
> > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> > ---
> > drivers/dma/pl330.c | 53
> > +++++++++++++++++++++++++++++++++++++----------
> ---
> > 1 files changed, 39 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> > index 586ab39..880f010 100644
> > --- a/drivers/dma/pl330.c
> > +++ b/drivers/dma/pl330.c
> > @@ -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);
> > + 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;
> > + if (slave_config->dst_addr_width)
> > + peri->burst_sz = __ffs(slave_config-
> >dst_addr_width);
> > + } 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);
> > + }
> > + break;
> As discussed yesterday, DMA_SLAVE_CONFIG is assumed to be madatory,
> so please dismantle the 'struct dma_pl330_peri', move fifo_addr,
> burst_sz and rqtype
> to 'struct dma_pl330_chan' and poison them - that will force clients
> to do DMA_SLAVE_CONFIG. Move 'peri_id' to 'struct dma_pl330_platdata'
> And protect the changes to channel parameters with lock.
It's fine that you understand my implementation.
I'm agree to remove unnecessary structure.
But, this item should bring the change of all DMA machine code and doesn't has
deeply relationship with the purpose of this patch series (samsung dma usage).
So, I'd like to make it to another patch and submit it.
How about my opinion ?
^ permalink raw reply [flat|nested] 52+ messages in thread
end of thread, other threads:[~2011-07-25 2:11 UTC | newest]
Thread overview: 52+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-20 10:46 [PATCH V4 00/13] To use DMA generic APIs for Samsung DMA Boojin Kim
2011-07-20 10:46 ` Boojin Kim
2011-07-20 10:46 ` [PATCH V4 01/13] DMA: PL330: Add support runtime PM for PL330 DMAC Boojin Kim
2011-07-20 10:46 ` Boojin Kim
2011-07-20 18:35 ` Jassi Brar
2011-07-20 18:35 ` Jassi Brar
2011-07-20 10:46 ` [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command Boojin Kim
2011-07-20 10:46 ` Boojin Kim
2011-07-20 19:17 ` Jassi Brar
2011-07-20 19:17 ` Jassi Brar
2011-07-21 4:13 ` Boojin Kim
2011-07-21 4:13 ` Boojin Kim
2011-07-21 5:00 ` Jassi Brar
2011-07-21 5:00 ` Jassi Brar
2011-07-21 6:34 ` Boojin Kim
2011-07-21 6:34 ` Boojin Kim
2011-07-21 7:31 ` Jassi Brar
2011-07-21 7:31 ` Jassi Brar
2011-07-21 8:11 ` Russell King - ARM Linux
2011-07-21 8:11 ` Russell King - ARM Linux
2011-07-21 9:14 ` Jassi Brar
2011-07-21 9:14 ` Jassi Brar
2011-07-21 10:23 ` Linus Walleij
2011-07-21 10:23 ` Linus Walleij
2011-07-21 14:28 ` Jassi Brar
2011-07-21 14:28 ` Jassi Brar
2011-07-21 15:28 ` Russell King - ARM Linux
2011-07-21 15:28 ` Russell King - ARM Linux
2011-07-22 13:59 ` Linus Walleij
2011-07-22 13:59 ` Linus Walleij
2011-07-21 11:29 ` Russell King - ARM Linux
2011-07-21 11:29 ` Russell King - ARM Linux
2011-07-21 15:12 ` Jassi Brar
2011-07-21 15:12 ` Jassi Brar
2011-07-21 15:23 ` Russell King - ARM Linux
2011-07-21 15:23 ` Russell King - ARM Linux
2011-07-21 15:56 ` Jassi Brar
2011-07-21 15:56 ` Jassi Brar
2011-07-21 16:02 ` Russell King - ARM Linux
2011-07-21 16:02 ` Russell King - ARM Linux
2011-07-22 5:42 ` Jassi Brar
2011-07-22 5:42 ` Jassi Brar
2011-07-25 2:10 ` Boojin Kim
2011-07-25 2:10 ` Boojin Kim
2011-07-20 10:46 ` [PATCH V4 03-2/13] DMA: PL330: Add DMA_CYCLIC capability Boojin Kim
2011-07-20 10:46 ` Boojin Kim
2011-07-20 10:46 ` [PATCH V4 05/13] ARM: SAMSUNG: Add common DMA operations Boojin Kim
2011-07-20 10:46 ` Boojin Kim
2011-07-20 16:04 ` [PATCH V4 00/13] To use DMA generic APIs for Samsung DMA Kukjin Kim
2011-07-20 16:04 ` Kukjin Kim
2011-07-22 12:07 ` Koul, Vinod
2011-07-22 12:07 ` Koul, Vinod
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.