[PATCH v2 00/14] dma: edma: Fixes for cyclic (audio) operation

[PATCH v2 00/14] dma: edma: Fixes for cyclic (audio) operation

[Peter Ujfalusi]

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(-)

-- 
1.9.1

[PATCH v2 00/14] dma: edma: Fixes for cyclic (audio) operation

[Peter Ujfalusi]

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(-)

-- 
1.9.1

[PATCH v2 00/14] dma: edma: Fixes for cyclic (audio) operation

[Peter Ujfalusi]

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(-)

-- 
1.9.1

[PATCH v2 01/14] platform_data: edma: Be precise with the paRAM struct

[Peter Ujfalusi]

The edmacc_param struct should follow the layout of the paRAM area in the
HW. Be explicit on the size of the fields (u32) and also mark the struct
as packed to avoid any padding on non 32bit architectures.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 include/linux/platform_data/edma.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/linux/platform_data/edma.h b/include/linux/platform_data/edma.h
index f50821cb64be..923f8a3e4ce0 100644
--- a/include/linux/platform_data/edma.h
+++ b/include/linux/platform_data/edma.h
@@ -43,15 +43,15 @@
 
 /* PaRAM slots are laid out like this */
 struct edmacc_param {
-	unsigned int opt;
-	unsigned int src;
-	unsigned int a_b_cnt;
-	unsigned int dst;
-	unsigned int src_dst_bidx;
-	unsigned int link_bcntrld;
-	unsigned int src_dst_cidx;
-	unsigned int ccnt;
-};
+	u32 opt;
+	u32 src;
+	u32 a_b_cnt;
+	u32 dst;
+	u32 src_dst_bidx;
+	u32 link_bcntrld;
+	u32 src_dst_cidx;
+	u32 ccnt;
+} __packed;
 
 /* fields in edmacc_param.opt */
 #define SAM		BIT(0)
-- 
1.9.1

[PATCH v2 01/14] platform_data: edma: Be precise with the paRAM struct

[Peter Ujfalusi]

The edmacc_param struct should follow the layout of the paRAM area in the
HW. Be explicit on the size of the fields (u32) and also mark the struct
as packed to avoid any padding on non 32bit architectures.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 include/linux/platform_data/edma.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/linux/platform_data/edma.h b/include/linux/platform_data/edma.h
index f50821cb64be..923f8a3e4ce0 100644
--- a/include/linux/platform_data/edma.h
+++ b/include/linux/platform_data/edma.h
@@ -43,15 +43,15 @@
 
 /* PaRAM slots are laid out like this */
 struct edmacc_param {
-	unsigned int opt;
-	unsigned int src;
-	unsigned int a_b_cnt;
-	unsigned int dst;
-	unsigned int src_dst_bidx;
-	unsigned int link_bcntrld;
-	unsigned int src_dst_cidx;
-	unsigned int ccnt;
-};
+	u32 opt;
+	u32 src;
+	u32 a_b_cnt;
+	u32 dst;
+	u32 src_dst_bidx;
+	u32 link_bcntrld;
+	u32 src_dst_cidx;
+	u32 ccnt;
+} __packed;
 
 /* fields in edmacc_param.opt */
 #define SAM		BIT(0)
-- 
1.9.1

[PATCH v2 01/14] platform_data: edma: Be precise with the paRAM struct

[Peter Ujfalusi]

The edmacc_param struct should follow the layout of the paRAM area in the
HW. Be explicit on the size of the fields (u32) and also mark the struct
as packed to avoid any padding on non 32bit architectures.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi-l0cyMroinI0@public.gmane.org>
---
 include/linux/platform_data/edma.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/linux/platform_data/edma.h b/include/linux/platform_data/edma.h
index f50821cb64be..923f8a3e4ce0 100644
--- a/include/linux/platform_data/edma.h
+++ b/include/linux/platform_data/edma.h
@@ -43,15 +43,15 @@
 
 /* PaRAM slots are laid out like this */
 struct edmacc_param {
-	unsigned int opt;
-	unsigned int src;
-	unsigned int a_b_cnt;
-	unsigned int dst;
-	unsigned int src_dst_bidx;
-	unsigned int link_bcntrld;
-	unsigned int src_dst_cidx;
-	unsigned int ccnt;
-};
+	u32 opt;
+	u32 src;
+	u32 a_b_cnt;
+	u32 dst;
+	u32 src_dst_bidx;
+	u32 link_bcntrld;
+	u32 src_dst_cidx;
+	u32 ccnt;
+} __packed;
 
 /* fields in edmacc_param.opt */
 #define SAM		BIT(0)
-- 
1.9.1

[PATCH v2 02/14] dma: edma: Correct the handling of src/dst_maxburst == 0

[Peter Ujfalusi]

