[PATCH 0/7] dmaengine: mmp_tdma: Make the driver actually work

[PATCH 0/7] dmaengine: mmp_tdma: Make the driver actually work

[Lubomir Rintel]

Hi,

chained to this message is a couple of improvements to the MMP Audio DMA
driver (mmp_tdma). Please consider applying them.

Patches 1 to 5 are various improvements robustness and error handling.

The most important patch is [PATCH 6/7], which makes the driver play
along with soc-generic-dmaengine-pcm. This is necessary to play sound on 
devicetree-based MMP2 machines.

The last patch drops MMP_SRAM dependency. This is a safe thing to do,
because nothing currently actually uses this (mmp_tdma) driver. There's a
redundant driver in sound/soc/pxa/mmp-pcm.c, which is also effectively
unused. Sigh.

Tested on an OLPC XO-1.75 laptop (along with changes to the MMP SSPA
driver).

Thank you
Lubo

[PATCH 1/7] dmaengine: mmp_tdma: Do not ignore slave config validation errors

[Lubomir Rintel]

With an invalid dma_slave_config set previously,
mmp_tdma_prep_dma_cyclic() would detect an error whilst configuring the
channel, but proceed happily on:

  [  120.756530] mmp-tdma d42a0800.adma: mmp_tdma: unknown burst size.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/dma/mmp_tdma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/mmp_tdma.c b/drivers/dma/mmp_tdma.c
index 10117f271b12b..51e08c16756ae 100644
--- a/drivers/dma/mmp_tdma.c
+++ b/drivers/dma/mmp_tdma.c
@@ -443,7 +443,8 @@ static struct dma_async_tx_descriptor *mmp_tdma_prep_dma_cyclic(
 	if (!desc)
 		goto err_out;
 
-	mmp_tdma_config_write(chan, direction, &tdmac->slave_config);
+	if (mmp_tdma_config_write(chan, direction, &tdmac->slave_config))
+		goto err_out;
 
 	while (buf < buf_len) {
 		desc = &tdmac->desc_arr[i];
-- 
2.26.0

[PATCH 2/7] dmaengine: mmp_tdma: Drop "mmp_tdma: from error messages

[Lubomir Rintel]

Drop a redundant "mmp_tdma:" from some error messages. The dev_err()
appends mostly the same thing for us:

  [  120.756530] mmp-tdma d42a0800.adma: mmp_tdma: unknown burst size.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/dma/mmp_tdma.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/mmp_tdma.c b/drivers/dma/mmp_tdma.c
index 51e08c16756ae..d559bb4d6a31d 100644
--- a/drivers/dma/mmp_tdma.c
+++ b/drivers/dma/mmp_tdma.c
@@ -235,7 +235,7 @@ static int mmp_tdma_config_chan(struct dma_chan *chan)
 			tdcr |= TDCR_BURSTSZ_128B;
 			break;
 		default:
-			dev_err(tdmac->dev, "mmp_tdma: unknown burst size.\n");
+			dev_err(tdmac->dev, "unknown burst size.\n");
 			return -EINVAL;
 		}
 
@@ -250,7 +250,7 @@ static int mmp_tdma_config_chan(struct dma_chan *chan)
 			tdcr |= TDCR_SSZ_32_BITS;
 			break;
 		default:
-			dev_err(tdmac->dev, "mmp_tdma: unknown bus size.\n");
+			dev_err(tdmac->dev, "unknown bus size.\n");
 			return -EINVAL;
 		}
 	} else if (tdmac->type == PXA910_SQU) {
@@ -276,7 +276,7 @@ static int mmp_tdma_config_chan(struct dma_chan *chan)
 			tdcr |= TDCR_BURSTSZ_SQU_32B;
 			break;
 		default:
-			dev_err(tdmac->dev, "mmp_tdma: unknown burst size.\n");
+			dev_err(tdmac->dev, "unknown burst size.\n");
 			return -EINVAL;
 		}
 	}
-- 
2.26.0

[PATCH 3/7] dmaengine: mmp_tdma: Validate the transfer direction

[Lubomir Rintel]

We only support DMA_DEV_TO_MEM and DMA_MEM_TO_DEV. Let's not do
undefined things with other values and reject them.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/dma/mmp_tdma.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/mmp_tdma.c b/drivers/dma/mmp_tdma.c
index d559bb4d6a31d..d574641791598 100644
--- a/drivers/dma/mmp_tdma.c
+++ b/drivers/dma/mmp_tdma.c
@@ -207,10 +207,17 @@ static int mmp_tdma_config_chan(struct dma_chan *chan)
 
 	mmp_tdma_disable_chan(chan);
 
