[PATCH v3 1/5] dmaengine: xilinx_dma: in axidma slave_sg and dma_cylic mode align split descriptors

[PATCH v3 1/5] dmaengine: xilinx_dma: in axidma slave_sg and dma_cylic mode align split descriptors

[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 if DRE is not enabled.

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

Cc: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
---
Changes in v2:
        - don't introduce copy_mask field, rather rely on already-esistent
          copy_align field. Suggested by Radhey Shyam Pandey
        - reword title
Changes in v3:
	- fix bug introduced in v2: wrong copy size when DRE is enabled
	  use implementation suggested by Radhey Shyam Pandey
---
 drivers/dma/xilinx/xilinx_dma.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index 27b523530c4a..113d9bf1b6a1 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -1793,6 +1793,16 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_slave_sg(
 			 */
 			copy = min_t(size_t, sg_dma_len(sg) - sg_used,
 				     XILINX_DMA_MAX_TRANS_LEN);
+
+			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 */
@@ -1898,6 +1908,16 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_dma_cyclic(
 			 */
 			copy = min_t(size_t, period_len - sg_used,
 				     XILINX_DMA_MAX_TRANS_LEN);
+
+			if ((copy + sg_used < period_len) &&
+			    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;
 			xilinx_axidma_buf(chan, hw, buf_addr, sg_used,
 					  period_len * i);
-- 
2.17.1

[PATCH v3 2/5] dt-bindings: dmaengine: xilinx_dma: add optional xlnx,sg-length-width 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.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Cc: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
---
Changes in v2:
	- change property name
	- property is now optional
	- cc DT maintainer
Changes in v3:
	- reword
	- cc DT maintainerS and ML
---
 Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
index a2b8bfaec43c..c894abe28baa 100644
--- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
+++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
@@ -41,6 +41,7 @@ Optional properties:
 - xlnx,include-sg: Tells configured for Scatter-mode in
 	the hardware.
 Optional properties for AXI DMA:
+- xlnx,sg-length-width: Should be the width of the length register as configured in h/w.
 - xlnx,mcdma: Tells whether configured for multi-channel mode in the hardware.
 Optional properties for VDMA:
 - xlnx,flush-fsync: Tells which channel to Flush on Frame sync.
-- 
2.17.1

[PATCH v3 3/5] dmaengine: xilinx_dma: program hardware supported buffer length

[Andrea Merello]

From: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>

AXI-DMA IP supports configurable (c_sg_length_width) buffer length
register width, hence read buffer length (xlnx,sg-length-width) DT
property and ensure that driver doesn't program buffer length
exceeding the supported limit. For VDMA and CDMA there is no change.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Andrea Merello <andrea.merello@gmail.com> [rebase, reword]
---
Changes in v2:
        - drop original patch and replace with the one in Xilinx tree
Changes in v3:
	- cc DT maintainers/ML
---
 drivers/dma/xilinx/xilinx_dma.c | 35 ++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index 113d9bf1b6a1..7f0ab904b749 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -158,7 +158,8 @@
 #define XILINX_DMA_REG_BTT		0x28
 
 /* AXI DMA Specific Masks/Bit fields */
-#define XILINX_DMA_MAX_TRANS_LEN	GENMASK(22, 0)
+#define XILINX_DMA_MAX_TRANS_LEN_MIN	8
+#define XILINX_DMA_MAX_TRANS_LEN_MAX	23
 #define XILINX_DMA_CR_COALESCE_MAX	GENMASK(23, 16)
 #define XILINX_DMA_CR_CYCLIC_BD_EN_MASK	BIT(4)
 #define XILINX_DMA_CR_COALESCE_SHIFT	16
@@ -418,6 +419,7 @@ struct xilinx_dma_config {
  * @rxs_clk: DMA s2mm stream clock
  * @nr_channels: Number of channels DMA device supports
  * @chan_id: DMA channel identifier
+ * @max_buffer_len: Max buffer length
  */
 struct xilinx_dma_device {
 	void __iomem *regs;
@@ -437,6 +439,7 @@ struct xilinx_dma_device {
 	struct clk *rxs_clk;
 	u32 nr_channels;
 	u32 chan_id;
+	u32 max_buffer_len;
 };
 
 /* Macros */
@@ -985,7 +988,7 @@ static enum dma_status xilinx_dma_tx_status(struct dma_chan *dchan,
 			list_for_each_entry(segment, &desc->segments, node) {
 				hw = &segment->hw;
 				residue += (hw->control - hw->status) &
-					   XILINX_DMA_MAX_TRANS_LEN;
+					   chan->xdev->max_buffer_len;
 			}
 		}
 		spin_unlock_irqrestore(&chan->lock, flags);
@@ -1237,7 +1240,7 @@ static void xilinx_cdma_start_transfer(struct xilinx_dma_chan *chan)
 
 		/* Start the transfer */
 		dma_ctrl_write(chan, XILINX_DMA_REG_BTT,
-				hw->control & XILINX_DMA_MAX_TRANS_LEN);
+				hw->control & chan->xdev->max_buffer_len);
 	}
 
 	list_splice_tail_init(&chan->pending_list, &chan->active_list);
@@ -1340,7 +1343,7 @@ static void xilinx_dma_start_transfer(struct xilinx_dma_chan *chan)
 
 		/* Start the transfer */
 		dma_ctrl_write(chan, XILINX_DMA_REG_BTT,
-			       hw->control & XILINX_DMA_MAX_TRANS_LEN);
+			       hw->control & chan->xdev->max_buffer_len);
 	}
 
 	list_splice_tail_init(&chan->pending_list, &chan->active_list);
@@ -1701,7 +1704,7 @@ xilinx_cdma_prep_memcpy(struct dma_chan *dchan, dma_addr_t dma_dst,
 	struct xilinx_cdma_tx_segment *segment;
 	struct xilinx_cdma_desc_hw *hw;
 
-	if (!len || len > XILINX_DMA_MAX_TRANS_LEN)
+	if (!len || len > chan->xdev->max_buffer_len)
 		return NULL;
 
 	desc = xilinx_dma_alloc_tx_descriptor(chan);
@@ -1792,7 +1795,7 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_slave_sg(
 			 * making sure it is less than the hw limit
 			 */
 			copy = min_t(size_t, sg_dma_len(sg) - sg_used,
-				     XILINX_DMA_MAX_TRANS_LEN);
+				     chan->xdev->max_buffer_len);
 
 			if ((copy + sg_used < sg_dma_len(sg)) &&
 			    chan->xdev->common.copy_align) {
@@ -1907,7 +1910,7 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_dma_cyclic(
 			 * making sure it is less than the hw limit
 			 */
 			copy = min_t(size_t, period_len - sg_used,
-				     XILINX_DMA_MAX_TRANS_LEN);
+				     chan->xdev->max_buffer_len);
 
 			if ((copy + sg_used < period_len) &&
 			    chan->xdev->common.copy_align) {
@@ -2590,7 +2593,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, len_width;
 	int i, err;
 
 	/* Allocate and initialize the DMA engine structure */
@@ -2622,8 +2625,22 @@ static int xilinx_dma_probe(struct platform_device *pdev)
 
 	/* 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)
+	xdev->max_buffer_len = GENMASK(XILINX_DMA_MAX_TRANS_LEN_MAX - 1, 0);
+
+	if (xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
 		xdev->mcdma = of_property_read_bool(node, "xlnx,mcdma");
+		if (!of_property_read_u32(node, "xlnx,sg-length-width",
+					  &len_width)) {
+			if (len_width < XILINX_DMA_MAX_TRANS_LEN_MIN ||
+			    len_width > XILINX_DMA_MAX_TRANS_LEN_MAX) {
+				dev_warn(xdev->dev,
+					 "invalid xlnx,sg-length-width property value using default width\n");
+			} else {
+				xdev->max_buffer_len = GENMASK(len_width - 1,
+							       0);
+			}
+		}
+	}
 
 	if (xdev->dma_config->dmatype == XDMA_TYPE_VDMA) {
 		err = of_property_read_u32(node, "xlnx,num-fstores",
-- 
2.17.1

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

[Andrea Merello]

The AXIDMA and CDMA 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.

No changes for VDMA.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Cc: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
---
Changes in v2:
	- autodetect only in !VDMA case
Changes in v3:
	- cc DT maintainers/ML
---
 drivers/dma/xilinx/xilinx_dma.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index 7f0ab904b749..43fcc71ff287 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)
@@ -406,7 +407,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;
@@ -2393,7 +2392,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
@@ -2486,6 +2484,15 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device *xdev,
 		chan->stop_transfer = xilinx_dma_stop_transfer;
 	}
 
+	/* check if SG is enabled (only for AXIDMA and CDMA) */
+	if (xdev->dma_config->dmatype != XDMA_TYPE_VDMA) {
+		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);
@@ -2624,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 */
-	xdev->has_sg = of_property_read_bool(node, "xlnx,include-sg");
 	xdev->max_buffer_len = GENMASK(XILINX_DMA_MAX_TRANS_LEN_MAX - 1, 0);
 
 	if (xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
-- 
2.17.1

[PATCH v3 5/5] dt-bindings: dmaengine: xilinx_dma: drop has-sg property

[Andrea Merello]

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

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Cc: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
---
Changes in v2:
	- cc DT maintainer
Changes in v3:
	- cc DT maintainerS/ML
---
 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 c894abe28baa..09a41843cbc0 100644
--- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
+++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
@@ -37,9 +37,6 @@ Required properties:
 Required properties for VDMA:
 - xlnx,num-fstores: Should be the number of framebuffers as configured in h/w.
 
-Optional properties:
-- xlnx,include-sg: Tells configured for Scatter-mode in
-	the hardware.
 Optional properties for AXI DMA:
 - xlnx,sg-length-width: Should be the width of the length register as configured in h/w.
 - xlnx,mcdma: Tells whether configured for multi-channel mode in the hardware.
-- 
2.17.1

Re: [PATCH v3 2/5] dt-bindings: dmaengine: xilinx_dma: add optional xlnx,sg-length-width property

[Rob Herring]

On Mon, Jun 25, 2018 at 11:27:21AM +0200, Andrea Merello wrote:
> The width of the "length register" cannot be autodetected, and it is now
> specified with a DT property. Add DOC for it.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: devicetree@vger.kernel.org
> Cc: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> ---
> Changes in v2:
> 	- change property name
> 	- property is now optional
> 	- cc DT maintainer
> Changes in v3:
> 	- reword
> 	- cc DT maintainerS and ML
> ---
>  Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> index a2b8bfaec43c..c894abe28baa 100644
> --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> @@ -41,6 +41,7 @@ Optional properties:
>  - xlnx,include-sg: Tells configured for Scatter-mode in
>  	the hardware.
>  Optional properties for AXI DMA:
> +- xlnx,sg-length-width: Should be the width of the length register as configured in h/w.

What are the units? What are valid values? What is the default if the 
property is not present?

>  - xlnx,mcdma: Tells whether configured for multi-channel mode in the hardware.
>  Optional properties for VDMA:
>  - xlnx,flush-fsync: Tells which channel to Flush on Frame sync.
> -- 
> 2.17.1
> 

Re: [PATCH v3 5/5] dt-bindings: dmaengine: xilinx_dma: drop has-sg property

[Rob Herring]

On Mon, Jun 25, 2018 at 11:27:24AM +0200, Andrea Merello wrote:
> This property is not needed anymore, because the driver now autodetects it.
> Delete references in documentation.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: devicetree@vger.kernel.org
> Cc: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> ---
> Changes in v2:
> 	- cc DT maintainer
> Changes in v3:
> 	- cc DT maintainerS/ML
> ---
>  Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt | 3 ---
>  1 file changed, 3 deletions(-)

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 1/5] dmaengine: xilinx_dma: in axidma slave_sg and dma_cylic mode align split descriptors

[Vinod]

On 25-06-18, 11:27, Andrea Merello wrote:
> 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 if DRE is not enabled.
> 
> This patch eventually adjusts the transfer size in order to make sure
> all operations start from an aligned address.
> 
> Cc: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> ---
> Changes in v2:
>         - don't introduce copy_mask field, rather rely on already-esistent
>           copy_align field. Suggested by Radhey Shyam Pandey
>         - reword title
> Changes in v3:
> 	- fix bug introduced in v2: wrong copy size when DRE is enabled
> 	  use implementation suggested by Radhey Shyam Pandey
> ---
>  drivers/dma/xilinx/xilinx_dma.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 27b523530c4a..113d9bf1b6a1 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -1793,6 +1793,16 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_slave_sg(
>  			 */
>  			copy = min_t(size_t, sg_dma_len(sg) - sg_used,
>  				     XILINX_DMA_MAX_TRANS_LEN);
> +
> +			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 */
> @@ -1898,6 +1908,16 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_dma_cyclic(
>  			 */
>  			copy = min_t(size_t, period_len - sg_used,
>  				     XILINX_DMA_MAX_TRANS_LEN);
> +
> +			if ((copy + sg_used < period_len) &&
> +			    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));
> +			}

same code pasted twice, can we have a routine for this... perhaps more
code can be made common too

-- 
~Vinod

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

[Vinod]

On 25-06-18, 11:27, Andrea Merello wrote:
> The AXIDMA and CDMA 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
                                ^^^^
versions

> tell the driver whether to assume the HW is is scatter-gather mode.
                                           ^^^^^
is in?

> 
> This patch makes the driver to autodetect this information. The DT
> property is not required anymore.
> 
> No changes for VDMA.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: devicetree@vger.kernel.org
> Cc: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> ---
> Changes in v2:
> 	- autodetect only in !VDMA case
> Changes in v3:
> 	- cc DT maintainers/ML
> ---
>  drivers/dma/xilinx/xilinx_dma.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 7f0ab904b749..43fcc71ff287 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)
> @@ -406,7 +407,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;
> @@ -2393,7 +2392,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
> @@ -2486,6 +2484,15 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device *xdev,
>  		chan->stop_transfer = xilinx_dma_stop_transfer;
>  	}
>  
> +	/* check if SG is enabled (only for AXIDMA and CDMA) */
> +	if (xdev->dma_config->dmatype != XDMA_TYPE_VDMA) {
> +		if (dma_ctrl_read(chan, XILINX_DMA_REG_DMASR) &
> +		    XILINX_DMA_DMASR_SG_MASK)

why not read this for VDMA too, will it return false?

> +			chan->has_sg = true;
> +		dev_dbg(chan->dev, "ch %d: SG %s\n", chan->id,
> +			chan->has_sg ? "enabled" : "disabled");

this debug print can be removed

-- 
~Vinod

Re: [PATCH v3 1/5] dmaengine: xilinx_dma: in axidma slave_sg and dma_cylic mode align split descriptors

[Andrea Merello]

On Fri, Jun 29, 2018 at 9:25 AM, Vinod <vkoul@kernel.org> wrote:
> On 25-06-18, 11:27, Andrea Merello wrote:
>> 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 if DRE is not enabled.
>>
>> This patch eventually adjusts the transfer size in order to make sure
>> all operations start from an aligned address.
>>
>> Cc: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
>> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
>> Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
>> ---
>> Changes in v2:
>>         - don't introduce copy_mask field, rather rely on already-esistent
>>           copy_align field. Suggested by Radhey Shyam Pandey
>>         - reword title
>> Changes in v3:
>>       - fix bug introduced in v2: wrong copy size when DRE is enabled
>>         use implementation suggested by Radhey Shyam Pandey
>> ---
>>  drivers/dma/xilinx/xilinx_dma.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
>> index 27b523530c4a..113d9bf1b6a1 100644
>> --- a/drivers/dma/xilinx/xilinx_dma.c
>> +++ b/drivers/dma/xilinx/xilinx_dma.c
>> @@ -1793,6 +1793,16 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_slave_sg(
>>                        */
>>                       copy = min_t(size_t, sg_dma_len(sg) - sg_used,
>>                                    XILINX_DMA_MAX_TRANS_LEN);
>> +
>> +                     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 */
>> @@ -1898,6 +1908,16 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_dma_cyclic(
>>                        */
>>                       copy = min_t(size_t, period_len - sg_used,
>>                                    XILINX_DMA_MAX_TRANS_LEN);
>> +
>> +                     if ((copy + sg_used < period_len) &&
>> +                         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));
>> +                     }
>
> same code pasted twice, can we have a routine for this... perhaps more
> code can be made common too

Yes, I see.. Indeed there was duplicated code before this series and
it is still there after it.

I can see if we can have a routine as you suggested at least for the
code portions touched by this patch. Do you eventually want this extra
change to be done in the same patch 1/5 or do you want a separate
patch i.e. 2/6 or 6/6 ?

> --
> ~Vinod

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

[Andrea Merello]

On Fri, Jun 29, 2018 at 9:37 AM, Vinod <vkoul@kernel.org> wrote:
> On 25-06-18, 11:27, Andrea Merello wrote:
>> The AXIDMA and CDMA 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
>                                 ^^^^
> versions
>

OK

>> tell the driver whether to assume the HW is is scatter-gather mode.
>                                            ^^^^^
> is in?

Yes, it is. Thanks

>>
>> This patch makes the driver to autodetect this information. The DT
>> property is not required anymore.
>>
>> No changes for VDMA.
>>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: devicetree@vger.kernel.org
>> Cc: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
>> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
>> Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
>> ---
>> Changes in v2:
>>       - autodetect only in !VDMA case
>> Changes in v3:
>>       - cc DT maintainers/ML
>> ---
>>  drivers/dma/xilinx/xilinx_dma.c | 14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
>> index 7f0ab904b749..43fcc71ff287 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)
>> @@ -406,7 +407,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;
>> @@ -2393,7 +2392,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
>> @@ -2486,6 +2484,15 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device *xdev,
>>               chan->stop_transfer = xilinx_dma_stop_transfer;
>>       }
>>
>> +     /* check if SG is enabled (only for AXIDMA and CDMA) */
>> +     if (xdev->dma_config->dmatype != XDMA_TYPE_VDMA) {
>> +             if (dma_ctrl_read(chan, XILINX_DMA_REG_DMASR) &
>> +                 XILINX_DMA_DMASR_SG_MASK)
>
> why not read this for VDMA too, will it return false?

AFAIK this bit is reserved in VDMA, so reading it can lead to any
random thing.. I would say that potentially it could even be used for
other purposes in future IP releases..

>> +                     chan->has_sg = true;
>> +             dev_dbg(chan->dev, "ch %d: SG %s\n", chan->id,
>> +                     chan->has_sg ? "enabled" : "disabled");
>
> this debug print can be removed

IMHO if someone will ever enable debug prints for debugging something
and then he/she reports us a log, then it would be useful to see in
the log if the IP was SG or not..

> --
> ~Vinod

Re: [PATCH v3 1/5] dmaengine: xilinx_dma: in axidma slave_sg and dma_cylic mode align split descriptors

[Vinod]

On 29-06-18, 09:46, Andrea Merello wrote:
> On Fri, Jun 29, 2018 at 9:25 AM, Vinod <vkoul@kernel.org> wrote:

> >> +
> >> +                     if ((copy + sg_used < period_len) &&
> >> +                         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));
> >> +                     }
> >
> > same code pasted twice, can we have a routine for this... perhaps more
> > code can be made common too
> 
> Yes, I see.. Indeed there was duplicated code before this series and
> it is still there after it.
> 
> I can see if we can have a routine as you suggested at least for the
> code portions touched by this patch. Do you eventually want this extra
> change to be done in the same patch 1/5 or do you want a separate
> patch i.e. 2/6 or 6/6 ?

Each patch should do one thing, so would make sense to move first and
then add you on top of that. 1/6 commonize and 2/6 add this bit.


-- 
~Vinod