When clients asks for maxburst = 0 it is basically the same case as if they
were asking for maxburst = 1 since in both case ASYNC need to be used and
the eDMA is expected to write/read one word per DMA request.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index cd04eb7b182e..2742867fd1e6 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -285,6 +285,10 @@ static int edma_config_pset(struct dma_chan *chan, struct edmacc_param *pset,
 	int absync;
 
 	acnt = dev_width;
+
+	/* src/dst_maxburst == 0 is the same case as src/dst_maxburst == 1 */
+	if (!burst)
+		burst = 1;
 	/*
 	 * If the maxburst is equal to the fifo width, use
 	 * A-synced transfers. This allows for large contiguous
-- 
1.9.1

[PATCH v2 02/14] dma: edma: Correct the handling of src/dst_maxburst == 0

[Peter Ujfalusi]

When clients asks for maxburst = 0 it is basically the same case as if they
were asking for maxburst = 1 since in both case ASYNC need to be used and
the eDMA is expected to write/read one word per DMA request.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index cd04eb7b182e..2742867fd1e6 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -285,6 +285,10 @@ static int edma_config_pset(struct dma_chan *chan, struct edmacc_param *pset,
 	int absync;
 
 	acnt = dev_width;
+
+	/* src/dst_maxburst == 0 is the same case as src/dst_maxburst == 1 */
+	if (!burst)
+		burst = 1;
 	/*
 	 * If the maxburst is equal to the fifo width, use
 	 * A-synced transfers. This allows for large contiguous
-- 
1.9.1

[PATCH v2 02/14] dma: edma: Correct the handling of src/dst_maxburst == 0

[Peter Ujfalusi]

When clients asks for maxburst = 0 it is basically the same case as if they
were asking for maxburst = 1 since in both case ASYNC need to be used and
the eDMA is expected to write/read one word per DMA request.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index cd04eb7b182e..2742867fd1e6 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -285,6 +285,10 @@ static int edma_config_pset(struct dma_chan *chan, struct edmacc_param *pset,
 	int absync;
 
 	acnt = dev_width;
+
+	/* src/dst_maxburst == 0 is the same case as src/dst_maxburst == 1 */
+	if (!burst)
+		burst = 1;
 	/*
 	 * If the maxburst is equal to the fifo width, use
 	 * A-synced transfers. This allows for large contiguous
-- 
1.9.1

[PATCH v2 03/14] dma: edma: Add support for DMA_PAUSE/RESUME operation

[Peter Ujfalusi]

Pause/Resume can be used by the audio stack when the stream is paused/resumed
The edma platform code has support for this and the legacy audio stack used
this.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 2742867fd1e6..7891378a03f0 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -240,6 +240,26 @@ static int edma_slave_config(struct edma_chan *echan,
 	return 0;
 }
 
+static int edma_dma_pause(struct edma_chan *echan)
+{
+	/* Pause/Resume only allowed with cyclic mode */
+	if (!echan->edesc->cyclic)
+		return -EINVAL;
+
+	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

[PATCH v2 03/14] dma: edma: Add support for DMA_PAUSE/RESUME operation

[Peter Ujfalusi]

Pause/Resume can be used by the audio stack when the stream is paused/resumed
The edma platform code has support for this and the legacy audio stack used
this.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 2742867fd1e6..7891378a03f0 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -240,6 +240,26 @@ static int edma_slave_config(struct edma_chan *echan,
 	return 0;
 }
 
+static int edma_dma_pause(struct edma_chan *echan)
+{
+	/* Pause/Resume only allowed with cyclic mode */
+	if (!echan->edesc->cyclic)
+		return -EINVAL;
+
+	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

[PATCH v2 03/14] dma: edma: Add support for DMA_PAUSE/RESUME operation

[Peter Ujfalusi]

Pause/Resume can be used by the audio stack when the stream is paused/resumed
The edma platform code has support for this and the legacy audio stack used
this.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi-l0cyMroinI0@public.gmane.org>
---
 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;
+
+	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

Re: [PATCH v2 03/14] dma: edma: Add support for DMA_PAUSE/RESUME operation

[Vinod Koul]

On Tue, Apr 01, 2014 at 04:06:04PM +0300, Peter Ujfalusi wrote:
> Pause/Resume can be used by the audio stack when the stream is paused/resumed
> The edma platform code has support for this and the legacy audio stack used
> this.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/dma/edma.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
> index 2742867fd1e6..7891378a03f0 100644
> --- a/drivers/dma/edma.c
> +++ b/drivers/dma/edma.c
> @@ -240,6 +240,26 @@ static int edma_slave_config(struct edma_chan *echan,
>  	return 0;
>  }
>  
> +static int edma_dma_pause(struct edma_chan *echan)
> +{
> +	/* Pause/Resume only allowed with cyclic mode */
> +	if (!echan->edesc->cyclic)
> +		return -EINVAL;
why this artificial restriction? The driver can do pause even for non cyclic
cases too? Yes the usage is in cyclic context but why should we limit any future
work on this?

-- 
~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
> 

-- 

[PATCH v2 03/14] dma: edma: Add support for DMA_PAUSE/RESUME operation

[Vinod Koul]

On Tue, Apr 01, 2014 at 04:06:04PM +0300, Peter Ujfalusi wrote:
> Pause/Resume can be used by the audio stack when the stream is paused/resumed
> The edma platform code has support for this and the legacy audio stack used
> this.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/dma/edma.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
> index 2742867fd1e6..7891378a03f0 100644
> --- a/drivers/dma/edma.c
> +++ b/drivers/dma/edma.c
> @@ -240,6 +240,26 @@ static int edma_slave_config(struct edma_chan *echan,
>  	return 0;
>  }
>  
> +static int edma_dma_pause(struct edma_chan *echan)
> +{
> +	/* Pause/Resume only allowed with cyclic mode */
> +	if (!echan->edesc->cyclic)
> +		return -EINVAL;
why this artificial restriction? The driver can do pause even for non cyclic
cases too? Yes the usage is in cyclic context but why should we limit any future
work on this?

-- 
~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
> 

-- 

Re: [PATCH v2 03/14] dma: edma: Add support for DMA_PAUSE/RESUME operation

[Joel Fernandes]

On 04/11/2014 11:43 AM, Vinod Koul wrote:
> On Tue, Apr 01, 2014 at 04:06:04PM +0300, Peter Ujfalusi wrote:
>> Pause/Resume can be used by the audio stack when the stream is paused/resumed
>> The edma platform code has support for this and the legacy audio stack used
>> this.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>>  drivers/dma/edma.c | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
>> index 2742867fd1e6..7891378a03f0 100644
>> --- a/drivers/dma/edma.c
>> +++ b/drivers/dma/edma.c
>> @@ -240,6 +240,26 @@ static int edma_slave_config(struct edma_chan *echan,
>>  	return 0;
>>  }
>>  
>> +static int edma_dma_pause(struct edma_chan *echan)
>> +{
>> +	/* Pause/Resume only allowed with cyclic mode */
>> +	if (!echan->edesc->cyclic)
>> +		return -EINVAL;
> why this artificial restriction? The driver can do pause even for non cyclic
> cases too? Yes the usage is in cyclic context but why should we limit any future
> work on this?
> 

We struggled with this, and we certainly we don't want pauses in
non-cyclic EDMA... we tried doing a "pause" and it was a disaster as the
events keep coming in and those can't be paused, and EDMA hardware wont
queue them during the pause, it'll just generate an error interrupt storm.

Thanks,
-Joel

[PATCH v2 03/14] dma: edma: Add support for DMA_PAUSE/RESUME operation

[Joel Fernandes]

On 04/11/2014 11:43 AM, Vinod Koul wrote:
> On Tue, Apr 01, 2014 at 04:06:04PM +0300, Peter Ujfalusi wrote:
>> Pause/Resume can be used by the audio stack when the stream is paused/resumed
>> The edma platform code has support for this and the legacy audio stack used
>> this.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>>  drivers/dma/edma.c | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
>> index 2742867fd1e6..7891378a03f0 100644
>> --- a/drivers/dma/edma.c
>> +++ b/drivers/dma/edma.c
>> @@ -240,6 +240,26 @@ static int edma_slave_config(struct edma_chan *echan,
>>  	return 0;
>>  }
>>  
>> +static int edma_dma_pause(struct edma_chan *echan)
>> +{
>> +	/* Pause/Resume only allowed with cyclic mode */
>> +	if (!echan->edesc->cyclic)
>> +		return -EINVAL;
> why this artificial restriction? The driver can do pause even for non cyclic
> cases too? Yes the usage is in cyclic context but why should we limit any future
> work on this?
> 

We struggled with this, and we certainly we don't want pauses in
non-cyclic EDMA... we tried doing a "pause" and it was a disaster as the
events keep coming in and those can't be paused, and EDMA hardware wont
queue them during the pause, it'll just generate an error interrupt storm.

Thanks,
-Joel

Re: [PATCH v2 03/14] dma: edma: Add support for DMA_PAUSE/RESUME operation

[Joel Fernandes]

On 04/11/2014 11:43 AM, Vinod Koul wrote:
> On Tue, Apr 01, 2014 at 04:06:04PM +0300, Peter Ujfalusi wrote:
>> Pause/Resume can be used by the audio stack when the stream is paused/resumed
>> The edma platform code has support for this and the legacy audio stack used
>> this.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi-l0cyMroinI0@public.gmane.org>
>> ---
>>  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

[PATCH v2 04/14] dma: edma: Set DMA_CYCLIC capability flag

[Peter Ujfalusi]

Indicate that the edma dmaengine driver has support for cyclic mode.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 arch/arm/common/edma.c | 1 +
 drivers/dma/edma.c     | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 41bca32409fc..86a8b263278f 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -1574,6 +1574,7 @@ static struct edma_soc_info *edma_setup_info_from_dt(struct device *dev,
 		return ERR_PTR(ret);
 
 	dma_cap_set(DMA_SLAVE, edma_filter_info.dma_cap);
+	dma_cap_set(DMA_CYCLIC, edma_filter_info.dma_cap);
 	of_dma_controller_register(dev->of_node, of_dma_simple_xlate,
 				   &edma_filter_info);
 
diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 7891378a03f0..1dd9e8806975 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -891,6 +891,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);
 
 	edma_dma_init(ecc, &ecc->dma_slave, &pdev->dev);
 
-- 
1.9.1

[PATCH v2 04/14] dma: edma: Set DMA_CYCLIC capability flag

[Peter Ujfalusi]

Indicate that the edma dmaengine driver has support for cyclic mode.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 arch/arm/common/edma.c | 1 +
 drivers/dma/edma.c     | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 41bca32409fc..86a8b263278f 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -1574,6 +1574,7 @@ static struct edma_soc_info *edma_setup_info_from_dt(struct device *dev,
 		return ERR_PTR(ret);
 
 	dma_cap_set(DMA_SLAVE, edma_filter_info.dma_cap);
+	dma_cap_set(DMA_CYCLIC, edma_filter_info.dma_cap);
 	of_dma_controller_register(dev->of_node, of_dma_simple_xlate,
 				   &edma_filter_info);
 
diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 7891378a03f0..1dd9e8806975 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -891,6 +891,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);
 
 	edma_dma_init(ecc, &ecc->dma_slave, &pdev->dev);
 
-- 
1.9.1

[PATCH v2 04/14] dma: edma: Set DMA_CYCLIC capability flag

[Peter Ujfalusi]

Indicate that the edma dmaengine driver has support for cyclic mode.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 arch/arm/common/edma.c | 1 +
 drivers/dma/edma.c     | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 41bca32409fc..86a8b263278f 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -1574,6 +1574,7 @@ static struct edma_soc_info *edma_setup_info_from_dt(struct device *dev,
 		return ERR_PTR(ret);
 
 	dma_cap_set(DMA_SLAVE, edma_filter_info.dma_cap);
+	dma_cap_set(DMA_CYCLIC, edma_filter_info.dma_cap);
 	of_dma_controller_register(dev->of_node, of_dma_simple_xlate,
 				   &edma_filter_info);
 
diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 7891378a03f0..1dd9e8806975 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -891,6 +891,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);
 
 	edma_dma_init(ecc, &ecc->dma_slave, &pdev->dev);
 
-- 
1.9.1

[PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

Use the EVENTQ_1 for default and leave the EVENTQ_0 to be used by high
priority channels, like audio.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 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 */
+	pdata->default_queue = EVENTQ_1;
 
 	prop = of_find_property(node, "ti,edma-xbar-event-map", &sz);
 	if (prop)
-- 
1.9.1

[PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

Use the EVENTQ_1 for default and leave the EVENTQ_0 to be used by high
priority channels, like audio.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 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 */
+	pdata->default_queue = EVENTQ_1;
 
 	prop = of_find_property(node, "ti,edma-xbar-event-map", &sz);
 	if (prop)
-- 
1.9.1

[PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

Use the EVENTQ_1 for default and leave the EVENTQ_0 to be used by high
priority channels, like audio.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 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 */
+	pdata->default_queue = EVENTQ_1;
 
 	prop = of_find_property(node, "ti,edma-xbar-event-map", &sz);
 	if (prop)
-- 
1.9.1

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Joel Fernandes]

On 04/01/2014 08:06 AM, Peter Ujfalusi wrote:
> Use the EVENTQ_1 for default and leave the EVENTQ_0 to be used by high
> priority channels, like audio.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

This looks good, though another way to do it would be to leave default
to Queue 0. Put audio in Queue 1, and change QUEPRI to make Queue 1 as
higher priority.

This is fine,
Acked-by: Joel Fernandes <joelf@ti.com>

Regards,
-Joel

[PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Joel Fernandes]

On 04/01/2014 08:06 AM, Peter Ujfalusi wrote:
> Use the EVENTQ_1 for default and leave the EVENTQ_0 to be used by high
> priority channels, like audio.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

This looks good, though another way to do it would be to leave default
to Queue 0. Put audio in Queue 1, and change QUEPRI to make Queue 1 as
higher priority.

This is fine,
Acked-by: Joel Fernandes <joelf@ti.com>

Regards,
-Joel

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Joel Fernandes]

On 04/01/2014 08:06 AM, Peter Ujfalusi wrote:
> Use the EVENTQ_1 for default and leave the EVENTQ_0 to be used by high
> priority channels, like audio.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi-l0cyMroinI0@public.gmane.org>

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 <joelf-l0cyMroinI0@public.gmane.org>

Regards,
-Joel

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Sekhar Nori]

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 <peter.ujfalusi@ti.com>

Acked-by: Sekhar Nori <nsekhar@ti.com>

> ---
>  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

[PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Sekhar Nori]

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 <peter.ujfalusi@ti.com>

Acked-by: Sekhar Nori <nsekhar@ti.com>

> ---
>  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

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Sekhar Nori]

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 <peter.ujfalusi-l0cyMroinI0@public.gmane.org>

Acked-by: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>

> ---
>  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

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

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 <peter.ujfalusi@ti.com>
> 
> Acked-by: Sekhar Nori <nsekhar@ti.com>
> 
>> ---
>>  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

[PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

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 <peter.ujfalusi@ti.com>
> 
> Acked-by: Sekhar Nori <nsekhar@ti.com>
> 
>> ---
>>  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

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

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 <peter.ujfalusi-l0cyMroinI0@public.gmane.org>
> 
> Acked-by: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
> 
>> ---
>>  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

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Sekhar Nori]

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 <peter.ujfalusi@ti.com>
>>
>> Acked-by: Sekhar Nori <nsekhar@ti.com>
>>
>>> ---
>>>  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

[PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Sekhar Nori]

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 <peter.ujfalusi@ti.com>
>>
>> Acked-by: Sekhar Nori <nsekhar@ti.com>
>>
>>> ---
>>>  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

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Sekhar Nori]

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 <peter.ujfalusi-l0cyMroinI0@public.gmane.org>
>>
>> Acked-by: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
>>
>>> ---
>>>  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

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

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 <peter.ujfalusi@ti.com>
>>>
>>> Acked-by: Sekhar Nori <nsekhar@ti.com>
>>>
>>>> ---
>>>>  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

[PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

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 <peter.ujfalusi@ti.com>
>>>
>>> Acked-by: Sekhar Nori <nsekhar@ti.com>
>>>
>>>> ---
>>>>  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

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

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 <peter.ujfalusi-l0cyMroinI0@public.gmane.org>
>>>
>>> Acked-by: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
>>>
>>>> ---
>>>>  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

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Vinod Koul]

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 <peter.ujfalusi@ti.com>
> >>>
> >>> Acked-by: Sekhar Nori <nsekhar@ti.com>
> >>>
> >>>> ---
> >>>>  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

-- 

[PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Vinod Koul]

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 <peter.ujfalusi@ti.com>
> >>>
> >>> Acked-by: Sekhar Nori <nsekhar@ti.com>
> >>>
> >>>> ---
> >>>>  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

-- 

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Vinod Koul]

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 <peter.ujfalusi@ti.com>
> >>>
> >>> Acked-by: Sekhar Nori <nsekhar@ti.com>
> >>>
> >>>> ---
> >>>>  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

-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Sekhar Nori]

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 <peter.ujfalusi@ti.com>
>>>>>
>>>>> Acked-by: Sekhar Nori <nsekhar@ti.com>
>>>>>
>>>>>> ---
>>>>>>  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

[PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Sekhar Nori]

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 <peter.ujfalusi@ti.com>
>>>>>
>>>>> Acked-by: Sekhar Nori <nsekhar@ti.com>
>>>>>
>>>>>> ---
>>>>>>  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

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Sekhar Nori]

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 <peter.ujfalusi-l0cyMroinI0@public.gmane.org>
>>>>>
>>>>> Acked-by: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
>>>>>
>>>>>> ---
>>>>>>  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

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

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 <peter.ujfalusi@ti.com>
>>>>>
>>>>> Acked-by: Sekhar Nori <nsekhar@ti.com>
>>>>>
>>>>>> ---
>>>>>>  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

[PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

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 <peter.ujfalusi@ti.com>
>>>>>
>>>>> Acked-by: Sekhar Nori <nsekhar@ti.com>
>>>>>
>>>>>> ---
>>>>>>  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

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

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 <peter.ujfalusi-l0cyMroinI0@public.gmane.org>
>>>>>
>>>>> Acked-by: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
>>>>>
>>>>>> ---
>>>>>>  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

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Vinod Koul]

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 <peter.ujfalusi@ti.com>
> >>>>>
> >>>>> Acked-by: Sekhar Nori <nsekhar@ti.com>
> >>>>>
> >>>>>> ---
> >>>>>>  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

-- 

[PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Vinod Koul]

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 <peter.ujfalusi@ti.com>
> >>>>>
> >>>>> Acked-by: Sekhar Nori <nsekhar@ti.com>
> >>>>>
> >>>>>> ---
> >>>>>>  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

-- 

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

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

[PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

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

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

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

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Vinod Koul]

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

[PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Vinod Koul]

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

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Vinod Koul]

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

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

Hi Vinod,

On 04/11/2014 03:46 PM, Vinod Koul wrote:
> 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?

I think DT would be the best place.
Not sure if we should set the range for this either. What I was thinking is to
add an optional new property to be set by the client nodes, using DMA:

mcasp0: mcasp@48038000 {
	compatible = "ti,am33xx-mcasp-audio";
	...
	dmas =	<&edma 8>,
		<&edma 9>;
	dma-names = "tx", "rx";
	dma-priorities = <2>, <2>;
};

We could agree that lower number means lower priority, higher is - well -
higher priority.
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.
In this way we only need to update the nodes which needs non default priority
for DMA.

What do you think?

-- 
Péter

[PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

Hi Vinod,

On 04/11/2014 03:46 PM, Vinod Koul wrote:
> 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?

I think DT would be the best place.
Not sure if we should set the range for this either. What I was thinking is to
add an optional new property to be set by the client nodes, using DMA:

mcasp0: mcasp at 48038000 {
	compatible = "ti,am33xx-mcasp-audio";
	...
	dmas =	<&edma 8>,
		<&edma 9>;
	dma-names = "tx", "rx";
	dma-priorities = <2>, <2>;
};

We could agree that lower number means lower priority, higher is - well -
higher priority.
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.
In this way we only need to update the nodes which needs non default priority
for DMA.

What do you think?

-- 
P?ter

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

Hi Vinod,

On 04/11/2014 03:46 PM, Vinod Koul wrote:
> 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?

I think DT would be the best place.
Not sure if we should set the range for this either. What I was thinking is to
add an optional new property to be set by the client nodes, using DMA:

mcasp0: mcasp@48038000 {
	compatible = "ti,am33xx-mcasp-audio";
	...
	dmas =	<&edma 8>,
		<&edma 9>;
	dma-names = "tx", "rx";
	dma-priorities = <2>, <2>;
};

We could agree that lower number means lower priority, higher is - well -
higher priority.
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.
In this way we only need to update the nodes which needs non default priority
for DMA.

What do you think?

-- 
Péter

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Sekhar Nori]

On Monday 14 April 2014 05:26 PM, Peter Ujfalusi wrote:
> Hi Vinod,
> 
> On 04/11/2014 03:46 PM, Vinod Koul wrote:
>> 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?
> 
> I think DT would be the best place.

In the strict sense of what DT is supposed to represent, DT is not the
best place for this. Priority is usecase driven rather that hardware
description driven. So on a reasonably less loaded system, I am sure you
could run audio even with a lower priority DMA queue.

Moreover, IMHO, encoding it in DT now will make it an ABI. Without a
wide variety of example use cases, I think it is too early to commit to
an ABI.

> Not sure if we should set the range for this either. What I was thinking is to
> add an optional new property to be set by the client nodes, using DMA:
> 
> mcasp0: mcasp@48038000 {
> 	compatible = "ti,am33xx-mcasp-audio";
> 	...
> 	dmas =	<&edma 8>,
> 		<&edma 9>;
> 	dma-names = "tx", "rx";
> 	dma-priorities = <2>, <2>;
> };
> 
> We could agree that lower number means lower priority, higher is - well -
> higher priority.
> 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.
> In this way we only need to update the nodes which needs non default priority
> for DMA.
> 
> What do you think?

