[PATCH] DMAEngine: Let dmac drivers set chan_id

[PATCH] DMAEngine: Let dmac drivers set chan_id

[Jassi Brar]

A DMAC driver might put chan_id to better use by directly
mapping system-wide channel numbers on them (and not DMAC
relative index). Now client drivers can employ simpler
dma_filter_fn callbacks.

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---

The rest of DMAC drivers already assign chan_id (albeit
only to be overwritten by core driver).

 drivers/dma/amba-pl08x.c  |    1 +
 drivers/dma/coh901318.c   |    3 ++-
 drivers/dma/dmaengine.c   |    1 -
 drivers/dma/fsldma.c      |    2 +-
 drivers/dma/imx-dma.c     |    1 +
 drivers/dma/imx-sdma.c    |    4 +++-
 drivers/dma/ioat/dma.c    |    1 +
 drivers/dma/iop-adma.c    |    1 +
 drivers/dma/mv_xor.c      |    1 +
 drivers/dma/mxs-dma.c     |    2 +-
 drivers/dma/ppc4xx/adma.c |    1 +
 drivers/dma/shdma.c       |    1 +
 drivers/dma/ste_dma40.c   |    1 +
 drivers/dma/txx9dmac.c    |    1 +
 14 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index e6d7228..40083ef 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1738,6 +1738,7 @@ static int pl08x_dma_init_virtual_channels(struct pl08x_driver_data *pl08x,
 		tasklet_init(&chan->tasklet, pl08x_tasklet,
 			     (unsigned long) chan);
 
+		chan->chan.chan_id = i;
 		list_add_tail(&chan->chan.device_node, &dmadev->channels);
 	}
 	dev_info(&pl08x->adev->dev, "initialized %d virtual %s channels\n",
diff --git a/drivers/dma/coh901318.c b/drivers/dma/coh901318.c
index af8c0b5..3d8b67a 100644
--- a/drivers/dma/coh901318.c
+++ b/drivers/dma/coh901318.c
@@ -1414,7 +1414,7 @@ void coh901318_base_init(struct dma_device *dma, const int *pick_chans,
 			 struct coh901318_base *base)
 {
 	int chans_i;
-	int i = 0;
+	int i = 0, id = 0;
 	struct coh901318_chan *cohc;
 
 	INIT_LIST_HEAD(&dma->channels);
@@ -1442,6 +1442,7 @@ void coh901318_base_init(struct dma_device *dma, const int *pick_chans,
 			tasklet_init(&cohc->tasklet, dma_tasklet,
 				     (unsigned long) cohc);
 
+			cohc->chan.chan_id = id++;
 			list_add_tail(&cohc->chan.device_node,
 				      &dma->channels);
 		}
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 8bcb15f..d166088 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -735,7 +735,6 @@ int dma_async_device_register(struct dma_device *device)
 			goto err_out;
 		}
 
-		chan->chan_id = chancnt++;
 		chan->dev->device.class = &dma_devclass;
 		chan->dev->device.parent = device->dev;
 		chan->dev->chan = chan;
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 8a78154..477a821 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -1309,7 +1309,7 @@ static int __devinit fsl_dma_chan_probe(struct fsldma_device *fdev,
 
 	/* Add the channel to DMA device channel list */
 	list_add_tail(&chan->common.device_node, &fdev->common.channels);
-	fdev->common.chancnt++;
+	chan->common.chan_id = fdev->common.chancnt++;
 
 	dev_info(fdev->dev, "#%d (%s), irq %d\n", chan->id, compatible,
 		 chan->irq != NO_IRQ ? chan->irq : fdev->irq);
diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c
index e18eaab..bbf8619 100644
--- a/drivers/dma/imx-dma.c
+++ b/drivers/dma/imx-dma.c
@@ -368,6 +368,7 @@ static int __init imxdma_probe(struct platform_device *pdev)
 		imxdmac->chan.device = &imxdma->dma_device;
 		imxdmac->channel = i;
 
+		imxdmac->chan.chan_id = i;
 		/* Add the channel to the DMAC list */
 		list_add_tail(&imxdmac->chan.device_node, &imxdma->dma_device.channels);
 	}
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index b6d1455..be1d481 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -1305,9 +1305,11 @@ static int __init sdma_probe(struct platform_device *pdev)
 		 * because we need it internally in the SDMA driver. This also means
 		 * that channel 0 in dmaengine counting matches sdma channel 1.
 		 */
-		if (i)
+		if (i) {
+			sdmac->chan.chan_id = i - 1;
 			list_add_tail(&sdmac->chan.device_node,
 					&sdma->dma_device.channels);
+		}
 	}
 
 	ret = sdma_init(sdma);
diff --git a/drivers/dma/ioat/dma.c b/drivers/dma/ioat/dma.c
index a4d6cb0..7c1dbe5 100644
--- a/drivers/dma/ioat/dma.c
+++ b/drivers/dma/ioat/dma.c
@@ -107,6 +107,7 @@ void ioat_init_channel(struct ioatdma_device *device, struct ioat_chan_common *c
 	chan->reg_base = device->reg_base + (0x80 * (idx + 1));
 	spin_lock_init(&chan->cleanup_lock);
 	chan->common.device = dma;
+	chan->common.chan_id = idx;
 	list_add_tail(&chan->common.device_node, &dma->channels);
 	device->idx[idx] = chan;
 	init_timer(&chan->timer);
diff --git a/drivers/dma/iop-adma.c b/drivers/dma/iop-adma.c
index e03f811..8778e63 100644
--- a/drivers/dma/iop-adma.c
+++ b/drivers/dma/iop-adma.c
@@ -1565,6 +1565,7 @@ static int __devinit iop_adma_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&iop_chan->chain);
 	INIT_LIST_HEAD(&iop_chan->all_slots);
 	iop_chan->common.device = dma_dev;
+	iop_chan->common.chan_id = 0;
 	list_add_tail(&iop_chan->common.device_node, &dma_dev->channels);
 
 	if (dma_has_cap(DMA_MEMCPY, dma_dev->cap_mask)) {
diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 954e334..722c668 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -1215,6 +1215,7 @@ static int __devinit mv_xor_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&mv_chan->all_slots);
 	mv_chan->common.device = dma_dev;
 
+	mv_chan->common.chan_id = 0;
 	list_add_tail(&mv_chan->common.device_node, &dma_dev->channels);
 
 	if (dma_has_cap(DMA_MEMCPY, dma_dev->cap_mask)) {
diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index 88aad4f..f79599e 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -655,7 +655,7 @@ static int __init mxs_dma_probe(struct platform_device *pdev)
 		tasklet_init(&mxs_chan->tasklet, mxs_dma_tasklet,
 			     (unsigned long) mxs_chan);
 
-
+		mxs_chan->chan.chan_id = i;
 		/* Add the channel to mxs_chan list */
 		list_add_tail(&mxs_chan->chan.device_node,
 			&mxs_dma->dma_device.channels);
diff --git a/drivers/dma/ppc4xx/adma.c b/drivers/dma/ppc4xx/adma.c
index fc457a7..bacd896 100644
--- a/drivers/dma/ppc4xx/adma.c
+++ b/drivers/dma/ppc4xx/adma.c
@@ -4529,6 +4529,7 @@ static int __devinit ppc440spe_adma_probe(struct platform_device *ofdev)
 	INIT_LIST_HEAD(&chan->all_slots);
 	chan->device = adev;
 	chan->common.device = &adev->common;
+	chan->common.chan_id = 0;
 	list_add_tail(&chan->common.device_node, &adev->common.channels);
 	tasklet_init(&chan->irq_tasklet, ppc440spe_adma_tasklet,
 		     (unsigned long)chan);
diff --git a/drivers/dma/shdma.c b/drivers/dma/shdma.c
index 0283300..469cdb8 100644
--- a/drivers/dma/shdma.c
+++ b/drivers/dma/shdma.c
@@ -1028,6 +1028,7 @@ static int __devinit sh_dmae_chan_probe(struct sh_dmae_device *shdev, int id,
 	INIT_LIST_HEAD(&new_sh_chan->ld_queue);
 	INIT_LIST_HEAD(&new_sh_chan->ld_free);
 
+	new_sh_chan->common.chan_id = id;
 	/* Add the channel to DMA device channel list */
 	list_add_tail(&new_sh_chan->common.device_node,
 			&shdev->common.channels);
diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
index 8f222d4..a30d37b 100644
--- a/drivers/dma/ste_dma40.c
+++ b/drivers/dma/ste_dma40.c
@@ -2345,6 +2345,7 @@ static void __init d40_chan_init(struct d40_base *base, struct dma_device *dma,
 		tasklet_init(&d40c->tasklet, dma_tasklet,
 			     (unsigned long) d40c);
 
+		d40c->chan.chan_id = i - offset;
 		list_add_tail(&d40c->chan.device_node,
 			      &dma->channels);
 	}
diff --git a/drivers/dma/txx9dmac.c b/drivers/dma/txx9dmac.c
index cbd83e36..271e8d3 100644
--- a/drivers/dma/txx9dmac.c
+++ b/drivers/dma/txx9dmac.c
@@ -1186,6 +1186,7 @@ static int __init txx9dmac_chan_probe(struct platform_device *pdev)
 	dc->ddev->chan[ch] = dc;
 	dc->chan.device = &dc->dma;
 	list_add_tail(&dc->chan.device_node, &dc->chan.device->channels);
+	dc->chan.chan_id = 0;
 	dc->chan.cookie = dc->completed = 1;
 
 	if (is_dmac64(dc))
-- 
1.7.4.1

[PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Jassi Brar]

A DMAC driver might put chan_id to better use by directly
mapping system-wide channel numbers on them (and not DMAC
relative index).

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 v2  Minor fix of restoring chancnt++ in dmaengine.c

 drivers/dma/amba-pl08x.c  |    1 +
 drivers/dma/coh901318.c   |    3 ++-
 drivers/dma/dmaengine.c   |    2 +-
 drivers/dma/fsldma.c      |    2 +-
 drivers/dma/imx-dma.c     |    1 +
 drivers/dma/imx-sdma.c    |    4 +++-
 drivers/dma/ioat/dma.c    |    1 +
 drivers/dma/iop-adma.c    |    1 +
 drivers/dma/mv_xor.c      |    1 +
 drivers/dma/mxs-dma.c     |    2 +-
 drivers/dma/ppc4xx/adma.c |    1 +
 drivers/dma/shdma.c       |    1 +
 drivers/dma/ste_dma40.c   |    1 +
 drivers/dma/txx9dmac.c    |    1 +
 14 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index e6d7228..40083ef 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1738,6 +1738,7 @@ static int pl08x_dma_init_virtual_channels(struct pl08x_driver_data *pl08x,
 		tasklet_init(&chan->tasklet, pl08x_tasklet,
 			     (unsigned long) chan);
 
+		chan->chan.chan_id = i;
 		list_add_tail(&chan->chan.device_node, &dmadev->channels);
 	}
 	dev_info(&pl08x->adev->dev, "initialized %d virtual %s channels\n",
diff --git a/drivers/dma/coh901318.c b/drivers/dma/coh901318.c
index af8c0b5..3d8b67a 100644
--- a/drivers/dma/coh901318.c
+++ b/drivers/dma/coh901318.c
@@ -1414,7 +1414,7 @@ void coh901318_base_init(struct dma_device *dma, const int *pick_chans,
 			 struct coh901318_base *base)
 {
 	int chans_i;
-	int i = 0;
+	int i = 0, id = 0;
 	struct coh901318_chan *cohc;
 
 	INIT_LIST_HEAD(&dma->channels);
@@ -1442,6 +1442,7 @@ void coh901318_base_init(struct dma_device *dma, const int *pick_chans,
 			tasklet_init(&cohc->tasklet, dma_tasklet,
 				     (unsigned long) cohc);
 
+			cohc->chan.chan_id = id++;
 			list_add_tail(&cohc->chan.device_node,
 				      &dma->channels);
 		}
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 8bcb15f..af52e26 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -735,7 +735,7 @@ int dma_async_device_register(struct dma_device *device)
 			goto err_out;
 		}
 
-		chan->chan_id = chancnt++;
+		chancnt++;
 		chan->dev->device.class = &dma_devclass;
 		chan->dev->device.parent = device->dev;
 		chan->dev->chan = chan;
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 8a78154..477a821 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -1309,7 +1309,7 @@ static int __devinit fsl_dma_chan_probe(struct fsldma_device *fdev,
 
 	/* Add the channel to DMA device channel list */
 	list_add_tail(&chan->common.device_node, &fdev->common.channels);
-	fdev->common.chancnt++;
+	chan->common.chan_id = fdev->common.chancnt++;
 
 	dev_info(fdev->dev, "#%d (%s), irq %d\n", chan->id, compatible,
 		 chan->irq != NO_IRQ ? chan->irq : fdev->irq);
diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c
index e18eaab..bbf8619 100644
--- a/drivers/dma/imx-dma.c
+++ b/drivers/dma/imx-dma.c
@@ -368,6 +368,7 @@ static int __init imxdma_probe(struct platform_device *pdev)
 		imxdmac->chan.device = &imxdma->dma_device;
 		imxdmac->channel = i;
 
+		imxdmac->chan.chan_id = i;
 		/* Add the channel to the DMAC list */
 		list_add_tail(&imxdmac->chan.device_node, &imxdma->dma_device.channels);
 	}
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index b6d1455..be1d481 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -1305,9 +1305,11 @@ static int __init sdma_probe(struct platform_device *pdev)
 		 * because we need it internally in the SDMA driver. This also means
 		 * that channel 0 in dmaengine counting matches sdma channel 1.
 		 */
-		if (i)
+		if (i) {
+			sdmac->chan.chan_id = i - 1;
 			list_add_tail(&sdmac->chan.device_node,
 					&sdma->dma_device.channels);
+		}
 	}
 
 	ret = sdma_init(sdma);
diff --git a/drivers/dma/ioat/dma.c b/drivers/dma/ioat/dma.c
index a4d6cb0..7c1dbe5 100644
--- a/drivers/dma/ioat/dma.c
+++ b/drivers/dma/ioat/dma.c
@@ -107,6 +107,7 @@ void ioat_init_channel(struct ioatdma_device *device, struct ioat_chan_common *c
 	chan->reg_base = device->reg_base + (0x80 * (idx + 1));
 	spin_lock_init(&chan->cleanup_lock);
 	chan->common.device = dma;
+	chan->common.chan_id = idx;
 	list_add_tail(&chan->common.device_node, &dma->channels);
 	device->idx[idx] = chan;
 	init_timer(&chan->timer);
diff --git a/drivers/dma/iop-adma.c b/drivers/dma/iop-adma.c
index e03f811..8778e63 100644
--- a/drivers/dma/iop-adma.c
+++ b/drivers/dma/iop-adma.c
@@ -1565,6 +1565,7 @@ static int __devinit iop_adma_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&iop_chan->chain);
 	INIT_LIST_HEAD(&iop_chan->all_slots);
 	iop_chan->common.device = dma_dev;
+	iop_chan->common.chan_id = 0;
 	list_add_tail(&iop_chan->common.device_node, &dma_dev->channels);
 
 	if (dma_has_cap(DMA_MEMCPY, dma_dev->cap_mask)) {
diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 954e334..722c668 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -1215,6 +1215,7 @@ static int __devinit mv_xor_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&mv_chan->all_slots);
 	mv_chan->common.device = dma_dev;
 
+	mv_chan->common.chan_id = 0;
 	list_add_tail(&mv_chan->common.device_node, &dma_dev->channels);
 
 	if (dma_has_cap(DMA_MEMCPY, dma_dev->cap_mask)) {
diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index 88aad4f..f79599e 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -655,7 +655,7 @@ static int __init mxs_dma_probe(struct platform_device *pdev)
 		tasklet_init(&mxs_chan->tasklet, mxs_dma_tasklet,
 			     (unsigned long) mxs_chan);
 
-
+		mxs_chan->chan.chan_id = i;
 		/* Add the channel to mxs_chan list */
 		list_add_tail(&mxs_chan->chan.device_node,
 			&mxs_dma->dma_device.channels);
diff --git a/drivers/dma/ppc4xx/adma.c b/drivers/dma/ppc4xx/adma.c
index fc457a7..bacd896 100644
--- a/drivers/dma/ppc4xx/adma.c
+++ b/drivers/dma/ppc4xx/adma.c
@@ -4529,6 +4529,7 @@ static int __devinit ppc440spe_adma_probe(struct platform_device *ofdev)
 	INIT_LIST_HEAD(&chan->all_slots);
 	chan->device = adev;
 	chan->common.device = &adev->common;
+	chan->common.chan_id = 0;
 	list_add_tail(&chan->common.device_node, &adev->common.channels);
 	tasklet_init(&chan->irq_tasklet, ppc440spe_adma_tasklet,
 		     (unsigned long)chan);
diff --git a/drivers/dma/shdma.c b/drivers/dma/shdma.c
index 0283300..469cdb8 100644
--- a/drivers/dma/shdma.c
+++ b/drivers/dma/shdma.c
@@ -1028,6 +1028,7 @@ static int __devinit sh_dmae_chan_probe(struct sh_dmae_device *shdev, int id,
 	INIT_LIST_HEAD(&new_sh_chan->ld_queue);
 	INIT_LIST_HEAD(&new_sh_chan->ld_free);
 
+	new_sh_chan->common.chan_id = id;
 	/* Add the channel to DMA device channel list */
 	list_add_tail(&new_sh_chan->common.device_node,
 			&shdev->common.channels);
diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
index 8f222d4..a30d37b 100644
--- a/drivers/dma/ste_dma40.c
+++ b/drivers/dma/ste_dma40.c
@@ -2345,6 +2345,7 @@ static void __init d40_chan_init(struct d40_base *base, struct dma_device *dma,
 		tasklet_init(&d40c->tasklet, dma_tasklet,
 			     (unsigned long) d40c);
 
+		d40c->chan.chan_id = i - offset;
 		list_add_tail(&d40c->chan.device_node,
 			      &dma->channels);
 	}
diff --git a/drivers/dma/txx9dmac.c b/drivers/dma/txx9dmac.c
index cbd83e36..271e8d3 100644
--- a/drivers/dma/txx9dmac.c
+++ b/drivers/dma/txx9dmac.c
@@ -1186,6 +1186,7 @@ static int __init txx9dmac_chan_probe(struct platform_device *pdev)
 	dc->ddev->chan[ch] = dc;
 	dc->chan.device = &dc->dma;
 	list_add_tail(&dc->chan.device_node, &dc->chan.device->channels);
+	dc->chan.chan_id = 0;
 	dc->chan.cookie = dc->completed = 1;
 
 	if (is_dmac64(dc))
-- 
1.7.4.1

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Linus Walleij]

On Thu, Jul 21, 2011 at 6:01 AM, Jassi Brar <jaswinder.singh@linaro.org> wrote:

> A DMAC driver might put chan_id to better use by directly
> mapping system-wide channel numbers on them (and not DMAC
> relative index).

Sorry, not following this, is the idea that instead of each DMAC
numbering its own channels [0...N] you want to establish a global
channel index number [0..N] that is unique across all DMACs
in the system, even if you have several of them?

Yours,
Linus Walleij

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Jaswinder Singh]

On 22 July 2011 20:57, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Jul 21, 2011 at 6:01 AM, Jassi Brar <jaswinder.singh@linaro.org> wrote:
>
>> A DMAC driver might put chan_id to better use by directly
>> mapping system-wide channel numbers on them (and not DMAC
>> relative index).
>
> Sorry, not following this, is the idea that instead of each DMAC
> numbering its own channels [0...N] you want to establish a global
> channel index number [0..N] that is unique across all DMACs
> in the system, even if you have several of them?
>
Something like that.
Some DMACs 'think' they set the chan_id but it is immidiately overwritten
by dma_async_device_register()

So it's not clear whose job is to set it. Apparently the core is just
"doing it for the DMAC driver".

First of all, we need to see if we really want the dmaengine.c to
set it's value?
I think not, because there is no way for a DMAC driver to assign
actually usable channel numbers to chan_id -- there might be 'holes'
in the channel sequence of a DMAC. For ex, some channels of a DMAC
don't reach any peripheral in a particular SoC.
So the 'contiguous' sysfs entries 'dma%dchan%d' only depict
mock numbering, which I am not sure is any useful.

Also, it should be possible to map channel suitability (job of filter function)
onto unique channel ID in a platform.
So client drivers could simply do

bool filter_fn(struct dma_chan *chan, void *ch)
{
       return chan->chan_id == ch;
}

And this, imho, should be what we should strive for if we are to have
common clients over different DMACs.



-j

-- 
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  -
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Williams, Dan J]

On Fri, Jul 22, 2011 at 10:43 AM, Jaswinder Singh
<jaswinder.singh@linaro.org> wrote:
> On 22 July 2011 20:57, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Thu, Jul 21, 2011 at 6:01 AM, Jassi Brar <jaswinder.singh@linaro.org> wrote:
>>
>>> A DMAC driver might put chan_id to better use by directly
>>> mapping system-wide channel numbers on them (and not DMAC
>>> relative index).
>>
>> Sorry, not following this, is the idea that instead of each DMAC
>> numbering its own channels [0...N] you want to establish a global
>> channel index number [0..N] that is unique across all DMACs
>> in the system, even if you have several of them?
>>
> Something like that.
> Some DMACs 'think' they set the chan_id but it is immidiately overwritten
> by dma_async_device_register()
>
> So it's not clear whose job is to set it. Apparently the core is just
> "doing it for the DMAC driver".
>
> First of all, we need to see if we really want the dmaengine.c to
> set it's value?
> I think not, because there is no way for a DMAC driver to assign
> actually usable channel numbers to chan_id -- there might be 'holes'
> in the channel sequence of a DMAC. For ex, some channels of a DMAC
> don't reach any peripheral in a particular SoC.
> So the 'contiguous' sysfs entries 'dma%dchan%d' only depict
> mock numbering, which I am not sure is any useful.
>
> Also, it should be possible to map channel suitability (job of filter function)
> onto unique channel ID in a platform.
> So client drivers could simply do
>
> bool filter_fn(struct dma_chan *chan, void *ch)
> {
>       return chan->chan_id == ch;
> }
>
> And this, imho, should be what we should strive for if we are to have
> common clients over different DMACs.

The chan_id really is just a presentation detail to sysfs.  Just like
scsi host numbers they simply identify that they are distinct but the
number has no significant meaning inside the kernel to the driver
stack.

Unless you guaranteed that every id is globally unique I don't see how
they are generically usable by common clients?

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Jaswinder Singh]

