[V4,03/14] DMA: PL330: Support DMA_SLAVE_CONFIG command
diff mbox

Message ID 1311557312-26107-4-git-send-email-boojin.kim@samsung.com
State New, archived
Headers show

Commit Message

boojin.kim July 25, 2011, 1:28 a.m. UTC
This patch adds support DMA_SLAVE_CONFIG command.

Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
---
 drivers/dma/pl330.c |   53 +++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 39 insertions(+), 14 deletions(-)

Comments

Vinod Koul July 25, 2011, 10:26 a.m. UTC | #1
On Mon, 2011-07-25 at 10:28 +0900, Boojin Kim wrote:
> This patch adds support DMA_SLAVE_CONFIG command.
> 
> Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
> ---
>  drivers/dma/pl330.c |   53 +++++++++++++++++++++++++++++++++++++-------------
>  1 files changed, 39 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 586ab39..880f010 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -256,25 +256,50 @@ static int pl330_alloc_chan_resources(struct dma_chan *chan)
>  static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned long arg)
>  {
>  	struct dma_pl330_chan *pch = to_pchan(chan);
> -	struct dma_pl330_desc *desc;
> +	struct dma_pl330_desc *desc, *_dt;
>  	unsigned long flags;
> +	struct dma_pl330_dmac *pdmac = pch->dmac;
> +	struct dma_slave_config *slave_config;
> +	struct dma_pl330_peri *peri;
> +	LIST_HEAD(list);
>  
> -	/* Only supports DMA_TERMINATE_ALL */
> -	if (cmd != DMA_TERMINATE_ALL)
> -		return -ENXIO;
> -
> -	spin_lock_irqsave(&pch->lock, flags);
> -
> -	/* FLUSH the PL330 Channel thread */
> -	pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
> +	switch (cmd) {
> +	case DMA_TERMINATE_ALL:
> +		spin_lock_irqsave(&pch->lock, flags);
>  
> -	/* Mark all desc done */
> -	list_for_each_entry(desc, &pch->work_list, node)
> -		desc->status = DONE;
> +		/* FLUSH the PL330 Channel thread */
> +		pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
>  
> -	spin_unlock_irqrestore(&pch->lock, flags);
> +		/* Mark all desc done */
> +		list_for_each_entry_safe(desc, _dt, &pch->work_list , node) {
> +			desc->status = DONE;
> +			pch->completed = desc->txd.cookie;
> +			list_move_tail(&desc->node, &list);
> +		}
>  
> -	pl330_tasklet((unsigned long) pch);
> +		list_splice_tail_init(&list, &pdmac->desc_pool);
> +		spin_unlock_irqrestore(&pch->lock, flags);
> +		break;
> +	case DMA_SLAVE_CONFIG:
> +		slave_config = (struct dma_slave_config *)arg;
> +		peri = pch->chan.private;
> +
> +		if (slave_config->direction == DMA_TO_DEVICE) {
> +			if (slave_config->dst_addr)
> +				peri->fifo_addr = slave_config->dst_addr;
> +			if (slave_config->dst_addr_width)
> +				peri->burst_sz = __ffs(slave_config->dst_addr_width);
This doesn't sound right, why can't you use xxx_maxburst field instead?

> +		} else if (slave_config->direction == DMA_FROM_DEVICE) {
> +			if (slave_config->src_addr)
> +				peri->fifo_addr = slave_config->src_addr;
> +			if (slave_config->src_addr_width)
> +				peri->burst_sz = __ffs(slave_config->src_addr_width);
ditto...
> +		}
> +		break;
> +	default:
> +		dev_err(pch->dmac->pif.dev, "Not supported command.\n");
> +		return -ENXIO;
> +	}
>  
>  	return 0;
>  }
boojin.kim July 25, 2011, 12:51 p.m. UTC | #2
Vinod Koul Wrote:
> Sent: Monday, July 25, 2011 7:27 PM
> To: Boojin Kim
> Cc: vinod.koul@intel.com; linux-arm-kernel@lists.infradead.org; linux-
> samsung-soc@vger.kernel.org; Kukjin Kim; Jassi Brar; Grant Likely;
> Mark Brown; Dan Williams
> Subject: Re: [PATCH V4 03/14] DMA: PL330: Support DMA_SLAVE_CONFIG
> command
>
> On Mon, 2011-07-25 at 10:28 +0900, Boojin Kim wrote:
> > This patch adds support DMA_SLAVE_CONFIG command.
> >
> > Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
> > ---
> >  drivers/dma/pl330.c |   53 +++++++++++++++++++++++++++++++++++++----
> ---------
> >  1 files changed, 39 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> > index 586ab39..880f010 100644
> > --- a/drivers/dma/pl330.c
> > +++ b/drivers/dma/pl330.c
> > @@ -256,25 +256,50 @@ static int pl330_alloc_chan_resources(struct
> dma_chan *chan)
> >  static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd
> cmd, unsigned long arg)
> >  {
> >  	struct dma_pl330_chan *pch = to_pchan(chan);
> > -	struct dma_pl330_desc *desc;
> > +	struct dma_pl330_desc *desc, *_dt;
> >  	unsigned long flags;
> > +	struct dma_pl330_dmac *pdmac = pch->dmac;
> > +	struct dma_slave_config *slave_config;
> > +	struct dma_pl330_peri *peri;
> > +	LIST_HEAD(list);
> >
> > -	/* Only supports DMA_TERMINATE_ALL */
> > -	if (cmd != DMA_TERMINATE_ALL)
> > -		return -ENXIO;
> > -
> > -	spin_lock_irqsave(&pch->lock, flags);
> > -
> > -	/* FLUSH the PL330 Channel thread */
> > -	pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
> > +	switch (cmd) {
> > +	case DMA_TERMINATE_ALL:
> > +		spin_lock_irqsave(&pch->lock, flags);
> >
> > -	/* Mark all desc done */
> > -	list_for_each_entry(desc, &pch->work_list, node)
> > -		desc->status = DONE;
> > +		/* FLUSH the PL330 Channel thread */
> > +		pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
> >
> > -	spin_unlock_irqrestore(&pch->lock, flags);
> > +		/* Mark all desc done */
> > +		list_for_each_entry_safe(desc, _dt, &pch->work_list , node)
> {
> > +			desc->status = DONE;
> > +			pch->completed = desc->txd.cookie;
> > +			list_move_tail(&desc->node, &list);
> > +		}
> >
> > -	pl330_tasklet((unsigned long) pch);
> > +		list_splice_tail_init(&list, &pdmac->desc_pool);
> > +		spin_unlock_irqrestore(&pch->lock, flags);
> > +		break;
> > +	case DMA_SLAVE_CONFIG:
> > +		slave_config = (struct dma_slave_config *)arg;
> > +		peri = pch->chan.private;
> > +
> > +		if (slave_config->direction == DMA_TO_DEVICE) {
> > +			if (slave_config->dst_addr)
> > +				peri->fifo_addr = slave_config->dst_addr;
> > +			if (slave_config->dst_addr_width)
> > +				peri->burst_sz = __ffs(slave_config-
> >dst_addr_width);
> This doesn't sound right, why can't you use xxx_maxburst field instead?
Can you explain the problem in more detail?
Do you mean that 'xxx_maxburst' takes place of 'xxx_addr_width' or __ffs() 
isn't proper here?

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

>
> > +		} else if (slave_config->direction == DMA_FROM_DEVICE) {
> > +			if (slave_config->src_addr)
> > +				peri->fifo_addr = slave_config->src_addr;
> > +			if (slave_config->src_addr_width)
> > +				peri->burst_sz = __ffs(slave_config-
> >src_addr_width);
> ditto...
> > +		}
> > +		break;
> > +	default:
> > +		dev_err(pch->dmac->pif.dev, "Not supported command.\n");
> > +		return -ENXIO;
> > +	}
> >
> >  	return 0;
> >  }
>
> --
> ~Vinod Koul
> Intel Corp.
Vinod Koul July 25, 2011, 1:48 p.m. UTC | #3
On Mon, 2011-07-25 at 21:51 +0900, Boojin Kim wrote:
> Vinod Koul Wrote:
> > Sent: Monday, July 25, 2011 7:27 PM
> > To: Boojin Kim
> > Cc: vinod.koul@intel.com; linux-arm-kernel@lists.infradead.org; linux-
> > samsung-soc@vger.kernel.org; Kukjin Kim; Jassi Brar; Grant Likely;
> > Mark Brown; Dan Williams
> > Subject: Re: [PATCH V4 03/14] DMA: PL330: Support DMA_SLAVE_CONFIG
> > command
> >
> > On Mon, 2011-07-25 at 10:28 +0900, Boojin Kim wrote:
> > > This patch adds support DMA_SLAVE_CONFIG command.
> > >
> > > Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
> > > ---
> > >  drivers/dma/pl330.c |   53 +++++++++++++++++++++++++++++++++++++----
> > ---------
> > >  1 files changed, 39 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> > > index 586ab39..880f010 100644
> > > --- a/drivers/dma/pl330.c
> > > +++ b/drivers/dma/pl330.c
> > > @@ -256,25 +256,50 @@ static int pl330_alloc_chan_resources(struct
> > dma_chan *chan)
> > >  static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd
> > cmd, unsigned long arg)
> > >  {
> > >  	struct dma_pl330_chan *pch = to_pchan(chan);
> > > -	struct dma_pl330_desc *desc;
> > > +	struct dma_pl330_desc *desc, *_dt;
> > >  	unsigned long flags;
> > > +	struct dma_pl330_dmac *pdmac = pch->dmac;
> > > +	struct dma_slave_config *slave_config;
> > > +	struct dma_pl330_peri *peri;
> > > +	LIST_HEAD(list);
> > >
> > > -	/* Only supports DMA_TERMINATE_ALL */
> > > -	if (cmd != DMA_TERMINATE_ALL)
> > > -		return -ENXIO;
> > > -
> > > -	spin_lock_irqsave(&pch->lock, flags);
> > > -
> > > -	/* FLUSH the PL330 Channel thread */
> > > -	pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
> > > +	switch (cmd) {
> > > +	case DMA_TERMINATE_ALL:
> > > +		spin_lock_irqsave(&pch->lock, flags);
> > >
> > > -	/* Mark all desc done */
> > > -	list_for_each_entry(desc, &pch->work_list, node)
> > > -		desc->status = DONE;
> > > +		/* FLUSH the PL330 Channel thread */
> > > +		pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
> > >
> > > -	spin_unlock_irqrestore(&pch->lock, flags);
> > > +		/* Mark all desc done */
> > > +		list_for_each_entry_safe(desc, _dt, &pch->work_list , node)
> > {
> > > +			desc->status = DONE;
> > > +			pch->completed = desc->txd.cookie;
> > > +			list_move_tail(&desc->node, &list);
> > > +		}
> > >
> > > -	pl330_tasklet((unsigned long) pch);
> > > +		list_splice_tail_init(&list, &pdmac->desc_pool);
> > > +		spin_unlock_irqrestore(&pch->lock, flags);
> > > +		break;
> > > +	case DMA_SLAVE_CONFIG:
> > > +		slave_config = (struct dma_slave_config *)arg;
> > > +		peri = pch->chan.private;
> > > +
> > > +		if (slave_config->direction == DMA_TO_DEVICE) {
> > > +			if (slave_config->dst_addr)
> > > +				peri->fifo_addr = slave_config->dst_addr;
> > > +			if (slave_config->dst_addr_width)
> > > +				peri->burst_sz = __ffs(slave_config-
> > >dst_addr_width);
> > This doesn't sound right, why can't you use xxx_maxburst field instead?
> Can you explain the problem in more detail?
> Do you mean that 'xxx_maxburst' takes place of 'xxx_addr_width' or __ffs() 
> isn't proper here?
> 
> Additionally, I will modify this code to consider '_maxburst' field.
__ffs is okay and sensible.
I meant you should use proper fields for passing info, which in this
case would be dst_maxburst for specifying destination burst sizes
boojin.kim July 26, 2011, 1:04 p.m. UTC | #4
Vinod Koul Wrote:
> Sent: Monday, July 25, 2011 10:48 PM
> To: Boojin Kim
> Cc: vinod.koul@intel.com; linux-arm-kernel@lists.infradead.org; linux-
> samsung-soc@vger.kernel.org; 'Kukjin Kim'; 'Jassi Brar'; 'Grant
> Likely'; 'Mark Brown'; 'Dan Williams'
> Subject: RE: [PATCH V4 03/14] DMA: PL330: Support DMA_SLAVE_CONFIG
> command
>
> On Mon, 2011-07-25 at 21:51 +0900, Boojin Kim wrote:
> > Vinod Koul Wrote:
> > > Sent: Monday, July 25, 2011 7:27 PM
> > > To: Boojin Kim
> > > Cc: vinod.koul@intel.com; linux-arm-kernel@lists.infradead.org;
> linux-
> > > samsung-soc@vger.kernel.org; Kukjin Kim; Jassi Brar; Grant Likely;
> > > Mark Brown; Dan Williams
> > > Subject: Re: [PATCH V4 03/14] DMA: PL330: Support DMA_SLAVE_CONFIG
> > > command
> > >
> > > On Mon, 2011-07-25 at 10:28 +0900, Boojin Kim wrote:
> > > > This patch adds support DMA_SLAVE_CONFIG command.
> > > >
> > > > Signed-off-by: Boojin Kim <boojin.kim@samsung.com>
> > > > ---
> > > >  drivers/dma/pl330.c |   53
> +++++++++++++++++++++++++++++++++++++----
> > > ---------
> > > >  1 files changed, 39 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> > > > index 586ab39..880f010 100644
> > > > --- a/drivers/dma/pl330.c
> > > > +++ b/drivers/dma/pl330.c
> > > > @@ -256,25 +256,50 @@ static int
> pl330_alloc_chan_resources(struct
> > > dma_chan *chan)
> > > >  static int pl330_control(struct dma_chan *chan, enum
> dma_ctrl_cmd
> > > cmd, unsigned long arg)
> > > >  {
> > > >  	struct dma_pl330_chan *pch = to_pchan(chan);
> > > > -	struct dma_pl330_desc *desc;
> > > > +	struct dma_pl330_desc *desc, *_dt;
> > > >  	unsigned long flags;
> > > > +	struct dma_pl330_dmac *pdmac = pch->dmac;
> > > > +	struct dma_slave_config *slave_config;
> > > > +	struct dma_pl330_peri *peri;
> > > > +	LIST_HEAD(list);
> > > >
> > > > -	/* Only supports DMA_TERMINATE_ALL */
> > > > -	if (cmd != DMA_TERMINATE_ALL)
> > > > -		return -ENXIO;
> > > > -
> > > > -	spin_lock_irqsave(&pch->lock, flags);
> > > > -
> > > > -	/* FLUSH the PL330 Channel thread */
> > > > -	pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
> > > > +	switch (cmd) {
> > > > +	case DMA_TERMINATE_ALL:
> > > > +		spin_lock_irqsave(&pch->lock, flags);
> > > >
> > > > -	/* Mark all desc done */
> > > > -	list_for_each_entry(desc, &pch->work_list, node)
> > > > -		desc->status = DONE;
> > > > +		/* FLUSH the PL330 Channel thread */
> > > > +		pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
> > > >
> > > > -	spin_unlock_irqrestore(&pch->lock, flags);
> > > > +		/* Mark all desc done */
> > > > +		list_for_each_entry_safe(desc, _dt, &pch-
> >work_list , node)
> > > {
> > > > +			desc->status = DONE;
> > > > +			pch->completed = desc->txd.cookie;
> > > > +			list_move_tail(&desc->node, &list);
> > > > +		}
> > > >
> > > > -	pl330_tasklet((unsigned long) pch);
> > > > +		list_splice_tail_init(&list, &pdmac->desc_pool);
> > > > +		spin_unlock_irqrestore(&pch->lock, flags);
> > > > +		break;
> > > > +	case DMA_SLAVE_CONFIG:
> > > > +		slave_config = (struct dma_slave_config *)arg;
> > > > +		peri = pch->chan.private;
> > > > +
> > > > +		if (slave_config->direction == DMA_TO_DEVICE) {
> > > > +			if (slave_config->dst_addr)
> > > > +				peri->fifo_addr = slave_config-
> >dst_addr;
> > > > +			if (slave_config->dst_addr_width)
> > > > +				peri->burst_sz = __ffs(slave_config-
> > > >dst_addr_width);
> > > This doesn't sound right, why can't you use xxx_maxburst field
> instead?
> > Can you explain the problem in more detail?
> > Do you mean that 'xxx_maxburst' takes place of 'xxx_addr_width' or
> __ffs()
> > isn't proper here?
> >
> > Additionally, I will modify this code to consider '_maxburst' field.
> __ffs is okay and sensible.
> I meant you should use proper fields for passing info, which in this
> case would be dst_maxburst for specifying destination burst sizes
I think the naming seems to make confusion.
'Peri->burst_sz' on this code means the width of fifo in bytes of the peri. 
It's same with the meaning 'xxx_addr_width'. So, I think it's proper usage.
'peri->burst_sz' is passed to PL330 HW. PL330 HW wants to get the power of 2 
based the fifo width for it.
For example, If fifo width is 8 bytes, PL330 HW should get '3' for it.
And I will modify code to consider xxx_maxburst.


> --
> ~Vinod Koul
> Intel Corp.
Russell King - ARM Linux July 26, 2011, 1:12 p.m. UTC | #5
On Tue, Jul 26, 2011 at 10:04:10PM +0900, Boojin Kim wrote:
> Vinod Koul Wrote:
> > I meant you should use proper fields for passing info, which in this
> > case would be dst_maxburst for specifying destination burst sizes
> I think the naming seems to make confusion.
> 'Peri->burst_sz' on this code means the width of fifo in bytes of the peri. 
> It's same with the meaning 'xxx_addr_width'. So, I think it's proper usage.
> 'peri->burst_sz' is passed to PL330 HW. PL330 HW wants to get the power of 2 
> based the fifo width for it.
> For example, If fifo width is 8 bytes, PL330 HW should get '3' for it.
> And I will modify code to consider xxx_maxburst.

IMHO from your description above, peri->burst_sz is misnamed.  Please
add a comment at this location to say something like:

	/* peri->burst_sz is the peripheral fifo width */

to explain this.

Thanks.

Patch
diff mbox

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 586ab39..880f010 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -256,25 +256,50 @@  static int pl330_alloc_chan_resources(struct dma_chan *chan)
 static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned long arg)
 {
 	struct dma_pl330_chan *pch = to_pchan(chan);
-	struct dma_pl330_desc *desc;
+	struct dma_pl330_desc *desc, *_dt;
 	unsigned long flags;
+	struct dma_pl330_dmac *pdmac = pch->dmac;
+	struct dma_slave_config *slave_config;
+	struct dma_pl330_peri *peri;
+	LIST_HEAD(list);
 
-	/* Only supports DMA_TERMINATE_ALL */
-	if (cmd != DMA_TERMINATE_ALL)
-		return -ENXIO;
-
-	spin_lock_irqsave(&pch->lock, flags);
-
-	/* FLUSH the PL330 Channel thread */
-	pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
+	switch (cmd) {
+	case DMA_TERMINATE_ALL:
+		spin_lock_irqsave(&pch->lock, flags);
 
-	/* Mark all desc done */
-	list_for_each_entry(desc, &pch->work_list, node)
-		desc->status = DONE;
+		/* FLUSH the PL330 Channel thread */
+		pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
 
-	spin_unlock_irqrestore(&pch->lock, flags);
+		/* Mark all desc done */
+		list_for_each_entry_safe(desc, _dt, &pch->work_list , node) {
+			desc->status = DONE;
+			pch->completed = desc->txd.cookie;
+			list_move_tail(&desc->node, &list);
+		}
 
-	pl330_tasklet((unsigned long) pch);
+		list_splice_tail_init(&list, &pdmac->desc_pool);
+		spin_unlock_irqrestore(&pch->lock, flags);
+		break;
+	case DMA_SLAVE_CONFIG:
+		slave_config = (struct dma_slave_config *)arg;
+		peri = pch->chan.private;
+
+		if (slave_config->direction == DMA_TO_DEVICE) {
+			if (slave_config->dst_addr)
+				peri->fifo_addr = slave_config->dst_addr;
+			if (slave_config->dst_addr_width)
+				peri->burst_sz = __ffs(slave_config->dst_addr_width);
+		} else if (slave_config->direction == DMA_FROM_DEVICE) {
+			if (slave_config->src_addr)
+				peri->fifo_addr = slave_config->src_addr;
+			if (slave_config->src_addr_width)
+				peri->burst_sz = __ffs(slave_config->src_addr_width);
+		}
+		break;
+	default:
+		dev_err(pch->dmac->pif.dev, "Not supported command.\n");
+		return -ENXIO;
+	}
 
 	return 0;
 }