If we are using dma_slave_config, I think it will be easiest to define
two levels of priority (HIGH and LOW, as thats all we see a need for
right now), and have the audio driver select the HIGH priority. If
nothing is set, EDMA can default to LOW.

Thanks,
Sekhar

[PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Sekhar Nori]

On Monday 14 April 2014 05:26 PM, Peter Ujfalusi wrote:
> Hi Vinod,
> 
> On 04/11/2014 03:46 PM, Vinod Koul wrote:
>> 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?
> 
> I think DT would be the best place.

In the strict sense of what DT is supposed to represent, DT is not the
best place for this. Priority is usecase driven rather that hardware
description driven. So on a reasonably less loaded system, I am sure you
could run audio even with a lower priority DMA queue.

Moreover, IMHO, encoding it in DT now will make it an ABI. Without a
wide variety of example use cases, I think it is too early to commit to
an ABI.

> Not sure if we should set the range for this either. What I was thinking is to
> add an optional new property to be set by the client nodes, using DMA:
> 
> mcasp0: mcasp at 48038000 {
> 	compatible = "ti,am33xx-mcasp-audio";
> 	...
> 	dmas =	<&edma 8>,
> 		<&edma 9>;
> 	dma-names = "tx", "rx";
> 	dma-priorities = <2>, <2>;
> };
> 
> We could agree that lower number means lower priority, higher is - well -
> higher priority.
> 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.
> In this way we only need to update the nodes which needs non default priority
> for DMA.
> 
> What do you think?

If we are using dma_slave_config, I think it will be easiest to define
two levels of priority (HIGH and LOW, as thats all we see a need for
right now), and have the audio driver select the HIGH priority. If
nothing is set, EDMA can default to LOW.

Thanks,
Sekhar

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Sekhar Nori]

On Monday 14 April 2014 05:26 PM, Peter Ujfalusi wrote:
> Hi Vinod,
> 
> On 04/11/2014 03:46 PM, Vinod Koul wrote:
>> 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?
> 
> I think DT would be the best place.

In the strict sense of what DT is supposed to represent, DT is not the
best place for this. Priority is usecase driven rather that hardware
description driven. So on a reasonably less loaded system, I am sure you
could run audio even with a lower priority DMA queue.

Moreover, IMHO, encoding it in DT now will make it an ABI. Without a
wide variety of example use cases, I think it is too early to commit to
an ABI.

> Not sure if we should set the range for this either. What I was thinking is to
> add an optional new property to be set by the client nodes, using DMA:
> 
> mcasp0: mcasp@48038000 {
> 	compatible = "ti,am33xx-mcasp-audio";
> 	...
> 	dmas =	<&edma 8>,
> 		<&edma 9>;
> 	dma-names = "tx", "rx";
> 	dma-priorities = <2>, <2>;
> };
> 
> We could agree that lower number means lower priority, higher is - well -
> higher priority.
> 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.
> In this way we only need to update the nodes which needs non default priority
> for DMA.
> 
> What do you think?

If we are using dma_slave_config, I think it will be easiest to define
two levels of priority (HIGH and LOW, as thats all we see a need for
right now), and have the audio driver select the HIGH priority. If
nothing is set, EDMA can default to LOW.

Thanks,
Sekhar

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

On 04/14/2014 03:12 PM, Sekhar Nori wrote:
> On Monday 14 April 2014 05:26 PM, Peter Ujfalusi wrote:
>> Hi Vinod,
>>
>> On 04/11/2014 03:46 PM, Vinod Koul wrote:
>>> 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?
>>
>> I think DT would be the best place.
> 
> In the strict sense of what DT is supposed to represent, DT is not the
> best place for this. Priority is usecase driven rather that hardware
> description driven.

While this is true, the DMA priority - if supported by the DMA engine - is a
HW feature as well. If it is supported by the HW it can be used to tune and
partition the system for the anticipated load or purpose.

> So on a reasonably less loaded system, I am sure you
> could run audio even with a lower priority DMA queue.

Yes, you can. But as soon as you have other devices using the same priority
(with eDMA3 at least) and asks for a 'long' transfer it can ruin the audio.
During audio playback/capture you execute a long MMC read for example can
introduce a glitch.

> Moreover, IMHO, encoding it in DT now will make it an ABI. Without a
> wide variety of example use cases, I think it is too early to commit to
> an ABI.

True, but we need to start from somewhere?

I'm fine if we handle this right now as I did in this series (to put only
cyclic on high priority) for now. With some forward looking changes in the
implementation of course.

>> Not sure if we should set the range for this either. What I was thinking is to
>> add an optional new property to be set by the client nodes, using DMA:
>>
>> mcasp0: mcasp@48038000 {
>> 	compatible = "ti,am33xx-mcasp-audio";
>> 	...
>> 	dmas =	<&edma 8>,
>> 		<&edma 9>;
>> 	dma-names = "tx", "rx";
>> 	dma-priorities = <2>, <2>;
>> };
>>
>> We could agree that lower number means lower priority, higher is - well -
>> higher priority.
>> 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.
>> In this way we only need to update the nodes which needs non default priority
>> for DMA.
>>
>> What do you think?
> 
> If we are using dma_slave_config, I think it will be easiest to define
> two levels of priority (HIGH and LOW, as thats all we see a need for
> right now), and have the audio driver select the HIGH priority. If
> nothing is set, EDMA can default to LOW.

But the information on which channel should be high priority should be coming
form somewhere... We can wire it in the audio stack, but I don't think it is a
good idea to start with.
And if we have high/low priority we could as well have an integer to specify
the desired level.

-- 
Péter

[PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

On 04/14/2014 03:12 PM, Sekhar Nori wrote:
> On Monday 14 April 2014 05:26 PM, Peter Ujfalusi wrote:
>> Hi Vinod,
>>
>> On 04/11/2014 03:46 PM, Vinod Koul wrote:
>>> 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?
>>
>> I think DT would be the best place.
> 
> In the strict sense of what DT is supposed to represent, DT is not the
> best place for this. Priority is usecase driven rather that hardware
> description driven.

While this is true, the DMA priority - if supported by the DMA engine - is a
HW feature as well. If it is supported by the HW it can be used to tune and
partition the system for the anticipated load or purpose.

> So on a reasonably less loaded system, I am sure you
> could run audio even with a lower priority DMA queue.

Yes, you can. But as soon as you have other devices using the same priority
(with eDMA3 at least) and asks for a 'long' transfer it can ruin the audio.
During audio playback/capture you execute a long MMC read for example can
introduce a glitch.

> Moreover, IMHO, encoding it in DT now will make it an ABI. Without a
> wide variety of example use cases, I think it is too early to commit to
> an ABI.

True, but we need to start from somewhere?

I'm fine if we handle this right now as I did in this series (to put only
cyclic on high priority) for now. With some forward looking changes in the
implementation of course.

>> Not sure if we should set the range for this either. What I was thinking is to
>> add an optional new property to be set by the client nodes, using DMA:
>>
>> mcasp0: mcasp at 48038000 {
>> 	compatible = "ti,am33xx-mcasp-audio";
>> 	...
>> 	dmas =	<&edma 8>,
>> 		<&edma 9>;
>> 	dma-names = "tx", "rx";
>> 	dma-priorities = <2>, <2>;
>> };
>>
>> We could agree that lower number means lower priority, higher is - well -
>> higher priority.
>> 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.
>> In this way we only need to update the nodes which needs non default priority
>> for DMA.
>>
>> What do you think?
> 
> If we are using dma_slave_config, I think it will be easiest to define
> two levels of priority (HIGH and LOW, as thats all we see a need for
> right now), and have the audio driver select the HIGH priority. If
> nothing is set, EDMA can default to LOW.

But the information on which channel should be high priority should be coming
form somewhere... We can wire it in the audio stack, but I don't think it is a
good idea to start with.
And if we have high/low priority we could as well have an integer to specify
the desired level.

-- 
P?ter

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

On 04/14/2014 03:12 PM, Sekhar Nori wrote:
> On Monday 14 April 2014 05:26 PM, Peter Ujfalusi wrote:
>> Hi Vinod,
>>
>> On 04/11/2014 03:46 PM, Vinod Koul wrote:
>>> 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?
>>
>> I think DT would be the best place.
> 
> In the strict sense of what DT is supposed to represent, DT is not the
> best place for this. Priority is usecase driven rather that hardware
> description driven.

While this is true, the DMA priority - if supported by the DMA engine - is a
HW feature as well. If it is supported by the HW it can be used to tune and
partition the system for the anticipated load or purpose.

> So on a reasonably less loaded system, I am sure you
> could run audio even with a lower priority DMA queue.

Yes, you can. But as soon as you have other devices using the same priority
(with eDMA3 at least) and asks for a 'long' transfer it can ruin the audio.
During audio playback/capture you execute a long MMC read for example can
introduce a glitch.

> Moreover, IMHO, encoding it in DT now will make it an ABI. Without a
> wide variety of example use cases, I think it is too early to commit to
> an ABI.

True, but we need to start from somewhere?

I'm fine if we handle this right now as I did in this series (to put only
cyclic on high priority) for now. With some forward looking changes in the
implementation of course.

>> Not sure if we should set the range for this either. What I was thinking is to
>> add an optional new property to be set by the client nodes, using DMA:
>>
>> mcasp0: mcasp@48038000 {
>> 	compatible = "ti,am33xx-mcasp-audio";
>> 	...
>> 	dmas =	<&edma 8>,
>> 		<&edma 9>;
>> 	dma-names = "tx", "rx";
>> 	dma-priorities = <2>, <2>;
>> };
>>
>> We could agree that lower number means lower priority, higher is - well -
>> higher priority.
>> 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.
>> In this way we only need to update the nodes which needs non default priority
>> for DMA.
>>
>> What do you think?
> 
> If we are using dma_slave_config, I think it will be easiest to define
> two levels of priority (HIGH and LOW, as thats all we see a need for
> right now), and have the audio driver select the HIGH priority. If
> nothing is set, EDMA can default to LOW.

But the information on which channel should be high priority should be coming
form somewhere... We can wire it in the audio stack, but I don't think it is a
good idea to start with.
And if we have high/low priority we could as well have an integer to specify
the desired level.

-- 
Péter

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Sekhar Nori]

On Monday 14 April 2014 06:11 PM, Peter Ujfalusi wrote:
> On 04/14/2014 03:12 PM, Sekhar Nori wrote:
>> On Monday 14 April 2014 05:26 PM, Peter Ujfalusi wrote:
>>> Hi Vinod,
>>>
>>> On 04/11/2014 03:46 PM, Vinod Koul wrote:
>>>> 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?
>>>
>>> I think DT would be the best place.
>>
>> In the strict sense of what DT is supposed to represent, DT is not the
>> best place for this. Priority is usecase driven rather that hardware
>> description driven.
> 
> While this is true, the DMA priority - if supported by the DMA engine - is a
> HW feature as well. If it is supported by the HW it can be used to tune and
> partition the system for the anticipated load or purpose.
> 
>> So on a reasonably less loaded system, I am sure you
>> could run audio even with a lower priority DMA queue.
> 
> Yes, you can. But as soon as you have other devices using the same priority
> (with eDMA3 at least) and asks for a 'long' transfer it can ruin the audio.
> During audio playback/capture you execute a long MMC read for example can
> introduce a glitch.
> 
>> Moreover, IMHO, encoding it in DT now will make it an ABI. Without a
>> wide variety of example use cases, I think it is too early to commit to
>> an ABI.
> 
> True, but we need to start from somewhere?

Right, and based on our IRC discussion, we are not really fixing up the
priority value space. That makes me much more comfortable with the idea.

>>> Not sure if we should set the range for this either. What I was thinking is to
>>> add an optional new property to be set by the client nodes, using DMA:
>>>
>>> mcasp0: mcasp@48038000 {
>>> 	compatible = "ti,am33xx-mcasp-audio";
>>> 	...
>>> 	dmas =	<&edma 8>,
>>> 		<&edma 9>;
>>> 	dma-names = "tx", "rx";
>>> 	dma-priorities = <2>, <2>;
>>> };
>>>

>>> We could agree that lower number means lower priority, higher is - well -
>>> higher priority.

Even this does not have to be uniform across, right? The numbers could
be left to interpretation per-SoC.

>>> 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.

Thanks,
Sekhar

[PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Sekhar Nori]

On Monday 14 April 2014 06:11 PM, Peter Ujfalusi wrote:
> On 04/14/2014 03:12 PM, Sekhar Nori wrote:
>> On Monday 14 April 2014 05:26 PM, Peter Ujfalusi wrote:
>>> Hi Vinod,
>>>
>>> On 04/11/2014 03:46 PM, Vinod Koul wrote:
>>>> 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?
>>>
>>> I think DT would be the best place.
>>
>> In the strict sense of what DT is supposed to represent, DT is not the
>> best place for this. Priority is usecase driven rather that hardware
>> description driven.
> 
> While this is true, the DMA priority - if supported by the DMA engine - is a
> HW feature as well. If it is supported by the HW it can be used to tune and
> partition the system for the anticipated load or purpose.
> 
>> So on a reasonably less loaded system, I am sure you
>> could run audio even with a lower priority DMA queue.
> 
> Yes, you can. But as soon as you have other devices using the same priority
> (with eDMA3 at least) and asks for a 'long' transfer it can ruin the audio.
> During audio playback/capture you execute a long MMC read for example can
> introduce a glitch.
> 
>> Moreover, IMHO, encoding it in DT now will make it an ABI. Without a
>> wide variety of example use cases, I think it is too early to commit to
>> an ABI.
> 
> True, but we need to start from somewhere?