On 23 July 2011 03:53, Williams, Dan J <dan.j.williams@intel.com> wrote:

>
> Unless you guaranteed that every id is globally unique I don't see how
> they are generically usable by common clients?
>
Yes. And the first step is to allow DMAC drivers to freely set chan_id value.
Platform could pass the list and mapping of supported 'global channels'
via platform_data(?) which the DMAC drivers could set in chan_id
And I am not sure of defining a new variable for that, because chan_id
is actually used only by some dmac drivers for internal purpose only -
which they could do by private variables.

Proposal to have global cross-platform enum of channel-IDs defined by
client drivers, was to be my next patch.
Though I think, this patch is valid in it's own light.

-j

-- 
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  -
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Williams, Dan J]

On Fri, Jul 22, 2011 at 8:56 PM, Jaswinder Singh
<jaswinder.singh@linaro.org> wrote:
> On 23 July 2011 03:53, Williams, Dan J <dan.j.williams@intel.com> wrote:
>
>>
>> Unless you guaranteed that every id is globally unique I don't see how
>> they are generically usable by common clients?
>>
> Yes. And the first step is to allow DMAC drivers to freely set chan_id value.
> Platform could pass the list and mapping of supported 'global channels'
> via platform_data(?) which the DMAC drivers could set in chan_id
> And I am not sure of defining a new variable for that, because chan_id
> is actually used only by some dmac drivers for internal purpose only -
> which they could do by private variables.

Yes, it does appear to have grown a lot of dubious usage in
drivers/dma/ for something that is described as just a "channel ID for
sysfs".  The intent was for drivers that need to maintain their own
hardware specific identification to define a local id scheme (like
iop-adma does in the iop3xx case).

I can sort of see why it is attractive for the goal of having clients
being able to talk to multiple DMACs since it is a field that all
channels already implement.  But, I don't think we should be mixing
sysfs presentation details with a capability that indicates a certain
compatibility level in a DMAC driver implementation.  If the goal is
to be able to use a single client with multiple drivers that, to me,
is asking for a new capability bit (dma_transaction_type) rather than
an id.  Do you have an example of the client and the DMACs that would
first take advantage of such a cross DMAC compatibility?

> Proposal to have global cross-platform enum of channel-IDs defined by
> client drivers, was to be my next patch.
> Though I think, this patch is valid in it's own light.

This is not the purpose of chan_id.

--
Dan

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Jaswinder Singh]

On 26 July 2011 00:25, Williams, Dan J <dan.j.williams@intel.com> wrote:
>>> Unless you guaranteed that every id is globally unique I don't see how
>>> they are generically usable by common clients?
>>>
>> Yes. And the first step is to allow DMAC drivers to freely set chan_id value.
>> Platform could pass the list and mapping of supported 'global channels'
>> via platform_data(?) which the DMAC drivers could set in chan_id
>> And I am not sure of defining a new variable for that, because chan_id
>> is actually used only by some dmac drivers for internal purpose only -
>> which they could do by private variables.
>
> Yes, it does appear to have grown a lot of dubious usage in
> drivers/dma/ for something that is described as just a "channel ID for
> sysfs".  The intent was for drivers that need to maintain their own
> hardware specific identification to define a local id scheme (like
> iop-adma does in the iop3xx case).

You mean chan_id is meant to be used for _internal_ purposes by DMAC drivers ?

Shouldn't there instead be just something like 'void *dmac_data' for
DMAC drivers to hang their private info on?

Sorry, I couldn't find chan_id in drivers/dma/iop-adma.c
Do I misunderstand ?

>
> I can sort of see why it is attractive for the goal of having clients
> being able to talk to multiple DMACs since it is a field that all
> channels already implement.  But, I don't think we should be mixing
> sysfs presentation details with a capability that indicates a certain
> compatibility level in a DMAC driver implementation.  If the goal is
> to be able to use a single client with multiple drivers that, to me,
> is asking for a new capability bit (dma_transaction_type) rather than
> an id.  Do you have an example of the client and the DMACs that would
> first take advantage of such a cross DMAC compatibility?
>
Apparently I fail to explain my well. let me ask you this....

Some DMAC drivers initialize chan_id before calling dma_async_device_register().
And dma_async_device_register() overwrites the chan_id _always_.

Clearly only _one_ of them should be setting chan_id. Which was meant to be?


>> Proposal to have global cross-platform enum of channel-IDs defined by
>> client drivers, was to be my next patch.
>> Though I think, this patch is valid in it's own light.
>
> This is not the purpose of chan_id.
Let us ignore it for a while.

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Williams, Dan J]

On Mon, Jul 25, 2011 at 12:17 PM, Jaswinder Singh
<jaswinder.singh@linaro.org> wrote:
> On 26 July 2011 00:25, Williams, Dan J <dan.j.williams@intel.com> wrote:
>>>> Unless you guaranteed that every id is globally unique I don't see how
>>>> they are generically usable by common clients?
>>>>
>>> Yes. And the first step is to allow DMAC drivers to freely set chan_id value.
>>> Platform could pass the list and mapping of supported 'global channels'
>>> via platform_data(?) which the DMAC drivers could set in chan_id
>>> And I am not sure of defining a new variable for that, because chan_id
>>> is actually used only by some dmac drivers for internal purpose only -
>>> which they could do by private variables.
>>
>> Yes, it does appear to have grown a lot of dubious usage in
>> drivers/dma/ for something that is described as just a "channel ID for
>> sysfs".  The intent was for drivers that need to maintain their own
>> hardware specific identification to define a local id scheme (like
>> iop-adma does in the iop3xx case).
>
> You mean chan_id is meant to be used for _internal_ purposes by DMAC drivers ?
>
> Shouldn't there instead be just something like 'void *dmac_data' for
> DMAC drivers to hang their private info on?
>
> Sorry, I couldn't find chan_id in drivers/dma/iop-adma.c
> Do I misunderstand ?

/**
 * struct iop_adma_device - internal representation of an ADMA device
 * @pdev: Platform device
 * @id: HW ADMA Device selector
 * @dma_desc_pool: base of DMA descriptor region (DMA address)
 * @dma_desc_pool_virt: base of DMA descriptor region (CPU address)
 * @common: embedded struct dma_device
 */
struct iop_adma_device {
        struct platform_device *pdev;
        int id;
        dma_addr_t dma_desc_pool;
        void *dma_desc_pool_virt;
        struct dma_device common;
};

Where that 'id' is the internal identifier and chan_id from struct
dma_chan is ignored.

>> I can sort of see why it is attractive for the goal of having clients
>> being able to talk to multiple DMACs since it is a field that all
>> channels already implement.  But, I don't think we should be mixing
>> sysfs presentation details with a capability that indicates a certain
>> compatibility level in a DMAC driver implementation.  If the goal is
>> to be able to use a single client with multiple drivers that, to me,
>> is asking for a new capability bit (dma_transaction_type) rather than
>> an id.  Do you have an example of the client and the DMACs that would
>> first take advantage of such a cross DMAC compatibility?
>>
> Apparently I fail to explain my well. let me ask you this....
>
> Some DMAC drivers initialize chan_id before calling dma_async_device_register().
> And dma_async_device_register() overwrites the chan_id _always_.
>
> Clearly only _one_ of them should be setting chan_id. Which was meant to be?

Correct, it is meant that chan_id is only a sysfs property.  Any
driver usage that is assuming chan_id is anything more than a
guaranteed unique number within a given dma_device's list of channels
is probably inferring too much.

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Jaswinder Singh]

On 26 July 2011 01:38, Williams, Dan J <dan.j.williams@intel.com> wrote:

>>> I can sort of see why it is attractive for the goal of having clients
>>> being able to talk to multiple DMACs since it is a field that all
>>> channels already implement.  But, I don't think we should be mixing
>>> sysfs presentation details with a capability that indicates a certain
>>> compatibility level in a DMAC driver implementation.  If the goal is
>>> to be able to use a single client with multiple drivers that, to me,
>>> is asking for a new capability bit (dma_transaction_type) rather than
>>> an id.  Do you have an example of the client and the DMACs that would
>>> first take advantage of such a cross DMAC compatibility?
>>>
>> Apparently I fail to explain my well. let me ask you this....
>>
>> Some DMAC drivers initialize chan_id before calling dma_async_device_register().
>> And dma_async_device_register() overwrites the chan_id _always_.
>>
>> Clearly only _one_ of them should be setting chan_id. Which was meant to be?
>
> Correct, it is meant that chan_id is only a sysfs property.  Any
> driver usage that is assuming chan_id is anything more than a
> guaranteed unique number within a given dma_device's list of channels
> is probably inferring too much.

So you mean dmac/client drivers are wrong if they make use of chan_id.
They shouldn't count upon it's value - which is set by DMA API for a completely
independent purpose, i.e, creating contiguous sysfs entries.

Since "chan_id is only a sysfs property" and the fact that it is used
only _once_
by the DMA API

In drivers/dma/dmaengine.c

      chan->chan_id = chancnt++;
      dev_set_name(&chan->dev->device, "dma%dchan%d",
                             device->dev_id, chan->chan_id);


