Re: [PATCH v3 02/12] ARM: edma: Don't clear EMR of channel in edma_stop
On Sun, Aug 11, 2013 at 11:25 PM, Sekhar Nori nsek...@ti.com wrote: On 8/8/2013 5:19 PM, Sekhar Nori wrote: On Monday 05 August 2013 09:44 PM, Joel Fernandes wrote: We certainly don't want error conditions to be cleared any other place but the EDMA error handler, as this will make us 'forget' about missed events we might need to know errors have occurred. This fixes a race condition where the EMR was being cleared by the transfer completion interrupt handler. Basically, what was happening was: Missed event | | V SG1-SG2-SG3-Null \ \__TC Interrupt (Almost same time as ARM is executing TC interrupt handler, an event got missed and also forgotten by clearing the EMR). This causes the following problems: 1. If error interrupt is also pending and TC interrupt clears the EMR by calling edma_stop as has been observed in the edma_callback function, the ARM will execute the error interrupt even though the EMR is clear. As a result, the dma_ccerr_handler returns IRQ_NONE. If this happens enough number of times, IRQ subsystem disables the interrupt thinking its spurious which makes error handler never execute again. 2. Also even if error handler doesn't return IRQ_NONE, the removing of EMR removes the knowledge about which channel had a missed event, and thus a manual trigger on such channels cannot be performed. The EMR is ultimately being cleared by the Error interrupt handler once it is handled so we remove code that does it in edma_stop and allow it to happen there. Signed-off-by: Joel Fernandes jo...@ti.com Queuing this for v3.11 fixes. While committing, I changed the headline to remove capitalization and made it more readable by removing register level details. The new headline is: ARM: edma: don't clear missed events in edma_stop() Forgot to ask, should this be tagged for stable? IOW, how serious is this race in current kernel (without the entire series applied)? I have never observed it myself - so please provide details how easy/difficult it is to hit this condition. The race was uncovered by recent EDMA patch series, So this patch can go in for next kernel release as such, I am not aware of any other DMA user that maybe uncovering the race condition. Thanks, -Joel ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3 08/12] dma: edma: Add one more required slot to MAX slots
On 08/12/2013 08:31 AM, Mark Brown wrote: On Mon, Aug 12, 2013 at 11:26:49AM +0530, Sekhar Nori wrote: No need for a separate patch for this, just do this in the patch where you include the two linked sets. Can you guys please think about the CC lists you're using for these patch serieses? I've certainly no interest in random patches to the DaVinci DMA controllers, and I suspect the same is true for most of the people on the CC list. Sorry. I was actually getting a sense of this. What I did initially was I copied the CC list from last years work on the same series hoping that it would be a complete mail chain. Bad judgement on my part. I should have paid more attention to Mark's rants about his inbox getting spammed with this series in those old threads ;) I guess moving forward I will use get_maintainer.pl and refine my git-send scripts with only interested folks. Any words of advice on when to and when not to CC someone remotely connected to a series is welcome. @Sekhar, let's keep only linux-omap, linux-davinci and few other interested folks on the CC chain for the rest of review on the series. Thanks, -Joel ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3 01/12] dma: edma: Setup parameters to DMA MAX_NR_SG at a time
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 jo...@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
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3 01/12] dma: edma: Setup parameters to DMA MAX_NR_SG at a time
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
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3 01/12] dma: edma: Setup parameters to DMA MAX_NR_SG at a time
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 jo...@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
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3 09/12] dma: edma: Implement multiple linked sets for continuity
On 08/12/2013 01:56 PM, Sekhar Nori wrote:
On Monday 05 August 2013 04:14 PM, Joel Fernandes wrote:
Here we implement splitting up of the total MAX number of slots
available for a channel into 2 cyclically linked sets. Transfer
completion Interrupts are enabled on both linked sets and respective
handler recycles them on completion to process the next linked set.
Both linked sets are cyclically linked to each other to ensure
continuity of DMA operations. Interrupt handlers execute asynchronously
to the EDMA events and recycles the linked sets at the right time,
as a result EDMA is not blocked or dependent on interrupts and DMA
continues till the end of the SG-lists without any interruption.
Suggested-by: Sekhar Nori nsek...@ti.com
Signed-off-by: Joel Fernandes jo...@ti.com
---
drivers/dma/edma.c | 157
+++-
1 file changed, 118 insertions(+), 39 deletions(-)
diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index df50a04..70923a2 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -48,6 +48,7 @@
/* Max of 16 segments per channel to conserve PaRAM slots */
#define MAX_NR_SG 16
+#define MAX_NR_LS (MAX_NR_SG 1)
#define EDMA_MAX_SLOTS (MAX_NR_SG+1)
#define EDMA_DESCRIPTORS16
@@ -57,6 +58,7 @@ struct edma_desc {
int absync;
int pset_nr;
int total_processed;
+int next_setup_linkset;
struct edmacc_param pset[0];
};
@@ -140,7 +142,9 @@ static void edma_execute(struct edma_chan *echan)
struct edma_desc *edesc;
struct device *dev = echan-vchan.chan.device-dev;
-int i, j, total_left, total_process;
+int i, total_left, total_link_set;
+int ls_cur_off, ls_next_off, slot_off;
+struct edmacc_param tmp_param;
/* If either we processed all psets or we're still not started */
if (!echan-edesc ||
@@ -159,48 +163,121 @@ static void edma_execute(struct edma_chan *echan)
/* Find out how many left */
total_left = edesc-pset_nr - edesc-total_processed;
-total_process = total_left MAX_NR_SG ? MAX_NR_SG : total_left;
-
-
-/* Write descriptor PaRAM set(s) */
-for (i = 0; i total_process; i++) {
-j = i + edesc-total_processed;
-edma_write_slot(echan-slot[i], edesc-pset[j]);
-dev_dbg(echan-vchan.chan.device-dev,
-\n pset[%d]:\n
- chnum\t%d\n
- slot\t%d\n
- opt\t%08x\n
- src\t%08x\n
- dst\t%08x\n
- abcnt\t%08x\n
- ccnt\t%08x\n
- bidx\t%08x\n
- cidx\t%08x\n
- lkrld\t%08x\n,
-j, echan-ch_num, echan-slot[i],
-edesc-pset[j].opt,
-edesc-pset[j].src,
-edesc-pset[j].dst,
-edesc-pset[j].a_b_cnt,
-edesc-pset[j].ccnt,
-edesc-pset[j].src_dst_bidx,
-edesc-pset[j].src_dst_cidx,
-edesc-pset[j].link_bcntrld);
-/* Link to the previous slot if not the last set */
-if (i != (total_process - 1))
+total_link_set = total_left MAX_NR_LS ? MAX_NR_LS : total_left;
The name you gave here sounds like this is defining total number of
linked PaRAM sets. Rather this is actually tracking the number of PaRAM
sets (slots) in current linked set, correct? Then may be just call it
'nslots' or even 'num_slots'? There are just too many variables with
total prefix to keep track of in this function!
I would rather just leave this naming alone. The code is quite self
documenting: total_link_set means Calculate what's the total size of a
Linkset, or total no.of slots in a linkset we need. This naming is fine
in my opinion and doesn't hurt line size at all, instead improving code
readability. I could dump the _ between link and set to make it:
total_linkset if that makes it any easier.
I agree there are too many variables in this function, but they each
serve a different purpose and required to implement the algorithm, which
is precisely I made them naming a bit more descriptive.
+
+/* First time, setup 2 cyclically linked sets, each containing half
+ the slots allocated for this channel */
+if (edesc-total_processed == 0) {
We dont need to check for this case for every DMA_COMPLETE interrupt.
May be move the initial setup to another function called from
edma_issue_pending()?
But how? That would only change the code to (?):
if (edesc-total_processed == 0) {
issue_pending();
}
Further it maybe appear that this case is uncommon
Re: [PATCH v3 07/12] dma: edma: Add function to dump a PaRAM set from PaRAM
On 08/12/2013 12:52 AM, Sekhar Nori wrote:
On Monday 05 August 2013 09:44 PM, Joel Fernandes wrote:
Previously, such a dump function was used but it wasn't reading
from the PaRAM, rather just from a edmacc_param structure, we
add a helpful function for debugging that directly reads from
the PaRAM and gives the current state correctly (including links
and interrupt information).
Signed-off-by: Joel Fernandes jo...@ti.com
You should convert existing instances of PaRAM set dump to use this new
function along with introducing it.
Well, the old dump callers were thrown out with completely rewritten
code. This rewritten code already contains the dump calls. Are you
saying pull out those dump pset calls into a separate patch?
Thanks,
-Joel
Sekhar
---
drivers/dma/edma.c | 31 +++
1 file changed, 31 insertions(+)
diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 080d669..a242269 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -102,6 +102,37 @@ static void edma_desc_free(struct virt_dma_desc *vdesc)
kfree(container_of(vdesc, struct edma_desc, vdesc));
}
+static inline void dump_pset(struct edma_chan *echan, int slot,
+ struct edmacc_param *pset_unused, int pset_idx)
+{
+struct edmacc_param pset_local, *pset;
+pset = pset_local;
+
+edma_read_slot(slot, pset);
+
+dev_dbg(echan-vchan.chan.device-dev,
+\n pset[%d]:\n
+ chnum\t%d\n
+ slot\t%d\n
+ opt\t%08x\n
+ src\t%08x\n
+ dst\t%08x\n
+ abcnt\t%08x\n
+ ccnt\t%08x\n
+ bidx\t%08x\n
+ cidx\t%08x\n
+ lkrld\t%08x\n,
+pset_idx, echan-ch_num, slot,
+pset[0].opt,
+pset[0].src,
+pset[0].dst,
+pset[0].a_b_cnt,
+pset[0].ccnt,
+pset[0].src_dst_bidx,
+pset[0].src_dst_cidx,
+pset[0].link_bcntrld);
+}
+
/* Dispatch a queued descriptor to the controller (caller holds lock) */
static void edma_execute(struct edma_chan *echan)
{
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3 08/12] dma: edma: Add one more required slot to MAX slots
On 08/12/2013 12:56 AM, Sekhar Nori wrote: On Monday 05 August 2013 09:44 PM, Joel Fernandes wrote: We'd now need a separate slot just for the channel and separate ones for the 2 linked sets, so we make adjustments to allocate an extra channel accordingly. Signed-off-by: Joel Fernandes jo...@ti.com No need for a separate patch for this, just do this in the patch where you include the two linked sets. Ok, I dropped this patch. Anyway, most of this patch has gone away now because of other changes you suggested. Thanks, -Joel ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3 00/12] edma: Add support for SG lists of any length
Following offline discussions with Sekhar, we discussed some ideas to change a few things in this patch series to make it fail-safe. As such, the only changes we are making for v4 will be to not cyclically link immediately but doing so only once the ISR has finished setup (apart from other style cleanups). Any conditions where we are not able to modify the link in time (due to heavily loaded system) will be detected and reported by the use of linking to a NULL set. The new approach will be fast because of no requirement to stall or wait, and any DMA issues with heavily loaded systems can be detected as error conditions. This should architecturally be the final version of the patch series to add DMA support for SG lists of any length. Thanks, -Joel On 08/05/2013 11:14 AM, Joel Fernandes wrote: Here is a more improved approach for DMA support of SG lists of any length in the EDMA DMA Engine driver. In the previous approach [1] we depended on error interrupts to detect missed events and manually retrigger them, however as discussed in [2], there are concerns this can be trouble some for high speed peripherals which may need a more real-time response from the DMA controller. In this approach, we divide the total no of MAX slots per channel, into 2 linked sets which are cyclically linked to each other (the cyclic link between the 2 sets make sure that the DMA is continuous till the whole SG list has exhausted). We then enable completion interrupts on both linked sets which results in recyling/preparing respective linked set with the next set of SG entries. The interrupt handler executes in parallel while the EDMA controller DMA's the next list. This results in no interruption. Special handling is done for first linked set (as we set up both linked sets initially before starting with the DMA), and last one where the cyclic link has to be broken and a link to the Dummy slot has to be created. Also we keep track of whether all pending DMA operations have completed before we can mark it as complete. [1] https://lkml.org/lkml/2013/7/29/312 [2] https://lkml.org/lkml/2013/7/30/54 Joel Fernandes (12): dma: edma: Setup parameters to DMA MAX_NR_SG at a time ARM: edma: Don't clear EMR of channel in edma_stop dma: edma: remove limits on number of slots dma: edma: Write out and handle MAX_NR_SG at a given time ARM: edma: Add function to enable interrupt for a PaRAM slot ARM: edma: Add pr_debug in edma_link dma: edma: Add function to dump a PaRAM set from PaRAM dma: edma: Add one more required slot to MAX slots dma: edma: Implement multiple linked sets for continuity dma: edma: Check if MAX_NR_SG is even in prep function dma: edma: Keep tracking of Pending interrupts (pending_acks) dma: edma: Return if nothing left todo in edma_execute arch/arm/common/edma.c | 18 ++- drivers/dma/edma.c | 279 +--- include/linux/platform_data/edma.h |1 + 3 files changed, 243 insertions(+), 55 deletions(-) ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
[PATCH v2] ARM: EDMA: Fix clearing of unused list for DT DMA resources
HWMOD removal for MMC and Crypto is breaking edma_start as the events are
being manually triggered due to unused channel list not being clear. Atleast
breakage has been seen on these peripherals, but it is expected Audio (McASP)
maybe breaking too.
This patch fixes the issue, by reading the dmas property from the DT node if
it exists and clearing the bits in the unused channel list so that these
channels
are not manually triggered.
v2 changes:
Reduced indendation by returning from if block.
Reviewed-by: Sekhar Nori nsek...@ti.com
Reported-by: Balaji T K balaj...@ti.com
Cc: Pantel Antoniou pa...@antoniou-consulting.com
Signed-off-by: Joel Fernandes jo...@ti.com
---
Note:
Patch should go in for -rc cycle as it fixes existing crypto drivers.
arch/arm/common/edma.c | 22 +++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 39ad030..3867e7e 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -560,14 +560,30 @@ static int reserve_contiguous_slots(int ctlr, unsigned
int id,
static int prepare_unused_channel_list(struct device *dev, void *data)
{
struct platform_device *pdev = to_platform_device(dev);
- int i, ctlr;
+ int i = 0, ctlr;
+ u32 dma_chan;
+ const __be32 *dma_chan_p;
+ struct property *prop;
+
+ if (dev-of_node) {
+ of_property_for_each_u32(dev-of_node, dmas, prop,
+dma_chan_p, dma_chan) {
+ if (i++ 1) {
+ ctlr = EDMA_CTLR(dma_chan);
+ clear_bit(EDMA_CHAN_SLOT(dma_chan),
+ edma_cc[ctlr]-edma_unused);
+ }
+ }
+ return;
+ }
- for (i = 0; i pdev-num_resources; i++) {
+ /* For non-OF case */
+ for (; i pdev-num_resources; i++) {
if ((pdev-resource[i].flags IORESOURCE_DMA)
(int)pdev-resource[i].start = 0) {
ctlr = EDMA_CTLR(pdev-resource[i].start);
clear_bit(EDMA_CHAN_SLOT(pdev-resource[i].start),
- edma_cc[ctlr]-edma_unused);
+ edma_cc[ctlr]-edma_unused);
}
}
--
1.8.1.2
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v2] ARM: EDMA: Fix clearing of unused list for DT DMA resources
On 08/26/2013 05:46 AM, Sekhar Nori wrote:
On Saturday 24 August 2013 01:23 AM, Joel Fernandes wrote:
HWMOD removal for MMC and Crypto is breaking edma_start as the events are
being manually triggered due to unused channel list not being clear. Atleast
breakage has been seen on these peripherals, but it is expected Audio (McASP)
maybe breaking too.
This patch fixes the issue, by reading the dmas property from the DT node
if
it exists and clearing the bits in the unused channel list so that these
channels
are not manually triggered.
v2 changes:
Reduced indendation by returning from if block.
Is this a v2 or v3 since you already sent a v2 about a month back?
No, there aren't any changes since v2, I just resubmitted the same patch.
Reviewed-by: Sekhar Nori nsek...@ti.com
Reported-by: Balaji T K balaj...@ti.com
Cc: Pantel Antoniou pa...@antoniou-consulting.com
Signed-off-by: Joel Fernandes jo...@ti.com
---
Note:
Patch should go in for -rc cycle as it fixes existing crypto drivers.
We agreed the patch is not needed in -rc cycle since there are no
current EDMA users in DT-boot?
There is now, crypto and EDMA DT patches are being merged in.
arch/arm/common/edma.c | 22 +++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 39ad030..3867e7e 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -560,14 +560,30 @@ static int reserve_contiguous_slots(int ctlr, unsigned
int id,
static int prepare_unused_channel_list(struct device *dev, void *data)
{
struct platform_device *pdev = to_platform_device(dev);
-int i, ctlr;
+int i = 0, ctlr;
+u32 dma_chan;
+const __be32 *dma_chan_p;
+struct property *prop;
+
+if (dev-of_node) {
+of_property_for_each_u32(dev-of_node, dmas, prop,
+ dma_chan_p, dma_chan) {
+if (i++ 1) {
+ctlr = EDMA_CTLR(dma_chan);
+clear_bit(EDMA_CHAN_SLOT(dma_chan),
+ edma_cc[ctlr]-edma_unused);
+}
I thought we agreed to do this differently using
of_property_count_strings() and of_parse_phandle_with_args(). I seemed
to have missed any discussion on why this cannot be done (if such a
discussion took place on the list).
I kind of missed that particular review comment after reading [1]. Because I
thought only change left was the indentation. Let me work on that comment and
resubmit as v3.
Regards,
-Joel
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
[PATCH] ARM: EDMA: Fix clearing of unused list for DT DMA resources
HWMOD removal for MMC is breaking edma_start as the events are being manually
triggered due to unused channel list not being clear.
This patch fixes the issue, by reading the dmas property from the DT node if
it exists and clearing the bits in the unused channel list. For this purpose
we use the of_* helpers to parse the arguments in the dmas phandle list.
Reviewed-by: Sekhar Nori nsek...@ti.com
Reported-by: Balaji T K balaj...@ti.com
Cc: Pantel Antoniou pa...@antoniou-consulting.com
Signed-off-by: Joel Fernandes jo...@ti.com
---
Changes since v1, in v2 and v3:
- Reduced indentation of non-of case by returning from of-case
- Using of_* helpers for parsing
Note:
This patch should go into the merge window as it is a critical bug fix.
arch/arm/common/edma.c | 23 +--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 39ad030..43c7b22 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -560,14 +560,33 @@ static int reserve_contiguous_slots(int ctlr, unsigned
int id,
static int prepare_unused_channel_list(struct device *dev, void *data)
{
struct platform_device *pdev = to_platform_device(dev);
- int i, ctlr;
+ int i, count, ctlr;
+ struct of_phandle_args dma_spec;
+ if (dev-of_node) {
+ count = of_property_count_strings(dev-of_node, dma-names);
+ if (count 0)
+ return 0;
+ for (i = 0; i count; i++) {
+ if (of_parse_phandle_with_args(dev-of_node, dmas,
+ #dma-cells, i,
+ dma_spec))
+ continue;
+
+ ctlr = EDMA_CTLR(dma_spec.args[0]);
+ clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]),
+ edma_cc[ctlr]-edma_unused);
+ }
+ return 0;
+ }
+
+ /* For non-OF case */
for (i = 0; i pdev-num_resources; i++) {
if ((pdev-resource[i].flags IORESOURCE_DMA)
(int)pdev-resource[i].start = 0) {
ctlr = EDMA_CTLR(pdev-resource[i].start);
clear_bit(EDMA_CHAN_SLOT(pdev-resource[i].start),
- edma_cc[ctlr]-edma_unused);
+ edma_cc[ctlr]-edma_unused);
}
}
--
1.8.1.2
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
[PATCH v4 3/6] ARM: edma: Add function to manually trigger an EDMA channel
Manual trigger for events missed as a result of splitting a
scatter gather list and DMA'ing it in batches. Add a helper
function to trigger a channel incase any such events are missed.
Signed-off-by: Joel Fernandes jo...@ti.com
---
arch/arm/common/edma.c | 17 +
include/linux/platform_data/edma.h | 2 ++
2 files changed, 19 insertions(+)
diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 3567ba1..67e8cf5 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -1236,6 +1236,23 @@ void edma_resume(unsigned channel)
}
EXPORT_SYMBOL(edma_resume);
+int edma_trigger_channel(unsigned channel)
+{
+ unsigned ctlr;
+ unsigned int mask;
+
+ ctlr = EDMA_CTLR(channel);
+ channel = EDMA_CHAN_SLOT(channel);
+ mask = BIT(channel 0x1f);
+
+ edma_shadow0_write_array(ctlr, SH_ESR, (channel 5), mask);
+
+ pr_debug(EDMA: ESR%d %08x\n, (channel 5),
+edma_shadow0_read_array(ctlr, SH_ESR, (channel 5)));
+ return 0;
+}
+EXPORT_SYMBOL(edma_trigger_channel);
+
/**
* edma_start - start dma on a channel
* @channel: channel being activated
diff --git a/include/linux/platform_data/edma.h
b/include/linux/platform_data/edma.h
index 57300fd..179fb91 100644
--- a/include/linux/platform_data/edma.h
+++ b/include/linux/platform_data/edma.h
@@ -180,4 +180,6 @@ struct edma_soc_info {
const s16 (*xbar_chans)[2];
};
+int edma_trigger_channel(unsigned);
+
#endif
--
1.8.1.2
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
[PATCH v4 0/6] dma: edma: Support scatter-lists of any length
The following series adds support to EDMA driver to enable DMA of scatter-gather lists of arbitrary length, but still make use of only a certain MAX number of slots at a time for a given channel. Thus free-ing up the rest of the slots to other slaves/channels. With this there is no need for slave drivers to query the EDMA driver about how much is the MAX it can send at a time as done in [1]. Drivers can send SG lists of any number of entries to DMA. Reference discussion at [2]. With this, all the patches for MMC and EDMA related to sg limits can be dropped. Tested omap-aes and omap_hsmmc drivers with different MAX number of slots, even just 1. In the case where it is 1, only 1-slot is used to DMA an entire scatter list of arbitrary length. Since this series touches EDMA private API code also shared with davinci-pcm, playback of a 16-bit 44.1KHz audio file with davinci-pcm has been tested. Sample test run with 1 vs 16 (MAX number of slots/SG) in omap-aes driver: MAX slots = 1: (128 bit key, 8192 byte blocks): 1266 operations in 1 seconds (10371072 bytes) MAX slots = 16: (128 bit key, 8192 byte blocks): 1601 operations in 1 seconds (13115392 bytes) Note: For the above test, 8K buffer is mapped into SG list of size 2 so only 2 slots are required. So beyond size 2, there will not be any noticeable performance improvement. But above experiment just shows as proof of concept that even using 1 slot is managed by just DMA'ing 1 SG entry at a time. Patch series history: v1: The initial patch series v2: Several style related cleanups. v3: Rewrote major portions of the series to avoid usage of error interrupts. v4: Go back to v1: After performing tests, it is determined that the v1 series is a good enough approach and that v3 triggers problems in certain cases. Tests have shown that there is not any noticeable performance difference between using vs not using error interrupts. v4 also squashes some patches in v1. At this point, we are freezing the overall architecture of the patch series to v4. [1] https://lkml.org/lkml/2013/7/18/432 [2] http://marc.info/?l=linux-omapm=137416733628831w=2 Joel Fernandes (6): dma: edma: Setup parameters to DMA MAX_NR_SG at a time dma: edma: Write out and handle MAX_NR_SG at a given time ARM: edma: Add function to manually trigger an EDMA channel dma: edma: Find missed events and issue them dma: edma: Leave linked to Null slot instead of DUMMY slot dma: edma: Remove limits on number of slots arch/arm/common/edma.c | 17 drivers/dma/edma.c | 164 - include/linux/platform_data/edma.h | 2 + 3 files changed, 144 insertions(+), 39 deletions(-) -- 1.8.1.2 ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
[PATCH v4 1/6] dma: edma: Setup parameters to DMA MAX_NR_SG at a time
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.
Also enable TC interrupts for slots that are a last in a current
iteration, or that fall on a MAX_NR_SG boundary.
Signed-off-by: Joel Fernandes jo...@ti.com
---
drivers/dma/edma.c | 18 +++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 5f3e532..e522ad5 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, nslots;
if (unlikely(!echan || !sgl || !sg_len))
return NULL;
@@ -262,8 +262,10 @@ 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 */
+ nslots = min_t(unsigned, MAX_NR_SG, sg_len);
+
+ for (i = 0; i nslots; i++) {
if (echan-slot[i] 0) {
echan-slot[i] =
edma_alloc_slot(EDMA_CTLR(echan-ch_num),
@@ -273,6 +275,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 +336,12 @@ 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 in a current SG set of transactions,
+ enable interrupts so that next set is processed */
+ if (!((i+1) % MAX_NR_SG))
+ edesc-pset[i].opt |= TCINTEN;
+
/* If this is the last set, enable completion interrupt flag */
if (i == sg_len - 1)
edesc-pset[i].opt |= TCINTEN;
--
1.8.1.2
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
[PATCH v4 2/6] dma: edma: Write out and handle MAX_NR_SG at a given time
Process SG-elements in batches of MAX_NR_SG if they are greater
than MAX_NR_SG. Due to this, at any given time only those many
slots will be used in the given channel no matter how long the
scatter list is. We keep track of how much has been written
inorder to process the next batch of elements in the scatter-list
and detect completion.
For such intermediate transfer completions (one batch of MAX_NR_SG),
make use of pause and resume functions instead of start and stop
when such intermediate transfer is in progress or completed as we
donot want to clear any pending events.
Signed-off-by: Joel Fernandes jo...@ti.com
---
drivers/dma/edma.c | 79 --
1 file changed, 53 insertions(+), 26 deletions(-)
diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index e522ad5..732829b 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -56,6 +56,7 @@ struct edma_desc {
struct list_headnode;
int absync;
int pset_nr;
+ int processed;
struct edmacc_param pset[0];
};
@@ -104,22 +105,34 @@ static void edma_desc_free(struct virt_dma_desc *vdesc)
/* Dispatch a queued descriptor to the controller (caller holds lock) */
static void edma_execute(struct edma_chan *echan)
{
- struct virt_dma_desc *vdesc = vchan_next_desc(echan-vchan);
+ struct virt_dma_desc *vdesc;
struct edma_desc *edesc;
- int i;
-
- if (!vdesc) {
- echan-edesc = NULL;
- return;
+ struct device *dev = echan-vchan.chan.device-dev;
+ int i, j, left, nslots;
+
+ /* If either we processed all psets or we're still not started */
+ if (!echan-edesc ||
+ echan-edesc-pset_nr == echan-edesc-processed) {
+ /* Get next vdesc */
+ vdesc = vchan_next_desc(echan-vchan);
+ if (!vdesc) {
+ echan-edesc = NULL;
+ return;
+ }
+ list_del(vdesc-node);
+ echan-edesc = to_edma_desc(vdesc-tx);
}
- list_del(vdesc-node);
+ edesc = echan-edesc;
- echan-edesc = edesc = to_edma_desc(vdesc-tx);
+ /* Find out how many left */
+ left = edesc-pset_nr - edesc-processed;
+ nslots = min(MAX_NR_SG, left);
/* Write descriptor PaRAM set(s) */
- for (i = 0; i edesc-pset_nr; i++) {
- edma_write_slot(echan-slot[i], edesc-pset[i]);
+ for (i = 0; i nslots; i++) {
+ j = i + edesc-processed;
+ edma_write_slot(echan-slot[i], edesc-pset[j]);
dev_dbg(echan-vchan.chan.device-dev,
\n pset[%d]:\n
chnum\t%d\n
@@ -132,24 +145,31 @@ static void edma_execute(struct edma_chan *echan)
bidx\t%08x\n
cidx\t%08x\n
lkrld\t%08x\n,
- i, echan-ch_num, echan-slot[i],
- edesc-pset[i].opt,
- edesc-pset[i].src,
- edesc-pset[i].dst,
- edesc-pset[i].a_b_cnt,
- edesc-pset[i].ccnt,
- edesc-pset[i].src_dst_bidx,
- edesc-pset[i].src_dst_cidx,
- edesc-pset[i].link_bcntrld);
+ j, echan-ch_num, echan-slot[i],
+ edesc-pset[j].opt,
+ edesc-pset[j].src,
+ edesc-pset[j].dst,
+ edesc-pset[j].a_b_cnt,
+ edesc-pset[j].ccnt,
+ edesc-pset[j].src_dst_bidx,
+ edesc-pset[j].src_dst_cidx,
+ edesc-pset[j].link_bcntrld);
/* Link to the previous slot if not the last set */
- if (i != (edesc-pset_nr - 1))
+ if (i != (nslots - 1))
edma_link(echan-slot[i], echan-slot[i+1]);
/* Final pset links to the dummy pset */
else
edma_link(echan-slot[i], echan-ecc-dummy_slot);
}
- edma_start(echan-ch_num);
+ edesc-processed += nslots;
+
+ edma_resume(echan-ch_num);
+
+ if (edesc-processed = MAX_NR_SG) {
+ dev_dbg(dev, first transfer starting %d\n, echan-ch_num);
+ edma_start(echan-ch_num);
+ }
}
static int edma_terminate_all(struct edma_chan *echan)
@@ -368,19 +388,26 @@ static void edma_callback(unsigned ch_num, u16 ch_status,
void *data)
struct edma_desc *edesc;
unsigned long flags;
- /* Stop the channel */
- edma_stop(echan-ch_num);
+ /* Pause the channel */
+ edma_pause(echan-ch_num);
switch (ch_status) {
case DMA_COMPLETE
[PATCH v4 5/6] dma: edma: Leave linked to Null slot instead of DUMMY slot
Dummy slot has been used as a way for missed-events not to be
reported as missing. This has been particularly troublesome for cases
where we might want to temporarily pause all incoming events.
For EDMA DMAC, there is no way to do any such pausing of events as
the occurence of the next event is not software controlled.
Using edma_pause in IRQ handlers doesn't help as by then the event
in concern from the slave is already missed.
Linking a dummy slot, is seen to absorb these events which we didn't
want to miss. So we don't link to dummy, but instead leave it linked
to NULL set, allow an error condition and detect the channel that
missed it.
Signed-off-by: Joel Fernandes jo...@ti.com
dma: edma: Link to dummy slot only for last SG list split
Consider the case where we have a scatter-list like:
SG1-SG2-SG3-SG4-SG5-SG6-Null
For ex, for a MAX_NR_SG of 2, earlier we were splitting this as:
SG1-SG2-Null
SG3-SG4-Null
SG5-SG6-Null
Now we split it as
SG1-SG2-Null
SG3-SG4-Null
SG5-SG6-Dummy
This approach results in lesser unwanted interrupts that occur
for the last list split. The Dummy slot has the property of not
raising an error condition if events are missed unlike the Null
slot. We are OK with this as we're done with processing the
whole list once we reach Dummy.
Signed-off-by: Joel Fernandes jo...@ti.com
---
drivers/dma/edma.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 0e77b57..25e47d4 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -158,13 +158,18 @@ static void edma_execute(struct edma_chan *echan)
/* Link to the previous slot if not the last set */
if (i != (nslots - 1))
edma_link(echan-slot[i], echan-slot[i+1]);
- /* Final pset links to the dummy pset */
- else
- edma_link(echan-slot[i], echan-ecc-dummy_slot);
}
edesc-processed += nslots;
+ /*
+* If this is either the last set in a set of SG-list transactions
+* then setup a link to the dummy slot, this results in all future
+* events being absorbed and that's OK because we're done
+*/
+ if (edesc-processed == edesc-pset_nr)
+ edma_link(echan-slot[nslots-1], echan-ecc-dummy_slot);
+
edma_resume(echan-ch_num);
if (edesc-processed = MAX_NR_SG) {
--
1.8.1.2
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
[PATCH v4 6/6] dma: edma: Remove limits on number of slots
With this series, this check is no longer required and
we shouldn't need to reject drivers DMA'ing more than the
MAX number of slots.
Signed-off-by: Joel Fernandes jo...@ti.com
---
drivers/dma/edma.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 25e47d4..d966413 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -287,12 +287,6 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
return NULL;
}
- if (sg_len MAX_NR_SG) {
- dev_err(dev, Exceeded max SG segments %d %d\n,
- sg_len, MAX_NR_SG);
- return NULL;
- }
-
edesc = kzalloc(sizeof(*edesc) + sg_len *
sizeof(edesc-pset[0]), GFP_ATOMIC);
if (!edesc) {
--
1.8.1.2
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
[PATCH v4 4/6] dma: edma: Find missed events and issue them
In an effort to move to using Scatter gather lists of any size with
EDMA as discussed at [1] instead of placing limitations on the driver,
we work through the limitations of the EDMAC hardware to find missed
events and issue them.
The sequence of events that require this are:
For the scenario where MAX slots for an EDMA channel is 3:
SG1 - SG2 - SG3 - SG4 - SG5 - SG6 - Null
The above SG list will have to be DMA'd in 2 sets:
(1) SG1 - SG2 - SG3 - Null
(2) SG4 - SG5 - SG6 - Null
After (1) is succesfully transferred, the events from the MMC controller
donot stop coming and are missed by the time we have setup the transfer
for (2). So here, we catch the events missed as an error condition and
issue them manually.
In the second part of the patch, we make handle the NULL slot cases:
For crypto IP, we continue to receive events even continuously in
NULL slot, the setup of the next set of SG elements happens after
the error handler executes. This is results in some recursion problems.
Due to this, we continously receive error interrupts when we manually
trigger an event from the error handler.
We fix this, by first detecting if the Channel is currently transferring
from a NULL slot or not, that's where the edma_read_slot in the error
callback from interrupt handler comes in. With this we can determine if
the set up of the next SG list has completed, and we manually trigger
only in this case. If the setup has _not_ completed, we are still in NULL
so we just set a missed flag and allow the manual triggerring to happen
in edma_execute which will be eventually called. This fixes the above
mentioned race conditions seen with the crypto drivers.
[1] http://marc.info/?l=linux-omapm=137416733628831w=2
Signed-off-by: Joel Fernandes jo...@ti.com
---
drivers/dma/edma.c | 50 +-
1 file changed, 49 insertions(+), 1 deletion(-)
diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 732829b..0e77b57 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -70,6 +70,7 @@ struct edma_chan {
int ch_num;
boolalloced;
int slot[EDMA_MAX_SLOTS];
+ int missed;
struct dma_slave_config cfg;
};
@@ -170,6 +171,20 @@ static void edma_execute(struct edma_chan *echan)
dev_dbg(dev, first transfer starting %d\n, echan-ch_num);
edma_start(echan-ch_num);
}
+
+ /*
+* This happens due to setup times between intermediate transfers
+* in long SG lists which have to be broken up into transfers of
+* MAX_NR_SG
+*/
+ if (echan-missed) {
+ dev_dbg(dev, missed event in execute detected\n);
+ edma_clean_channel(echan-ch_num);
+ edma_stop(echan-ch_num);
+ edma_start(echan-ch_num);
+ edma_trigger_channel(echan-ch_num);
+ echan-missed = 0;
+ }
}
static int edma_terminate_all(struct edma_chan *echan)
@@ -387,6 +402,7 @@ static void edma_callback(unsigned ch_num, u16 ch_status,
void *data)
struct device *dev = echan-vchan.chan.device-dev;
struct edma_desc *edesc;
unsigned long flags;
+ struct edmacc_param p;
/* Pause the channel */
edma_pause(echan-ch_num);
@@ -414,7 +430,39 @@ static void edma_callback(unsigned ch_num, u16 ch_status,
void *data)
break;
case DMA_CC_ERROR:
- dev_dbg(dev, transfer error on channel %d\n, ch_num);
+ spin_lock_irqsave(echan-vchan.lock, flags);
+
+ edma_read_slot(EDMA_CHAN_SLOT(echan-slot[0]), p);
+
+ /*
+* Issue later based on missed flag which will be sure
+* to happen as:
+* (1) we finished transmitting an intermediate slot and
+* edma_execute is coming up.
+* (2) or we finished current transfer and issue will
+* call edma_execute.
+*
+* Important note: issuing can be dangerous here and
+* lead to some nasty recursion when we are in a NULL
+* slot. So we avoid doing so and set the missed flag.
+*/
+ if (p.a_b_cnt == 0 p.ccnt == 0) {
+ dev_dbg(dev, Error occurred, looks like slot is null,
just setting miss\n);
+ echan-missed = 1;
+ } else {
+ /*
+* The slot is already programmed but the event got
+* missed, so its safe to issue it here.
+*/
+ dev_dbg(dev, Error occurred but slot is non-null,
TRIGGERING\n);
+ edma_clean_channel(echan-ch_num);
+ edma_stop(echan-ch_num
Re: [PATCH v4 2/6] dma: edma: Write out and handle MAX_NR_SG at a given time
On 09/02/2013 11:08 PM, Vinod Koul wrote:
On Thu, Aug 29, 2013 at 06:05:41PM -0500, Joel Fernandes wrote:
Process SG-elements in batches of MAX_NR_SG if they are greater
than MAX_NR_SG. Due to this, at any given time only those many
slots will be used in the given channel no matter how long the
scatter list is. We keep track of how much has been written
inorder to process the next batch of elements in the scatter-list
and detect completion.
For such intermediate transfer completions (one batch of MAX_NR_SG),
make use of pause and resume functions instead of start and stop
when such intermediate transfer is in progress or completed as we
donot want to clear any pending events.
Signed-off-by: Joel Fernandes jo...@ti.com
---
drivers/dma/edma.c | 79
--
1 file changed, 53 insertions(+), 26 deletions(-)
diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index e522ad5..732829b 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -56,6 +56,7 @@ struct edma_desc {
struct list_headnode;
int absync;
int pset_nr;
+int processed;
struct edmacc_param pset[0];
};
@@ -104,22 +105,34 @@ static void edma_desc_free(struct virt_dma_desc *vdesc)
/* Dispatch a queued descriptor to the controller (caller holds lock) */
static void edma_execute(struct edma_chan *echan)
{
-struct virt_dma_desc *vdesc = vchan_next_desc(echan-vchan);
+struct virt_dma_desc *vdesc;
struct edma_desc *edesc;
-int i;
-
-if (!vdesc) {
-echan-edesc = NULL;
-return;
+struct device *dev = echan-vchan.chan.device-dev;
+int i, j, left, nslots;
+
+/* If either we processed all psets or we're still not started */
+if (!echan-edesc ||
+echan-edesc-pset_nr == echan-edesc-processed) {
+/* Get next vdesc */
+vdesc = vchan_next_desc(echan-vchan);
+if (!vdesc) {
+echan-edesc = NULL;
+return;
+}
+list_del(vdesc-node);
+echan-edesc = to_edma_desc(vdesc-tx);
}
-list_del(vdesc-node);
+edesc = echan-edesc;
-echan-edesc = edesc = to_edma_desc(vdesc-tx);
+/* Find out how many left */
+left = edesc-pset_nr - edesc-processed;
+nslots = min(MAX_NR_SG, left);
/* Write descriptor PaRAM set(s) */
-for (i = 0; i edesc-pset_nr; i++) {
-edma_write_slot(echan-slot[i], edesc-pset[i]);
+for (i = 0; i nslots; i++) {
+j = i + edesc-processed;
+edma_write_slot(echan-slot[i], edesc-pset[j]);
dev_dbg(echan-vchan.chan.device-dev,
\n pset[%d]:\n
chnum\t%d\n
@@ -132,24 +145,31 @@ static void edma_execute(struct edma_chan *echan)
bidx\t%08x\n
cidx\t%08x\n
lkrld\t%08x\n,
-i, echan-ch_num, echan-slot[i],
-edesc-pset[i].opt,
-edesc-pset[i].src,
-edesc-pset[i].dst,
-edesc-pset[i].a_b_cnt,
-edesc-pset[i].ccnt,
-edesc-pset[i].src_dst_bidx,
-edesc-pset[i].src_dst_cidx,
-edesc-pset[i].link_bcntrld);
+j, echan-ch_num, echan-slot[i],
+edesc-pset[j].opt,
+edesc-pset[j].src,
+edesc-pset[j].dst,
+edesc-pset[j].a_b_cnt,
+edesc-pset[j].ccnt,
+edesc-pset[j].src_dst_bidx,
+edesc-pset[j].src_dst_cidx,
+edesc-pset[j].link_bcntrld);
/* Link to the previous slot if not the last set */
-if (i != (edesc-pset_nr - 1))
+if (i != (nslots - 1))
edma_link(echan-slot[i], echan-slot[i+1]);
/* Final pset links to the dummy pset */
else
edma_link(echan-slot[i], echan-ecc-dummy_slot);
}
-edma_start(echan-ch_num);
+edesc-processed += nslots;
+
+edma_resume(echan-ch_num);
+
+if (edesc-processed = MAX_NR_SG) {
+dev_dbg(dev, first transfer starting %d\n, echan-ch_num);
+edma_start(echan-ch_num);
+}
}
static int edma_terminate_all(struct edma_chan *echan)
@@ -368,19 +388,26 @@ static void edma_callback(unsigned ch_num, u16
ch_status, void *data)
struct edma_desc *edesc;
unsigned long flags;
-/* Stop the channel */
-edma_stop(echan-ch_num);
+/* Pause the channel */
+edma_pause(echan-ch_num);
switch (ch_status) {
case DMA_COMPLETE:
-dev_dbg(dev, transfer
Re: [PATCH v2] ARM: EDMA: Fix clearing of unused list for DT DMA resources
On 09/06/2013 02:15 PM, Mark Jackson wrote:
On 06/09/13 20:13, Mark Jackson wrote:
On 23/08/13 20:53, Joel Fernandes wrote:
HWMOD removal for MMC and Crypto is breaking edma_start as the events are
being manually triggered due to unused channel list not being clear. Atleast
breakage has been seen on these peripherals, but it is expected Audio
(McASP)
maybe breaking too.
This patch fixes the issue, by reading the dmas property from the DT node
if
it exists and clearing the bits in the unused channel list so that these
channels
are not manually triggered.
v2 changes:
Reduced indendation by returning from if block.
Reviewed-by: Sekhar Nori nsek...@ti.com
Reported-by: Balaji T K balaj...@ti.com
Cc: Pantel Antoniou pa...@antoniou-consulting.com
Signed-off-by: Joel Fernandes jo...@ti.com
---
Note:
Patch should go in for -rc cycle as it fixes existing crypto drivers.
arch/arm/common/edma.c | 22 +++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 39ad030..3867e7e 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -560,14 +560,30 @@ static int reserve_contiguous_slots(int ctlr,
unsigned int id,
static int prepare_unused_channel_list(struct device *dev, void *data)
{
struct platform_device *pdev = to_platform_device(dev);
- int i, ctlr;
+ int i = 0, ctlr;
+ u32 dma_chan;
+ const __be32 *dma_chan_p;
+ struct property *prop;
+
+ if (dev-of_node) {
+ of_property_for_each_u32(dev-of_node, dmas, prop,
+dma_chan_p, dma_chan) {
+ if (i++ 1) {
+ ctlr = EDMA_CTLR(dma_chan);
+ clear_bit(EDMA_CHAN_SLOT(dma_chan),
+ edma_cc[ctlr]-edma_unused);
+ }
+ }
+ return;
This should return something here, otherwise we get:-
arch/arm/common/edma.c: In function 'prepare_unused_channel_list':
arch/arm/common/edma.c:577:3: warning: 'return' with no value, in function
returning non-void [-Wreturn-type]
Other than that I can confirm it fixes the issue for me ...
Thanks for pointing that out. Will fix it in the next revision.
Regards,
-Joel
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
[PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
HWMOD removal for MMC is breaking edma_start as the events are being manually
triggered due to unused channel list not being clear.
This patch fixes the issue, by reading the dmas property from the DT node if
it exists and clearing the bits in the unused channel list. For this purpose
we use the of_* helpers to parse the arguments in the dmas phandle list.
Reviewed-by: Sekhar Nori nsek...@ti.com
Reported-by: Balaji T K balaj...@ti.com
Cc: Pantel Antoniou pa...@antoniou-consulting.com
Signed-off-by: Joel Fernandes jo...@ti.com
---
This patch is a repost of v2 with minor change of return value.
As such this patch fixes DMA breakages on all dependent peripherals (DMA, AES,
MMC, McASP..) so should go in for -rc cycle.
arch/arm/common/edma.c | 22 +++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 117f955..88ea067 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -560,14 +560,30 @@ static int reserve_contiguous_slots(int ctlr, unsigned
int id,
static int prepare_unused_channel_list(struct device *dev, void *data)
{
struct platform_device *pdev = to_platform_device(dev);
- int i, ctlr;
+ int i = 0, ctlr;
+ u32 dma_chan;
+ const __be32 *dma_chan_p;
+ struct property *prop;
+
+ if (dev-of_node) {
+ of_property_for_each_u32(dev-of_node, dmas, prop,
+dma_chan_p, dma_chan) {
+ if (i++ 1) {
+ ctlr = EDMA_CTLR(dma_chan);
+ clear_bit(EDMA_CHAN_SLOT(dma_chan),
+ edma_cc[ctlr]-edma_unused);
+ }
+ }
+ return 0;
+ }
- for (i = 0; i pdev-num_resources; i++) {
+ /* For non-OF case */
+ for (; i pdev-num_resources; i++) {
if ((pdev-resource[i].flags IORESOURCE_DMA)
(int)pdev-resource[i].start = 0) {
ctlr = EDMA_CTLR(pdev-resource[i].start);
clear_bit(EDMA_CHAN_SLOT(pdev-resource[i].start),
- edma_cc[ctlr]-edma_unused);
+ edma_cc[ctlr]-edma_unused);
}
}
--
1.8.1.2
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
On 09/12/2013 04:58 AM, Sekhar Nori wrote:
On Wednesday 11 September 2013 12:22 AM, Joel Fernandes wrote:
HWMOD removal for MMC is breaking edma_start as the events are being manually
triggered due to unused channel list not being clear.
[..]
It is better to send one version with all comments fixed. Helps avoid
confusion.
Ok, here is the final version with all comments fixed, please apply this one:
---8---
From: Joel Fernandes jo...@ti.com
Subject: [PATCH v4] ARM: EDMA: Fix clearing of unused list for DT DMA resources
HWMOD removal for MMC is breaking edma_start as the events are being manually
triggered due to unused channel list not being clear.
This patch fixes the issue, by reading the dmas property from the DT node if
it exists and clearing the bits in the unused channel list. For this purpose
we use the of_* helpers to parse the arguments in the dmas phandle list.
Reviewed-by: Sekhar Nori nsek...@ti.com
Reported-by: Balaji T K balaj...@ti.com
Cc: Pantel Antoniou pa...@antoniou-consulting.com
Signed-off-by: Joel Fernandes jo...@ti.com
---
Changes since v1, in v2 and v3:
- Reduced indentation of non-of case by returning from of-case
- Using of_* helpers for parsing
Note:
This patch should go into the merge window as it is a critical bug fix.
arch/arm/common/edma.c | 23 +--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 39ad030..43c7b22 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -560,14 +560,33 @@ static int reserve_contiguous_slots(int ctlr, unsigned int
id,
static int prepare_unused_channel_list(struct device *dev, void *data)
{
struct platform_device *pdev = to_platform_device(dev);
- int i, ctlr;
+ int i, count, ctlr;
+ struct of_phandle_args dma_spec;
+ if (dev-of_node) {
+ count = of_property_count_strings(dev-of_node, dma-names);
+ if (count 0)
+ return 0;
+ for (i = 0; i count; i++) {
+ if (of_parse_phandle_with_args(dev-of_node, dmas,
+ #dma-cells, i,
+ dma_spec))
+ continue;
+
+ ctlr = EDMA_CTLR(dma_spec.args[0]);
+ clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]),
+ edma_cc[ctlr]-edma_unused);
+ }
+ return 0;
+ }
+
+ /* For non-OF case */
for (i = 0; i pdev-num_resources; i++) {
if ((pdev-resource[i].flags IORESOURCE_DMA)
(int)pdev-resource[i].start = 0) {
ctlr = EDMA_CTLR(pdev-resource[i].start);
clear_bit(EDMA_CHAN_SLOT(pdev-resource[i].start),
- edma_cc[ctlr]-edma_unused);
+ edma_cc[ctlr]-edma_unused);
}
}
--
1.8.1.2
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
On 09/16/2013 06:48 AM, Sekhar Nori wrote:
Hi Joel,
On Saturday 14 September 2013 06:27 AM, Joel Fernandes wrote:
From: Joel Fernandes jo...@ti.com
Subject: [PATCH v4] ARM: EDMA: Fix clearing of unused list for DT DMA
resources
HWMOD removal for MMC is breaking edma_start as the events are being manually
triggered due to unused channel list not being clear.
This patch fixes the issue, by reading the dmas property from the DT node
if
it exists and clearing the bits in the unused channel list. For this purpose
we use the of_* helpers to parse the arguments in the dmas phandle list.
Reviewed-by: Sekhar Nori nsek...@ti.com
Reported-by: Balaji T K balaj...@ti.com
Cc: Pantel Antoniou pa...@antoniou-consulting.com
Signed-off-by: Joel Fernandes jo...@ti.com
---
Changes since v1, in v2 and v3:
- Reduced indentation of non-of case by returning from of-case
- Using of_* helpers for parsing
Note:
This patch should go into the merge window as it is a critical bug fix.
I still cannot find any users of edma in the device tree sources either
in linux-next or linus/master. Why cannot this wait until v3.13?
I understand this affects only DT users of EDMA. But I get so many private
reports of breakage due to this patch not being there that I think it will save
everyone a lot of pain, specially folks creating integration trees to have this
patch available by default.
Further, EDMA DT enabling is surely to go in for 3.13, so its best if this is
applied in advance here.
I feel we shouldn't leave code intentionally broken just because it is not yet
enabled in DTS, specially when it is about to be enabled in DT. For example, a
potential problem is MMC/SD file system corruption due to DMA failure.
arch/arm/common/edma.c | 23 +--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 39ad030..43c7b22 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -560,14 +560,33 @@ static int reserve_contiguous_slots(int ctlr, unsigned
int
id,
static int prepare_unused_channel_list(struct device *dev, void *data)
{
struct platform_device *pdev = to_platform_device(dev);
-int i, ctlr;
+int i, count, ctlr;
+struct of_phandle_args dma_spec;
+if (dev-of_node) {
+count = of_property_count_strings(dev-of_node, dma-names);
+if (count 0)
+return 0;
+for (i = 0; i count; i++) {
+if (of_parse_phandle_with_args(dev-of_node, dmas,
+ #dma-cells, i,
+ dma_spec))
+continue;
This will break for the case where devices on platform bus use non-EDMA
dma controllers like SDMA or CPPI (DRA7x has both EDMA and SDMA on the
same chip). You need to do an additional check to make sure the dma
controller is indeed EDMA. Something like.
Ok, edma is probed earlier so I could never see any problem.
Thanks for pointing this out,
Using the below method is more future-proof than using compatible literal
strings directly. The only problem is the matches table has to be defined
earlier in the sources. What do you think?
if (!of_match_node(edma_of_ids, dma_spec.np) {
of_node_put(dma_spec.np);
continue;
}
if(!of_device_is_compatible(dma_spec.np, ti,edma3))
continue;
Don forget to call of_node_put() on dma_spec.np (something that needs to
be done even with your current code).
Ok, will do.
+
+ctlr = EDMA_CTLR(dma_spec.args[0]);
+clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]),
+ edma_cc[ctlr]-edma_unused);
We don't support the second controller when using DT and the controller
number is not really encoded in the argument to edma phandle. So just
simplify this to:
clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]),
edma_cc[0]-edma_unused);
I think let's not make that assumption just incase in the future we support more
than one EDMA controller for DT-based boot. Is that ok?
+}
+return 0;
+}
+
+/* For non-OF case */
for (i = 0; i pdev-num_resources; i++) {
if ((pdev-resource[i].flags IORESOURCE_DMA)
(int)pdev-resource[i].start = 0) {
ctlr = EDMA_CTLR(pdev-resource[i].start);
clear_bit(EDMA_CHAN_SLOT(pdev-resource[i].start),
-edma_cc[ctlr]-edma_unused);
+ edma_cc[ctlr]-edma_unused);
This is a useful change and I am okay with it happening in this
otherwise unrelated patch, but please mention this in changelog.
Below is the updated version (v5), can
Re: [PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
On 09/17/2013 12:08 AM, Sekhar Nori wrote: [..] I still cannot find any users of edma in the device tree sources either in linux-next or linus/master. Why cannot this wait until v3.13? I understand this affects only DT users of EDMA. But I get so many private reports of breakage due to this patch not being there that I think it will save everyone a lot of pain, specially folks creating integration trees to have this patch available by default. Well, I do agree that the current DT support for EDMA is incomplete without this patch even if there are no in-kernel users of it. I will try sending this for the next -rc if we get to the final version in time and after that its upto the upstreams to take it. Ok, except for the one minor nit below my last scissor patch is good to go. + + ctlr = EDMA_CTLR(dma_spec.args[0]); + clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]), +edma_cc[ctlr]-edma_unused); We don't support the second controller when using DT and the controller number is not really encoded in the argument to edma phandle. So just simplify this to: clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]), edma_cc[0]-edma_unused); I think let's not make that assumption just incase in the future we support more than one EDMA controller for DT-based boot. Is that ok? We don't write future proof code in that _hope_ that it will get used someday. In fact this will confuse the reader into wondering if support for second channel controller is present or not as the code will send Nope I don't agree with this at all.. EDMA CC ctrl will not be hardcoded. We need to write future-proof code to make sure sudden regressions don't pop up when say a second EDMA CC is introduced.. further edma_cc[ctrl] pattern is used all through out the code so what you're asking to do doesn't make much sense in this context. There's no reason to break out of this pattern. It actually will confuse the reader more. Second controller can be present in future. I don't want to come back to change the code when we introduce more than 1 CC which is possible in the future. mixed messages. In short, we aim for consistency with situation today, not tomorrow. What you're asking to do infact breaks consistency with the rest of the code. Besides, I can bet that when second CC support does get added, it is very unlikely that the CC number is get encoded into channel number when using DT. Even if it is not encoded, the data structure for edma_cc is an array and what you're asking is to hardcode the controller number to 0 always. No way I'm going to hard code controller number to a single value. Different topic but anyway why wouldn't ctrl number be encoded in the channel? That's clean, and saves variables and extra structures. Better use of the integer bitmap making up the Ctrl and channel number of small ranges. Regards, -Joel ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
On 09/17/2013 01:05 AM, Sekhar Nori wrote:
[..]
mixed messages. In short, we aim for consistency with situation today,
not tomorrow.
What you're asking to do infact breaks consistency with the rest of the code.
Well, ideally we support second CC even with DT to be consistent all
around. Since that has not happened, I don't want the code to pretend
that it it supports the second channel controller with DT that too only
in parts of code.
Ok, I agree that the bindings don't talk about encoding a controller number in
the channel provided from DT so it shouldn't assume that without binding
updates. Following this suggestion, I've update the patch to the below:
---8---
From: Joel Fernandes jo...@ti.com
Subject: [PATCH v6] ARM: EDMA: Fix clearing of unused list for DT DMA resources
HWMOD removal for MMC is breaking edma_start as the events are being manually
triggered due to unused channel list not being clear.
The above issue is fixed by reading the dmas property from the DT node if it
exists and clearing the bits in the unused channel list if the dma controller
used by any device is EDMA. For this purpose we use the of_* helpers to parse
the arguments in the dmas phandle list.
Also introduced is a minor clean up of a checkpatch error in old code.
Reviewed-by: Sekhar Nori nsek...@ti.com
Reported-by: Balaji T K balaj...@ti.com
Cc: Pantel Antoniou pa...@antoniou-consulting.com
Signed-off-by: Joel Fernandes jo...@ti.com
---
Changes since initial version:
- Using of_node_put on dma_spec's node pointer.
- Update changelog with minor cleanup information.
- For DT-case, set controller number as 0.
- Reduced indentation of non-of case by returning from of-case
- Using of_* helpers for parsing
arch/arm/common/edma.c | 38 +++---
1 file changed, 31 insertions(+), 7 deletions(-)
diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 117f955..8e1a024 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -269,6 +269,11 @@ static const struct edmacc_param dummy_paramset = {
.ccnt = 1,
};
+static const struct of_device_id edma_of_ids[] = {
+ { .compatible = ti,edma3, },
+ {}
+};
+
/*/
static void map_dmach_queue(unsigned ctlr, unsigned ch_no,
@@ -560,14 +565,38 @@ static int reserve_contiguous_slots(int ctlr, unsigned
int id,
static int prepare_unused_channel_list(struct device *dev, void *data)
{
struct platform_device *pdev = to_platform_device(dev);
- int i, ctlr;
+ int i, count, ctlr;
+ struct of_phandle_args dma_spec;
+ if (dev-of_node) {
+ count = of_property_count_strings(dev-of_node, dma-names);
+ if (count 0)
+ return 0;
+ for (i = 0; i count; i++) {
+ if (of_parse_phandle_with_args(dev-of_node, dmas,
+ #dma-cells, i,
+ dma_spec))
+ continue;
+
+ if (!of_match_node(edma_of_ids, dma_spec.np)) {
+ of_node_put(dma_spec.np);
+ continue;
+ }
+
+ clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]),
+ edma_cc[0]-edma_unused);
+ of_node_put(dma_spec.np);
+ }
+ return 0;
+ }
+
+ /* For non-OF case */
for (i = 0; i pdev-num_resources; i++) {
if ((pdev-resource[i].flags IORESOURCE_DMA)
(int)pdev-resource[i].start = 0) {
ctlr = EDMA_CTLR(pdev-resource[i].start);
clear_bit(EDMA_CHAN_SLOT(pdev-resource[i].start),
- edma_cc[ctlr]-edma_unused);
+ edma_cc[ctlr]-edma_unused);
}
}
@@ -1762,11 +1791,6 @@ static int edma_probe(struct platform_device *pdev)
return 0;
}
-static const struct of_device_id edma_of_ids[] = {
- { .compatible = ti,edma3, },
- {}
-};
-
static struct platform_driver edma_driver = {
.driver = {
.name = edma,
--
1.8.1.2
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
[PATCH 1/3] dma: edma: Split out PaRAM set calculations into its own function
PaRAM set calculation is abstracted into its own function to
enable better reuse for other DMA cases such as cyclic. We adapt
the Slave SG case to use the new function.
This provides a much cleaner abstraction to the internals of the
PaRAM set. However, any PaRAM attributes that are not common to
all DMA types must be set separately such as setting of interrupts.
This function takes care of the most-common attributes.
Also added comments clarifying A-sync case calculations.
Signed-off-by: Joel Fernandes jo...@ti.com
---
drivers/dma/edma.c | 198 ++---
1 file changed, 126 insertions(+), 72 deletions(-)
diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index ff50ff4..725b00c 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -250,6 +250,117 @@ static int edma_control(struct dma_chan *chan, enum
dma_ctrl_cmd cmd,
return ret;
}
+/*
+ * A PaRAM set configuration abstraction used by other modes
+ * @chan: Channel who's PaRAM set we're configuring
+ * @pset: PaRAM set to initialize and setup.
+ * @src_addr: Source address of the DMA
+ * @dst_addr: Destination address of the DMA
+ * @burst: In units of dev_width, how much to send
+ * @dev_width: How much is the dev_width
+ * @dma_length: Total length of the DMA transfer
+ * @direction: Direction of the transfer
+ */
+static int edma_config_pset(struct dma_chan *chan, struct edmacc_param *pset,
+ dma_addr_t src_addr, dma_addr_t dst_addr, u32 burst,
+ enum dma_slave_buswidth dev_width, unsigned int dma_length,
+ enum dma_transfer_direction direction)
+{
+ struct edma_chan *echan = to_edma_chan(chan);
+ struct device *dev = chan-device-dev;
+ int acnt, bcnt, ccnt, cidx;
+ int src_bidx, dst_bidx, src_cidx, dst_cidx;
+ int absync;
+
+ acnt = dev_width;
+ /*
+* If the maxburst is equal to the fifo width, use
+* A-synced transfers. This allows for large contiguous
+* buffer transfers using only one PaRAM set.
+*/
+ if (burst == 1) {
+ /*
+* For the A-sync case, bcnt and ccnt are the remainder
+* and quotient respectively of the division of:
+* (dma_length / acnt) by (SZ_64K -1). This is so
+* that in case bcnt over flows, we have ccnt to use.
+* Note: In A-sync tranfer only, bcntrld is used, but it
+* only applies for sg_dma_len(sg) = SZ_64K.
+* In this case, the best way adopted is- bccnt for the
+* first frame will be the remainder below. Then for
+* every successive frame, bcnt will be SZ_64K-1. This
+* is assured as bcntrld = 0x in end of function.
+*/
+ absync = false;
+ ccnt = dma_length / acnt / (SZ_64K - 1);
+ bcnt = dma_length / acnt - ccnt * (SZ_64K - 1);
+ /*
+* If bcnt is non-zero, we have a remainder and hence an
+* extra frame to transfer, so increment ccnt.
+*/
+ if (bcnt)
+ ccnt++;
+ else
+ bcnt = SZ_64K - 1;
+ cidx = acnt;
+ } else {
+ /*
+* If maxburst is greater than the fifo address_width,
+* use AB-synced transfers where A count is the fifo
+* address_width and B count is the maxburst. In this
+* case, we are limited to transfers of C count frames
+* of (address_width * maxburst) where C count is limited
+* to SZ_64K-1. This places an upper bound on the length
+* of an SG segment that can be handled.
+*/
+ absync = true;
+ bcnt = burst;
+ ccnt = dma_length / (acnt * bcnt);
+ if (ccnt (SZ_64K - 1)) {
+ dev_err(dev, Exceeded max SG segment size\n);
+ return -EINVAL;
+ }
+ cidx = acnt * bcnt;
+ }
+
+ if (direction == DMA_MEM_TO_DEV) {
+ src_bidx = acnt;
+ src_cidx = cidx;
+ dst_bidx = 0;
+ dst_cidx = 0;
+ } else if (direction == DMA_DEV_TO_MEM) {
+ src_bidx = 0;
+ src_cidx = 0;
+ dst_bidx = acnt;
+ dst_cidx = cidx;
+ } else {
+ dev_err(dev, %s: direction not implemented yet\n, __func__);
+ return -EINVAL;
+ }
+
+ pset-opt = EDMA_TCC(EDMA_CHAN_SLOT(echan-ch_num));
+ /* Configure A or AB synchronized transfers */
+ if (absync)
+ pset-opt |= SYNCDIM;
+
+ pset-src = src_addr;
+ pset-dst = dst_addr;
+
+ pset-src_dst_bidx = (dst_bidx 16) | src_bidx;
+ pset-src_dst_cidx = (dst_cidx 16) | src_cidx;
+
+ pset
[PATCH 3/3] dma: edma: Increase maximum SG limit to 20
davinci-pcm uses 16 as the no.of periods. With this, in EDMA we have to allocate atleast 17 slots: 1 slot for channel, and 16 slots the periods. Due to this, the MAX_NR_SG limitation causes problems, set it to 20 to make cyclic DMA work when davinci-pcm is converted to use DMA Engine. Also add a comment clarifying this. Signed-off-by: Joel Fernandes jo...@ti.com --- drivers/dma/edma.c | 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c index 9b63e1e..407b496 100644 --- a/drivers/dma/edma.c +++ b/drivers/dma/edma.c @@ -46,8 +46,14 @@ #define EDMA_CHANS 64 #endif /* CONFIG_ARCH_DAVINCI_DA8XX */ -/* Max of 16 segments per channel to conserve PaRAM slots */ -#define MAX_NR_SG 16 +/* + * Max of 20 segments per channel to conserve PaRAM slots + * Also note that MAX_NR_SG should be atleast the no.of periods + * that are required for ASoC, otherwise DMA prep calls will + * fail. Today davinci-pcm is the only user of this driver and + * requires atleast 17 slots, so we setup the default to 20. + */ +#define MAX_NR_SG 20 #define EDMA_MAX_SLOTS MAX_NR_SG #define EDMA_DESCRIPTORS 16 -- 1.8.1.2 ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
[PATCH 2/3] dma: edma: Add support for Cyclic DMA
Using the PaRAM configuration function that we split for reuse by the
different DMA types, we implement Cyclic DMA support.
For the cyclic case, we pass different configuration parameters to this
function, and handle all the Cyclic-specific functionality separately.
Callbacks to the DMA users are handled using vchan_cyclic_callback in
the virt-dma layer. Linking is handled the same way as the slave SG case
except for the last slot where we link it back to the first one in a
cyclic fashion.
For continuity, we check for cases where no.of periods is great than the
MAX number of slots the driver can allocate for a particular descriptor
and error out on such cases.
Signed-off-by: Joel Fernandes jo...@ti.com
---
drivers/dma/edma.c | 159 ++---
1 file changed, 151 insertions(+), 8 deletions(-)
diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 725b00c..9b63e1e 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -54,6 +54,7 @@
struct edma_desc {
struct virt_dma_descvdesc;
struct list_headnode;
+ int cyclic;
int absync;
int pset_nr;
int processed;
@@ -167,8 +168,13 @@ static void edma_execute(struct edma_chan *echan)
* then setup a link to the dummy slot, this results in all future
* events being absorbed and that's OK because we're done
*/
- if (edesc-processed == edesc-pset_nr)
- edma_link(echan-slot[nslots-1], echan-ecc-dummy_slot);
+ if (edesc-processed == edesc-pset_nr) {
+ if (edesc-cyclic)
+ edma_link(echan-slot[nslots-1], echan-slot[1]);
+ else
+ edma_link(echan-slot[nslots-1],
+ echan-ecc-dummy_slot);
+ }
edma_resume(echan-ch_num);
@@ -449,6 +455,138 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
return vchan_tx_prep(echan-vchan, edesc-vdesc, tx_flags);
}
+static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
+ struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
+ size_t period_len, enum dma_transfer_direction direction,
+ unsigned long tx_flags, void *context)
+{
+ struct edma_chan *echan = to_edma_chan(chan);
+ struct device *dev = chan-device-dev;
+ struct edma_desc *edesc;
+ dma_addr_t src_addr, dst_addr;
+ enum dma_slave_buswidth dev_width;
+ u32 burst;
+ int i, ret, nr_periods;
+
+ if (unlikely(!echan || !buf_len || !period_len))
+ return NULL;
+
+ if (direction == DMA_DEV_TO_MEM) {
+ src_addr = echan-cfg.src_addr;
+ dst_addr = buf_addr;
+ dev_width = echan-cfg.src_addr_width;
+ burst = echan-cfg.src_maxburst;
+ } else if (direction == DMA_MEM_TO_DEV) {
+ src_addr = buf_addr;
+ dst_addr = echan-cfg.dst_addr;
+ dev_width = echan-cfg.dst_addr_width;
+ burst = echan-cfg.dst_maxburst;
+ } else {
+ dev_err(dev, %s: bad direction?\n, __func__);
+ return NULL;
+ }
+
+ if (dev_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) {
+ dev_err(dev, Undefined slave buswidth\n);
+ return NULL;
+ }
+
+ if (unlikely(buf_len % period_len)) {
+ dev_err(dev, Period should be multiple of Buffer length\n);
+ return NULL;
+ }
+
+ nr_periods = (buf_len / period_len) + 1;
+
+ /*
+* Cyclic DMA users such as audio cannot tolerate delays introduced
+* by cases where the number of periods is more than the maximum
+* number of SGs the EDMA driver can handle at a time. For DMA types
+* such as Slave SGs, such delays are tolerable and synchronized,
+* but the synchronization is difficult to achieve with Cyclic and
+* cannot be guaranteed, so we error out early.
+*/
+ if (nr_periods MAX_NR_SG)
+ return NULL;
+
+ edesc = kzalloc(sizeof(*edesc) + nr_periods *
+ sizeof(edesc-pset[0]), GFP_ATOMIC);
+ if (!edesc) {
+ dev_dbg(dev, Failed to allocate a descriptor\n);
+ return NULL;
+ }
+
+ edesc-cyclic = 1;
+ edesc-pset_nr = nr_periods;
+
+ dev_dbg(dev, %s: nr_periods=%d\n, __func__, nr_periods);
+ dev_dbg(dev, %s: period_len=%d\n, __func__, period_len);
+ dev_dbg(dev, %s: buf_len=%d\n, __func__, buf_len);
+
+ for (i = 0; i nr_periods; i++) {
+ /* Allocate a PaRAM slot, if needed */
+ if (echan-slot[i] 0) {
+ echan-slot[i] =
+ edma_alloc_slot(EDMA_CTLR(echan-ch_num
[PATCH 0/3] dma: edma: Add cyclic DMA support
The following series adds Cyclic DMA support to TI EDMA DMA Engine driver. First we split out the calculations for the Slave DMA case into a separate function so that we may reuse it for the calculations of Cyclic DMA parameters. Next patch then adds the actual support for Cyclic DMA, enables interrupts correctly and uses's the callbacks in virt-dma during interrupts thus signalling back to the ALSA layer that a period was transmitted. Some background on motivation for this series: Currently, only user of Cyclic DMA in EDMA is davinci-pcm driver. As of today, this driver directly calls into the EDMA private API (arch/arm/common/edma.c) without going through the DMAEngine. davinci-pcm in future will be modified to use DMA Engine framework for Cyclic DMA instead of directly using the Private API. However that's a much larger effort, involving dealing with ping-pong from SRAM on user's of the Davinci McASP, etc. As a first step, we add Cyclic DMA support to the EDMA driver so that this may be used when the actual conversion of davinci-pcm happens. Tested series along with couple of hacks to davinci-pcm to work with DMA Engine: g...@github.com:joelagnel/linux-kernel.git (branch dma/cyclic) Joel Fernandes (3): dma: edma: Split out PaRAM set calculations into its own function dma: edma: Add support for Cyclic DMA dma: edma: Increase maximum SG limit to 20 drivers/dma/edma.c | 350 + 1 file changed, 273 insertions(+), 77 deletions(-) -- 1.8.1.2 ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
[PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
HWMOD removal for MMC is breaking edma_start as the events are being manually
triggered due to unused channel list not being clear.
The above issue is fixed by reading the dmas property from the DT node if it
exists and clearing the bits in the unused channel list if the dma controller
used by any device is EDMA. For this purpose we use the of_* helpers to parse
the arguments in the dmas phandle list.
Also introduced is a minor clean up of a checkpatch error in old code.
Reviewed-by: Sekhar Nori nsek...@ti.com
Reported-by: Balaji T K balaj...@ti.com
Cc: Sekhar Nori nsek...@ti.com
Cc: Tony Lindgren t...@atomide.com
Cc: Olof Johansson o...@lixom.net
Cc: Nishanth Menon n...@ti.com
Cc: Pantel Antoniou pa...@antoniou-consulting.com
Cc: Jason Kridner jkrid...@beagleboard.org
Cc: Koen Kooi k...@dominion.thruhere.net
Signed-off-by: Joel Fernandes jo...@ti.com
---
Just resending this patch after discussing with Sekhar and Olof.
AM335x is being booted by many users such as the beaglebone community. DT is
the only boot method available for all these users. EDMA is required for the
operation for many common peripherals in AM335x SoC such as McASP, MMC and
Crypto.
Although EDMA DT nodes are going in only for 3.13, in reality the kernel has
been used for more than a year with EDMA code and out of tree EDMA DTS patches.
Hence though the DT nodes are still not in mainline, this patch can be still be
considered a critical fix as such and it would be great if it could be included
in 3.12-rc release. Thanks.
More details about why this broke an existing feature folks were using:
Previously the DMA resources for platform devices were being populated through
HWMOD, however with the recent clean ups with HWMOD, this data has been moved
to Device tree. The EDMA code though is not aware of this so it fails to fetch
the DMA resources correctly which it needs to prepare the unused channel list
(basically doesn't properly clear the channels that are in use, in the unused
list).
arch/arm/common/edma.c | 38 +++---
1 file changed, 31 insertions(+), 7 deletions(-)
diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 117f955..8e1a024 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -269,6 +269,11 @@ static const struct edmacc_param dummy_paramset = {
.ccnt = 1,
};
+static const struct of_device_id edma_of_ids[] = {
+ { .compatible = ti,edma3, },
+ {}
+};
+
/*/
static void map_dmach_queue(unsigned ctlr, unsigned ch_no,
@@ -560,14 +565,38 @@ static int reserve_contiguous_slots(int ctlr, unsigned
int id,
static int prepare_unused_channel_list(struct device *dev, void *data)
{
struct platform_device *pdev = to_platform_device(dev);
- int i, ctlr;
+ int i, count, ctlr;
+ struct of_phandle_args dma_spec;
+ if (dev-of_node) {
+ count = of_property_count_strings(dev-of_node, dma-names);
+ if (count 0)
+ return 0;
+ for (i = 0; i count; i++) {
+ if (of_parse_phandle_with_args(dev-of_node, dmas,
+ #dma-cells, i,
+ dma_spec))
+ continue;
+
+ if (!of_match_node(edma_of_ids, dma_spec.np)) {
+ of_node_put(dma_spec.np);
+ continue;
+ }
+
+ clear_bit(EDMA_CHAN_SLOT(dma_spec.args[0]),
+ edma_cc[0]-edma_unused);
+ of_node_put(dma_spec.np);
+ }
+ return 0;
+ }
+
+ /* For non-OF case */
for (i = 0; i pdev-num_resources; i++) {
if ((pdev-resource[i].flags IORESOURCE_DMA)
(int)pdev-resource[i].start = 0) {
ctlr = EDMA_CTLR(pdev-resource[i].start);
clear_bit(EDMA_CHAN_SLOT(pdev-resource[i].start),
- edma_cc[ctlr]-edma_unused);
+ edma_cc[ctlr]-edma_unused);
}
}
@@ -1762,11 +1791,6 @@ static int edma_probe(struct platform_device *pdev)
return 0;
}
-static const struct of_device_id edma_of_ids[] = {
- { .compatible = ti,edma3, },
- {}
-};
-
static struct platform_driver edma_driver = {
.driver = {
.name = edma,
--
1.8.1.2
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
On 09/26/2013 06:13 PM, Olof Johansson wrote: On Thu, Sep 26, 2013 at 2:55 PM, Joel Fernandes jo...@ti.com wrote: HWMOD removal for MMC is breaking edma_start as the events are being manually triggered due to unused channel list not being clear. The above issue is fixed by reading the dmas property from the DT node if it exists and clearing the bits in the unused channel list if the dma controller used by any device is EDMA. For this purpose we use the of_* helpers to parse the arguments in the dmas phandle list. Also introduced is a minor clean up of a checkpatch error in old code. Reviewed-by: Sekhar Nori nsek...@ti.com Reported-by: Balaji T K balaj...@ti.com Cc: Sekhar Nori nsek...@ti.com Cc: Tony Lindgren t...@atomide.com Cc: Olof Johansson o...@lixom.net Cc: Nishanth Menon n...@ti.com Cc: Pantel Antoniou pa...@antoniou-consulting.com Cc: Jason Kridner jkrid...@beagleboard.org Cc: Koen Kooi k...@dominion.thruhere.net Signed-off-by: Joel Fernandes jo...@ti.com --- Just resending this patch after discussing with Sekhar and Olof. Actually, the patch you talked to me about was v3 of this. It seems that you have reposted v6 but labelled it v3. This is very confusing. Sorry about this. :-( This is indeed v6. AM335x is being booted by many users such as the beaglebone community. DT is the only boot method available for all these users. EDMA is required for the operation for many common peripherals in AM335x SoC such as McASP, MMC and Crypto. Although EDMA DT nodes are going in only for 3.13, in reality the kernel has been used for more than a year with EDMA code and out of tree EDMA DTS patches. Hence though the DT nodes are still not in mainline, this patch can be still be considered a critical fix as such and it would be great if it could be included in 3.12-rc release. Thanks. This is really the root of this problem. If you sit on code out of tree for a year, and something breaks that we couldn't even have detected since we didn't have the out-of-tree pieces. We'll help you the first few times (such as now) but we will eventually stop caring. When I started looking at EDMA in June, I noticed that a lot had already been merged. EDMA DMA Engine driver itself was merged last year, no worries there. but the long pending list of fixes to be made to the driver had to written and rewritten multiple times which took a long time. Due to this, the EDMA device tree entries could not be merged in fear that doing so would cause problems such as MMC/SD corruption etc. If I was in a worse mood, then I'd just say that since your users already has to have out-of-tree code to even use this functionality, they could just add this fix on top of that stack of patches. But I'm in a slightly better mood than that and I'll pick it up this time. :) Thank you! :) More details about why this broke an existing feature folks were using: Previously the DMA resources for platform devices were being populated through HWMOD, however with the recent clean ups with HWMOD, this data has been moved to Device tree. The EDMA code though is not aware of this so it fails to fetch the DMA resources correctly which it needs to prepare the unused channel list (basically doesn't properly clear the channels that are in use, in the unused list). So that we can learn for next time: What should we (as in us maintainers and you TI) have done differently to avoid this? I think a little on both sides can be improved. For TI, a bit more work toward explaining patches better in change logs so that maintainers understand and are willing to help to get the code upstream would help. A lot of improvement to internal policies have been made for developing on upstream, and that's certainly a good thing but there's still more room for improvement. For maintainers, EDMA code would have been kept breathing all these months (atleast 8 months) if those temporary fixes mentioned above would have been merged into the kernel instead of not applied. With those fixes in place, dts could have been enabled and EDMA would be fully working all these months. This would have certainly made things a lot easier. Rewriting stuff the right way is OK but if it is a _lot_ of effort, then to keep things alive, merging stuff for now specially if they are one-liners could be made acceptable. EDMA fixes have now been written the right way, so those temporary fixes are no longer needed :) but it certainly took a long time to do it the right way during which things were dead. Only thing pending for working EDMA now is the dts patches which are already in Benoit's for-3.13 tree and this patch that you're picking up. Thanks Olof for your help! :) regards, -Joel ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
On 09/27/2013 02:49 AM, Sekhar Nori wrote: On 9/27/2013 5:58 AM, Joel Fernandes wrote: On 09/26/2013 06:13 PM, Olof Johansson wrote: On Thu, Sep 26, 2013 at 2:55 PM, Joel Fernandes jo...@ti.com wrote: HWMOD removal for MMC is breaking edma_start as the events are being manually triggered due to unused channel list not being clear. The above issue is fixed by reading the dmas property from the DT node if it exists and clearing the bits in the unused channel list if the dma controller used by any device is EDMA. For this purpose we use the of_* helpers to parse the arguments in the dmas phandle list. Also introduced is a minor clean up of a checkpatch error in old code. Reviewed-by: Sekhar Nori nsek...@ti.com Reported-by: Balaji T K balaj...@ti.com Cc: Sekhar Nori nsek...@ti.com Cc: Tony Lindgren t...@atomide.com Cc: Olof Johansson o...@lixom.net Cc: Nishanth Menon n...@ti.com Cc: Pantel Antoniou pa...@antoniou-consulting.com Cc: Jason Kridner jkrid...@beagleboard.org Cc: Koen Kooi k...@dominion.thruhere.net Signed-off-by: Joel Fernandes jo...@ti.com --- Just resending this patch after discussing with Sekhar and Olof. Actually, the patch you talked to me about was v3 of this. It seems that you have reposted v6 but labelled it v3. This is very confusing. Sorry about this. :-( This is indeed v6. AM335x is being booted by many users such as the beaglebone community. DT is the only boot method available for all these users. EDMA is required for the operation for many common peripherals in AM335x SoC such as McASP, MMC and Crypto. Although EDMA DT nodes are going in only for 3.13, in reality the kernel has been used for more than a year with EDMA code and out of tree EDMA DTS patches. Hence though the DT nodes are still not in mainline, this patch can be still be considered a critical fix as such and it would be great if it could be included in 3.12-rc release. Thanks. This is really the root of this problem. If you sit on code out of tree for a year, and something breaks that we couldn't even have detected since we didn't have the out-of-tree pieces. We'll help you the first few times (such as now) but we will eventually stop caring. When I started looking at EDMA in June, I noticed that a lot had already been merged. EDMA DMA Engine driver itself was merged last year, no worries there. but the long pending list of fixes to be made to the driver had to written and rewritten multiple times which took a long time. Due to this, the EDMA device tree entries could not be merged in fear that doing so would cause problems such as MMC/SD corruption etc. If I was in a worse mood, then I'd just say that since your users already has to have out-of-tree code to even use this functionality, they could just add this fix on top of that stack of patches. But I'm in a slightly better mood than that and I'll pick it up this time. :) Thank you! :) More details about why this broke an existing feature folks were using: Previously the DMA resources for platform devices were being populated through HWMOD, however with the recent clean ups with HWMOD, this data has been moved to Device tree. The EDMA code though is not aware of this so it fails to fetch the DMA resources correctly which it needs to prepare the unused channel list (basically doesn't properly clear the channels that are in use, in the unused list). So that we can learn for next time: What should we (as in us maintainers and you TI) have done differently to avoid this? I think a little on both sides can be improved. For TI, a bit more work toward explaining patches better in change logs so that maintainers understand and are willing to help to get the code upstream would help. A lot of improvement to internal policies have been made for developing on upstream, and that's certainly a good thing but there's still more room forard improvement. For maintainers, EDMA code would have been kept breathing all these months (atleast 8 months) if those temporary fixes mentioned above would have been merged into the kernel instead of not applied. With those fixes in place, dts could have been enabled and EDMA would be fully working all these months. This would have certainly made things a lot easier. Rewriting stuff the right way is OK but if it is a _lot_ of effort, then to keep things alive, merging stuff for now specially if they are one-liners could be made acceptable. Joel, can you give a link to the one-liner temporary fix that was was not merged? I am unable to put it in context. Sure, not exactly a one-line but maybe couple of lines (see below). Also wasn't referring to anything you're maintaining. I was just continuing the original discussion of where we can improve as a community. My point was very trivial changes that keep things working such as this one should be merged in time to keep things working: http://lkml.indiana.edu
Re: [PATCH v3] ARM: EDMA: Fix clearing of unused list for DT DMA resources
On 09/27/2013 04:04 AM, Sekhar Nori wrote: On 9/27/2013 5:58 AM, Joel Fernandes wrote: On 09/26/2013 06:13 PM, Olof Johansson wrote: On Thu, Sep 26, 2013 at 2:55 PM, Joel Fernandes jo...@ti.com wrote: HWMOD removal for MMC is breaking edma_start as the events are being manually triggered due to unused channel list not being clear. The above issue is fixed by reading the dmas property from the DT node if it exists and clearing the bits in the unused channel list if the dma controller used by any device is EDMA. For this purpose we use the of_* helpers to parse the arguments in the dmas phandle list. Also introduced is a minor clean up of a checkpatch error in old code. Reviewed-by: Sekhar Nori nsek...@ti.com Reported-by: Balaji T K balaj...@ti.com Cc: Sekhar Nori nsek...@ti.com Cc: Tony Lindgren t...@atomide.com Cc: Olof Johansson o...@lixom.net Cc: Nishanth Menon n...@ti.com Cc: Pantel Antoniou pa...@antoniou-consulting.com Cc: Jason Kridner jkrid...@beagleboard.org Cc: Koen Kooi k...@dominion.thruhere.net Signed-off-by: Joel Fernandes jo...@ti.com --- Just resending this patch after discussing with Sekhar and Olof. Actually, the patch you talked to me about was v3 of this. It seems that you have reposted v6 but labelled it v3. This is very confusing. Sorry about this. :-( This is indeed v6. AM335x is being booted by many users such as the beaglebone community. DT is the only boot method available for all these users. EDMA is required for the operation for many common peripherals in AM335x SoC such as McASP, MMC and Crypto. Although EDMA DT nodes are going in only for 3.13, in reality the kernel has been used for more than a year with EDMA code and out of tree EDMA DTS patches. Hence though the DT nodes are still not in mainline, this patch can be still be considered a critical fix as such and it would be great if it could be included in 3.12-rc release. Thanks. This is really the root of this problem. If you sit on code out of tree for a year, and something breaks that we couldn't even have detected since we didn't have the out-of-tree pieces. We'll help you the first few times (such as now) but we will eventually stop caring. When I started looking at EDMA in June, I noticed that a lot had already been merged. EDMA DMA Engine driver itself was merged last year, no worries there. but the long pending list of fixes to be made to the driver had to written and rewritten multiple times which took a long time. Due to this, the EDMA device tree entries could not be merged in fear that doing so would cause problems such as MMC/SD corruption etc. If I was in a worse mood, then I'd just say that since your users already has to have out-of-tree code to even use this functionality, they could just add this fix on top of that stack of patches. But I'm in a slightly better mood than that and I'll pick it up this time. :) Thank you! :) More details about why this broke an existing feature folks were using: Previously the DMA resources for platform devices were being populated through HWMOD, however with the recent clean ups with HWMOD, this data has been moved to Device tree. The EDMA code though is not aware of this so it fails to fetch the DMA resources correctly which it needs to prepare the unused channel list (basically doesn't properly clear the channels that are in use, in the unused list). So that we can learn for next time: What should we (as in us maintainers and you TI) have done differently to avoid this? I think a little on both sides can be improved. Since we are in lessons learnt mode, I think as developers we need to learn to prioritize fixes over other features we are working on. I went back to the chronology of this patch series. Sure, I agree with this. Will definitely work on it. 22nd July 2013: v2 posted 29th July 2013: Discussion on whether the patch can wait till *v3.12* merge window. 29th July 2013: comments given on v2 22nd Aug 2013:Pull request's sent by Sekhar for v3.12 24th Aug 2013:another v2 posted (all comments given earlier not addressed, received some comments on build warnings) 27th Aug 2013:another v3 posted (all comments given on 29th July not addressed) 10th Sept 2013: another v3 posted (all comments given on 29th July not addressed) [some discussion on comments and why this cannot wait until v3.13] 17th Sept 2013: Final version ready for merge posted. 26th Sept 2013: Another v3 posted, this time for Olof to send into v3.12-rc See, early on, the patch was actually in consideration for v3.12 merge. The barrier of entry into -rc cycle is pretty high. So if you have an opportunity to hit a merge window, utilize that by prioritizing this work over anything else you may be doing. I know you got busy with adding support for SG lists and all
Re: [PATCH 2/3] dma: edma: Add support for Cyclic DMA
On 10/31/2013 09:10 AM, Vinod Koul wrote: On Thu, Oct 24, 2013 at 12:57:02PM -0500, Joel Fernandes wrote: Rebased on slave-dma/next branch and reapplied: Looks like your MUA caused lines to get wrapped and patch is corrupt, can you pls resend again using git-send email. I tried even the patch from patchworks but that too failed! Oops my bad, I ran into wordwrap issues. thanks for pointing this out, fixed my MUA and wont happen again! Will rebase/resend through git-send-email thanks, -Joel ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH] dma: edma: fix incorrect SG list handling
On 03/18/2014 10:28 AM, Vinod Koul wrote: On Mon, Mar 17, 2014 at 09:14:14AM -0400, Jon Ringle wrote: On Mon, 17 Mar 2014, Sekhar Nori wrote: Hi Jon, On Monday 17 March 2014 06:28 PM, Jon Ringle wrote: On Mon, 17 Mar 2014, Sekhar Nori wrote: The code to handle any length SG lists calls edma_resume() even before edma_start() is called. This is incorrect because edma_resume() enables edma events on the channel after which CPU (in edma_start) cannot clear posted events by writing to ECR (per the EDMA user's guide). Because of this EDMA transfers fail to start if due to some reason there is a pending EDMA event registered even before EDMA transfers are started. This can happen if an EDMA event is a byproduct of device initialization. Fix this by calling edma_resume() only if it is not the first batch of MAX_NR_SG elements. Without this patch, MMC/SD fails to function on DA850 EVM with DMA. The behaviour is triggered by specific IP and this can explain why the issue was not reported before (example with MMC/SD on AM335x). Tested on DA850 EVM and AM335x EVM-SK using MMC/SD card. Cc: sta...@vger.kernel.org # v3.12.x+ Cc: Joel Fernandes jo...@ti.com Reported-by: Jon Ringle jrin...@gridpoint.com Signed-off-by: Sekhar Nori nsek...@ti.com --- Jon, can you please confirm this fixes the issue you reported? The patch does not apply on linux-3.12 due to changes to the 3 context lines at the start of the hunk. But, I manually fixed up the code and it does fix the issue on our AM1808 board. Thanks for the testing. The patch is meant for latest mainline but based on what you said, a manual backport to v3.12-stable will be required. Can you please reply with a formal Tested-by: ? Tested-by: Jon Ringle jrin...@gridpoint.com where is this patch, somehow am not able to find in my inbox or archives... I found it archived here: http://comments.gmane.org/gmane.linux.davinci/28569 Patch doesn't breaking anything for MAX_NR_SG list size on AM335x, so it looks good. Acked-by: Joel Fernandes jo...@ti.com Regards, -Joel ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT
On 04/01/2014 08:06 AM, Peter Ujfalusi wrote: Use the EVENTQ_1 for default and leave the EVENTQ_0 to be used by high priority channels, like audio. Signed-off-by: Peter Ujfalusi peter.ujfal...@ti.com This looks good, though another way to do it would be to leave default to Queue 0. Put audio in Queue 1, and change QUEPRI to make Queue 1 as higher priority. This is fine, Acked-by: Joel Fernandes jo...@ti.com Regards, -Joel ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v2 08/14] DMA: edma: Use different eventq for cyclic channels
On 04/01/2014 08:06 AM, Peter Ujfalusi wrote: To improve latency with cyclic DMA operation it is preferred to use different eventq/tc than the default which is used by all other drivers (mmc, spi, i2c, etc). When preparing the cyclic dma ask for non default queue for the channel which is going to be used with cyclic mode. Signed-off-by: Peter Ujfalusi peter.ujfal...@ti.com --- drivers/dma/edma.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c index 1dd9e8806975..10048b40fac8 100644 --- a/drivers/dma/edma.c +++ b/drivers/dma/edma.c @@ -628,6 +628,9 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic( edesc-pset[i].opt |= TCINTEN; } + /* Use different eventq/tc for cyclic DMA to reduce latency */ + edma_request_non_default_queue(echan-ch_num); + return vchan_tx_prep(echan-vchan, edesc-vdesc, tx_flags); } Is there any way to guarantee that the non-default queue is of the highest priority, or in other words default queue is of lowest priority. I know you set queue 1 as default because by default 0 is higher priority. And then assigning non-default queue. When assigning default to Queue 1, it would be good to also call assign_priority_to_queue and set QUEPRI to 7 for Queue 1. Since 0, 2 and 4 are all non-defaults. Thanks, -Joel ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v2 10/14] dma: edma: Simplify direction configuration in edma_config_pset()
On 04/01/2014 08:06 AM, Peter Ujfalusi wrote:
We only support DEV_TO_MEM or MEM_TO_DEV directions with edma driver and the
check for the direction has been already done in the function calling
edma_config_pset().
The error reporting is redundant and also the else if () can be removed.
NAK. Please don't do this. I have been working on MEM_TO_MEM support as
well so leave it as it is for future.
Thanks,
-Joel
Signed-off-by: Peter Ujfalusi peter.ujfal...@ti.com
---
drivers/dma/edma.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 855766672aa9..d954099650ae 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -372,14 +372,12 @@ static int edma_config_pset(struct dma_chan *chan,
struct edmacc_param *pset,
src_cidx = cidx;
dst_bidx = 0;
dst_cidx = 0;
- } else if (direction == DMA_DEV_TO_MEM) {
+ } else {
+ /* DMA_DEV_TO_MEM */
src_bidx = 0;
src_cidx = 0;
dst_bidx = acnt;
dst_cidx = cidx;
- } else {
- dev_err(dev, %s: direction not implemented yet\n, __func__);
- return -EINVAL;
}
pset-opt = EDMA_TCC(EDMA_CHAN_SLOT(echan-ch_num));
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v2 00/14] dma: edma: Fixes for cyclic (audio) operation
Hi Peter, Other than patches 8/14 and 10/14 which I responded to, you could add my Acked-by, or add it to the series itself once you make the changes and drop 10. Acked-by: Joel Fernandes jo...@ti.com Thanks, -Joel On 04/01/2014 08:06 AM, Peter Ujfalusi wrote: Hi, This is basically a resend of the previous series: https://lkml.org/lkml/2014/3/13/119 with removed ASoC patches (most of them are applied already). Changes since v1: - ASoC patches removed - Comments from Andriy Shevchenko addressed - patch added to fix cases when src/dst_maxburst is set to 0 Adding support for DMA pause/resume Possibility to select non default event queue/TC for cyclic (audio) dma channels: all devices using the eDMA via dmaengine was assigned to the default EQ/TC (mmc, i2c, spi, etc, and audio). This is not optimal from system performance point of view since sharing the same EQ/TC can cause latency spikes for cyclic channels (long DMA transfers for MMC for example). While debugging the edma to get things sorted out I noticed that the debug was too verbose and the important information was hidden even when the we did not asked for verbose dmaengine debug. I have included some debug cleanups for the edma dmaengine driver also. Regards, Peter --- Peter Ujfalusi (14): platform_data: edma: Be precise with the paRAM struct dma: edma: Correct the handling of src/dst_maxburst == 0 dma: edma: Add support for DMA_PAUSE/RESUME operation dma: edma: Set DMA_CYCLIC capability flag arm: common: edma: Select event queue 1 as default when booted with DT arm: common: edma: Save the number of event queues/TCs arm: common: edma: API to request non default queue for a channel DMA: edma: Use different eventq for cyclic channels dma: edma: Implement device_slave_caps callback dma: edma: Simplify direction configuration in edma_config_pset() dma: edma: Reduce debug print verbosity for non verbose debugging dma: edma: Prefix debug prints where the text were identical in prep callbacks dma: edma: Add channel number to debug prints dma: edma: Print the direction value as well when it is not supported arch/arm/common/edma.c | 34 +- drivers/dma/edma.c | 96 +- include/linux/platform_data/edma.h | 20 3 files changed, 119 insertions(+), 31 deletions(-) ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v2 10/14] dma: edma: Simplify direction configuration in edma_config_pset()
On 04/11/2014 01:39 AM, Peter Ujfalusi wrote:
On 04/11/2014 01:40 AM, Joel Fernandes wrote:
On 04/01/2014 08:06 AM, Peter Ujfalusi wrote:
We only support DEV_TO_MEM or MEM_TO_DEV directions with edma driver and the
check for the direction has been already done in the function calling
edma_config_pset().
The error reporting is redundant and also the else if () can be removed.
NAK. Please don't do this. I have been working on MEM_TO_MEM support as
well so leave it as it is for future.
Sure. It is still valid to say that the error else {} will never going to
happen since you have the same check in the calling function and they already
filtered the non implemented direction.
That's true. Though the patch removes the else if which would mean more
work later ;)
I'll leave this out from v3.
Thanks.
Regards,
-Joel
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v2 03/14] dma: edma: Add support for DMA_PAUSE/RESUME operation
On 04/11/2014 11:43 AM, Vinod Koul wrote:
On Tue, Apr 01, 2014 at 04:06:04PM +0300, Peter Ujfalusi wrote:
Pause/Resume can be used by the audio stack when the stream is paused/resumed
The edma platform code has support for this and the legacy audio stack used
this.
Signed-off-by: Peter Ujfalusi peter.ujfal...@ti.com
---
drivers/dma/edma.c | 28
1 file changed, 28 insertions(+)
diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 2742867fd1e6..7891378a03f0 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -240,6 +240,26 @@ static int edma_slave_config(struct edma_chan *echan,
return 0;
}
+static int edma_dma_pause(struct edma_chan *echan)
+{
+/* Pause/Resume only allowed with cyclic mode */
+if (!echan-edesc-cyclic)
+return -EINVAL;
why this artificial restriction? The driver can do pause even for non cyclic
cases too? Yes the usage is in cyclic context but why should we limit any
future
work on this?
We struggled with this, and we certainly we don't want pauses in
non-cyclic EDMA... we tried doing a pause and it was a disaster as the
events keep coming in and those can't be paused, and EDMA hardware wont
queue them during the pause, it'll just generate an error interrupt storm.
Thanks,
-Joel
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v2 08/14] DMA: edma: Use different eventq for cyclic channels
On 04/11/2014 11:47 AM, Vinod Koul wrote: On Thu, Apr 10, 2014 at 11:36:30AM -0500, Joel Fernandes wrote: On 04/01/2014 08:06 AM, Peter Ujfalusi wrote: To improve latency with cyclic DMA operation it is preferred to use different eventq/tc than the default which is used by all other drivers (mmc, spi, i2c, etc). When preparing the cyclic dma ask for non default queue for the channel which is going to be used with cyclic mode. Signed-off-by: Peter Ujfalusi peter.ujfal...@ti.com --- drivers/dma/edma.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c index 1dd9e8806975..10048b40fac8 100644 --- a/drivers/dma/edma.c +++ b/drivers/dma/edma.c @@ -628,6 +628,9 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic( edesc-pset[i].opt |= TCINTEN; } + /* Use different eventq/tc for cyclic DMA to reduce latency */ + edma_request_non_default_queue(echan-ch_num); + return vchan_tx_prep(echan-vchan, edesc-vdesc, tx_flags); } Is there any way to guarantee that the non-default queue is of the highest priority, or in other words default queue is of lowest priority. well as we are discussing in other thread, it would make sense to pass the required priority (i am using audio so pls give me highest one) Yes, I'm aware of that part of the discussion ;) I also replied there. This post was sent much earlier on. thanks, -Joel ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT
On 04/16/2014 07:59 AM, Peter Ujfalusi wrote: [..] If the dma-priority is missing we should assume lowest priority (0). The highest priority depends on the platform. For eDMA3 in AM335x it is three level. For designware controller you might have the range 0-8 as valid. The question is how to get this information into use? We can take the priority number in the core when the dma channel is requested and add field to struct dma_chan in order to store it and the DMA drivers could have access to it. How about Vinod's idea of extending dma_slave_config? Priority is similar to rest of the runtime setup dmaengine_slave_config() is meant to do. The dma_slave_config is prepared by the client drivers, so they would need to be updated to handle the priority for the DMA. This would lead to duplicated code - unless we have a small function in dmaengine core to fetch this information, but still all dmaengine clients would need to call that. IMHO it would be better to let the dmaengine core to take the priority for the channel when it has been asked so client drivers does not need to know about it. I still feel it is much cleaner to keep this out of DT and have audio driver configure the channel manually for higher priority. Because, AFAIK audio is the only device that uses slave DMA and needs higher priority. I'd imagine everyone else who needs high priority, have their own dedicated DMAC, so from that sense I don't see the priority mechanism being used a lot anywhere else but audio, so atleast we can rule out things like code duplication in other drivers. Priority can be set to a default of low for those drivers that don't configure the channel for priority. I'm also OK with EDMA driver configuring channel for higher priority manually for the Cyclic case like you did initially. Thanks, -Joel ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3 00/10] dma: edma: Fixes for cyclic (audio) operation
On 04/14/2014 06:41 AM, Peter Ujfalusi wrote: Hi, Changes since v2: - Dropped patch 10 from v2 (simplify direction configuration...) - Dropped the channel priority related patches since we are going to go via different route for configuring the priority. - Added ACK from Joel for the patches since they are not changed since v2 Changes since v1: - ASoC patches removed - Comments from Andriy Shevchenko addressed - patch added to fix cases when src/dst_maxburst is set to 0 The series contains now only: Support for DMA pause/resume in cyclic mode device_slave_caps callback and DMA_CYCLIC flag correction. While debugging the edma to get things sorted out I noticed that the debug was too verbose and the important information was hidden even when the we did not asked for verbose dmaengine debug. I have included some debug cleanups for the edma dmaengine driver also. Regards, Peter --- Peter Ujfalusi (10): platform_data: edma: Be precise with the paRAM struct arm: common: edma: Save the number of event queues/TCs dmaengine: edma: Correct the handling of src/dst_maxburst == 0 dmaengine: edma: Add support for DMA_PAUSE/RESUME operation dmaengine: edma: Set DMA_CYCLIC capability flag dmaengine: edma: Implement device_slave_caps callback dmaengine: edma: Reduce debug print verbosity for non verbose debugging dmaengine: edma: Prefix debug prints where the text were identical in prep callbacks dmaengine: edma: Add channel number to debug prints dmaengine: edma: Print the direction value as well when it is not supported arch/arm/common/edma.c | 4 ++ drivers/dma/edma.c | 87 ++ include/linux/platform_data/edma.h | 18 3 files changed, 83 insertions(+), 26 deletions(-) I reviewed and tested all the patches in the new series to make sure it doesn't break anything with non-cyclic users (MMC and Crypto). Reviewed-and-Tested-by: Joel Fernandes jo...@ti.com regards, -Joel ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
[PATCH] dmaengine: edma: No need save/restore interrupt flags during spin_lock in IRQ
The vchan lock in edma_callback is acquired in hard interrupt context. As
interrupts are already disabled, there's no point in save/restoring interrupt
mask bit or cpsr flags.
Get rid of flags local variable and use spin_lock instead of spin_lock_irqsave.
Signed-off-by: Joel Fernandes jo...@ti.com
---
drivers/dma/edma.c |9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 91849aa..25a75e2 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -638,7 +638,6 @@ static void edma_callback(unsigned ch_num, u16 ch_status,
void *data)
struct edma_chan *echan = data;
struct device *dev = echan-vchan.chan.device-dev;
struct edma_desc *edesc;
- unsigned long flags;
struct edmacc_param p;
edesc = echan-edesc;
@@ -649,7 +648,7 @@ static void edma_callback(unsigned ch_num, u16 ch_status,
void *data)
switch (ch_status) {
case EDMA_DMA_COMPLETE:
- spin_lock_irqsave(echan-vchan.lock, flags);
+ spin_lock(echan-vchan.lock);
if (edesc) {
if (edesc-cyclic) {
@@ -665,11 +664,11 @@ static void edma_callback(unsigned ch_num, u16 ch_status,
void *data)
}
}
- spin_unlock_irqrestore(echan-vchan.lock, flags);
+ spin_unlock(echan-vchan.lock);
break;
case EDMA_DMA_CC_ERROR:
- spin_lock_irqsave(echan-vchan.lock, flags);
+ spin_lock(echan-vchan.lock);
edma_read_slot(EDMA_CHAN_SLOT(echan-slot[0]), p);
@@ -700,7 +699,7 @@ static void edma_callback(unsigned ch_num, u16 ch_status,
void *data)
edma_trigger_channel(echan-ch_num);
}
- spin_unlock_irqrestore(echan-vchan.lock, flags);
+ spin_unlock(echan-vchan.lock);
break;
default:
--
1.7.9.5
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
[PATCH] [FIX] dmaengine: virt-dma: Free descriptor after callback
Free the vd (virt descriptor) after the callback is called. In EDMA driver atleast which uses virt-dma, we make use of the desc during the callback and if its dangerously freed before the callback is called. I also noticed this in omap-dma dmaengine driver. Cc: Vinod Koul vinod.k...@intel.com Cc: Dan Williams dan.j.willi...@intel.com Cc: Russell King rmk+ker...@arm.linux.org.uk Signed-off-by: Joel Fernandes jo...@ti.com --- drivers/dma/virt-dma.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/dma/virt-dma.c b/drivers/dma/virt-dma.c index 6f80432..98aeb7f 100644 --- a/drivers/dma/virt-dma.c +++ b/drivers/dma/virt-dma.c @@ -84,10 +84,10 @@ static void vchan_complete(unsigned long arg) list_del(vd-node); - vc-desc_free(vd); - if (cb) cb(cb_data); + + vc-desc_free(vd); } } -- 1.7.9.5 ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH] [FIX] dmaengine: virt-dma: Free descriptor after callback
On 04/18/2014 03:50 AM, Russell King - ARM Linux wrote: On Thu, Apr 17, 2014 at 07:56:50PM -0500, Joel Fernandes wrote: Free the vd (virt descriptor) after the callback is called. In EDMA driver atleast which uses virt-dma, we make use of the desc during the callback and if its dangerously freed before the callback is called. I also noticed this in omap-dma dmaengine driver. You've missed the vital bit of information: why do you make use of the descriptor afterwards? You shouldn't. omap-dma doesn't either. Once clients submit their request to DMA engine, they must not hold any kind of reference to the descriptor other than the cookie. Sorry, I confused edma/omap-dma callbacks for virt dma callbacks. Anyway, I think there is still a chance in edma that we refer to the echan-edesc pointer later on after virt-dma calls the free (in edma_execute), so I'll just NULL that out to be safe and submit a patch. Thanks. regards, -Joel ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
[PATCH] dmaengine: edma: Add DMA memcpy support
We add DMA memcpy support to EDMA driver. Successful tests performed using
dmatest kernel module. Copy alignment is set to DMA_SLAVE_BUSWIDTH_4_BYTES and
users must ensure length is aligned so that copy is performed fully.
Signed-off-by: Joel Fernandes jo...@ti.com
---
drivers/dma/edma.c | 51 +++
1 file changed, 51 insertions(+)
diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 25a75e2..072f642 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -378,6 +378,11 @@ static int edma_config_pset(struct dma_chan *chan, struct
edmacc_param *pset,
src_cidx = 0;
dst_bidx = acnt;
dst_cidx = cidx;
+ } else if (direction == DMA_MEM_TO_MEM) {
+ src_bidx = acnt;
+ src_cidx = cidx;
+ dst_bidx = acnt;
+ dst_cidx = cidx;
} else {
dev_err(dev, %s: direction not implemented yet\n, __func__);
return -EINVAL;
@@ -498,6 +503,44 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
return vchan_tx_prep(echan-vchan, edesc-vdesc, tx_flags);
}
+struct dma_async_tx_descriptor *edma_prep_dma_memcpy(
+ struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
+ size_t len, unsigned long tx_flags)
+{
+ int ret;
+ struct edma_desc *edesc;
+ struct device *dev = chan-device-dev;
+ struct edma_chan *echan = to_edma_chan(chan);
+
+ if (unlikely(!echan || !len))
+ return NULL;
+
+ edesc = kzalloc(sizeof(*edesc) + sizeof(edesc-pset[0]), GFP_ATOMIC);
+ if (!edesc) {
+ dev_dbg(dev, Failed to allocate a descriptor\n);
+ return NULL;
+ }
+
+ edesc-pset_nr = 1;
+
+ ret = edma_config_pset(chan, edesc-pset[0], src, dest, 1,
+ DMA_SLAVE_BUSWIDTH_4_BYTES, len, DMA_MEM_TO_MEM);
+ if (ret 0)
+ return NULL;
+
+ edesc-absync = ret;
+
+ /*
+* Enable intermediate transfer chaining to re-trigger channel
+* on completion of every TR, and enable transfer-completion
+* interrupt on completion of the whole transfer.
+*/
+ edesc-pset[0].opt |= ITCCHEN;
+ edesc-pset[0].opt |= TCINTEN;
+
+ return vchan_tx_prep(echan-vchan, edesc-vdesc, tx_flags);
+}
+
static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
size_t period_len, enum dma_transfer_direction direction,
@@ -875,6 +918,7 @@ static void edma_dma_init(struct edma_cc *ecc, struct
dma_device *dma,
{
dma-device_prep_slave_sg = edma_prep_slave_sg;
dma-device_prep_dma_cyclic = edma_prep_dma_cyclic;
+ dma-device_prep_dma_memcpy = edma_prep_dma_memcpy;
dma-device_alloc_chan_resources = edma_alloc_chan_resources;
dma-device_free_chan_resources = edma_free_chan_resources;
dma-device_issue_pending = edma_issue_pending;
@@ -883,6 +927,12 @@ static void edma_dma_init(struct edma_cc *ecc, struct
dma_device *dma,
dma-device_slave_caps = edma_dma_device_slave_caps;
dma-dev = dev;
+ /*
+* code using dma memcpy must make sure alignment of
+* length is at dma-copy_align boundary.
+*/
+ dma-copy_align = DMA_SLAVE_BUSWIDTH_4_BYTES;
+
INIT_LIST_HEAD(dma-channels);
}
@@ -911,6 +961,7 @@ static int edma_probe(struct platform_device *pdev)
dma_cap_zero(ecc-dma_slave.cap_mask);
dma_cap_set(DMA_SLAVE, ecc-dma_slave.cap_mask);
dma_cap_set(DMA_CYCLIC, ecc-dma_slave.cap_mask);
+ dma_cap_set(DMA_MEMCPY, ecc-dma_slave.cap_mask);
edma_dma_init(ecc, ecc-dma_slave, pdev-dev);
--
1.7.9.5
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3 00/10] dma: edma: Fixes for cyclic (audio) operation
Hi Vinod, Dan, On 04/14/2014 06:41 AM, Peter Ujfalusi wrote: Hi, Changes since v2: - Dropped patch 10 from v2 (simplify direction configuration...) - Dropped the channel priority related patches since we are going to go via different route for configuring the priority. - Added ACK from Joel for the patches since they are not changed since v2 Changes since v1: - ASoC patches removed - Comments from Andriy Shevchenko addressed - patch added to fix cases when src/dst_maxburst is set to 0 The series contains now only: Support for DMA pause/resume in cyclic mode device_slave_caps callback and DMA_CYCLIC flag correction. While debugging the edma to get things sorted out I noticed that the debug was too verbose and the important information was hidden even when the we did not asked for verbose dmaengine debug. I have included some debug cleanups for the edma dmaengine driver also. I reviewed/tested these patches and they look OK to me. Also the point of contention was priority which is now dropped from the series. If the patches look OK and there are no further review comments can they be queued for -next? I also have a memcpy and another fix patch for edma so I could queue all together in my tree and send a consolidated pull request to make it easier. thanks, -Joel ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
[PATCH 2/2] dmaengine: edma: update DMA memcpy to use new param element
edma param struct is now within an edma_pset struct introduced in Thomas Gleixner's edma tx status series. Update memcpy function for the same. Cc: Thomas Gleixner t...@linutronix.de Signed-off-by: Joel Fernandes jo...@ti.com --- drivers/dma/edma.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c index 8926078..ec9c410 100644 --- a/drivers/dma/edma.c +++ b/drivers/dma/edma.c @@ -582,8 +582,8 @@ struct dma_async_tx_descriptor *edma_prep_dma_memcpy( * on completion of every TR, and enable transfer-completion * interrupt on completion of the whole transfer. */ - edesc-pset[0].opt |= ITCCHEN; - edesc-pset[0].opt |= TCINTEN; + edesc-pset[0].param.opt |= ITCCHEN; + edesc-pset[0].param.opt |= TCINTEN; return vchan_tx_prep(echan-vchan, edesc-vdesc, tx_flags); } -- 1.7.9.5 ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
[PATCH 1/2] dmaengine: edma: Document variables used for residue accounting
The granular residue accounting code uses certain variables specifically
for residue accounting. Document these in the structure declaration.
Also move around some elements and group them together.
Cc: Thomas Gleixner t...@linutronix.de
Signed-off-by: Joel Fernandes jo...@ti.com
---
drivers/dma/edma.c | 26 --
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 4c574f60..8926078 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -70,12 +70,34 @@ struct edma_desc {
int cyclic;
int absync;
int pset_nr;
+ struct edma_chan*echan;
int processed;
+
+ /*
+* The following 4 elements are used for residue accounting.
+*
+* - processed_stat: the number of SG elements we have traversed
+* so far to cover accounting. This is updated directly to processed
+* during edma_callback and is always = processed, because processed
+* refers to the number of pending transfer (programmed to EDMA
+* controller), where as processed_stat tracks number of transfers
+* accounted for so far.
+*
+* - residue: The amount of bytes we have left to transfer for this desc
+*
+* - residue_stat: The residue in bytes of data we have covered
+* so far for accounting. This is updated directly to residue
+* during callbacks to keep it current.
+*
+* - sg_len: Tracks the length of the current intermediate transfer,
+* this is required to update the residue during intermediate transfer
+* completion callback.
+*/
int processed_stat;
- u32 residue;
u32 sg_len;
+ u32 residue;
u32 residue_stat;
- struct edma_chan*echan;
+
struct edma_psetpset[0];
};
--
1.7.9.5
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v3 01/10] platform_data: edma: Be precise with the paRAM struct
On 05/27/2014 05:22 AM, Peter Ujfalusi wrote: On 05/27/2014 12:32 AM, Olof Johansson wrote: [..] I came across this patch when I was looking at a pull request from Sekhar for EDMA cleanups, and it made me look closer at the contents of this file. The include/linux/platform_data/ directory is meant to hold platform_data definitions for drivers, and nothing more. platform_data/edma.h also contains a whole bunch of interface definitions for the driver. They do not belong there, and should be moved to a different include file. That also includes the above struct, because as far as I can tell it's a runtime state structure, not something that is passed in with platform data. Can someone please clean this up? Thanks. I think Joel is working on to move/merge the code from arch/arm/common/edma.c to drivers/dma/edma.c Yes, I am planning to work on that soon. But there is an issue, more on that discussed below.. I'm sure within this work he is going to clean up the header file as well. Agreed. The private API should not be expored in any header and should be exclusive for the EDMA dmaengine driver ideally. As a first step I think the non platform_data content can be moved as include/linux/edma.h or probably as ti-edma.h? sound/soc/davinci/davinci-pcm.c: This still uses the EDMA private API in arch/arm/common/edma.c. Peter, any idea when the private usage will be removed fully, and we switch to dmaengine for ASoC? Before that can happen, we can't clean up or do any merges. What I'd like to do is fold the private API into the dmaengine driver and eliminate the need to expose the private API, thus also getting rid of the interface declarations Olof referred to. thanks, -Joel ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