Right, and based on our IRC discussion, we are not really fixing up the
priority value space. That makes me much more comfortable with the idea.

>>> Not sure if we should set the range for this either. What I was thinking is to
>>> add an optional new property to be set by the client nodes, using DMA:
>>>
>>> mcasp0: mcasp at 48038000 {
>>> 	compatible = "ti,am33xx-mcasp-audio";
>>> 	...
>>> 	dmas =	<&edma 8>,
>>> 		<&edma 9>;
>>> 	dma-names = "tx", "rx";
>>> 	dma-priorities = <2>, <2>;
>>> };
>>>

>>> We could agree that lower number means lower priority, higher is - well -
>>> higher priority.

Even this does not have to be uniform across, right? The numbers could
be left to interpretation per-SoC.

>>> 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.

Thanks,
Sekhar

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Sekhar Nori]

On Monday 14 April 2014 06:11 PM, Peter Ujfalusi wrote:
> On 04/14/2014 03:12 PM, Sekhar Nori wrote:
>> On Monday 14 April 2014 05:26 PM, Peter Ujfalusi wrote:
>>> Hi Vinod,
>>>
>>> On 04/11/2014 03:46 PM, Vinod Koul wrote:
>>>> 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?
>>>
>>> I think DT would be the best place.
>>
>> In the strict sense of what DT is supposed to represent, DT is not the
>> best place for this. Priority is usecase driven rather that hardware
>> description driven.
> 
> While this is true, the DMA priority - if supported by the DMA engine - is a
> HW feature as well. If it is supported by the HW it can be used to tune and
> partition the system for the anticipated load or purpose.
> 
>> So on a reasonably less loaded system, I am sure you
>> could run audio even with a lower priority DMA queue.
> 
> Yes, you can. But as soon as you have other devices using the same priority
> (with eDMA3 at least) and asks for a 'long' transfer it can ruin the audio.
> During audio playback/capture you execute a long MMC read for example can
> introduce a glitch.
> 
>> Moreover, IMHO, encoding it in DT now will make it an ABI. Without a
>> wide variety of example use cases, I think it is too early to commit to
>> an ABI.
> 
> True, but we need to start from somewhere?

Right, and based on our IRC discussion, we are not really fixing up the
priority value space. That makes me much more comfortable with the idea.

>>> Not sure if we should set the range for this either. What I was thinking is to
>>> add an optional new property to be set by the client nodes, using DMA:
>>>
>>> mcasp0: mcasp@48038000 {
>>> 	compatible = "ti,am33xx-mcasp-audio";
>>> 	...
>>> 	dmas =	<&edma 8>,
>>> 		<&edma 9>;
>>> 	dma-names = "tx", "rx";
>>> 	dma-priorities = <2>, <2>;
>>> };
>>>

>>> We could agree that lower number means lower priority, higher is - well -
>>> higher priority.

Even this does not have to be uniform across, right? The numbers could
be left to interpretation per-SoC.

>>> 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.

Thanks,
Sekhar

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

On 04/14/2014 05:32 PM, Sekhar Nori wrote:
>> Yes, you can. But as soon as you have other devices using the same priority
>> (with eDMA3 at least) and asks for a 'long' transfer it can ruin the audio.
>> During audio playback/capture you execute a long MMC read for example can
>> introduce a glitch.
>>
>>> Moreover, IMHO, encoding it in DT now will make it an ABI. Without a
>>> wide variety of example use cases, I think it is too early to commit to
>>> an ABI.
>>
>> True, but we need to start from somewhere?
> 
> Right, and based on our IRC discussion, we are not really fixing up the
> priority value space. That makes me much more comfortable with the idea.

The only thing we should agree on is the 0 means lowest priority. I think this
will help in case when a new kernel is fed with an older dt blob where the
property does not exist.

> 
>>>> Not sure if we should set the range for this either. What I was thinking is to
>>>> add an optional new property to be set by the client nodes, using DMA:
>>>>
>>>> mcasp0: mcasp@48038000 {
>>>> 	compatible = "ti,am33xx-mcasp-audio";
>>>> 	...
>>>> 	dmas =	<&edma 8>,
>>>> 		<&edma 9>;
>>>> 	dma-names = "tx", "rx";
>>>> 	dma-priorities = <2>, <2>;
>>>> };
>>>>
> 
>>>> We could agree that lower number means lower priority, higher is - well -
>>>> higher priority.
> 
> Even this does not have to be uniform across, right? The numbers could
> be left to interpretation per-SoC.
> 
>>>> 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.

-- 
Péter

[PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

On 04/14/2014 05:32 PM, Sekhar Nori wrote:
>> Yes, you can. But as soon as you have other devices using the same priority
>> (with eDMA3 at least) and asks for a 'long' transfer it can ruin the audio.
>> During audio playback/capture you execute a long MMC read for example can
>> introduce a glitch.
>>
>>> Moreover, IMHO, encoding it in DT now will make it an ABI. Without a
>>> wide variety of example use cases, I think it is too early to commit to
>>> an ABI.
>>
>> True, but we need to start from somewhere?
> 
> Right, and based on our IRC discussion, we are not really fixing up the
> priority value space. That makes me much more comfortable with the idea.

The only thing we should agree on is the 0 means lowest priority. I think this
will help in case when a new kernel is fed with an older dt blob where the
property does not exist.

> 
>>>> Not sure if we should set the range for this either. What I was thinking is to
>>>> add an optional new property to be set by the client nodes, using DMA:
>>>>
>>>> mcasp0: mcasp at 48038000 {
>>>> 	compatible = "ti,am33xx-mcasp-audio";
>>>> 	...
>>>> 	dmas =	<&edma 8>,
>>>> 		<&edma 9>;
>>>> 	dma-names = "tx", "rx";
>>>> 	dma-priorities = <2>, <2>;
>>>> };
>>>>
> 
>>>> We could agree that lower number means lower priority, higher is - well -
>>>> higher priority.
> 
> Even this does not have to be uniform across, right? The numbers could
> be left to interpretation per-SoC.
> 
>>>> 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.

-- 
P?ter

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

On 04/14/2014 05:32 PM, Sekhar Nori wrote:
>> Yes, you can. But as soon as you have other devices using the same priority
>> (with eDMA3 at least) and asks for a 'long' transfer it can ruin the audio.
>> During audio playback/capture you execute a long MMC read for example can
>> introduce a glitch.
>>
>>> Moreover, IMHO, encoding it in DT now will make it an ABI. Without a
>>> wide variety of example use cases, I think it is too early to commit to
>>> an ABI.
>>
>> True, but we need to start from somewhere?
> 
> Right, and based on our IRC discussion, we are not really fixing up the
> priority value space. That makes me much more comfortable with the idea.

The only thing we should agree on is the 0 means lowest priority. I think this
will help in case when a new kernel is fed with an older dt blob where the
property does not exist.

> 
>>>> Not sure if we should set the range for this either. What I was thinking is to
>>>> add an optional new property to be set by the client nodes, using DMA:
>>>>
>>>> mcasp0: mcasp@48038000 {
>>>> 	compatible = "ti,am33xx-mcasp-audio";
>>>> 	...
>>>> 	dmas =	<&edma 8>,
>>>> 		<&edma 9>;
>>>> 	dma-names = "tx", "rx";
>>>> 	dma-priorities = <2>, <2>;
>>>> };
>>>>
> 
>>>> We could agree that lower number means lower priority, higher is - well -
>>>> higher priority.
> 
> Even this does not have to be uniform across, right? The numbers could
> be left to interpretation per-SoC.
> 
>>>> 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.

-- 
Péter

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Joel Fernandes]

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

[PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Joel Fernandes]

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

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Joel Fernandes]

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

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

On 04/16/2014 07:05 PM, Joel Fernandes wrote:
> 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.

So how should we go about this?
I'm fine to select higher priority in the eDMA driver for now when a cyclic
channel is configured and later when we have (if we ever have) generic way to
handle DMA channel/transaction priority we can switch eDMA as well to use it.

-- 
Péter

[PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

On 04/16/2014 07:05 PM, Joel Fernandes wrote:
> 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.

So how should we go about this?
I'm fine to select higher priority in the eDMA driver for now when a cyclic
channel is configured and later when we have (if we ever have) generic way to
handle DMA channel/transaction priority we can switch eDMA as well to use it.

-- 
P?ter

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

On 04/16/2014 07:05 PM, Joel Fernandes wrote:
> 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.

So how should we go about this?
I'm fine to select higher priority in the eDMA driver for now when a cyclic
channel is configured and later when we have (if we ever have) generic way to
handle DMA channel/transaction priority we can switch eDMA as well to use it.

-- 
Péter

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Joel Fernandes]

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 <peter.ujfalusi@ti.com>
>>>>>
>>>>> Acked-by: Sekhar Nori <nsekhar@ti.com>
>>>>>
>>>>>> ---
>>>>>>  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

[PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Joel Fernandes]

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 <peter.ujfalusi@ti.com>
>>>>>
>>>>> Acked-by: Sekhar Nori <nsekhar@ti.com>
>>>>>
>>>>>> ---
>>>>>>  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

Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Joel Fernandes]

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 <peter.ujfalusi-l0cyMroinI0@public.gmane.org>
>>>>>
>>>>> Acked-by: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
>>>>>
>>>>>> ---
>>>>>>  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

[PATCH v2 06/14] arm: common: edma: Save the number of event queues/TCs

[Peter Ujfalusi]

For later use save the number of queues available for the CC.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 arch/arm/common/edma.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 19520e2519d9..be267b2080be 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -1770,6 +1770,9 @@ static int edma_probe(struct platform_device *pdev)
 			map_queue_tc(j, queue_tc_mapping[i][0],
 					queue_tc_mapping[i][1]);
 
+		/* Save the number of TCs */
+		edma_cc[j]->num_tc = i;
+
 		/* Event queue priority mapping */
 		for (i = 0; queue_priority_mapping[i][0] != -1; i++)
 			assign_priority_to_queue(j,
-- 
1.9.1

[PATCH v2 06/14] arm: common: edma: Save the number of event queues/TCs

[Peter Ujfalusi]

For later use save the number of queues available for the CC.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 arch/arm/common/edma.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 19520e2519d9..be267b2080be 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -1770,6 +1770,9 @@ static int edma_probe(struct platform_device *pdev)
 			map_queue_tc(j, queue_tc_mapping[i][0],
 					queue_tc_mapping[i][1]);
 
+		/* Save the number of TCs */
+		edma_cc[j]->num_tc = i;
+
 		/* Event queue priority mapping */
 		for (i = 0; queue_priority_mapping[i][0] != -1; i++)
 			assign_priority_to_queue(j,
-- 
1.9.1

[PATCH v2 06/14] arm: common: edma: Save the number of event queues/TCs

[Peter Ujfalusi]

For later use save the number of queues available for the CC.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi-l0cyMroinI0@public.gmane.org>
---
 arch/arm/common/edma.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 19520e2519d9..be267b2080be 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -1770,6 +1770,9 @@ static int edma_probe(struct platform_device *pdev)
 			map_queue_tc(j, queue_tc_mapping[i][0],
 					queue_tc_mapping[i][1]);
 
+		/* Save the number of TCs */
+		edma_cc[j]->num_tc = i;
+
 		/* Event queue priority mapping */
 		for (i = 0; queue_priority_mapping[i][0] != -1; i++)
 			assign_priority_to_queue(j,
-- 
1.9.1

[PATCH v2 07/14] arm: common: edma: API to request non default queue for a channel

[Peter Ujfalusi]

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 <peter.ujfalusi@ti.com>
---
 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);
 
 /**
  * edma_free_channel - deallocate DMA channel
diff --git a/include/linux/platform_data/edma.h b/include/linux/platform_data/edma.h
index 923f8a3e4ce0..5d0a1b98f205 100644
--- a/include/linux/platform_data/edma.h
+++ b/include/linux/platform_data/edma.h
@@ -117,6 +117,8 @@ int edma_alloc_channel(int channel,
 	void *data, enum dma_event_q);
 void edma_free_channel(unsigned channel);
 
+void edma_request_non_default_queue(int channel);
+
 /* alloc/free parameter RAM slots */
 int edma_alloc_slot(unsigned ctlr, int slot);
 void edma_free_slot(unsigned slot);
-- 
1.9.1

[PATCH v2 07/14] arm: common: edma: API to request non default queue for a channel

[Peter Ujfalusi]

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 <peter.ujfalusi@ti.com>
---
 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);
 
 /**
  * edma_free_channel - deallocate DMA channel
diff --git a/include/linux/platform_data/edma.h b/include/linux/platform_data/edma.h
index 923f8a3e4ce0..5d0a1b98f205 100644
--- a/include/linux/platform_data/edma.h
+++ b/include/linux/platform_data/edma.h
@@ -117,6 +117,8 @@ int edma_alloc_channel(int channel,
 	void *data, enum dma_event_q);
 void edma_free_channel(unsigned channel);
 
+void edma_request_non_default_queue(int channel);
+
 /* alloc/free parameter RAM slots */
 int edma_alloc_slot(unsigned ctlr, int slot);
 void edma_free_slot(unsigned slot);
-- 
1.9.1

[PATCH v2 07/14] arm: common: edma: API to request non default queue for a channel

[Peter Ujfalusi]

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 <peter.ujfalusi-l0cyMroinI0@public.gmane.org>
---
 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);
 
 /**
  * edma_free_channel - deallocate DMA channel
diff --git a/include/linux/platform_data/edma.h b/include/linux/platform_data/edma.h
index 923f8a3e4ce0..5d0a1b98f205 100644
--- a/include/linux/platform_data/edma.h
+++ b/include/linux/platform_data/edma.h
@@ -117,6 +117,8 @@ int edma_alloc_channel(int channel,
 	void *data, enum dma_event_q);
 void edma_free_channel(unsigned channel);
 
+void edma_request_non_default_queue(int channel);
+
 /* alloc/free parameter RAM slots */
 int edma_alloc_slot(unsigned ctlr, int slot);
 void edma_free_slot(unsigned slot);