Can't we do away with chan_id altogether ? by having

      dev_set_name(&chan->dev->device, "dma%dchan%d",
                             device->dev_id, chancnt++);

I mean why make every instance of dma_chan bigger by 4bytes ?

So why shouldn't we remove chan_id completely from the DMA API ?

Thanks,
Jassi


-- 
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  -
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Williams, Dan J]

On Tue, Jul 26, 2011 at 7:30 AM, Jaswinder Singh
<jaswinder.singh@linaro.org> wrote:
> On 26 July 2011 01:38, Williams, Dan J <dan.j.williams@intel.com> wrote:
>> Correct, it is meant that chan_id is only a sysfs property.  Any
>> driver usage that is assuming chan_id is anything more than a
>> guaranteed unique number within a given dma_device's list of channels
>> is probably inferring too much.
>
> So you mean dmac/client drivers are wrong if they make use of chan_id.
> They shouldn't count upon it's value - which is set by DMA API for a completely
> independent purpose, i.e, creating contiguous sysfs entries.

They can count on it being unique, and maybe the fact that it is in
the same order as dma_device.channels.

>
> Since "chan_id is only a sysfs property" and the fact that it is used
> only _once_
> by the DMA API
>
> In drivers/dma/dmaengine.c
>
>      chan->chan_id = chancnt++;
>      dev_set_name(&chan->dev->device, "dma%dchan%d",
>                             device->dev_id, chan->chan_id);
>
>
> Can't we do away with chan_id altogether ? by having
>
>      dev_set_name(&chan->dev->device, "dma%dchan%d",
>                             device->dev_id, chancnt++);
>
> I mean why make every instance of dma_chan bigger by 4bytes ?
>
> So why shouldn't we remove chan_id completely from the DMA API ?

Good point...  Let's remove chan_id from the core and push it into the
drivers that need it.

--
Dan

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Jaswinder Singh]

On 26 July 2011 20:59, Williams, Dan J <dan.j.williams@intel.com> wrote:
> On Tue, Jul 26, 2011 at 7:30 AM, Jaswinder Singh
> <jaswinder.singh@linaro.org> wrote:
>> On 26 July 2011 01:38, Williams, Dan J <dan.j.williams@intel.com> wrote:
>>> Correct, it is meant that chan_id is only a sysfs property.  Any
>>> driver usage that is assuming chan_id is anything more than a
>>> guaranteed unique number within a given dma_device's list of channels
>>> is probably inferring too much.
>>
>> So you mean dmac/client drivers are wrong if they make use of chan_id.
>> They shouldn't count upon it's value - which is set by DMA API for a completely
>> independent purpose, i.e, creating contiguous sysfs entries.
>
> They can count on it being unique, and maybe the fact that it is in
> the same order as dma_device.channels.
The latter implies the former. And it is already the dmac driver that
decides the
rank of a channel in the list.

>
>>
>> Since "chan_id is only a sysfs property" and the fact that it is used
>> only _once_
>> by the DMA API
>>
>> In drivers/dma/dmaengine.c
>>
>>      chan->chan_id = chancnt++;
>>      dev_set_name(&chan->dev->device, "dma%dchan%d",
>>                             device->dev_id, chan->chan_id);
>>
>>
>> Can't we do away with chan_id altogether ? by having
>>
>>      dev_set_name(&chan->dev->device, "dma%dchan%d",
>>                             device->dev_id, chancnt++);
>>
>> I mean why make every instance of dma_chan bigger by 4bytes ?
>>
>> So why shouldn't we remove chan_id completely from the DMA API ?
>
> Good point...  Let's remove chan_id from the core and push it into the
> drivers that need it.
>
If you agree, I would preserve the chan_id in 'struct dma_chan' but remove
any assignment to it in dmaengine.c  and let the dmac drivers use it freely.
That would:-
a) Let dmac drivers decide what numbers they want to show up in sysfs.
b) chan_id is easily reachable by client drivers, so it is better this way.
c) It would mean lesser and simpler changes to extant users of it.

Thanks,
-Jassi

-- 
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  -
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Koul, Vinod]

On Tue, 2011-07-26 at 23:42 +0530, Jaswinder Singh wrote:
> On 26 July 2011 20:59, Williams, Dan J <dan.j.williams@intel.com> wrote:
> > On Tue, Jul 26, 2011 at 7:30 AM, Jaswinder Singh
> > <jaswinder.singh@linaro.org> wrote:
> >> On 26 July 2011 01:38, Williams, Dan J <dan.j.williams@intel.com> wrote:
> >>> Correct, it is meant that chan_id is only a sysfs property.  Any
> >>> driver usage that is assuming chan_id is anything more than a
> >>> guaranteed unique number within a given dma_device's list of channels
> >>> is probably inferring too much.
> >>
> >> So you mean dmac/client drivers are wrong if they make use of chan_id.
> >> They shouldn't count upon it's value - which is set by DMA API for a completely
> >> independent purpose, i.e, creating contiguous sysfs entries.
> >
> > They can count on it being unique, and maybe the fact that it is in
> > the same order as dma_device.channels.
> The latter implies the former. And it is already the dmac driver that
> decides the
> rank of a channel in the list.
> 
> >
> >>
> >> Since "chan_id is only a sysfs property" and the fact that it is used
> >> only _once_
> >> by the DMA API
> >>
> >> In drivers/dma/dmaengine.c
> >>
> >>      chan->chan_id = chancnt++;
> >>      dev_set_name(&chan->dev->device, "dma%dchan%d",
> >>                             device->dev_id, chan->chan_id);
> >>
> >>
> >> Can't we do away with chan_id altogether ? by having
> >>
> >>      dev_set_name(&chan->dev->device, "dma%dchan%d",
> >>                             device->dev_id, chancnt++);
> >>
> >> I mean why make every instance of dma_chan bigger by 4bytes ?
> >>
> >> So why shouldn't we remove chan_id completely from the DMA API ?
> >
> > Good point...  Let's remove chan_id from the core and push it into the
> > drivers that need it.
> >
> If you agree, I would preserve the chan_id in 'struct dma_chan' but remove
> any assignment to it in dmaengine.c  and let the dmac drivers use it freely.
> That would:-
> a) Let dmac drivers decide what numbers they want to show up in sysfs.
> b) chan_id is easily reachable by client drivers, so it is better this way.
> c) It would mean lesser and simpler changes to extant users of it.
But this can cause conflict between two controllers who think they are
assigning unique numbers. IMO sysfs representation needs to be with
dmaengine only. How do we guarantee uniqueness b/w two controllers?

Also I am not sure about the approach: The whole point is to make filter
function select based on some channel number "x", but you should filter
channels based on controller you want and capability of a channel. If
caps is not enough to filter we should add more flags but asking that I
need channel 'y' doesn't sound right to me.
This is all coming from that fact that some drivers assume channel 'a'
is for this type of transfer and channel 'b' for something else, for a
dma controller that really doesn't matter, as all channels have similar
capabilities and can do similar things so you should
_get_channel_based_on_caps rather than on some numbering.

Lastly, why do you need a channel reserved for some type of transfer, is
it for assigning h/w interface for dma transfer, if so that can be
achieved in different ways as well


-- 
~Vinod

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Jaswinder Singh]

On 27 July 2011 09:51, Koul, Vinod <vinod.koul@intel.com> wrote:
> On Tue, 2011-07-26 at 23:42 +0530, Jaswinder Singh wrote:
>> On 26 July 2011 20:59, Williams, Dan J <dan.j.williams@intel.com> wrote:
>> > On Tue, Jul 26, 2011 at 7:30 AM, Jaswinder Singh
>> > <jaswinder.singh@linaro.org> wrote:
>> >> On 26 July 2011 01:38, Williams, Dan J <dan.j.williams@intel.com> wrote:
>> >>> Correct, it is meant that chan_id is only a sysfs property.  Any
>> >>> driver usage that is assuming chan_id is anything more than a
>> >>> guaranteed unique number within a given dma_device's list of channels
>> >>> is probably inferring too much.
>> >>
>> >> So you mean dmac/client drivers are wrong if they make use of chan_id.
>> >> They shouldn't count upon it's value - which is set by DMA API for a completely
>> >> independent purpose, i.e, creating contiguous sysfs entries.
>> >
>> > They can count on it being unique, and maybe the fact that it is in
>> > the same order as dma_device.channels.
>> The latter implies the former. And it is already the dmac driver that
>> decides the
>> rank of a channel in the list.
>>
>> >
>> >>
>> >> Since "chan_id is only a sysfs property" and the fact that it is used
>> >> only _once_
>> >> by the DMA API
>> >>
>> >> In drivers/dma/dmaengine.c
>> >>
>> >>      chan->chan_id = chancnt++;
>> >>      dev_set_name(&chan->dev->device, "dma%dchan%d",
>> >>                             device->dev_id, chan->chan_id);
>> >>
>> >>
>> >> Can't we do away with chan_id altogether ? by having
>> >>
>> >>      dev_set_name(&chan->dev->device, "dma%dchan%d",
>> >>                             device->dev_id, chancnt++);
>> >>
>> >> I mean why make every instance of dma_chan bigger by 4bytes ?
>> >>
>> >> So why shouldn't we remove chan_id completely from the DMA API ?
>> >
>> > Good point...  Let's remove chan_id from the core and push it into the
>> > drivers that need it.
>> >
>> If you agree, I would preserve the chan_id in 'struct dma_chan' but remove
>> any assignment to it in dmaengine.c  and let the dmac drivers use it freely.
>> That would:-
>> a) Let dmac drivers decide what numbers they want to show up in sysfs.
>> b) chan_id is easily reachable by client drivers, so it is better this way.
>> c) It would mean lesser and simpler changes to extant users of it.
> But this can cause conflict between two controllers who think they are
> assigning unique numbers.
Could you please clarify, which two controllers ?

> IMO sysfs representation needs to be with
> dmaengine only. How do we guarantee uniqueness b/w two controllers?
Again, how is chan_id currently unique between two controllers ?

Btw, do you not want to keep chan_id in dma_chan or do you not want to change
anything at all ?

>
> Also I am not sure about the approach: The whole point is to make filter
> function select based on some channel number "x", but you should filter
> channels based on controller you want and capability of a channel. If
> caps is not enough to filter we should add more flags but asking that I
> need channel 'y' doesn't sound right to me.
> This is all coming from that fact that some drivers assume channel 'a'
> is for this type of transfer and channel 'b' for something else, for a
> dma controller that really doesn't matter, as all channels have similar
> capabilities and can do similar things so you should
> _get_channel_based_on_caps rather than on some numbering.
>
> Lastly, why do you need a channel reserved for some type of transfer, is
> it for assigning h/w interface for dma transfer, if so that can be
> achieved in different ways as well

Please look at this patch from POV of utility of chan_id, which isn't much
currently as explainined.

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Koul, Vinod]

On Wed, 2011-07-27 at 12:47 +0530, Jaswinder Singh wrote:
> On 27 July 2011 09:51, Koul, Vinod <vinod.koul@intel.com> wrote:
> > On Tue, 2011-07-26 at 23:42 +0530, Jaswinder Singh wrote:
> >> On 26 July 2011 20:59, Williams, Dan J <dan.j.williams@intel.com> wrote:
> >> > On Tue, Jul 26, 2011 at 7:30 AM, Jaswinder Singh
> >> > <jaswinder.singh@linaro.org> wrote:
> >> >> On 26 July 2011 01:38, Williams, Dan J <dan.j.williams@intel.com> wrote:
> >> >>> Correct, it is meant that chan_id is only a sysfs property.  Any
> >> >>> driver usage that is assuming chan_id is anything more than a
> >> >>> guaranteed unique number within a given dma_device's list of channels
> >> >>> is probably inferring too much.
> >> >>
> >> >> So you mean dmac/client drivers are wrong if they make use of chan_id.
> >> >> They shouldn't count upon it's value - which is set by DMA API for a completely
> >> >> independent purpose, i.e, creating contiguous sysfs entries.
> >> >
> >> > They can count on it being unique, and maybe the fact that it is in
> >> > the same order as dma_device.channels.
> >> The latter implies the former. And it is already the dmac driver that
> >> decides the
> >> rank of a channel in the list.
> >>
> >> >
> >> >>
> >> >> Since "chan_id is only a sysfs property" and the fact that it is used
> >> >> only _once_
> >> >> by the DMA API
> >> >>
> >> >> In drivers/dma/dmaengine.c
> >> >>
> >> >>      chan->chan_id = chancnt++;
> >> >>      dev_set_name(&chan->dev->device, "dma%dchan%d",
> >> >>                             device->dev_id, chan->chan_id);
> >> >>
> >> >>
> >> >> Can't we do away with chan_id altogether ? by having
> >> >>
> >> >>      dev_set_name(&chan->dev->device, "dma%dchan%d",
> >> >>                             device->dev_id, chancnt++);
> >> >>
> >> >> I mean why make every instance of dma_chan bigger by 4bytes ?
> >> >>
> >> >> So why shouldn't we remove chan_id completely from the DMA API ?
> >> >
> >> > Good point...  Let's remove chan_id from the core and push it into the
> >> > drivers that need it.
> >> >
> >> If you agree, I would preserve the chan_id in 'struct dma_chan' but remove
> >> any assignment to it in dmaengine.c  and let the dmac drivers use it freely.
> >> That would:-
> >> a) Let dmac drivers decide what numbers they want to show up in sysfs.
> >> b) chan_id is easily reachable by client drivers, so it is better this way.
> >> c) It would mean lesser and simpler changes to extant users of it.
> > But this can cause conflict between two controllers who think they are
> > assigning unique numbers.
> Could you please clarify, which two controllers ?
You can have two different DMACs in same system. At least I have two
from current intel_mid_dma which are used. Both give their channel id
starting from 0, 1....
Further as we integrate video, audio, spi, emmc dmacs possibility of
having multiple dmacs will increase in a system
> 
> > IMO sysfs representation needs to be with
> > dmaengine only. How do we guarantee uniqueness b/w two controllers?
> Again, how is chan_id currently unique between two controllers ?
> 
> Btw, do you not want to keep chan_id in dma_chan or do you not want to change
> anything at all ?
> 
> >
> > Also I am not sure about the approach: The whole point is to make filter
> > function select based on some channel number "x", but you should filter
> > channels based on controller you want and capability of a channel. If
> > caps is not enough to filter we should add more flags but asking that I
> > need channel 'y' doesn't sound right to me.
> > This is all coming from that fact that some drivers assume channel 'a'
> > is for this type of transfer and channel 'b' for something else, for a
> > dma controller that really doesn't matter, as all channels have similar
> > capabilities and can do similar things so you should
> > _get_channel_based_on_caps rather than on some numbering.
> >
> > Lastly, why do you need a channel reserved for some type of transfer, is
> > it for assigning h/w interface for dma transfer, if so that can be
> > achieved in different ways as well
> 
> Please look at this patch from POV of utility of chan_id, which isn't much
> currently as explainined.
Sorry I didn't get you. 
As I understand you are trying to simplify the filter function by
assigning unique ids to all channels, but whole point itself is not
quite right, as I said selection should be based on capability and not
channel number "x". 
Can you please help me understand why channel number "x" is important
and strictly needed by some client?

-- 
~Vinod

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Koul, Vinod]

On Wed, 2011-07-27 at 12:59 +0300, Mika Westerberg wrote:
> On Wed, Jul 27, 2011 at 02:32:26PM +0530, Koul, Vinod wrote:
> > Can you please help me understand why channel number "x" is important
> > and strictly needed by some client?
> 
> At least in ep93xx we have even channel numbers for TX and odd for RX (or
> vice versa, can't rememeber). This is a hardware restriction. So the client
> code then filters the channels based on that using ->chan_id.
> 
> As the caps are not per channel, that is the only way I was able to make it
> work.
Thanks, 
Is this a dma controller limitation or due to the fact even channels get
assigned h/w interface corresponding to TX and so on.

At least on dw synopsis controllers all channels are same and you need
to assign the respective h/w interface of the port (rx and tx) you are
transferring to. If we pass and configure this in driver then client
would just need channel with capabilities and not specific number.

I am not sure if this is true for other type of controllers...

-- 
~Vinod

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Mika Westerberg]

