diff mbox

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

Message ID 1314050385-9617-2-git-send-email-thomas.abraham@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Thomas Abraham Aug. 22, 2011, 9:59 p.m. UTC
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(-)

Comments

boojin.kim Aug. 24, 2011, 1:21 a.m. UTC | #1
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
Thomas Abraham Aug. 24, 2011, 3:22 a.m. UTC | #2
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.

[...]
Rob Herring Aug. 24, 2011, 11:44 a.m. UTC | #3
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;
>  };
Thomas Abraham Aug. 24, 2011, 2:25 p.m. UTC | #4
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
>

[...]
diff mbox

Patch

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;
 };