-- 
1.9.1

Re: [PATCH v2 07/14] arm: common: edma: API to request non default queue for a channel

[Sekhar Nori]

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 <peter.ujfalusi@ti.com>
> ---
>  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

[PATCH v2 07/14] arm: common: edma: API to request non default queue for a channel

[Sekhar Nori]

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 <peter.ujfalusi@ti.com>
> ---
>  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

Re: [PATCH v2 07/14] arm: common: edma: API to request non default queue for a channel

[Sekhar Nori]

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 <peter.ujfalusi@ti.com>
> ---
>  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

[PATCH v2 08/14] DMA: edma: Use different eventq for cyclic channels

[Peter Ujfalusi]

To improve latency with cyclic DMA operation it is preferred to
use different eventq/tc than the default which is used by all
other drivers (mmc, spi, i2c, etc).
When preparing the cyclic dma ask for non default queue for the
channel which is going to be used with cyclic mode.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 1dd9e8806975..10048b40fac8 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -628,6 +628,9 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
 		edesc->pset[i].opt |= TCINTEN;
 	}
 
+	/* Use different eventq/tc for cyclic DMA to reduce latency */
+	edma_request_non_default_queue(echan->ch_num);
+
 	return vchan_tx_prep(&echan->vchan, &edesc->vdesc, tx_flags);
 }
 
-- 
1.9.1

[PATCH v2 08/14] DMA: edma: Use different eventq for cyclic channels

[Peter Ujfalusi]

To improve latency with cyclic DMA operation it is preferred to
use different eventq/tc than the default which is used by all
other drivers (mmc, spi, i2c, etc).
When preparing the cyclic dma ask for non default queue for the
channel which is going to be used with cyclic mode.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 1dd9e8806975..10048b40fac8 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -628,6 +628,9 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
 		edesc->pset[i].opt |= TCINTEN;
 	}
 
+	/* Use different eventq/tc for cyclic DMA to reduce latency */
+	edma_request_non_default_queue(echan->ch_num);
+
 	return vchan_tx_prep(&echan->vchan, &edesc->vdesc, tx_flags);
 }
 
-- 
1.9.1

[PATCH v2 08/14] DMA: edma: Use different eventq for cyclic channels

[Peter Ujfalusi]

To improve latency with cyclic DMA operation it is preferred to
use different eventq/tc than the default which is used by all
other drivers (mmc, spi, i2c, etc).
When preparing the cyclic dma ask for non default queue for the
channel which is going to be used with cyclic mode.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi-l0cyMroinI0@public.gmane.org>
---
 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);
 }
 
-- 
1.9.1

Re: [PATCH v2 08/14] DMA: edma: Use different eventq for cyclic channels

[Joel Fernandes]

On 04/01/2014 08:06 AM, Peter Ujfalusi wrote:
> To improve latency with cyclic DMA operation it is preferred to
> use different eventq/tc than the default which is used by all
> other drivers (mmc, spi, i2c, etc).
> When preparing the cyclic dma ask for non default queue for the
> channel which is going to be used with cyclic mode.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/dma/edma.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
> index 1dd9e8806975..10048b40fac8 100644
> --- a/drivers/dma/edma.c
> +++ b/drivers/dma/edma.c
> @@ -628,6 +628,9 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
>  		edesc->pset[i].opt |= TCINTEN;
>  	}
>  
> +	/* Use different eventq/tc for cyclic DMA to reduce latency */
> +	edma_request_non_default_queue(echan->ch_num);
> +
>  	return vchan_tx_prep(&echan->vchan, &edesc->vdesc, tx_flags);
>  }
>  
> 

Is there any way to guarantee that the non-default queue is of the
highest priority, or in other words default queue is of lowest priority.
I know you set queue 1 as default because by default 0 is higher
priority. And then assigning non-default queue.

When assigning default to Queue 1, it would be good to also call
assign_priority_to_queue and set QUEPRI to 7 for Queue 1. Since 0, 2 and
4 are all non-defaults.

Thanks,
-Joel

[PATCH v2 08/14] DMA: edma: Use different eventq for cyclic channels

[Joel Fernandes]

On 04/01/2014 08:06 AM, Peter Ujfalusi wrote:
> To improve latency with cyclic DMA operation it is preferred to
> use different eventq/tc than the default which is used by all
> other drivers (mmc, spi, i2c, etc).
> When preparing the cyclic dma ask for non default queue for the
> channel which is going to be used with cyclic mode.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/dma/edma.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
> index 1dd9e8806975..10048b40fac8 100644
> --- a/drivers/dma/edma.c
> +++ b/drivers/dma/edma.c
> @@ -628,6 +628,9 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
>  		edesc->pset[i].opt |= TCINTEN;
>  	}
>  
> +	/* Use different eventq/tc for cyclic DMA to reduce latency */
> +	edma_request_non_default_queue(echan->ch_num);
> +
>  	return vchan_tx_prep(&echan->vchan, &edesc->vdesc, tx_flags);
>  }
>  
> 

Is there any way to guarantee that the non-default queue is of the
highest priority, or in other words default queue is of lowest priority.
I know you set queue 1 as default because by default 0 is higher
priority. And then assigning non-default queue.

When assigning default to Queue 1, it would be good to also call
assign_priority_to_queue and set QUEPRI to 7 for Queue 1. Since 0, 2 and
4 are all non-defaults.

Thanks,
-Joel

Re: [PATCH v2 08/14] DMA: edma: Use different eventq for cyclic channels

[Joel Fernandes]

On 04/01/2014 08:06 AM, Peter Ujfalusi wrote:
> To improve latency with cyclic DMA operation it is preferred to
> use different eventq/tc than the default which is used by all
> other drivers (mmc, spi, i2c, etc).
> When preparing the cyclic dma ask for non default queue for the
> channel which is going to be used with cyclic mode.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi-l0cyMroinI0@public.gmane.org>
> ---
>  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

Re: [PATCH v2 08/14] DMA: edma: Use different eventq for cyclic channels

[Vinod Koul]

On Thu, Apr 10, 2014 at 11:36:30AM -0500, Joel Fernandes wrote:
> On 04/01/2014 08:06 AM, Peter Ujfalusi wrote:
> > To improve latency with cyclic DMA operation it is preferred to
> > use different eventq/tc than the default which is used by all
> > other drivers (mmc, spi, i2c, etc).
> > When preparing the cyclic dma ask for non default queue for the
> > channel which is going to be used with cyclic mode.
> > 
> > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> > ---
> >  drivers/dma/edma.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
> > index 1dd9e8806975..10048b40fac8 100644
> > --- a/drivers/dma/edma.c
> > +++ b/drivers/dma/edma.c
> > @@ -628,6 +628,9 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
> >  		edesc->pset[i].opt |= TCINTEN;
> >  	}
> >  
> > +	/* Use different eventq/tc for cyclic DMA to reduce latency */
> > +	edma_request_non_default_queue(echan->ch_num);
> > +
> >  	return vchan_tx_prep(&echan->vchan, &edesc->vdesc, tx_flags);
> >  }
> >  
> > 
> 
> Is there any way to guarantee that the non-default queue is of the
> highest priority, or in other words default queue is of lowest priority.
well as we are discussing in other thread, it would make sense to pass the
required priority (i am using audio so pls give me highest one)

-- 
~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

-- 

[PATCH v2 08/14] DMA: edma: Use different eventq for cyclic channels

[Vinod Koul]

On Thu, Apr 10, 2014 at 11:36:30AM -0500, Joel Fernandes wrote:
> On 04/01/2014 08:06 AM, Peter Ujfalusi wrote:
> > To improve latency with cyclic DMA operation it is preferred to
> > use different eventq/tc than the default which is used by all
> > other drivers (mmc, spi, i2c, etc).
> > When preparing the cyclic dma ask for non default queue for the
> > channel which is going to be used with cyclic mode.
> > 
> > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> > ---
> >  drivers/dma/edma.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
> > index 1dd9e8806975..10048b40fac8 100644
> > --- a/drivers/dma/edma.c
> > +++ b/drivers/dma/edma.c
> > @@ -628,6 +628,9 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
> >  		edesc->pset[i].opt |= TCINTEN;
> >  	}
> >  
> > +	/* Use different eventq/tc for cyclic DMA to reduce latency */
> > +	edma_request_non_default_queue(echan->ch_num);
> > +
> >  	return vchan_tx_prep(&echan->vchan, &edesc->vdesc, tx_flags);
> >  }
> >  
> > 
> 
> Is there any way to guarantee that the non-default queue is of the
> highest priority, or in other words default queue is of lowest priority.
well as we are discussing in other thread, it would make sense to pass the
required priority (i am using audio so pls give me highest one)

-- 
~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

-- 

Re: [PATCH v2 08/14] DMA: edma: Use different eventq for cyclic channels

[Vinod Koul]

On Thu, Apr 10, 2014 at 11:36:30AM -0500, Joel Fernandes wrote:
> On 04/01/2014 08:06 AM, Peter Ujfalusi wrote:
> > To improve latency with cyclic DMA operation it is preferred to
> > use different eventq/tc than the default which is used by all
> > other drivers (mmc, spi, i2c, etc).
> > When preparing the cyclic dma ask for non default queue for the
> > channel which is going to be used with cyclic mode.
> > 
> > Signed-off-by: Peter Ujfalusi <peter.ujfalusi-l0cyMroinI0@public.gmane.org>
> > ---
> >  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

-- 

Re: [PATCH v2 08/14] DMA: edma: Use different eventq for cyclic channels

[Joel Fernandes]

On 04/11/2014 11:47 AM, Vinod Koul wrote:
> On Thu, Apr 10, 2014 at 11:36:30AM -0500, Joel Fernandes wrote:
>> On 04/01/2014 08:06 AM, Peter Ujfalusi wrote:
>>> To improve latency with cyclic DMA operation it is preferred to
>>> use different eventq/tc than the default which is used by all
>>> other drivers (mmc, spi, i2c, etc).
>>> When preparing the cyclic dma ask for non default queue for the
>>> channel which is going to be used with cyclic mode.
>>>
>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>>> ---
>>>  drivers/dma/edma.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
>>> index 1dd9e8806975..10048b40fac8 100644
>>> --- a/drivers/dma/edma.c
>>> +++ b/drivers/dma/edma.c
>>> @@ -628,6 +628,9 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
>>>  		edesc->pset[i].opt |= TCINTEN;
>>>  	}
>>>  
>>> +	/* Use different eventq/tc for cyclic DMA to reduce latency */
>>> +	edma_request_non_default_queue(echan->ch_num);
>>> +
>>>  	return vchan_tx_prep(&echan->vchan, &edesc->vdesc, tx_flags);
>>>  }
>>>  
>>>
>>
>> Is there any way to guarantee that the non-default queue is of the
>> highest priority, or in other words default queue is of lowest priority.
> well as we are discussing in other thread, it would make sense to pass the
> required priority (i am using audio so pls give me highest one)
> 

Yes, I'm aware of that part of the discussion ;) I also replied there.
This post was sent much earlier on.

thanks,

-Joel

[PATCH v2 08/14] DMA: edma: Use different eventq for cyclic channels

[Joel Fernandes]

On 04/11/2014 11:47 AM, Vinod Koul wrote:
> On Thu, Apr 10, 2014 at 11:36:30AM -0500, Joel Fernandes wrote:
>> On 04/01/2014 08:06 AM, Peter Ujfalusi wrote:
>>> To improve latency with cyclic DMA operation it is preferred to
>>> use different eventq/tc than the default which is used by all
>>> other drivers (mmc, spi, i2c, etc).
>>> When preparing the cyclic dma ask for non default queue for the
>>> channel which is going to be used with cyclic mode.
>>>
>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>>> ---
>>>  drivers/dma/edma.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
>>> index 1dd9e8806975..10048b40fac8 100644
>>> --- a/drivers/dma/edma.c
>>> +++ b/drivers/dma/edma.c
>>> @@ -628,6 +628,9 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
>>>  		edesc->pset[i].opt |= TCINTEN;
>>>  	}
>>>  
>>> +	/* Use different eventq/tc for cyclic DMA to reduce latency */
>>> +	edma_request_non_default_queue(echan->ch_num);
>>> +
>>>  	return vchan_tx_prep(&echan->vchan, &edesc->vdesc, tx_flags);
>>>  }
>>>  
>>>
>>
>> Is there any way to guarantee that the non-default queue is of the
>> highest priority, or in other words default queue is of lowest priority.
> well as we are discussing in other thread, it would make sense to pass the
> required priority (i am using audio so pls give me highest one)
> 

Yes, I'm aware of that part of the discussion ;) I also replied there.
This post was sent much earlier on.

thanks,

-Joel

Re: [PATCH v2 08/14] DMA: edma: Use different eventq for cyclic channels

[Joel Fernandes]