On Wed, Jul 27, 2011 at 02:32:26PM +0530, Koul, Vinod wrote:
> Can you please help me understand why channel number "x" is important
> and strictly needed by some client?

At least in ep93xx we have even channel numbers for TX and odd for RX (or
vice versa, can't rememeber). This is a hardware restriction. So the client
code then filters the channels based on that using ->chan_id.

As the caps are not per channel, that is the only way I was able to make it
work.

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Mika Westerberg]

On Wed, Jul 27, 2011 at 03:04:24PM +0530, Koul, Vinod wrote:

> Is this a dma controller limitation or due to the fact even channels get
> assigned h/w interface corresponding to TX and so on.

I think the later but not sure how it is implemented in hw. The
documentation only states that even channels are for TX and odd for RX.
There is no way of changing the direction as far I can tell.

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Jaswinder Singh]

On 27 July 2011 14:32, Koul, Vinod <vinod.koul@intel.com> wrote:
>> >> >>> Correct, it is meant that chan_id is only a sysfs property.  Any
>> >> >>> driver usage that is assuming chan_id is anything more than a
>> >> >>> guaranteed unique number within a given dma_device's list of channels
>> >> >>> is probably inferring too much.
>> >> >>
>> >> >> So you mean dmac/client drivers are wrong if they make use of chan_id.
>> >> >> They shouldn't count upon it's value - which is set by DMA API for a completely
>> >> >> independent purpose, i.e, creating contiguous sysfs entries.
>> >> >
>> >> > They can count on it being unique, and maybe the fact that it is in
>> >> > the same order as dma_device.channels.
>> >> The latter implies the former. And it is already the dmac driver that
>> >> decides the
>> >> rank of a channel in the list.
>> >>
>> >> >
>> >> >>
>> >> >> Since "chan_id is only a sysfs property" and the fact that it is used
>> >> >> only _once_
>> >> >> by the DMA API
>> >> >>
>> >> >> In drivers/dma/dmaengine.c
>> >> >>
>> >> >>      chan->chan_id = chancnt++;
>> >> >>      dev_set_name(&chan->dev->device, "dma%dchan%d",
>> >> >>                             device->dev_id, chan->chan_id);
>> >> >>
>> >> >>
>> >> >> Can't we do away with chan_id altogether ? by having
>> >> >>
>> >> >>      dev_set_name(&chan->dev->device, "dma%dchan%d",
>> >> >>                             device->dev_id, chancnt++);
>> >> >>
>> >> >> I mean why make every instance of dma_chan bigger by 4bytes ?
>> >> >>
>> >> >> So why shouldn't we remove chan_id completely from the DMA API ?
>> >> >
>> >> > Good point...  Let's remove chan_id from the core and push it into the
>> >> > drivers that need it.
>> >> >
>> >> If you agree, I would preserve the chan_id in 'struct dma_chan' but remove
>> >> any assignment to it in dmaengine.c  and let the dmac drivers use it freely.
>> >> That would:-
>> >> a) Let dmac drivers decide what numbers they want to show up in sysfs.
>> >> b) chan_id is easily reachable by client drivers, so it is better this way.
>> >> c) It would mean lesser and simpler changes to extant users of it.
>> > But this can cause conflict between two controllers who think they are
>> > assigning unique numbers.
>> Could you please clarify, which two controllers ?
> You can have two different DMACs in same system. At least I have two
> from current intel_mid_dma which are used. Both give their channel id
> starting from 0, 1....
> Further as we integrate video, audio, spi, emmc dmacs possibility of
> having multiple dmacs will increase in a system
Most of Samsung's S5P series have 3 DMACs - 2 for peripherals and 1 for mem->mem
But that is not the point.

This patch in no way affects what values currently a dmac driver
assigns to chan_id

>>
>> > IMO sysfs representation needs to be with
>> > dmaengine only. How do we guarantee uniqueness b/w two controllers?
>> Again, how is chan_id currently unique between two controllers ?
>>
>> Btw, do you not want to keep chan_id in dma_chan or do you not want to change
>> anything at all ?
>>
>> >
>> > Also I am not sure about the approach: The whole point is to make filter
>> > function select based on some channel number "x", but you should filter
>> > channels based on controller you want and capability of a channel. If
>> > caps is not enough to filter we should add more flags but asking that I
>> > need channel 'y' doesn't sound right to me.
>> > This is all coming from that fact that some drivers assume channel 'a'
>> > is for this type of transfer and channel 'b' for something else, for a
>> > dma controller that really doesn't matter, as all channels have similar
>> > capabilities and can do similar things so you should
>> > _get_channel_based_on_caps rather than on some numbering.
>> >
>> > Lastly, why do you need a channel reserved for some type of transfer, is
>> > it for assigning h/w interface for dma transfer, if so that can be
>> > achieved in different ways as well
>>
>> Please look at this patch from POV of utility of chan_id, which isn't much
>> currently as explainined.
> Sorry I didn't get you.
> As I understand you are trying to simplify the filter function by
> assigning unique ids to all channels,
No dear. Let me put it precisely.

Even if we make no further change to the dmaengine, this patch is the right
thing to do today.

Please have a look at the patch and figure if it breaks anything or is not the
right change. Plz ignore what you _think_ I plan to do next.

> but whole point itself is not
> quite right, as I said selection should be based on capability and not
> channel number "x".
* I absolutely agree *


> Can you please help me understand why channel number "x" is important
> and strictly needed by some client?
I never said that. I don't think so.

I request you to please have a look at

1) What I propose
        http://lists.infradead.org/pipermail/linux-arm-kernel/2011-July/059212.html

2) Why RMK thinks I am the biggest idiot on earth
        http://lists.infradead.org/pipermail/linux-arm-kernel/2011-July/059217.html

3) How I ask for better proof of that
       http://lists.infradead.org/pipermail/linux-arm-kernel/2011-July/059223.html

On a serious note, my proposal, and the reply, shows the possibility
of having :-
a) Client drivers that are truly platform agnostic -- no platform_data
poking for
      channel selection
b) A standard way to express capabilities of a channel -- chan_id is just one,
     we can employ different method if need be.
c) DMAENGINE API smart enough to provide best channel selection for a request.

Finally, let us please take this discussion there, because this patch should be
judged only under it's own light.

Thanks,
Jassi


-- 
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  -
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Jaswinder Singh]

On 27 July 2011 15:29, Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> As the caps are not per channel, that is the only way I was able to make it
> work.
FWIW, my proposal(in other thread) tags capabilities to a channel.

> At least in ep93xx we have even channel numbers for TX and odd for RX (or
> vice versa, can't rememeber). This is a hardware restriction. So the client
> code then filters the channels based on that using ->chan_id.
This should be a piece of cake to handle by my proposal.

If interested please get involved in that thread and help check if it
can make things better.
Thanks.

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Williams, Dan J]

On Wed, Jul 27, 2011 at 7:50 AM, Jaswinder Singh
<jaswinder.singh@linaro.org> wrote:
> On 27 July 2011 15:29, Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
>
>> As the caps are not per channel, that is the only way I was able to make it
>> work.
> FWIW, my proposal(in other thread) tags capabilities to a channel.
>

Where is that other thread?

>> At least in ep93xx we have even channel numbers for TX and odd for RX (or
>> vice versa, can't rememeber). This is a hardware restriction. So the client
>> code then filters the channels based on that using ->chan_id.
> This should be a piece of cake to handle by my proposal.
>
> If interested please get involved in that thread and help check if it
> can make things better.

I think it is fine for chan_id to be disconnected from sysfs and given
to drivers as common field for some other purpose (not convinced that
1:N client-to-driver is a valid use for this single id, hence why I'd
like to see the full proposal).

But it definitely should not be a field that drivers control in
non-uniform ways and gets exposed to sysfs.  The chan_id for the class
device is just an opaque integer just like the host number for scsi
hba's.  If the user wants to know device specific details about that
channel that information can be retrieved by following the device
link.

For example:
# cat /sys/class/dma/dma0chan0/device/device
0x3c20

That is the actual hardware specific channel identification.  The
class device is not the hardware device, and having drivers set
chan_id to something hardware device specific conflates that fact.
Now, if you want a new/separate attribute of a class device that is
set in some uniform fashion that is a different discussion, but it's
not clear to me why sysfs would need to expose these details?

--
Dan

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Jaswinder Singh]

On 27 July 2011 22:06, Williams, Dan J <dan.j.williams@intel.com> wrote:
> On Wed, Jul 27, 2011 at 7:50 AM, Jaswinder Singh
> <jaswinder.singh@linaro.org> wrote:
>> On 27 July 2011 15:29, Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
>>
>>> As the caps are not per channel, that is the only way I was able to make it
>>> work.
>> FWIW, my proposal(in other thread) tags capabilities to a channel.
>>
>
> Where is that other thread?
>

<copied from my reply to vinod >

I request you to please have a look at

1) What I propose
       http://lists.infradead.org/pipermail/linux-arm-kernel/2011-July/059212.html

2) Why RMK thinks I am the biggest idiot on earth
       http://lists.infradead.org/pipermail/linux-arm-kernel/2011-July/059217.html

3) How I ask for better proof of that
      http://lists.infradead.org/pipermail/linux-arm-kernel/2011-July/059223.html


Please give more serious look to what I called 'overall approach' in
the first post.
Implementation is secondary.

Thanks,
Jassi

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Russell King]

On Wed, Jul 27, 2011 at 10:44:53PM +0530, Jaswinder Singh wrote:
> 1) What I propose
>        http://lists.infradead.org/pipermail/linux-arm-kernel/2011-July/059212.html
> 
> 2) Why RMK thinks I am the biggest idiot on earth
>        http://lists.infradead.org/pipermail/linux-arm-kernel/2011-July/059217.html
> 
> 3) How I ask for better proof of that
>       http://lists.infradead.org/pipermail/linux-arm-kernel/2011-July/059223.html

Look, your idea is completely mad and insane - you just can't represent
the matching stuff as capabilities.

How do you deal with a peripheral being linked to a _specific_ DMA
engine on a _specific_ DMA request signal?  What if your system has
two DMA engines, each with 32 request signals?  Are you going to have
something like a 128-bit capability mask?

Peripheral drivers don't know what DMA signal the SoC designer may have
chosen.  Peripheral drivers don't know what DMA engine they're connected
to.  Yet again, I say, the only place which knows that is data associated
with the _platform_.  The platform has to be involved with binding the
DMA engine plus DMA channel with the peripheral.  You can't get away from
that.  Not with capabilities.  Not with stuff from the peripheral driver
saying "I want a M2P channel" in a capability field, etc

So I think your idea is totally unworkable, and it doesn't come close to
fitting with any DMA setup I've seen.

If that means you think I'm calling you an idiot, then so be it.  I just
think you're wrong on a purely technical level.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Russell King]

On Wed, Jul 27, 2011 at 08:00:23PM +0530, Jaswinder Singh wrote:
> On 27 July 2011 14:32, Koul, Vinod <vinod.koul@intel.com> wrote:
> > You can have two different DMACs in same system. At least I have two
> > from current intel_mid_dma which are used. Both give their channel id
> > starting from 0, 1....
> > Further as we integrate video, audio, spi, emmc dmacs possibility of
> > having multiple dmacs will increase in a system
>
> Most of Samsung's S5P series have 3 DMACs - 2 for peripherals and 1 for
> mem->mem But that is not the point.
> 
> This patch in no way affects what values currently a dmac driver
> assigns to chan_id

Then *explain* how the chan_id is used to match the channel which the
peripheral requires when you have three DMA controllers, each with
channels numbered 0 to 7.

> > Sorry I didn't get you.
> > As I understand you are trying to simplify the filter function by
> > assigning unique ids to all channels,
>
> No dear. Let me put it precisely.
> 
> Even if we make no further change to the dmaengine, this patch is the right
> thing to do today.

You sound like a politician.  "the right thing to do" is a cop-out.  That
says "believe me, I know I'm right, but I can't say why I'm right, I just
am."  Basically, it means that the person saying it has no clue on the
subject they're talking about.

If you do have a clue, then don't say that infuriating phrase, but give an
actual reason.

> On a serious note, my proposal, and the reply, shows the possibility
> of having :-
> a) Client drivers that are truly platform agnostic -- no platform_data
> poking for
>       channel selection

I really doubt that's even possible.  Take this setup:

MMCI ---> DMAC

where the DMAC has 32 request signals, and 8 channels.  The MMCI is
connected to two of them.  The DMAC can supply any of its physical
channels for MMCI.

Board 1 has the MMCI connected to request signals #1 and #3.  Board 2
has the MMCI connected to request signals #8 and #22.  Board 3 has
the MMCI connected through an external FPGA mux, which can route the
MMCI requests to DMA request signals #1, #2 or #3.

Now, explain how a channel is selected for each of those two boards, and
the DMA controller is provided with the relevant request signal is found
without platform data involved, using _only_ your 'capabilities' bitfield
and the channel ID number.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Jaswinder Singh]

On 28 July 2011 01:58, Russell King <rmk@arm.linux.org.uk> wrote:
> On Wed, Jul 27, 2011 at 10:44:53PM +0530, Jaswinder Singh wrote:
>> 1) What I propose
>>        http://lists.infradead.org/pipermail/linux-arm-kernel/2011-July/059212.html
>>
>> 2) Why RMK thinks I am the biggest idiot on earth
>>        http://lists.infradead.org/pipermail/linux-arm-kernel/2011-July/059217.html
>>
>> 3) How I ask for better proof of that
>>       http://lists.infradead.org/pipermail/linux-arm-kernel/2011-July/059223.html

> How do you deal with a peripheral being linked to a _specific_ DMA
> engine on a _specific_ DMA request signal?  What if your system has
> two DMA engines, each with 32 request signals?  Are you going to have
> something like a 128-bit capability mask?

I am afraid, you don't get the idea.

Let's consider even more 'complicated' and general case:-

