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
On Tue, Apr 01, 2014 at 04:06:01PM +0300, 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 Okay I have noticed lots of folks are using dma: which IMO isn't right here, would prefer folks to use the right subsystem name, so dmaengine: would be right subject line :) -- ~Vinod > > 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(-) > > -- > 1.9.1 > -- ___ 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 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) -- ~Vinod > 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 03/14] dma: edma: Add support for DMA_PAUSE/RESUME operation
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?
--
~Vinod
> +
> + edma_pause(echan->ch_num);
> + return 0;
> +}
> +
> +static int edma_dma_resume(struct edma_chan *echan)
> +{
> + /* Pause/Resume only allowed with cyclic mode */
> + if (!echan->edesc->cyclic)
> + return -EINVAL;
> +
> + edma_resume(echan->ch_num);
> + return 0;
> +}
> +
> static int edma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
> unsigned long arg)
> {
> @@ -255,6 +275,14 @@ static int edma_control(struct dma_chan *chan, enum
> dma_ctrl_cmd cmd,
> config = (struct dma_slave_config *)arg;
> ret = edma_slave_config(echan, config);
> break;
> + case DMA_PAUSE:
> + ret = edma_dma_pause(echan);
> + break;
> +
> + case DMA_RESUME:
> + ret = edma_dma_resume(echan);
> + break;
> +
> default:
> ret = -ENOSYS;
> }
> --
> 1.9.1
>
--
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
[GIT PULL] remoteproc cleanups for 3.15
The following changes since commit cfbf8d4857c26a8a307fb7cd258074c9dcd8c691: Linux 3.14-rc4 (2014-02-23 17:40:03 -0800) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/ohad/remoteproc.git tags/remoteproc-3.15-cleanups for you to fetch changes up to bd88acba5f9809af48f66267bb16024b9e33cf2b: remoteproc/ste_modem: staticize local symbols (2014-02-24 11:16:16 +0200) Several remoteproc cleanup patches coming from Jingoo Han, Julia Lawall and Uwe Kleine-König. Jingoo Han (1): remoteproc/ste_modem: staticize local symbols Julia Lawall (1): remoteproc/davinci: simplify use of devm_ioremap_resource Uwe Kleine-König (1): remoteproc/davinci: drop needless devm_clk_put drivers/remoteproc/da8xx_remoteproc.c | 16 +--- drivers/remoteproc/ste_modem_rproc.c | 4 ++-- 2 files changed, 3 insertions(+), 17 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 05/14] arm: common: edma: Select event queue 1 as default when booted with DT
On Fri, Apr 11, 2014 at 03:23:54PM +0300, Peter Ujfalusi wrote: > On 04/11/2014 02:31 PM, Vinod Koul wrote: > > >> I would say that it is channel based config. I don't see the reason why > >> would > >> one mix different priorities on a configured channel between descriptors. > >> > >>> If not then we can add this in dma_slave_config ? > >> > >> So adding to the struct for example: > >> bool high_priority; > > > > In designware controller, we can have channel priorties from 0 to 7 which > > IIRC 7 > > being highest. So bool wont work. unsigned int/u8 would be good. > > I see. But from a generic code what number should one pass if we want to get > the highest priority? With eDMA3 we essentially have three levels (see later) > so if we receive 7 as priority what shall we do? Just treat as highest? But if > we receive 4, which is somewhere in the middle for designware it is still > means highest for us. > > I see this too small step partitioning in common code a bit problematic when > it comes to how to interpret the 'magic numbers'. > Also how all the driver in the system will decide the priority number? I'm > sure they will pick something conveniently average for themselves and I > imagine audio would try to pick something which is bigger than the numbers > others have taken... > > > How about your controller, is it binary? > > We also have priority from 0 to 7, 0 being the highest priority. We have 3 > Transfer Controllers/Event Queues and we can set the priority for the TC/EQ > and assign the given channel to the desired TC/EQ. > So in reality we have 3 priorities to choose from in my view since we only > have 3 TC/EQ in eDMA3 (of AM335x/AM447x) on other SoCs we can have different > number of TC/EQ. I think the number shouldn't be viewed in absolute terms. If we decide that (lets say) 0-7, then any controller should map 0 to lowest and 7 to highest. For your case you can do this and then intermediate numbers would be medium priority. Such a system might work well... Also how would a client driver know which priority to use? Would it come from DT? -- ~Vinod ___ 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 02:31 PM, Vinod Koul wrote: >> I would say that it is channel based config. I don't see the reason why would >> one mix different priorities on a configured channel between descriptors. >> >>> If not then we can add this in dma_slave_config ? >> >> So adding to the struct for example: >> bool high_priority; > > In designware controller, we can have channel priorties from 0 to 7 which > IIRC 7 > being highest. So bool wont work. unsigned int/u8 would be good. I see. But from a generic code what number should one pass if we want to get the highest priority? With eDMA3 we essentially have three levels (see later) so if we receive 7 as priority what shall we do? Just treat as highest? But if we receive 4, which is somewhere in the middle for designware it is still means highest for us. I see this too small step partitioning in common code a bit problematic when it comes to how to interpret the 'magic numbers'. Also how all the driver in the system will decide the priority number? I'm sure they will pick something conveniently average for themselves and I imagine audio would try to pick something which is bigger than the numbers others have taken... > How about your controller, is it binary? We also have priority from 0 to 7, 0 being the highest priority. We have 3 Transfer Controllers/Event Queues and we can set the priority for the TC/EQ and assign the given channel to the desired TC/EQ. So in reality we have 3 priorities to choose from in my view since we only have 3 TC/EQ in eDMA3 (of AM335x/AM447x) on other SoCs we can have different number of TC/EQ. -- Péter ___ 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 Fri, Apr 11, 2014 at 02:32:28PM +0300, Peter Ujfalusi wrote: > Hi Vinod, > > On 04/11/2014 12:42 PM, 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? > > No, it is not. The original series tackled the DMA queue selection within the > edma driver stack. It was selecting high priority channel for cyclic (audio) > use only, all other DMA channels were executed on a lower priority queue. > > >> 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. > > I would say that it is channel based config. I don't see the reason why would > one mix different priorities on a configured channel between descriptors. > > > If not then we can add this in dma_slave_config ? > > So adding to the struct for example: > bool high_priority; In designware controller, we can have channel priorties from 0 to 7 which IIRC 7 being highest. So bool wont work. unsigned int/u8 would be good. How about your controller, is it binary? -- ~Vinod > > I'm not sure if it helps if we have let's say three priority level like, low, > normal and high. > I don't think that any client would ask for low priority instead using the > normal priority. > > > I can forsee its usage on slave controllers, so yes its useful :) > > True I'm sure it is going to be used as soon as it is available if the HW > supports priorities. > > -- > Péter -- ___ 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
Hi Vinod, On 04/11/2014 12:42 PM, 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? No, it is not. The original series tackled the DMA queue selection within the edma driver stack. It was selecting high priority channel for cyclic (audio) use only, all other DMA channels were executed on a lower priority queue. >> 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. I would say that it is channel based config. I don't see the reason why would one mix different priorities on a configured channel between descriptors. > If not then we can add this in dma_slave_config ? So adding to the struct for example: bool high_priority; I'm not sure if it helps if we have let's say three priority level like, low, normal and high. I don't think that any client would ask for low priority instead using the normal priority. > I can forsee its usage on slave controllers, so yes its useful :) True I'm sure it is going to be used as soon as it is available if the HW supports priorities. -- Péter ___ 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 Friday 11 April 2014 03:12 PM, 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? No, the current series has an EDMA specific way of managing priority. > >> 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 ? The priority will be per-channel not per-transaction (at least for the use case we are talking about here). Thanks, Sekhar ___ 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 0/2] introduce devm_mdiobus_alloc/devm_mdiobus_free
Hi Florian, On 04/11/2014 07:55 AM, Florian Fainelli wrote: > Hi Grygorii, > > 2014-04-04 6:40 GMT-07:00 Grygorii Strashko : >> Introduce a resource managed devm_mdiobus_alloc()/devm_mdiobus_free() >> to automatically clean up MDIO bus alocations made by MDIO drivers, >> thus leading to simplified MDIO drivers code. >> >> Update Davinci MDIO driver ss example of new devm APIs usage. > > This does look good at first glance. net-next is currently closed at > the moment, so this will have to be merged later. Thanks. It can wait for 3.16, so I'll update & resend after rc1. I have few more patches for davinci_mdio.c, so my intention here was to check if I can base them on top of new API or not :) > > At some point, we might also want to handle the mdio_bus irq array, as > that one is also usually dynamically allocated. Maybe we could just do > a static irq[PHY_MAX_ADDR] allocation, 32 times the size of an integer > might not be worth a potential leak. It sounds good, but first of all irq field of mii_bus structure has to be made private. And drivers have to use getter/setters to access it - then its type can be changes simply and safely. By the way, mdiobus_register() can be handled using DEVM approach too, but it will a bit more complex. [...] > Regards, - grygorii ___ 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 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 :) -- ~Vinod > > Not sure what to do with eDMA's 8 priority level. With that we could have high > priority; low priority; low, but not the lowest priority; about in the middle > priority; etc. > > We could have also new callback in the dma_device struct with needed wrappers > to set priority level, but where to draw the range? High, Mid and Low? Range > of 0 - 10? > > -- > Péter -- ___ 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 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. 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. Not sure what to do with eDMA's 8 priority level. With that we could have high priority; low priority; low, but not the lowest priority; about in the middle priority; etc. We could have also new callback in the dma_device struct with needed wrappers to set priority level, but where to draw the range? High, Mid and Low? Range of 0 - 10? -- Péter ___ 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 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. Thanks, Sekhar ___ 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 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. -- Péter ___ 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 07/14] arm: common: edma: API to request non default queue for a channel
On Tuesday 01 April 2014 06:36 PM, Peter Ujfalusi wrote:
> When using eDMA3 via dmaengine all dma channels will use the default queue.
> Since during request time we do not have means to change this it need to be
> done
> later, before the DMA has been started.
> With the added function it is possible to move the channel to a non default
> queue if it is possible, otherwise (when only one EQ/TC is available for the
> CC)
> the default queue is going to be used.
> For example: For optimal system performance the audio (cyclic) DMA should
> be placed to a separate queue which is different than what the rest of the
> system is using.
>
> Signed-off-by: Peter Ujfalusi
> ---
> arch/arm/common/edma.c | 27 +++
> include/linux/platform_data/edma.h | 2 ++
> 2 files changed, 29 insertions(+)
>
> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
> index be267b2080be..eaf6dd19f082 100644
> --- a/arch/arm/common/edma.c
> +++ b/arch/arm/common/edma.c
> @@ -712,6 +712,33 @@ int edma_alloc_channel(int channel,
> }
> EXPORT_SYMBOL(edma_alloc_channel);
>
> +/**
> + * edma_request_non_default_queue - try to map the channel to non default
> queue
> + * @channel: dma channel returned from edma_alloc_channel()
> + *
> + * For certain type of applications like audio it is preferred to not use the
> + * default event queue/tc to avoid eDMA caused latency.
> + *
> + * This function will iterate through the event queues available for the CC
> and
> + * picks the first EQ/TC which is not set as the default for the CC
> + */
> +void edma_request_non_default_queue(int channel)
> +{
> + unsigned ctlr = EDMA_CTLR(channel);
> + enum dma_event_q eventq_no = EVENTQ_DEFAULT;
> + int i;
> +
> + for (i = 0; i < edma_cc[ctlr]->num_tc; i++) {
> + if (i != edma_cc[ctlr]->default_queue) {
> + eventq_no = i;
> + break;
> + }
> + }
> +
> + channel = EDMA_CHAN_SLOT(channel);
> + map_dmach_queue(ctlr, channel, eventq_no);
> +}
> +EXPORT_SYMBOL(edma_request_non_default_queue);
With this API, the caller cannot be sure of what the final affect on the
channel will be. It depends a lot on what the default queue setting is.
And that can change independent of this API.
I am not an expert in dmaengine, but it looks like we are trying to
workaround the fact that dmaengine does not support multiple transfer
priorities for a dma_chan. If yes, it will be better to add support for
doing this in dmaengine core itself. A generic priority space could be
supported which each DMA device can then map to its hardware support.
Thanks,
Sekhar
___
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 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). Thanks, Sekhar ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