On 04/11/2014 11:47 AM, Vinod Koul wrote:
> On Thu, Apr 10, 2014 at 11:36:30AM -0500, Joel Fernandes wrote:
>> On 04/01/2014 08:06 AM, Peter Ujfalusi wrote:
>>> To improve latency with cyclic DMA operation it is preferred to
>>> use different eventq/tc than the default which is used by all
>>> other drivers (mmc, spi, i2c, etc).
>>> When preparing the cyclic dma ask for non default queue for the
>>> channel which is going to be used with cyclic mode.
>>>
>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi-l0cyMroinI0@public.gmane.org>
>>> ---
>>>  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

[PATCH v2 09/14] dma: edma: Implement device_slave_caps callback

[Peter Ujfalusi]

With the callback implemented omap-dma can provide information to client
drivers regarding to supported address widths, directions, residue
granularity, etc.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 10048b40fac8..855766672aa9 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -855,6 +855,23 @@ static void __init edma_chan_init(struct edma_cc *ecc,
 	}
 }
 
+#define EDMA_DMA_BUSWIDTHS	(BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) | \
+				 BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | \
+				 BIT(DMA_SLAVE_BUSWIDTH_4_BYTES))
+
+static int edma_dma_device_slave_caps(struct dma_chan *dchan,
+				      struct dma_slave_caps *caps)
+{
+	caps->src_addr_widths = EDMA_DMA_BUSWIDTHS;
+	caps->dstn_addr_widths = EDMA_DMA_BUSWIDTHS;
+	caps->directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
+	caps->cmd_pause = true;
+	caps->cmd_terminate = true;
+	caps->residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
+
+	return 0;
+}
+
 static void edma_dma_init(struct edma_cc *ecc, struct dma_device *dma,
 			  struct device *dev)
 {
@@ -865,6 +882,7 @@ static void edma_dma_init(struct edma_cc *ecc, struct dma_device *dma,
 	dma->device_issue_pending = edma_issue_pending;
 	dma->device_tx_status = edma_tx_status;
 	dma->device_control = edma_control;
+	dma->device_slave_caps = edma_dma_device_slave_caps;
 	dma->dev = dev;
 
 	INIT_LIST_HEAD(&dma->channels);
-- 
1.9.1

[PATCH v2 09/14] dma: edma: Implement device_slave_caps callback

[Peter Ujfalusi]

With the callback implemented omap-dma can provide information to client
drivers regarding to supported address widths, directions, residue
granularity, etc.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 10048b40fac8..855766672aa9 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -855,6 +855,23 @@ static void __init edma_chan_init(struct edma_cc *ecc,
 	}
 }
 
+#define EDMA_DMA_BUSWIDTHS	(BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) | \
+				 BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | \
+				 BIT(DMA_SLAVE_BUSWIDTH_4_BYTES))
+
+static int edma_dma_device_slave_caps(struct dma_chan *dchan,
+				      struct dma_slave_caps *caps)
+{
+	caps->src_addr_widths = EDMA_DMA_BUSWIDTHS;
+	caps->dstn_addr_widths = EDMA_DMA_BUSWIDTHS;
+	caps->directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
+	caps->cmd_pause = true;
+	caps->cmd_terminate = true;
+	caps->residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
+
+	return 0;
+}
+
 static void edma_dma_init(struct edma_cc *ecc, struct dma_device *dma,
 			  struct device *dev)
 {
@@ -865,6 +882,7 @@ static void edma_dma_init(struct edma_cc *ecc, struct dma_device *dma,
 	dma->device_issue_pending = edma_issue_pending;
 	dma->device_tx_status = edma_tx_status;
 	dma->device_control = edma_control;
+	dma->device_slave_caps = edma_dma_device_slave_caps;
 	dma->dev = dev;
 
 	INIT_LIST_HEAD(&dma->channels);
-- 
1.9.1

[PATCH v2 09/14] dma: edma: Implement device_slave_caps callback

[Peter Ujfalusi]

With the callback implemented omap-dma can provide information to client
drivers regarding to supported address widths, directions, residue
granularity, etc.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi-l0cyMroinI0@public.gmane.org>
---
 drivers/dma/edma.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 10048b40fac8..855766672aa9 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -855,6 +855,23 @@ static void __init edma_chan_init(struct edma_cc *ecc,
 	}
 }
 
+#define EDMA_DMA_BUSWIDTHS	(BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) | \
+				 BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | \
+				 BIT(DMA_SLAVE_BUSWIDTH_4_BYTES))
+
+static int edma_dma_device_slave_caps(struct dma_chan *dchan,
+				      struct dma_slave_caps *caps)
+{
+	caps->src_addr_widths = EDMA_DMA_BUSWIDTHS;
+	caps->dstn_addr_widths = EDMA_DMA_BUSWIDTHS;
+	caps->directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
+	caps->cmd_pause = true;
+	caps->cmd_terminate = true;
+	caps->residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
+
+	return 0;
+}
+
 static void edma_dma_init(struct edma_cc *ecc, struct dma_device *dma,
 			  struct device *dev)
 {
@@ -865,6 +882,7 @@ static void edma_dma_init(struct edma_cc *ecc, struct dma_device *dma,
 	dma->device_issue_pending = edma_issue_pending;
 	dma->device_tx_status = edma_tx_status;
 	dma->device_control = edma_control;
+	dma->device_slave_caps = edma_dma_device_slave_caps;
 	dma->dev = dev;
 
 	INIT_LIST_HEAD(&dma->channels);
-- 
1.9.1

[PATCH v2 10/14] dma: edma: Simplify direction configuration in edma_config_pset()

[Peter Ujfalusi]

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.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 855766672aa9..d954099650ae 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -372,14 +372,12 @@ static int edma_config_pset(struct dma_chan *chan, struct edmacc_param *pset,
 		src_cidx = cidx;
 		dst_bidx = 0;
 		dst_cidx = 0;
-	} else if (direction == DMA_DEV_TO_MEM)  {
+	} else {
+		/* DMA_DEV_TO_MEM */
 		src_bidx = 0;
 		src_cidx = 0;
 		dst_bidx = acnt;
 		dst_cidx = cidx;
-	} else {
-		dev_err(dev, "%s: direction not implemented yet\n", __func__);
-		return -EINVAL;
 	}
 
 	pset->opt = EDMA_TCC(EDMA_CHAN_SLOT(echan->ch_num));
-- 
1.9.1

[PATCH v2 10/14] dma: edma: Simplify direction configuration in edma_config_pset()

[Peter Ujfalusi]

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.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 855766672aa9..d954099650ae 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -372,14 +372,12 @@ static int edma_config_pset(struct dma_chan *chan, struct edmacc_param *pset,
 		src_cidx = cidx;
 		dst_bidx = 0;
 		dst_cidx = 0;
-	} else if (direction == DMA_DEV_TO_MEM)  {
+	} else {
+		/* DMA_DEV_TO_MEM */
 		src_bidx = 0;
 		src_cidx = 0;
 		dst_bidx = acnt;
 		dst_cidx = cidx;
-	} else {
-		dev_err(dev, "%s: direction not implemented yet\n", __func__);
-		return -EINVAL;
 	}
 
 	pset->opt = EDMA_TCC(EDMA_CHAN_SLOT(echan->ch_num));
-- 
1.9.1

[PATCH v2 10/14] dma: edma: Simplify direction configuration in edma_config_pset()

[Peter Ujfalusi]

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.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi-l0cyMroinI0@public.gmane.org>
---
 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));
-- 
1.9.1

Re: [PATCH v2 10/14] dma: edma: Simplify direction configuration in edma_config_pset()

[Joel Fernandes]

On 04/01/2014 08:06 AM, Peter Ujfalusi wrote:
> We only support DEV_TO_MEM or MEM_TO_DEV directions with edma driver and the
> check for the direction has been already done in the function calling
> edma_config_pset().
> The error reporting is redundant and also the "else if ()" can be removed.
> 

NAK. Please don't do this. I have been working on MEM_TO_MEM support as
well so leave it as it is for future.

Thanks,
-Joel

> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/dma/edma.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
> index 855766672aa9..d954099650ae 100644
> --- a/drivers/dma/edma.c
> +++ b/drivers/dma/edma.c
> @@ -372,14 +372,12 @@ static int edma_config_pset(struct dma_chan *chan, struct edmacc_param *pset,
>  		src_cidx = cidx;
>  		dst_bidx = 0;
>  		dst_cidx = 0;
> -	} else if (direction == DMA_DEV_TO_MEM)  {
> +	} else {
> +		/* DMA_DEV_TO_MEM */
>  		src_bidx = 0;
>  		src_cidx = 0;
>  		dst_bidx = acnt;
>  		dst_cidx = cidx;
> -	} else {
> -		dev_err(dev, "%s: direction not implemented yet\n", __func__);
> -		return -EINVAL;
>  	}
>  
>  	pset->opt = EDMA_TCC(EDMA_CHAN_SLOT(echan->ch_num));
> 

[PATCH v2 10/14] dma: edma: Simplify direction configuration in edma_config_pset()

[Joel Fernandes]

On 04/01/2014 08:06 AM, Peter Ujfalusi wrote:
> We only support DEV_TO_MEM or MEM_TO_DEV directions with edma driver and the
> check for the direction has been already done in the function calling
> edma_config_pset().
> The error reporting is redundant and also the "else if ()" can be removed.
> 

NAK. Please don't do this. I have been working on MEM_TO_MEM support as
well so leave it as it is for future.

Thanks,
-Joel

> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/dma/edma.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
> index 855766672aa9..d954099650ae 100644
> --- a/drivers/dma/edma.c
> +++ b/drivers/dma/edma.c
> @@ -372,14 +372,12 @@ static int edma_config_pset(struct dma_chan *chan, struct edmacc_param *pset,
>  		src_cidx = cidx;
>  		dst_bidx = 0;
>  		dst_cidx = 0;
> -	} else if (direction == DMA_DEV_TO_MEM)  {
> +	} else {
> +		/* DMA_DEV_TO_MEM */
>  		src_bidx = 0;
>  		src_cidx = 0;
>  		dst_bidx = acnt;
>  		dst_cidx = cidx;
> -	} else {
> -		dev_err(dev, "%s: direction not implemented yet\n", __func__);
> -		return -EINVAL;
>  	}
>  
>  	pset->opt = EDMA_TCC(EDMA_CHAN_SLOT(echan->ch_num));
> 

Re: [PATCH v2 10/14] dma: edma: Simplify direction configuration in edma_config_pset()

[Joel Fernandes]

On 04/01/2014 08:06 AM, Peter Ujfalusi wrote:
> We only support DEV_TO_MEM or MEM_TO_DEV directions with edma driver and the
> check for the direction has been already done in the function calling
> edma_config_pset().
> The error reporting is redundant and also the "else if ()" can be removed.
> 

NAK. Please don't do this. I have been working on MEM_TO_MEM support as
well so leave it as it is for future.

Thanks,
-Joel

> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/dma/edma.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
> index 855766672aa9..d954099650ae 100644
> --- a/drivers/dma/edma.c
> +++ b/drivers/dma/edma.c
> @@ -372,14 +372,12 @@ static int edma_config_pset(struct dma_chan *chan, struct edmacc_param *pset,
>  		src_cidx = cidx;
>  		dst_bidx = 0;
>  		dst_cidx = 0;
> -	} else if (direction == DMA_DEV_TO_MEM)  {
> +	} else {
> +		/* DMA_DEV_TO_MEM */
>  		src_bidx = 0;
>  		src_cidx = 0;
>  		dst_bidx = acnt;
>  		dst_cidx = cidx;
> -	} else {
> -		dev_err(dev, "%s: direction not implemented yet\n", __func__);
> -		return -EINVAL;
>  	}
>  
>  	pset->opt = EDMA_TCC(EDMA_CHAN_SLOT(echan->ch_num));
> 

Re: [PATCH v2 10/14] dma: edma: Simplify direction configuration in edma_config_pset()

[Peter Ujfalusi]

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.

I'll leave this out from v3.

-- 
Péter

> 
> Thanks,
> -Joel
> 
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>>  drivers/dma/edma.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
>> index 855766672aa9..d954099650ae 100644
>> --- a/drivers/dma/edma.c
>> +++ b/drivers/dma/edma.c
>> @@ -372,14 +372,12 @@ static int edma_config_pset(struct dma_chan *chan, struct edmacc_param *pset,
>>  		src_cidx = cidx;
>>  		dst_bidx = 0;
>>  		dst_cidx = 0;
>> -	} else if (direction == DMA_DEV_TO_MEM)  {
>> +	} else {
>> +		/* DMA_DEV_TO_MEM */
>>  		src_bidx = 0;
>>  		src_cidx = 0;
>>  		dst_bidx = acnt;
>>  		dst_cidx = cidx;
>> -	} else {
>> -		dev_err(dev, "%s: direction not implemented yet\n", __func__);
>> -		return -EINVAL;
>>  	}
>>  
>>  	pset->opt = EDMA_TCC(EDMA_CHAN_SLOT(echan->ch_num));
>>
> 

[PATCH v2 10/14] dma: edma: Simplify direction configuration in edma_config_pset()

[Peter Ujfalusi]

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.

I'll leave this out from v3.

-- 
P?ter