The SoC/platform has 5 DMACs with 64request-signals(channels) each.
Say, only 16 of these 64request signals can be active at a time,
i.e, dmac has 16 internal work-horse threads(doesn't really matter to the API)
The platform is very flexible and it is possible for an implementation
(the board) to route _any_ request signal on _any_ dmac to _any_ peripheral.

Say, the board 'PITA', decides to use
   DMAC0_Chan8  and DMAC1_Chan31   for  MMC2_RX
   DMAC4_Chan57 and DMAC3__Chan8   for  MMC2_TX

That is, each of MMC2 RX and TX are connected to 2 request-signals.
Duplex channels don't make much difference to the scheme.

Obviously, a platform with such a flexible dmac need the board to
provide the channel map. And the dmac driver ought to be written
accordingly i.e, platform-independent, well always.

So the board, arch/arm/mach-abc/board-pita.c, will provide the
RequestSignal-Peripheral map to the platform arch/arm/mach-abc/dma.c
in platform specific format.
Or via DT when we have it.

The platform must then pass on the RequestSignal-Peripheral map to the
dmac driver via platform_data in the format the platform-independent
dmac driver expects.

At this point the dmac has been 'stripped-off' of its channel-mapping
flexibility.
The dmac driver now has fixed(board specific) RequestSignal-Peripheral
map via platfrom_data.

{
So far, I haven't assumed the capability of a DMAC to switch, say,
the RequestSignal_7 from MMC to UART at _runtime_
If you think such dmacs and platforms exist, please let me know.
I will update the proposal.
}

In the probe, the DMAC driver will populate the tag/capability-list
of the dma_chan corresponding to each RequestSignal. Remember
the dmac driver already got it from platform.

{
Tag / Capability-List
***********************
So far the practical capabilities(rather limitations) that I assumed, could
be expressed in 32 bits. If need be we can either increase the size or use
some common data structure, say struct dma_chan_cap, to express capabilities.
This 'tag' could be a pointer to a data-structure or simply a u32 or whatever.
 *** That is implementation detail ***
Please get over the impression that I insist on using chan_id for the purpose.
I do not.
}

For ex, for 'PITA' board, the dmac driver (via info directly gotten
from platform) will announce
  cap_rs8  := 'MMC' | '2ndInstance' | 'Dev->Mem'   //via probe of DMAC0
  cap_rs31 := 'MMC' | '2ndInstance' | 'Dev->Mem'   //via probe of DMAC1
  cap_rs8  := 'MMC' | '2ndInstance' | 'Mem->Dev'   //via probe of DMAC3
  cap_rs57 := 'MMC' | '2ndInstance' | 'Mem->Dev'   //via probe of DMAC4

Btw, I have also defined an 'identity/priority' 7bits field - which will
be a part of channel's 'capability' and used by DMAENGINE API to tell
  {DMAC0_Chan8 from DMAC1_Chan31} and {DMAC4_Chan57 from DMAC3__Chan8}

Ideally, in cases like 'PITA', the capabilities(including priority) of a
channel should come from the board --> platform --> dmac driver.
Otherwise, just platform --> dmac driver


CLIENTS
***********
Any client driver already knows the Device(MMC, USB, SPI etc) and
its instance(via platform_id) it is working for.
It also knows what type(burst size/len, simplex, duplex etc) of
transfers is it gonna need.
The client conveys these(and other) requirements to the DMAENGINE API
using a global platform-independent format(say struct dma_chan_capreq).


DMAENGINE API
**********************
All the complexity should lie here.
The client would have already specified enough requirements that
in most cases only one of all free channels would meet them.
And if there are 'n' more channels that could do the job -- the
one with least value in 'identity/priority' field(gotten from platform),
will be assigned and marked busy.


For easy reference, I copy-paste again the capabilities/limitatons
that I have taken care of :-
Assuming
************
a) There are no more than 256 types of DMA'able devices
   (MMC, USB, SPI etc) --  [8bits]

b) A type of device never has more than 16 instances in a platform
   (MMC-0, MMC-1, MMC-2 etc) --  [4bits]

c) Mem->Mem, Mem->Dev, Dev->Mem, Dev->Dev capability in [4bits]

d) Max_Burst_Len for any channel is less than 64KB, in power of two [4bits]
   Support programmable Max_Burst_Len(tunable in geometric steps of 2)  [1bit]

e) Max_RW_width/fifo_width is less than 128, in power of two -- [3bits]
   Support programmable Max_RW_Width(tunable in geometric steps of 2)  [1bit]

f) Finally, No platform has more than 128 channels with identicial
   capabilities - identity/priority [7bits]

Thanks

PS: I am not replying to other comments, because I think this detailed
explanation
would either change your opinion or help you deliver the fatal blow to
my proposal.
I just want to contain the discussion. If you think I must solve the
equation for a
particular set of parameters you have, I'll be happy to.

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Jaswinder Singh]

On 28 July 2011 02:07, Russell King <rmk@arm.linux.org.uk> wrote:
> On Wed, Jul 27, 2011 at 08:00:23PM +0530, Jaswinder Singh wrote:
>> On 27 July 2011 14:32, Koul, Vinod <vinod.koul@intel.com> wrote:
>> > You can have two different DMACs in same system. At least I have two
>> > from current intel_mid_dma which are used. Both give their channel id
>> > starting from 0, 1....
>> > Further as we integrate video, audio, spi, emmc dmacs possibility of
>> > having multiple dmacs will increase in a system
>>
>> Most of Samsung's S5P series have 3 DMACs - 2 for peripherals and 1 for
>> mem->mem But that is not the point.
>>
>> This patch in no way affects what values currently a dmac driver
>> assigns to chan_id
>
> Then *explain* how the chan_id is used to match the channel which the
> peripheral requires when you have three DMA controllers, each with
> channels numbered 0 to 7.
>
>> > Sorry I didn't get you.
>> > As I understand you are trying to simplify the filter function by
>> > assigning unique ids to all channels,
>>
>> No dear. Let me put it precisely.
>>
>> Even if we make no further change to the dmaengine, this patch is the right
>> thing to do today.
>
> You sound like a politician.  "the right thing to do" is a cop-out.  That
> says "believe me, I know I'm right, but I can't say why I'm right, I just
> am."  Basically, it means that the person saying it has no clue on the
> subject they're talking about.

Why don't you look at the _patch_ and see if it's correct or not ?
Rather than passing judgement on my character.

>
> If you do have a clue, then don't say that infuriating phrase, but give an
> actual reason.
>
Please look at the first few posts in this thread.
I and Dan have gone through it cleanly enough.

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Russell King]

On Thu, Jul 28, 2011 at 04:26:25PM +0530, Jaswinder Singh wrote:
> On 28 July 2011 02:07, Russell King <rmk@arm.linux.org.uk> wrote:
> > On Wed, Jul 27, 2011 at 08:00:23PM +0530, Jaswinder Singh wrote:
> >> On 27 July 2011 14:32, Koul, Vinod <vinod.koul@intel.com> wrote:
> >> > You can have two different DMACs in same system. At least I have two
> >> > from current intel_mid_dma which are used. Both give their channel id
> >> > starting from 0, 1....
> >> > Further as we integrate video, audio, spi, emmc dmacs possibility of
> >> > having multiple dmacs will increase in a system
> >>
> >> Most of Samsung's S5P series have 3 DMACs - 2 for peripherals and 1 for
> >> mem->mem But that is not the point.
> >>
> >> This patch in no way affects what values currently a dmac driver
> >> assigns to chan_id
> >
> > Then *explain* how the chan_id is used to match the channel which the
> > peripheral requires when you have three DMA controllers, each with
> > channels numbered 0 to 7.
> >
> >> > Sorry I didn't get you.
> >> > As I understand you are trying to simplify the filter function by
> >> > assigning unique ids to all channels,
> >>
> >> No dear. Let me put it precisely.
> >>
> >> Even if we make no further change to the dmaengine, this patch is the right
> >> thing to do today.
> >
> > You sound like a politician.  "the right thing to do" is a cop-out.  That
> > says "believe me, I know I'm right, but I can't say why I'm right, I just
> > am."  Basically, it means that the person saying it has no clue on the
> > subject they're talking about.
> 
> Why don't you look at the _patch_ and see if it's correct or not ?
> Rather than passing judgement on my character.

Oh for fuck sake, this is absolutely useless.  I gave you specific examples
to explain your idea.  You refuse to do so.  So my conclusion is that your
idea can not satisfy those scenarios.

As my examples are based on _real_ boards which I have here, the conclusion
I come to is that your idea is completely unworkable and so doesn't warrant
even reading the code.

That's not passing judgement on your character.

Your complete refusal to explain how your idea applies to my example cases
does _by_ _itself_ pass a judgement on your character.  It doesn't require
any personal involvement on my part to achieve that.

So, all in all I am no longer interested in your obviously unworkable
solution.  So I say NAK to it _until_ you can provide me with the
explaination I've asked for.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Jaswinder Singh]

On 28 July 2011 02:07, Russell King <rmk@arm.linux.org.uk> wrote:
>
>> On a serious note, my proposal, and the reply, shows the possibility
>> of having :-
>> a) Client drivers that are truly platform agnostic -- no platform_data
>> poking for
>>       channel selection
>
> I really doubt that's even possible.  Take this setup:

I don't want to suggest anything wrong just because I didn't understand
your h/w.
I hope you will be kind enough to help me better understand your setup,
so that I have a fair chance to present my proposal.

A simple 'yes' or a 'no'(with clarification) is all I ask.

>
> MMCI ---> DMAC
>
> where the DMAC has 32 request signals, and 8 channels.
The DMAC is similar to PL330.
Only max 8 request-signals can be active at any time.
Is my understanding right ?

> The MMCI is connected to two of them.
I don't know anything about MMCI.
So I assume it is just another third party MMC controller.
It simply needs 2 dma channels(for RX, TX each) - be it from a DMAC
that has a programmable RequestSignal->Peripheral map or a fixed map.
Is my understanding right ?

> The DMAC can supply any of its physical channels for MMCI.
The RequestSignal->Peripheral map is decided during board design
and can not be changed later.
Is my understanding right ?

> Board 1 has the MMCI connected to request signals #1 and #3.
> Board 2 has the MMCI connected to request signals #8 and #22.
Say,
 Board1
     MMCI_RX  -> #1
     MMCI_TX  -> #3

 Board2
     MMCI_RX  -> #8
     MMCI_TX  -> #22

> Board 3 has the MMCI connected through an external FPGA mux, which can route the
> MMCI requests to DMA request signals #1, #2 or #3.
Say
 Board3
     MMCI_RX  -> #{1,2,3}
     MMCI_TX  -> #{1,2,3}
And you can't change the route(mapping) after the dmac driver has been loaded.
Is my understanding right ?

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Williams, Dan J]

On Thu, Jul 28, 2011 at 10:54 AM, Jaswinder Singh
<jaswinder.singh@linaro.org> wrote:
> On 28 July 2011 02:07, Russell King <rmk@arm.linux.org.uk> wrote:
>>
>>> On a serious note, my proposal, and the reply, shows the possibility
>>> of having :-
>>> a) Client drivers that are truly platform agnostic -- no platform_data
>>> poking for
>>>       channel selection
>>
>> I really doubt that's even possible.  Take this setup:
>
> I don't want to suggest anything wrong just because I didn't understand
> your h/w.
> I hope you will be kind enough to help me better understand your setup,
> so that I have a fair chance to present my proposal.

I don't understand all the slave usage models but the proposal seems
fragile.  It wants to encode a whole bunch of arch specific details
into a 32-bit code, but it still seems to me that there will always be
platform specific details that don't fit the code.  Especially when we
are still striving to ensure common behavior across drivers.  It seems
the first step should be getting a few instances of a client driver
interfacing with multiple dma drivers (via platform specific
machinery) and see if anything common falls out of those
implementations that can be pulled into dmaengine.  The proposal seems
to be addressing a problem that we don't yet have (several clients
associating with multiple channels in disparate and un-maintainable
ways).

--
Dan

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Jaswinder Singh]

On 28 July 2011 23:44, Williams, Dan J <dan.j.williams@intel.com> wrote:
> On Thu, Jul 28, 2011 at 10:54 AM, Jaswinder Singh
> <jaswinder.singh@linaro.org> wrote:
>> On 28 July 2011 02:07, Russell King <rmk@arm.linux.org.uk> wrote:
>>>
>>>> On a serious note, my proposal, and the reply, shows the possibility
>>>> of having :-
>>>> a) Client drivers that are truly platform agnostic -- no platform_data
>>>> poking for
>>>>       channel selection
>>>
>>> I really doubt that's even possible.  Take this setup:
>>
>> I don't want to suggest anything wrong just because I didn't understand
>> your h/w.
>> I hope you will be kind enough to help me better understand your setup,
>> so that I have a fair chance to present my proposal.
>
> I don't understand all the slave usage models but the proposal seems
> fragile.  It wants to encode a whole bunch of arch specific details
> into a 32-bit code, but it still seems to me that there will always be
> platform specific details that don't fit the code.  Especially when we
> are still striving to ensure common behavior across drivers.  It seems
> the first step should be getting a few instances of a client driver
> interfacing with multiple dma drivers (via platform specific
> machinery) and see if anything common falls out of those
> implementations that can be pulled into dmaengine.  The proposal seems
> to be addressing a problem that we don't yet have (several clients
> associating with multiple channels in disparate and un-maintainable
> ways).

Dan, did you get time to read https://lkml.org/lkml/2011/7/28/97 ?

And please try to pose a scenario(however extreme) that you think
can't be handled by the mechanism.
Let us either kill the proposal or start implementing it if it's good.

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Linus Walleij]

On Thu, Jul 28, 2011 at 12:44 PM, Jaswinder Singh
<jaswinder.singh@linaro.org> wrote:

> {
> So far, I haven't assumed the capability of a DMAC to switch, say,
> the RequestSignal_7 from MMC to UART at _runtime_
> If you think such dmacs and platforms exist, please let me know.
> I will update the proposal.
> }

Such DMACs exist, in the ARM Versatile and RealView boards...
Check drivers/dma/amba-pl08x.c, notice callback functions named
int (*get_signal)(struct pl08x_dma_chan *);
void (*put_signal)(struct pl08x_dma_chan *);
from <linux/amba/pl08x.h>

These implement optional runtime muxing of the request signals,
and it is through these two callbacks because there is a mux
which is external to the pl08x itself that is used, and the muxing
is machine-specific and implemented in the platform. I have been
working on patches using it like the below (still being worked on).

The muxing is not guaranteed to always work - i.e. you may
request a DMA transfer and it may fail because there is no
free channel to mux in. And then the driver has to fall back to
PIO.

The reason the system looks like this is due to constructing it
with a mixture of silicon and FPGAs on different bus bridges
with a limited amount of pins routed (I think).

This is the platform-specific runtime muxing for mach-realview:

+/*
+ * DMA config
+ */
+
+/* State of the big DMA mux */
+static u32 current_mux = 0x00;
+static u32 mux_users = 0x00;
+static spinlock_t current_mux_lock = SPIN_LOCK_UNLOCKED;
+
+static int pl081_get_signal(struct pl08x_dma_chan *ch)
+{
+	struct pl08x_channel_data *cd = ch->cd;
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&current_mux_lock, flags);
+	/*
+	 * We're on the same mux so fine, go ahead!
+	 */
+	if (cd->muxval == current_mux) {
+		mux_users ++;
+		spin_unlock_irqrestore(&current_mux_lock, flags);
+		/* We still have to write it since it may be OFF by default */
+		val = readl(__io_address(REALVIEW_SYS_DMAPSR));
+		val &= 0xFFFFFFFCU;
+		val |= current_mux;
+		writel(val, __io_address(REALVIEW_SYS_DMAPSR));
+		return cd->min_signal;
+	}
+	/*
+	 * If we're not on the same mux and there are already
+	 * users on the other mux setting, tough luck, the client
+	 * can come back and retry or give up and fall back to
+	 * PIO mode.
+	 */
+	if (mux_users) {
+		spin_unlock_irqrestore(&current_mux_lock, flags);
+		return -EBUSY;
+	}
+
+	/* Switch mux setting */
+	current_mux = cd->muxval;
+
+	val = readl(__io_address(REALVIEW_SYS_DMAPSR));
+	val &= 0xFFFFFFFCU;
+	val |= cd->muxval;
+	writel(val, __io_address(REALVIEW_SYS_DMAPSR));
+
+	pr_info("%s: muxing in %s in bank %d writing value "
+		"%08x to register %08x\n",
+		__func__, ch->name, cd->muxval,
+		val, REALVIEW_SYS_DMAPSR);
+
+	spin_unlock_irqrestore(&current_mux_lock, flags);
+
+	return cd->min_signal;
+}
+
+static void pl081_put_signal(struct pl08x_dma_chan *ch)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&current_mux_lock, flags);
+	mux_users--;
+	spin_unlock_irqrestore(&current_mux_lock, flags);
+}
+
+#define PRIMECELL_DEFAULT_CCTL (PL080_BSIZE_8 <<
PL080_CONTROL_SB_SIZE_SHIFT | \
+				PL080_BSIZE_8 << PL080_CONTROL_DB_SIZE_SHIFT | \
+				PL080_WIDTH_32BIT << PL080_CONTROL_SWIDTH_SHIFT | \
+				PL080_WIDTH_32BIT << PL080_CONTROL_DWIDTH_SHIFT | \
+				PL080_CONTROL_PROT_SYS)
+
+/* Muxed channels as found in most RealViews */
+struct pl08x_channel_data realview_chan_data[] = {
+	/* Muxed on signal bank 0 */
+	[0] = {
+		.bus_id = "usb0",
+		.min_signal = 0,
+		.max_signal = 0,
+		.muxval = 0x00,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.periph_buses = PL08X_AHB1 | PL08X_AHB2,
+	},
+	[1] = {
+		.bus_id = "usb1",
+		.min_signal = 1,
+		.max_signal = 1,
+		.muxval = 0x00,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.periph_buses = PL08X_AHB1 | PL08X_AHB2,
+	},
+	[2] = {
+		.bus_id = "t1dmac0",
+		.min_signal = 2,
+		.max_signal = 2,
+		.muxval = 0x00,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.periph_buses = PL08X_AHB1 | PL08X_AHB2,
+	},
+	[3] = {
+		.bus_id = "mci0",
+		.min_signal = 3,
+		.max_signal = 3,
+		.muxval = 0x00,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.periph_buses = PL08X_AHB1 | PL08X_AHB2,
+	},
+	[4] = {
+		.bus_id = "aacitx",
+		.min_signal = 4,
+		.max_signal = 4,
+		.muxval = 0x00,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.periph_buses = PL08X_AHB1 | PL08X_AHB2,
+	},
+	[5] = {
+		.bus_id = "aacirx",
+		.min_signal = 5,
+		.max_signal = 5,
+		.muxval = 0x00,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.periph_buses = PL08X_AHB1 | PL08X_AHB2,
+	},
+	[6] = {
+		.bus_id = "scirx",
+		.min_signal = 6,
+		.max_signal = 6,
+		.muxval = 0x00,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.periph_buses = PL08X_AHB1 | PL08X_AHB2,
+	},
+	[7] = {
+		.bus_id = "scitx",
+		.min_signal = 7,
+		.max_signal = 7,
+		.muxval = 0x00,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.periph_buses = PL08X_AHB1 | PL08X_AHB2,
+	},
+	/* Muxed on signal bank 1 */
+	[8] = {
+		.bus_id = "ssprx",
+		.min_signal = 0,
+		.max_signal = 0,
+		.muxval = 0x01,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.periph_buses = PL08X_AHB1 | PL08X_AHB2,
+	},
+	[9] = {
+		.bus_id = "ssptx",
+		.min_signal = 1,
+		.max_signal = 1,
+		.muxval = 0x01,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.periph_buses = PL08X_AHB1 | PL08X_AHB2,
+	},
+	[10] = {
+		.bus_id = "uart2rx",
+		.min_signal = 2,
+		.max_signal = 2,
+		.muxval = 0x01,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.periph_buses = PL08X_AHB1 | PL08X_AHB2,
+	},
+	[11] = {
+		.bus_id = "uart2tx",
+		.min_signal = 3,
+		.max_signal = 3,
+		.muxval = 0x01,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.periph_buses = PL08X_AHB1 | PL08X_AHB2,
+	},
+	[12] = {
+		.bus_id = "uart1rx",
+		.min_signal = 4,
+		.max_signal = 4,
+		.muxval = 0x01,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.periph_buses = PL08X_AHB1 | PL08X_AHB2,
+	},
+	[13] = {
+		.bus_id = "uart1tx",
+		.min_signal = 5,
+		.max_signal = 5,
+		.muxval = 0x01,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.periph_buses = PL08X_AHB1 | PL08X_AHB2,
+	},
+	[14] = {
+		.bus_id = "uart0rx",
+		.min_signal = 6,
+		.max_signal = 6,
+		.muxval = 0x01,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.periph_buses = PL08X_AHB1 | PL08X_AHB2,
+	},
+	[15] = {
+		.bus_id = "uart0tx",
+		.min_signal = 7,
+		.max_signal = 7,
+		.muxval = 0x01,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.periph_buses = PL08X_AHB1 | PL08X_AHB2,
+	},
+};
+
+struct pl08x_platform_data pl081_plat_data = {
+	.memcpy_channel = {
+		.bus_id = "memcpy",
+		/*
+		 * We pass in some optimal memcpy config, the
+		 * driver will augment it if need be. 256 byte
+		 * bursts and 32bit bus width.
+		 */
+		.cctl =
+		(PL080_BSIZE_256 << PL080_CONTROL_SB_SIZE_SHIFT |	\
+		 PL080_BSIZE_256 << PL080_CONTROL_DB_SIZE_SHIFT |	\
+		 PL080_WIDTH_32BIT << PL080_CONTROL_SWIDTH_SHIFT |	\
+		 PL080_WIDTH_32BIT << PL080_CONTROL_DWIDTH_SHIFT |	\
+		 PL080_CONTROL_PROT_BUFF |				\
+		 PL080_CONTROL_PROT_CACHE |				\
+		 PL080_CONTROL_PROT_SYS),
+		.periph_buses = PL08X_AHB1 | PL08X_AHB2,
+	},
+	.slave_channels = realview_chan_data,
+	.num_slave_channels = ARRAY_SIZE(realview_chan_data),
+	.get_signal = pl081_get_signal,
+	.put_signal = pl081_put_signal,
+};


This is the platform-specific runtime muxing for mach-versatile:


