Re: [PATCH 2/3] ARM: SAMSUNG: Modify pl330 channel filter function

Re: [PATCH 2/3] ARM: SAMSUNG: Modify pl330 channel filter function

[Russell King - ARM Linux]

On Tue, Aug 23, 2011 at 03:29:44AM +0530, Thomas Abraham wrote:
> With the change in dma channels private data information, the pl330
> channel filter function is modified to look for a simple u8 value
> instead of the now unused 'struct dma_pl330_peri' value.
> 
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
>  arch/arm/plat-samsung/dma-ops.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
> index 6e3d9ab..4f1430b 100644
> --- a/arch/arm/plat-samsung/dma-ops.c
> +++ b/arch/arm/plat-samsung/dma-ops.c
> @@ -19,8 +19,8 @@
>  
>  static inline bool pl330_filter(struct dma_chan *chan, void *param)
>  {
> -	struct dma_pl330_peri *peri = chan->private;
> -	return peri->peri_id == (unsigned)param;
> +	u8 *peri_id = chan->private;
> +	return *peri_id == (unsigned)param;
>  }

As a word of warning - this will fail horribly if you have mixed DMA
engines in the system.  While we wait for a better scheme, we really
need to sort out these filter functions properly and stop poking
about in data which may not be what we expect.

One solution to that is to move pl330_filter() into drivers/dma/pl330.c
and have it check chan->device->dev->driver == &pl330_driver.driver
_before_ we start accessing chan->private.

That's not a comment against your patch - you're not really changing
the brokenness of this function.  It would be good to see a follow up
patch fixing it properly though.

[PATCH 2/3] ARM: SAMSUNG: Modify pl330 channel filter function

[Russell King - ARM Linux]

On Tue, Aug 23, 2011 at 03:29:44AM +0530, Thomas Abraham wrote:
> With the change in dma channels private data information, the pl330
> channel filter function is modified to look for a simple u8 value
> instead of the now unused 'struct dma_pl330_peri' value.
> 
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
>  arch/arm/plat-samsung/dma-ops.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
> index 6e3d9ab..4f1430b 100644
> --- a/arch/arm/plat-samsung/dma-ops.c
> +++ b/arch/arm/plat-samsung/dma-ops.c
> @@ -19,8 +19,8 @@
>  
>  static inline bool pl330_filter(struct dma_chan *chan, void *param)
>  {
> -	struct dma_pl330_peri *peri = chan->private;
> -	return peri->peri_id == (unsigned)param;
> +	u8 *peri_id = chan->private;
> +	return *peri_id == (unsigned)param;
>  }

As a word of warning - this will fail horribly if you have mixed DMA
engines in the system.  While we wait for a better scheme, we really
need to sort out these filter functions properly and stop poking
about in data which may not be what we expect.

One solution to that is to move pl330_filter() into drivers/dma/pl330.c
and have it check chan->device->dev->driver == &pl330_driver.driver
_before_ we start accessing chan->private.

That's not a comment against your patch - you're not really changing
the brokenness of this function.  It would be good to see a follow up
patch fixing it properly though.

[RESEND][PATCH 0/3] dma: pl330: Simplify platform data for pl330 driver

[Thomas Abraham]

The rqtype parameter specified in platform data as part of the
'struct dma_pl330_peri' can be infered from the direction of the transfer
specified with transfer requests. So the rqtype parameter can be removed
and 'struct dma_pl330_peri' is then left with only one field 'peri_id'
which itself can be moved into 'struct dma_pl330_platdata' and the
entire 'struct dma_pl330_peri' can be eliminated.

This is first step towards adding device tree support for pl330 dma driver.
With rqtype information inferred from transfer requests, it need not be
included in device tree to describe each hardware dma request.

This patchset is based on following tree.
http://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git
and branch 'kgene/next/topic-dma-pl330'.

This patchset has been implemented and tested only for Exynos4. If this
patchset is acceptable, then pl330 driver platform data for other samsung
platforms will also be modfied.

Thomas Abraham (3):
  DMA: PL330: Infer transfer direction from transfer request instead of platform data
  ARM: SAMSUNG: Modify pl330 channel filter function
  ARM: EXYNOS4: Modify platform data for pl330 driver

 arch/arm/mach-exynos4/dma.c     |  220 +++++++++++----------------------------
 arch/arm/plat-samsung/dma-ops.c |    4 +-
 drivers/dma/pl330.c             |   56 ++--------
 include/linux/amba/pl330.h      |   14 +--
 4 files changed, 75 insertions(+), 219 deletions(-)

[RESEND][PATCH 0/3] dma: pl330: Simplify platform data for pl330 driver

[Thomas Abraham]

The rqtype parameter specified in platform data as part of the
'struct dma_pl330_peri' can be infered from the direction of the transfer
specified with transfer requests. So the rqtype parameter can be removed
and 'struct dma_pl330_peri' is then left with only one field 'peri_id'
which itself can be moved into 'struct dma_pl330_platdata' and the
entire 'struct dma_pl330_peri' can be eliminated.

This is first step towards adding device tree support for pl330 dma driver.
With rqtype information inferred from transfer requests, it need not be
included in device tree to describe each hardware dma request.

This patchset is based on following tree.
http://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git
and branch 'kgene/next/topic-dma-pl330'.

This patchset has been implemented and tested only for Exynos4. If this
patchset is acceptable, then pl330 driver platform data for other samsung
platforms will also be modfied.

Thomas Abraham (3):
  DMA: PL330: Infer transfer direction from transfer request instead of platform data
  ARM: SAMSUNG: Modify pl330 channel filter function
  ARM: EXYNOS4: Modify platform data for pl330 driver

 arch/arm/mach-exynos4/dma.c     |  220 +++++++++++----------------------------
 arch/arm/plat-samsung/dma-ops.c |    4 +-
 drivers/dma/pl330.c             |   56 ++--------
 include/linux/amba/pl330.h      |   14 +--
 4 files changed, 75 insertions(+), 219 deletions(-)

[PATCH 1/3] DMA: PL330: Infer transfer direction from transfer request instead of platform data

[Thomas Abraham]

The transfer direction for a channel can be inferred from the transfer
request and the need for specifying transfer direction in platfrom data
can be eliminated. So the structure definition 'struct dma_pl330_peri'
is no longer required.

With the 'struct dma_pl330_peri' removed, the dma controller transfer
capabilities cannot be inferred any longer. Hence, the dma controller
capabilities is specified using platforme data.