> 
> Thanks,
> -Joel
> 
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>>  drivers/dma/edma.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
>> index 855766672aa9..d954099650ae 100644
>> --- a/drivers/dma/edma.c
>> +++ b/drivers/dma/edma.c
>> @@ -372,14 +372,12 @@ static int edma_config_pset(struct dma_chan *chan, struct edmacc_param *pset,
>>  		src_cidx = cidx;
>>  		dst_bidx = 0;
>>  		dst_cidx = 0;
>> -	} else if (direction == DMA_DEV_TO_MEM)  {
>> +	} else {
>> +		/* DMA_DEV_TO_MEM */
>>  		src_bidx = 0;
>>  		src_cidx = 0;
>>  		dst_bidx = acnt;
>>  		dst_cidx = cidx;
>> -	} else {
>> -		dev_err(dev, "%s: direction not implemented yet\n", __func__);
>> -		return -EINVAL;
>>  	}
>>  
>>  	pset->opt = EDMA_TCC(EDMA_CHAN_SLOT(echan->ch_num));
>>
> 

Re: [PATCH v2 10/14] dma: edma: Simplify direction configuration in edma_config_pset()

[Peter Ujfalusi]

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.

I'll leave this out from v3.

-- 
Péter

> 
> Thanks,
> -Joel
> 
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi-l0cyMroinI0@public.gmane.org>
>> ---
>>  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));
>>
> 

Re: [PATCH v2 10/14] dma: edma: Simplify direction configuration in edma_config_pset()

[Joel Fernandes]

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

[PATCH v2 10/14] dma: edma: Simplify direction configuration in edma_config_pset()

[Joel Fernandes]

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

Re: [PATCH v2 10/14] dma: edma: Simplify direction configuration in edma_config_pset()

[Joel Fernandes]

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

[PATCH v2 11/14] dma: edma: Reduce debug print verbosity for non verbose debugging

[Peter Ujfalusi]

Do not print the paRAM information when verbose debugging is not asked and
also reduce the number of lines printed in edma_prep_dma_cyclic()

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index d954099650ae..e8de2e84a60a 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -141,7 +141,7 @@ static void edma_execute(struct edma_chan *echan)
 	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,
+		dev_vdbg(echan->vchan.chan.device->dev,
 			"\n pset[%d]:\n"
 			"  chnum\t%d\n"
 			"  slot\t%d\n"
@@ -558,9 +558,8 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
 	edesc->cyclic = 1;
 	edesc->pset_nr = nslots;
 
-	dev_dbg(dev, "%s: nslots=%d\n", __func__, nslots);
-	dev_dbg(dev, "%s: period_len=%d\n", __func__, period_len);
-	dev_dbg(dev, "%s: buf_len=%d\n", __func__, buf_len);
+	dev_dbg(dev, "%s: channel=%d nslots=%d period_len=%zu buf_len=%zu\n",
+		__func__, echan->ch_num, nslots, period_len, buf_len);
 
 	for (i = 0; i < nslots; i++) {
 		/* Allocate a PaRAM slot, if needed */
@@ -594,8 +593,8 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
 		else
 			src_addr += period_len;
 
-		dev_dbg(dev, "%s: Configure period %d of buf:\n", __func__, i);
-		dev_dbg(dev,
+		dev_vdbg(dev, "%s: Configure period %d of buf:\n", __func__, i);
+		dev_vdbg(dev,
 			"\n pset[%d]:\n"
 			"  chnum\t%d\n"
 			"  slot\t%d\n"
-- 
1.9.1

[PATCH v2 11/14] dma: edma: Reduce debug print verbosity for non verbose debugging

[Peter Ujfalusi]

Do not print the paRAM information when verbose debugging is not asked and
also reduce the number of lines printed in edma_prep_dma_cyclic()

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index d954099650ae..e8de2e84a60a 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -141,7 +141,7 @@ static void edma_execute(struct edma_chan *echan)
 	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,
+		dev_vdbg(echan->vchan.chan.device->dev,
 			"\n pset[%d]:\n"
 			"  chnum\t%d\n"
 			"  slot\t%d\n"
@@ -558,9 +558,8 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
 	edesc->cyclic = 1;
 	edesc->pset_nr = nslots;
 
-	dev_dbg(dev, "%s: nslots=%d\n", __func__, nslots);
-	dev_dbg(dev, "%s: period_len=%d\n", __func__, period_len);
-	dev_dbg(dev, "%s: buf_len=%d\n", __func__, buf_len);
+	dev_dbg(dev, "%s: channel=%d nslots=%d period_len=%zu buf_len=%zu\n",
+		__func__, echan->ch_num, nslots, period_len, buf_len);
 
 	for (i = 0; i < nslots; i++) {
 		/* Allocate a PaRAM slot, if needed */
@@ -594,8 +593,8 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
 		else
 			src_addr += period_len;
 
-		dev_dbg(dev, "%s: Configure period %d of buf:\n", __func__, i);
-		dev_dbg(dev,
+		dev_vdbg(dev, "%s: Configure period %d of buf:\n", __func__, i);
+		dev_vdbg(dev,
 			"\n pset[%d]:\n"
 			"  chnum\t%d\n"
 			"  slot\t%d\n"
-- 
1.9.1

[PATCH v2 11/14] dma: edma: Reduce debug print verbosity for non verbose debugging

[Peter Ujfalusi]

Do not print the paRAM information when verbose debugging is not asked and
also reduce the number of lines printed in edma_prep_dma_cyclic()

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index d954099650ae..e8de2e84a60a 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -141,7 +141,7 @@ static void edma_execute(struct edma_chan *echan)
 	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,
+		dev_vdbg(echan->vchan.chan.device->dev,
 			"\n pset[%d]:\n"
 			"  chnum\t%d\n"
 			"  slot\t%d\n"
@@ -558,9 +558,8 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
 	edesc->cyclic = 1;
 	edesc->pset_nr = nslots;
 
-	dev_dbg(dev, "%s: nslots=%d\n", __func__, nslots);
-	dev_dbg(dev, "%s: period_len=%d\n", __func__, period_len);
-	dev_dbg(dev, "%s: buf_len=%d\n", __func__, buf_len);
+	dev_dbg(dev, "%s: channel=%d nslots=%d period_len=%zu buf_len=%zu\n",
+		__func__, echan->ch_num, nslots, period_len, buf_len);
 
 	for (i = 0; i < nslots; i++) {
 		/* Allocate a PaRAM slot, if needed */
@@ -594,8 +593,8 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
 		else
 			src_addr += period_len;
 
-		dev_dbg(dev, "%s: Configure period %d of buf:\n", __func__, i);
-		dev_dbg(dev,
+		dev_vdbg(dev, "%s: Configure period %d of buf:\n", __func__, i);
+		dev_vdbg(dev,
 			"\n pset[%d]:\n"
 			"  chnum\t%d\n"
 			"  slot\t%d\n"
-- 
1.9.1

[PATCH v2 12/14] dma: edma: Prefix debug prints where the text were identical in prep callbacks

[Peter Ujfalusi]

prep_slave_sg and prep_dma_cyclic callbacks have mostly same failure cases
with the same texts printed in case we hit them. It helps when debugging if
we know exactly which callback generated the errors.
At the same time change the debug level for descriptor allocation failure
from dbg to err since all other error cases are dev_err and this failure is
similarly fatal as the other ones.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index e8de2e84a60a..7feb6231276e 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -434,14 +434,14 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
 	}
 
 	if (dev_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) {
-		dev_err(dev, "Undefined slave buswidth\n");
+		dev_err(dev, "%s: Undefined slave buswidth\n", __func__);
 		return NULL;
 	}
 
 	edesc = kzalloc(sizeof(*edesc) + sg_len *
 		sizeof(edesc->pset[0]), GFP_ATOMIC);
 	if (!edesc) {
-		dev_dbg(dev, "Failed to allocate a descriptor\n");
+		dev_err(dev, "%s: Failed to allocate a descriptor\n", __func__);
 		return NULL;
 	}
 
@@ -457,7 +457,8 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
 						EDMA_SLOT_ANY);
 			if (echan->slot[i] < 0) {
 				kfree(edesc);
-				dev_err(dev, "Failed to allocate slot\n");
+				dev_err(dev, "%s: Failed to allocate slot\n",
+					__func__);
 				return NULL;
 			}
 		}
@@ -526,7 +527,7 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
 	}
 
 	if (dev_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) {
-		dev_err(dev, "Undefined slave buswidth\n");
+		dev_err(dev, "%s: Undefined slave buswidth\n", __func__);
 		return NULL;
 	}
 
@@ -551,7 +552,7 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
 	edesc = kzalloc(sizeof(*edesc) + nslots *
 		sizeof(edesc->pset[0]), GFP_ATOMIC);
 	if (!edesc) {
-		dev_dbg(dev, "Failed to allocate a descriptor\n");
+		dev_err(dev, "%s: Failed to allocate a descriptor\n", __func__);
 		return NULL;
 	}
 
@@ -569,7 +570,8 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
 						EDMA_SLOT_ANY);
 			if (echan->slot[i] < 0) {
 				kfree(edesc);
-				dev_err(dev, "Failed to allocate slot\n");
+				dev_err(dev, "%s: Failed to allocate slot\n",
+					__func__);
 				return NULL;
 			}
 		}
-- 
1.9.1

[PATCH v2 12/14] dma: edma: Prefix debug prints where the text were identical in prep callbacks

[Peter Ujfalusi]

prep_slave_sg and prep_dma_cyclic callbacks have mostly same failure cases
with the same texts printed in case we hit them. It helps when debugging if
we know exactly which callback generated the errors.
At the same time change the debug level for descriptor allocation failure
from dbg to err since all other error cases are dev_err and this failure is
similarly fatal as the other ones.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index e8de2e84a60a..7feb6231276e 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -434,14 +434,14 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
 	}
 
 	if (dev_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) {
-		dev_err(dev, "Undefined slave buswidth\n");
+		dev_err(dev, "%s: Undefined slave buswidth\n", __func__);
 		return NULL;
 	}
 
 	edesc = kzalloc(sizeof(*edesc) + sg_len *
 		sizeof(edesc->pset[0]), GFP_ATOMIC);
 	if (!edesc) {
-		dev_dbg(dev, "Failed to allocate a descriptor\n");
+		dev_err(dev, "%s: Failed to allocate a descriptor\n", __func__);
 		return NULL;
 	}
 
@@ -457,7 +457,8 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
 						EDMA_SLOT_ANY);
 			if (echan->slot[i] < 0) {
 				kfree(edesc);
-				dev_err(dev, "Failed to allocate slot\n");
+				dev_err(dev, "%s: Failed to allocate slot\n",
+					__func__);
 				return NULL;
 			}
 		}
@@ -526,7 +527,7 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
 	}
 
 	if (dev_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) {
-		dev_err(dev, "Undefined slave buswidth\n");
+		dev_err(dev, "%s: Undefined slave buswidth\n", __func__);
 		return NULL;
 	}
 
@@ -551,7 +552,7 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
 	edesc = kzalloc(sizeof(*edesc) + nslots *
 		sizeof(edesc->pset[0]), GFP_ATOMIC);
 	if (!edesc) {
-		dev_dbg(dev, "Failed to allocate a descriptor\n");
+		dev_err(dev, "%s: Failed to allocate a descriptor\n", __func__);
 		return NULL;
 	}
 
@@ -569,7 +570,8 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
 						EDMA_SLOT_ANY);
 			if (echan->slot[i] < 0) {
 				kfree(edesc);
-				dev_err(dev, "Failed to allocate slot\n");
+				dev_err(dev, "%s: Failed to allocate slot\n",
+					__func__);
 				return NULL;
 			}
 		}
-- 
1.9.1

[PATCH v2 12/14] dma: edma: Prefix debug prints where the text were identical in prep callbacks

[Peter Ujfalusi]

prep_slave_sg and prep_dma_cyclic callbacks have mostly same failure cases
with the same texts printed in case we hit them. It helps when debugging if
we know exactly which callback generated the errors.
At the same time change the debug level for descriptor allocation failure
from dbg to err since all other error cases are dev_err and this failure is
similarly fatal as the other ones.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index e8de2e84a60a..7feb6231276e 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -434,14 +434,14 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
 	}
 
 	if (dev_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) {
-		dev_err(dev, "Undefined slave buswidth\n");
+		dev_err(dev, "%s: Undefined slave buswidth\n", __func__);
 		return NULL;
 	}
 
 	edesc = kzalloc(sizeof(*edesc) + sg_len *
 		sizeof(edesc->pset[0]), GFP_ATOMIC);
 	if (!edesc) {
-		dev_dbg(dev, "Failed to allocate a descriptor\n");
+		dev_err(dev, "%s: Failed to allocate a descriptor\n", __func__);
 		return NULL;
 	}
 
@@ -457,7 +457,8 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
 						EDMA_SLOT_ANY);
 			if (echan->slot[i] < 0) {
 				kfree(edesc);
-				dev_err(dev, "Failed to allocate slot\n");
+				dev_err(dev, "%s: Failed to allocate slot\n",
+					__func__);
 				return NULL;
 			}
 		}
@@ -526,7 +527,7 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
 	}
 
 	if (dev_width == DMA_SLAVE_BUSWIDTH_UNDEFINED) {
-		dev_err(dev, "Undefined slave buswidth\n");
+		dev_err(dev, "%s: Undefined slave buswidth\n", __func__);
 		return NULL;
 	}
 
@@ -551,7 +552,7 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
 	edesc = kzalloc(sizeof(*edesc) + nslots *
 		sizeof(edesc->pset[0]), GFP_ATOMIC);
 	if (!edesc) {
-		dev_dbg(dev, "Failed to allocate a descriptor\n");
+		dev_err(dev, "%s: Failed to allocate a descriptor\n", __func__);
 		return NULL;
 	}
 
