| Message ID | 1375719297-12871-2-git-send-email-joelf@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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 --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;
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(-)