[PATCH 1/6] dmaengine: xilinx_dma: fix splitting transfer causes misalignments

[PATCH 1/6] dmaengine: xilinx_dma: fix splitting transfer causes misalignments

[Andrea Merello]

Whenever a single or cyclic transaction is prepared, the driver
could eventually split it over several SG descriptors in order
to deal with the HW maximum transfer length.

This could end up in DMA operations starting from a misaligned
address. This seems fatal for the HW.

This patch eventually adjusts the transfer size in order to make sure
all operations start from an aligned address.

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 drivers/dma/xilinx/xilinx_dma.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index 27b523530c4a..a516e7ffef21 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -376,6 +376,7 @@ struct xilinx_dma_chan {
 	void (*start_transfer)(struct xilinx_dma_chan *chan);
 	int (*stop_transfer)(struct xilinx_dma_chan *chan);
 	u16 tdest;
+	u32 copy_mask;
 };
 
 /**
@@ -1789,10 +1790,14 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_slave_sg(
 
 			/*
 			 * Calculate the maximum number of bytes to transfer,
-			 * making sure it is less than the hw limit
+			 * making sure it is less than the hw limit and that
+			 * the next chuck start address is aligned
 			 */
-			copy = min_t(size_t, sg_dma_len(sg) - sg_used,
-				     XILINX_DMA_MAX_TRANS_LEN);
+			copy = sg_dma_len(sg) - sg_used;
+			if (copy > XILINX_DMA_MAX_TRANS_LEN)
+				copy = XILINX_DMA_MAX_TRANS_LEN &
+					chan->copy_mask;
+
 			hw = &segment->hw;
 
 			/* Fill in the descriptor */
