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

[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 at ti.com>
Acked-by: Joel Fernandes <joelf at 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)

[Olof Johansson]

Hi,

Post by 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 at ti.com>
Acked-by: Joel Fernandes <joelf at 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

[Peter Ujfalusi]

Post by Olof Johansson
Hi,

Post by 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 at ti.com>
Acked-by: Joel Fernandes <joelf at 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?

[Joel Fernandes]

[..]

Post by Peter Ujfalusi

Post by Olof Johansson
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..

Post by Peter Ujfalusi
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.

Post by Peter Ujfalusi
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

[Peter Ujfalusi]

Post by Joel Fernandes
[..]

Post by Peter Ujfalusi

Post by Olof Johansson
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..

Post by Peter Ujfalusi
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.

Post by Peter Ujfalusi
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.

Post by Joel Fernandes
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

[Peter Ujfalusi]

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

Signed-off-by: Peter Ujfalusi <peter.ujfalusi at ti.com>
Acked-by: Joel Fernandes <joelf at 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,

[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 at ti.com>
Acked-by: Joel Fernandes <joelf at 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

[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 at ti.com>
Acked-by: Joel Fernandes <joelf at 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;
}

[Peter Ujfalusi]

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

Signed-off-by: Peter Ujfalusi <peter.ujfalusi at ti.com>
Acked-by: Joel Fernandes <joelf at 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);

[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 at ti.com>
Acked-by: Joel Fernandes <joelf at 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);

[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 at ti.com>
Acked-by: Joel Fernandes <joelf at 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"

[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 at ti.com>
Acked-by: Joel Fernandes <joelf at 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;
}
}

[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 at ti.com>
Acked-by: Joel Fernandes <joelf at 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;

[Vinod Koul]

Post by 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 at ti.com>
Acked-by: Joel Fernandes <joelf at ti.com>

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

--
~Vinod

[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 at ti.com>
Acked-by: Joel Fernandes <joelf at 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;
}

[Joel Fernandes]

Post by Peter Ujfalusi
Hi,
- 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
- ASoC patches removed
- Comments from Andriy Shevchenko addressed
- patch added to fix cases when src/dst_maxburst is set to 0
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
---
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 at ti.com>


regards,
-Joel

[Joel Fernandes]

Hi Vinod, Dan,

Post by Peter Ujfalusi
Hi,
- 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
- ASoC patches removed
- Comments from Andriy Shevchenko addressed
- patch added to fix cases when src/dst_maxburst is set to 0
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

[Vinod Koul]

Post by Peter Ujfalusi
Hi,
- 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
- ASoC patches removed
- Comments from Andriy Shevchenko addressed
- patch added to fix cases when src/dst_maxburst is set to 0
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

Post by Peter Ujfalusi
Regards,
Peter
---
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

--