-	if (tdmac->dir == DMA_MEM_TO_DEV)
-		tdcr = TDCR_DSTDIR_ADDR_HOLD | TDCR_SRCDIR_ADDR_INC;
-	else if (tdmac->dir == DMA_DEV_TO_MEM)
+	switch (tdmac->dir) {
+	case DMA_DEV_TO_MEM:
 		tdcr = TDCR_SRCDIR_ADDR_HOLD | TDCR_DSTDIR_ADDR_INC;
+		break;
+	case DMA_MEM_TO_DEV:
+		tdcr = TDCR_DSTDIR_ADDR_HOLD | TDCR_SRCDIR_ADDR_INC;
+		break;
+	default:
+		dev_err(tdmac->dev, "invalid transfer direction\n");
+		return -EINVAL;
+	}
 
 	if (tdmac->type == MMP_AUD_TDMA) {
 		tdcr |= TDCR_PACKMOD;
@@ -455,12 +462,18 @@ static struct dma_async_tx_descriptor *mmp_tdma_prep_dma_cyclic(
 			desc->nxt_desc = tdmac->desc_arr_phys +
 				sizeof(*desc) * (i + 1);
 
-		if (direction == DMA_MEM_TO_DEV) {
-			desc->src_addr = dma_addr;
-			desc->dst_addr = tdmac->dev_addr;
-		} else {
+		switch (direction) {
+		case DMA_DEV_TO_MEM:
 			desc->src_addr = tdmac->dev_addr;
 			desc->dst_addr = dma_addr;
+			break;
+		case DMA_MEM_TO_DEV:
+			desc->src_addr = dma_addr;
+			desc->dst_addr = tdmac->dev_addr;
+			break;
+		default:
+			dev_err(tdmac->dev, "invalid transfer direction\n");
+			goto err_out;
 		}
 		desc->byte_cnt = period_len;
 		dma_addr += period_len;
@@ -510,14 +523,20 @@ static int mmp_tdma_config_write(struct dma_chan *chan,
 {
 	struct mmp_tdma_chan *tdmac = to_mmp_tdma_chan(chan);
 
-	if (dir == DMA_DEV_TO_MEM) {
+	switch (dir) {
+	case DMA_DEV_TO_MEM:
 		tdmac->dev_addr = dmaengine_cfg->src_addr;
 		tdmac->burst_sz = dmaengine_cfg->src_maxburst;
 		tdmac->buswidth = dmaengine_cfg->src_addr_width;
-	} else {
+		break;
+	case DMA_MEM_TO_DEV:
 		tdmac->dev_addr = dmaengine_cfg->dst_addr;
 		tdmac->burst_sz = dmaengine_cfg->dst_maxburst;
 		tdmac->buswidth = dmaengine_cfg->dst_addr_width;
+		break;
+	default:
+		dev_err(tdmac->dev, "invalid transfer direction\n");
+		return -EINVAL;
 	}
 	tdmac->dir = dir;
 
-- 
2.26.0

[PATCH 4/7] dmaengine: mmp_tdma: Reset channel error on release

[Lubomir Rintel]

When a channel configuration fails, the status of the channel is set to
DEV_ERROR so that an attempt to submit it fails. However, this status
sticks until the heat end of the universe, making it impossible to
recover from the error.

Let's reset it when the channel is released so that further use of the
channel with correct configuration is not impacted.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/dma/mmp_tdma.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/dma/mmp_tdma.c b/drivers/dma/mmp_tdma.c
index d574641791598..0b1aa6eab1801 100644
--- a/drivers/dma/mmp_tdma.c
+++ b/drivers/dma/mmp_tdma.c
@@ -370,6 +370,8 @@ static void mmp_tdma_free_descriptor(struct mmp_tdma_chan *tdmac)
 		gen_pool_free(gpool, (unsigned long)tdmac->desc_arr,
 				size);
 	tdmac->desc_arr = NULL;
+	if (tdmac->status == DMA_ERROR)
+		tdmac->status = DMA_COMPLETE;
 
 	return;
 }
-- 
2.26.0

[PATCH 5/7] dmaengine: mmp_tdma: Log an error if channel is in wrong state

[Lubomir Rintel]

Let's log an error if the channel can't be prepared because it is in an
unexpected state.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/dma/mmp_tdma.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/mmp_tdma.c b/drivers/dma/mmp_tdma.c
index 0b1aa6eab1801..191640f26246d 100644
--- a/drivers/dma/mmp_tdma.c
+++ b/drivers/dma/mmp_tdma.c
@@ -436,8 +436,10 @@ static struct dma_async_tx_descriptor *mmp_tdma_prep_dma_cyclic(
 	int num_periods = buf_len / period_len;
 	int i = 0, buf = 0;
 
-	if (tdmac->status != DMA_COMPLETE)
+	if (tdmac->status != DMA_COMPLETE) {
+		dev_err(tdmac->dev, "controller busy");
 		return NULL;
+	}
 
 	if (period_len > TDMA_MAX_XFER_BYTES) {
 		dev_err(tdmac->dev,
-- 
2.26.0

[PATCH 6/7] dmaengine: mmp_tdma: Fill in slave capabilities

[Lubomir Rintel]

This makes dma_get_slave_caps() work with the device so that it could
actually be used with soc-generic-dmaengine-pcm.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/dma/mmp_tdma.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/dma/mmp_tdma.c b/drivers/dma/mmp_tdma.c
index 191640f26246d..38f2298879881 100644
--- a/drivers/dma/mmp_tdma.c
+++ b/drivers/dma/mmp_tdma.c
@@ -725,6 +725,17 @@ static int mmp_tdma_probe(struct platform_device *pdev)
 	tdev->device.device_terminate_all = mmp_tdma_terminate_all;
 	tdev->device.copy_align = DMAENGINE_ALIGN_8_BYTES;
 
+	tdev->device.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
+	if (type == MMP_AUD_TDMA) {
+		tdev->device.max_burst = SZ_128;
+		tdev->device.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+		tdev->device.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+	} else if (type == PXA910_SQU) {
+		tdev->device.max_burst = SZ_32;
+	}
+	tdev->device.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
+	tdev->device.descriptor_reuse = true;
+
 	dma_set_mask(&pdev->dev, DMA_BIT_MASK(64));
 	platform_set_drvdata(pdev, tdev);
 
-- 
2.26.0

[PATCH 7/7] dmaengine: mmp_tdma: Remove the MMP_SRAM dependency

[Lubomir Rintel]

A generic SRAM will driver for Device Tree enabled platforms will do as
well.

The non-DT drivers that use mmp_tdma to transfer audio samples to and from
the audio SRAM should depend on MMP_SRAM themselves.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/dma/Kconfig | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 0924836443152..bd0b6abd32fd3 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -394,12 +394,10 @@ config MMP_TDMA
 	bool "MMP Two-Channel DMA support"
 	depends on ARCH_MMP || COMPILE_TEST
 	select DMA_ENGINE
-	select MMP_SRAM if ARCH_MMP
 	select GENERIC_ALLOCATOR
 	help
 	  Support the MMP Two-Channel DMA engine.
 	  This engine used for MMP Audio DMA and pxa910 SQU.
-	  It needs sram driver under mach-mmp.
 
 config MOXART_DMA
 	tristate "MOXART DMA support"
-- 
2.26.0

Re: [PATCH 3/7] dmaengine: mmp_tdma: Validate the transfer direction

[Vinod Koul]

On 19-04-20, 18:49, Lubomir Rintel wrote:
> We only support DMA_DEV_TO_MEM and DMA_MEM_TO_DEV. Let's not do
> undefined things with other values and reject them.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---
>  drivers/dma/mmp_tdma.c | 37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/dma/mmp_tdma.c b/drivers/dma/mmp_tdma.c
> index d559bb4d6a31d..d574641791598 100644
> --- a/drivers/dma/mmp_tdma.c
> +++ b/drivers/dma/mmp_tdma.c
> @@ -207,10 +207,17 @@ static int mmp_tdma_config_chan(struct dma_chan *chan)
>  
>  	mmp_tdma_disable_chan(chan);
>  
> -	if (tdmac->dir == DMA_MEM_TO_DEV)
> -		tdcr = TDCR_DSTDIR_ADDR_HOLD | TDCR_SRCDIR_ADDR_INC;
> -	else if (tdmac->dir == DMA_DEV_TO_MEM)
> +	switch (tdmac->dir) {
> +	case DMA_DEV_TO_MEM:
>  		tdcr = TDCR_SRCDIR_ADDR_HOLD | TDCR_DSTDIR_ADDR_INC;
> +		break;
> +	case DMA_MEM_TO_DEV:
> +		tdcr = TDCR_DSTDIR_ADDR_HOLD | TDCR_SRCDIR_ADDR_INC;
> +		break;
> +	default:
> +		dev_err(tdmac->dev, "invalid transfer direction\n");
> +		return -EINVAL;
> +	}

You can use macros is_slave_direction() to validate

>  	if (tdmac->type == MMP_AUD_TDMA) {
>  		tdcr |= TDCR_PACKMOD;
> @@ -455,12 +462,18 @@ static struct dma_async_tx_descriptor *mmp_tdma_prep_dma_cyclic(
>  			desc->nxt_desc = tdmac->desc_arr_phys +
>  				sizeof(*desc) * (i + 1);
>  

It would make more sense to use is_slave_direction() and reject up early
in the function and proceed only when good :)

> -		if (direction == DMA_MEM_TO_DEV) {
> -			desc->src_addr = dma_addr;
> -			desc->dst_addr = tdmac->dev_addr;
> -		} else {
> +		switch (direction) {
> +		case DMA_DEV_TO_MEM:
>  			desc->src_addr = tdmac->dev_addr;
>  			desc->dst_addr = dma_addr;
> +			break;
> +		case DMA_MEM_TO_DEV:
> +			desc->src_addr = dma_addr;
> +			desc->dst_addr = tdmac->dev_addr;
> +			break;
> +		default:
> +			dev_err(tdmac->dev, "invalid transfer direction\n");
> +			goto err_out;
>  		}
>  		desc->byte_cnt = period_len;
>  		dma_addr += period_len;
> @@ -510,14 +523,20 @@ static int mmp_tdma_config_write(struct dma_chan *chan,
>  {
>  	struct mmp_tdma_chan *tdmac = to_mmp_tdma_chan(chan);
>  
> -	if (dir == DMA_DEV_TO_MEM) {
> +	switch (dir) {
> +	case DMA_DEV_TO_MEM:
>  		tdmac->dev_addr = dmaengine_cfg->src_addr;
>  		tdmac->burst_sz = dmaengine_cfg->src_maxburst;
>  		tdmac->buswidth = dmaengine_cfg->src_addr_width;
> -	} else {
> +		break;
> +	case DMA_MEM_TO_DEV:
>  		tdmac->dev_addr = dmaengine_cfg->dst_addr;
>  		tdmac->burst_sz = dmaengine_cfg->dst_maxburst;
>  		tdmac->buswidth = dmaengine_cfg->dst_addr_width;
> +		break;
> +	default:
> +		dev_err(tdmac->dev, "invalid transfer direction\n");
> +		return -EINVAL;

is this required, if you have checked in all _prep() fns then you are
guaranteed that this will never hit, right?

-- 
~Vinod

Re: [PATCH 1/7] dmaengine: mmp_tdma: Do not ignore slave config validation errors

[Vinod Koul]

On 19-04-20, 18:49, Lubomir Rintel wrote:
> With an invalid dma_slave_config set previously,
> mmp_tdma_prep_dma_cyclic() would detect an error whilst configuring the
> channel, but proceed happily on:
> 
>   [  120.756530] mmp-tdma d42a0800.adma: mmp_tdma: unknown burst size.

Applied to fixes, thanks

-- 
~Vinod

Re: [PATCH 2/7] dmaengine: mmp_tdma: Drop "mmp_tdma: from error messages

[Vinod Koul]

On 19-04-20, 18:49, Lubomir Rintel wrote:
> Drop a redundant "mmp_tdma:" from some error messages. The dev_err()
> appends mostly the same thing for us:
> 
>   [  120.756530] mmp-tdma d42a0800.adma: mmp_tdma: unknown burst size.

Applied, thanks

-- 
~Vinod

Re: [PATCH 4/7] dmaengine: mmp_tdma: Reset channel error on release

[Vinod Koul]

On 19-04-20, 18:49, Lubomir Rintel wrote:
> When a channel configuration fails, the status of the channel is set to
> DEV_ERROR so that an attempt to submit it fails. However, this status
> sticks until the heat end of the universe, making it impossible to
> recover from the error.
> 
> Let's reset it when the channel is released so that further use of the
> channel with correct configuration is not impacted.

Applied to fixes, thanks
-- 
~Vinod

Re: [PATCH 5/7] dmaengine: mmp_tdma: Log an error if channel is in wrong state

[Vinod Koul]

On 19-04-20, 18:49, Lubomir Rintel wrote:
> Let's log an error if the channel can't be prepared because it is in an
> unexpected state.

Applied, thanks

-- 
~Vinod

Re: [PATCH 6/7] dmaengine: mmp_tdma: Fill in slave capabilities

[Vinod Koul]

On 19-04-20, 18:49, Lubomir Rintel wrote:
> This makes dma_get_slave_caps() work with the device so that it could
> actually be used with soc-generic-dmaengine-pcm.

Applied, thanks

-- 
~Vinod

Re: [PATCH 7/7] dmaengine: mmp_tdma: Remove the MMP_SRAM dependency

[Vinod Koul]

On 19-04-20, 18:49, Lubomir Rintel wrote:
> A generic SRAM will driver for Device Tree enabled platforms will do as
> well.
> 
> The non-DT drivers that use mmp_tdma to transfer audio samples to and from
> the audio SRAM should depend on MMP_SRAM themselves.

Applied, thanks

-- 
~Vinod