@@ -569,7 +570,8 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
 						EDMA_SLOT_ANY);
 			if (echan->slot[i] < 0) {
 				kfree(edesc);
-				dev_err(dev, "Failed to allocate slot\n");
+				dev_err(dev, "%s: Failed to allocate slot\n",
+					__func__);
 				return NULL;
 			}
 		}
-- 
1.9.1

[PATCH v2 13/14] dma: edma: Add channel number to debug prints

[Peter Ujfalusi]

It helps to identify issues if we have some information regarding to the
channel which the event is associated.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 7feb6231276e..2413237840f8 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -185,7 +185,8 @@ static void edma_execute(struct edma_chan *echan)
 	edma_resume(echan->ch_num);
 
 	if (edesc->processed <= MAX_NR_SG) {
-		dev_dbg(dev, "first transfer starting %d\n", echan->ch_num);
+		dev_dbg(dev, "first transfer starting on channel %d\n",
+			echan->ch_num);
 		edma_start(echan->ch_num);
 	}
 
@@ -195,7 +196,7 @@ static void edma_execute(struct edma_chan *echan)
 	 * MAX_NR_SG
 	 */
 	if (echan->missed) {
-		dev_dbg(dev, "missed event in execute detected\n");
+		dev_dbg(dev, "missed event on channel %d\n", echan->ch_num);
 		edma_clean_channel(echan->ch_num);
 		edma_stop(echan->ch_num);
 		edma_start(echan->ch_num);
@@ -736,7 +737,7 @@ static int edma_alloc_chan_resources(struct dma_chan *chan)
 	echan->alloced = true;
 	echan->slot[0] = echan->ch_num;
 
-	dev_dbg(dev, "allocated channel for %u:%u\n",
+	dev_dbg(dev, "allocated channel %d for %u:%u\n", echan->ch_num,
 		EDMA_CTLR(echan->ch_num), EDMA_CHAN_SLOT(echan->ch_num));
 
 	return 0;
-- 
1.9.1

[PATCH v2 13/14] dma: edma: Add channel number to debug prints

[Peter Ujfalusi]

It helps to identify issues if we have some information regarding to the
channel which the event is associated.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 7feb6231276e..2413237840f8 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -185,7 +185,8 @@ static void edma_execute(struct edma_chan *echan)
 	edma_resume(echan->ch_num);
 
 	if (edesc->processed <= MAX_NR_SG) {
-		dev_dbg(dev, "first transfer starting %d\n", echan->ch_num);
+		dev_dbg(dev, "first transfer starting on channel %d\n",
+			echan->ch_num);
 		edma_start(echan->ch_num);
 	}
 
@@ -195,7 +196,7 @@ static void edma_execute(struct edma_chan *echan)
 	 * MAX_NR_SG
 	 */
 	if (echan->missed) {
-		dev_dbg(dev, "missed event in execute detected\n");
+		dev_dbg(dev, "missed event on channel %d\n", echan->ch_num);
 		edma_clean_channel(echan->ch_num);
 		edma_stop(echan->ch_num);
 		edma_start(echan->ch_num);
@@ -736,7 +737,7 @@ static int edma_alloc_chan_resources(struct dma_chan *chan)
 	echan->alloced = true;
 	echan->slot[0] = echan->ch_num;
 
-	dev_dbg(dev, "allocated channel for %u:%u\n",
+	dev_dbg(dev, "allocated channel %d for %u:%u\n", echan->ch_num,
 		EDMA_CTLR(echan->ch_num), EDMA_CHAN_SLOT(echan->ch_num));
 
 	return 0;
-- 
1.9.1

[PATCH v2 13/14] dma: edma: Add channel number to debug prints

[Peter Ujfalusi]

It helps to identify issues if we have some information regarding to the
channel which the event is associated.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 7feb6231276e..2413237840f8 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -185,7 +185,8 @@ static void edma_execute(struct edma_chan *echan)
 	edma_resume(echan->ch_num);
 
 	if (edesc->processed <= MAX_NR_SG) {
-		dev_dbg(dev, "first transfer starting %d\n", echan->ch_num);
+		dev_dbg(dev, "first transfer starting on channel %d\n",
+			echan->ch_num);
 		edma_start(echan->ch_num);
 	}
 
@@ -195,7 +196,7 @@ static void edma_execute(struct edma_chan *echan)
 	 * MAX_NR_SG
 	 */
 	if (echan->missed) {
-		dev_dbg(dev, "missed event in execute detected\n");
+		dev_dbg(dev, "missed event on channel %d\n", echan->ch_num);
 		edma_clean_channel(echan->ch_num);
 		edma_stop(echan->ch_num);
 		edma_start(echan->ch_num);
@@ -736,7 +737,7 @@ static int edma_alloc_chan_resources(struct dma_chan *chan)
 	echan->alloced = true;
 	echan->slot[0] = echan->ch_num;
 
-	dev_dbg(dev, "allocated channel for %u:%u\n",
+	dev_dbg(dev, "allocated channel %d for %u:%u\n", echan->ch_num,
 		EDMA_CTLR(echan->ch_num), EDMA_CHAN_SLOT(echan->ch_num));
 
 	return 0;
-- 
1.9.1

[PATCH v2 14/14] dma: edma: Print the direction value as well when it is not supported

[Peter Ujfalusi]

In case of not supported direction it is better to print the direction also.
It is unlikely, but in such an event it helps with the debugging.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 2413237840f8..912bf0190f45 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -430,7 +430,7 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
 		dev_width = echan->cfg.dst_addr_width;
 		burst = echan->cfg.dst_maxburst;
 	} else {
-		dev_err(dev, "%s: bad direction?\n", __func__);
+		dev_err(dev, "%s: bad direction: %d\n", __func__, direction);
 		return NULL;
 	}
 
@@ -523,7 +523,7 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
 		dev_width = echan->cfg.dst_addr_width;
 		burst = echan->cfg.dst_maxburst;
 	} else {
-		dev_err(dev, "%s: bad direction?\n", __func__);
+		dev_err(dev, "%s: bad direction: %d\n", __func__, direction);
 		return NULL;
 	}
 
-- 
1.9.1

[PATCH v2 14/14] dma: edma: Print the direction value as well when it is not supported

[Peter Ujfalusi]

In case of not supported direction it is better to print the direction also.
It is unlikely, but in such an event it helps with the debugging.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 2413237840f8..912bf0190f45 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -430,7 +430,7 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
 		dev_width = echan->cfg.dst_addr_width;
 		burst = echan->cfg.dst_maxburst;
 	} else {
-		dev_err(dev, "%s: bad direction?\n", __func__);
+		dev_err(dev, "%s: bad direction: %d\n", __func__, direction);
 		return NULL;
 	}
 
@@ -523,7 +523,7 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
 		dev_width = echan->cfg.dst_addr_width;
 		burst = echan->cfg.dst_maxburst;
 	} else {
-		dev_err(dev, "%s: bad direction?\n", __func__);
+		dev_err(dev, "%s: bad direction: %d\n", __func__, direction);
 		return NULL;
 	}
 
-- 
1.9.1

[PATCH v2 14/14] dma: edma: Print the direction value as well when it is not supported

[Peter Ujfalusi]

In case of not supported direction it is better to print the direction also.
It is unlikely, but in such an event it helps with the debugging.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 2413237840f8..912bf0190f45 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -430,7 +430,7 @@ static struct dma_async_tx_descriptor *edma_prep_slave_sg(
 		dev_width = echan->cfg.dst_addr_width;
 		burst = echan->cfg.dst_maxburst;
 	} else {
-		dev_err(dev, "%s: bad direction?\n", __func__);
+		dev_err(dev, "%s: bad direction: %d\n", __func__, direction);
 		return NULL;
 	}
 
@@ -523,7 +523,7 @@ static struct dma_async_tx_descriptor *edma_prep_dma_cyclic(
 		dev_width = echan->cfg.dst_addr_width;
 		burst = echan->cfg.dst_maxburst;
 	} else {
-		dev_err(dev, "%s: bad direction?\n", __func__);
+		dev_err(dev, "%s: bad direction: %d\n", __func__, direction);
 		return NULL;
 	}
 
-- 
1.9.1

Re: [PATCH v2 00/14] dma: edma: Fixes for cyclic (audio) operation

[Joel Fernandes]

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 <joelf@ti.com>

Thanks,
-Joel

On 04/01/2014 08:06 AM, Peter Ujfalusi wrote:
> Hi,
> 
> This is basically a resend of the previous series:
> https://lkml.org/lkml/2014/3/13/119
> with removed ASoC patches (most of them are applied already).
> 
> Changes since v1:
> - ASoC patches removed
> - Comments from Andriy Shevchenko addressed
> - patch added to fix cases when src/dst_maxburst is set to 0
> 
> Adding support for DMA pause/resume
> Possibility to select non default event queue/TC for cyclic (audio) dma
> channels: all devices using the eDMA via dmaengine was assigned to the default
> EQ/TC (mmc, i2c, spi, etc, and audio). This is not optimal from system
> performance point of view since sharing the same EQ/TC can cause latency spikes
> for cyclic channels (long DMA transfers for MMC for example).
> 
> While debugging the edma to get things sorted out I noticed that the debug was
> too verbose and the important information was hidden even when the we did not
> asked for verbose dmaengine debug.
> I have included some debug cleanups for the edma dmaengine driver also.
> 
> Regards,
> Peter
> ---
> Peter Ujfalusi (14):
>   platform_data: edma: Be precise with the paRAM struct
>   dma: edma: Correct the handling of src/dst_maxburst == 0
>   dma: edma: Add support for DMA_PAUSE/RESUME operation
>   dma: edma: Set DMA_CYCLIC capability flag
>   arm: common: edma: Select event queue 1 as default when booted with DT
>   arm: common: edma: Save the number of event queues/TCs
>   arm: common: edma: API to request non default queue for a channel
>   DMA: edma: Use different eventq for cyclic channels
>   dma: edma: Implement device_slave_caps callback
>   dma: edma: Simplify direction configuration in edma_config_pset()
>   dma: edma: Reduce debug print verbosity for non verbose debugging
>   dma: edma: Prefix debug prints where the text were identical in prep
>     callbacks
>   dma: edma: Add channel number to debug prints
>   dma: edma: Print the direction value as well when it is not supported
> 
>  arch/arm/common/edma.c             | 34 +++++++++++++-
>  drivers/dma/edma.c                 | 96 +++++++++++++++++++++++++++++---------
>  include/linux/platform_data/edma.h | 20 ++++----
>  3 files changed, 119 insertions(+), 31 deletions(-)
> 

[PATCH v2 00/14] dma: edma: Fixes for cyclic (audio) operation

[Joel Fernandes]

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 <joelf@ti.com>

Thanks,
-Joel

On 04/01/2014 08:06 AM, Peter Ujfalusi wrote:
> Hi,
> 
> This is basically a resend of the previous series:
> https://lkml.org/lkml/2014/3/13/119
> with removed ASoC patches (most of them are applied already).
> 
> Changes since v1:
> - ASoC patches removed
> - Comments from Andriy Shevchenko addressed
> - patch added to fix cases when src/dst_maxburst is set to 0
> 
> Adding support for DMA pause/resume
> Possibility to select non default event queue/TC for cyclic (audio) dma
> channels: all devices using the eDMA via dmaengine was assigned to the default
> EQ/TC (mmc, i2c, spi, etc, and audio). This is not optimal from system
> performance point of view since sharing the same EQ/TC can cause latency spikes
> for cyclic channels (long DMA transfers for MMC for example).
> 
> While debugging the edma to get things sorted out I noticed that the debug was
> too verbose and the important information was hidden even when the we did not
> asked for verbose dmaengine debug.
> I have included some debug cleanups for the edma dmaengine driver also.
> 
> Regards,
> Peter
> ---
> Peter Ujfalusi (14):
>   platform_data: edma: Be precise with the paRAM struct
>   dma: edma: Correct the handling of src/dst_maxburst == 0
>   dma: edma: Add support for DMA_PAUSE/RESUME operation
>   dma: edma: Set DMA_CYCLIC capability flag
>   arm: common: edma: Select event queue 1 as default when booted with DT
>   arm: common: edma: Save the number of event queues/TCs
>   arm: common: edma: API to request non default queue for a channel
>   DMA: edma: Use different eventq for cyclic channels
>   dma: edma: Implement device_slave_caps callback
>   dma: edma: Simplify direction configuration in edma_config_pset()
>   dma: edma: Reduce debug print verbosity for non verbose debugging
>   dma: edma: Prefix debug prints where the text were identical in prep
>     callbacks
>   dma: edma: Add channel number to debug prints
>   dma: edma: Print the direction value as well when it is not supported
> 
>  arch/arm/common/edma.c             | 34 +++++++++++++-
>  drivers/dma/edma.c                 | 96 +++++++++++++++++++++++++++++---------
>  include/linux/platform_data/edma.h | 20 ++++----
>  3 files changed, 119 insertions(+), 31 deletions(-)
> 

Re: [PATCH v2 00/14] dma: edma: Fixes for cyclic (audio) operation

[Joel Fernandes]

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 <joelf-l0cyMroinI0@public.gmane.org>

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(-)
> 

Re: [PATCH v2 00/14] dma: edma: Fixes for cyclic (audio) operation

[Vinod Koul]

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: xxxx 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
> 

-- 

[PATCH v2 00/14] dma: edma: Fixes for cyclic (audio) operation

[Vinod Koul]

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: xxxx 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
> 

--