[PATCH v3 00/10] dma: edma: Fixes for cyclic (audio) operation

[PATCH v3 00/10] dma: edma: Fixes for cyclic (audio) operation

[Peter Ujfalusi]

Hi,

Changes since v2:
- Dropped patch 10 from v2 (simplify direction configuration...)
- Dropped the channel priority related patches since we are going to go via
  different route for configuring the priority.
- Added ACK from Joel for the patches since they are not changed since v2

Changes since v1:
- ASoC patches removed
- Comments from Andriy Shevchenko addressed
- patch added to fix cases when src/dst_maxburst is set to 0

The series contains now only:
Support for DMA pause/resume in cyclic mode
device_slave_caps callback and DMA_CYCLIC flag correction.
While debugging the edma to get things sorted out I noticed that the debug was
too verbose and the important information was hidden even when the we did not
asked for verbose dmaengine debug.
I have included some debug cleanups for the edma dmaengine driver also.

Regards,
Peter
---
Peter Ujfalusi (10):
  platform_data: edma: Be precise with the paRAM struct
  arm: common: edma: Save the number of event queues/TCs
  dmaengine: edma: Correct the handling of src/dst_maxburst == 0
  dmaengine: edma: Add support for DMA_PAUSE/RESUME operation
  dmaengine: edma: Set DMA_CYCLIC capability flag
  dmaengine: edma: Implement device_slave_caps callback
  dmaengine: edma: Reduce debug print verbosity for non verbose
    debugging
  dmaengine: edma: Prefix debug prints where the text were identical in
    prep callbacks
  dmaengine: edma: Add channel number to debug prints
  dmaengine: edma: Print the direction value as well when it is not
    supported

 arch/arm/common/edma.c             |  4 ++
 drivers/dma/edma.c                 | 87 ++++++++++++++++++++++++++++++--------
 include/linux/platform_data/edma.h | 18 ++++----
 3 files changed, 83 insertions(+), 26 deletions(-)

-- 
1.9.2

[PATCH v3 01/10] 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>
Acked-by: Joel Fernandes <joelf@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.2

[PATCH v3 02/10] 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>
Acked-by: Joel Fernandes <joelf@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 41bca32409fc..999266bf69b9 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -1768,6 +1768,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.2

[PATCH v3 03/10] dmaengine: 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>
Acked-by: Joel Fernandes <joelf@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.2

[PATCH v3 04/10] dmaengine: 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>
Acked-by: Joel Fernandes <joelf@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.2

[PATCH v3 05/10] dmaengine: 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>
Acked-by: Joel Fernandes <joelf@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 999266bf69b9..0b37f7734d0f 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.2

[PATCH v3 06/10] dmaengine: 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>
Acked-by: Joel Fernandes <joelf@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 1dd9e8806975..2f58c04cbcb1 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -852,6 +852,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)
 {
@@ -862,6 +879,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.2

[PATCH v3 07/10] dmaengine: 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>
Acked-by: Joel Fernandes <joelf@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 2f58c04cbcb1..6d9edc47150d 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"
@@ -560,9 +560,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 */
@@ -596,8 +595,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.2

[PATCH v3 08/10] dmaengine: 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>
Acked-by: Joel Fernandes <joelf@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 6d9edc47150d..bc8175c92e0c 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -436,14 +436,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;
 	}
 
@@ -459,7 +459,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;
 			}
 		}
@@ -528,7 +529,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;
 	}
 
@@ -553,7 +554,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;
 	}
 
@@ -571,7 +572,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.2

[PATCH v3 09/10] dmaengine: 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>
Acked-by: Joel Fernandes <joelf@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 bc8175c92e0c..4aa5eb005b5c 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);
@@ -735,7 +736,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.2

[PATCH v3 10/10] dmaengine: 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>
Acked-by: Joel Fernandes <joelf@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 4aa5eb005b5c..91849aa50de1 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -432,7 +432,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;
 	}
 
@@ -525,7 +525,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.2

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

[Joel Fernandes]

