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
[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
Signed-off-by: Joel Fernandes
---
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
[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 Signed-off-by: Joel Fernandes --- 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
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] 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
---
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] [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] [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 Cc: Dan Williams Cc: Russell King Signed-off-by: Joel Fernandes --- 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
[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
---
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
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 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/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 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 >>> --- >>> 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 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
>> ---
>> 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 05/14] arm: common: edma: Select event queue 1 as default when booted with DT
On 04/11/2014 04:42 AM, Vinod Koul wrote:> On Fri, Apr 11, 2014 at 12:38:00PM +0300, Peter Ujfalusi wrote: >> On 04/11/2014 11:56 AM, Sekhar Nori wrote: >>> On Friday 11 April 2014 02:20 PM, Peter Ujfalusi wrote: On 04/11/2014 11:17 AM, Sekhar Nori wrote: > On Tuesday 01 April 2014 06:36 PM, 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 > > Acked-by: Sekhar Nori > >> --- >> arch/arm/common/edma.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c >> index 86a8b263278f..19520e2519d9 100644 >> --- a/arch/arm/common/edma.c >> +++ b/arch/arm/common/edma.c >> @@ -1546,7 +1546,8 @@ static int edma_of_parse_dt(struct device *dev, >> >> pdata->queue_priority_mapping = queue_priority_map; >> >> -pdata->default_queue = 0; >> +/* select queue 1 as default */ > > It will be nice to expand the comment with explanation of why this is > being chosen as default (lower priority queue by default for typical > bulk data transfer). Yes, extended comment is a good idea. For the next version I think I'm going to change the code around default TC/Queue and the non default queue selection, mostly based on Joel's comment: EVENTQ_1 as default queue. Set the EVENTQ_1 priority to 7 EVENTQ_0 priority is going to stay 0 and EVENTQ_2 as 2 Add new member to struct edma, like high_pri_queue. When we set the queue priorities in edma_probe() we look for the highest priority queue and save the number in high_pri_queue. I will rename the edma_request_non_default_queue() to edma_request_high_pri_queue() and it will assign the channel to the high priority queue. I think this way it is going to be more explicit and IMHO a bit more safer in a sense the we are going to get high priority when we ask for it. >>> >>> Sounds much better. I had posted some ideas about adding support for >>> channel priority in the core code but we can leave that for Vinod and >>> Dan to say if they really see a need for that. > Is it part of this series? > >> If we do it via the dmaengine core I think it would be better to have a new >> flag to be passed to dmaengine_prep_dma_*(). >> We could have for example: >> DMA_PREP_HIGH_PRI as flag to indicate that we need high priority DMA if it is >> possible. >> We can watch for this flag in the edma driver and act accordingly. >> ALSA's dmaengine_pcm_prepare_and_submit() could set this flag unconditionally >> since audio should be treated in this way if the DMA IP can do this. > Will the priority be different for each descriptor or would be based on channel > usage. If not then we can add this in dma_slave_config ? > > I can forsee its usage on slave controllers, so yes its useful :) > I agree, a better place to do this would be in the config stage, since we can set in stone (for EDMA atleast) that the channel has a certain priority. I can see where per-desc priority may help, such as a case where 2 users of the same channel want to submit different priority desc. For EDMA hardware though, there is no such support and the priority mapping is from channel->queue. One way to add per-desc support would be to change the priority of the channel itself for every desc issued, based on the priority stored in the desc itself which would be stored in the prep stage. But currently we don't have such a usecase of changing priorities based on desc. 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/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 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 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/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
> ---
> 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 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 > --- > 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 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 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 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] 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: # v3.12.x+ >>>>> Cc: Joel Fernandes >>>>> Reported-by: Jon Ringle >>>>> Signed-off-by: Sekhar Nori >>>>> --- >>>>> 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 > 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 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 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 2/3] dma: edma: Add support for Cyclic DMA
On 10/24/2013 11:38 AM, Vinod Koul wrote:
> On Tue, Oct 22, 2013 at 10:30:43AM -0500, Joel Fernandes wrote:
>> On 10/21/2013 01:53 AM, Vinod Koul wrote:
>>> On Mon, Sep 23, 2013 at 06:05:14PM -0500, Joel Fernandes wrote:
>>>> + nr_periods = (buf_len / period_len) + 1;
>>> ?
>>>
>>> consider the case of buf = period_len, above makes nr_period = 2.
>>>
>>> Or buf len 100, period len 50, makes nr_period = 3
>>>
>>> Both doesnt seem right to me?
>>
>> I guess that variable name is misleading.
>>
>> nr_periods is actually the total no.of slots needed to process the request.
>> Its
>> value is 1 greater than the total number of periods.
> Okay sounds good to me. I tried applying below but looks like it fails as I
> have
> already applied, 1 & 3. Can you pls rebase this resend
Rebased on slave-dma/next branch and reapplied:
8<---
From: Joel Fernandes
Subject: [PATCH v2] 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
---
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 748891f..ecebaf3 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -60,6 +60,7 @@
struct edma_desc {
struct virt_dma_descvdesc;
struct list_headnode;
+ int cyclic;
int absync;
int pset_nr;
int processed;
@@ -173,8 +174,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);
@@ -456,6 +462,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, nslots;
+
+ 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;
+ }
+
+ nslots = (buf_len / period_len) + 1;
+
+ /*
+* Cyclic DMA users such as audio cannot tolerate delays introduced
+
Re: [PATCH 2/3] dma: edma: Add support for Cyclic DMA
On 10/21/2013 01:53 AM, Vinod Koul wrote:
> On Mon, Sep 23, 2013 at 06:05:14PM -0500, Joel Fernandes wrote:
>
>> @@ -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;
> ?
>
> consider the case of buf = period_len, above makes nr_period = 2.
>
> Or buf len 100, period len 50, makes nr_period = 3
>
> Both doesnt seem right to me?
I guess that variable name is misleading.
nr_periods is actually the total no.of slots needed to process the request. Its
value is 1 greater than the total number of periods.
This is because the channel uses one dedicated slot that it continuously
refreshed (not read-only). The other slots are read-only, and are continuously
copied into the dedicated channel's slot.
Below is the updated patch where I changed the nr_periods variable name to
nslots for clarity.
---8<---
From: Joel Fernandes
Subject: [PATCH v2] 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
---
v2 changes:
- changed variable name- nr_periods to nslots, to make the context more clear.
drivers/dma/edma.c | 160 ++---
1 file changed, 152 insertions(+), 8 deletions(-)
diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 9b6004f..0fef450 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
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 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 >>>> Reported-by: Balaji T K >>>> Cc: Sekhar Nori >>>> Cc: Tony Lindgren >>>> Cc: Olof Johansson >>>> Cc: Nishanth Menon >>>> Cc: Pantel Antoniou >>>> Cc: Jason Kridner >>>> Cc: Koen Kooi >>>> Signed-off-by: Joel Fernandes >>>> --- >>>> 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 2
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 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 >>>> Reported-by: Balaji T K >>>> Cc: Sekhar Nori >>>> Cc: Tony Lindgren >>>> Cc: Olof Johansson >>>> Cc: Nishanth Menon >>>> Cc: Pantel Antoniou >>>> Cc: Jason Kridner >>>> Cc: Koen Kooi >>>> Signed-off-by: Joel Fernandes >>>> --- >>>> 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 o
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 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 >> Reported-by: Balaji T K >> Cc: Sekhar Nori >> Cc: Tony Lindgren >> Cc: Olof Johansson >> Cc: Nishanth Menon >> Cc: Pantel Antoniou >> Cc: Jason Kridner >> Cc: Koen Kooi >> Signed-off-by: Joel Fernandes >> --- >> 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
[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
Reported-by: Balaji T K
Cc: Sekhar Nori
Cc: Tony Lindgren
Cc: Olof Johansson
Cc: Nishanth Menon
Cc: Pantel Antoniou
Cc: Jason Kridner
Cc: Koen Kooi
Signed-off-by: Joel Fernandes
---
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
[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 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
---
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) |
[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 --- 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
---
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 PaRA
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
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
Reported-by: Balaji T K
Cc: Pantel Antoniou
Signed-off-by: Joel Fernandes
---
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
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/16/2013 06:48 AM, Sekhar Nori wrote:
> Hi Joel,
>
> On Saturday 14 September 2013 06:27 AM, Joel Fernandes wrote:
>> From: Joel Fernandes
>> 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
>> Reported-by: Balaji T K
>> Cc: Pantel Antoniou
>> Signed-off-by: Joel Fernandes
>> ---
>> 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 (
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
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
Reported-by: Balaji T K
Cc: Pantel Antoniou
Signed-off-by: Joel Fernandes
---
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 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
Reported-by: Balaji T K
Cc: Pantel Antoniou
Signed-off-by: Joel Fernandes
---
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 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
>>> Reported-by: Balaji T K
>>> Cc: Pantel Antoniou
>>> Signed-off-by: Joel Fernandes
>>> ---
>>> 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
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
>> ---
>> 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_
[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-omap&m=137416733628831&w=2
Signed-off-by: Joel Fernandes
---
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, &
[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
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
---
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
---
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 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
---
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)
[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
---
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 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-omap&m=137416733628831&w=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 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
---
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] 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
Reported-by: Balaji T K
Cc: Pantel Antoniou
Signed-off-by: Joel Fernandes
---
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 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
>> Reported-by: Balaji T K
>> Cc: Pantel Antoniou
>> Signed-off-by: Joel Fernandes
>> ---
>> 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 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
Reported-by: Balaji T K
Cc: Pantel Antoniou
Signed-off-by: Joel Fernandes
---
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 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
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 > > 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 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
>
> 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 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
>> Signed-off-by: Joel Fernandes
>> ---
>> 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 t
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
>>> ---
>>> 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 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
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
>> ---
>> 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 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 02/12] ARM: edma: Don't clear EMR of channel in edma_stop
On Sun, Aug 11, 2013 at 11:25 PM, Sekhar Nori 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 >> >> 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