@@ -1894,10 +1899,14 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_dma_cyclic(
 
 			/*
 			 * Calculate the maximum number of bytes to transfer,
-			 * making sure it is less than the hw limit
+			 * making sure it is less than the hw limit and that
+			 * the next chuck start address is aligned
 			 */
-			copy = min_t(size_t, period_len - sg_used,
-				     XILINX_DMA_MAX_TRANS_LEN);
+			copy = period_len - sg_used;
+			if (copy > XILINX_DMA_MAX_TRANS_LEN)
+				copy = XILINX_DMA_MAX_TRANS_LEN &
+					chan->copy_mask;
+
 			hw = &segment->hw;
 			xilinx_axidma_buf(chan, hw, buf_addr, sg_used,
 					  period_len * i);
@@ -2402,8 +2411,12 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device *xdev,
 	if (width > 8)
 		has_dre = false;
 
-	if (!has_dre)
+	if (has_dre) {
+		chan->copy_mask = ~0;
+	} else {
 		xdev->common.copy_align = fls(width - 1);
+		chan->copy_mask = ~(width - 1);
+	}
 
 	if (of_device_is_compatible(node, "xlnx,axi-vdma-mm2s-channel") ||
 	    of_device_is_compatible(node, "xlnx,axi-dma-mm2s-channel") ||
-- 
2.17.1

[PATCH 2/6] dmaengine: xilinx_dma: fix completion callback is not invoked for each DMA operation

[Andrea Merello]

API specification says: "On completion of each DMA operation, the next in
queue is started and a tasklet triggered. The tasklet will then call the
client driver completion callback routine for notification, if set."

Currently the driver keeps a "desc_pendingcount" counter of the total
descriptor pending, and it uses as IRQ coalesce threshold, as result it
only calls the CBs after ALL pending operations are completed, which is
wrong.

This patch uses disable IRQ coalesce and checks for the completion flag
for the descriptors (which is further divided in segments).

Possibly a better optimization could be using proper IRQ coalesce
threshold to get an IRQ after all segments of the descriptors are done.
But we don't do that yet..

NOTE: for now we do this only for AXI DMA, other DMA flavors are
untested/untouched.
This is loosely based on
commit 65df81a6dc74 ("xilinx_dma: IrqThreshold set incorrectly, unreliable.")
in my linux-4.6-zynq tree

From: Jeremy Trimble [original patch]
Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 drivers/dma/xilinx/xilinx_dma.c | 39 +++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index a516e7ffef21..cf12f7147f07 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -164,6 +164,7 @@
 #define XILINX_DMA_CR_COALESCE_SHIFT	16
 #define XILINX_DMA_BD_SOP		BIT(27)
 #define XILINX_DMA_BD_EOP		BIT(26)
+#define XILINX_DMA_BD_CMPLT		BIT(31)
 #define XILINX_DMA_COALESCE_MAX		255
 #define XILINX_DMA_NUM_DESCS		255
 #define XILINX_DMA_NUM_APP_WORDS	5
@@ -1274,12 +1275,9 @@ static void xilinx_dma_start_transfer(struct xilinx_dma_chan *chan)
 
 	reg = dma_ctrl_read(chan, XILINX_DMA_REG_DMACR);
 
-	if (chan->desc_pendingcount <= XILINX_DMA_COALESCE_MAX) {
-		reg &= ~XILINX_DMA_CR_COALESCE_MAX;
-		reg |= chan->desc_pendingcount <<
-				  XILINX_DMA_CR_COALESCE_SHIFT;
-		dma_ctrl_write(chan, XILINX_DMA_REG_DMACR, reg);
-	}
+	reg &= ~XILINX_DMA_CR_COALESCE_MAX;
+	reg |= 1 << XILINX_DMA_CR_COALESCE_SHIFT;
+	dma_ctrl_write(chan, XILINX_DMA_REG_DMACR, reg);
 
 	if (chan->has_sg && !chan->xdev->mcdma)
 		xilinx_write(chan, XILINX_DMA_REG_CURDESC,
@@ -1378,6 +1376,20 @@ static void xilinx_dma_complete_descriptor(struct xilinx_dma_chan *chan)
 		return;
 
 	list_for_each_entry_safe(desc, next, &chan->active_list, node) {
+		if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
+			/*
+			 * Check whether the last segment in this descriptor
+			 * has been completed.
+			 */
+			const struct xilinx_axidma_tx_segment *const tail_seg =
+				list_last_entry(&desc->segments,
+						struct xilinx_axidma_tx_segment,
+						node);
+
+			/* we've processed all the completed descriptors */
+			if (!(tail_seg->hw.status & XILINX_DMA_BD_CMPLT))
+				break;
+		}
 		list_del(&desc->node);
 		if (!desc->cyclic)
 			dma_cookie_complete(&desc->async_tx);
@@ -1826,14 +1838,13 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_slave_sg(
 				   struct xilinx_axidma_tx_segment, node);
 	desc->async_tx.phys = segment->phys;
 
-	/* For the last DMA_MEM_TO_DEV transfer, set EOP */
-	if (chan->direction == DMA_MEM_TO_DEV) {
-		segment->hw.control |= XILINX_DMA_BD_SOP;
-		segment = list_last_entry(&desc->segments,
-					  struct xilinx_axidma_tx_segment,
-					  node);
-		segment->hw.control |= XILINX_DMA_BD_EOP;
-	}
+	/* For the first transfer, set SOP */
+	segment->hw.control |= XILINX_DMA_BD_SOP;
+	/* For the last transfer, set EOP */
+	segment = list_last_entry(&desc->segments,
+				  struct xilinx_axidma_tx_segment,
+				  node);
+	segment->hw.control |= XILINX_DMA_BD_EOP;
 
 	return &desc->async_tx;
 
-- 
2.17.1

[PATCH 3/6] dt-bindings: xilinx_dma: add required xlnx, lengthregwidth property

[Andrea Merello]

The width of the "length register" cannot be autodetected, and it is now
specified with a DT property. Add DOC for it.

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
index a2b8bfaec43c..acecdc5d8d47 100644
--- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
+++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
@@ -36,6 +36,8 @@ Required properties:
 
 Required properties for VDMA:
 - xlnx,num-fstores: Should be the number of framebuffers as configured in h/w.
+Required properties for AXI DMA:
+- xlnx,lengthregwidth: Should be the width of the length register as configured in h/w.
 
 Optional properties:
 - xlnx,include-sg: Tells configured for Scatter-mode in
-- 
2.17.1

[PATCH 4/6] dmaengine: xilinx_dma: fix hardcoded maximum transfer length may be wrong

[Andrea Merello]

The maximum transfer length is currently hardcoded in the driver, but
it depends by how the soft-IP is actually configured.

This seems to affect also max possible length for SG transfers.

This patch introduce a new DT property in order to operate with proper
maximum transfer length.

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 drivers/dma/xilinx/xilinx_dma.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index cf12f7147f07..bdbc8ba9092a 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -439,6 +439,7 @@ struct xilinx_dma_device {
 	struct clk *rxs_clk;
 	u32 nr_channels;
 	u32 chan_id;
+	int max_transfer;
 };
 
 /* Macros */
@@ -1806,8 +1807,8 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_slave_sg(
 			 * the next chuck start address is aligned
 			 */
 			copy = sg_dma_len(sg) - sg_used;
-			if (copy > XILINX_DMA_MAX_TRANS_LEN)
-				copy = XILINX_DMA_MAX_TRANS_LEN &
+			if (copy > chan->xdev->max_transfer)
+				copy = chan->xdev->max_transfer &
 					chan->copy_mask;
 
 			hw = &segment->hw;
@@ -1914,8 +1915,8 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_dma_cyclic(
 			 * the next chuck start address is aligned
 			 */
 			copy = period_len - sg_used;
-			if (copy > XILINX_DMA_MAX_TRANS_LEN)
-				copy = XILINX_DMA_MAX_TRANS_LEN &
+			if (copy > chan->xdev->max_transfer)
+				copy = chan->xdev->max_transfer &
 					chan->copy_mask;
 
 			hw = &segment->hw;
@@ -2594,7 +2595,7 @@ static int xilinx_dma_probe(struct platform_device *pdev)
 	struct xilinx_dma_device *xdev;
 	struct device_node *child, *np = pdev->dev.of_node;
 	struct resource *io;
-	u32 num_frames, addr_width;
+	u32 num_frames, addr_width, lenreg_width;
 	int i, err;
 
 	/* Allocate and initialize the DMA engine structure */
@@ -2625,9 +2626,18 @@ static int xilinx_dma_probe(struct platform_device *pdev)
 		return PTR_ERR(xdev->regs);
 
 	/* Retrieve the DMA engine properties from the device tree */
-	xdev->has_sg = of_property_read_bool(node, "xlnx,include-sg");
-	if (xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA)
+
+	if (xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
 		xdev->mcdma = of_property_read_bool(node, "xlnx,mcdma");
+		err = of_property_read_u32(node, "xlnx,lengthregwidth",
+					&lenreg_width);
+		if (err < 0) {
+			dev_err(xdev->dev,
+				"missing xlnx,lengthregwidth property\n");
+			return err;
+		}
+		xdev->max_transfer = GENMASK(lenreg_width - 1, 0);
+	}
 
 	if (xdev->dma_config->dmatype == XDMA_TYPE_VDMA) {
 		err = of_property_read_u32(node, "xlnx,num-fstores",
-- 
2.17.1

[PATCH 5/6] dmaengine: xilinx_dma: autodetect whether the HW supports scatter-gather

[Andrea Merello]

The HW can be either direct-access or scatter-gather version. These are
SW incompatible.

The driver can handle both version: a DT property was used to
tell the driver whether to assume the HW is is scatter-gather mode.

This patch makes the driver to autodetect this information. The DT
property is not required anymore.

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 drivers/dma/xilinx/xilinx_dma.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index bdbc8ba9092a..8c6e818e596f 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -86,6 +86,7 @@
 #define XILINX_DMA_DMASR_DMA_DEC_ERR		BIT(6)
 #define XILINX_DMA_DMASR_DMA_SLAVE_ERR		BIT(5)
 #define XILINX_DMA_DMASR_DMA_INT_ERR		BIT(4)
+#define XILINX_DMA_DMASR_SG_MASK		BIT(3)
 #define XILINX_DMA_DMASR_IDLE			BIT(1)
 #define XILINX_DMA_DMASR_HALTED		BIT(0)
 #define XILINX_DMA_DMASR_DELAY_MASK		GENMASK(31, 24)
@@ -407,7 +408,6 @@ struct xilinx_dma_config {
  * @dev: Device Structure
  * @common: DMA device structure
  * @chan: Driver specific DMA channel
- * @has_sg: Specifies whether Scatter-Gather is present or not
  * @mcdma: Specifies whether Multi-Channel is present or not
  * @flush_on_fsync: Flush on frame sync
  * @ext_addr: Indicates 64 bit addressing is supported by dma device
@@ -426,7 +426,6 @@ struct xilinx_dma_device {
 	struct device *dev;
 	struct dma_device common;
 	struct xilinx_dma_chan *chan[XILINX_DMA_MAX_CHANS_PER_DEVICE];
-	bool has_sg;
 	bool mcdma;
 	u32 flush_on_fsync;
 	bool ext_addr;
@@ -2391,7 +2390,6 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device *xdev,
 
 	chan->dev = xdev->dev;
 	chan->xdev = xdev;
-	chan->has_sg = xdev->has_sg;
 	chan->desc_pendingcount = 0x0;
 	chan->ext_addr = xdev->ext_addr;
 	/* This variable ensures that descriptors are not
@@ -2488,6 +2486,13 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device *xdev,
 		chan->stop_transfer = xilinx_dma_stop_transfer;
 	}
 
+	/* check if SG is enabled */
+	if (dma_ctrl_read(chan, XILINX_DMA_REG_DMASR) &
+		XILINX_DMA_DMASR_SG_MASK)
+		chan->has_sg = true;
+	dev_dbg(chan->dev, "ch %d: SG %s\n", chan->id,
+		chan->has_sg ? "enabled" : "disabled");
+
 	/* Initialize the tasklet */
 	tasklet_init(&chan->tasklet, xilinx_dma_do_tasklet,
 			(unsigned long)chan);
@@ -2626,7 +2631,6 @@ static int xilinx_dma_probe(struct platform_device *pdev)
 		return PTR_ERR(xdev->regs);
 
 	/* Retrieve the DMA engine properties from the device tree */
-
 	if (xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
 		xdev->mcdma = of_property_read_bool(node, "xlnx,mcdma");
 		err = of_property_read_u32(node, "xlnx,lengthregwidth",
-- 
2.17.1

[PATCH 6/6] dt-bindings: xilinx_dma: drop has-sg property

[Andrea Merello]

This property is not needed anymore, because the driver now autodetects it.
Delete references in documentation.

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
index acecdc5d8d47..53341314eeb8 100644
--- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
+++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
@@ -39,9 +39,6 @@ Required properties for VDMA:
 Required properties for AXI DMA:
 - xlnx,lengthregwidth: Should be the width of the length register as configured in h/w.
 
-Optional properties:
-- xlnx,include-sg: Tells configured for Scatter-mode in
-	the hardware.
 Optional properties for AXI DMA:
 - xlnx,mcdma: Tells whether configured for multi-channel mode in the hardware.
 Optional properties for VDMA:
-- 
2.17.1

[PATCH 1/6] dmaengine: xilinx_dma: fix splitting transfer causes misalignments

[Radhey Shyam Pandey]

> -----Original Message-----
> From: dmaengine-owner at vger.kernel.org [mailto:dmaengine-
> owner at vger.kernel.org] On Behalf Of Andrea Merello
> Sent: Wednesday, June 20, 2018 2:07 PM
> To: vkoul at kernel.org; dan.j.williams at intel.com; Michal Simek
> <michals@xilinx.com>; Appana Durga Kedareswara Rao
> <appanad@xilinx.com>; dmaengine at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
> Andrea Merello <andrea.merello@gmail.com>
> Subject: [PATCH 1/6] dmaengine: xilinx_dma: fix splitting transfer causes
> misalignments

We should rephrase commit message to something like "In axidma
slave_sg and dma_cylic mode align split descriptors" 

> 
> Whenever a single or cyclic transaction is prepared, the driver
> could eventually split it over several SG descriptors in order
> to deal with the HW maximum transfer length.
> 
> This could end up in DMA operations starting from a misaligned
> address. This seems fatal for the HW.
This seems fatal for the HW if DRE is not enabled.

> 
> This patch eventually adjusts the transfer size in order to make sure
> all operations start from an aligned address.
> 
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> ---
>  drivers/dma/xilinx/xilinx_dma.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 27b523530c4a..a516e7ffef21 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -376,6 +376,7 @@ struct xilinx_dma_chan {
>  	void (*start_transfer)(struct xilinx_dma_chan *chan);
>  	int (*stop_transfer)(struct xilinx_dma_chan *chan);
>  	u16 tdest;
> +	u32 copy_mask;
We can reuse copy_align itself.  See below.

>  };
> 
>  /**
> @@ -1789,10 +1790,14 @@ static struct dma_async_tx_descriptor
> *xilinx_dma_prep_slave_sg(
> 
>  			/*
>  			 * Calculate the maximum number of bytes to transfer,
> -			 * making sure it is less than the hw limit
> +			 * making sure it is less than the hw limit and that
> +			 * the next chuck start address is aligned

/s/chuck/chunk
>  			 */
> -			copy = min_t(size_t, sg_dma_len(sg) - sg_used,
> -				     XILINX_DMA_MAX_TRANS_LEN);
> +			copy = sg_dma_len(sg) - sg_used;
> +			if (copy > XILINX_DMA_MAX_TRANS_LEN)
> +				copy = XILINX_DMA_MAX_TRANS_LEN &
> +					chan->copy_mask;
> +


In below implementation, we can reuse copy_align.
Same for dma_cyclic.

if ((copy + sg_used  < sg_dma_len(sg)) &&
	chan->xdev->common.copy_align) {
	/* If this is not the last descriptor, make sure
	  * the next one will be properly aligned
	 */
	copy = rounddown(copy,
               (1 << chan->xdev->common.copy_align));
     }

>  			hw = &segment->hw;
> 
>  			/* Fill in the descriptor */
> @@ -1894,10 +1899,14 @@ static struct dma_async_tx_descriptor
> *xilinx_dma_prep_dma_cyclic(
> 
>  			/*
>  			 * Calculate the maximum number of bytes to transfer,
> -			 * making sure it is less than the hw limit
> +			 * making sure it is less than the hw limit and that
> +			 * the next chuck start address is aligned
>  			 */
> -			copy = min_t(size_t, period_len - sg_used,
> -				     XILINX_DMA_MAX_TRANS_LEN);
> +			copy = period_len - sg_used;
> +			if (copy > XILINX_DMA_MAX_TRANS_LEN)
> +				copy = XILINX_DMA_MAX_TRANS_LEN &
> +					chan->copy_mask;
> +
>  			hw = &segment->hw;
>  			xilinx_axidma_buf(chan, hw, buf_addr, sg_used,
>  					  period_len * i);
> @@ -2402,8 +2411,12 @@ static int xilinx_dma_chan_probe(struct
> xilinx_dma_device *xdev,
>  	if (width > 8)
>  		has_dre = false;
> 
> -	if (!has_dre)
> +	if (has_dre) {
> +		chan->copy_mask = ~0;
> +	} else {
>  		xdev->common.copy_align = fls(width - 1);
> +		chan->copy_mask = ~(width - 1);
> +	}

As mentioned above we don't need this additional field.

> 
>  	if (of_device_is_compatible(node, "xlnx,axi-vdma-mm2s-channel") ||
>  	    of_device_is_compatible(node, "xlnx,axi-dma-mm2s-channel") ||
> --
> 2.17.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/6] dmaengine: xilinx_dma: fix completion callback is not invoked for each DMA operation

[Radhey Shyam Pandey]

> -----Original Message-----
> From: dmaengine-owner at vger.kernel.org [mailto:dmaengine-
> owner at vger.kernel.org] On Behalf Of Andrea Merello
> Sent: Wednesday, June 20, 2018 2:07 PM
> To: vkoul at kernel.org; dan.j.williams at intel.com; Michal Simek
> <michals@xilinx.com>; Appana Durga Kedareswara Rao
> <appanad@xilinx.com>; dmaengine at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
> Andrea Merello <andrea.merello@gmail.com>
> Subject: [PATCH 2/6] dmaengine: xilinx_dma: fix completion callback is not
> invoked for each DMA operation
> 
> API specification says: "On completion of each DMA operation, the next in
> queue is started and a tasklet triggered. The tasklet will then call the
> client driver completion callback routine for notification, if set."
> 
> Currently the driver keeps a "desc_pendingcount" counter of the total
> descriptor pending, and it uses as IRQ coalesce threshold, as result it
> only calls the CBs after ALL pending operations are completed, which is
> wrong.
I think IRQ coalescing enable/disable should be configurable. 
Performance related usecases will need this support.

> 
> This patch uses disable IRQ coalesce and checks for the completion flag
> for the descriptors (which is further divided in segments).
> 
> Possibly a better optimization could be using proper IRQ coalesce
> threshold to get an IRQ after all segments of the descriptors are done.
> But we don't do that yet..
> 
> NOTE: for now we do this only for AXI DMA, other DMA flavors are
> untested/untouched.
> This is loosely based on
> commit 65df81a6dc74 ("xilinx_dma: IrqThreshold set incorrectly, unreliable.")
> in my linux-4.6-zynq tree
NOTE description doesn't help much.  

> 
> From: Jeremy Trimble [original patch]
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> ---
>  drivers/dma/xilinx/xilinx_dma.c | 39 +++++++++++++++++++++------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index a516e7ffef21..cf12f7147f07 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -164,6 +164,7 @@
>  #define XILINX_DMA_CR_COALESCE_SHIFT	16
>  #define XILINX_DMA_BD_SOP		BIT(27)
>  #define XILINX_DMA_BD_EOP		BIT(26)
> +#define XILINX_DMA_BD_CMPLT		BIT(31)
>  #define XILINX_DMA_COALESCE_MAX		255
>  #define XILINX_DMA_NUM_DESCS		255
>  #define XILINX_DMA_NUM_APP_WORDS	5
> @@ -1274,12 +1275,9 @@ static void xilinx_dma_start_transfer(struct
> xilinx_dma_chan *chan)
> 
>  	reg = dma_ctrl_read(chan, XILINX_DMA_REG_DMACR);
> 
> -	if (chan->desc_pendingcount <= XILINX_DMA_COALESCE_MAX) {
> -		reg &= ~XILINX_DMA_CR_COALESCE_MAX;
> -		reg |= chan->desc_pendingcount <<
> -				  XILINX_DMA_CR_COALESCE_SHIFT;
> -		dma_ctrl_write(chan, XILINX_DMA_REG_DMACR, reg);
> -	}
> +	reg &= ~XILINX_DMA_CR_COALESCE_MAX;
> +	reg |= 1 << XILINX_DMA_CR_COALESCE_SHIFT;
> +	dma_ctrl_write(chan, XILINX_DMA_REG_DMACR, reg);
> 
>  	if (chan->has_sg && !chan->xdev->mcdma)
>  		xilinx_write(chan, XILINX_DMA_REG_CURDESC,
> @@ -1378,6 +1376,20 @@ static void xilinx_dma_complete_descriptor(struct
> xilinx_dma_chan *chan)
>  		return;
> 
>  	list_for_each_entry_safe(desc, next, &chan->active_list, node) {
> +		if (chan->xdev->dma_config->dmatype ==
> XDMA_TYPE_AXIDMA) {
> +			/*
> +			 * Check whether the last segment in this descriptor
> +			 * has been completed.
> +			 */
> +			const struct xilinx_axidma_tx_segment *const tail_seg
> =
> +				list_last_entry(&desc->segments,
> +						struct
> xilinx_axidma_tx_segment,
> +						node);
> +
> +			/* we've processed all the completed descriptors */
> +			if (!(tail_seg->hw.status & XILINX_DMA_BD_CMPLT))
> +				break;
> +		}
>  		list_del(&desc->node);
>  		if (!desc->cyclic)
>  			dma_cookie_complete(&desc->async_tx);
> @@ -1826,14 +1838,13 @@ static struct dma_async_tx_descriptor
> *xilinx_dma_prep_slave_sg(
>  				   struct xilinx_axidma_tx_segment, node);
>  	desc->async_tx.phys = segment->phys;
> 
> -	/* For the last DMA_MEM_TO_DEV transfer, set EOP */
> -	if (chan->direction == DMA_MEM_TO_DEV) {
> -		segment->hw.control |= XILINX_DMA_BD_SOP;
> -		segment = list_last_entry(&desc->segments,
> -					  struct xilinx_axidma_tx_segment,
> -					  node);
> -		segment->hw.control |= XILINX_DMA_BD_EOP;
> -	}
> +	/* For the first transfer, set SOP */
> +	segment->hw.control |= XILINX_DMA_BD_SOP;
> +	/* For the last transfer, set EOP */
> +	segment = list_last_entry(&desc->segments,
> +				  struct xilinx_axidma_tx_segment,
> +				  node);
> +	segment->hw.control |= XILINX_DMA_BD_EOP;
> 
>  	return &desc->async_tx;
> 
> --
> 2.17.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/6] dmaengine: xilinx_dma: fix splitting transfer causes misalignments

[Andrea Merello]

On Wed, Jun 20, 2018 at 1:37 PM, Radhey Shyam Pandey <radheys@xilinx.com> wrote:
>> -----Original Message-----
>> From: dmaengine-owner at vger.kernel.org [mailto:dmaengine-
>> owner at vger.kernel.org] On Behalf Of Andrea Merello
>> Sent: Wednesday, June 20, 2018 2:07 PM
>> To: vkoul at kernel.org; dan.j.williams at intel.com; Michal Simek
>> <michals@xilinx.com>; Appana Durga Kedareswara Rao
>> <appanad@xilinx.com>; dmaengine at vger.kernel.org
>> Cc: linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
>> Andrea Merello <andrea.merello@gmail.com>
>> Subject: [PATCH 1/6] dmaengine: xilinx_dma: fix splitting transfer causes
>> misalignments
>
> We should rephrase commit message to something like "In axidma
> slave_sg and dma_cylic mode align split descriptors"

OK

>>
>> Whenever a single or cyclic transaction is prepared, the driver
>> could eventually split it over several SG descriptors in order
>> to deal with the HW maximum transfer length.
>>
>> This could end up in DMA operations starting from a misaligned
>> address. This seems fatal for the HW.
> This seems fatal for the HW if DRE is not enabled.

OK

>>
>> This patch eventually adjusts the transfer size in order to make sure
>> all operations start from an aligned address.
>>
>> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
>> ---
>>  drivers/dma/xilinx/xilinx_dma.c | 27 ++++++++++++++++++++-------
>>  1 file changed, 20 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
>> index 27b523530c4a..a516e7ffef21 100644
>> --- a/drivers/dma/xilinx/xilinx_dma.c
>> +++ b/drivers/dma/xilinx/xilinx_dma.c
>> @@ -376,6 +376,7 @@ struct xilinx_dma_chan {
>>       void (*start_transfer)(struct xilinx_dma_chan *chan);
>>       int (*stop_transfer)(struct xilinx_dma_chan *chan);
>>       u16 tdest;
>> +     u32 copy_mask;
> We can reuse copy_align itself.  See below.

OK

>>  };
>>
>>  /**
>> @@ -1789,10 +1790,14 @@ static struct dma_async_tx_descriptor
>> *xilinx_dma_prep_slave_sg(
>>
>>                       /*
>>                        * Calculate the maximum number of bytes to transfer,
>> -                      * making sure it is less than the hw limit
>> +                      * making sure it is less than the hw limit and that
>> +                      * the next chuck start address is aligned
>
> /s/chuck/chunk

OK

>>                        */
>> -                     copy = min_t(size_t, sg_dma_len(sg) - sg_used,
>> -                                  XILINX_DMA_MAX_TRANS_LEN);
>> +                     copy = sg_dma_len(sg) - sg_used;
>> +                     if (copy > XILINX_DMA_MAX_TRANS_LEN)
>> +                             copy = XILINX_DMA_MAX_TRANS_LEN &
>> +                                     chan->copy_mask;
>> +
>
>
> In below implementation, we can reuse copy_align.
> Same for dma_cyclic.
>
> if ((copy + sg_used  < sg_dma_len(sg)) &&
>         chan->xdev->common.copy_align) {
>         /* If this is not the last descriptor, make sure
>           * the next one will be properly aligned
>          */
>         copy = rounddown(copy,
>                (1 << chan->xdev->common.copy_align));
>      }

OK for the general idea. But to me it seems a bit more complicated than needed:
What's the point in setting 'copy' with min_t, performing also the
subtraction sg_dma_len(sg) - sg_used, and then add sg_used again? What
about something like:


-                       copy = min_t(size_t, sg_dma_len(sg) - sg_used,
-                                    XILINX_DMA_MAX_TRANS_LEN);
+                       copy = sg_dma_len(sg) - sg_used;
+                       if (copy > XILINX_DMA_MAX_TRANS_LEN &&
+                           chan->xdev->common.copy_align)
+                               copy = rounddown(XILINX_DMA_MAX_TRANS_LEN,
+                                        (1 << chan->xdev->common.copy_align));
+


>>                       hw = &segment->hw;
>>
>>                       /* Fill in the descriptor */
>> @@ -1894,10 +1899,14 @@ static struct dma_async_tx_descriptor
>> *xilinx_dma_prep_dma_cyclic(
>>
>>                       /*
>>                        * Calculate the maximum number of bytes to transfer,
>> -                      * making sure it is less than the hw limit
>> +                      * making sure it is less than the hw limit and that
>> +                      * the next chuck start address is aligned
>>                        */
>> -                     copy = min_t(size_t, period_len - sg_used,
>> -                                  XILINX_DMA_MAX_TRANS_LEN);
>> +                     copy = period_len - sg_used;
>> +                     if (copy > XILINX_DMA_MAX_TRANS_LEN)
>> +                             copy = XILINX_DMA_MAX_TRANS_LEN &
>> +                                     chan->copy_mask;
>> +
>>                       hw = &segment->hw;
>>                       xilinx_axidma_buf(chan, hw, buf_addr, sg_used,
>>                                         period_len * i);
>> @@ -2402,8 +2411,12 @@ static int xilinx_dma_chan_probe(struct
>> xilinx_dma_device *xdev,
>>       if (width > 8)
>>               has_dre = false;
>>
>> -     if (!has_dre)
>> +     if (has_dre) {
>> +             chan->copy_mask = ~0;
>> +     } else {
>>               xdev->common.copy_align = fls(width - 1);
>> +             chan->copy_mask = ~(width - 1);
>> +     }
>
> As mentioned above we don't need this additional field.

OK

>>
>>       if (of_device_is_compatible(node, "xlnx,axi-vdma-mm2s-channel") ||
>>           of_device_is_compatible(node, "xlnx,axi-dma-mm2s-channel") ||
>> --
>> 2.17.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/6] dmaengine: xilinx_dma: fix completion callback is not invoked for each DMA operation

[Andrea Merello]

On Wed, Jun 20, 2018 at 2:36 PM, Radhey Shyam Pandey <radheys@xilinx.com> wrote:
>> -----Original Message-----
>> From: dmaengine-owner at vger.kernel.org [mailto:dmaengine-
>> owner at vger.kernel.org] On Behalf Of Andrea Merello
>> Sent: Wednesday, June 20, 2018 2:07 PM
>> To: vkoul at kernel.org; dan.j.williams at intel.com; Michal Simek
>> <michals@xilinx.com>; Appana Durga Kedareswara Rao
>> <appanad@xilinx.com>; dmaengine at vger.kernel.org
>> Cc: linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
>> Andrea Merello <andrea.merello@gmail.com>
>> Subject: [PATCH 2/6] dmaengine: xilinx_dma: fix completion callback is not
>> invoked for each DMA operation
>>
>> API specification says: "On completion of each DMA operation, the next in
>> queue is started and a tasklet triggered. The tasklet will then call the
>> client driver completion callback routine for notification, if set."
>>
>> Currently the driver keeps a "desc_pendingcount" counter of the total
>> descriptor pending, and it uses as IRQ coalesce threshold, as result it
>> only calls the CBs after ALL pending operations are completed, which is
>> wrong.
> I think IRQ coalescing enable/disable should be configurable.
> Performance related usecases will need this support.

I didn't intend this (only) wrt performances; my concern was mostly
wrt correctness. If my point of view is wrong then I'll drop this
patch from the series.

(.. I might respin it again in future: I had a patch wrt an old driver
version that allowed submitting new descriptors to the HW while the
DMA is running, and in this case disabling coalesce is needed i.e. in
order to submit a new empty buffer whenever the DMA finishes a
transfer without waiting the DMA to stop).

BTW, is there any dmaengine API suitable for setting interrupt coalesce?

>>
>> This patch uses disable IRQ coalesce and checks for the completion flag
>> for the descriptors (which is further divided in segments).
>>
>> Possibly a better optimization could be using proper IRQ coalesce
>> threshold to get an IRQ after all segments of the descriptors are done.
>> But we don't do that yet..
>>
>> NOTE: for now we do this only for AXI DMA, other DMA flavors are
>> untested/untouched.
>> This is loosely based on
>> commit 65df81a6dc74 ("xilinx_dma: IrqThreshold set incorrectly, unreliable.")
>> in my linux-4.6-zynq tree
> NOTE description doesn't help much.
>
>>
>> From: Jeremy Trimble [original patch]
>> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
>> ---
>>  drivers/dma/xilinx/xilinx_dma.c | 39 +++++++++++++++++++++------------
>>  1 file changed, 25 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
>> index a516e7ffef21..cf12f7147f07 100644
>> --- a/drivers/dma/xilinx/xilinx_dma.c
>> +++ b/drivers/dma/xilinx/xilinx_dma.c
>> @@ -164,6 +164,7 @@
>>  #define XILINX_DMA_CR_COALESCE_SHIFT 16
>>  #define XILINX_DMA_BD_SOP            BIT(27)
>>  #define XILINX_DMA_BD_EOP            BIT(26)
>> +#define XILINX_DMA_BD_CMPLT          BIT(31)
>>  #define XILINX_DMA_COALESCE_MAX              255
>>  #define XILINX_DMA_NUM_DESCS         255
>>  #define XILINX_DMA_NUM_APP_WORDS     5
>> @@ -1274,12 +1275,9 @@ static void xilinx_dma_start_transfer(struct
>> xilinx_dma_chan *chan)
>>
>>       reg = dma_ctrl_read(chan, XILINX_DMA_REG_DMACR);
>>
>> -     if (chan->desc_pendingcount <= XILINX_DMA_COALESCE_MAX) {
>> -             reg &= ~XILINX_DMA_CR_COALESCE_MAX;
>> -             reg |= chan->desc_pendingcount <<
>> -                               XILINX_DMA_CR_COALESCE_SHIFT;
>> -             dma_ctrl_write(chan, XILINX_DMA_REG_DMACR, reg);
>> -     }
>> +     reg &= ~XILINX_DMA_CR_COALESCE_MAX;
>> +     reg |= 1 << XILINX_DMA_CR_COALESCE_SHIFT;
>> +     dma_ctrl_write(chan, XILINX_DMA_REG_DMACR, reg);
>>
>>       if (chan->has_sg && !chan->xdev->mcdma)
>>               xilinx_write(chan, XILINX_DMA_REG_CURDESC,
>> @@ -1378,6 +1376,20 @@ static void xilinx_dma_complete_descriptor(struct
>> xilinx_dma_chan *chan)
>>               return;
>>
>>       list_for_each_entry_safe(desc, next, &chan->active_list, node) {
>> +             if (chan->xdev->dma_config->dmatype ==
>> XDMA_TYPE_AXIDMA) {
>> +                     /*
>> +                      * Check whether the last segment in this descriptor
>> +                      * has been completed.
>> +                      */
>> +                     const struct xilinx_axidma_tx_segment *const tail_seg
>> =
>> +                             list_last_entry(&desc->segments,
>> +                                             struct
>> xilinx_axidma_tx_segment,
>> +                                             node);
>> +
>> +                     /* we've processed all the completed descriptors */
>> +                     if (!(tail_seg->hw.status & XILINX_DMA_BD_CMPLT))
>> +                             break;
>> +             }
>>               list_del(&desc->node);
>>               if (!desc->cyclic)
>>                       dma_cookie_complete(&desc->async_tx);
>> @@ -1826,14 +1838,13 @@ static struct dma_async_tx_descriptor
>> *xilinx_dma_prep_slave_sg(
>>                                  struct xilinx_axidma_tx_segment, node);
>>       desc->async_tx.phys = segment->phys;
>>
>> -     /* For the last DMA_MEM_TO_DEV transfer, set EOP */
>> -     if (chan->direction == DMA_MEM_TO_DEV) {
>> -             segment->hw.control |= XILINX_DMA_BD_SOP;
>> -             segment = list_last_entry(&desc->segments,
>> -                                       struct xilinx_axidma_tx_segment,
>> -                                       node);
>> -             segment->hw.control |= XILINX_DMA_BD_EOP;
>> -     }
>> +     /* For the first transfer, set SOP */
>> +     segment->hw.control |= XILINX_DMA_BD_SOP;
>> +     /* For the last transfer, set EOP */
>> +     segment = list_last_entry(&desc->segments,
>> +                               struct xilinx_axidma_tx_segment,
>> +                               node);
>> +     segment->hw.control |= XILINX_DMA_BD_EOP;
>>
>>       return &desc->async_tx;
>>
>> --
>> 2.17.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/6] dt-bindings: xilinx_dma: add required xlnx,lengthregwidth property

[Radhey Shyam Pandey]

> -----Original Message-----
> From: dmaengine-owner at vger.kernel.org [mailto:dmaengine-
> owner at vger.kernel.org] On Behalf Of Andrea Merello
> Sent: Wednesday, June 20, 2018 2:07 PM
> To: vkoul at kernel.org; dan.j.williams at intel.com; Michal Simek
> <michals@xilinx.com>; Appana Durga Kedareswara Rao
> <appanad@xilinx.com>; dmaengine at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
> Andrea Merello <andrea.merello@gmail.com>
> Subject: [PATCH 3/6] dt-bindings: xilinx_dma: add required
> xlnx,lengthregwidth property

dt-bindings: dmaengine: xilinx_dma

Please also include DT folks.
> 
> The width of the "length register" cannot be autodetected, and it is now
> specified with a DT property. Add DOC for it.
> 
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> ---
>  Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> index a2b8bfaec43c..acecdc5d8d47 100644
> --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> @@ -36,6 +36,8 @@ Required properties:
> 
>  Required properties for VDMA:
>  - xlnx,num-fstores: Should be the number of framebuffers as configured in
> h/w.
> +Required properties for AXI DMA:
> +- xlnx,lengthregwidth: Should be the width of the length register as
> configured in h/w.

One suggestion to be inline with IP property naming we can rename 
this prop to "xlnx,sg-length-width"? Please take a look at Xilinx tree
we have this feature added in the master branch. It would be good
to consolidate both implementations and upstream. Let me know 
if there are any followup queries. 
 
> 
>  Optional properties:
>  - xlnx,include-sg: Tells configured for Scatter-mode in
> --
> 2.17.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/6] dt-bindings: xilinx_dma: add required xlnx,lengthregwidth property

[Radhey Shyam Pandey]

> -----Original Message-----
> From: Radhey Shyam Pandey
> Sent: Wednesday, June 20, 2018 7:13 PM
> To: Andrea Merello <andrea.merello@gmail.com>; vkoul at kernel.org;
> dan.j.williams at intel.com; Michal Simek <michals@xilinx.com>; Appana Durga
> Kedareswara Rao <appanad@xilinx.com>; dmaengine at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org
> Subject: RE: [PATCH 3/6] dt-bindings: xilinx_dma: add required
> xlnx,lengthregwidth property
> 
> 
> > -----Original Message-----
> > From: dmaengine-owner at vger.kernel.org [mailto:dmaengine-
> > owner at vger.kernel.org] On Behalf Of Andrea Merello
> > Sent: Wednesday, June 20, 2018 2:07 PM
> > To: vkoul at kernel.org; dan.j.williams at intel.com; Michal Simek
> > <michals@xilinx.com>; Appana Durga Kedareswara Rao
> > <appanad@xilinx.com>; dmaengine at vger.kernel.org
> > Cc: linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
> > Andrea Merello <andrea.merello@gmail.com>
> > Subject: [PATCH 3/6] dt-bindings: xilinx_dma: add required
> > xlnx,lengthregwidth property
> 
> dt-bindings: dmaengine: xilinx_dma
> 
> Please also include DT folks.
> >
> > The width of the "length register" cannot be autodetected, and it is now
> > specified with a DT property. Add DOC for it.
> >
> > Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> > b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> > index a2b8bfaec43c..acecdc5d8d47 100644
> > --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> > +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> > @@ -36,6 +36,8 @@ Required properties:
> >
> >  Required properties for VDMA:
> >  - xlnx,num-fstores: Should be the number of framebuffers as configured in
> > h/w.
> > +Required properties for AXI DMA:
> > +- xlnx,lengthregwidth: Should be the width of the length register as
> > configured in h/w.
> 
> One suggestion to be inline with IP property naming we can rename
> this prop to "xlnx,sg-length-width"? Please take a look at Xilinx tree
> we have this feature added in the master branch. It would be good
> to consolidate both implementations and upstream. Let me know
> if there are any followup queries.

It should be ok to cherrypick 3/6 and 4/6 (xlnx,sg-length-width)
from Xilinx tree and include it in your v2 patch series. 

> 
> >
> >  Optional properties:
> >  - xlnx,include-sg: Tells configured for Scatter-mode in
> > --
> > 2.17.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe dmaengine" in
> > the body of a message to majordomo at vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/6] dmaengine: xilinx_dma: autodetect whether the HW supports scatter-gather

[Radhey Shyam Pandey]

> -----Original Message-----
> From: dmaengine-owner at vger.kernel.org [mailto:dmaengine-
> owner at vger.kernel.org] On Behalf Of Andrea Merello
> Sent: Wednesday, June 20, 2018 2:07 PM
> To: vkoul at kernel.org; dan.j.williams at intel.com; Michal Simek
> <michals@xilinx.com>; Appana Durga Kedareswara Rao
> <appanad@xilinx.com>; dmaengine at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
> Andrea Merello <andrea.merello@gmail.com>
> Subject: [PATCH 5/6] dmaengine: xilinx_dma: autodetect whether the HW
> supports scatter-gather
> 
> The HW can be either direct-access or scatter-gather version. These are
> SW incompatible.
> 
> The driver can handle both version: a DT property was used to
> tell the driver whether to assume the HW is is scatter-gather mode.
> 
> This patch makes the driver to autodetect this information. The DT
> property is not required anymore.
> 
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> ---
>  drivers/dma/xilinx/xilinx_dma.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index bdbc8ba9092a..8c6e818e596f 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -86,6 +86,7 @@
>  #define XILINX_DMA_DMASR_DMA_DEC_ERR		BIT(6)
>  #define XILINX_DMA_DMASR_DMA_SLAVE_ERR		BIT(5)
>  #define XILINX_DMA_DMASR_DMA_INT_ERR		BIT(4)
> +#define XILINX_DMA_DMASR_SG_MASK		BIT(3)
>  #define XILINX_DMA_DMASR_IDLE			BIT(1)
>  #define XILINX_DMA_DMASR_HALTED		BIT(0)
>  #define XILINX_DMA_DMASR_DELAY_MASK		GENMASK(31, 24)
> @@ -407,7 +408,6 @@ struct xilinx_dma_config {
>   * @dev: Device Structure
>   * @common: DMA device structure
>   * @chan: Driver specific DMA channel
> - * @has_sg: Specifies whether Scatter-Gather is present or not
>   * @mcdma: Specifies whether Multi-Channel is present or not
>   * @flush_on_fsync: Flush on frame sync
>   * @ext_addr: Indicates 64 bit addressing is supported by dma device
> @@ -426,7 +426,6 @@ struct xilinx_dma_device {
>  	struct device *dev;
>  	struct dma_device common;
>  	struct xilinx_dma_chan
> *chan[XILINX_DMA_MAX_CHANS_PER_DEVICE];
> -	bool has_sg;
>  	bool mcdma;
>  	u32 flush_on_fsync;
>  	bool ext_addr;
> @@ -2391,7 +2390,6 @@ static int xilinx_dma_chan_probe(struct
> xilinx_dma_device *xdev,
> 
>  	chan->dev = xdev->dev;
>  	chan->xdev = xdev;
> -	chan->has_sg = xdev->has_sg;
>  	chan->desc_pendingcount = 0x0;
>  	chan->ext_addr = xdev->ext_addr;
>  	/* This variable ensures that descriptors are not
> @@ -2488,6 +2486,13 @@ static int xilinx_dma_chan_probe(struct
> xilinx_dma_device *xdev,
>  		chan->stop_transfer = xilinx_dma_stop_transfer;
>  	}
> 
> +	/* check if SG is enabled */
> +	if (dma_ctrl_read(chan, XILINX_DMA_REG_DMASR) &
> +		XILINX_DMA_DMASR_SG_MASK)
I think SGIncld mask is only applicable for AXI DMA and CDMA IP.
For VDMA IP this bit is reserved. 
.
> +		chan->has_sg = true;
> +	dev_dbg(chan->dev, "ch %d: SG %s\n", chan->id,
> +		chan->has_sg ? "enabled" : "disabled");
> +
>  	/* Initialize the tasklet */
>  	tasklet_init(&chan->tasklet, xilinx_dma_do_tasklet,
>  			(unsigned long)chan);
> @@ -2626,7 +2631,6 @@ static int xilinx_dma_probe(struct platform_device
> *pdev)
>  		return PTR_ERR(xdev->regs);
> 
>  	/* Retrieve the DMA engine properties from the device tree */
> -
Unrelated change

>  	if (xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
>  		xdev->mcdma = of_property_read_bool(node, "xlnx,mcdma");
>  		err = of_property_read_u32(node, "xlnx,lengthregwidth",
> --
> 2.17.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/6] dmaengine: xilinx_dma: autodetect whether the HW supports scatter-gather

[Andrea Merello]

On Wed, Jun 20, 2018 at 4:43 PM, Radhey Shyam Pandey <radheys@xilinx.com> wrote:
>> -----Original Message-----
>> From: dmaengine-owner at vger.kernel.org [mailto:dmaengine-
>> owner at vger.kernel.org] On Behalf Of Andrea Merello
>> Sent: Wednesday, June 20, 2018 2:07 PM
>> To: vkoul at kernel.org; dan.j.williams at intel.com; Michal Simek
>> <michals@xilinx.com>; Appana Durga Kedareswara Rao
>> <appanad@xilinx.com>; dmaengine at vger.kernel.org
>> Cc: linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
>> Andrea Merello <andrea.merello@gmail.com>
>> Subject: [PATCH 5/6] dmaengine: xilinx_dma: autodetect whether the HW
>> supports scatter-gather
>>
>> The HW can be either direct-access or scatter-gather version. These are
>> SW incompatible.
>>
>> The driver can handle both version: a DT property was used to
>> tell the driver whether to assume the HW is is scatter-gather mode.
>>
>> This patch makes the driver to autodetect this information. The DT
>> property is not required anymore.
>>
>> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
>> ---
>>  drivers/dma/xilinx/xilinx_dma.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
>> index bdbc8ba9092a..8c6e818e596f 100644
>> --- a/drivers/dma/xilinx/xilinx_dma.c
>> +++ b/drivers/dma/xilinx/xilinx_dma.c
>> @@ -86,6 +86,7 @@
>>  #define XILINX_DMA_DMASR_DMA_DEC_ERR         BIT(6)
>>  #define XILINX_DMA_DMASR_DMA_SLAVE_ERR               BIT(5)
>>  #define XILINX_DMA_DMASR_DMA_INT_ERR         BIT(4)
>> +#define XILINX_DMA_DMASR_SG_MASK             BIT(3)
>>  #define XILINX_DMA_DMASR_IDLE                        BIT(1)
>>  #define XILINX_DMA_DMASR_HALTED              BIT(0)
>>  #define XILINX_DMA_DMASR_DELAY_MASK          GENMASK(31, 24)
>> @@ -407,7 +408,6 @@ struct xilinx_dma_config {
>>   * @dev: Device Structure
>>   * @common: DMA device structure
>>   * @chan: Driver specific DMA channel
>> - * @has_sg: Specifies whether Scatter-Gather is present or not
>>   * @mcdma: Specifies whether Multi-Channel is present or not
>>   * @flush_on_fsync: Flush on frame sync
>>   * @ext_addr: Indicates 64 bit addressing is supported by dma device
>> @@ -426,7 +426,6 @@ struct xilinx_dma_device {
>>       struct device *dev;
>>       struct dma_device common;
>>       struct xilinx_dma_chan
>> *chan[XILINX_DMA_MAX_CHANS_PER_DEVICE];
>> -     bool has_sg;
>>       bool mcdma;
>>       u32 flush_on_fsync;
>>       bool ext_addr;
>> @@ -2391,7 +2390,6 @@ static int xilinx_dma_chan_probe(struct
>> xilinx_dma_device *xdev,
>>
>>       chan->dev = xdev->dev;
>>       chan->xdev = xdev;
>> -     chan->has_sg = xdev->has_sg;
>>       chan->desc_pendingcount = 0x0;
>>       chan->ext_addr = xdev->ext_addr;
>>       /* This variable ensures that descriptors are not
>> @@ -2488,6 +2486,13 @@ static int xilinx_dma_chan_probe(struct
>> xilinx_dma_device *xdev,
>>               chan->stop_transfer = xilinx_dma_stop_transfer;
>>       }
>>
>> +     /* check if SG is enabled */
>> +     if (dma_ctrl_read(chan, XILINX_DMA_REG_DMASR) &
>> +             XILINX_DMA_DMASR_SG_MASK)
> I think SGIncld mask is only applicable for AXI DMA and CDMA IP.
> For VDMA IP this bit is reserved.

OK. I can make it conditional wrt the IP type. As far as I can see
VDMA IP has not the two (SG vs no-SG) variant at all, so all should be
still OK.

> .
>> +             chan->has_sg = true;
>> +     dev_dbg(chan->dev, "ch %d: SG %s\n", chan->id,
>> +             chan->has_sg ? "enabled" : "disabled");
>> +
>>       /* Initialize the tasklet */
>>       tasklet_init(&chan->tasklet, xilinx_dma_do_tasklet,
>>                       (unsigned long)chan);
>> @@ -2626,7 +2631,6 @@ static int xilinx_dma_probe(struct platform_device
>> *pdev)
>>               return PTR_ERR(xdev->regs);
>>
>>       /* Retrieve the DMA engine properties from the device tree */
>> -
> Unrelated change

Oops.. Sorry.

>
>>       if (xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
>>               xdev->mcdma = of_property_read_bool(node, "xlnx,mcdma");
>>               err = of_property_read_u32(node, "xlnx,lengthregwidth",
>> --
>> 2.17.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/6] dmaengine: xilinx_dma: fix completion callback is not invoked for each DMA operation

[Radhey Shyam Pandey]

> -----Original Message-----
> From: Andrea Merello [mailto:andrea.merello at gmail.com]
> Sent: Wednesday, June 20, 2018 7:02 PM
> To: Radhey Shyam Pandey <radheys@xilinx.com>
> Cc: vkoul at kernel.org; dan.j.williams at intel.com; Michal Simek
> <michals@xilinx.com>; Appana Durga Kedareswara Rao
> <appanad@xilinx.com>; dmaengine at vger.kernel.org; linux-arm-
> kernel at lists.infradead.org; linux-kernel at vger.kernel.org
> Subject: Re: [PATCH 2/6] dmaengine: xilinx_dma: fix completion callback is not
> invoked for each DMA operation
> 
> On Wed, Jun 20, 2018 at 2:36 PM, Radhey Shyam Pandey
> <radheys@xilinx.com> wrote:
> >> -----Original Message-----
> >> From: dmaengine-owner at vger.kernel.org [mailto:dmaengine-
> >> owner at vger.kernel.org] On Behalf Of Andrea Merello
> >> Sent: Wednesday, June 20, 2018 2:07 PM
> >> To: vkoul at kernel.org; dan.j.williams at intel.com; Michal Simek
> >> <michals@xilinx.com>; Appana Durga Kedareswara Rao
> >> <appanad@xilinx.com>; dmaengine at vger.kernel.org
> >> Cc: linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
> >> Andrea Merello <andrea.merello@gmail.com>
> >> Subject: [PATCH 2/6] dmaengine: xilinx_dma: fix completion callback is not
> >> invoked for each DMA operation
> >>
> >> API specification says: "On completion of each DMA operation, the next in
> >> queue is started and a tasklet triggered. The tasklet will then call the
> >> client driver completion callback routine for notification, if set."
> >>
> >> Currently the driver keeps a "desc_pendingcount" counter of the total
> >> descriptor pending, and it uses as IRQ coalesce threshold, as result it
> >> only calls the CBs after ALL pending operations are completed, which is
> >> wrong.
> > I think IRQ coalescing enable/disable should be configurable.
> > Performance related usecases will need this support.
> 
> I didn't intend this (only) wrt performances; my concern was mostly
> wrt correctness. If my point of view is wrong then I'll drop this
> patch from the series.
> 
If coalescing is enabled driver will know DMA completion when
all packet counts are processed. So I think it's correct implementation.
Making interrupt coalescing configurable will address all usecases.

> (.. I might respin it again in future: I had a patch wrt an old driver
> version that allowed submitting new descriptors to the HW while the
> DMA is running, and in this case disabling coalesce is needed i.e. in
> order to submit a new empty buffer whenever the DMA finishes a
> transfer without waiting the DMA to stop).
> 
> BTW, is there any dmaengine API suitable for setting interrupt coalesce?
> 
> >>
> >> This patch uses disable IRQ coalesce and checks for the completion flag
> >> for the descriptors (which is further divided in segments).
> >>
> >> Possibly a better optimization could be using proper IRQ coalesce
> >> threshold to get an IRQ after all segments of the descriptors are done.
> >> But we don't do that yet..
> >>
> >> NOTE: for now we do this only for AXI DMA, other DMA flavors are
> >> untested/untouched.
> >> This is loosely based on
> >> commit 65df81a6dc74 ("xilinx_dma: IrqThreshold set incorrectly,
> unreliable.")
> >> in my linux-4.6-zynq tree
> > NOTE description doesn't help much.
> >
> >>
> >> From: Jeremy Trimble [original patch]
> >> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> >> ---
> >>  drivers/dma/xilinx/xilinx_dma.c | 39 +++++++++++++++++++++------------
> >>  1 file changed, 25 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/drivers/dma/xilinx/xilinx_dma.c
> b/drivers/dma/xilinx/xilinx_dma.c
> >> index a516e7ffef21..cf12f7147f07 100644
> >> --- a/drivers/dma/xilinx/xilinx_dma.c
> >> +++ b/drivers/dma/xilinx/xilinx_dma.c
> >> @@ -164,6 +164,7 @@
> >>  #define XILINX_DMA_CR_COALESCE_SHIFT 16
> >>  #define XILINX_DMA_BD_SOP            BIT(27)
> >>  #define XILINX_DMA_BD_EOP            BIT(26)
> >> +#define XILINX_DMA_BD_CMPLT          BIT(31)
> >>  #define XILINX_DMA_COALESCE_MAX              255
> >>  #define XILINX_DMA_NUM_DESCS         255
> >>  #define XILINX_DMA_NUM_APP_WORDS     5
> >> @@ -1274,12 +1275,9 @@ static void xilinx_dma_start_transfer(struct
> >> xilinx_dma_chan *chan)
> >>
> >>       reg = dma_ctrl_read(chan, XILINX_DMA_REG_DMACR);
> >>
> >> -     if (chan->desc_pendingcount <= XILINX_DMA_COALESCE_MAX) {
> >> -             reg &= ~XILINX_DMA_CR_COALESCE_MAX;
> >> -             reg |= chan->desc_pendingcount <<
> >> -                               XILINX_DMA_CR_COALESCE_SHIFT;
> >> -             dma_ctrl_write(chan, XILINX_DMA_REG_DMACR, reg);
> >> -     }
> >> +     reg &= ~XILINX_DMA_CR_COALESCE_MAX;
> >> +     reg |= 1 << XILINX_DMA_CR_COALESCE_SHIFT;
> >> +     dma_ctrl_write(chan, XILINX_DMA_REG_DMACR, reg);
> >>
> >>       if (chan->has_sg && !chan->xdev->mcdma)
> >>               xilinx_write(chan, XILINX_DMA_REG_CURDESC,
> >> @@ -1378,6 +1376,20 @@ static void
> xilinx_dma_complete_descriptor(struct
> >> xilinx_dma_chan *chan)
> >>               return;
> >>
> >>       list_for_each_entry_safe(desc, next, &chan->active_list, node) {
> >> +             if (chan->xdev->dma_config->dmatype ==
> >> XDMA_TYPE_AXIDMA) {
> >> +                     /*
> >> +                      * Check whether the last segment in this descriptor
> >> +                      * has been completed.
> >> +                      */
> >> +                     const struct xilinx_axidma_tx_segment *const tail_seg
> >> =
> >> +                             list_last_entry(&desc->segments,
> >> +                                             struct
> >> xilinx_axidma_tx_segment,
> >> +                                             node);
> >> +
> >> +                     /* we've processed all the completed descriptors */
> >> +                     if (!(tail_seg->hw.status & XILINX_DMA_BD_CMPLT))
> >> +                             break;
> >> +             }
> >>               list_del(&desc->node);
> >>               if (!desc->cyclic)
> >>                       dma_cookie_complete(&desc->async_tx);
> >> @@ -1826,14 +1838,13 @@ static struct dma_async_tx_descriptor
> >> *xilinx_dma_prep_slave_sg(
> >>                                  struct xilinx_axidma_tx_segment, node);
> >>       desc->async_tx.phys = segment->phys;
> >>
> >> -     /* For the last DMA_MEM_TO_DEV transfer, set EOP */
> >> -     if (chan->direction == DMA_MEM_TO_DEV) {
> >> -             segment->hw.control |= XILINX_DMA_BD_SOP;
> >> -             segment = list_last_entry(&desc->segments,
> >> -                                       struct xilinx_axidma_tx_segment,
> >> -                                       node);
> >> -             segment->hw.control |= XILINX_DMA_BD_EOP;
> >> -     }
> >> +     /* For the first transfer, set SOP */
> >> +     segment->hw.control |= XILINX_DMA_BD_SOP;
> >> +     /* For the last transfer, set EOP */
> >> +     segment = list_last_entry(&desc->segments,
> >> +                               struct xilinx_axidma_tx_segment,
> >> +                               node);
> >> +     segment->hw.control |= XILINX_DMA_BD_EOP;
> >>
> >>       return &desc->async_tx;
> >>
> >> --
> >> 2.17.1
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
> >> the body of a message to majordomo at vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html