On 04/14/2014 06:41 AM, Peter Ujfalusi wrote:
> Hi,
> 
> Changes since v2:
> - Dropped patch 10 from v2 (simplify direction configuration...)
> - Dropped the channel priority related patches since we are going to go via
>   different route for configuring the priority.
> - Added ACK from Joel for the patches since they are not changed since v2
> 
> Changes since v1:
> - ASoC patches removed
> - Comments from Andriy Shevchenko addressed
> - patch added to fix cases when src/dst_maxburst is set to 0
> 
> The series contains now only:
> Support for DMA pause/resume in cyclic mode
> device_slave_caps callback and DMA_CYCLIC flag correction.
> While debugging the edma to get things sorted out I noticed that the debug was
> too verbose and the important information was hidden even when the we did not
> asked for verbose dmaengine debug.
> I have included some debug cleanups for the edma dmaengine driver also.
> 
> Regards,
> Peter
> ---
> Peter Ujfalusi (10):
>   platform_data: edma: Be precise with the paRAM struct
>   arm: common: edma: Save the number of event queues/TCs
>   dmaengine: edma: Correct the handling of src/dst_maxburst == 0
>   dmaengine: edma: Add support for DMA_PAUSE/RESUME operation
>   dmaengine: edma: Set DMA_CYCLIC capability flag
>   dmaengine: edma: Implement device_slave_caps callback
>   dmaengine: edma: Reduce debug print verbosity for non verbose
>     debugging
>   dmaengine: edma: Prefix debug prints where the text were identical in
>     prep callbacks
>   dmaengine: edma: Add channel number to debug prints
>   dmaengine: edma: Print the direction value as well when it is not
>     supported
> 
>  arch/arm/common/edma.c             |  4 ++
>  drivers/dma/edma.c                 | 87 ++++++++++++++++++++++++++++++--------
>  include/linux/platform_data/edma.h | 18 ++++----
>  3 files changed, 83 insertions(+), 26 deletions(-)
> 

I reviewed and tested all the patches in the new series to make sure it
doesn't break anything with non-cyclic users (MMC and Crypto).

Reviewed-and-Tested-by: Joel Fernandes <joelf@ti.com>


regards,
-Joel

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

[Joel Fernandes]

Hi Vinod, Dan,

On 04/14/2014 06:41 AM, Peter Ujfalusi wrote:
> Hi,
> 
> Changes since v2:
> - Dropped patch 10 from v2 (simplify direction configuration...)
> - Dropped the channel priority related patches since we are going to go via
>   different route for configuring the priority.
> - Added ACK from Joel for the patches since they are not changed since v2
> 
> Changes since v1:
> - ASoC patches removed
> - Comments from Andriy Shevchenko addressed
> - patch added to fix cases when src/dst_maxburst is set to 0
> 
> The series contains now only:
> Support for DMA pause/resume in cyclic mode
> device_slave_caps callback and DMA_CYCLIC flag correction.
> While debugging the edma to get things sorted out I noticed that the debug was
> too verbose and the important information was hidden even when the we did not
> asked for verbose dmaengine debug.
> I have included some debug cleanups for the edma dmaengine driver also.

I reviewed/tested these patches and they look OK to me. Also the point
of contention was priority which is now dropped from the series. If the
patches look OK and there are no further review comments can they be
queued for -next?

I also have a memcpy and another fix patch for edma so I could queue all
together in my tree and send a consolidated pull request to make it easier.

thanks,
 -Joel

Re: [PATCH v3 09/10] dmaengine: edma: Add channel number to debug prints

[Vinod Koul]

On Mon, Apr 14, 2014 at 02:42:04PM +0300, Peter Ujfalusi wrote:
> 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>
> Acked-by: Joel Fernandes <joelf@ti.com>

This failed to apply, cna you pls rebase this and resend...

--	
~Vinod

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

[Vinod Koul]

On Mon, Apr 14, 2014 at 02:41:55PM +0300, Peter Ujfalusi wrote:
> Hi,
> 
> Changes since v2:
> - Dropped patch 10 from v2 (simplify direction configuration...)
> - Dropped the channel priority related patches since we are going to go via
>   different route for configuring the priority.
> - Added ACK from Joel for the patches since they are not changed since v2
> 
> Changes since v1:
> - ASoC patches removed
> - Comments from Andriy Shevchenko addressed
> - patch added to fix cases when src/dst_maxburst is set to 0
> 
> The series contains now only:
> Support for DMA pause/resume in cyclic mode
> device_slave_caps callback and DMA_CYCLIC flag correction.
> While debugging the edma to get things sorted out I noticed that the debug was
> too verbose and the important information was hidden even when the we did not
> asked for verbose dmaengine debug.
> I have included some debug cleanups for the edma dmaengine driver also.
> 
Applied all, expect 9th one!

