diff mbox

[v3,01/12] dma: edma: Setup parameters to DMA MAX_NR_SG at a time

Message ID 1375719297-12871-2-git-send-email-joelf@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Joel Fernandes Aug. 5, 2013, 4:14 p.m. UTC
Changes are made here for configuring existing parameters to support
DMA'ing them out in batches as needed.

Also allocate as many as slots as needed by the SG list, but not more
than MAX_NR_SG. Then these slots will be reused accordingly.
For ex, if MAX_NR_SG=10, and number of SG entries is 40, still only
10 slots will be allocated to DMA the entire SG list of size 40.

Signed-off-by: Joel Fernandes <joelf@ti.com>
---
 drivers/dma/edma.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Sekhar Nori Aug. 12, 2013, 7:15 a.m. UTC | #1
On Monday 05 August 2013 09:44 PM, Joel Fernandes wrote:
> Changes are made here for configuring existing parameters to support
> DMA'ing them out in batches as needed.
> 
> Also allocate as many as slots as needed by the SG list, but not more
> than MAX_NR_SG. Then these slots will be reused accordingly.
> For ex, if MAX_NR_SG=10, and number of SG entries is 40, still only
> 10 slots will be allocated to DMA the entire SG list of size 40.
> 
> Signed-off-by: Joel Fernandes <joelf@ti.com>
> ---
>  drivers/dma/edma.c |   14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
> index 5f3e532..7b0853c 100644
> --- a/drivers/dma/edma.c
> +++ b/drivers/dma/edma.c
> @@ -222,9 +222,9 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
>  	enum dma_slave_buswidth dev_width;
>  	u32 burst;
>  	struct scatterlist *sg;
> -	int i;
>  	int acnt, bcnt, ccnt, src, dst, cidx;
>  	int src_bidx, dst_bidx, src_cidx, dst_cidx;
> +	int i, num_slots_needed;

'nslots' is more to my liking. Better keep variable names short.

