Re: [PATCH v1 1/4] dmaengine: imx-sdma: add memcpy interface

Re: [PATCH v1 1/4] dmaengine: imx-sdma: add memcpy interface

[Vinod]

Hi Robin,

On 11-07-18, 00:23, Robin Gong wrote:
> Add MEMCPY support, meanwhile, add SDMA_BD_MAX_CNT instead
> of '0xffff'.

latter part should be its own patch. Never mix things

> +static struct dma_async_tx_descriptor *sdma_prep_memcpy(
> +		struct dma_chan *chan, dma_addr_t dma_dst,
> +		dma_addr_t dma_src, size_t len, unsigned long flags)
> +{
> +	struct sdma_channel *sdmac = to_sdma_chan(chan);
> +	struct sdma_engine *sdma = sdmac->sdma;
> +	int channel = sdmac->channel;
> +	size_t count;
> +	int i = 0, param;
> +	struct sdma_buffer_descriptor *bd;
> +	struct sdma_desc *desc;
> +
> +	if (!chan || !len)
> +		return NULL;
> +
> +	dev_dbg(sdma->dev, "memcpy: %pad->%pad, len=%zu, channel=%d.\n",
> +		&dma_src, &dma_dst, len, channel);
> +
> +	desc = sdma_transfer_init(sdmac, DMA_MEM_TO_MEM, len / SDMA_BD_MAX_CNT
> +					+ 1);

this looks quite odd to read consider:

        esc = sdma_transfer_init(sdmac, DMA_MEM_TO_MEM,
                                 len / SDMA_BD_MAX_CNT + 1);

> +	if (!desc)
> +		goto err_out;
> +
> +	do {
> +		count = min_t(size_t, len, SDMA_BD_MAX_CNT);
> +		bd = &desc->bd[i];
> +		bd->buffer_addr = dma_src;
> +		bd->ext_buffer_addr = dma_dst;
> +		bd->mode.count = count;
> +		desc->chn_count += count;
> +
> +		switch (sdmac->word_size) {
> +		case DMA_SLAVE_BUSWIDTH_4_BYTES:

This looks wrong, we are in memcpy, there is no SLAVE so no SLAVE
widths..

>  static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
>  		struct dma_chan *chan, struct scatterlist *sgl,
>  		unsigned int sg_len, enum dma_transfer_direction direction,
> @@ -1344,9 +1431,9 @@ static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
>  
>  		count = sg_dma_len(sg);
>  
> -		if (count > 0xffff) {
> +		if (count > SDMA_BD_MAX_CNT) {
>  			dev_err(sdma->dev, "SDMA channel %d: maximum bytes for sg entry exceeded: %d > %d\n",
> -					channel, count, 0xffff);
> +					channel, count, SDMA_BD_MAX_CNT);

these changes dont belong to this patch

> @@ -1486,6 +1573,8 @@ static int sdma_config(struct dma_chan *chan,
>  		sdmac->watermark_level |= (dmaengine_cfg->dst_maxburst << 16) &
>  			SDMA_WATERMARK_LEVEL_HWML;
>  		sdmac->word_size = dmaengine_cfg->dst_addr_width;
> +	} else if (dmaengine_cfg->direction == DMA_MEM_TO_MEM) {
> +		sdmac->word_size = dmaengine_cfg->dst_addr_width;

same here too, we are in .device_config which deals with slave. Not
memcpy!

>  	} else {
>  		sdmac->per_address = dmaengine_cfg->dst_addr;
>  		sdmac->watermark_level = dmaengine_cfg->dst_maxburst *
> @@ -1902,6 +1991,7 @@ static int sdma_probe(struct platform_device *pdev)
>  
>  	dma_cap_set(DMA_SLAVE, sdma->dma_device.cap_mask);
>  	dma_cap_set(DMA_CYCLIC, sdma->dma_device.cap_mask);
> +	dma_cap_set(DMA_MEMCPY, sdma->dma_device.cap_mask);
>  
>  	INIT_LIST_HEAD(&sdma->dma_device.channels);
>  	/* Initialize channel parameters */
> @@ -1968,9 +2058,11 @@ static int sdma_probe(struct platform_device *pdev)
>  	sdma->dma_device.dst_addr_widths = SDMA_DMA_BUSWIDTHS;
>  	sdma->dma_device.directions = SDMA_DMA_DIRECTIONS;
>  	sdma->dma_device.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
> +	sdma->dma_device.device_prep_dma_memcpy = sdma_prep_memcpy;
>  	sdma->dma_device.device_issue_pending = sdma_issue_pending;
>  	sdma->dma_device.dev->dma_parms = &sdma->dma_parms;
> -	dma_set_max_seg_size(sdma->dma_device.dev, 65535);
> +	sdma->dma_device.copy_align = DMAENGINE_ALIGN_4_BYTES;
> +	dma_set_max_seg_size(sdma->dma_device.dev, SDMA_BD_MAX_CNT);

this line should not be part of this patch

-- 
~Vinod

Re: [PATCH v1 2/4] dmaengine: imx-sdma: add check_bd_buswidth() to kill the dulicated code

[Vinod]

On 11-07-18, 00:23, Robin Gong wrote:
> Add check_bd_buswidth() to minimize the code size.

this looks mostly fine and I think this should be first patch..

> 
> Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> ---
>  drivers/dma/imx-sdma.c | 64 +++++++++++++++++++++++---------------------------
>  1 file changed, 29 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 27ccabf..ed2267d 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -1326,6 +1326,33 @@ static struct sdma_desc *sdma_transfer_init(struct sdma_channel *sdmac,
>  	return NULL;
>  }
>  
> +static int check_bd_buswidth(struct sdma_buffer_descriptor *bd,
> +			     struct sdma_channel *sdmac, int count,
> +			     dma_addr_t dma_dst, dma_addr_t dma_src)
> +{
> +	int ret = 0;
> +
> +	switch (sdmac->word_size) {
> +	case DMA_SLAVE_BUSWIDTH_4_BYTES:
> +		bd->mode.command = 0;
> +		if ((count | dma_dst | dma_src) & 3)
> +			ret = -EINVAL;
> +		break;

empty line after each break please

> +	case DMA_SLAVE_BUSWIDTH_2_BYTES:
> +		bd->mode.command = 2;
> +		if ((count | dma_dst | dma_src) & 1)
> +			ret = -EINVAL;
> +		break;
> +	case DMA_SLAVE_BUSWIDTH_1_BYTE:
> +		 bd->mode.command = 1;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
-- 
~Vinod

Re: [PATCH v1 3/4] dmaengine: imx-sdma: support dmatest

[Vinod]

On 11-07-18, 00:23, Robin Gong wrote:
> dmatest(memcpy) will never call dmaengine_slave_config before prep,

and that should have been a hint to you that you should not expect that

> so jobs in dmaengine_slave_config need to be moved into somewhere
> before device_prep_dma_memcpy. Besides, dmatest never setup chan
> ->private as other common case like uart/audio/spi will always setup
> chan->private. Here check it to judge if it's dmatest case and do
> jobs in slave_config.

and you should not do anything for dmatest. Supporting it means memcpy
implementation is not correct :)

> 
> Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> ---
>  drivers/dma/imx-sdma.c | 37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index ed2267d..48f3749 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -1222,10 +1222,36 @@ static int sdma_alloc_chan_resources(struct dma_chan *chan)
>  {
>  	struct sdma_channel *sdmac = to_sdma_chan(chan);
>  	struct imx_dma_data *data = chan->private;
> +	struct imx_dma_data default_data;
>  	int prio, ret;
>  
> -	if (!data)
> -		return -EINVAL;
> +	ret = clk_enable(sdmac->sdma->clk_ipg);
> +	if (ret)
> +		return ret;
> +	ret = clk_enable(sdmac->sdma->clk_ahb);
> +	if (ret)
> +		goto disable_clk_ipg;
> +	/*
> +	 * dmatest(memcpy) will never call dmaengine_slave_config before prep,
> +	 * so jobs in dmaengine_slave_config need to be moved into somewhere
> +	 * before device_prep_dma_memcpy. Besides, dmatest never setup chan
> +	 * ->private as other common cases like uart/audio/spi will setup
> +	 * chan->private always. Here check it to judge if it's dmatest case
> +	 * and do jobs in slave_config.
> +	 */
> +	if (!data) {
> +		dev_warn(sdmac->sdma->dev, "dmatest is running?\n");

why is that a warning!

> +		sdmac->word_size  =  sdmac->sdma->dma_device.copy_align;
> +		default_data.priority = 2;
> +		default_data.peripheral_type = IMX_DMATYPE_MEMORY;
> +		default_data.dma_request = 0;
> +		default_data.dma_request2 = 0;
> +		data = &default_data;
> +
> +		sdma_config_ownership(sdmac, false, true, false);
> +		sdma_get_pc(sdmac, IMX_DMATYPE_MEMORY);
> +		sdma_load_context(sdmac);
> +	}

this needs to be default for memcpy

-- 
~Vinod

[PATCH v1 0/4] add memcpy support for sdma

[Robin Gong]

This patchset is to add memcpy interface for imx-sdma, besides,to
support dmatest and enable config related, so that easily test dma
without any other device support such as uart/audio/spi...

Robin Gong (4):
  dmaengine: imx-sdma: add memcpy interface
  dmaengine: imx-sdma: add check_bd_buswidth() to kill the dulicated
    code
  dmaengine: imx-sdma: support dmatest
  ARM: configs: imx_v6_v7_defconfig: add DMATEST support

 arch/arm/configs/imx_v6_v7_defconfig |   3 +-
 drivers/dma/imx-sdma.c               | 172 ++++++++++++++++++++++++++++-------
 2 files changed, 139 insertions(+), 36 deletions(-)

-- 
2.7.4

[PATCH v1 1/4] dmaengine: imx-sdma: add memcpy interface

[Robin Gong]

Add MEMCPY support, meanwhile, add SDMA_BD_MAX_CNT instead
of '0xffff'.

Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
 drivers/dma/imx-sdma.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 98 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 3b622d6..27ccabf 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -341,6 +341,7 @@ struct sdma_desc {
  * @pc_from_device:	script address for those device_2_memory
  * @pc_to_device:	script address for those memory_2_device
  * @device_to_device:	script address for those device_2_device
+ * @pc_to_pc:		script address for those memory_2_memory
  * @flags:		loop mode or not
  * @per_address:	peripheral source or destination address in common case
  *                      destination address in p_2_p case
@@ -366,6 +367,7 @@ struct sdma_channel {
 	enum dma_slave_buswidth		word_size;
 	unsigned int			pc_from_device, pc_to_device;
 	unsigned int			device_to_device;
+	unsigned int                    pc_to_pc;
 	unsigned long			flags;
 	dma_addr_t			per_address, per_address2;
 	unsigned long			event_mask[2];
@@ -385,6 +387,8 @@ struct sdma_channel {
 
 #define SDMA_FIRMWARE_MAGIC 0x414d4453
 
+#define SDMA_BD_MAX_CNT	0xffff
+
 /**
  * struct sdma_firmware_header - Layout of the firmware image
  *
@@ -868,14 +872,16 @@ static void sdma_get_pc(struct sdma_channel *sdmac,
 	 * These are needed once we start to support transfers between
 	 * two peripherals or memory-to-memory transfers
 	 */
-	int per_2_per = 0;
+	int per_2_per = 0, emi_2_emi = 0;
 
 	sdmac->pc_from_device = 0;
 	sdmac->pc_to_device = 0;
 	sdmac->device_to_device = 0;
+	sdmac->pc_to_pc = 0;
 
 	switch (peripheral_type) {
 	case IMX_DMATYPE_MEMORY:
+		emi_2_emi = sdma->script_addrs->ap_2_ap_addr;
 		break;
 	case IMX_DMATYPE_DSP:
 		emi_2_per = sdma->script_addrs->bp_2_ap_addr;
@@ -948,6 +954,7 @@ static void sdma_get_pc(struct sdma_channel *sdmac,
 	sdmac->pc_from_device = per_2_emi;
 	sdmac->pc_to_device = emi_2_per;
 	sdmac->device_to_device = per_2_per;
+	sdmac->pc_to_pc = emi_2_emi;
 }
 
 static int sdma_load_context(struct sdma_channel *sdmac)
@@ -964,6 +971,8 @@ static int sdma_load_context(struct sdma_channel *sdmac)
 		load_address = sdmac->pc_from_device;
 	else if (sdmac->direction == DMA_DEV_TO_DEV)
 		load_address = sdmac->device_to_device;
+	else if (sdmac->direction == DMA_MEM_TO_MEM)
+		load_address = sdmac->pc_to_pc;
 	else
 		load_address = sdmac->pc_to_device;
 
@@ -1317,6 +1326,84 @@ static struct sdma_desc *sdma_transfer_init(struct sdma_channel *sdmac,
 	return NULL;
 }
 
+static struct dma_async_tx_descriptor *sdma_prep_memcpy(
+		struct dma_chan *chan, dma_addr_t dma_dst,
+		dma_addr_t dma_src, size_t len, unsigned long flags)
+{
+	struct sdma_channel *sdmac = to_sdma_chan(chan);
+	struct sdma_engine *sdma = sdmac->sdma;
+	int channel = sdmac->channel;
+	size_t count;
+	int i = 0, param;
+	struct sdma_buffer_descriptor *bd;
+	struct sdma_desc *desc;
+
+	if (!chan || !len)
+		return NULL;
+
+	dev_dbg(sdma->dev, "memcpy: %pad->%pad, len=%zu, channel=%d.\n",
+		&dma_src, &dma_dst, len, channel);
+
+	desc = sdma_transfer_init(sdmac, DMA_MEM_TO_MEM, len / SDMA_BD_MAX_CNT
+					+ 1);
+	if (!desc)
+		goto err_out;
+
+	do {
+		count = min_t(size_t, len, SDMA_BD_MAX_CNT);
+		bd = &desc->bd[i];
+		bd->buffer_addr = dma_src;
+		bd->ext_buffer_addr = dma_dst;
+		bd->mode.count = count;
+		desc->chn_count += count;
+
+		switch (sdmac->word_size) {
+		case DMA_SLAVE_BUSWIDTH_4_BYTES:
+			bd->mode.command = 0;
+			if ((count | dma_src | dma_dst) & 3)
+				goto err_bd_out;
+			break;
+		case DMA_SLAVE_BUSWIDTH_2_BYTES:
+			bd->mode.command = 2;
+			if ((count | dma_src | dma_dst) & 1)
+				goto err_bd_out;
+			break;
+		case DMA_SLAVE_BUSWIDTH_1_BYTE:
+			bd->mode.command = 1;
+			break;
+		default:
+			goto err_bd_out;
+		}
+
+		dma_src += count;
+		dma_dst += count;
+		len -= count;
+		i++;
+
+		param = BD_DONE | BD_EXTD | BD_CONT;
+		/* last bd */
+		if (!len) {
+			param |= BD_INTR;
+			param |= BD_LAST;
+			param &= ~BD_CONT;
+		}
+
+		dev_dbg(sdma->dev, "entry %d: count: %zd dma: 0x%x %s%s\n",
+				i, count, bd->buffer_addr,
+				param & BD_WRAP ? "wrap" : "",
+				param & BD_INTR ? " intr" : "");
+
+		bd->mode.status = param;
+	} while (len);
+
+	return vchan_tx_prep(&sdmac->vc, &desc->vd, flags);
+err_bd_out:
+	sdma_free_bd(desc);
+	kfree(desc);
+err_out:
+	return NULL;
+}
+
 static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
 		struct dma_chan *chan, struct scatterlist *sgl,
 		unsigned int sg_len, enum dma_transfer_direction direction,
@@ -1344,9 +1431,9 @@ static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
 
 		count = sg_dma_len(sg);
 
-		if (count > 0xffff) {
+		if (count > SDMA_BD_MAX_CNT) {
 			dev_err(sdma->dev, "SDMA channel %d: maximum bytes for sg entry exceeded: %d > %d\n",
-					channel, count, 0xffff);
+					channel, count, SDMA_BD_MAX_CNT);
 			goto err_bd_out;
 		}
 
@@ -1421,9 +1508,9 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
 
 	sdmac->flags |= IMX_DMA_SG_LOOP;
 
-	if (period_len > 0xffff) {
+	if (period_len > SDMA_BD_MAX_CNT) {
 		dev_err(sdma->dev, "SDMA channel %d: maximum period size exceeded: %zu > %d\n",
-				channel, period_len, 0xffff);
+				channel, period_len, SDMA_BD_MAX_CNT);
 		goto err_bd_out;
 	}
 
@@ -1486,6 +1573,8 @@ static int sdma_config(struct dma_chan *chan,
 		sdmac->watermark_level |= (dmaengine_cfg->dst_maxburst << 16) &
 			SDMA_WATERMARK_LEVEL_HWML;
 		sdmac->word_size = dmaengine_cfg->dst_addr_width;
+	} else if (dmaengine_cfg->direction == DMA_MEM_TO_MEM) {
+		sdmac->word_size = dmaengine_cfg->dst_addr_width;
 	} else {
 		sdmac->per_address = dmaengine_cfg->dst_addr;
 		sdmac->watermark_level = dmaengine_cfg->dst_maxburst *
@@ -1902,6 +1991,7 @@ static int sdma_probe(struct platform_device *pdev)
 
 	dma_cap_set(DMA_SLAVE, sdma->dma_device.cap_mask);
 	dma_cap_set(DMA_CYCLIC, sdma->dma_device.cap_mask);
+	dma_cap_set(DMA_MEMCPY, sdma->dma_device.cap_mask);
 
 	INIT_LIST_HEAD(&sdma->dma_device.channels);
 	/* Initialize channel parameters */
@@ -1968,9 +2058,11 @@ static int sdma_probe(struct platform_device *pdev)
 	sdma->dma_device.dst_addr_widths = SDMA_DMA_BUSWIDTHS;
 	sdma->dma_device.directions = SDMA_DMA_DIRECTIONS;
 	sdma->dma_device.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
+	sdma->dma_device.device_prep_dma_memcpy = sdma_prep_memcpy;
 	sdma->dma_device.device_issue_pending = sdma_issue_pending;
 	sdma->dma_device.dev->dma_parms = &sdma->dma_parms;
-	dma_set_max_seg_size(sdma->dma_device.dev, 65535);
+	sdma->dma_device.copy_align = DMAENGINE_ALIGN_4_BYTES;
+	dma_set_max_seg_size(sdma->dma_device.dev, SDMA_BD_MAX_CNT);
 
 	platform_set_drvdata(pdev, sdma);
 
-- 
2.7.4

[PATCH v1 2/4] dmaengine: imx-sdma: add check_bd_buswidth() to kill the dulicated code

[Robin Gong]

Add check_bd_buswidth() to minimize the code size.

Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
 drivers/dma/imx-sdma.c | 64 +++++++++++++++++++++++---------------------------
 1 file changed, 29 insertions(+), 35 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 27ccabf..ed2267d 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -1326,6 +1326,33 @@ static struct sdma_desc *sdma_transfer_init(struct sdma_channel *sdmac,
 	return NULL;
 }
 
+static int check_bd_buswidth(struct sdma_buffer_descriptor *bd,
+			     struct sdma_channel *sdmac, int count,
+			     dma_addr_t dma_dst, dma_addr_t dma_src)
+{
+	int ret = 0;
+
+	switch (sdmac->word_size) {
+	case DMA_SLAVE_BUSWIDTH_4_BYTES:
+		bd->mode.command = 0;
+		if ((count | dma_dst | dma_src) & 3)
+			ret = -EINVAL;
+		break;
+	case DMA_SLAVE_BUSWIDTH_2_BYTES:
+		bd->mode.command = 2;
+		if ((count | dma_dst | dma_src) & 1)
+			ret = -EINVAL;
+		break;
+	case DMA_SLAVE_BUSWIDTH_1_BYTE:
+		 bd->mode.command = 1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
 static struct dma_async_tx_descriptor *sdma_prep_memcpy(
 		struct dma_chan *chan, dma_addr_t dma_dst,
 		dma_addr_t dma_src, size_t len, unsigned long flags)
@@ -1357,23 +1384,8 @@ static struct dma_async_tx_descriptor *sdma_prep_memcpy(
 		bd->mode.count = count;
 		desc->chn_count += count;
 
-		switch (sdmac->word_size) {
-		case DMA_SLAVE_BUSWIDTH_4_BYTES:
-			bd->mode.command = 0;
-			if ((count | dma_src | dma_dst) & 3)
-				goto err_bd_out;
-			break;
-		case DMA_SLAVE_BUSWIDTH_2_BYTES:
-			bd->mode.command = 2;
-			if ((count | dma_src | dma_dst) & 1)
-				goto err_bd_out;
-			break;
-		case DMA_SLAVE_BUSWIDTH_1_BYTE:
-			bd->mode.command = 1;
-			break;
-		default:
+		if (check_bd_buswidth(bd, sdmac, count, dma_dst, dma_src))
 			goto err_bd_out;
-		}
 
 		dma_src += count;
 		dma_dst += count;
@@ -1440,27 +1452,9 @@ static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
 		bd->mode.count = count;
 		desc->chn_count += count;
 
-		if (sdmac->word_size > DMA_SLAVE_BUSWIDTH_4_BYTES)
+		if (check_bd_buswidth(bd, sdmac, count, 0, sg->dma_address))
 			goto err_bd_out;
 
-		switch (sdmac->word_size) {
-		case DMA_SLAVE_BUSWIDTH_4_BYTES:
-			bd->mode.command = 0;
-			if (count & 3 || sg->dma_address & 3)
-				goto err_bd_out;
-			break;
-		case DMA_SLAVE_BUSWIDTH_2_BYTES:
-			bd->mode.command = 2;
-			if (count & 1 || sg->dma_address & 1)
-				goto err_bd_out;
-			break;
-		case DMA_SLAVE_BUSWIDTH_1_BYTE:
-			bd->mode.command = 1;
-			break;
-		default:
-			goto err_bd_out;
-		}
-
 		param = BD_DONE | BD_EXTD | BD_CONT;
 
 		if (i + 1 == sg_len) {
-- 
2.7.4

[PATCH v1 3/4] dmaengine: imx-sdma: support dmatest

[Robin Gong]

dmatest(memcpy) will never call dmaengine_slave_config before prep,
so jobs in dmaengine_slave_config need to be moved into somewhere
before device_prep_dma_memcpy. Besides, dmatest never setup chan
->private as other common case like uart/audio/spi will always setup
chan->private. Here check it to judge if it's dmatest case and do
jobs in slave_config.

Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
 drivers/dma/imx-sdma.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index ed2267d..48f3749 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -1222,10 +1222,36 @@ static int sdma_alloc_chan_resources(struct dma_chan *chan)
 {
 	struct sdma_channel *sdmac = to_sdma_chan(chan);
 	struct imx_dma_data *data = chan->private;
+	struct imx_dma_data default_data;
 	int prio, ret;
 
-	if (!data)
-		return -EINVAL;
+	ret = clk_enable(sdmac->sdma->clk_ipg);
+	if (ret)
+		return ret;
+	ret = clk_enable(sdmac->sdma->clk_ahb);
+	if (ret)
+		goto disable_clk_ipg;
+	/*
+	 * dmatest(memcpy) will never call dmaengine_slave_config before prep,
+	 * so jobs in dmaengine_slave_config need to be moved into somewhere
+	 * before device_prep_dma_memcpy. Besides, dmatest never setup chan
+	 * ->private as other common cases like uart/audio/spi will setup
+	 * chan->private always. Here check it to judge if it's dmatest case
+	 * and do jobs in slave_config.
+	 */
+	if (!data) {
+		dev_warn(sdmac->sdma->dev, "dmatest is running?\n");
+		sdmac->word_size  =  sdmac->sdma->dma_device.copy_align;
+		default_data.priority = 2;
+		default_data.peripheral_type = IMX_DMATYPE_MEMORY;
+		default_data.dma_request = 0;
+		default_data.dma_request2 = 0;
+		data = &default_data;
+
+		sdma_config_ownership(sdmac, false, true, false);
+		sdma_get_pc(sdmac, IMX_DMATYPE_MEMORY);
+		sdma_load_context(sdmac);
+	}
 
 	switch (data->priority) {
 	case DMA_PRIO_HIGH:
@@ -1244,13 +1270,6 @@ static int sdma_alloc_chan_resources(struct dma_chan *chan)
 	sdmac->event_id0 = data->dma_request;
 	sdmac->event_id1 = data->dma_request2;
 
-	ret = clk_enable(sdmac->sdma->clk_ipg);
-	if (ret)
-		return ret;
-	ret = clk_enable(sdmac->sdma->clk_ahb);
-	if (ret)
-		goto disable_clk_ipg;
-
 	ret = sdma_set_channel_priority(sdmac, prio);
 	if (ret)
 		goto disable_clk_ahb;
-- 
2.7.4

[PATCH v1 4/4] ARM: configs: imx_v6_v7_defconfig: add DMATEST support

[Robin Gong]

Add DMATEST support and remove invalid options, such as
CONFIG_BT_HCIUART_H4 is default enabled and CONFIG_SND_SOC_IMX_WM8962
is out of date and not appear in any config file.

Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
 arch/arm/configs/imx_v6_v7_defconfig | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
index e381d05..f28d4d9 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -81,7 +81,6 @@ CONFIG_CAN=y
 CONFIG_CAN_FLEXCAN=y
 CONFIG_BT=y
 CONFIG_BT_HCIUART=y
-CONFIG_BT_HCIUART_H4=y
 CONFIG_BT_HCIUART_LL=y
 CONFIG_CFG80211=y
 CONFIG_CFG80211_WEXT=y
@@ -282,7 +281,6 @@ CONFIG_SND_SOC_FSL_ASRC=y
 CONFIG_SND_IMX_SOC=y
 CONFIG_SND_SOC_PHYCORE_AC97=y
 CONFIG_SND_SOC_EUKREA_TLV320=y
-CONFIG_SND_SOC_IMX_WM8962=y
 CONFIG_SND_SOC_IMX_ES8328=y
 CONFIG_SND_SOC_IMX_SGTL5000=y
 CONFIG_SND_SOC_IMX_SPDIF=y
@@ -371,6 +369,7 @@ CONFIG_DMADEVICES=y
 CONFIG_FSL_EDMA=y
 CONFIG_IMX_SDMA=y
 CONFIG_MXS_DMA=y
+CONFIG_DMATEST=m
 CONFIG_STAGING=y
 CONFIG_STAGING_MEDIA=y
 CONFIG_VIDEO_IMX_MEDIA=y
-- 
2.7.4

RE: [PATCH v1 1/4] dmaengine: imx-sdma: add memcpy interface

[Robin Gong]

> Hi Robin,
> 
> On 11-07-18, 00:23, Robin Gong wrote:
> > Add MEMCPY support, meanwhile, add SDMA_BD_MAX_CNT instead of
> > '0xffff'.
> 
> latter part should be its own patch. Never mix things
Okay, I will split it even for this minor change.
> 
> > +static struct dma_async_tx_descriptor *sdma_prep_memcpy(
> > +		struct dma_chan *chan, dma_addr_t dma_dst,
> > +		dma_addr_t dma_src, size_t len, unsigned long flags) {
> > +	struct sdma_channel *sdmac = to_sdma_chan(chan);
> > +	struct sdma_engine *sdma = sdmac->sdma;
> > +	int channel = sdmac->channel;
> > +	size_t count;
> > +	int i = 0, param;
> > +	struct sdma_buffer_descriptor *bd;
> > +	struct sdma_desc *desc;
> > +
> > +	if (!chan || !len)
> > +		return NULL;
> > +
> > +	dev_dbg(sdma->dev, "memcpy: %pad->%pad, len=%zu, channel=%d.\n",
> > +		&dma_src, &dma_dst, len, channel);
> > +
> > +	desc = sdma_transfer_init(sdmac, DMA_MEM_TO_MEM, len /
> SDMA_BD_MAX_CNT
> > +					+ 1);
> 
> this looks quite odd to read consider:
> 
>         esc = sdma_transfer_init(sdmac, DMA_MEM_TO_MEM,
>                                  len / SDMA_BD_MAX_CNT + 1);
> 
Okay, will fix on v2.
> > +	if (!desc)
> > +		goto err_out;
> > +
> > +	do {
> > +		count = min_t(size_t, len, SDMA_BD_MAX_CNT);
> > +		bd = &desc->bd[i];
> > +		bd->buffer_addr = dma_src;
> > +		bd->ext_buffer_addr = dma_dst;
> > +		bd->mode.count = count;
> > +		desc->chn_count += count;
> > +
> > +		switch (sdmac->word_size) {
> > +		case DMA_SLAVE_BUSWIDTH_4_BYTES:
> 
> This looks wrong, we are in memcpy, there is no SLAVE so no SLAVE widths..
> 
Okay, will remove check bus width.
> >  static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
> >  		struct dma_chan *chan, struct scatterlist *sgl,
> >  		unsigned int sg_len, enum dma_transfer_direction direction, @@
> > -1344,9 +1431,9 @@ static struct dma_async_tx_descriptor
> > *sdma_prep_slave_sg(
> >
> >  		count = sg_dma_len(sg);
> >
> > -		if (count > 0xffff) {
> > +		if (count > SDMA_BD_MAX_CNT) {
> >  			dev_err(sdma->dev, "SDMA channel %d: maximum bytes for sg
> entry exceeded: %d > %d\n",
> > -					channel, count, 0xffff);
> > +					channel, count, SDMA_BD_MAX_CNT);
> 
> these changes dont belong to this patch
Will split in v2.
> 
> > @@ -1486,6 +1573,8 @@ static int sdma_config(struct dma_chan *chan,
> >  		sdmac->watermark_level |= (dmaengine_cfg->dst_maxburst << 16)
> &
> >  			SDMA_WATERMARK_LEVEL_HWML;
> >  		sdmac->word_size = dmaengine_cfg->dst_addr_width;
> > +	} else if (dmaengine_cfg->direction == DMA_MEM_TO_MEM) {
> > +		sdmac->word_size = dmaengine_cfg->dst_addr_width;
> 
> same here too, we are in .device_config which deals with slave. Not memcpy!
Will remove it.
> 
> >  	} else {
> >  		sdmac->per_address = dmaengine_cfg->dst_addr;
> >  		sdmac->watermark_level = dmaengine_cfg->dst_maxburst * @@
> -1902,6
> > +1991,7 @@ static int sdma_probe(struct platform_device *pdev)
> >
> >  	dma_cap_set(DMA_SLAVE, sdma->dma_device.cap_mask);
> >  	dma_cap_set(DMA_CYCLIC, sdma->dma_device.cap_mask);
> > +	dma_cap_set(DMA_MEMCPY, sdma->dma_device.cap_mask);
> >
> >  	INIT_LIST_HEAD(&sdma->dma_device.channels);
> >  	/* Initialize channel parameters */
> > @@ -1968,9 +2058,11 @@ static int sdma_probe(struct platform_device
> *pdev)
> >  	sdma->dma_device.dst_addr_widths = SDMA_DMA_BUSWIDTHS;
> >  	sdma->dma_device.directions = SDMA_DMA_DIRECTIONS;
> >  	sdma->dma_device.residue_granularity =
> > DMA_RESIDUE_GRANULARITY_SEGMENT;
> > +	sdma->dma_device.device_prep_dma_memcpy = sdma_prep_memcpy;
> >  	sdma->dma_device.device_issue_pending = sdma_issue_pending;
> >  	sdma->dma_device.dev->dma_parms = &sdma->dma_parms;
> > -	dma_set_max_seg_size(sdma->dma_device.dev, 65535);
> > +	sdma->dma_device.copy_align = DMAENGINE_ALIGN_4_BYTES;
> > +	dma_set_max_seg_size(sdma->dma_device.dev, SDMA_BD_MAX_CNT);
> 
> this line should not be part of this patch
> 
> --
> ~Vinod

RE: [PATCH v1 2/4] dmaengine: imx-sdma: add check_bd_buswidth() to kill the dulicated code

[Robin Gong]

> -----Original Message-----
> From: Vinod [mailto:vkoul@kernel.org]
> Sent: 2018年7月10日 23:31
> To: Robin Gong <yibin.gong@nxp.com>
> Cc: dan.j.williams@intel.com; shawnguo@kernel.org;
> s.hauer@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> linux@armlinux.org.uk; linux-arm-kernel@lists.infradead.org;
> kernel@pengutronix.de; dmaengine@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v1 2/4] dmaengine: imx-sdma: add check_bd_buswidth() to
> kill the dulicated code
> 
> On 11-07-18, 00:23, Robin Gong wrote:
> > Add check_bd_buswidth() to minimize the code size.
> 
> this looks mostly fine and I think this should be first patch..
Since no need to check bus width in memcpy case, I'll remove this patch too.
> 
> >
> > Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> > ---
> >  drivers/dma/imx-sdma.c | 64
> > +++++++++++++++++++++++---------------------------
> >  1 file changed, 29 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index
> > 27ccabf..ed2267d 100644
> > --- a/drivers/dma/imx-sdma.c
> > +++ b/drivers/dma/imx-sdma.c
> > @@ -1326,6 +1326,33 @@ static struct sdma_desc
> *sdma_transfer_init(struct sdma_channel *sdmac,
> >  	return NULL;
> >  }
> >
> > +static int check_bd_buswidth(struct sdma_buffer_descriptor *bd,
> > +			     struct sdma_channel *sdmac, int count,
> > +			     dma_addr_t dma_dst, dma_addr_t dma_src) {
> > +	int ret = 0;
> > +
> > +	switch (sdmac->word_size) {
> > +	case DMA_SLAVE_BUSWIDTH_4_BYTES:
> > +		bd->mode.command = 0;
> > +		if ((count | dma_dst | dma_src) & 3)
> > +			ret = -EINVAL;
> > +		break;
> 
> empty line after each break please
> 
> > +	case DMA_SLAVE_BUSWIDTH_2_BYTES:
> > +		bd->mode.command = 2;
> > +		if ((count | dma_dst | dma_src) & 1)
> > +			ret = -EINVAL;
> > +		break;
> > +	case DMA_SLAVE_BUSWIDTH_1_BYTE:
> > +		 bd->mode.command = 1;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> --
> ~Vinod

Re: [PATCH v1 1/4] dmaengine: imx-sdma: add memcpy interface

[Sascha Hauer]

On Wed, Jul 11, 2018 at 12:23:10AM +0800, Robin Gong wrote:
> Add MEMCPY support, meanwhile, add SDMA_BD_MAX_CNT instead
> of '0xffff'.
> 
> Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> ---
> +static struct dma_async_tx_descriptor *sdma_prep_memcpy(
> +		struct dma_chan *chan, dma_addr_t dma_dst,
> +		dma_addr_t dma_src, size_t len, unsigned long flags)
> +{
> +	struct sdma_channel *sdmac = to_sdma_chan(chan);
> +	struct sdma_engine *sdma = sdmac->sdma;
> +	int channel = sdmac->channel;
> +	size_t count;
> +	int i = 0, param;
> +	struct sdma_buffer_descriptor *bd;
> +	struct sdma_desc *desc;
> +
> +	if (!chan || !len)
> +		return NULL;
> +
> +	dev_dbg(sdma->dev, "memcpy: %pad->%pad, len=%zu, channel=%d.\n",
> +		&dma_src, &dma_dst, len, channel);
> +
> +	desc = sdma_transfer_init(sdmac, DMA_MEM_TO_MEM, len / SDMA_BD_MAX_CNT
> +					+ 1);
> +	if (!desc)
> +		goto err_out;
> +
> +	do {
> +		count = min_t(size_t, len, SDMA_BD_MAX_CNT);

When len is bigger than 0xffff you initialize count to 0xffff...

> +		bd = &desc->bd[i];
> +		bd->buffer_addr = dma_src;
> +		bd->ext_buffer_addr = dma_dst;
> +		bd->mode.count = count;
> +		desc->chn_count += count;
> +
> +		switch (sdmac->word_size) {
> +		case DMA_SLAVE_BUSWIDTH_4_BYTES:
> +			bd->mode.command = 0;
> +			if ((count | dma_src | dma_dst) & 3)
> +				goto err_bd_out;
> +			break;

...In which case you bail out here with an error.

Please make sure bigger transfers are working.

> +		case DMA_SLAVE_BUSWIDTH_2_BYTES:
> +			bd->mode.command = 2;
> +			if ((count | dma_src | dma_dst) & 1)
> +				goto err_bd_out;
> +			break;
> +		case DMA_SLAVE_BUSWIDTH_1_BYTE:
> +			bd->mode.command = 1;
> +			break;
> +		default:
> +			goto err_bd_out;
> +		}
> +
> +		dma_src += count;
> +		dma_dst += count;
> +		len -= count;
> +		i++;
> +
> +		param = BD_DONE | BD_EXTD | BD_CONT;

Probably better readable if you drop BD_CONT here and do a

	if (len) {
		param |= BD_CONT;
	} else  {
		...
	}

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v1 2/4] dmaengine: imx-sdma: add check_bd_buswidth() to kill the dulicated code

[Sascha Hauer]

On Wed, Jul 11, 2018 at 12:23:11AM +0800, Robin Gong wrote:
> Add check_bd_buswidth() to minimize the code size.
> 
> Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> ---
>  drivers/dma/imx-sdma.c | 64 +++++++++++++++++++++++---------------------------
>  1 file changed, 29 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 27ccabf..ed2267d 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -1326,6 +1326,33 @@ static struct sdma_desc *sdma_transfer_init(struct sdma_channel *sdmac,
>  	return NULL;
>  }
>  
> +static int check_bd_buswidth(struct sdma_buffer_descriptor *bd,
> +			     struct sdma_channel *sdmac, int count,
> +			     dma_addr_t dma_dst, dma_addr_t dma_src)
> +{

Better name this function set_bd_buswidth. I would assume that a
function named check_foo actually checks something, but doesn't set
anything.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [PATCH v1 3/4] dmaengine: imx-sdma: support dmatest

[Robin Gong]

> -----Original Message-----
> From: Vinod [mailto:vkoul@kernel.org]
> Sent: 2018年7月10日 23:33
> To: Robin Gong <yibin.gong@nxp.com>
> Cc: dan.j.williams@intel.com; shawnguo@kernel.org;
> s.hauer@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> linux@armlinux.org.uk; linux-arm-kernel@lists.infradead.org;
> kernel@pengutronix.de; dmaengine@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v1 3/4] dmaengine: imx-sdma: support dmatest
> 
> On 11-07-18, 00:23, Robin Gong wrote:
> > dmatest(memcpy) will never call dmaengine_slave_config before prep,
> 
> and that should have been a hint to you that you should not expect that
> 
> > so jobs in dmaengine_slave_config need to be moved into somewhere
> > before device_prep_dma_memcpy. Besides, dmatest never setup chan
> > ->private as other common case like uart/audio/spi will always setup
> > chan->private. Here check it to judge if it's dmatest case and do
> > jobs in slave_config.
> 
> and you should not do anything for dmatest. Supporting it means memcpy
> implementation is not correct :)
Okay, I will any word about dmatest here since memcpy assume no calling
slave_config.
> 
> >
> > Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> > ---
> >  drivers/dma/imx-sdma.c | 37 ++++++++++++++++++++++++++++---------
> >  1 file changed, 28 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index
> > ed2267d..48f3749 100644
> > --- a/drivers/dma/imx-sdma.c
> > +++ b/drivers/dma/imx-sdma.c
> > @@ -1222,10 +1222,36 @@ static int sdma_alloc_chan_resources(struct
> > dma_chan *chan)  {
> >  	struct sdma_channel *sdmac = to_sdma_chan(chan);
> >  	struct imx_dma_data *data = chan->private;
> > +	struct imx_dma_data default_data;
> >  	int prio, ret;
> >
> > -	if (!data)
> > -		return -EINVAL;
> > +	ret = clk_enable(sdmac->sdma->clk_ipg);
> > +	if (ret)
> > +		return ret;
> > +	ret = clk_enable(sdmac->sdma->clk_ahb);
> > +	if (ret)
> > +		goto disable_clk_ipg;
> > +	/*
> > +	 * dmatest(memcpy) will never call dmaengine_slave_config before prep,
> > +	 * so jobs in dmaengine_slave_config need to be moved into somewhere
> > +	 * before device_prep_dma_memcpy. Besides, dmatest never setup chan
> > +	 * ->private as other common cases like uart/audio/spi will setup
> > +	 * chan->private always. Here check it to judge if it's dmatest case
> > +	 * and do jobs in slave_config.
> > +	 */
> > +	if (!data) {
> > +		dev_warn(sdmac->sdma->dev, "dmatest is running?\n");
> 
> why is that a warning!
Current SDMA driver assume filter function to set chan->private with specific data 
(struct imx_dma_data dma_data)like below (sound/soc/fsl/fsl_asrc_dma.c):
static bool filter(struct dma_chan *chan, void *param)
{
        if (!imx_dma_is_general_purpose(chan))
                return false;
        chan->private = param;
        return true;
}

But in memcpy case, at lease dmatest case, no chan->private set in its filter function.
So here take dmatest a special case and do some prepare jobs for memcpy. But if the
Upper device driver call dma_request_channel() with their specific filter without
'chan->private' setting in the future. The warning message is a useful hint to them to
Add 'chan->private' in filter function.  Or doc it somewhere?
> 
> > +		sdmac->word_size  =  sdmac->sdma->dma_device.copy_align;
> > +		default_data.priority = 2;
> > +		default_data.peripheral_type = IMX_DMATYPE_MEMORY;
> > +		default_data.dma_request = 0;
> > +		default_data.dma_request2 = 0;
> > +		data = &default_data;
> > +
> > +		sdma_config_ownership(sdmac, false, true, false);
> > +		sdma_get_pc(sdmac, IMX_DMATYPE_MEMORY);
> > +		sdma_load_context(sdmac);
> > +	}
> 
> this needs to be default for memcpy
> 
> --
> ~Vinod

Re: [PATCH v1 3/4] dmaengine: imx-sdma: support dmatest

[s.hauer]

On Wed, Jul 11, 2018 at 06:37:02AM +0000, Robin Gong wrote:
> 
> > -----Original Message-----
> > From: Vinod [mailto:vkoul@kernel.org]
> > Sent: 2018年7月10日 23:33
> > To: Robin Gong <yibin.gong@nxp.com>
> > Cc: dan.j.williams@intel.com; shawnguo@kernel.org;
> > s.hauer@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> > linux@armlinux.org.uk; linux-arm-kernel@lists.infradead.org;
> > kernel@pengutronix.de; dmaengine@vger.kernel.org;
> > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [PATCH v1 3/4] dmaengine: imx-sdma: support dmatest
> > 
> > On 11-07-18, 00:23, Robin Gong wrote:
> > > dmatest(memcpy) will never call dmaengine_slave_config before prep,
> > 
> > and that should have been a hint to you that you should not expect that
> > 
> > > so jobs in dmaengine_slave_config need to be moved into somewhere
> > > before device_prep_dma_memcpy. Besides, dmatest never setup chan
> > > ->private as other common case like uart/audio/spi will always setup
> > > chan->private. Here check it to judge if it's dmatest case and do
> > > jobs in slave_config.
> > 
> > and you should not do anything for dmatest. Supporting it means memcpy
> > implementation is not correct :)
> Okay, I will any word about dmatest here since memcpy assume no calling
> slave_config.
> > 
> > >
> > > Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> > > ---
> > >  drivers/dma/imx-sdma.c | 37 ++++++++++++++++++++++++++++---------
> > >  1 file changed, 28 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index
> > > ed2267d..48f3749 100644
> > > --- a/drivers/dma/imx-sdma.c
> > > +++ b/drivers/dma/imx-sdma.c
> > > @@ -1222,10 +1222,36 @@ static int sdma_alloc_chan_resources(struct
> > > dma_chan *chan)  {
> > >  	struct sdma_channel *sdmac = to_sdma_chan(chan);
> > >  	struct imx_dma_data *data = chan->private;
> > > +	struct imx_dma_data default_data;
> > >  	int prio, ret;
> > >
> > > -	if (!data)
> > > -		return -EINVAL;
> > > +	ret = clk_enable(sdmac->sdma->clk_ipg);
> > > +	if (ret)
> > > +		return ret;
> > > +	ret = clk_enable(sdmac->sdma->clk_ahb);
> > > +	if (ret)
> > > +		goto disable_clk_ipg;
> > > +	/*
> > > +	 * dmatest(memcpy) will never call dmaengine_slave_config before prep,
> > > +	 * so jobs in dmaengine_slave_config need to be moved into somewhere
> > > +	 * before device_prep_dma_memcpy. Besides, dmatest never setup chan
> > > +	 * ->private as other common cases like uart/audio/spi will setup
> > > +	 * chan->private always. Here check it to judge if it's dmatest case
> > > +	 * and do jobs in slave_config.
> > > +	 */
> > > +	if (!data) {
> > > +		dev_warn(sdmac->sdma->dev, "dmatest is running?\n");
> > 
> > why is that a warning!
> Current SDMA driver assume filter function to set chan->private with specific data 
> (struct imx_dma_data dma_data)like below (sound/soc/fsl/fsl_asrc_dma.c):
> static bool filter(struct dma_chan *chan, void *param)
> {
>         if (!imx_dma_is_general_purpose(chan))
>                 return false;
>         chan->private = param;
>         return true;
> }
> 
> But in memcpy case, at lease dmatest case, no chan->private set in its filter function.
> So here take dmatest a special case and do some prepare jobs for memcpy. But if the
> Upper device driver call dma_request_channel() with their specific filter without
> 'chan->private' setting in the future. The warning message is a useful hint to them to
> Add 'chan->private' in filter function.  Or doc it somewhere?

Instead of doing heuristics to guess whether we are doing memcpy you
could instead make memcpy the default when slave_config is not called,
i.e. drop the if (!data) check completely.

> > 
> > > +		sdmac->word_size  =  sdmac->sdma->dma_device.copy_align;
> > > +		default_data.priority = 2;
> > > +		default_data.peripheral_type = IMX_DMATYPE_MEMORY;
> > > +		default_data.dma_request = 0;
> > > +		default_data.dma_request2 = 0;
> > > +		data = &default_data;
> > > +
> > > +		sdma_config_ownership(sdmac, false, true, false);
> > > +		sdma_get_pc(sdmac, IMX_DMATYPE_MEMORY);
> > > +		sdma_load_context(sdmac);
> > > +	}
> > 
> > this needs to be default for memcpy

The problem seems to be that we do not know whether we are doing memcpy
or not. Normally we get the information how a channel is to be
configured in dma_device->device_config, but this function is not called
in the memcpy case.

An alternative might also be to do the setup in dma_device->device_prep_dma_memcpy.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [PATCH v1 1/4] dmaengine: imx-sdma: add memcpy interface

[Robin Gong]

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> Sent: 2018年7月11日 14:25
> To: Robin Gong <yibin.gong@nxp.com>
> Cc: vkoul@kernel.org; dan.j.williams@intel.com; shawnguo@kernel.org; Fabio
> Estevam <fabio.estevam@nxp.com>; linux@armlinux.org.uk;
> linux-arm-kernel@lists.infradead.org; kernel@pengutronix.de;
> dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v1 1/4] dmaengine: imx-sdma: add memcpy interface
> 
> On Wed, Jul 11, 2018 at 12:23:10AM +0800, Robin Gong wrote:
> > Add MEMCPY support, meanwhile, add SDMA_BD_MAX_CNT instead of
> > '0xffff'.
> >
> > Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> > ---
> > +static struct dma_async_tx_descriptor *sdma_prep_memcpy(
> > +		struct dma_chan *chan, dma_addr_t dma_dst,
> > +		dma_addr_t dma_src, size_t len, unsigned long flags) {
> > +	struct sdma_channel *sdmac = to_sdma_chan(chan);
> > +	struct sdma_engine *sdma = sdmac->sdma;
> > +	int channel = sdmac->channel;
> > +	size_t count;
> > +	int i = 0, param;
> > +	struct sdma_buffer_descriptor *bd;
> > +	struct sdma_desc *desc;
> > +
> > +	if (!chan || !len)
> > +		return NULL;
> > +
> > +	dev_dbg(sdma->dev, "memcpy: %pad->%pad, len=%zu, channel=%d.\n",
> > +		&dma_src, &dma_dst, len, channel);
> > +
> > +	desc = sdma_transfer_init(sdmac, DMA_MEM_TO_MEM, len /
> SDMA_BD_MAX_CNT
> > +					+ 1);
> > +	if (!desc)
> > +		goto err_out;
> > +
> > +	do {
> > +		count = min_t(size_t, len, SDMA_BD_MAX_CNT);
> 
> When len is bigger than 0xffff you initialize count to 0xffff...
In this case, the data will be split into several bds, for example,
If the total count is 0x10000, two bd used then. One is for 0xffff,
Another is for the last 1
> 
> > +		bd = &desc->bd[i];
> > +		bd->buffer_addr = dma_src;
> > +		bd->ext_buffer_addr = dma_dst;
> > +		bd->mode.count = count;
> > +		desc->chn_count += count;
> > +
> > +		switch (sdmac->word_size) {
> > +		case DMA_SLAVE_BUSWIDTH_4_BYTES:
> > +			bd->mode.command = 0;
> > +			if ((count | dma_src | dma_dst) & 3)
> > +				goto err_bd_out;
> > +			break;
> 
> ...In which case you bail out here with an error.
In case dma_src/dma_dst is not align with bus width.
But I'll remove such bus width in v2 as Vinod comments.
> 
> Please make sure bigger transfers are working.
> 
> > +		case DMA_SLAVE_BUSWIDTH_2_BYTES:
> > +			bd->mode.command = 2;
> > +			if ((count | dma_src | dma_dst) & 1)
> > +				goto err_bd_out;
> > +			break;
> > +		case DMA_SLAVE_BUSWIDTH_1_BYTE:
> > +			bd->mode.command = 1;
> > +			break;
> > +		default:
> > +			goto err_bd_out;
> > +		}
> > +
> > +		dma_src += count;
> > +		dma_dst += count;
> > +		len -= count;
> > +		i++;
> > +
> > +		param = BD_DONE | BD_EXTD | BD_CONT;
> 
> Probably better readable if you drop BD_CONT here and do a
> 
> 	if (len) {
> 		param |= BD_CONT;
> 	} else  {
> 		...
> 	}
Okay. Will improve it.
> 
> Sascha
> 
> --
> Pengutronix e.K.                           |
> |
> Industrial Linux Solutions                 |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> pengutronix.de%2F&amp;data=02%7C01%7Cyibin.gong%40nxp.com%7C0aa96
> 135702b4414dbe708d5e6f709fe%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
> 0%7C0%7C636668871060103585&amp;sdata=k2h0RIWlujaCs8ioduJsnJAfGW0
> ZS7uzlHMMtjpQ%2Fw4%3D&amp;reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> +49-5121-206917-5555 |

Re: [PATCH v1 1/4] dmaengine: imx-sdma: add memcpy interface

[Sascha Hauer]

On Wed, Jul 11, 2018 at 06:56:18AM +0000, Robin Gong wrote:
> 
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> > Sent: 2018年7月11日 14:25
> > To: Robin Gong <yibin.gong@nxp.com>
> > Cc: vkoul@kernel.org; dan.j.williams@intel.com; shawnguo@kernel.org; Fabio
> > Estevam <fabio.estevam@nxp.com>; linux@armlinux.org.uk;
> > linux-arm-kernel@lists.infradead.org; kernel@pengutronix.de;
> > dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
> > <linux-imx@nxp.com>
> > Subject: Re: [PATCH v1 1/4] dmaengine: imx-sdma: add memcpy interface
> > 
> > On Wed, Jul 11, 2018 at 12:23:10AM +0800, Robin Gong wrote:
> > > Add MEMCPY support, meanwhile, add SDMA_BD_MAX_CNT instead of
> > > '0xffff'.
> > >
> > > Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> > > ---
> > > +static struct dma_async_tx_descriptor *sdma_prep_memcpy(
> > > +		struct dma_chan *chan, dma_addr_t dma_dst,
> > > +		dma_addr_t dma_src, size_t len, unsigned long flags) {
> > > +	struct sdma_channel *sdmac = to_sdma_chan(chan);
> > > +	struct sdma_engine *sdma = sdmac->sdma;
> > > +	int channel = sdmac->channel;
> > > +	size_t count;
> > > +	int i = 0, param;
> > > +	struct sdma_buffer_descriptor *bd;
> > > +	struct sdma_desc *desc;
> > > +
> > > +	if (!chan || !len)
> > > +		return NULL;
> > > +
> > > +	dev_dbg(sdma->dev, "memcpy: %pad->%pad, len=%zu, channel=%d.\n",
> > > +		&dma_src, &dma_dst, len, channel);
> > > +
> > > +	desc = sdma_transfer_init(sdmac, DMA_MEM_TO_MEM, len /
> > SDMA_BD_MAX_CNT
> > > +					+ 1);
> > > +	if (!desc)
> > > +		goto err_out;
> > > +
> > > +	do {
> > > +		count = min_t(size_t, len, SDMA_BD_MAX_CNT);
> > 
> > When len is bigger than 0xffff you initialize count to 0xffff...
> In this case, the data will be split into several bds, for example,
> If the total count is 0x10000, two bd used then. One is for 0xffff,
> Another is for the last 1

And you are doing byte size DMA? Wouldn't word size accesses be more
optimal?

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [PATCH v1 1/4] dmaengine: imx-sdma: add memcpy interface

[Robin Gong]

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> Sent: 2018年7月11日 15:01
> To: Robin Gong <yibin.gong@nxp.com>
> Cc: vkoul@kernel.org; dan.j.williams@intel.com; shawnguo@kernel.org; Fabio
> Estevam <fabio.estevam@nxp.com>; linux@armlinux.org.uk;
> linux-arm-kernel@lists.infradead.org; kernel@pengutronix.de;
> dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v1 1/4] dmaengine: imx-sdma: add memcpy interface
> 
> On Wed, Jul 11, 2018 at 06:56:18AM +0000, Robin Gong wrote:
> >
> > > -----Original Message-----
> > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> > > Sent: 2018年7月11日 14:25
> > > To: Robin Gong <yibin.gong@nxp.com>
> > > Cc: vkoul@kernel.org; dan.j.williams@intel.com; shawnguo@kernel.org;
> > > Fabio Estevam <fabio.estevam@nxp.com>; linux@armlinux.org.uk;
> > > linux-arm-kernel@lists.infradead.org; kernel@pengutronix.de;
> > > dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > dl-linux-imx <linux-imx@nxp.com>
> > > Subject: Re: [PATCH v1 1/4] dmaengine: imx-sdma: add memcpy
> > > interface
> > >
> > > On Wed, Jul 11, 2018 at 12:23:10AM +0800, Robin Gong wrote:
> > > > Add MEMCPY support, meanwhile, add SDMA_BD_MAX_CNT instead of
> > > > '0xffff'.
> > > >
> > > > Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> > > > ---
> > > > +static struct dma_async_tx_descriptor *sdma_prep_memcpy(
> > > > +		struct dma_chan *chan, dma_addr_t dma_dst,
> > > > +		dma_addr_t dma_src, size_t len, unsigned long flags) {
> > > > +	struct sdma_channel *sdmac = to_sdma_chan(chan);
> > > > +	struct sdma_engine *sdma = sdmac->sdma;
> > > > +	int channel = sdmac->channel;
> > > > +	size_t count;
> > > > +	int i = 0, param;
> > > > +	struct sdma_buffer_descriptor *bd;
> > > > +	struct sdma_desc *desc;
> > > > +
> > > > +	if (!chan || !len)
> > > > +		return NULL;
> > > > +
> > > > +	dev_dbg(sdma->dev, "memcpy: %pad->%pad, len=%zu,
> channel=%d.\n",
> > > > +		&dma_src, &dma_dst, len, channel);
> > > > +
> > > > +	desc = sdma_transfer_init(sdmac, DMA_MEM_TO_MEM, len /
> > > SDMA_BD_MAX_CNT
> > > > +					+ 1);
> > > > +	if (!desc)
> > > > +		goto err_out;
> > > > +
> > > > +	do {
> > > > +		count = min_t(size_t, len, SDMA_BD_MAX_CNT);
> > >
> > > When len is bigger than 0xffff you initialize count to 0xffff...
> > In this case, the data will be split into several bds, for example, If
> > the total count is 0x10000, two bd used then. One is for 0xffff,
> > Another is for the last 1
> 
> And you are doing byte size DMA? Wouldn't word size accesses be more
> optimal?
> 
> Sascha
Default is words, and I'll force the buswidth to word and set into BD.
sdma->dma_device.copy_align = DMAENGINE_ALIGN_4_BYTES;
> 
> 
> --
> Pengutronix e.K.                           |
> |
> Industrial Linux Solutions                 |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> pengutronix.de%2F&amp;data=02%7C01%7Cyibin.gong%40nxp.com%7C41fbd
> 3765b10424a7f1e08d5e6fc196d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
> 0%7C0%7C636668892793035822&amp;sdata=mLKjTKaojuO1Zv%2F4ohwkkzeK
> FDFbmqYturqh6eSblIM%3D&amp;reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> +49-5121-206917-5555 |

Re: [PATCH v1 1/4] dmaengine: imx-sdma: add memcpy interface

[Sascha Hauer]

On Wed, Jul 11, 2018 at 07:05:23AM +0000, Robin Gong wrote:
> 
> 
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> > Sent: 2018年7月11日 15:01
> > To: Robin Gong <yibin.gong@nxp.com>
> > Cc: vkoul@kernel.org; dan.j.williams@intel.com; shawnguo@kernel.org; Fabio
> > Estevam <fabio.estevam@nxp.com>; linux@armlinux.org.uk;
> > linux-arm-kernel@lists.infradead.org; kernel@pengutronix.de;
> > dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
> > <linux-imx@nxp.com>
> > Subject: Re: [PATCH v1 1/4] dmaengine: imx-sdma: add memcpy interface
> > 
> > On Wed, Jul 11, 2018 at 06:56:18AM +0000, Robin Gong wrote:
> > >
> > > > -----Original Message-----
> > > > From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> > > > Sent: 2018年7月11日 14:25
> > > > To: Robin Gong <yibin.gong@nxp.com>
> > > > Cc: vkoul@kernel.org; dan.j.williams@intel.com; shawnguo@kernel.org;
> > > > Fabio Estevam <fabio.estevam@nxp.com>; linux@armlinux.org.uk;
> > > > linux-arm-kernel@lists.infradead.org; kernel@pengutronix.de;
> > > > dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > > dl-linux-imx <linux-imx@nxp.com>
> > > > Subject: Re: [PATCH v1 1/4] dmaengine: imx-sdma: add memcpy
> > > > interface
> > > >
> > > > On Wed, Jul 11, 2018 at 12:23:10AM +0800, Robin Gong wrote:
> > > > > Add MEMCPY support, meanwhile, add SDMA_BD_MAX_CNT instead of
> > > > > '0xffff'.
> > > > >
> > > > > Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> > > > > ---
> > > > > +static struct dma_async_tx_descriptor *sdma_prep_memcpy(
> > > > > +		struct dma_chan *chan, dma_addr_t dma_dst,
> > > > > +		dma_addr_t dma_src, size_t len, unsigned long flags) {
> > > > > +	struct sdma_channel *sdmac = to_sdma_chan(chan);
> > > > > +	struct sdma_engine *sdma = sdmac->sdma;
> > > > > +	int channel = sdmac->channel;
> > > > > +	size_t count;
> > > > > +	int i = 0, param;
> > > > > +	struct sdma_buffer_descriptor *bd;
> > > > > +	struct sdma_desc *desc;
> > > > > +
> > > > > +	if (!chan || !len)
> > > > > +		return NULL;
> > > > > +
> > > > > +	dev_dbg(sdma->dev, "memcpy: %pad->%pad, len=%zu,
> > channel=%d.\n",
> > > > > +		&dma_src, &dma_dst, len, channel);
> > > > > +
> > > > > +	desc = sdma_transfer_init(sdmac, DMA_MEM_TO_MEM, len /
> > > > SDMA_BD_MAX_CNT
> > > > > +					+ 1);
> > > > > +	if (!desc)
> > > > > +		goto err_out;
> > > > > +
> > > > > +	do {
> > > > > +		count = min_t(size_t, len, SDMA_BD_MAX_CNT);
> > > >
> > > > When len is bigger than 0xffff you initialize count to 0xffff...
> > > In this case, the data will be split into several bds, for example, If
> > > the total count is 0x10000, two bd used then. One is for 0xffff,
> > > Another is for the last 1
> > 
> > And you are doing byte size DMA? Wouldn't word size accesses be more
> > optimal?
> > 
> > Sascha
> Default is words, and I'll force the buswidth to word and set into BD.
> sdma->dma_device.copy_align = DMAENGINE_ALIGN_4_BYTES;

So guess which alignment the second transfer has when the first has the
size 0xffff.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v1 1/4] dmaengine: imx-sdma: add memcpy interface

[Vinod]

On 11-07-18, 05:34, Robin Gong wrote:
> > On 11-07-18, 00:23, Robin Gong wrote:
> > > Add MEMCPY support, meanwhile, add SDMA_BD_MAX_CNT instead of
> > > '0xffff'.
> > 
> > latter part should be its own patch. Never mix things
> Okay, I will split it even for this minor change.

Yes, a patch should represent _one_ thing, describe that one thing and
do that one thing.

Anything else, doesn't matter how simple or complex should be an
individual patch

> > > +	if (!desc)
> > > +		goto err_out;
> > > +
> > > +	do {
> > > +		count = min_t(size_t, len, SDMA_BD_MAX_CNT);
> > > +		bd = &desc->bd[i];
> > > +		bd->buffer_addr = dma_src;
> > > +		bd->ext_buffer_addr = dma_dst;
> > > +		bd->mode.count = count;
> > > +		desc->chn_count += count;
> > > +
> > > +		switch (sdmac->word_size) {
> > > +		case DMA_SLAVE_BUSWIDTH_4_BYTES:
> > 
> > This looks wrong, we are in memcpy, there is no SLAVE so no SLAVE widths..
> > 
> Okay, will remove check bus width.

it is not about bus_width but the fact that you are using slave
concepts. In memcpy we have _no_ slave, hence do not use anything
related to slave including dma_slave_config

-- 
~Vinod

RE: [PATCH v1 3/4] dmaengine: imx-sdma: support dmatest

[Robin Gong]

> -----Original Message-----
> From: s.hauer@pengutronix.de [mailto:s.hauer@pengutronix.de]
> Sent: 2018年7月11日 14:54
> To: Robin Gong <yibin.gong@nxp.com>
> Cc: Vinod <vkoul@kernel.org>; dan.j.williams@intel.com;
> shawnguo@kernel.org; Fabio Estevam <fabio.estevam@nxp.com>;
> linux@armlinux.org.uk; linux-arm-kernel@lists.infradead.org;
> kernel@pengutronix.de; dmaengine@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v1 3/4] dmaengine: imx-sdma: support dmatest
> 
> On Wed, Jul 11, 2018 at 06:37:02AM +0000, Robin Gong wrote:
> >
> > > -----Original Message-----
> > > From: Vinod [mailto:vkoul@kernel.org]
> > > Sent: 2018年7月10日 23:33
> > > To: Robin Gong <yibin.gong@nxp.com>
> > > Cc: dan.j.williams@intel.com; shawnguo@kernel.org;
> > > s.hauer@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> > > linux@armlinux.org.uk; linux-arm-kernel@lists.infradead.org;
> > > kernel@pengutronix.de; dmaengine@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > > Subject: Re: [PATCH v1 3/4] dmaengine: imx-sdma: support dmatest
> > >
> > > On 11-07-18, 00:23, Robin Gong wrote:
> > > > dmatest(memcpy) will never call dmaengine_slave_config before
> > > > prep,
> > >
> > > and that should have been a hint to you that you should not expect
> > > that
> > >
> > > > so jobs in dmaengine_slave_config need to be moved into somewhere
> > > > before device_prep_dma_memcpy. Besides, dmatest never setup chan
> > > > ->private as other common case like uart/audio/spi will always
> > > > ->setup
> > > > chan->private. Here check it to judge if it's dmatest case and do
> > > > jobs in slave_config.
> > >
> > > and you should not do anything for dmatest. Supporting it means
> > > memcpy implementation is not correct :)
> > Okay, I will any word about dmatest here since memcpy assume no
> > calling slave_config.
> > >
> > > >
> > > > Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> > > > ---
> > > >  drivers/dma/imx-sdma.c | 37 ++++++++++++++++++++++++++++---------
> > > >  1 file changed, 28 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index
> > > > ed2267d..48f3749 100644
> > > > --- a/drivers/dma/imx-sdma.c
> > > > +++ b/drivers/dma/imx-sdma.c
> > > > @@ -1222,10 +1222,36 @@ static int
> > > > sdma_alloc_chan_resources(struct dma_chan *chan)  {
> > > >  	struct sdma_channel *sdmac = to_sdma_chan(chan);
> > > >  	struct imx_dma_data *data = chan->private;
> > > > +	struct imx_dma_data default_data;
> > > >  	int prio, ret;
> > > >
> > > > -	if (!data)
> > > > -		return -EINVAL;
> > > > +	ret = clk_enable(sdmac->sdma->clk_ipg);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +	ret = clk_enable(sdmac->sdma->clk_ahb);
> > > > +	if (ret)
> > > > +		goto disable_clk_ipg;
> > > > +	/*
> > > > +	 * dmatest(memcpy) will never call dmaengine_slave_config before
> prep,
> > > > +	 * so jobs in dmaengine_slave_config need to be moved into
> somewhere
> > > > +	 * before device_prep_dma_memcpy. Besides, dmatest never setup
> chan
> > > > +	 * ->private as other common cases like uart/audio/spi will setup
> > > > +	 * chan->private always. Here check it to judge if it's dmatest case
> > > > +	 * and do jobs in slave_config.
> > > > +	 */
> > > > +	if (!data) {
> > > > +		dev_warn(sdmac->sdma->dev, "dmatest is running?\n");
> > >
> > > why is that a warning!
> > Current SDMA driver assume filter function to set chan->private with
> > specific data (struct imx_dma_data dma_data)like below
> (sound/soc/fsl/fsl_asrc_dma.c):
> > static bool filter(struct dma_chan *chan, void *param) {
> >         if (!imx_dma_is_general_purpose(chan))
> >                 return false;
> >         chan->private = param;
> >         return true;
> > }
> >
> > But in memcpy case, at lease dmatest case, no chan->private set in its filter
> function.
> > So here take dmatest a special case and do some prepare jobs for
> > memcpy. But if the Upper device driver call dma_request_channel() with
> > their specific filter without 'chan->private' setting in the future.
> > The warning message is a useful hint to them to Add 'chan->private' in filter
> function.  Or doc it somewhere?
> 
> Instead of doing heuristics to guess whether we are doing memcpy you could
> instead make memcpy the default when slave_config is not called, i.e. drop the
> if (!data) check completely.
Yes, for memcpy case, that's a good way, but how to warning the future case
Without setup 'chan->private'...
> 
> > >
> > > > +		sdmac->word_size  =
> sdmac->sdma->dma_device.copy_align;
> > > > +		default_data.priority = 2;
> > > > +		default_data.peripheral_type = IMX_DMATYPE_MEMORY;
> > > > +		default_data.dma_request = 0;
> > > > +		default_data.dma_request2 = 0;
> > > > +		data = &default_data;
> > > > +
> > > > +		sdma_config_ownership(sdmac, false, true, false);
> > > > +		sdma_get_pc(sdmac, IMX_DMATYPE_MEMORY);
> > > > +		sdma_load_context(sdmac);
> > > > +	}
> > >
> > > this needs to be default for memcpy
> 
> The problem seems to be that we do not know whether we are doing memcpy
> or not. Normally we get the information how a channel is to be configured in
> dma_device->device_config, but this function is not called in the memcpy case.
> 
> An alternative might also be to do the setup in
> dma_device->device_prep_dma_memcpy.
Yes, I've think about it before, but such prepare steps only needed in once time...
> 
> Sascha
> 
> --
> Pengutronix e.K.                           |
> |
> Industrial Linux Solutions                 |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> pengutronix.de%2F&amp;data=02%7C01%7Cyibin.gong%40nxp.com%7C3fcf03
> db12f441398fbb08d5e6fb142b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
> %7C0%7C636668888416328960&amp;sdata=E1DT1BW4b5Q1VWgkMNZqA28
> oK%2FVVQviC8qF2%2BqG0Feo%3D&amp;reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> +49-5121-206917-5555 |

Re: [PATCH v1 3/4] dmaengine: imx-sdma: support dmatest

[Vinod]

On 11-07-18, 08:53, s.hauer@pengutronix.de wrote:
> On Wed, Jul 11, 2018 at 06:37:02AM +0000, Robin Gong wrote:
> > 
> > > -----Original Message-----
> > > From: Vinod [mailto:vkoul@kernel.org]
> > > Sent: 2018年7月10日 23:33
> > > To: Robin Gong <yibin.gong@nxp.com>
> > > Cc: dan.j.williams@intel.com; shawnguo@kernel.org;
> > > s.hauer@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> > > linux@armlinux.org.uk; linux-arm-kernel@lists.infradead.org;
> > > kernel@pengutronix.de; dmaengine@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > > Subject: Re: [PATCH v1 3/4] dmaengine: imx-sdma: support dmatest
> > > 
> > > On 11-07-18, 00:23, Robin Gong wrote:
> > > > dmatest(memcpy) will never call dmaengine_slave_config before prep,
> > > 
> > > and that should have been a hint to you that you should not expect that
> > > 
> > > > so jobs in dmaengine_slave_config need to be moved into somewhere
> > > > before device_prep_dma_memcpy. Besides, dmatest never setup chan
> > > > ->private as other common case like uart/audio/spi will always setup
> > > > chan->private. Here check it to judge if it's dmatest case and do
> > > > jobs in slave_config.
> > > 
> > > and you should not do anything for dmatest. Supporting it means memcpy
> > > implementation is not correct :)
> > Okay, I will any word about dmatest here since memcpy assume no calling
> > slave_config.
> > > 
> > > >
> > > > Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> > > > ---
> > > >  drivers/dma/imx-sdma.c | 37 ++++++++++++++++++++++++++++---------
> > > >  1 file changed, 28 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index
> > > > ed2267d..48f3749 100644
> > > > --- a/drivers/dma/imx-sdma.c
> > > > +++ b/drivers/dma/imx-sdma.c
> > > > @@ -1222,10 +1222,36 @@ static int sdma_alloc_chan_resources(struct
> > > > dma_chan *chan)  {
> > > >  	struct sdma_channel *sdmac = to_sdma_chan(chan);
> > > >  	struct imx_dma_data *data = chan->private;
> > > > +	struct imx_dma_data default_data;
> > > >  	int prio, ret;
> > > >
> > > > -	if (!data)
> > > > -		return -EINVAL;
> > > > +	ret = clk_enable(sdmac->sdma->clk_ipg);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +	ret = clk_enable(sdmac->sdma->clk_ahb);
> > > > +	if (ret)
> > > > +		goto disable_clk_ipg;
> > > > +	/*
> > > > +	 * dmatest(memcpy) will never call dmaengine_slave_config before prep,
> > > > +	 * so jobs in dmaengine_slave_config need to be moved into somewhere
> > > > +	 * before device_prep_dma_memcpy. Besides, dmatest never setup chan
> > > > +	 * ->private as other common cases like uart/audio/spi will setup
> > > > +	 * chan->private always. Here check it to judge if it's dmatest case
> > > > +	 * and do jobs in slave_config.
> > > > +	 */
> > > > +	if (!data) {
> > > > +		dev_warn(sdmac->sdma->dev, "dmatest is running?\n");
> > > 
> > > why is that a warning!
> > Current SDMA driver assume filter function to set chan->private with specific data 
> > (struct imx_dma_data dma_data)like below (sound/soc/fsl/fsl_asrc_dma.c):
> > static bool filter(struct dma_chan *chan, void *param)
> > {
> >         if (!imx_dma_is_general_purpose(chan))
> >                 return false;
> >         chan->private = param;
> >         return true;
> > }
> > 
> > But in memcpy case, at lease dmatest case, no chan->private set in its filter function.
> > So here take dmatest a special case and do some prepare jobs for memcpy. But if the
> > Upper device driver call dma_request_channel() with their specific filter without
> > 'chan->private' setting in the future. The warning message is a useful hint to them to
> > Add 'chan->private' in filter function.  Or doc it somewhere?
> 
> Instead of doing heuristics to guess whether we are doing memcpy you
> could instead make memcpy the default when slave_config is not called,
> i.e. drop the if (!data) check completely.
> 
> > > 
> > > > +		sdmac->word_size  =  sdmac->sdma->dma_device.copy_align;
> > > > +		default_data.priority = 2;
> > > > +		default_data.peripheral_type = IMX_DMATYPE_MEMORY;
> > > > +		default_data.dma_request = 0;
> > > > +		default_data.dma_request2 = 0;
> > > > +		data = &default_data;
> > > > +
> > > > +		sdma_config_ownership(sdmac, false, true, false);
> > > > +		sdma_get_pc(sdmac, IMX_DMATYPE_MEMORY);
> > > > +		sdma_load_context(sdmac);
> > > > +	}
> > > 
> > > this needs to be default for memcpy
> 
> The problem seems to be that we do not know whether we are doing memcpy
> or not. Normally we get the information how a channel is to be
> configured in dma_device->device_config, but this function is not called
> in the memcpy case.

Not really true, device_config only provides parameters to be
configured for next slave transaction

> An alternative might also be to do the setup in dma_device->device_prep_dma_memcpy.

Precisely, see how other drivers do this

Let's roll back a bit and foresee why is this required.

In case of memcpy, you need to tell DMA to do transfer from src to dstn
and size. Additional parameters like buswidth etc should be derived for
maximum throughput (after all we are dma, people want it to be done
fastest)

Now for slave, you are interfacing with a peripheral and don't know how
that is setup. So you need to match the parameters, otherwise you get
overflow or underflow and hence need for device_config

Please do not derive additional notions from these, please do not assume
anything else, unless provided in documentation :)

In doubt, just ask!

HTH
-- 
~Vinod

RE: [PATCH v1 3/4] dmaengine: imx-sdma: support dmatest

[Robin Gong]

> -----Original Message-----
> From: Vinod [mailto:vkoul@kernel.org]
> Sent: 2018年7月11日 15:19
> To: s.hauer@pengutronix.de
> Cc: Robin Gong <yibin.gong@nxp.com>; dan.j.williams@intel.com;
> shawnguo@kernel.org; Fabio Estevam <fabio.estevam@nxp.com>;
> linux@armlinux.org.uk; linux-arm-kernel@lists.infradead.org;
> kernel@pengutronix.de; dmaengine@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v1 3/4] dmaengine: imx-sdma: support dmatest
> 
> On 11-07-18, 08:53, s.hauer@pengutronix.de wrote:
> > On Wed, Jul 11, 2018 at 06:37:02AM +0000, Robin Gong wrote:
> > >
> > > > -----Original Message-----
> > > > From: Vinod [mailto:vkoul@kernel.org]
> > > > Sent: 2018年7月10日 23:33
> > > > To: Robin Gong <yibin.gong@nxp.com>
> > > > Cc: dan.j.williams@intel.com; shawnguo@kernel.org;
> > > > s.hauer@pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>;
> > > > linux@armlinux.org.uk; linux-arm-kernel@lists.infradead.org;
> > > > kernel@pengutronix.de; dmaengine@vger.kernel.org;
> > > > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > > > Subject: Re: [PATCH v1 3/4] dmaengine: imx-sdma: support dmatest
> > > >
> > > > On 11-07-18, 00:23, Robin Gong wrote:
> > > > > dmatest(memcpy) will never call dmaengine_slave_config before
> > > > > prep,
> > > >
> > > > and that should have been a hint to you that you should not expect
> > > > that
> > > >
> > > > > so jobs in dmaengine_slave_config need to be moved into
> > > > > somewhere before device_prep_dma_memcpy. Besides, dmatest never
> > > > > setup chan
> > > > > ->private as other common case like uart/audio/spi will always
> > > > > ->setup
> > > > > chan->private. Here check it to judge if it's dmatest case and
> > > > > chan->do
> > > > > jobs in slave_config.
> > > >
> > > > and you should not do anything for dmatest. Supporting it means
> > > > memcpy implementation is not correct :)
> > > Okay, I will any word about dmatest here since memcpy assume no
> > > calling slave_config.
> > > >
> > > > >
> > > > > Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> > > > > ---
> > > > >  drivers/dma/imx-sdma.c | 37
> > > > > ++++++++++++++++++++++++++++---------
> > > > >  1 file changed, 28 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > > > > index
> > > > > ed2267d..48f3749 100644
> > > > > --- a/drivers/dma/imx-sdma.c
> > > > > +++ b/drivers/dma/imx-sdma.c
> > > > > @@ -1222,10 +1222,36 @@ static int
> > > > > sdma_alloc_chan_resources(struct dma_chan *chan)  {
> > > > >  	struct sdma_channel *sdmac = to_sdma_chan(chan);
> > > > >  	struct imx_dma_data *data = chan->private;
> > > > > +	struct imx_dma_data default_data;
> > > > >  	int prio, ret;
> > > > >
> > > > > -	if (!data)
> > > > > -		return -EINVAL;
> > > > > +	ret = clk_enable(sdmac->sdma->clk_ipg);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +	ret = clk_enable(sdmac->sdma->clk_ahb);
> > > > > +	if (ret)
> > > > > +		goto disable_clk_ipg;
> > > > > +	/*
> > > > > +	 * dmatest(memcpy) will never call dmaengine_slave_config before
> prep,
> > > > > +	 * so jobs in dmaengine_slave_config need to be moved into
> somewhere
> > > > > +	 * before device_prep_dma_memcpy. Besides, dmatest never setup
> chan
> > > > > +	 * ->private as other common cases like uart/audio/spi will setup
> > > > > +	 * chan->private always. Here check it to judge if it's dmatest case
> > > > > +	 * and do jobs in slave_config.
> > > > > +	 */
> > > > > +	if (!data) {
> > > > > +		dev_warn(sdmac->sdma->dev, "dmatest is running?\n");
> > > >
> > > > why is that a warning!
> > > Current SDMA driver assume filter function to set chan->private with
> > > specific data (struct imx_dma_data dma_data)like below
> (sound/soc/fsl/fsl_asrc_dma.c):
> > > static bool filter(struct dma_chan *chan, void *param) {
> > >         if (!imx_dma_is_general_purpose(chan))
> > >                 return false;
> > >         chan->private = param;
> > >         return true;
> > > }
> > >
> > > But in memcpy case, at lease dmatest case, no chan->private set in its filter
> function.
> > > So here take dmatest a special case and do some prepare jobs for
> > > memcpy. But if the Upper device driver call dma_request_channel()
> > > with their specific filter without 'chan->private' setting in the
> > > future. The warning message is a useful hint to them to Add 'chan->private'
> in filter function.  Or doc it somewhere?
> >
> > Instead of doing heuristics to guess whether we are doing memcpy you
> > could instead make memcpy the default when slave_config is not called,
> > i.e. drop the if (!data) check completely.
> >
> > > >
> > > > > +		sdmac->word_size  =
> sdmac->sdma->dma_device.copy_align;
> > > > > +		default_data.priority = 2;
> > > > > +		default_data.peripheral_type = IMX_DMATYPE_MEMORY;
> > > > > +		default_data.dma_request = 0;
> > > > > +		default_data.dma_request2 = 0;
> > > > > +		data = &default_data;
> > > > > +
> > > > > +		sdma_config_ownership(sdmac, false, true, false);
> > > > > +		sdma_get_pc(sdmac, IMX_DMATYPE_MEMORY);
> > > > > +		sdma_load_context(sdmac);
> > > > > +	}
> > > >
> > > > this needs to be default for memcpy
> >
> > The problem seems to be that we do not know whether we are doing
> > memcpy or not. Normally we get the information how a channel is to be
> > configured in dma_device->device_config, but this function is not
> > called in the memcpy case.
> 
> Not really true, device_config only provides parameters to be configured for
> next slave transaction
> 
> > An alternative might also be to do the setup in
> dma_device->device_prep_dma_memcpy.
> 
> Precisely, see how other drivers do this
> 
> Let's roll back a bit and foresee why is this required.
> 
> In case of memcpy, you need to tell DMA to do transfer from src to dstn and
> size. Additional parameters like buswidth etc should be derived for maximum
> throughput (after all we are dma, people want it to be done
> fastest)
> 
> Now for slave, you are interfacing with a peripheral and don't know how that is
> setup. So you need to match the parameters, otherwise you get overflow or
> underflow and hence need for device_config
> 
> Please do not derive additional notions from these, please do not assume
> anything else, unless provided in documentation :)
I will move such prepare jobs from slave_config to device_prep_dma_memcpy
Instead of device_alloc_chan_resources as I did in v1, thus we have no 'chan->private'
issue, just like drivers/dma/stm32-mdma.c. The only limitation is those prepare jobs
(some register setting) will be done every time memcpy instead of only one time in slave_config
or v1 case. Is that ok?
> 
> In doubt, just ask!
> 
> HTH
> --
> ~Vinod

Re: [PATCH v1 3/4] dmaengine: imx-sdma: support dmatest

[Vinod]

On 11-07-18, 08:16, Robin Gong wrote:

> > > The problem seems to be that we do not know whether we are doing
> > > memcpy or not. Normally we get the information how a channel is to be
> > > configured in dma_device->device_config, but this function is not
> > > called in the memcpy case.
> > 
> > Not really true, device_config only provides parameters to be configured for
> > next slave transaction
> > 
> > > An alternative might also be to do the setup in
> > dma_device->device_prep_dma_memcpy.
> > 
> > Precisely, see how other drivers do this
> > 
> > Let's roll back a bit and foresee why is this required.
> > 
> > In case of memcpy, you need to tell DMA to do transfer from src to dstn and
> > size. Additional parameters like buswidth etc should be derived for maximum
> > throughput (after all we are dma, people want it to be done
> > fastest)
> > 
> > Now for slave, you are interfacing with a peripheral and don't know how that is
> > setup. So you need to match the parameters, otherwise you get overflow or
> > underflow and hence need for device_config
> > 
> > Please do not derive additional notions from these, please do not assume
> > anything else, unless provided in documentation :)
> I will move such prepare jobs from slave_config to device_prep_dma_memcpy
> Instead of device_alloc_chan_resources as I did in v1, thus we have no 'chan->private'
> issue, just like drivers/dma/stm32-mdma.c. The only limitation is those prepare jobs
> (some register setting) will be done every time memcpy instead of only one time in slave_config
> or v1 case. Is that ok?

sounds fine to me

-- 
~Vinod

[PATCH v1 0/4] add memcpy support for sdma

[Robin Gong]

This patchset is to add memcpy interface for imx-sdma, besides,to
support dmatest and enable config related, so that easily test dma
without any other device support such as uart/audio/spi...

Robin Gong (4):
  dmaengine: imx-sdma: add memcpy interface
  dmaengine: imx-sdma: add check_bd_buswidth() to kill the dulicated
    code
  dmaengine: imx-sdma: support dmatest
  ARM: configs: imx_v6_v7_defconfig: add DMATEST support

 arch/arm/configs/imx_v6_v7_defconfig |   3 +-
 drivers/dma/imx-sdma.c               | 172 ++++++++++++++++++++++++++++-------
 2 files changed, 139 insertions(+), 36 deletions(-)

-- 
2.7.4