-- 
~Vinod

> Regards,
> Peter
> ---
> Peter Ujfalusi (10):
>   platform_data: edma: Be precise with the paRAM struct
>   arm: common: edma: Save the number of event queues/TCs
>   dmaengine: edma: Correct the handling of src/dst_maxburst == 0
>   dmaengine: edma: Add support for DMA_PAUSE/RESUME operation
>   dmaengine: edma: Set DMA_CYCLIC capability flag
>   dmaengine: edma: Implement device_slave_caps callback
>   dmaengine: edma: Reduce debug print verbosity for non verbose
>     debugging
>   dmaengine: edma: Prefix debug prints where the text were identical in
>     prep callbacks
>   dmaengine: edma: Add channel number to debug prints
>   dmaengine: edma: Print the direction value as well when it is not
>     supported
> 
>  arch/arm/common/edma.c             |  4 ++
>  drivers/dma/edma.c                 | 87 ++++++++++++++++++++++++++++++--------
>  include/linux/platform_data/edma.h | 18 ++++----
>  3 files changed, 83 insertions(+), 26 deletions(-)
> 
> -- 
> 1.9.2
> 

-- 

Re: [PATCH v3 01/10] platform_data: edma: Be precise with the paRAM struct

[Olof Johansson]

Hi,

On Mon, Apr 14, 2014 at 4:41 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> 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>
> Acked-by: Joel Fernandes <joelf@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)

I came across this patch when I was looking at a pull request from
Sekhar for EDMA cleanups, and it made me look closer at the contents
of this file.

The include/linux/platform_data/ directory is meant to hold
platform_data definitions for drivers, and nothing more.
platform_data/edma.h also contains a whole bunch of interface
definitions for the driver. They do not belong there, and should be
moved to a different include file.

That also includes the above struct, because as far as I can tell it's
a runtime state structure, not something that is passed in with
platform data.

Can someone please clean this up? Thanks.


-Olof

Re: [PATCH v3 01/10] platform_data: edma: Be precise with the paRAM struct

[Peter Ujfalusi]

On 05/27/2014 12:32 AM, Olof Johansson wrote:
> Hi,
> 
> On Mon, Apr 14, 2014 at 4:41 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>> 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>
>> Acked-by: Joel Fernandes <joelf@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)
> 
> I came across this patch when I was looking at a pull request from
> Sekhar for EDMA cleanups, and it made me look closer at the contents
> of this file.
> 
> The include/linux/platform_data/ directory is meant to hold
> platform_data definitions for drivers, and nothing more.
> platform_data/edma.h also contains a whole bunch of interface
> definitions for the driver. They do not belong there, and should be
> moved to a different include file.
> 
> That also includes the above struct, because as far as I can tell it's
> a runtime state structure, not something that is passed in with
> platform data.
> 
> Can someone please clean this up? Thanks.

I think Joel is working on to move/merge the code from arch/arm/common/edma.c
to drivers/dma/edma.c
I'm sure within this work he is going to clean up the header file as well.
As a first step I think the non platform_data content can be moved as
include/linux/edma.h or probably as ti-edma.h?

-- 
Péter

Re: [PATCH v3 01/10] platform_data: edma: Be precise with the paRAM struct

[Joel Fernandes]

On 05/27/2014 05:22 AM, Peter Ujfalusi wrote:
> On 05/27/2014 12:32 AM, Olof Johansson wrote:
[..]
>>
>> I came across this patch when I was looking at a pull request from
>> Sekhar for EDMA cleanups, and it made me look closer at the contents
>> of this file.
>>
>> The include/linux/platform_data/ directory is meant to hold
>> platform_data definitions for drivers, and nothing more.
>> platform_data/edma.h also contains a whole bunch of interface
>> definitions for the driver. They do not belong there, and should be
>> moved to a different include file.
>>
>> That also includes the above struct, because as far as I can tell it's
>> a runtime state structure, not something that is passed in with
>> platform data.
>>
>> Can someone please clean this up? Thanks.
> 
> I think Joel is working on to move/merge the code from arch/arm/common/edma.c
> to drivers/dma/edma.c