>  
>  	if (unlikely(!echan || !sgl || !sg_len))
>  		return NULL;
> @@ -262,8 +262,11 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
>  
>  	edesc->pset_nr = sg_len;
>  
> -	for_each_sg(sgl, sg, sg_len, i) {
> -		/* Allocate a PaRAM slot, if needed */
> +	/* Allocate a PaRAM slot, if needed */
> +
> +	num_slots_needed = sg_len > MAX_NR_SG ? MAX_NR_SG : sg_len;

nslots = min(MAX_NR_SG, sg_len);

> +
> +	for (i = 0; i < num_slots_needed; i++) {
>  		if (echan->slot[i] < 0) {
>  			echan->slot[i] =
>  				edma_alloc_slot(EDMA_CTLR(echan->ch_num),
> @@ -273,6 +276,10 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
>  				return NULL;
>  			}
>  		}
> +	}
> +
> +	/* Configure PaRAM sets for each SG */
> +	for_each_sg(sgl, sg, sg_len, i) {
>  
>  		acnt = dev_width;
>  
> @@ -330,6 +337,7 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
>  		/* Configure A or AB synchronized transfers */
>  		if (edesc->absync)
>  			edesc->pset[i].opt |= SYNCDIM;
> +

Random extra newline.

The patch as such is fine, but I dont think it makes lot of sense
standalone. This needs to be merged into the patch where you actually
handle the entire SG list in batches.

Thanks,
Sekhar
Joel Fernandes Aug. 12, 2013, 11:55 p.m. UTC | #2
Dropped quite a few from the CC list...

On 08/12/2013 02:15 AM, Sekhar Nori wrote:
> On Monday 05 August 2013 09:44 PM, Joel Fernandes wrote:
>> Changes are made here for configuring existing parameters to support
>> DMA'ing them out in batches as needed.
>>
>> Also allocate as many as slots as needed by the SG list, but not more
>> than MAX_NR_SG. Then these slots will be reused accordingly.
>> For ex, if MAX_NR_SG=10, and number of SG entries is 40, still only
>> 10 slots will be allocated to DMA the entire SG list of size 40.
>>
>> Signed-off-by: Joel Fernandes <joelf@ti.com>
>> ---
>>  drivers/dma/edma.c |   14 +++++++++++---
>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
>> index 5f3e532..7b0853c 100644
>> --- a/drivers/dma/edma.c
>> +++ b/drivers/dma/edma.c
>> @@ -222,9 +222,9 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
>>  	enum dma_slave_buswidth dev_width;
>>  	u32 burst;
>>  	struct scatterlist *sg;
>> -	int i;
>>  	int acnt, bcnt, ccnt, src, dst, cidx;
>>  	int src_bidx, dst_bidx, src_cidx, dst_cidx;
>> +	int i, num_slots_needed;
> 
> 'nslots' is more to my liking. Better keep variable names short.
> 
>>  
>>  	if (unlikely(!echan || !sgl || !sg_len))
>>  		return NULL;
>> @@ -262,8 +262,11 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
>>  
>>  	edesc->pset_nr = sg_len;
>>  
>> -	for_each_sg(sgl, sg, sg_len, i) {
>> -		/* Allocate a PaRAM slot, if needed */
>> +	/* Allocate a PaRAM slot, if needed */
>> +
>> +	num_slots_needed = sg_len > MAX_NR_SG ? MAX_NR_SG : sg_len;
> 
> nslots = min(MAX_NR_SG, sg_len);

I agree the original naming was quite long. I would rather using
something more descriptive though than nslots. How does slots_needed sound?

Thanks,

-Joel
Joel Fernandes Aug. 13, 2013, 12:05 a.m. UTC | #3
Responding to other comments in this post,

On 08/12/2013 02:15 AM, Sekhar Nori wrote:

[..]
>>  	if (unlikely(!echan || !sgl || !sg_len))
>>  		return NULL;
>> @@ -262,8 +262,11 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
>>  
>>  	edesc->pset_nr = sg_len;
>>  
>> -	for_each_sg(sgl, sg, sg_len, i) {
>> -		/* Allocate a PaRAM slot, if needed */
>> +	/* Allocate a PaRAM slot, if needed */
>> +
>> +	num_slots_needed = sg_len > MAX_NR_SG ? MAX_NR_SG : sg_len;
> 
> nslots = min(MAX_NR_SG, sg_len);

Changed to this, with the +1

> 
>> +
>> +	for (i = 0; i < num_slots_needed; i++) {
>>  		if (echan->slot[i] < 0) {
>>  			echan->slot[i] =
>>  				edma_alloc_slot(EDMA_CTLR(echan->ch_num),
>> @@ -273,6 +276,10 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
>>  				return NULL;
>>  			}
>>  		}
>> +	}
>> +
>> +	/* Configure PaRAM sets for each SG */
>> +	for_each_sg(sgl, sg, sg_len, i) {
>>  
>>  		acnt = dev_width;
>>  
>> @@ -330,6 +337,7 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
>>  		/* Configure A or AB synchronized transfers */
>>  		if (edesc->absync)
>>  			edesc->pset[i].opt |= SYNCDIM;
>> +
> 
> Random extra newline.

Removing..

> 
> The patch as such is fine, but I dont think it makes lot of sense
> standalone. This needs to be merged into the patch where you actually
> handle the entire SG list in batches.

I think it does actually, this patch just takes care of preparing the
param set list correctly and allocating slots. It doesn't the actual DMA
or take part in the algorithm. As a result, the patch can be reused
incase in future the main algorithm is rewritten in a subsequent series.
Further this patch was reused straight from old implementation so it
proved to be useful being a separate patch last time. I also plan to
rewrite just this functionality in the future.

Thanks,

-Joel
Joel Fernandes Aug. 13, 2013, 12:19 a.m. UTC | #4
On 08/12/2013 06:55 PM, Joel Fernandes wrote:
> Dropped quite a few from the CC list...
> 
> On 08/12/2013 02:15 AM, Sekhar Nori wrote:
>> On Monday 05 August 2013 09:44 PM, Joel Fernandes wrote:
>>> Changes are made here for configuring existing parameters to support
>>> DMA'ing them out in batches as needed.
>>>
>>> Also allocate as many as slots as needed by the SG list, but not more
>>> than MAX_NR_SG. Then these slots will be reused accordingly.
>>> For ex, if MAX_NR_SG=10, and number of SG entries is 40, still only
>>> 10 slots will be allocated to DMA the entire SG list of size 40.
>>>
>>> Signed-off-by: Joel Fernandes <joelf@ti.com>
>>> ---
>>>  drivers/dma/edma.c |   14 +++++++++++---
>>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
>>> index 5f3e532..7b0853c 100644
>>> --- a/drivers/dma/edma.c
>>> +++ b/drivers/dma/edma.c
>>> @@ -222,9 +222,9 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
>>>  	enum dma_slave_buswidth dev_width;
>>>  	u32 burst;
>>>  	struct scatterlist *sg;
>>> -	int i;
>>>  	int acnt, bcnt, ccnt, src, dst, cidx;
>>>  	int src_bidx, dst_bidx, src_cidx, dst_cidx;
>>> +	int i, num_slots_needed;
>>
>> 'nslots' is more to my liking. Better keep variable names short.
>>
>>>  
>>>  	if (unlikely(!echan || !sgl || !sg_len))
>>>  		return NULL;
>>> @@ -262,8 +262,11 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
>>>  
>>>  	edesc->pset_nr = sg_len;
>>>  
>>> -	for_each_sg(sgl, sg, sg_len, i) {
>>> -		/* Allocate a PaRAM slot, if needed */
>>> +	/* Allocate a PaRAM slot, if needed */
>>> +
>>> +	num_slots_needed = sg_len > MAX_NR_SG ? MAX_NR_SG : sg_len;
>>
>> nslots = min(MAX_NR_SG, sg_len);
> 
> I agree the original naming was quite long. I would rather using
> something more descriptive though than nslots. How does slots_needed sound?

Sorry for the noise, nslots is fine and I've changed it to the same.

-Joel
diff mbox

Patch

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 5f3e532..7b0853c 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -222,9 +222,9 @@  static struct dma_async_tx_descriptor *edma_prep_slave_sg(
 	enum dma_slave_buswidth dev_width;
 	u32 burst;
 	struct scatterlist *sg;
-	int i;
 	int acnt, bcnt, ccnt, src, dst, cidx;
 	int src_bidx, dst_bidx, src_cidx, dst_cidx;
+	int i, num_slots_needed;
 
 	if (unlikely(!echan || !sgl || !sg_len))
 		return NULL;
@@ -262,8 +262,11 @@  static struct dma_async_tx_descriptor *edma_prep_slave_sg(
 
 	edesc->pset_nr = sg_len;
 
-	for_each_sg(sgl, sg, sg_len, i) {
-		/* Allocate a PaRAM slot, if needed */
+	/* Allocate a PaRAM slot, if needed */
+
+	num_slots_needed = sg_len > MAX_NR_SG ? MAX_NR_SG : sg_len;
+
+	for (i = 0; i < num_slots_needed; i++) {
 		if (echan->slot[i] < 0) {
 			echan->slot[i] =
 				edma_alloc_slot(EDMA_CTLR(echan->ch_num),
@@ -273,6 +276,10 @@  static struct dma_async_tx_descriptor *edma_prep_slave_sg(
 				return NULL;
 			}
 		}
+	}
+
+	/* Configure PaRAM sets for each SG */
+	for_each_sg(sgl, sg, sg_len, i) {
 
 		acnt = dev_width;
 
@@ -330,6 +337,7 @@  static struct dma_async_tx_descriptor *edma_prep_slave_sg(
 		/* Configure A or AB synchronized transfers */
 		if (edesc->absync)
 			edesc->pset[i].opt |= SYNCDIM;
+
 		/* If this is the last set, enable completion interrupt flag */
 		if (i == sg_len - 1)
 			edesc->pset[i].opt |= TCINTEN;