Cc: Jassi Brar <jassisinghbrar@gmail.com>
Cc: Boojin Kim <boojin.kim@samsung.com>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 drivers/dma/pl330.c        |   56 ++++++++------------------------------------
 include/linux/amba/pl330.h |   14 +++--------
 2 files changed, 14 insertions(+), 56 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 3a0baac..6592b9a 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -507,7 +507,7 @@ pluck_desc(struct dma_pl330_dmac *pdmac)
 static struct dma_pl330_desc *pl330_get_desc(struct dma_pl330_chan *pch)
 {
 	struct dma_pl330_dmac *pdmac = pch->dmac;
-	struct dma_pl330_peri *peri = pch->chan.private;
+	u8 *peri_id = pch->chan.private;
 	struct dma_pl330_desc *desc;
 
 	/* Pluck one desc from the pool of DMAC */
@@ -531,14 +531,7 @@ static struct dma_pl330_desc *pl330_get_desc(struct dma_pl330_chan *pch)
 	desc->pchan = pch;
 	desc->txd.cookie = 0;
 	async_tx_ack(&desc->txd);
-
-	if (peri) {
-		desc->req.rqtype = peri->rqtype;
-		desc->req.peri = pch->chan.chan_id;
-	} else {
-		desc->req.rqtype = MEMTOMEM;
-		desc->req.peri = 0;
-	}
+	desc->req.peri = (peri_id) ? pch->chan.chan_id : 0;
 
 	dma_async_tx_descriptor_init(&desc->txd, &pch->chan);
 
@@ -625,12 +618,14 @@ static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic(
 	case DMA_TO_DEVICE:
 		desc->rqcfg.src_inc = 1;
 		desc->rqcfg.dst_inc = 0;
+		desc->req.rqtype = MEMTODEV;
 		src = dma_addr;
 		dst = pch->fifo_addr;
 		break;
 	case DMA_FROM_DEVICE:
 		desc->rqcfg.src_inc = 0;
 		desc->rqcfg.dst_inc = 1;
+		desc->req.rqtype = DEVTOMEM;
 		src = pch->fifo_addr;
 		dst = dma_addr;
 		break;
@@ -657,16 +652,12 @@ pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,
 {
 	struct dma_pl330_desc *desc;
 	struct dma_pl330_chan *pch = to_pchan(chan);
-	struct dma_pl330_peri *peri = chan->private;
 	struct pl330_info *pi;
 	int burst;
 
 	if (unlikely(!pch || !len))
 		return NULL;
 
-	if (peri && peri->rqtype != MEMTOMEM)
-		return NULL;
-
 	pi = &pch->dmac->pif;
 
 	desc = __pl330_prep_dma_memcpy(pch, dst, src, len);
@@ -675,6 +666,7 @@ pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,
 
 	desc->rqcfg.src_inc = 1;
 	desc->rqcfg.dst_inc = 1;
+	desc->req.rqtype = MEMTOMEM;
 
 	/* Select max possible burst size */
 	burst = pi->pcfg.data_bus_width / 8;
@@ -703,25 +695,14 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 {
 	struct dma_pl330_desc *first, *desc = NULL;
 	struct dma_pl330_chan *pch = to_pchan(chan);
-	struct dma_pl330_peri *peri = chan->private;
 	struct scatterlist *sg;
 	unsigned long flags;
 	int i;
 	dma_addr_t addr;
 
-	if (unlikely(!pch || !sgl || !sg_len || !peri))
+	if (unlikely(!pch || !sgl || !sg_len))
 		return NULL;
 
-	/* Make sure the direction is consistent */
-	if ((direction == DMA_TO_DEVICE &&
-				peri->rqtype != MEMTODEV) ||
-			(direction == DMA_FROM_DEVICE &&
-				peri->rqtype != DEVTOMEM)) {
-		dev_err(pch->dmac->pif.dev, "%s:%d Invalid Direction\n",
-				__func__, __LINE__);
-		return NULL;
-	}
-
 	addr = pch->fifo_addr;
 
 	first = NULL;
@@ -761,11 +742,13 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		if (direction == DMA_TO_DEVICE) {
 			desc->rqcfg.src_inc = 1;
 			desc->rqcfg.dst_inc = 0;
+			desc->req.rqtype = MEMTODEV;
 			fill_px(&desc->px,
 				addr, sg_dma_address(sg), sg_dma_len(sg));
 		} else {
 			desc->rqcfg.src_inc = 0;
 			desc->rqcfg.dst_inc = 1;
+			desc->req.rqtype = DEVTOMEM;
 			fill_px(&desc->px,
 				sg_dma_address(sg), addr, sg_dma_len(sg));
 		}
@@ -872,27 +855,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 
 	for (i = 0; i < num_chan; i++) {
 		pch = &pdmac->peripherals[i];
-		if (pdat) {
-			struct dma_pl330_peri *peri = &pdat->peri[i];
-
-			switch (peri->rqtype) {
-			case MEMTOMEM:
-				dma_cap_set(DMA_MEMCPY, pd->cap_mask);
-				break;
-			case MEMTODEV:
-			case DEVTOMEM:
-				dma_cap_set(DMA_SLAVE, pd->cap_mask);
-				dma_cap_set(DMA_CYCLIC, pd->cap_mask);
-				break;
-			default:
-				dev_err(&adev->dev, "DEVTODEV Not Supported\n");
-				continue;
-			}
-			pch->chan.private = peri;
-		} else {
-			dma_cap_set(DMA_MEMCPY, pd->cap_mask);
-			pch->chan.private = NULL;
-		}
+		pch->chan.private = (pdat) ? &pdat->peri_id[i] : NULL;
 
 		INIT_LIST_HEAD(&pch->work_list);
 		spin_lock_init(&pch->lock);
@@ -907,6 +870,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 	}
 
 	pd->dev = &adev->dev;
+	pd->cap_mask = pdat->cap_mask;
 
 	pd->device_alloc_chan_resources = pl330_alloc_chan_resources;
 	pd->device_free_chan_resources = pl330_free_chan_resources;
diff --git a/include/linux/amba/pl330.h b/include/linux/amba/pl330.h
index d12f077..0b87300 100644
--- a/include/linux/amba/pl330.h
+++ b/include/linux/amba/pl330.h
@@ -13,15 +13,7 @@
 #define	__AMBA_PL330_H_
 
 #include <asm/hardware/pl330.h>
-
-struct dma_pl330_peri {
-	/*
-	 * Peri_Req i/f of the DMAC that is
-	 * peripheral could be reached from.
-	 */
-	u8 peri_id; /* specific dma id */
-	enum pl330_reqtype rqtype;
-};
+#include <linux/dmaengine.h>
 
 struct dma_pl330_platdata {
 	/*
@@ -33,7 +25,9 @@ struct dma_pl330_platdata {
 	 */
 	u8 nr_valid_peri;
 	/* Array of valid peripherals */
-	struct dma_pl330_peri *peri;
+	u8 *peri_id;
+	/* Operational capabilities */
+	dma_cap_mask_t cap_mask;
 	/* Bytes to allocate for MC buffer */
 	unsigned mcbuf_sz;
 };
-- 
1.6.6.rc2

[PATCH 1/3] DMA: PL330: Infer transfer direction from transfer request instead of platform data

[Thomas Abraham]

The transfer direction for a channel can be inferred from the transfer
request and the need for specifying transfer direction in platfrom data
can be eliminated. So the structure definition 'struct dma_pl330_peri'
is no longer required.

With the 'struct dma_pl330_peri' removed, the dma controller transfer
capabilities cannot be inferred any longer. Hence, the dma controller
capabilities is specified using platforme data.

Cc: Jassi Brar <jassisinghbrar@gmail.com>
Cc: Boojin Kim <boojin.kim@samsung.com>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 drivers/dma/pl330.c        |   56 ++++++++------------------------------------
 include/linux/amba/pl330.h |   14 +++--------
 2 files changed, 14 insertions(+), 56 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 3a0baac..6592b9a 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -507,7 +507,7 @@ pluck_desc(struct dma_pl330_dmac *pdmac)
 static struct dma_pl330_desc *pl330_get_desc(struct dma_pl330_chan *pch)
 {
 	struct dma_pl330_dmac *pdmac = pch->dmac;
-	struct dma_pl330_peri *peri = pch->chan.private;
+	u8 *peri_id = pch->chan.private;
 	struct dma_pl330_desc *desc;
 
 	/* Pluck one desc from the pool of DMAC */
@@ -531,14 +531,7 @@ static struct dma_pl330_desc *pl330_get_desc(struct dma_pl330_chan *pch)
 	desc->pchan = pch;
 	desc->txd.cookie = 0;
 	async_tx_ack(&desc->txd);
-
-	if (peri) {
-		desc->req.rqtype = peri->rqtype;
-		desc->req.peri = pch->chan.chan_id;
-	} else {
-		desc->req.rqtype = MEMTOMEM;
-		desc->req.peri = 0;
-	}
+	desc->req.peri = (peri_id) ? pch->chan.chan_id : 0;
 
 	dma_async_tx_descriptor_init(&desc->txd, &pch->chan);
 
@@ -625,12 +618,14 @@ static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic(
 	case DMA_TO_DEVICE:
 		desc->rqcfg.src_inc = 1;
 		desc->rqcfg.dst_inc = 0;
+		desc->req.rqtype = MEMTODEV;
 		src = dma_addr;
 		dst = pch->fifo_addr;
 		break;
 	case DMA_FROM_DEVICE:
 		desc->rqcfg.src_inc = 0;
 		desc->rqcfg.dst_inc = 1;
+		desc->req.rqtype = DEVTOMEM;
 		src = pch->fifo_addr;
 		dst = dma_addr;
 		break;
@@ -657,16 +652,12 @@ pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,
 {
 	struct dma_pl330_desc *desc;
 	struct dma_pl330_chan *pch = to_pchan(chan);
-	struct dma_pl330_peri *peri = chan->private;
 	struct pl330_info *pi;
 	int burst;
 
 	if (unlikely(!pch || !len))
 		return NULL;
 
-	if (peri && peri->rqtype != MEMTOMEM)
-		return NULL;
-
 	pi = &pch->dmac->pif;
 
 	desc = __pl330_prep_dma_memcpy(pch, dst, src, len);
@@ -675,6 +666,7 @@ pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,
 
 	desc->rqcfg.src_inc = 1;
 	desc->rqcfg.dst_inc = 1;
+	desc->req.rqtype = MEMTOMEM;
 
 	/* Select max possible burst size */
 	burst = pi->pcfg.data_bus_width / 8;
@@ -703,25 +695,14 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 {
 	struct dma_pl330_desc *first, *desc = NULL;
 	struct dma_pl330_chan *pch = to_pchan(chan);
-	struct dma_pl330_peri *peri = chan->private;
 	struct scatterlist *sg;
 	unsigned long flags;
 	int i;
 	dma_addr_t addr;
 
-	if (unlikely(!pch || !sgl || !sg_len || !peri))
+	if (unlikely(!pch || !sgl || !sg_len))
 		return NULL;
 
-	/* Make sure the direction is consistent */
-	if ((direction == DMA_TO_DEVICE &&
-				peri->rqtype != MEMTODEV) ||
-			(direction == DMA_FROM_DEVICE &&
-				peri->rqtype != DEVTOMEM)) {
-		dev_err(pch->dmac->pif.dev, "%s:%d Invalid Direction\n",
-				__func__, __LINE__);
-		return NULL;
-	}
-
 	addr = pch->fifo_addr;
 
 	first = NULL;
@@ -761,11 +742,13 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		if (direction == DMA_TO_DEVICE) {
 			desc->rqcfg.src_inc = 1;
 			desc->rqcfg.dst_inc = 0;
+			desc->req.rqtype = MEMTODEV;
 			fill_px(&desc->px,
 				addr, sg_dma_address(sg), sg_dma_len(sg));
 		} else {
 			desc->rqcfg.src_inc = 0;
 			desc->rqcfg.dst_inc = 1;
+			desc->req.rqtype = DEVTOMEM;
 			fill_px(&desc->px,
 				sg_dma_address(sg), addr, sg_dma_len(sg));
 		}
@@ -872,27 +855,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 
 	for (i = 0; i < num_chan; i++) {
 		pch = &pdmac->peripherals[i];
-		if (pdat) {
-			struct dma_pl330_peri *peri = &pdat->peri[i];
-
-			switch (peri->rqtype) {
-			case MEMTOMEM:
-				dma_cap_set(DMA_MEMCPY, pd->cap_mask);
-				break;
-			case MEMTODEV:
-			case DEVTOMEM:
-				dma_cap_set(DMA_SLAVE, pd->cap_mask);
-				dma_cap_set(DMA_CYCLIC, pd->cap_mask);
-				break;
-			default:
-				dev_err(&adev->dev, "DEVTODEV Not Supported\n");
-				continue;
-			}
-			pch->chan.private = peri;
-		} else {
-			dma_cap_set(DMA_MEMCPY, pd->cap_mask);
-			pch->chan.private = NULL;
-		}
+		pch->chan.private = (pdat) ? &pdat->peri_id[i] : NULL;
 
 		INIT_LIST_HEAD(&pch->work_list);
 		spin_lock_init(&pch->lock);
@@ -907,6 +870,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 	}
 
 	pd->dev = &adev->dev;
+	pd->cap_mask = pdat->cap_mask;
 
 	pd->device_alloc_chan_resources = pl330_alloc_chan_resources;
 	pd->device_free_chan_resources = pl330_free_chan_resources;
diff --git a/include/linux/amba/pl330.h b/include/linux/amba/pl330.h
index d12f077..0b87300 100644
--- a/include/linux/amba/pl330.h
+++ b/include/linux/amba/pl330.h
@@ -13,15 +13,7 @@
 #define	__AMBA_PL330_H_
 
 #include <asm/hardware/pl330.h>
-
-struct dma_pl330_peri {
-	/*
-	 * Peri_Req i/f of the DMAC that is
-	 * peripheral could be reached from.
-	 */
-	u8 peri_id; /* specific dma id */
-	enum pl330_reqtype rqtype;
-};
+#include <linux/dmaengine.h>
 
 struct dma_pl330_platdata {
 	/*
@@ -33,7 +25,9 @@ struct dma_pl330_platdata {
 	 */
 	u8 nr_valid_peri;
 	/* Array of valid peripherals */
-	struct dma_pl330_peri *peri;
+	u8 *peri_id;
+	/* Operational capabilities */
+	dma_cap_mask_t cap_mask;
 	/* Bytes to allocate for MC buffer */
 	unsigned mcbuf_sz;
 };
-- 
1.6.6.rc2

[PATCH 2/3] ARM: SAMSUNG: Modify pl330 channel filter function

[Thomas Abraham]

With the change in dma channels private data information, the pl330
channel filter function is modified to look for a simple u8 value
instead of the now unused 'struct dma_pl330_peri' value.

Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 arch/arm/plat-samsung/dma-ops.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
index 6e3d9ab..4f1430b 100644
--- a/arch/arm/plat-samsung/dma-ops.c
+++ b/arch/arm/plat-samsung/dma-ops.c
@@ -19,8 +19,8 @@
 
 static inline bool pl330_filter(struct dma_chan *chan, void *param)
 {
-	struct dma_pl330_peri *peri = chan->private;
-	return peri->peri_id == (unsigned)param;
+	u8 *peri_id = chan->private;
+	return *peri_id == (unsigned)param;
 }
 
 static unsigned samsung_dmadev_request(enum dma_ch dma_ch,
-- 
1.6.6.rc2

[PATCH 2/3] ARM: SAMSUNG: Modify pl330 channel filter function

[Thomas Abraham]

With the change in dma channels private data information, the pl330
channel filter function is modified to look for a simple u8 value
instead of the now unused 'struct dma_pl330_peri' value.

Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 arch/arm/plat-samsung/dma-ops.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
index 6e3d9ab..4f1430b 100644
--- a/arch/arm/plat-samsung/dma-ops.c
+++ b/arch/arm/plat-samsung/dma-ops.c
@@ -19,8 +19,8 @@
 
 static inline bool pl330_filter(struct dma_chan *chan, void *param)
 {
-	struct dma_pl330_peri *peri = chan->private;
-	return peri->peri_id == (unsigned)param;
+	u8 *peri_id = chan->private;
+	return *peri_id == (unsigned)param;
 }
 
 static unsigned samsung_dmadev_request(enum dma_ch dma_ch,
-- 
1.6.6.rc2

[PATCH 3/3] ARM: EXYNOS4: Modify platform data for pl330 driver

[Thomas Abraham]

With the 'struct dma_pl330_peri' removed, the platfrom data for dma
driver can be simplified to a simple list of peripheral request ids.

Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 arch/arm/mach-exynos4/dma.c |  220 ++++++++++++-------------------------------
 1 files changed, 59 insertions(+), 161 deletions(-)

diff --git a/arch/arm/mach-exynos4/dma.c b/arch/arm/mach-exynos4/dma.c
index d57d662..f905114 100644
--- a/arch/arm/mach-exynos4/dma.c
+++ b/arch/arm/mach-exynos4/dma.c
@@ -35,95 +35,40 @@
 
 static u64 dma_dmamask = DMA_BIT_MASK(32);
 
-struct dma_pl330_peri pdma0_peri[28] = {
-	{
-		.peri_id = (u8)DMACH_PCM0_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_PCM0_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_PCM2_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_PCM2_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_MSM_REQ0,
-	}, {
-		.peri_id = (u8)DMACH_MSM_REQ2,
-	}, {
-		.peri_id = (u8)DMACH_SPI0_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_SPI0_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_SPI2_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_SPI2_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_I2S0S_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_I2S0_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_I2S0_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_UART0_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_UART0_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_UART2_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_UART2_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_UART4_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_UART4_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS0_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS0_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS2_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS2_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS4_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS4_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_AC97_MICIN,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_AC97_PCMIN,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_AC97_PCMOUT,
-		.rqtype = MEMTODEV,
-	},
+u8 pdma0_peri[] = {
+	DMACH_PCM0_RX,
+	DMACH_PCM0_TX,
+	DMACH_PCM2_RX,
+	DMACH_PCM2_TX,
+	DMACH_MSM_REQ0,
+	DMACH_MSM_REQ2,
+	DMACH_SPI0_RX,
+	DMACH_SPI0_TX,
+	DMACH_SPI2_RX,
+	DMACH_SPI2_TX,
+	DMACH_I2S0S_TX,
+	DMACH_I2S0_RX,
+	DMACH_I2S0_TX,
+	DMACH_UART0_RX,
+	DMACH_UART0_TX,
+	DMACH_UART2_RX,
+	DMACH_UART2_TX,
+	DMACH_UART4_RX,
+	DMACH_UART4_TX,
+	DMACH_SLIMBUS0_RX,
+	DMACH_SLIMBUS0_TX,
+	DMACH_SLIMBUS2_RX,
+	DMACH_SLIMBUS2_TX,
+	DMACH_SLIMBUS4_RX,
+	DMACH_SLIMBUS4_TX,
+	DMACH_AC97_MICIN,
+	DMACH_AC97_PCMIN,
+	DMACH_AC97_PCMOUT,
 };
 
 struct dma_pl330_platdata exynos4_pdma0_pdata = {
 	.nr_valid_peri = ARRAY_SIZE(pdma0_peri),
-	.peri = pdma0_peri,
+	.peri_id = pdma0_peri,
 };
 
 struct amba_device exynos4_device_pdma0 = {
@@ -142,86 +87,37 @@ struct amba_device exynos4_device_pdma0 = {
 	.periphid = 0x00041330,
 };
 
-struct dma_pl330_peri pdma1_peri[25] = {
-	{
-		.peri_id = (u8)DMACH_PCM0_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_PCM0_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_PCM1_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_PCM1_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_MSM_REQ1,
-	}, {
-		.peri_id = (u8)DMACH_MSM_REQ3,
-	}, {
-		.peri_id = (u8)DMACH_SPI1_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_SPI1_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_I2S0S_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_I2S0_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_I2S0_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_I2S1_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_I2S1_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_UART0_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_UART0_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_UART1_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_UART1_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_UART3_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_UART3_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS1_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS1_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS3_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS3_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS5_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS5_TX,
-		.rqtype = MEMTODEV,
-	},
+u8 pdma1_peri[] = {
+	DMACH_PCM0_RX,
+	DMACH_PCM0_TX,
+	DMACH_PCM1_RX,
+	DMACH_PCM1_TX,
+	DMACH_MSM_REQ1,
+	DMACH_MSM_REQ3,
+	DMACH_SPI1_RX,
+	DMACH_SPI1_TX,
+	DMACH_I2S0S_TX,
+	DMACH_I2S0_RX,
+	DMACH_I2S0_TX,
+	DMACH_I2S1_RX,
+	DMACH_I2S1_TX,
+	DMACH_UART0_RX,
+	DMACH_UART0_TX,
+	DMACH_UART1_RX,
+	DMACH_UART1_TX,
+	DMACH_UART3_RX,
+	DMACH_UART3_TX,
+	DMACH_SLIMBUS1_RX,
+	DMACH_SLIMBUS1_TX,
+	DMACH_SLIMBUS3_RX,
+	DMACH_SLIMBUS3_TX,
+	DMACH_SLIMBUS5_RX,
+	DMACH_SLIMBUS5_TX,
 };
 
 struct dma_pl330_platdata exynos4_pdma1_pdata = {
 	.nr_valid_peri = ARRAY_SIZE(pdma1_peri),
-	.peri = pdma1_peri,
+	.peri_id = pdma1_peri,
 };
 
 struct amba_device exynos4_device_pdma1 = {
@@ -242,6 +138,8 @@ struct amba_device exynos4_device_pdma1 = {
 
 static int __init exynos4_dma_init(void)
 {
+	dma_cap_set(DMA_SLAVE, exynos4_pdma0_pdata.cap_mask);
+	dma_cap_set(DMA_CYCLIC, exynos4_pdma0_pdata.cap_mask);
 	amba_device_register(&exynos4_device_pdma0, &iomem_resource);
 
 	return 0;
-- 
1.6.6.rc2

[PATCH 3/3] ARM: EXYNOS4: Modify platform data for pl330 driver

[Thomas Abraham]

With the 'struct dma_pl330_peri' removed, the platfrom data for dma
driver can be simplified to a simple list of peripheral request ids.

Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 arch/arm/mach-exynos4/dma.c |  220 ++++++++++++-------------------------------
 1 files changed, 59 insertions(+), 161 deletions(-)

diff --git a/arch/arm/mach-exynos4/dma.c b/arch/arm/mach-exynos4/dma.c
index d57d662..f905114 100644
--- a/arch/arm/mach-exynos4/dma.c
+++ b/arch/arm/mach-exynos4/dma.c
@@ -35,95 +35,40 @@
 
 static u64 dma_dmamask = DMA_BIT_MASK(32);
 
-struct dma_pl330_peri pdma0_peri[28] = {
-	{
-		.peri_id = (u8)DMACH_PCM0_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_PCM0_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_PCM2_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_PCM2_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_MSM_REQ0,
-	}, {
-		.peri_id = (u8)DMACH_MSM_REQ2,
-	}, {
-		.peri_id = (u8)DMACH_SPI0_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_SPI0_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_SPI2_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_SPI2_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_I2S0S_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_I2S0_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_I2S0_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_UART0_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_UART0_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_UART2_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_UART2_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_UART4_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_UART4_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS0_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS0_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS2_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS2_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS4_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS4_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_AC97_MICIN,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_AC97_PCMIN,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_AC97_PCMOUT,
-		.rqtype = MEMTODEV,
-	},
+u8 pdma0_peri[] = {
+	DMACH_PCM0_RX,
+	DMACH_PCM0_TX,
+	DMACH_PCM2_RX,
+	DMACH_PCM2_TX,
+	DMACH_MSM_REQ0,
+	DMACH_MSM_REQ2,
+	DMACH_SPI0_RX,
+	DMACH_SPI0_TX,
+	DMACH_SPI2_RX,
+	DMACH_SPI2_TX,
+	DMACH_I2S0S_TX,
+	DMACH_I2S0_RX,
+	DMACH_I2S0_TX,
+	DMACH_UART0_RX,
+	DMACH_UART0_TX,
+	DMACH_UART2_RX,
+	DMACH_UART2_TX,
+	DMACH_UART4_RX,
+	DMACH_UART4_TX,
+	DMACH_SLIMBUS0_RX,
+	DMACH_SLIMBUS0_TX,
+	DMACH_SLIMBUS2_RX,
+	DMACH_SLIMBUS2_TX,
+	DMACH_SLIMBUS4_RX,
+	DMACH_SLIMBUS4_TX,
+	DMACH_AC97_MICIN,
+	DMACH_AC97_PCMIN,
+	DMACH_AC97_PCMOUT,
 };
 
 struct dma_pl330_platdata exynos4_pdma0_pdata = {
 	.nr_valid_peri = ARRAY_SIZE(pdma0_peri),
-	.peri = pdma0_peri,
+	.peri_id = pdma0_peri,
 };
 
 struct amba_device exynos4_device_pdma0 = {
@@ -142,86 +87,37 @@ struct amba_device exynos4_device_pdma0 = {
 	.periphid = 0x00041330,
 };
 
-struct dma_pl330_peri pdma1_peri[25] = {
-	{
-		.peri_id = (u8)DMACH_PCM0_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_PCM0_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_PCM1_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_PCM1_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_MSM_REQ1,
-	}, {
-		.peri_id = (u8)DMACH_MSM_REQ3,
-	}, {
-		.peri_id = (u8)DMACH_SPI1_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_SPI1_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_I2S0S_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_I2S0_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_I2S0_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_I2S1_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_I2S1_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_UART0_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_UART0_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_UART1_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_UART1_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_UART3_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_UART3_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS1_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS1_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS3_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS3_TX,
-		.rqtype = MEMTODEV,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS5_RX,
-		.rqtype = DEVTOMEM,
-	}, {
-		.peri_id = (u8)DMACH_SLIMBUS5_TX,
-		.rqtype = MEMTODEV,
-	},
+u8 pdma1_peri[] = {
+	DMACH_PCM0_RX,
+	DMACH_PCM0_TX,
+	DMACH_PCM1_RX,
+	DMACH_PCM1_TX,
+	DMACH_MSM_REQ1,
+	DMACH_MSM_REQ3,
+	DMACH_SPI1_RX,
+	DMACH_SPI1_TX,
+	DMACH_I2S0S_TX,
+	DMACH_I2S0_RX,
+	DMACH_I2S0_TX,
+	DMACH_I2S1_RX,
+	DMACH_I2S1_TX,
+	DMACH_UART0_RX,
+	DMACH_UART0_TX,
+	DMACH_UART1_RX,
+	DMACH_UART1_TX,
+	DMACH_UART3_RX,
+	DMACH_UART3_TX,
+	DMACH_SLIMBUS1_RX,
+	DMACH_SLIMBUS1_TX,
+	DMACH_SLIMBUS3_RX,
+	DMACH_SLIMBUS3_TX,
+	DMACH_SLIMBUS5_RX,
+	DMACH_SLIMBUS5_TX,
 };
 
 struct dma_pl330_platdata exynos4_pdma1_pdata = {
 	.nr_valid_peri = ARRAY_SIZE(pdma1_peri),
-	.peri = pdma1_peri,
+	.peri_id = pdma1_peri,
 };
 
 struct amba_device exynos4_device_pdma1 = {
@@ -242,6 +138,8 @@ struct amba_device exynos4_device_pdma1 = {
 
 static int __init exynos4_dma_init(void)
 {
+	dma_cap_set(DMA_SLAVE, exynos4_pdma0_pdata.cap_mask);
+	dma_cap_set(DMA_CYCLIC, exynos4_pdma0_pdata.cap_mask);
 	amba_device_register(&exynos4_device_pdma0, &iomem_resource);
 
 	return 0;
-- 
1.6.6.rc2

Re: [PATCH 2/3] ARM: SAMSUNG: Modify pl330 channel filter function

[Thomas Abraham]

Hi Russell,

On 23 August 2011 03:22, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Aug 23, 2011 at 03:29:44AM +0530, Thomas Abraham wrote:
>> With the change in dma channels private data information, the pl330
>> channel filter function is modified to look for a simple u8 value
>> instead of the now unused 'struct dma_pl330_peri' value.
>>
>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> ---
>>  arch/arm/plat-samsung/dma-ops.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
>> index 6e3d9ab..4f1430b 100644
>> --- a/arch/arm/plat-samsung/dma-ops.c
>> +++ b/arch/arm/plat-samsung/dma-ops.c
>> @@ -19,8 +19,8 @@
>>
>>  static inline bool pl330_filter(struct dma_chan *chan, void *param)
>>  {
>> -     struct dma_pl330_peri *peri = chan->private;
>> -     return peri->peri_id == (unsigned)param;
>> +     u8 *peri_id = chan->private;
>> +     return *peri_id == (unsigned)param;
>>  }
>
> As a word of warning - this will fail horribly if you have mixed DMA
> engines in the system.  While we wait for a better scheme, we really
> need to sort out these filter functions properly and stop poking
> about in data which may not be what we expect.
>
> One solution to that is to move pl330_filter() into drivers/dma/pl330.c
> and have it check chan->device->dev->driver == &pl330_driver.driver
> _before_ we start accessing chan->private.
>
> That's not a comment against your patch - you're not really changing
> the brokenness of this function.  It would be good to see a follow up
> patch fixing it properly though.
>

Ok. I will prepare a patch to move the pl330_filter function into
drivers/dma/pl330.c and also add the check that you have proposed.

Thanks for your help.

Regards,
Thomas.

[PATCH 2/3] ARM: SAMSUNG: Modify pl330 channel filter function

[Thomas Abraham]

Hi Russell,

On 23 August 2011 03:22, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Aug 23, 2011 at 03:29:44AM +0530, Thomas Abraham wrote:
>> With the change in dma channels private data information, the pl330
>> channel filter function is modified to look for a simple u8 value
>> instead of the now unused 'struct dma_pl330_peri' value.
>>
>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> ---
>> ?arch/arm/plat-samsung/dma-ops.c | ? ?4 ++--
>> ?1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
>> index 6e3d9ab..4f1430b 100644
>> --- a/arch/arm/plat-samsung/dma-ops.c
>> +++ b/arch/arm/plat-samsung/dma-ops.c
>> @@ -19,8 +19,8 @@
>>
>> ?static inline bool pl330_filter(struct dma_chan *chan, void *param)
>> ?{
>> - ? ? struct dma_pl330_peri *peri = chan->private;
>> - ? ? return peri->peri_id == (unsigned)param;
>> + ? ? u8 *peri_id = chan->private;
>> + ? ? return *peri_id == (unsigned)param;
>> ?}
>
> As a word of warning - this will fail horribly if you have mixed DMA
> engines in the system. ?While we wait for a better scheme, we really
> need to sort out these filter functions properly and stop poking
> about in data which may not be what we expect.
>
> One solution to that is to move pl330_filter() into drivers/dma/pl330.c
> and have it check chan->device->dev->driver == &pl330_driver.driver
> _before_ we start accessing chan->private.
>
> That's not a comment against your patch - you're not really changing
> the brokenness of this function. ?It would be good to see a follow up
> patch fixing it properly though.
>

Ok. I will prepare a patch to move the pl330_filter function into
drivers/dma/pl330.c and also add the check that you have proposed.

Thanks for your help.

Regards,
Thomas.

RE: [PATCH 1/3] DMA: PL330: Infer transfer direction from transfer request instead of platform data

[Boojin Kim]

Thomas Abraham wrote:
> Sent: Tuesday, August 23, 2011 7:00 AM
> To: linux-arm-kernel@lists.infradead.org
> Cc: linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
> vinod.koul@intel.com; Jassi Brar; Boojin Kim
> Subject: [PATCH 1/3] DMA: PL330: Infer transfer direction from
> transfer request instead of platform data
>
> The transfer direction for a channel can be inferred from the transfer
> request and the need for specifying transfer direction in platfrom
> data
> can be eliminated. So the structure definition 'struct dma_pl330_peri'
> is no longer required.
>
> With the 'struct dma_pl330_peri' removed, the dma controller transfer
> capabilities cannot be inferred any longer. Hence, the dma controller
> capabilities is specified using platforme data.
>
> Cc: Jassi Brar <jassisinghbrar@gmail.com>
> Cc: Boojin Kim <boojin.kim@samsung.com>
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
>  drivers/dma/pl330.c        |   56 ++++++++------------------------------
> ------
>  include/linux/amba/pl330.h |   14 +++--------
>  2 files changed, 14 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 3a0baac..6592b9a 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -507,7 +507,7 @@ pluck_desc(struct dma_pl330_dmac *pdmac)
>  static struct dma_pl330_desc *pl330_get_desc(struct dma_pl330_chan
> *pch)
>  {
>  	struct dma_pl330_dmac *pdmac = pch->dmac;
> -	struct dma_pl330_peri *peri = pch->chan.private;
> +	u8 *peri_id = pch->chan.private;
>  	struct dma_pl330_desc *desc;
>
>  	/* Pluck one desc from the pool of DMAC */
> @@ -531,14 +531,7 @@ static struct dma_pl330_desc
> *pl330_get_desc(struct dma_pl330_chan *pch)
>  	desc->pchan = pch;
>  	desc->txd.cookie = 0;
>  	async_tx_ack(&desc->txd);
> -
> -	if (peri) {
> -		desc->req.rqtype = peri->rqtype;
> -		desc->req.peri = pch->chan.chan_id;
> -	} else {
> -		desc->req.rqtype = MEMTOMEM;
> -		desc->req.peri = 0;
> -	}
> +	desc->req.peri = (peri_id) ? pch->chan.chan_id : 0;
>
>  	dma_async_tx_descriptor_init(&desc->txd, &pch->chan);
>
> @@ -625,12 +618,14 @@ static struct dma_async_tx_descriptor
> *pl330_prep_dma_cyclic(
>  	case DMA_TO_DEVICE:
>  		desc->rqcfg.src_inc = 1;
>  		desc->rqcfg.dst_inc = 0;
> +		desc->req.rqtype = MEMTODEV;
>  		src = dma_addr;
>  		dst = pch->fifo_addr;
>  		break;
>  	case DMA_FROM_DEVICE:
>  		desc->rqcfg.src_inc = 0;
>  		desc->rqcfg.dst_inc = 1;
> +		desc->req.rqtype = DEVTOMEM;
>  		src = pch->fifo_addr;
>  		dst = dma_addr;
>  		break;
> @@ -657,16 +652,12 @@ pl330_prep_dma_memcpy(struct dma_chan *chan,
> dma_addr_t dst,
>  {
>  	struct dma_pl330_desc *desc;
>  	struct dma_pl330_chan *pch = to_pchan(chan);
> -	struct dma_pl330_peri *peri = chan->private;
>  	struct pl330_info *pi;
>  	int burst;
>
>  	if (unlikely(!pch || !len))
>  		return NULL;
>
> -	if (peri && peri->rqtype != MEMTOMEM)
> -		return NULL;
> -
>  	pi = &pch->dmac->pif;
>
>  	desc = __pl330_prep_dma_memcpy(pch, dst, src, len);
> @@ -675,6 +666,7 @@ pl330_prep_dma_memcpy(struct dma_chan *chan,
> dma_addr_t dst,
>
>  	desc->rqcfg.src_inc = 1;
>  	desc->rqcfg.dst_inc = 1;
> +	desc->req.rqtype = MEMTOMEM;
>
>  	/* Select max possible burst size */
>  	burst = pi->pcfg.data_bus_width / 8;
> @@ -703,25 +695,14 @@ pl330_prep_slave_sg(struct dma_chan *chan,
> struct scatterlist *sgl,
>  {
>  	struct dma_pl330_desc *first, *desc = NULL;
>  	struct dma_pl330_chan *pch = to_pchan(chan);
> -	struct dma_pl330_peri *peri = chan->private;
>  	struct scatterlist *sg;
>  	unsigned long flags;
>  	int i;
>  	dma_addr_t addr;
>
> -	if (unlikely(!pch || !sgl || !sg_len || !peri))
> +	if (unlikely(!pch || !sgl || !sg_len))
>  		return NULL;
>
> -	/* Make sure the direction is consistent */
> -	if ((direction == DMA_TO_DEVICE &&
> -				peri->rqtype != MEMTODEV) ||
> -			(direction == DMA_FROM_DEVICE &&
> -				peri->rqtype != DEVTOMEM)) {
> -		dev_err(pch->dmac->pif.dev, "%s:%d Invalid Direction\n",
> -				__func__, __LINE__);
> -		return NULL;
> -	}
> -
>  	addr = pch->fifo_addr;
>
>  	first = NULL;
> @@ -761,11 +742,13 @@ pl330_prep_slave_sg(struct dma_chan *chan,
> struct scatterlist *sgl,
>  		if (direction == DMA_TO_DEVICE) {
>  			desc->rqcfg.src_inc = 1;
>  			desc->rqcfg.dst_inc = 0;
> +			desc->req.rqtype = MEMTODEV;
>  			fill_px(&desc->px,
>  				addr, sg_dma_address(sg), sg_dma_len(sg));
>  		} else {
>  			desc->rqcfg.src_inc = 0;
>  			desc->rqcfg.dst_inc = 1;
> +			desc->req.rqtype = DEVTOMEM;
>  			fill_px(&desc->px,
>  				sg_dma_address(sg), addr, sg_dma_len(sg));
>  		}
> @@ -872,27 +855,7 @@ pl330_probe(struct amba_device *adev, const
> struct amba_id *id)
>
>  	for (i = 0; i < num_chan; i++) {
>  		pch = &pdmac->peripherals[i];
> -		if (pdat) {
> -			struct dma_pl330_peri *peri = &pdat->peri[i];
> -
> -			switch (peri->rqtype) {
> -			case MEMTOMEM:
> -				dma_cap_set(DMA_MEMCPY, pd->cap_mask);
> -				break;
> -			case MEMTODEV:
> -			case DEVTOMEM:
> -				dma_cap_set(DMA_SLAVE, pd->cap_mask);
> -				dma_cap_set(DMA_CYCLIC, pd->cap_mask);
> -				break;
If remove capabilities setting, dmaengine() doesn't search the channel that
client requests with specific capability.
So, I think dma_request_channel() would fail.

> -			default:
> -				dev_err(&adev->dev, "DEVTODEV Not
> Supported\n");
> -				continue;
> -			}
> -			pch->chan.private = peri;
> -		} else {
> -			dma_cap_set(DMA_MEMCPY, pd->cap_mask);
> -			pch->chan.private = NULL;
> -		}
> +		pch->chan.private = (pdat) ? &pdat->peri_id[i] : NULL;
>
>  		INIT_LIST_HEAD(&pch->work_list);
>  		spin_lock_init(&pch->lock);
> @@ -907,6 +870,7 @@ pl330_probe(struct amba_device *adev, const struct
> amba_id *id)
>  	}
>
>  	pd->dev = &adev->dev;
> +	pd->cap_mask = pdat->cap_mask;
>
>  	pd->device_alloc_chan_resources = pl330_alloc_chan_resources;
>  	pd->device_free_chan_resources = pl330_free_chan_resources;
> diff --git a/include/linux/amba/pl330.h b/include/linux/amba/pl330.h
> index d12f077..0b87300 100644
> --- a/include/linux/amba/pl330.h
> +++ b/include/linux/amba/pl330.h
> @@ -13,15 +13,7 @@
>  #define	__AMBA_PL330_H_
>
>  #include <asm/hardware/pl330.h>
> -
> -struct dma_pl330_peri {
> -	/*
> -	 * Peri_Req i/f of the DMAC that is
> -	 * peripheral could be reached from.
> -	 */
> -	u8 peri_id; /* specific dma id */
> -	enum pl330_reqtype rqtype;
> -};
> +#include <linux/dmaengine.h>
>
>  struct dma_pl330_platdata {
>  	/*
> @@ -33,7 +25,9 @@ struct dma_pl330_platdata {
>  	 */
>  	u8 nr_valid_peri;
>  	/* Array of valid peripherals */
> -	struct dma_pl330_peri *peri;
> +	u8 *peri_id;
> +	/* Operational capabilities */
> +	dma_cap_mask_t cap_mask;
>  	/* Bytes to allocate for MC buffer */
>  	unsigned mcbuf_sz;
>  };
> --
> 1.6.6.rc2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-
> samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] DMA: PL330: Infer transfer direction from transfer request instead of platform data

[Boojin Kim]

Thomas Abraham wrote:
> Sent: Tuesday, August 23, 2011 7:00 AM
> To: linux-arm-kernel at lists.infradead.org
> Cc: linux-samsung-soc at vger.kernel.org; kgene.kim at samsung.com;
> vinod.koul at intel.com; Jassi Brar; Boojin Kim
> Subject: [PATCH 1/3] DMA: PL330: Infer transfer direction from
> transfer request instead of platform data
>
> The transfer direction for a channel can be inferred from the transfer
> request and the need for specifying transfer direction in platfrom
> data
> can be eliminated. So the structure definition 'struct dma_pl330_peri'
> is no longer required.
>
> With the 'struct dma_pl330_peri' removed, the dma controller transfer
> capabilities cannot be inferred any longer. Hence, the dma controller
> capabilities is specified using platforme data.
>
> Cc: Jassi Brar <jassisinghbrar@gmail.com>
> Cc: Boojin Kim <boojin.kim@samsung.com>
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
>  drivers/dma/pl330.c        |   56 ++++++++------------------------------
> ------
>  include/linux/amba/pl330.h |   14 +++--------
>  2 files changed, 14 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 3a0baac..6592b9a 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -507,7 +507,7 @@ pluck_desc(struct dma_pl330_dmac *pdmac)
>  static struct dma_pl330_desc *pl330_get_desc(struct dma_pl330_chan
> *pch)
>  {
>  	struct dma_pl330_dmac *pdmac = pch->dmac;
> -	struct dma_pl330_peri *peri = pch->chan.private;
> +	u8 *peri_id = pch->chan.private;
>  	struct dma_pl330_desc *desc;
>
>  	/* Pluck one desc from the pool of DMAC */
> @@ -531,14 +531,7 @@ static struct dma_pl330_desc
> *pl330_get_desc(struct dma_pl330_chan *pch)
>  	desc->pchan = pch;
>  	desc->txd.cookie = 0;
>  	async_tx_ack(&desc->txd);
> -
> -	if (peri) {
> -		desc->req.rqtype = peri->rqtype;
> -		desc->req.peri = pch->chan.chan_id;
> -	} else {
> -		desc->req.rqtype = MEMTOMEM;
> -		desc->req.peri = 0;
> -	}
> +	desc->req.peri = (peri_id) ? pch->chan.chan_id : 0;
>
>  	dma_async_tx_descriptor_init(&desc->txd, &pch->chan);
>
> @@ -625,12 +618,14 @@ static struct dma_async_tx_descriptor
> *pl330_prep_dma_cyclic(
>  	case DMA_TO_DEVICE:
>  		desc->rqcfg.src_inc = 1;
>  		desc->rqcfg.dst_inc = 0;
> +		desc->req.rqtype = MEMTODEV;
>  		src = dma_addr;
>  		dst = pch->fifo_addr;
>  		break;
>  	case DMA_FROM_DEVICE:
>  		desc->rqcfg.src_inc = 0;
>  		desc->rqcfg.dst_inc = 1;
> +		desc->req.rqtype = DEVTOMEM;
>  		src = pch->fifo_addr;
>  		dst = dma_addr;
>  		break;
> @@ -657,16 +652,12 @@ pl330_prep_dma_memcpy(struct dma_chan *chan,
> dma_addr_t dst,
>  {
>  	struct dma_pl330_desc *desc;
>  	struct dma_pl330_chan *pch = to_pchan(chan);
> -	struct dma_pl330_peri *peri = chan->private;
>  	struct pl330_info *pi;
>  	int burst;
>
>  	if (unlikely(!pch || !len))
>  		return NULL;
>
> -	if (peri && peri->rqtype != MEMTOMEM)
> -		return NULL;
> -
>  	pi = &pch->dmac->pif;
>
>  	desc = __pl330_prep_dma_memcpy(pch, dst, src, len);
> @@ -675,6 +666,7 @@ pl330_prep_dma_memcpy(struct dma_chan *chan,
> dma_addr_t dst,
>
>  	desc->rqcfg.src_inc = 1;
>  	desc->rqcfg.dst_inc = 1;
> +	desc->req.rqtype = MEMTOMEM;
>
>  	/* Select max possible burst size */
>  	burst = pi->pcfg.data_bus_width / 8;
> @@ -703,25 +695,14 @@ pl330_prep_slave_sg(struct dma_chan *chan,
> struct scatterlist *sgl,
>  {
>  	struct dma_pl330_desc *first, *desc = NULL;
>  	struct dma_pl330_chan *pch = to_pchan(chan);
> -	struct dma_pl330_peri *peri = chan->private;
>  	struct scatterlist *sg;
>  	unsigned long flags;
>  	int i;
>  	dma_addr_t addr;
>
> -	if (unlikely(!pch || !sgl || !sg_len || !peri))
> +	if (unlikely(!pch || !sgl || !sg_len))
>  		return NULL;
>
> -	/* Make sure the direction is consistent */
> -	if ((direction == DMA_TO_DEVICE &&
> -				peri->rqtype != MEMTODEV) ||
> -			(direction == DMA_FROM_DEVICE &&
> -				peri->rqtype != DEVTOMEM)) {
> -		dev_err(pch->dmac->pif.dev, "%s:%d Invalid Direction\n",
> -				__func__, __LINE__);
> -		return NULL;
> -	}
> -
>  	addr = pch->fifo_addr;
>
>  	first = NULL;
> @@ -761,11 +742,13 @@ pl330_prep_slave_sg(struct dma_chan *chan,
> struct scatterlist *sgl,
>  		if (direction == DMA_TO_DEVICE) {
>  			desc->rqcfg.src_inc = 1;
>  			desc->rqcfg.dst_inc = 0;
> +			desc->req.rqtype = MEMTODEV;
>  			fill_px(&desc->px,
>  				addr, sg_dma_address(sg), sg_dma_len(sg));
>  		} else {
>  			desc->rqcfg.src_inc = 0;
>  			desc->rqcfg.dst_inc = 1;
> +			desc->req.rqtype = DEVTOMEM;
>  			fill_px(&desc->px,
>  				sg_dma_address(sg), addr, sg_dma_len(sg));
>  		}
> @@ -872,27 +855,7 @@ pl330_probe(struct amba_device *adev, const
> struct amba_id *id)
>
>  	for (i = 0; i < num_chan; i++) {
>  		pch = &pdmac->peripherals[i];
> -		if (pdat) {
> -			struct dma_pl330_peri *peri = &pdat->peri[i];
> -
> -			switch (peri->rqtype) {
> -			case MEMTOMEM:
> -				dma_cap_set(DMA_MEMCPY, pd->cap_mask);
> -				break;
> -			case MEMTODEV:
> -			case DEVTOMEM:
> -				dma_cap_set(DMA_SLAVE, pd->cap_mask);
> -				dma_cap_set(DMA_CYCLIC, pd->cap_mask);
> -				break;
If remove capabilities setting, dmaengine() doesn't search the channel that
client requests with specific capability.
So, I think dma_request_channel() would fail.

> -			default:
> -				dev_err(&adev->dev, "DEVTODEV Not
> Supported\n");
> -				continue;
> -			}
> -			pch->chan.private = peri;
> -		} else {
> -			dma_cap_set(DMA_MEMCPY, pd->cap_mask);
> -			pch->chan.private = NULL;
> -		}
> +		pch->chan.private = (pdat) ? &pdat->peri_id[i] : NULL;
>
>  		INIT_LIST_HEAD(&pch->work_list);
>  		spin_lock_init(&pch->lock);
> @@ -907,6 +870,7 @@ pl330_probe(struct amba_device *adev, const struct
> amba_id *id)
>  	}
>
>  	pd->dev = &adev->dev;
> +	pd->cap_mask = pdat->cap_mask;
>
>  	pd->device_alloc_chan_resources = pl330_alloc_chan_resources;
>  	pd->device_free_chan_resources = pl330_free_chan_resources;
> diff --git a/include/linux/amba/pl330.h b/include/linux/amba/pl330.h
> index d12f077..0b87300 100644
> --- a/include/linux/amba/pl330.h
> +++ b/include/linux/amba/pl330.h
> @@ -13,15 +13,7 @@
>  #define	__AMBA_PL330_H_
>
>  #include <asm/hardware/pl330.h>
> -
> -struct dma_pl330_peri {
> -	/*
> -	 * Peri_Req i/f of the DMAC that is
> -	 * peripheral could be reached from.
> -	 */
> -	u8 peri_id; /* specific dma id */
> -	enum pl330_reqtype rqtype;
> -};
> +#include <linux/dmaengine.h>
>
>  struct dma_pl330_platdata {
>  	/*
> @@ -33,7 +25,9 @@ struct dma_pl330_platdata {
>  	 */
>  	u8 nr_valid_peri;
>  	/* Array of valid peripherals */
> -	struct dma_pl330_peri *peri;
> +	u8 *peri_id;
> +	/* Operational capabilities */
> +	dma_cap_mask_t cap_mask;
>  	/* Bytes to allocate for MC buffer */
>  	unsigned mcbuf_sz;
>  };
> --
> 1.6.6.rc2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-
> samsung-soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] DMA: PL330: Infer transfer direction from transfer request instead of platform data

[Thomas Abraham]

Hi Boojin,

On 24 August 2011 06:51, Boojin Kim <boojin.kim@samsung.com> wrote:
> Thomas Abraham wrote:
>> Sent: Tuesday, August 23, 2011 7:00 AM
>> To: linux-arm-kernel@lists.infradead.org
>> Cc: linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
>> vinod.koul@intel.com; Jassi Brar; Boojin Kim
>> Subject: [PATCH 1/3] DMA: PL330: Infer transfer direction from
>> transfer request instead of platform data
>>
>> The transfer direction for a channel can be inferred from the transfer
>> request and the need for specifying transfer direction in platfrom
>> data
>> can be eliminated. So the structure definition 'struct dma_pl330_peri'
>> is no longer required.
>>
>> With the 'struct dma_pl330_peri' removed, the dma controller transfer
>> capabilities cannot be inferred any longer. Hence, the dma controller
>> capabilities is specified using platforme data.
>>
>> Cc: Jassi Brar <jassisinghbrar@gmail.com>
>> Cc: Boojin Kim <boojin.kim@samsung.com>
>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> ---
>>  drivers/dma/pl330.c        |   56 ++++++++------------------------------
>> ------
>>  include/linux/amba/pl330.h |   14 +++--------
>>  2 files changed, 14 insertions(+), 56 deletions(-)
>>

[...]

>>       for (i = 0; i < num_chan; i++) {
>>               pch = &pdmac->peripherals[i];
>> -             if (pdat) {
>> -                     struct dma_pl330_peri *peri = &pdat->peri[i];
>> -
>> -                     switch (peri->rqtype) {
>> -                     case MEMTOMEM:
>> -                             dma_cap_set(DMA_MEMCPY, pd->cap_mask);
>> -                             break;
>> -                     case MEMTODEV:
>> -                     case DEVTOMEM:
>> -                             dma_cap_set(DMA_SLAVE, pd->cap_mask);
>> -                             dma_cap_set(DMA_CYCLIC, pd->cap_mask);
>> -                             break;
> If remove capabilities setting, dmaengine() doesn't search the channel that
> client requests with specific capability.
> So, I think dma_request_channel() would fail.

The capabilities is now supplied using platform data. There is a new
member 'cap_mask' added to the pl330 driver platform data structure.
The capabilities for a pl330 controller can be specified using
cap_mask. For exynos4, the capabilities is set up in the
exynos4_dma_init function. These changes are part of the third patch
in this patchset.

Thanks for your review.

Regards,
Thomas.

[...]

[PATCH 1/3] DMA: PL330: Infer transfer direction from transfer request instead of platform data

[Thomas Abraham]

Hi Boojin,

On 24 August 2011 06:51, Boojin Kim <boojin.kim@samsung.com> wrote:
> Thomas Abraham wrote:
>> Sent: Tuesday, August 23, 2011 7:00 AM
>> To: linux-arm-kernel at lists.infradead.org
>> Cc: linux-samsung-soc at vger.kernel.org; kgene.kim at samsung.com;
>> vinod.koul at intel.com; Jassi Brar; Boojin Kim
>> Subject: [PATCH 1/3] DMA: PL330: Infer transfer direction from
>> transfer request instead of platform data
>>
>> The transfer direction for a channel can be inferred from the transfer
>> request and the need for specifying transfer direction in platfrom
>> data
>> can be eliminated. So the structure definition 'struct dma_pl330_peri'
>> is no longer required.
>>
>> With the 'struct dma_pl330_peri' removed, the dma controller transfer
>> capabilities cannot be inferred any longer. Hence, the dma controller
>> capabilities is specified using platforme data.
>>
>> Cc: Jassi Brar <jassisinghbrar@gmail.com>
>> Cc: Boojin Kim <boojin.kim@samsung.com>
>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> ---
>> ?drivers/dma/pl330.c ? ? ? ?| ? 56 ++++++++------------------------------
>> ------
>> ?include/linux/amba/pl330.h | ? 14 +++--------
>> ?2 files changed, 14 insertions(+), 56 deletions(-)
>>

[...]

>> ? ? ? for (i = 0; i < num_chan; i++) {
>> ? ? ? ? ? ? ? pch = &pdmac->peripherals[i];
>> - ? ? ? ? ? ? if (pdat) {
>> - ? ? ? ? ? ? ? ? ? ? struct dma_pl330_peri *peri = &pdat->peri[i];
>> -
>> - ? ? ? ? ? ? ? ? ? ? switch (peri->rqtype) {
>> - ? ? ? ? ? ? ? ? ? ? case MEMTOMEM:
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? dma_cap_set(DMA_MEMCPY, pd->cap_mask);
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
>> - ? ? ? ? ? ? ? ? ? ? case MEMTODEV:
>> - ? ? ? ? ? ? ? ? ? ? case DEVTOMEM:
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? dma_cap_set(DMA_SLAVE, pd->cap_mask);
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? dma_cap_set(DMA_CYCLIC, pd->cap_mask);
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
> If remove capabilities setting, dmaengine() doesn't search the channel that
> client requests with specific capability.
> So, I think dma_request_channel() would fail.

The capabilities is now supplied using platform data. There is a new
member 'cap_mask' added to the pl330 driver platform data structure.
The capabilities for a pl330 controller can be specified using
cap_mask. For exynos4, the capabilities is set up in the
exynos4_dma_init function. These changes are part of the third patch
in this patchset.

Thanks for your review.

Regards,
Thomas.

[...]

RE: [PATCH 1/3] DMA: PL330: Infer transfer direction from transfer request instead of platform data

[Boojin Kim]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=EUC-KR, Size: 3266 bytes --]

Thomas Abraham wrote:
> Sent: Wednesday, August 24, 2011 12:23 PM
> To: Boojin Kim
> Cc: linux-arm-kernel@lists.infradead.org; linux-samsung-
> soc@vger.kernel.org; kgene.kim@samsung.com; vinod.koul@intel.com;
> Jassi Brar
> Subject: Re: [PATCH 1/3] DMA: PL330: Infer transfer direction from
> transfer request instead of platform data
>
> Hi Boojin,
>
> On 24 August 2011 06:51, Boojin Kim <boojin.kim@samsung.com> wrote:
> > Thomas Abraham wrote:
> >> Sent: Tuesday, August 23, 2011 7:00 AM
> >> To: linux-arm-kernel@lists.infradead.org
> >> Cc: linux-samsung-soc@vger.kernel.org; kgene.kim@samsung.com;
> >> vinod.koul@intel.com; Jassi Brar; Boojin Kim
> >> Subject: [PATCH 1/3] DMA: PL330: Infer transfer direction from
> >> transfer request instead of platform data
> >>
> >> The transfer direction for a channel can be inferred from the
> transfer
> >> request and the need for specifying transfer direction in platfrom
> >> data
> >> can be eliminated. So the structure definition 'struct
> dma_pl330_peri'
> >> is no longer required.
> >>
> >> With the 'struct dma_pl330_peri' removed, the dma controller
> transfer
> >> capabilities cannot be inferred any longer. Hence, the dma
> controller
> >> capabilities is specified using platforme data.
> >>
> >> Cc: Jassi Brar <jassisinghbrar@gmail.com>
> >> Cc: Boojin Kim <boojin.kim@samsung.com>
> >> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> >> ---
> >>  drivers/dma/pl330.c        |   56 ++++++++--------------------------
> ----
> >> ------
> >>  include/linux/amba/pl330.h |   14 +++--------
> >>  2 files changed, 14 insertions(+), 56 deletions(-)
> >>
>
> [...]
>
> >>       for (i = 0; i < num_chan; i++) {
> >>               pch = &pdmac->peripherals[i];
> >> -             if (pdat) {
> >> -                     struct dma_pl330_peri *peri = &pdat->peri[i];
> >> -
> >> -                     switch (peri->rqtype) {
> >> -                     case MEMTOMEM:
> >> -                             dma_cap_set(DMA_MEMCPY, pd->cap_mask);
> >> -                             break;
> >> -                     case MEMTODEV:
> >> -                     case DEVTOMEM:
> >> -                             dma_cap_set(DMA_SLAVE, pd->cap_mask);
> >> -                             dma_cap_set(DMA_CYCLIC, pd->cap_mask);
> >> -                             break;
> > If remove capabilities setting, dmaengine() doesn't search the
> channel that
> > client requests with specific capability.
> > So, I think dma_request_channel() would fail.
>
> The capabilities is now supplied using platform data. There is a new
> member 'cap_mask' added to the pl330 driver platform data structure.
> The capabilities for a pl330 controller can be specified using
> cap_mask. For exynos4, the capabilities is set up in the
> exynos4_dma_init function. These changes are part of the third patch
> in this patchset.
>
> Thanks for your review.
>
> Regards,
> Thomas.
Okay, It's good to me.
Thanks,
Boojin

>
> [...]
> --
> To unsubscribe from this list: send the line "unsubscribe linux-
> samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] DMA: PL330: Infer transfer direction from transfer request instead of platform data

[Boojin Kim]

Thomas Abraham wrote:
> Sent: Wednesday, August 24, 2011 12:23 PM
> To: Boojin Kim
> Cc: linux-arm-kernel at lists.infradead.org; linux-samsung-
> soc at vger.kernel.org; kgene.kim at samsung.com; vinod.koul at intel.com;
> Jassi Brar
> Subject: Re: [PATCH 1/3] DMA: PL330: Infer transfer direction from
> transfer request instead of platform data
>
> Hi Boojin,
>
> On 24 August 2011 06:51, Boojin Kim <boojin.kim@samsung.com> wrote:
> > Thomas Abraham wrote:
> >> Sent: Tuesday, August 23, 2011 7:00 AM
> >> To: linux-arm-kernel at lists.infradead.org
> >> Cc: linux-samsung-soc at vger.kernel.org; kgene.kim at samsung.com;
> >> vinod.koul at intel.com; Jassi Brar; Boojin Kim
> >> Subject: [PATCH 1/3] DMA: PL330: Infer transfer direction from
> >> transfer request instead of platform data
> >>
> >> The transfer direction for a channel can be inferred from the
> transfer
> >> request and the need for specifying transfer direction in platfrom
> >> data
> >> can be eliminated. So the structure definition 'struct
> dma_pl330_peri'
> >> is no longer required.
> >>
> >> With the 'struct dma_pl330_peri' removed, the dma controller
> transfer
> >> capabilities cannot be inferred any longer. Hence, the dma
> controller
> >> capabilities is specified using platforme data.
> >>
> >> Cc: Jassi Brar <jassisinghbrar@gmail.com>
> >> Cc: Boojin Kim <boojin.kim@samsung.com>
> >> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> >> ---
> >> ?drivers/dma/pl330.c ? ? ? ?| ? 56 ++++++++--------------------------
> ----
> >> ------
> >> ?include/linux/amba/pl330.h | ? 14 +++--------
> >> ?2 files changed, 14 insertions(+), 56 deletions(-)
> >>
>
> [...]
>
> >> ? ? ? for (i = 0; i < num_chan; i++) {
> >> ? ? ? ? ? ? ? pch = &pdmac->peripherals[i];
> >> - ? ? ? ? ? ? if (pdat) {
> >> - ? ? ? ? ? ? ? ? ? ? struct dma_pl330_peri *peri = &pdat->peri[i];
> >> -
> >> - ? ? ? ? ? ? ? ? ? ? switch (peri->rqtype) {
> >> - ? ? ? ? ? ? ? ? ? ? case MEMTOMEM:
> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? dma_cap_set(DMA_MEMCPY, pd->cap_mask);
> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
> >> - ? ? ? ? ? ? ? ? ? ? case MEMTODEV:
> >> - ? ? ? ? ? ? ? ? ? ? case DEVTOMEM:
> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? dma_cap_set(DMA_SLAVE, pd->cap_mask);
> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? dma_cap_set(DMA_CYCLIC, pd->cap_mask);
> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
> > If remove capabilities setting, dmaengine() doesn't search the
> channel that
> > client requests with specific capability.
> > So, I think dma_request_channel() would fail.
>
> The capabilities is now supplied using platform data. There is a new
> member 'cap_mask' added to the pl330 driver platform data structure.
> The capabilities for a pl330 controller can be specified using
> cap_mask. For exynos4, the capabilities is set up in the
> exynos4_dma_init function. These changes are part of the third patch
> in this patchset.
>
> Thanks for your review.
>
> Regards,
> Thomas.
Okay, It's good to me.
Thanks,
Boojin

>
> [...]
> --
> To unsubscribe from this list: send the line "unsubscribe linux-
> samsung-soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] DMA: PL330: Infer transfer direction from transfer request instead of platform data

[Rob Herring]

Thomas,

On 08/22/2011 04:59 PM, Thomas Abraham wrote:
> The transfer direction for a channel can be inferred from the transfer
> request and the need for specifying transfer direction in platfrom data
> can be eliminated. So the structure definition 'struct dma_pl330_peri'
> is no longer required.
> 
> With the 'struct dma_pl330_peri' removed, the dma controller transfer
> capabilities cannot be inferred any longer. Hence, the dma controller
> capabilities is specified using platforme data.
> 
> Cc: Jassi Brar <jassisinghbrar@gmail.com>
> Cc: Boojin Kim <boojin.kim@samsung.com>
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
>  drivers/dma/pl330.c        |   56 ++++++++------------------------------------
>  include/linux/amba/pl330.h |   14 +++--------
>  2 files changed, 14 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 3a0baac..6592b9a 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -507,7 +507,7 @@ pluck_desc(struct dma_pl330_dmac *pdmac)
>  static struct dma_pl330_desc *pl330_get_desc(struct dma_pl330_chan *pch)
>  {
>  	struct dma_pl330_dmac *pdmac = pch->dmac;
> -	struct dma_pl330_peri *peri = pch->chan.private;
> +	u8 *peri_id = pch->chan.private;
>  	struct dma_pl330_desc *desc;
>  
>  	/* Pluck one desc from the pool of DMAC */
> @@ -531,14 +531,7 @@ static struct dma_pl330_desc *pl330_get_desc(struct dma_pl330_chan *pch)
>  	desc->pchan = pch;
>  	desc->txd.cookie = 0;
>  	async_tx_ack(&desc->txd);
> -
> -	if (peri) {
> -		desc->req.rqtype = peri->rqtype;
> -		desc->req.peri = pch->chan.chan_id;
> -	} else {
> -		desc->req.rqtype = MEMTOMEM;
> -		desc->req.peri = 0;
> -	}
> +	desc->req.peri = (peri_id) ? pch->chan.chan_id : 0;
>  
>  	dma_async_tx_descriptor_init(&desc->txd, &pch->chan);
>  
> @@ -625,12 +618,14 @@ static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic(
>  	case DMA_TO_DEVICE:
>  		desc->rqcfg.src_inc = 1;
>  		desc->rqcfg.dst_inc = 0;
> +		desc->req.rqtype = MEMTODEV;
>  		src = dma_addr;
>  		dst = pch->fifo_addr;
>  		break;
>  	case DMA_FROM_DEVICE:
>  		desc->rqcfg.src_inc = 0;
>  		desc->rqcfg.dst_inc = 1;
> +		desc->req.rqtype = DEVTOMEM;
>  		src = pch->fifo_addr;
>  		dst = dma_addr;
>  		break;
> @@ -657,16 +652,12 @@ pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,
>  {
>  	struct dma_pl330_desc *desc;
>  	struct dma_pl330_chan *pch = to_pchan(chan);
> -	struct dma_pl330_peri *peri = chan->private;
>  	struct pl330_info *pi;
>  	int burst;
>  
>  	if (unlikely(!pch || !len))
>  		return NULL;
>  
> -	if (peri && peri->rqtype != MEMTOMEM)
> -		return NULL;
> -
>  	pi = &pch->dmac->pif;
>  
>  	desc = __pl330_prep_dma_memcpy(pch, dst, src, len);
> @@ -675,6 +666,7 @@ pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,
>  
>  	desc->rqcfg.src_inc = 1;
>  	desc->rqcfg.dst_inc = 1;
> +	desc->req.rqtype = MEMTOMEM;
>  
>  	/* Select max possible burst size */
>  	burst = pi->pcfg.data_bus_width / 8;
> @@ -703,25 +695,14 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  {
>  	struct dma_pl330_desc *first, *desc = NULL;
>  	struct dma_pl330_chan *pch = to_pchan(chan);
> -	struct dma_pl330_peri *peri = chan->private;
>  	struct scatterlist *sg;
>  	unsigned long flags;
>  	int i;
>  	dma_addr_t addr;
>  
> -	if (unlikely(!pch || !sgl || !sg_len || !peri))
> +	if (unlikely(!pch || !sgl || !sg_len))
>  		return NULL;
>  
> -	/* Make sure the direction is consistent */
> -	if ((direction == DMA_TO_DEVICE &&
> -				peri->rqtype != MEMTODEV) ||
> -			(direction == DMA_FROM_DEVICE &&
> -				peri->rqtype != DEVTOMEM)) {
> -		dev_err(pch->dmac->pif.dev, "%s:%d Invalid Direction\n",
> -				__func__, __LINE__);
> -		return NULL;
> -	}
> -
>  	addr = pch->fifo_addr;
>  
>  	first = NULL;
> @@ -761,11 +742,13 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  		if (direction == DMA_TO_DEVICE) {
>  			desc->rqcfg.src_inc = 1;
>  			desc->rqcfg.dst_inc = 0;
> +			desc->req.rqtype = MEMTODEV;
>  			fill_px(&desc->px,
>  				addr, sg_dma_address(sg), sg_dma_len(sg));
>  		} else {
>  			desc->rqcfg.src_inc = 0;
>  			desc->rqcfg.dst_inc = 1;
> +			desc->req.rqtype = DEVTOMEM;
>  			fill_px(&desc->px,
>  				sg_dma_address(sg), addr, sg_dma_len(sg));
>  		}
> @@ -872,27 +855,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>  
>  	for (i = 0; i < num_chan; i++) {
>  		pch = &pdmac->peripherals[i];
> -		if (pdat) {
> -			struct dma_pl330_peri *peri = &pdat->peri[i];
> -
> -			switch (peri->rqtype) {
> -			case MEMTOMEM:
> -				dma_cap_set(DMA_MEMCPY, pd->cap_mask);
> -				break;
> -			case MEMTODEV:
> -			case DEVTOMEM:
> -				dma_cap_set(DMA_SLAVE, pd->cap_mask);
> -				dma_cap_set(DMA_CYCLIC, pd->cap_mask);
> -				break;
> -			default:
> -				dev_err(&adev->dev, "DEVTODEV Not Supported\n");
> -				continue;
> -			}
> -			pch->chan.private = peri;
> -		} else {
> -			dma_cap_set(DMA_MEMCPY, pd->cap_mask);
> -			pch->chan.private = NULL;
> -		}
> +		pch->chan.private = (pdat) ? &pdat->peri_id[i] : NULL;

Don't need parentheses here.

>  
>  		INIT_LIST_HEAD(&pch->work_list);
>  		spin_lock_init(&pch->lock);
> @@ -907,6 +870,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>  	}
>  
>  	pd->dev = &adev->dev;
> +	pd->cap_mask = pdat->cap_mask;

You are re-introducing the requirement to have platform data which I
just made optional. For mem to mem transfers, there is no reason to have
platform data. In my case, we only support mem to mem transfers.

How about:
pd->cap_mask = pdat ? pdat->cap_mask : DMA_MEMCPY;

Also, should you be using dma_cap_set here?

Rob

>  
>  	pd->device_alloc_chan_resources = pl330_alloc_chan_resources;
>  	pd->device_free_chan_resources = pl330_free_chan_resources;
> diff --git a/include/linux/amba/pl330.h b/include/linux/amba/pl330.h
> index d12f077..0b87300 100644
> --- a/include/linux/amba/pl330.h
> +++ b/include/linux/amba/pl330.h
> @@ -13,15 +13,7 @@
>  #define	__AMBA_PL330_H_
>  
>  #include <asm/hardware/pl330.h>
> -
> -struct dma_pl330_peri {
> -	/*
> -	 * Peri_Req i/f of the DMAC that is
> -	 * peripheral could be reached from.
> -	 */
> -	u8 peri_id; /* specific dma id */
> -	enum pl330_reqtype rqtype;
> -};
> +#include <linux/dmaengine.h>
>  
>  struct dma_pl330_platdata {
>  	/*
> @@ -33,7 +25,9 @@ struct dma_pl330_platdata {
>  	 */
>  	u8 nr_valid_peri;
>  	/* Array of valid peripherals */
> -	struct dma_pl330_peri *peri;
> +	u8 *peri_id;
> +	/* Operational capabilities */
> +	dma_cap_mask_t cap_mask;
>  	/* Bytes to allocate for MC buffer */
>  	unsigned mcbuf_sz;
>  };

[PATCH 1/3] DMA: PL330: Infer transfer direction from transfer request instead of platform data

[Rob Herring]

Thomas,

On 08/22/2011 04:59 PM, Thomas Abraham wrote:
> The transfer direction for a channel can be inferred from the transfer
> request and the need for specifying transfer direction in platfrom data
> can be eliminated. So the structure definition 'struct dma_pl330_peri'
> is no longer required.
> 
> With the 'struct dma_pl330_peri' removed, the dma controller transfer
> capabilities cannot be inferred any longer. Hence, the dma controller
> capabilities is specified using platforme data.
> 
> Cc: Jassi Brar <jassisinghbrar@gmail.com>
> Cc: Boojin Kim <boojin.kim@samsung.com>
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
>  drivers/dma/pl330.c        |   56 ++++++++------------------------------------
>  include/linux/amba/pl330.h |   14 +++--------
>  2 files changed, 14 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 3a0baac..6592b9a 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -507,7 +507,7 @@ pluck_desc(struct dma_pl330_dmac *pdmac)
>  static struct dma_pl330_desc *pl330_get_desc(struct dma_pl330_chan *pch)
>  {
>  	struct dma_pl330_dmac *pdmac = pch->dmac;
> -	struct dma_pl330_peri *peri = pch->chan.private;
> +	u8 *peri_id = pch->chan.private;
>  	struct dma_pl330_desc *desc;
>  
>  	/* Pluck one desc from the pool of DMAC */
> @@ -531,14 +531,7 @@ static struct dma_pl330_desc *pl330_get_desc(struct dma_pl330_chan *pch)
>  	desc->pchan = pch;
>  	desc->txd.cookie = 0;
>  	async_tx_ack(&desc->txd);
> -
> -	if (peri) {
> -		desc->req.rqtype = peri->rqtype;
> -		desc->req.peri = pch->chan.chan_id;
> -	} else {
> -		desc->req.rqtype = MEMTOMEM;
> -		desc->req.peri = 0;
> -	}
> +	desc->req.peri = (peri_id) ? pch->chan.chan_id : 0;
>  
>  	dma_async_tx_descriptor_init(&desc->txd, &pch->chan);
>  
> @@ -625,12 +618,14 @@ static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic(
>  	case DMA_TO_DEVICE:
>  		desc->rqcfg.src_inc = 1;
>  		desc->rqcfg.dst_inc = 0;
> +		desc->req.rqtype = MEMTODEV;
>  		src = dma_addr;
>  		dst = pch->fifo_addr;
>  		break;
>  	case DMA_FROM_DEVICE:
>  		desc->rqcfg.src_inc = 0;
>  		desc->rqcfg.dst_inc = 1;
> +		desc->req.rqtype = DEVTOMEM;
>  		src = pch->fifo_addr;
>  		dst = dma_addr;
>  		break;
> @@ -657,16 +652,12 @@ pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,
>  {
>  	struct dma_pl330_desc *desc;
>  	struct dma_pl330_chan *pch = to_pchan(chan);
> -	struct dma_pl330_peri *peri = chan->private;
>  	struct pl330_info *pi;
>  	int burst;
>  
>  	if (unlikely(!pch || !len))
>  		return NULL;
>  
> -	if (peri && peri->rqtype != MEMTOMEM)
> -		return NULL;
> -
>  	pi = &pch->dmac->pif;
>  
>  	desc = __pl330_prep_dma_memcpy(pch, dst, src, len);
> @@ -675,6 +666,7 @@ pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,
>  
>  	desc->rqcfg.src_inc = 1;
>  	desc->rqcfg.dst_inc = 1;
> +	desc->req.rqtype = MEMTOMEM;
>  
>  	/* Select max possible burst size */
>  	burst = pi->pcfg.data_bus_width / 8;
> @@ -703,25 +695,14 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  {
>  	struct dma_pl330_desc *first, *desc = NULL;
>  	struct dma_pl330_chan *pch = to_pchan(chan);
> -	struct dma_pl330_peri *peri = chan->private;
>  	struct scatterlist *sg;
>  	unsigned long flags;
>  	int i;
>  	dma_addr_t addr;
>  
> -	if (unlikely(!pch || !sgl || !sg_len || !peri))
> +	if (unlikely(!pch || !sgl || !sg_len))
>  		return NULL;
>  
> -	/* Make sure the direction is consistent */
> -	if ((direction == DMA_TO_DEVICE &&
> -				peri->rqtype != MEMTODEV) ||
> -			(direction == DMA_FROM_DEVICE &&
> -				peri->rqtype != DEVTOMEM)) {
> -		dev_err(pch->dmac->pif.dev, "%s:%d Invalid Direction\n",
> -				__func__, __LINE__);
> -		return NULL;
> -	}
> -
>  	addr = pch->fifo_addr;
>  
>  	first = NULL;
> @@ -761,11 +742,13 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  		if (direction == DMA_TO_DEVICE) {
>  			desc->rqcfg.src_inc = 1;
>  			desc->rqcfg.dst_inc = 0;
> +			desc->req.rqtype = MEMTODEV;
>  			fill_px(&desc->px,
>  				addr, sg_dma_address(sg), sg_dma_len(sg));
>  		} else {
>  			desc->rqcfg.src_inc = 0;
>  			desc->rqcfg.dst_inc = 1;
> +			desc->req.rqtype = DEVTOMEM;
>  			fill_px(&desc->px,
>  				sg_dma_address(sg), addr, sg_dma_len(sg));
>  		}
> @@ -872,27 +855,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>  
>  	for (i = 0; i < num_chan; i++) {
>  		pch = &pdmac->peripherals[i];
> -		if (pdat) {
> -			struct dma_pl330_peri *peri = &pdat->peri[i];
> -
> -			switch (peri->rqtype) {
> -			case MEMTOMEM:
> -				dma_cap_set(DMA_MEMCPY, pd->cap_mask);
> -				break;
> -			case MEMTODEV:
> -			case DEVTOMEM:
> -				dma_cap_set(DMA_SLAVE, pd->cap_mask);
> -				dma_cap_set(DMA_CYCLIC, pd->cap_mask);
> -				break;
> -			default:
> -				dev_err(&adev->dev, "DEVTODEV Not Supported\n");
> -				continue;
> -			}
> -			pch->chan.private = peri;
> -		} else {
> -			dma_cap_set(DMA_MEMCPY, pd->cap_mask);
> -			pch->chan.private = NULL;
> -		}
> +		pch->chan.private = (pdat) ? &pdat->peri_id[i] : NULL;

Don't need parentheses here.

>  
>  		INIT_LIST_HEAD(&pch->work_list);
>  		spin_lock_init(&pch->lock);
> @@ -907,6 +870,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>  	}
>  
>  	pd->dev = &adev->dev;
> +	pd->cap_mask = pdat->cap_mask;

You are re-introducing the requirement to have platform data which I
just made optional. For mem to mem transfers, there is no reason to have
platform data. In my case, we only support mem to mem transfers.

How about:
pd->cap_mask = pdat ? pdat->cap_mask : DMA_MEMCPY;

Also, should you be using dma_cap_set here?

Rob

>  
>  	pd->device_alloc_chan_resources = pl330_alloc_chan_resources;
>  	pd->device_free_chan_resources = pl330_free_chan_resources;
> diff --git a/include/linux/amba/pl330.h b/include/linux/amba/pl330.h
> index d12f077..0b87300 100644
> --- a/include/linux/amba/pl330.h
> +++ b/include/linux/amba/pl330.h
> @@ -13,15 +13,7 @@
>  #define	__AMBA_PL330_H_
>  
>  #include <asm/hardware/pl330.h>
> -
> -struct dma_pl330_peri {
> -	/*
> -	 * Peri_Req i/f of the DMAC that is
> -	 * peripheral could be reached from.
> -	 */
> -	u8 peri_id; /* specific dma id */
> -	enum pl330_reqtype rqtype;
> -};
> +#include <linux/dmaengine.h>
>  
>  struct dma_pl330_platdata {
>  	/*
> @@ -33,7 +25,9 @@ struct dma_pl330_platdata {
>  	 */
>  	u8 nr_valid_peri;
>  	/* Array of valid peripherals */
> -	struct dma_pl330_peri *peri;
> +	u8 *peri_id;
> +	/* Operational capabilities */
> +	dma_cap_mask_t cap_mask;
>  	/* Bytes to allocate for MC buffer */
>  	unsigned mcbuf_sz;
>  };

Re: [PATCH 1/3] DMA: PL330: Infer transfer direction from transfer request instead of platform data

[Thomas Abraham]

Hi Rob,

On 24 August 2011 17:14, Rob Herring <robherring2@gmail.com> wrote:
> Thomas,
>
> On 08/22/2011 04:59 PM, Thomas Abraham wrote:
>> The transfer direction for a channel can be inferred from the transfer
>> request and the need for specifying transfer direction in platfrom data
>> can be eliminated. So the structure definition 'struct dma_pl330_peri'
>> is no longer required.
>>
>> With the 'struct dma_pl330_peri' removed, the dma controller transfer
>> capabilities cannot be inferred any longer. Hence, the dma controller
>> capabilities is specified using platforme data.
>>
>> Cc: Jassi Brar <jassisinghbrar@gmail.com>
>> Cc: Boojin Kim <boojin.kim@samsung.com>
>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> ---
>>  drivers/dma/pl330.c        |   56 ++++++++------------------------------------
>>  include/linux/amba/pl330.h |   14 +++--------
>>  2 files changed, 14 insertions(+), 56 deletions(-)
>>
>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>> index 3a0baac..6592b9a 100644
>> --- a/drivers/dma/pl330.c
>> +++ b/drivers/dma/pl330.c

[...]

>> @@ -872,27 +855,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>>
>>       for (i = 0; i < num_chan; i++) {
>>               pch = &pdmac->peripherals[i];
>> -             if (pdat) {
>> -                     struct dma_pl330_peri *peri = &pdat->peri[i];
>> -
>> -                     switch (peri->rqtype) {
>> -                     case MEMTOMEM:
>> -                             dma_cap_set(DMA_MEMCPY, pd->cap_mask);
>> -                             break;
>> -                     case MEMTODEV:
>> -                     case DEVTOMEM:
>> -                             dma_cap_set(DMA_SLAVE, pd->cap_mask);
>> -                             dma_cap_set(DMA_CYCLIC, pd->cap_mask);
>> -                             break;
>> -                     default:
>> -                             dev_err(&adev->dev, "DEVTODEV Not Supported\n");
>> -                             continue;
>> -                     }
>> -                     pch->chan.private = peri;
>> -             } else {
>> -                     dma_cap_set(DMA_MEMCPY, pd->cap_mask);
>> -                     pch->chan.private = NULL;
>> -             }
>> +             pch->chan.private = (pdat) ? &pdat->peri_id[i] : NULL;
>
> Don't need parentheses here.

Ok. I will fix this in the next version of this patchset.

>
>>
>>               INIT_LIST_HEAD(&pch->work_list);
>>               spin_lock_init(&pch->lock);
>> @@ -907,6 +870,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>>       }
>>
>>       pd->dev = &adev->dev;
>> +     pd->cap_mask = pdat->cap_mask;
>
> You are re-introducing the requirement to have platform data which I
> just made optional. For mem to mem transfers, there is no reason to have
> platform data. In my case, we only support mem to mem transfers.
>
> How about:
> pd->cap_mask = pdat ? pdat->cap_mask : DMA_MEMCPY;

All of your changes to make platform data optional is still retained
in this patch. But, as you said, the above change that I did to get
the capabilities from pdata would not work when no pdata is supplied
(as in the case mem-to-mem transfers). Thanks for pointing this out.

This can be fixed as below

if (pdat)
        pd->cap_mask = pdat->cap_mask;
else
        dma_cap_set(DMA_MEMCPY, pd->cap_mask);

>
> Also, should you be using dma_cap_set here?

Yes. That would be required. I will do these changes and resubmit the
patch. Thanks for your review.

Regards,
Thomas.

>
> Rob
>

[...]

[PATCH 1/3] DMA: PL330: Infer transfer direction from transfer request instead of platform data

[Thomas Abraham]

Hi Rob,

On 24 August 2011 17:14, Rob Herring <robherring2@gmail.com> wrote:
> Thomas,
>
> On 08/22/2011 04:59 PM, Thomas Abraham wrote:
>> The transfer direction for a channel can be inferred from the transfer
>> request and the need for specifying transfer direction in platfrom data
>> can be eliminated. So the structure definition 'struct dma_pl330_peri'
>> is no longer required.
>>
>> With the 'struct dma_pl330_peri' removed, the dma controller transfer
>> capabilities cannot be inferred any longer. Hence, the dma controller
>> capabilities is specified using platforme data.
>>
>> Cc: Jassi Brar <jassisinghbrar@gmail.com>
>> Cc: Boojin Kim <boojin.kim@samsung.com>
>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> ---
>> ?drivers/dma/pl330.c ? ? ? ?| ? 56 ++++++++------------------------------------
>> ?include/linux/amba/pl330.h | ? 14 +++--------
>> ?2 files changed, 14 insertions(+), 56 deletions(-)
>>
>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>> index 3a0baac..6592b9a 100644
>> --- a/drivers/dma/pl330.c
>> +++ b/drivers/dma/pl330.c

[...]

>> @@ -872,27 +855,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>>
>> ? ? ? for (i = 0; i < num_chan; i++) {
>> ? ? ? ? ? ? ? pch = &pdmac->peripherals[i];
>> - ? ? ? ? ? ? if (pdat) {
>> - ? ? ? ? ? ? ? ? ? ? struct dma_pl330_peri *peri = &pdat->peri[i];
>> -
>> - ? ? ? ? ? ? ? ? ? ? switch (peri->rqtype) {
>> - ? ? ? ? ? ? ? ? ? ? case MEMTOMEM:
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? dma_cap_set(DMA_MEMCPY, pd->cap_mask);
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
>> - ? ? ? ? ? ? ? ? ? ? case MEMTODEV:
>> - ? ? ? ? ? ? ? ? ? ? case DEVTOMEM:
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? dma_cap_set(DMA_SLAVE, pd->cap_mask);
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? dma_cap_set(DMA_CYCLIC, pd->cap_mask);
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
>> - ? ? ? ? ? ? ? ? ? ? default:
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? dev_err(&adev->dev, "DEVTODEV Not Supported\n");
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? continue;
>> - ? ? ? ? ? ? ? ? ? ? }
>> - ? ? ? ? ? ? ? ? ? ? pch->chan.private = peri;
>> - ? ? ? ? ? ? } else {
>> - ? ? ? ? ? ? ? ? ? ? dma_cap_set(DMA_MEMCPY, pd->cap_mask);
>> - ? ? ? ? ? ? ? ? ? ? pch->chan.private = NULL;
>> - ? ? ? ? ? ? }
>> + ? ? ? ? ? ? pch->chan.private = (pdat) ? &pdat->peri_id[i] : NULL;
>
> Don't need parentheses here.

Ok. I will fix this in the next version of this patchset.

>
>>
>> ? ? ? ? ? ? ? INIT_LIST_HEAD(&pch->work_list);
>> ? ? ? ? ? ? ? spin_lock_init(&pch->lock);
>> @@ -907,6 +870,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>> ? ? ? }
>>
>> ? ? ? pd->dev = &adev->dev;
>> + ? ? pd->cap_mask = pdat->cap_mask;
>
> You are re-introducing the requirement to have platform data which I
> just made optional. For mem to mem transfers, there is no reason to have
> platform data. In my case, we only support mem to mem transfers.
>
> How about:
> pd->cap_mask = pdat ? pdat->cap_mask : DMA_MEMCPY;

All of your changes to make platform data optional is still retained
in this patch. But, as you said, the above change that I did to get
the capabilities from pdata would not work when no pdata is supplied
(as in the case mem-to-mem transfers). Thanks for pointing this out.

This can be fixed as below

if (pdat)
        pd->cap_mask = pdat->cap_mask;
else
        dma_cap_set(DMA_MEMCPY, pd->cap_mask);

>
> Also, should you be using dma_cap_set here?

Yes. That would be required. I will do these changes and resubmit the
patch. Thanks for your review.

Regards,
Thomas.

>
> Rob
>

[...]

Re: [PATCH 1/3] DMA: PL330: Infer transfer direction from transfer request instead of platform data

[Jassi Brar]

On Tue, Aug 23, 2011 at 3:20 AM, Thomas Abraham
<thomas.abraham@linaro.org> wrote:
> The transfer direction for a channel can be inferred from the transfer
> request and the need for specifying transfer direction in platfrom data
> can be eliminated. So the structure definition 'struct dma_pl330_peri'
> is no longer required.
>
> With the 'struct dma_pl330_peri' removed, the dma controller transfer
> capabilities cannot be inferred any longer. Hence, the dma controller
> capabilities is specified using platforme data.
>
> Cc: Jassi Brar <jassisinghbrar@gmail.com>
> Cc: Boojin Kim <boojin.kim@samsung.com>
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>

Looks ok.

Acked-by: Jassi Brar <jassisinghbrar@gmail.com>

[PATCH 1/3] DMA: PL330: Infer transfer direction from transfer request instead of platform data

[Thomas Abraham]

The transfer direction for a channel can be inferred from the transfer
request and the need for specifying transfer direction in platfrom data
can be eliminated. So the structure definition 'struct dma_pl330_peri'
is no longer required.

With the 'struct dma_pl330_peri' removed, the dma controller transfer
capabilities cannot be inferred any longer. Hence, the dma controller
capabilities is specified using platforme data.

Cc: Jassi Brar <jassisinghbrar@gmail.com>
Cc: Boojin Kim <boojin.kim@samsung.com>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 drivers/dma/pl330.c        |   56 ++++++++------------------------------------
 include/linux/amba/pl330.h |   14 +++--------
 2 files changed, 14 insertions(+), 56 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 3a0baac..6592b9a 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -507,7 +507,7 @@ pluck_desc(struct dma_pl330_dmac *pdmac)
 static struct dma_pl330_desc *pl330_get_desc(struct dma_pl330_chan *pch)
 {
 	struct dma_pl330_dmac *pdmac = pch->dmac;
-	struct dma_pl330_peri *peri = pch->chan.private;
+	u8 *peri_id = pch->chan.private;
 	struct dma_pl330_desc *desc;
 
 	/* Pluck one desc from the pool of DMAC */
@@ -531,14 +531,7 @@ static struct dma_pl330_desc *pl330_get_desc(struct dma_pl330_chan *pch)
 	desc->pchan = pch;
 	desc->txd.cookie = 0;
 	async_tx_ack(&desc->txd);
-
-	if (peri) {
-		desc->req.rqtype = peri->rqtype;
-		desc->req.peri = pch->chan.chan_id;
-	} else {
-		desc->req.rqtype = MEMTOMEM;
-		desc->req.peri = 0;
-	}
+	desc->req.peri = (peri_id) ? pch->chan.chan_id : 0;
 
 	dma_async_tx_descriptor_init(&desc->txd, &pch->chan);
 
@@ -625,12 +618,14 @@ static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic(
 	case DMA_TO_DEVICE:
 		desc->rqcfg.src_inc = 1;
 		desc->rqcfg.dst_inc = 0;
+		desc->req.rqtype = MEMTODEV;
 		src = dma_addr;
 		dst = pch->fifo_addr;
 		break;
 	case DMA_FROM_DEVICE:
 		desc->rqcfg.src_inc = 0;
 		desc->rqcfg.dst_inc = 1;
+		desc->req.rqtype = DEVTOMEM;
 		src = pch->fifo_addr;
 		dst = dma_addr;
 		break;
@@ -657,16 +652,12 @@ pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,
 {
 	struct dma_pl330_desc *desc;
 	struct dma_pl330_chan *pch = to_pchan(chan);
-	struct dma_pl330_peri *peri = chan->private;
 	struct pl330_info *pi;
 	int burst;
 
 	if (unlikely(!pch || !len))
 		return NULL;
 
-	if (peri && peri->rqtype != MEMTOMEM)
-		return NULL;
-
 	pi = &pch->dmac->pif;
 
 	desc = __pl330_prep_dma_memcpy(pch, dst, src, len);
@@ -675,6 +666,7 @@ pl330_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dst,
 
 	desc->rqcfg.src_inc = 1;
 	desc->rqcfg.dst_inc = 1;
+	desc->req.rqtype = MEMTOMEM;
 
 	/* Select max possible burst size */
 	burst = pi->pcfg.data_bus_width / 8;
@@ -703,25 +695,14 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 {
 	struct dma_pl330_desc *first, *desc = NULL;
 	struct dma_pl330_chan *pch = to_pchan(chan);
-	struct dma_pl330_peri *peri = chan->private;
 	struct scatterlist *sg;
 	unsigned long flags;
 	int i;
 	dma_addr_t addr;
 
-	if (unlikely(!pch || !sgl || !sg_len || !peri))
+	if (unlikely(!pch || !sgl || !sg_len))
 		return NULL;
 
-	/* Make sure the direction is consistent */
-	if ((direction == DMA_TO_DEVICE &&
-				peri->rqtype != MEMTODEV) ||
-			(direction == DMA_FROM_DEVICE &&
-				peri->rqtype != DEVTOMEM)) {
-		dev_err(pch->dmac->pif.dev, "%s:%d Invalid Direction\n",
-				__func__, __LINE__);
-		return NULL;
-	}
-
 	addr = pch->fifo_addr;
 
 	first = NULL;
@@ -761,11 +742,13 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		if (direction == DMA_TO_DEVICE) {
 			desc->rqcfg.src_inc = 1;
 			desc->rqcfg.dst_inc = 0;
+			desc->req.rqtype = MEMTODEV;
 			fill_px(&desc->px,
 				addr, sg_dma_address(sg), sg_dma_len(sg));
 		} else {
 			desc->rqcfg.src_inc = 0;
 			desc->rqcfg.dst_inc = 1;
+			desc->req.rqtype = DEVTOMEM;
 			fill_px(&desc->px,
 				sg_dma_address(sg), addr, sg_dma_len(sg));
 		}
@@ -872,27 +855,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 
 	for (i = 0; i < num_chan; i++) {
 		pch = &pdmac->peripherals[i];
-		if (pdat) {
-			struct dma_pl330_peri *peri = &pdat->peri[i];
-
-			switch (peri->rqtype) {
-			case MEMTOMEM:
-				dma_cap_set(DMA_MEMCPY, pd->cap_mask);
-				break;
-			case MEMTODEV:
-			case DEVTOMEM:
-				dma_cap_set(DMA_SLAVE, pd->cap_mask);
-				dma_cap_set(DMA_CYCLIC, pd->cap_mask);
-				break;
-			default:
-				dev_err(&adev->dev, "DEVTODEV Not Supported\n");
-				continue;
-			}
-			pch->chan.private = peri;
-		} else {
-			dma_cap_set(DMA_MEMCPY, pd->cap_mask);
-			pch->chan.private = NULL;
-		}
+		pch->chan.private = (pdat) ? &pdat->peri_id[i] : NULL;
 
 		INIT_LIST_HEAD(&pch->work_list);
 		spin_lock_init(&pch->lock);
@@ -907,6 +870,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 	}
 
 	pd->dev = &adev->dev;
+	pd->cap_mask = pdat->cap_mask;
 
 	pd->device_alloc_chan_resources = pl330_alloc_chan_resources;
 	pd->device_free_chan_resources = pl330_free_chan_resources;
diff --git a/include/linux/amba/pl330.h b/include/linux/amba/pl330.h
index d12f077..0b87300 100644
--- a/include/linux/amba/pl330.h
+++ b/include/linux/amba/pl330.h
@@ -13,15 +13,7 @@
 #define	__AMBA_PL330_H_
 
 #include <asm/hardware/pl330.h>
-
-struct dma_pl330_peri {
-	/*
-	 * Peri_Req i/f of the DMAC that is
-	 * peripheral could be reached from.
-	 */
-	u8 peri_id; /* specific dma id */
-	enum pl330_reqtype rqtype;
-};
+#include <linux/dmaengine.h>
 
 struct dma_pl330_platdata {
 	/*
@@ -33,7 +25,9 @@ struct dma_pl330_platdata {
 	 */
 	u8 nr_valid_peri;
 	/* Array of valid peripherals */
-	struct dma_pl330_peri *peri;
+	u8 *peri_id;
+	/* Operational capabilities */
+	dma_cap_mask_t cap_mask;
 	/* Bytes to allocate for MC buffer */
 	unsigned mcbuf_sz;
 };
-- 
1.6.6.rc2