+/*
+ * DMA config
+ * The DMA channel routing is static on the PA926EJ-S
+ * and dynamic on the PB926EJ-S using SYS_DMAPSR0..SYS_DMAPSR2
+ */
+
+struct pb926ejs_dma_signal {
+	unsigned int id;
+	struct pl08x_dma_chan *user;
+	u32 ctrlreg;
+};
+
+/*
+ * The three first channels on ARM926EJ-S are muxed,
+ * but to make things simple we enable all channels
+ * to be muxed, however most of the channels will just
+ * me muxed on top of themselves.
+ */
+static struct pb926ejs_dma_signal pl080_muxtab[] = {
+	[0] = {
+		.id = 0,
+		.ctrlreg = VERSATILE_SYS_DMAPSR0,
+	},
+	[1] = {
+		.id = 1,
+		.ctrlreg = VERSATILE_SYS_DMAPSR1,
+	},
+	[2] = {
+		.id = 2,
+		.ctrlreg = VERSATILE_SYS_DMAPSR2,
+	},
+	[3] = {
+		.id = 3,
+	},
+	[4] = {
+		.id = 4,
+	},
+	[5] = {
+		.id = 5,
+	},
+	[6] = {
+		.id = 6,
+	},
+	[7] = {
+		.id = 7,
+	},
+	[8] = {
+		.id = 8,
+	},
+	[9] = {
+		.id = 9,
+	},
+	[10] = {
+		.id = 10,
+	},
+	[11] = {
+		.id = 11,
+	},
+	[12] = {
+		.id = 12,
+	},
+	[13] = {
+		.id = 13,
+	},
+	[14] = {
+		.id = 14,
+	},
+	[15] = {
+		.id = 15,
+	},
+};
+
+/* This is a lock for the above muxing array */
+static spinlock_t muxlock = SPIN_LOCK_UNLOCKED;
+
+static int pl080_get_signal(struct pl08x_dma_chan *ch)
+{
+	struct pl08x_channel_data *cd = ch->cd;
+
+	pr_debug("requesting DMA signal on channel %s\n", ch->name);
+
+	/*
+	 * The AB926EJ-S is simple - only static assignments
+	 * so the channel is already muxed in and ready to go.
+	 */
+	if (machine_is_versatile_ab())
+		return cd->min_signal;
+
+	/* The PB926EJ-S is hairier */
+	if (machine_is_versatile_pb()) {
+		unsigned long flags;
+		int i;
+
+		if (cd->min_signal > ARRAY_SIZE(pl080_muxtab) ||
+		    cd->max_signal > ARRAY_SIZE(pl080_muxtab) ||
+		    (cd->max_signal < cd->min_signal)) {
+			pr_err("%s: illegal muxing constraints for %s\n",
+			       __func__, ch->name);
+			return -EINVAL;
+		}
+		/* Try to find a signal to use */
+		spin_lock_irqsave(&muxlock, flags);
+		for (i = cd->min_signal; i <= cd->max_signal; i++) {
+			if (!pl080_muxtab[i].user) {
+				u32 val;
+
+				pl080_muxtab[i].user = ch;
+				/* If the channels need to be muxed in, mux them! */
+				if (pl080_muxtab[i].ctrlreg) {
+					val = readl(__io_address(pl080_muxtab[i].ctrlreg));
+					val &= 0xFFFFFF70U;
+					val |= 0x80; /* Turn on muxing */
+					val |= cd->muxval; /* Mux in the right peripheral */
+					writel(val, __io_address(pl080_muxtab[i].ctrlreg));
+					pr_debug("%s: muxing in %s at channel %d writing value "
+						 "%08x to register %08x\n",
+						 __func__, ch->name, i,
+						 val, pl080_muxtab[i].ctrlreg);
+				}
+				spin_unlock_irqrestore(&muxlock, flags);
+				return pl080_muxtab[i].id;
+			}
+		}
+		spin_unlock_irqrestore(&muxlock, flags);
+	}
+
+	return -EBUSY;
+}
+
+static void pl080_put_signal(struct pl08x_dma_chan *ch)
+{
+	struct pl08x_channel_data *cd = ch->cd;
+	unsigned long flags;
+	int i;
+
+	pr_debug("releasing DMA signal on channel %s\n", ch->name);
+	if (cd->min_signal > ARRAY_SIZE(pl080_muxtab) ||
+	    cd->max_signal > ARRAY_SIZE(pl080_muxtab) ||
+	    (cd->max_signal < cd->min_signal)) {
+		pr_err("%s: illegal muxing constraints for %s\n",
+		       __func__, ch->name);
+		return;
+	}
+	spin_lock_irqsave(&muxlock, flags);
+	for (i = cd->min_signal; i <= cd->max_signal; i++) {
+		if (pl080_muxtab[i].user == ch) {
+			pl080_muxtab[i].user = NULL;
+			if (pl080_muxtab[i].ctrlreg) {
+				u32 val;
+
+				val = readl(__io_address(pl080_muxtab[i].ctrlreg));
+				val &= 0xFFFFFF70U; /* Disable, select no channel */
+				writel(val, __io_address(pl080_muxtab[i].ctrlreg));
+				pr_debug("%s: muxing out %s at channel %d writing value "
+				       "%08x to register %08x\n",
+				       __func__, ch->name, i,
+				       val, pl080_muxtab[i].ctrlreg);
+			}
+			spin_unlock_irqrestore(&muxlock, flags);
+			return;
+		}
+	}
+	spin_unlock_irqrestore(&muxlock, flags);
+	pr_debug("%s: unable to release muxing on channel %s\n",
+	       __func__, ch->name);
+}
+
+struct pl08x_platform_data pl080_plat_data = {
+	.memcpy_channel = {
+		.bus_id = "memcpy",
+		/*
+		 * We pass in some optimal memcpy config, the
+		 * driver will augment it if need be. 256 byte
+		 * bursts and 32bit bus width.
+		 */
+		.cctl =
+		(PL080_BSIZE_256 << PL080_CONTROL_SB_SIZE_SHIFT |	\
+		 PL080_BSIZE_256 << PL080_CONTROL_DB_SIZE_SHIFT |	\
+		 PL080_WIDTH_32BIT << PL080_CONTROL_SWIDTH_SHIFT |	\
+		 PL080_WIDTH_32BIT << PL080_CONTROL_DWIDTH_SHIFT |	\
+		 PL080_CONTROL_PROT_BUFF |				\
+		 PL080_CONTROL_PROT_CACHE |				\
+		 PL080_CONTROL_PROT_SYS),
+		/* Flow control: DMAC controls this */
+		.ccfg = PL080_FLOW_MEM2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	.get_signal = pl080_get_signal,
+	.put_signal = pl080_put_signal,
+};
+
+#define PRIMECELL_DEFAULT_CCTL (PL080_BSIZE_8 <<
PL080_CONTROL_SB_SIZE_SHIFT | \
+			PL080_BSIZE_8 << PL080_CONTROL_DB_SIZE_SHIFT | \
+			PL080_WIDTH_32BIT << PL080_CONTROL_SWIDTH_SHIFT | \
+			PL080_WIDTH_32BIT << PL080_CONTROL_DWIDTH_SHIFT | \
+			PL080_CONTROL_PROT_SYS)
+
+static struct pl08x_channel_data ab926ejs_chan_data[] = {
+	[0] = {
+		.bus_id = "aacirx",
+		.min_signal = 0,
+		.max_signal = 0,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_PER2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[1] = {
+		.bus_id = "aacitx",
+		.min_signal = 1,
+		.max_signal = 1,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_MEM2PER << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[2] = {
+		.bus_id = "mci0",
+		.min_signal = 2,
+		.max_signal = 2,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+	},
+	[3] = {
+		.bus_id = "reserved",
+		.min_signal = 3,
+		.max_signal = 3,
+	},
+	[4] = {
+		.bus_id = "reserved",
+		.min_signal = 4,
+		.max_signal = 4,
+	},
+	[5] = {
+		.bus_id = "reserved",
+		.min_signal = 5,
+		.max_signal = 5,
+	},
+	[6] = {
+		.bus_id = "scirx",
+		.min_signal = 6,
+		.max_signal = 6,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_PER2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[7] = {
+		.bus_id = "scitx",
+		.min_signal = 7,
+		.max_signal = 7,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_MEM2PER << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[8] = {
+		.bus_id = "ssprx",
+		.min_signal = 8,
+		.max_signal = 8,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_PER2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[9] = {
+		.bus_id = "ssptx",
+		.min_signal = 9,
+		.max_signal = 9,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_MEM2PER << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[10] = {
+		.bus_id = "uart2rx",
+		.min_signal = 10,
+		.max_signal = 10,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_PER2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[11] = {
+		.bus_id = "uart2tx",
+		.min_signal = 11,
+		.max_signal = 11,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_MEM2PER << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[12] = {
+		.bus_id = "uart1rx",
+		.min_signal = 12,
+		.max_signal = 12,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_PER2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[13] = {
+		.bus_id = "uart1tx",
+		.min_signal = 13,
+		.max_signal = 13,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_MEM2PER << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[14] = {
+		.bus_id = "uart0rx",
+		.min_signal = 14,
+		.max_signal = 14,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_PER2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[15] = {
+		.bus_id = "uart0tx",
+		.min_signal = 15,
+		.max_signal = 15,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_MEM2PER << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+};
+
+/* For the PB926EJ-S we define some extra virtual channels */
+static struct pl08x_channel_data pb926ejs_chan_data[] = {
+	[0] = {
+		/* Muxed on channel 0-3 */
+		.bus_id = "aacitx",
+		.min_signal = 0,
+		.max_signal = 2,
+		.muxval = 0x00,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_MEM2PER << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[1] = {
+		/* Muxed on channel 0-3 */
+		.bus_id = "aacirx",
+		.min_signal = 0,
+		.max_signal = 2,
+		.muxval = 0x01,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_PER2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[2] = {
+		/* Muxed on channel 0-3 */
+		.bus_id = "usba",
+		.min_signal = 0,
+		.max_signal = 2,
+		.muxval = 0x02,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+	},
+	[3] = {
+		/* Muxed on channel 0-3 */
+		.bus_id = "usbb",
+		.min_signal = 0,
+		.max_signal = 2,
+		.muxval = 0x03,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+	},
+	[4] = {
+		/* Muxed on channel 0-3 */
+		.bus_id = "mci0",
+		.min_signal = 0,
+		.max_signal = 2,
+		.muxval = 0x04,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+	},
+	[5] = {
+		/* Muxed on channel 0-3 */
+		.bus_id = "mci1",
+		.min_signal = 0,
+		.max_signal = 2,
+		.muxval = 0x05,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+	},
+	[6] = {
+		/* Muxed on channel 0-3 */
+		.bus_id = "uart3tx",
+		.min_signal = 0,
+		.max_signal = 2,
+		.muxval = 0x06,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_MEM2PER << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[7] = {
+		/* Muxed on channel 0-3 */
+		.bus_id = "uart3rx",
+		.min_signal = 0,
+		.max_signal = 2,
+		.muxval = 0x07,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_PER2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[8] = {
+		/* Muxed on channel 0-3 */
+		.bus_id = "sciinta",
+		.min_signal = 0,
+		.max_signal = 2,
+		.muxval = 0x08,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+	},
+	[9] = {
+		/* Muxed on channel 0-3 */
+		.bus_id = "sciintb",
+		.min_signal = 0,
+		.max_signal = 2,
+		.muxval = 0x09,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+	},
+	[10] = {
+		.bus_id = "scirx",
+		.min_signal = 6,
+		.max_signal = 6,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_PER2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[11] = {
+		.bus_id = "scitx",
+		.min_signal = 7,
+		.max_signal = 7,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_MEM2PER << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[12] = {
+		.bus_id = "ssprx",
+		.min_signal = 8,
+		.max_signal = 8,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_PER2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[13] = {
+		.bus_id = "ssptx",
+		.min_signal = 9,
+		.max_signal = 9,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_MEM2PER << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[14] = {
+		.bus_id = "uart2rx",
+		.min_signal = 10,
+		.max_signal = 10,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_PER2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[15] = {
+		.bus_id = "uart2tx",
+		.min_signal = 11,
+		.max_signal = 11,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_MEM2PER << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[16] = {
+		.bus_id = "uart1rx",
+		.min_signal = 12,
+		.max_signal = 12,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_PER2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[17] = {
+		.bus_id = "uart1tx",
+		.min_signal = 13,
+		.max_signal = 13,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_MEM2PER << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[18] = {
+		.bus_id = "uart0rx",
+		.min_signal = 14,
+		.max_signal = 14,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_PER2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+	[19] = {
+		.bus_id = "uart0tx",
+		.min_signal = 15,
+		.max_signal = 15,
+		.cctl = PRIMECELL_DEFAULT_CCTL,
+		.ccfg = PL080_FLOW_MEM2PER << PL080_CONFIG_FLOW_CONTROL_SHIFT,
+	},
+};
+
+static void __init pl080_fixup(void)
+{
+	if (machine_is_versatile_ab()) {
+		pl080_plat_data.slave_channels = ab926ejs_chan_data;
+		pl080_plat_data.num_slave_channels =
+			ARRAY_SIZE(ab926ejs_chan_data);
+	}
+	if (machine_is_versatile_pb()) {
+		pl080_plat_data.slave_channels = pb926ejs_chan_data;
+		pl080_plat_data.num_slave_channels =
+			ARRAY_SIZE(pb926ejs_chan_data);
+	}
+}


Thanks,
Linus Walleij

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Russell King]

On Thu, Jul 28, 2011 at 04:14:34PM +0530, Jaswinder Singh wrote:
> For ex, for 'PITA' board, the dmac driver (via info directly gotten
> from platform) will announce
>   cap_rs8  := 'MMC' | '2ndInstance' | 'Dev->Mem'   //via probe of DMAC0
>   cap_rs31 := 'MMC' | '2ndInstance' | 'Dev->Mem'   //via probe of DMAC1
>   cap_rs8  := 'MMC' | '2ndInstance' | 'Mem->Dev'   //via probe of DMAC3
>   cap_rs57 := 'MMC' | '2ndInstance' | 'Mem->Dev'   //via probe of DMAC4

Most of this didn't look too bad until I got to here.

> Assuming
> ************
> a) There are no more than 256 types of DMA'able devices
>    (MMC, USB, SPI etc) --  [8bits]

Who allocates the 'type of dma' number ?

> b) A type of device never has more than 16 instances in a platform
>    (MMC-0, MMC-1, MMC-2 etc) --  [4bits]

How is the instance number given to devices ?

Are we expecting to have subsystems register with some kind of entity
which gives out 'type' and 'instance' numbers just to satisfy this?

In any case, if you look at Linus W's patches on LAKML for DMA on the
Versatile/Realview platforms, or look at the way AMBA drivers like MMCI
and PL011 UART deal with the 'filter' business, then I think you'd
realize that there's ways to deal with this match problem which are far
more flexible than your solution.

What we do there is:

1. We provide a match function from the platform to the peripheral driver.
   ==> this could be your special generic filter function.
2. We provide the match functions data from the platform to the peripheral
   driver.
   ==> this could be your special capability mask for that specific device.

So, as things stand _today_ if a platform wants to use your scheme, it
can.  But - and this is what makes things more flexible - if it needs to
do something else which your scheme can't handle, such as controlling an
external MUX which has nothing to do with the DMAC or peripheral apart
from sitting in the middle between the two - then it can do it its own
way.

I think that's a more flexible approach than any which enforces random
kinds of capabilities.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Russell King]

On Thu, Jul 28, 2011 at 11:24:12PM +0530, Jaswinder Singh wrote:
> On 28 July 2011 02:07, Russell King <rmk@arm.linux.org.uk> wrote:
> >
> >> On a serious note, my proposal, and the reply, shows the possibility
> >> of having :-
> >> a) Client drivers that are truly platform agnostic -- no platform_data
> >> poking for
> >>       channel selection
> >
> > I really doubt that's even possible.  Take this setup:
> 
> I don't want to suggest anything wrong just because I didn't understand
> your h/w.
> I hope you will be kind enough to help me better understand your setup,
> so that I have a fair chance to present my proposal.
> 
> A simple 'yes' or a 'no'(with clarification) is all I ask.
> 
> >
> > MMCI ---> DMAC
> >
> > where the DMAC has 32 request signals, and 8 channels.
> The DMAC is similar to PL330.

That coincidence is merely that.

> Only max 8 request-signals can be active at any time.
> Is my understanding right ?

Correct.

> > The MMCI is connected to two of them.
> I don't know anything about MMCI.
> So I assume it is just another third party MMC controller.
> It simply needs 2 dma channels(for RX, TX each) - be it from a DMAC
> that has a programmable RequestSignal->Peripheral map or a fixed map.
> Is my understanding right ?

Correct.

> > The DMAC can supply any of its physical channels for MMCI.
> The RequestSignal->Peripheral map is decided during board design
> and can not be changed later.
> Is my understanding right ?

No.  See the board#3 case..

> > Board 1 has the MMCI connected to request signals #1 and #3.
> > Board 2 has the MMCI connected to request signals #8 and #22.
> Say,
>  Board1
>      MMCI_RX  -> #1
>      MMCI_TX  -> #3
> 
>  Board2
>      MMCI_RX  -> #8
>      MMCI_TX  -> #22
> 
> > Board 3 has the MMCI connected through an external FPGA mux, which can route the
> > MMCI requests to DMA request signals #1, #2 or #3.
> Say
>  Board3
>      MMCI_RX  -> #{1,2,3}
>      MMCI_TX  -> #{1,2,3}
> And you can't change the route(mapping) after the dmac driver has
> been loaded.

No.  You have to change it dynamically at run time according to the
DMA activity, because DMA request signals #1, #2 and #3 are shared
between 6 devices.  To make matters worse, it's not six on any of
RQ#1 RQ#2 RQ#3, but some on a couple, some on another couple, and
some on all three.

BTW, we do support this with Linus W's code (which he's posted to
this thread.)

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Russell King]

On Fri, Jul 29, 2011 at 12:27:58AM +0200, Linus Walleij wrote:
> +static struct pl08x_channel_data ab926ejs_chan_data[] = {
> +	[0] = {
> +		.bus_id = "aacirx",
> +		.min_signal = 0,
> +		.max_signal = 0,
> +		.cctl = PRIMECELL_DEFAULT_CCTL,
> +		.ccfg = PL080_FLOW_PER2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT,

Linus,

We really should sync our two implementations for this - all the ccfg
stuff should be gone from these structures, esp. as its already long
gone from the structure itself.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Koul, Vinod]

On Fri, 2011-07-29 at 00:27 +0200, Linus Walleij wrote:
> On Thu, Jul 28, 2011 at 12:44 PM, Jaswinder Singh
> <jaswinder.singh@linaro.org> wrote:
> 
> > {
> > So far, I haven't assumed the capability of a DMAC to switch, say,
> > the RequestSignal_7 from MMC to UART at _runtime_
> > If you think such dmacs and platforms exist, please let me know.
> > I will update the proposal.
> > }
> 
> Such DMACs exist, in the ARM Versatile and RealView boards...
> Check drivers/dma/amba-pl08x.c, notice callback functions named
> int (*get_signal)(struct pl08x_dma_chan *);
> void (*put_signal)(struct pl08x_dma_chan *);
> from <linux/amba/pl08x.h>
> 
> These implement optional runtime muxing of the request signals,
> and it is through these two callbacks because there is a mux
> which is external to the pl08x itself that is used, and the muxing
> is machine-specific and implemented in the platform. I have been
> working on patches using it like the below (still being worked on).
> 
Yes, even in intel_mid_dmac we have such a mux so that DMAC channels can
transfer to any of the h/w interfaces. This configuration can be passed
by client (which instance) and dmac configures it :)
So all channels for a controller can do transfers to any of the
peripherals...

-- 
~Vinod

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Linus Walleij]

2011/7/29 Russell King <rmk@arm.linux.org.uk>:
> On Fri, Jul 29, 2011 at 12:27:58AM +0200, Linus Walleij wrote:
>> +static struct pl08x_channel_data ab926ejs_chan_data[] = {
>> +     [0] = {
>> +             .bus_id = "aacirx",
>> +             .min_signal = 0,
>> +             .max_signal = 0,
>> +             .cctl = PRIMECELL_DEFAULT_CCTL,
>> +             .ccfg = PL080_FLOW_PER2MEM << PL080_CONFIG_FLOW_CONTROL_SHIFT,
>
> Linus,
>
> We really should sync our two implementations for this - all the ccfg
> stuff should be gone from these structures, esp. as its already long
> gone from the structure itself.

Sure, it was merely an illustration. I figured you must have some code
since you've been testing it on AACI and whatnot. I'm currently in the
countryside, I plan to pick this up when I get back to the office (where
the PB11MPCore is) in mid-august.

If you have some platform data + mux code that fits with your latest
patches to the PL08x driver, then please poste to LAKML and I'll have
them tested ASAP.

Yours,
Linus Walleij

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Jaswinder Singh]

On 29 July 2011 04:05, Russell King <rmk@arm.linux.org.uk> wrote:
> On Thu, Jul 28, 2011 at 04:14:34PM +0530, Jaswinder Singh wrote:
>> For ex, for 'PITA' board, the dmac driver (via info directly gotten
>> from platform) will announce
>>   cap_rs8  := 'MMC' | '2ndInstance' | 'Dev->Mem'   //via probe of DMAC0
>>   cap_rs31 := 'MMC' | '2ndInstance' | 'Dev->Mem'   //via probe of DMAC1
>>   cap_rs8  := 'MMC' | '2ndInstance' | 'Mem->Dev'   //via probe of DMAC3
>>   cap_rs57 := 'MMC' | '2ndInstance' | 'Mem->Dev'   //via probe of DMAC4
>
> Most of this didn't look too bad until I got to here.
>
>> Assuming
>> ************
>> a) There are no more than 256 types of DMA'able devices
>>    (MMC, USB, SPI etc) --  [8bits]
>
> Who allocates the 'type of dma' number ?
Look at it as a way for clients to tell the class of device they
drive, and for platforms to declare suitable candidate channel(s)
that have the capability to talk to that class of device controllers.

RequestSignal<->Peripheral link is fixed :-
    Platform --> dmac_driver   //from the DMAC chapter of the SoC's manual
RequestSignal<->Peripheral link is decided during board design :-
    Board --> platform --> dmac_driver

In kernel, there would be a list of global defines in DMAENGINE API's
namespace.
Please look at the end of this post to get an idea of how it might look.


>> b) A type of device never has more than 16 instances in a platform
>>    (MMC-0, MMC-1, MMC-2 etc) --  [4bits]
>
> How is the instance number given to devices ?
Same as how the 'type of dma' is decided on the dmac side of API.
RequestSignal<->Peripheral link is fixed :-
    Platform --> dmac_driver   //from the DMAC chapter of the SoC's manual
RequestSignal<->Peripheral link is decided during board design :-
    Board --> platform --> dmac_driver


> Are we expecting to have subsystems register with some kind of entity
> which gives out 'type' and 'instance' numbers just to satisfy this?
Nopes. Simply a matter of client-developer checking a header file and
the dmac_driver developer checking the same header file and DMAC chapter
of SoC's manual.


> In any case, if you look at Linus W's patches on LAKML for DMA on the
> Versatile/Realview platforms, or look at the way AMBA drivers like MMCI
> and PL011 UART deal with the 'filter' business, then I think you'd
> realize that there's ways to deal with this match problem which are far
> more flexible than your solution.

Looking at drivers/mmc/host/mmci.c and drivers/tty/serial/amba-pl011.c ...

They force platforms to have these members in the platform_data
	bool (*dma_filter)(struct dma_chan *chan, void *filter_param);
        void *dma_rx_param;
        void *dma_tx_param;

Only to ritualistically pass them back during dma_request_channel.
And that is the _least_ any client must do.

The 'filter' function and the 'data' simply have to loop back to the
dmac driver side of API and have _nothing_ to do with Client driver.

If we are to have common clients and reusable dmac drivers ...
Wouldn't this mechanism force _every_ platform to have the same two as
members of platform_data for _every_ controller it contains?
Afterall, most controllers could employ DMA.

Not to forget, you'll also have to standardize the dmac_driver<-->platform
interface.

Sorry, I don't think this, rather any, 'filter' thing could survive for long.

My proposal concentrates the whole channel selection logic within
the DMAENGINE API. Filter mechanism could be gotten away.



> What we do there is:
>
> 1. We provide a match function from the platform to the peripheral driver.
>   ==> this could be your special generic filter function.
> 2. We provide the match functions data from the platform to the peripheral
>   driver.
>   ==> this could be your special capability mask for that specific device.
I am no more talking about Samsung's channel selection mechanism.
If the filter callbacks are absolutely simplified by 'good' implementation on
the dmac side of the API -- why not drop the filter altogether ?
And if the filter callbacks would still contain platform specific stuff, sure
there is something crooked.
And as I said, we really need to rethink if this 'filter' mechanism
could survive.


> which your scheme can't handle, such as controlling an
> external MUX which has nothing to do with the DMAC or peripheral apart
> from sitting in the middle between the two - then it can do it its own
> way.
If it has nothing to do with the Client, nothing to do with the DMAC,
IMHO, it has absolutely nothing to do with the DMAENGINE API.

If anything, it should be folded into the {dmac_driver + platform + board}
logic. Perhaps, simply by having the dmac_driver do a callback on platform.
Something like
  err = rs_activate(DMACd_RSs)	//before starting the xfers
  rs_deactivate(DMACd_RSs)	//after xfers-done irq

And yes, this would very well work if clients and dmac_drivers are written
cleanly.
Client Drivers :- Whereever they request a channel, their design should
 take into account the fact that even after being allocated a channel,
 the DMAC might refuse to do a transfer due to contention on the bus.

DMAC Drivers :- They should not lock resources(like ReqSig) upon channel
allocation to a client, but only when they need to transfer.
Also, impo, they should completely dissociate actual h/w identity of a
req-sig from the 'token' they return to clients as the 'channel'.
That is, dma_request_channel should only pave a 'software-path' to actual
h/w channels - which may or may not be available when the client needs them.

Your scenario of ext-fpga-mux is similar to PL330 - main difference being the
request-signal activation mechanism is completely contained within the PL330
(only 8/32 can be active at a time) where as you need to manipulate ext-fpga for
channel selection.



-------------------------8<--------------------

***** In  include/linux/dmaengine.h  ****


/* ! This is just to give an idea. Please don't get hung on optimizations ! */

/*
 * _SAY_ we agree to use u32 for tag/capability-list
 *
 * Constraints/Capabilities
 * ************************
 * # MRWW - Max_RW_width/fifo_width, in power of 2 [3bits]
 *
 * # PMRWW - If Max_RW_Width is programmable(in geometric steps of 2) [1bit]
 *
 * # MBL - Max_Burst_Len for the channel, in power of 2 [4bits]
 *
 * # PMBL - If Max_Burst_Len is programmable(in geometric steps of 2) [1bit]
 *
 * # DIR - Mem->Mem, Mem->Dev, Dev->Mem, Dev->Devi capability [4bits]
 *
 * # DEV - Class of the device(MMC, USB, SPI etc) [7bits]
 *
 * # INST - Controller Instance(MMC-0, MMC-1, MMC-2 etc) [3bits]
 *
 * # PRI - Priority of a channel amoung set of equally capable ones [6bits]
 *
 * # XXX - Reserved for future use
 *
 * [XXX_3][MRWW_3][PMRWW_1][MBL_4][PMBL_1][DIR_4][DEV_7][INST_3][PRI_6]
 */

#define DCH_CAP_PRI_OFF		0
#define DCH_CAP_PRI_MASK	(0x3f << DCH_CAP_PRI_OFF)
#define DCH_CAP_INST_OFF	6
#define DCH_CAP_INST_MASK	(0x7 << DCH_CAP_INST_OFF)
#define DCH_CAP_DEV_OFF		9
#define DCH_CAP_DEV_MASK	(0x7f << DCH_CAP_DEV_OFF)

#define DCH_CAP_DIR_M2M	(1 << 15)
#define DCH_CAP_DIR_M2P	(1 << 16)
#define DCH_CAP_DIR_P2M	(1 << 17)
#define DCH_CAP_DIR_P2P	(1 << 18)

#define DCH_CAP_PMBL_OFF	20
#define DCH_CAP_PMBL_MASK	(0x1 << DCH_CAP_PMBL_OFF)
#define DCH_CAP_MBL_OFF		21
#define DCH_CAP_MBL_MASK	(0xf << DCH_CAP_MBL_OFF)
#define DCH_CAP_PMRWW_OFF	25
#define DCH_CAP_PMRWW_MASK	(0x1 << DCH_CAP_PMRWW_OFF)
#define DCH_CAP_MRWW_OFF	26
#define DCH_CAP_MRWW_MASK	(0x7 << DCH_CAP_MRWW_OFF)


/*** Helpers for DMAENGINE API ***/
#define GET_DEVTYPE(c)	(((c) & DCH_CAP_DEV_MASK) >> DCH_CAP_DEV_OFF)
#define GET_DEVINST(c)	(((c) & DCH_CAP_INST_MASK) >> DCH_CAP_INST_OFF)
#define GET_CHANPRI(c)	(((c) & DCH_CAP_PRI_MASK) >> DCH_CAP_PRI_OFF)
#define IS_DCH_M2M(c)	((c) & DCH_CAP_DIR_M2M)
#define IS_DCH_M2P(c)	((c) & DCH_CAP_DIR_M2P)
#define IS_DCH_P2M(c)	((c) & DCH_CAP_DIR_P2M)
#define IS_DCH_P2P(c)	((c) & DCH_CAP_DIR_P2P)


/*** Helpers for Platform ***/
#define SET_DEVTYPE(c, t) do {	\
				c &= ~DCH_CAP_DEV_MASK; \
				c |= ((t) << DCH_CAP_DEV_OFF); \
			  } while (0)

#define SET_DEVINST(c, i) do {	\
				c &= ~DCH_CAP_INST_MASK; \
				c |= ((t) << DCH_CAP_INST_OFF); \
			  } while (0)

#define SET_CHANPRI(c, p) do {	\
				c &= ~DCH_CAP_PRI_MASK; \
				c |= ((t) << DCH_CAP_PRI_OFF); \
			  } while (0)

#define SET_DCHDIR(c, m2m, m2p, p2m, p2p) do { \
						if (m2m) \
							c |= DCH_CAP_DIR_M2M; \
						else \
							c &= ~DCH_CAP_DIR_M2M; \
						if (m2p) \
							c |= DCH_CAP_DIR_M2P; \
						else \
							c &= ~DCH_CAP_DIR_M2P; \
						if (p2m) \
							c |= DCH_CAP_DIR_P2M; \
						else \
							c &= ~DCH_CAP_DIR_P2M; \
						if (p2p) \
							c |= DCH_CAP_DIR_P2P; \
						else \
							c &= ~DCH_CAP_DIR_P2P; \
					  } while (0)


/*** Helpers for Clients ***/
#define DCH_RESET_CAP(c)		do {c = 0;} while (0)
#define DCH_REQCAP_DEV(c, TYPE)		SET_DEVTYPE(c, TYPE)
#define DCH_REQCAP_INST(c, INST)	SET_DEVINST(c, INST)
#define DCH_REQCAP_TXRX(c)		SET_DCHDIR(c, 0, 1, 1, 0)
#define DCH_REQCAP_M2M(c)		SET_DCHDIR(c, 1, 0, 0, 0)
#define DCH_REQCAP_M2P(c)		SET_DCHDIR(c, 0, 1, 0, 0)
#define DCH_REQCAP_P2M(c)		SET_DCHDIR(c, 0, 0, 1, 0)
#define DCH_REQCAP_P2P(c)		SET_DCHDIR(c, 0, 0, 0, 1)


/*
 * A Client driver should add a new class to this list.
 * Platforms should only look-up this list to set appropriate
 * 'dev-type' for the DMA channel. Even if a platform has
 * many more channels, it doesn't matter if the client
 * driver doesn't exist.
 */
#define DCHAN_TODEV_AC97	0
#define DCHAN_TODEV_CAMERA	1
#define DCHAN_TODEV_GPU		2
#define DCHAN_TODEV_I2S		3
#define DCHAN_TODEV_LCD		4
#define DCHAN_TODEV_MMC		5
#define DCHAN_TODEV_SPI		6
#define DCHAN_TODEV_UART	7
#define DCHAN_TODEV_USBD	8
#define DCHAN_TODEV_NAND	9
#define DCHAN_TODEV_ATA		10


Thanks

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Jaswinder Singh]

On 29 July 2011 04:10, Russell King <rmk@arm.linux.org.uk> wrote:

>> > Board 3 has the MMCI connected through an external FPGA mux, which can route the
>> > MMCI requests to DMA request signals #1, #2 or #3.
>> Say
>>  Board3
>>      MMCI_RX  -> #{1,2,3}
>>      MMCI_TX  -> #{1,2,3}
>> And you can't change the route(mapping) after the dmac driver has
>> been loaded.
>
> No.  You have to change it dynamically at run time according to the
> DMA activity, because DMA request signals #1, #2 and #3 are shared
> between 6 devices.  To make matters worse, it's not six on any of
> RQ#1 RQ#2 RQ#3, but some on a couple, some on another couple, and
> some on all three.
>
> BTW, we do support this with Linus W's code (which he's posted to
> this thread.)

In case you missed my reply to these runtime switching cases,
please have a look at https://lkml.org/lkml/2011/7/29/211
Assuming you have read that, I write hence ...

A solution to Board-3 would work for Board-1,2 as well.
Rather this is how every dmac driver should be written, imho.

For a dmac driver, allocating a channel should be purely a s/w thing i.e, just
as a way for it to see what features the client needs and inform
immediately if it is impossible. Successful return of channel must not be
taken as a guarantee of any successful transfer by the clients.
Nothing esoteric, but important  to keep in mind nonetheless.

Since Board-3 has fpga-mux routing req-sigs, probably more number
of peripherals could be reached on board-3. Anyways, the dmac driver would
get every reachable h/w channel from board via platform and happily
allot to clients as if every channel is available all the time.

As a design, your dmac driver should lock as less h/w resources as possible
during channel allocation - ideally, zero, and it should be possible when
afterall the dmac-driver can't guarantee success of xfers.

 //Just before starting the xfers.
 if (plat->rs_activate)      // NULL for board-1 and 2
     err = plat->rs_activate(DMACd_RSs)
 else
     err = 0;

......do xfers .....

 //After xfers-done 'irq'
 if (plat->rs_deactivate)       // NULL for board-1 and 2
      plat->rs_deactivate(DMACd_RSs)

Depending upon time taken by rs_activate, you might want to schedule
rs_deactivate after sometime rather than calling it immediately.

This also indicates submitting xfers in batches, a good practice for clients.

All of the above is to prove that -- Runtime switching isn't as big a deal as
is made out to be. It should be done _transparently_ to the Client drivers.
So, shouldn't be a part of the DMAENGINE API.

So basically your setup should work just as fine as any other
statically allocated
req-signals. i.e, from the DMAENGINE API pov.

Thanks

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Russell King]

On Sat, Jul 30, 2011 at 06:39:13PM +0530, Jaswinder Singh wrote:
> On 29 July 2011 04:10, Russell King <rmk@arm.linux.org.uk> wrote:
> 
> >> > Board 3 has the MMCI connected through an external FPGA mux, which can route the
> >> > MMCI requests to DMA request signals #1, #2 or #3.
> >> Say
> >>  Board3
> >>      MMCI_RX  -> #{1,2,3}
> >>      MMCI_TX  -> #{1,2,3}
> >> And you can't change the route(mapping) after the dmac driver has
> >> been loaded.
> >
> > No.  You have to change it dynamically at run time according to the
> > DMA activity, because DMA request signals #1, #2 and #3 are shared
> > between 6 devices.  To make matters worse, it's not six on any of
> > RQ#1 RQ#2 RQ#3, but some on a couple, some on another couple, and
> > some on all three.
> >
> > BTW, we do support this with Linus W's code (which he's posted to
> > this thread.)
> 
> In case you missed my reply to these runtime switching cases,
> please have a look at https://lkml.org/lkml/2011/7/29/211

Stop pointing me at archives.  I'm not reading links during an email
discussion.  It's bad enough the mass of words that you include in
your replies to work out what you're saying.

And actually now, I'm completely lost over exactly what provides what
with your idea.

Could you please put together a _complete_ example of how you see my
board examples #1, #2 and #3 working, covering your _entire_ idea in
one _single_ _concise_ email with no references elsewhere, covering
in each case:

- how a peripheral device driver obtains, is allocated, or is provided with
  this 'capability' mask.
- how a DMA engine driver is told which 'capability' masks from peripheral
  drivers it is to match.
- how that is translated to a DMA device channels.
- how that translates to DMA request signals attached directly to the DMA
  device.
- how that translates to DMA request signals attached to some kind of MUX
  which in turn is attached to a DMA request signal attached to the DMA
  device.

Better still, do it in pseudo-C code, or real C code if you think that'll
help (there's nothing like seeing the actual code.)

You have for reference:
1. The AMBA drivers in the kernel tree (MMCI, PL011 UART).
	drivers/tty/serial/amba-pl011.c
	drivers/mmc/host/mmci.c
2. The DMA engine code for PL080 in the kernel tree.
	drivers/dma/amba-pl08x.c
3. Linus' example code for the Versatile/Realview platforms.
	(attached to his email.)

This is enough to cover the cases I outlined in boards #1,#2,#3.

To tie this down further, into real hardware, so lets say:
board #1: is Versatile PB926 with an on-board UART0 - so DMA controller
	request signal #14 for receive and DMA controller request signal
	#15 for transmit.

board #2: is Realview EB with an on-board UART1 - so DMA controller
	request signal #12 (RX) and #13 (TX).

board #3: is Versatile PB926 with the FPGA-implemented MMCI1 - so
	DMA controller request signals #0 to #2 inclusive may be used,
	via three separate MUXes (one per request signal) requiring
	the selected mux to be programmed with value '5'.  This same
	channel can be used for either DMA-to-device or DMA-from-device,
	but the MMCI device driver may request two separate 'dma channels'
	if they are available.

	To ensure all cases are covered, please assume that the AACI
	is also using DMA continuously for a long time in this case,
	which may be using any of #0 to #2 via the mux, and so how
	the AACI's in-use DMA device request signal is avoided.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCHv2] DMAEngine: Let dmac drivers to set chan_id

[Jaswinder Singh]

On 30 July 2011 19:52, Russell King <rmk@arm.linux.org.uk> wrote:
> On Sat, Jul 30, 2011 at 06:39:13PM +0530, Jaswinder Singh wrote:
>> On 29 July 2011 04:10, Russell King <rmk@arm.linux.org.uk> wrote:
>>
>> >> > Board 3 has the MMCI connected through an external FPGA mux, which can route the
>> >> > MMCI requests to DMA request signals #1, #2 or #3.
>> >> Say
>> >>  Board3
>> >>      MMCI_RX  -> #{1,2,3}
>> >>      MMCI_TX  -> #{1,2,3}
>> >> And you can't change the route(mapping) after the dmac driver has
>> >> been loaded.
>> >
>> > No.  You have to change it dynamically at run time according to the
>> > DMA activity, because DMA request signals #1, #2 and #3 are shared
>> > between 6 devices.  To make matters worse, it's not six on any of
>> > RQ#1 RQ#2 RQ#3, but some on a couple, some on another couple, and
>> > some on all three.
>> >
>> > BTW, we do support this with Linus W's code (which he's posted to
>> > this thread.)
>>
>> In case you missed my reply to these runtime switching cases,
>> please have a look at https://lkml.org/lkml/2011/7/29/211
>
> Stop pointing me at archives.  I'm not reading links during an email
> discussion.  It's bad enough the mass of words that you include in
> your replies to work out what you're saying.
>
> And actually now, I'm completely lost over exactly what provides what
> with your idea.
>
> Could you please put together a _complete_ example of how you see my
> board examples #1, #2 and #3 working, covering your _entire_ idea in
> one _single_ _concise_ email with no references elsewhere, covering
> in each case:
>
> - how a peripheral device driver obtains, is allocated, or is provided with
>  this 'capability' mask.
> - how a DMA engine driver is told which 'capability' masks from peripheral
>  drivers it is to match.
> - how that is translated to a DMA device channels.
> - how that translates to DMA request signals attached directly to the DMA
>  device.
> - how that translates to DMA request signals attached to some kind of MUX
>  which in turn is attached to a DMA request signal attached to the DMA
>  device.
>
> Better still, do it in pseudo-C code, or real C code if you think that'll
> help (there's nothing like seeing the actual code.)
>
> You have for reference:
> 1. The AMBA drivers in the kernel tree (MMCI, PL011 UART).
>        drivers/tty/serial/amba-pl011.c
>        drivers/mmc/host/mmci.c
> 2. The DMA engine code for PL080 in the kernel tree.
>        drivers/dma/amba-pl08x.c
> 3. Linus' example code for the Versatile/Realview platforms.
>        (attached to his email.)
>
> This is enough to cover the cases I outlined in boards #1,#2,#3.
>
> To tie this down further, into real hardware, so lets say:
> board #1: is Versatile PB926 with an on-board UART0 - so DMA controller
>        request signal #14 for receive and DMA controller request signal
>        #15 for transmit.
>
> board #2: is Realview EB with an on-board UART1 - so DMA controller
>        request signal #12 (RX) and #13 (TX).
>
> board #3: is Versatile PB926 with the FPGA-implemented MMCI1 - so
>        DMA controller request signals #0 to #2 inclusive may be used,
>        via three separate MUXes (one per request signal) requiring
>        the selected mux to be programmed with value '5'.  This same
>        channel can be used for either DMA-to-device or DMA-from-device,
>        but the MMCI device driver may request two separate 'dma channels'
>        if they are available.
>
>        To ensure all cases are covered, please assume that the AACI
>        is also using DMA continuously for a long time in this case,
>        which may be using any of #0 to #2 via the mux, and so how
>        the AACI's in-use DMA device request signal is avoided.
>
Would love to.
But only after Linaro Connect of 1 week.. and then out of time not officially
assigned to it. So it might be soon or late.