Yes, I am planning to work on that soon. But there is an issue, more on
that discussed below..

> I'm sure within this work he is going to clean up the header file as well.

Agreed. The private API should not be expored in any header and should
be exclusive for the EDMA dmaengine driver ideally.

> As a first step I think the non platform_data content can be moved as
> include/linux/edma.h or probably as ti-edma.h?
> 

sound/soc/davinci/davinci-pcm.c: This still uses the EDMA private API in
arch/arm/common/edma.c. Peter, any idea when the private usage will be
removed fully, and we switch to dmaengine for ASoC? Before that can
happen, we can't clean up or do any merges.

What I'd like to do is fold the private API into the dmaengine driver
and eliminate the need to expose the private API, thus also getting rid
of the interface declarations Olof referred to.

thanks,

-Joel

Re: [PATCH v3 01/10] platform_data: edma: Be precise with the paRAM struct

[Peter Ujfalusi]

On 05/27/2014 06:03 PM, Joel Fernandes wrote:
> On 05/27/2014 05:22 AM, Peter Ujfalusi wrote:
>> On 05/27/2014 12:32 AM, Olof Johansson wrote:
> [..]
>>>
>>> I came across this patch when I was looking at a pull request from
>>> Sekhar for EDMA cleanups, and it made me look closer at the contents
>>> of this file.
>>>
>>> The include/linux/platform_data/ directory is meant to hold
>>> platform_data definitions for drivers, and nothing more.
>>> platform_data/edma.h also contains a whole bunch of interface
>>> definitions for the driver. They do not belong there, and should be
>>> moved to a different include file.
>>>
>>> That also includes the above struct, because as far as I can tell it's
>>> a runtime state structure, not something that is passed in with
>>> platform data.
>>>
>>> Can someone please clean this up? Thanks.
>>
>> I think Joel is working on to move/merge the code from arch/arm/common/edma.c
>> to drivers/dma/edma.c
> 
> Yes, I am planning to work on that soon. But there is an issue, more on
> that discussed below..
> 
>> I'm sure within this work he is going to clean up the header file as well.
> 
> Agreed. The private API should not be expored in any header and should
> be exclusive for the EDMA dmaengine driver ideally.
> 
>> As a first step I think the non platform_data content can be moved as
>> include/linux/edma.h or probably as ti-edma.h?
>>
> 
> sound/soc/davinci/davinci-pcm.c: This still uses the EDMA private API in
> arch/arm/common/edma.c. Peter, any idea when the private usage will be
> removed fully, and we switch to dmaengine for ASoC? Before that can
> happen, we can't clean up or do any merges.

We have the edma-pcm platform driver upstream already which I'm using locally
for a long time now on AM335x/AM437x. I'm planning to send a patch to do the
same upstream after the 3.16 window closes.
But, davinci-pcm has a mode called 'ping-pong' which is not available via
dmaengine and this mode is used by several daVinci SoCs to overcome buffer
underflow/overflow issues. This mode essentially means in playback case:
      dma_ch1       dma_ch2
SDRAM -------> SRAM -------> McASP

ch1 is to move a block of samples to SRAM from where ch2 will copy the samples
word by word to McASP.

If we move all davinci SoCs to use the edma-pcm, we are going to loose this
mode. As a note: the edma-pcm is confirmed to work fine on the tested daVinci
boards.
I think what we need to do first: find a board which is using ping-pong mode,
put under stress test in:
- davinci-pcm, ping-pong mode
- davinci-pcm, no ping-pong mode
- edma-pcm

and see how edma-pcm behaves compared to the davinci-pcm. One of the issue
with davinci-pcm is that in non ping-pong mode it reconfigures the eDMA after
every period, which is a bad thing. The dmaengine implementation does not need
to do that, so we might be fine there.

> What I'd like to do is fold the private API into the dmaengine driver
> and eliminate the need to expose the private API, thus also getting rid
> of the interface declarations Olof referred to.
> 
> thanks,
> 
> -Joel
> 


-- 
Péter