[PATCH -next 0/8] AXI DMA driver improvements

[PATCH -next 0/8] AXI DMA driver improvements

[Radhey Shyam Pandey]

This patchset adds callback result, descriptor residue
calculation and some regression fixes. 

Nicholas Graumann (7):
  dmaengine: xilinx_dma: Merge get_callback and _invoke
  dmaengine: xilinx_dma: Introduce xilinx_dma_get_residue
  dmaengine: xilinx_dma: Add callback_result support
  dmaengine: xilinx_dma: Remove residue from channel data
  dmaengine: xilinx_dma: Print debug message when no free tx segments
  dmaengine: xilinx_dma: Check for both idle and halted state in axidma
    stop_transfer
  dmaengine: xilinx_dma: Clear desc_pendingcount in xilinx_dma_reset

Radhey Shyam Pandey (1):
  dmaengine: xilinx_dma: Remove desc_callback_valid check

 drivers/dma/xilinx/xilinx_dma.c | 115 +++++++++++++++++++++++++++++-----------
 1 file changed, 85 insertions(+), 30 deletions(-)

-- 
2.7.4

[PATCH -next 1/8] dmaengine: xilinx_dma: Remove desc_callback_valid check

[Radhey Shyam Pandey]

In descriptor cleanup the call to desc_callback_valid can be safely
removed as both callback pointers i.e callback_result and callback
are anyway checked in invoke(). There is no much benefit in having
redundant checks.

Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
Signed-off-by: Nicholas Graumann <nick.graumann@gmail.com>
Reviewed-by: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
---
 drivers/dma/xilinx/xilinx_dma.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index e7dc3c4..8bbf997 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -832,11 +832,9 @@ static void xilinx_dma_chan_desc_cleanup(struct xilinx_dma_chan *chan)
 
 		/* Run the link descriptor callback function */
 		dmaengine_desc_get_callback(&desc->async_tx, &cb);
-		if (dmaengine_desc_callback_valid(&cb)) {
-			spin_unlock_irqrestore(&chan->lock, flags);
-			dmaengine_desc_callback_invoke(&cb, NULL);
-			spin_lock_irqsave(&chan->lock, flags);
-		}
+		spin_unlock_irqrestore(&chan->lock, flags);
+		dmaengine_desc_callback_invoke(&cb, NULL);
+		spin_lock_irqsave(&chan->lock, flags);
 
 		/* Run any dependencies, then free the descriptor */
 		dma_run_dependencies(&desc->async_tx);
-- 
2.7.4

[PATCH -next 2/8] dmaengine: xilinx_dma: Merge get_callback and _invoke

[Radhey Shyam Pandey]

From: Nicholas Graumann <nick.graumann@gmail.com>

The dma api provides a single interface to get the appropriate callback
and invoke it directly. Prefer using it.

Signed-off-by: Nicholas Graumann <nick.graumann@gmail.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
---
 drivers/dma/xilinx/xilinx_dma.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index 8bbf997..9909bfb 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -820,8 +820,6 @@ static void xilinx_dma_chan_desc_cleanup(struct xilinx_dma_chan *chan)
 	spin_lock_irqsave(&chan->lock, flags);
 
 	list_for_each_entry_safe(desc, next, &chan->done_list, node) {
-		struct dmaengine_desc_callback cb;
-
 		if (desc->cyclic) {
 			xilinx_dma_chan_handle_cyclic(chan, desc, &flags);
 			break;
@@ -831,9 +829,8 @@ static void xilinx_dma_chan_desc_cleanup(struct xilinx_dma_chan *chan)
 		list_del(&desc->node);
 
 		/* Run the link descriptor callback function */
-		dmaengine_desc_get_callback(&desc->async_tx, &cb);
 		spin_unlock_irqrestore(&chan->lock, flags);
-		dmaengine_desc_callback_invoke(&cb, NULL);
+		dmaengine_desc_get_callback_invoke(&desc->async_tx, NULL);
 		spin_lock_irqsave(&chan->lock, flags);
 
 		/* Run any dependencies, then free the descriptor */
-- 
2.7.4

[PATCH -next 3/8] dmaengine: xilinx_dma: Introduce xilinx_dma_get_residue

[Radhey Shyam Pandey]

From: Nicholas Graumann <nick.graumann@gmail.com>

Introduce a function that can calculate residues for IPs that support it:
AXI DMA and CDMA.

Signed-off-by: Nicholas Graumann <nick.graumann@gmail.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
---
 drivers/dma/xilinx/xilinx_dma.c | 75 ++++++++++++++++++++++++++++++-----------
 1 file changed, 56 insertions(+), 19 deletions(-)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index 9909bfb..4094adb 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -787,6 +787,51 @@ static void xilinx_dma_free_chan_resources(struct dma_chan *dchan)
 }
 
 /**
+ * xilinx_dma_get_residue - Compute residue for a given descriptor
+ * @chan: Driver specific dma channel
+ * @desc: dma transaction descriptor
+ *
+ * Return: The number of residue bytes for the descriptor.
+ */
+static u32 xilinx_dma_get_residue(struct xilinx_dma_chan *chan,
+				  struct xilinx_dma_tx_descriptor *desc)
+{
+	struct xilinx_cdma_tx_segment *cdma_seg;
+	struct xilinx_axidma_tx_segment *axidma_seg;
+	struct xilinx_cdma_desc_hw *cdma_hw;
+	struct xilinx_axidma_desc_hw *axidma_hw;
+	struct list_head *entry;
+	u32 residue = 0;
+
+	/**
+	 * VDMA and simple mode do not support residue reporting, so the
+	 * residue field will always be 0.
+	 */
+	if (chan->xdev->dma_config->dmatype == XDMA_TYPE_VDMA || !chan->has_sg)
+		return residue;
+
+	list_for_each(entry, &desc->segments) {
+		if (chan->xdev->dma_config->dmatype == XDMA_TYPE_CDMA) {
+			cdma_seg = list_entry(entry,
+					      struct xilinx_cdma_tx_segment,
+					      node);
+			cdma_hw = &cdma_seg->hw;
+			residue += (cdma_hw->control - cdma_hw->status) &
+				   chan->xdev->max_buffer_len;
+		} else {
+			axidma_seg = list_entry(entry,
+						struct xilinx_axidma_tx_segment,
+						node);
+			axidma_hw = &axidma_seg->hw;
+			residue += (axidma_hw->control - axidma_hw->status) &
+				   chan->xdev->max_buffer_len;
+		}
+	}
+
+	return residue;
+}
+
+/**
  * xilinx_dma_chan_handle_cyclic - Cyclic dma callback
  * @chan: Driver specific dma channel
  * @desc: dma transaction descriptor
@@ -995,33 +1040,22 @@ static enum dma_status xilinx_dma_tx_status(struct dma_chan *dchan,
 {
 	struct xilinx_dma_chan *chan = to_xilinx_chan(dchan);
 	struct xilinx_dma_tx_descriptor *desc;
-	struct xilinx_axidma_tx_segment *segment;
-	struct xilinx_axidma_desc_hw *hw;
 	enum dma_status ret;
 	unsigned long flags;
-	u32 residue = 0;
 
 	ret = dma_cookie_status(dchan, cookie, txstate);
 	if (ret == DMA_COMPLETE || !txstate)
 		return ret;
 
-	if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
-		spin_lock_irqsave(&chan->lock, flags);
+	spin_lock_irqsave(&chan->lock, flags);
 
-		desc = list_last_entry(&chan->active_list,
-				       struct xilinx_dma_tx_descriptor, node);
-		if (chan->has_sg) {
-			list_for_each_entry(segment, &desc->segments, node) {
-				hw = &segment->hw;
-				residue += (hw->control - hw->status) &
-					   chan->xdev->max_buffer_len;
-			}
-		}
-		spin_unlock_irqrestore(&chan->lock, flags);
+	desc = list_last_entry(&chan->active_list,
+			       struct xilinx_dma_tx_descriptor, node);
+	chan->residue = xilinx_dma_get_residue(chan, desc);
 
-		chan->residue = residue;
-		dma_set_residue(txstate, chan->residue);
-	}
+	spin_unlock_irqrestore(&chan->lock, flags);
+
+	dma_set_residue(txstate, chan->residue);
 
 	return ret;
 }
@@ -2701,12 +2735,15 @@ static int xilinx_dma_probe(struct platform_device *pdev)
 					  xilinx_dma_prep_dma_cyclic;
 		xdev->common.device_prep_interleaved_dma =
 					xilinx_dma_prep_interleaved;
-		/* Residue calculation is supported by only AXI DMA */
+		/* Residue calculation is supported by only AXI DMA and CDMA */
 		xdev->common.residue_granularity =
 					  DMA_RESIDUE_GRANULARITY_SEGMENT;
 	} else if (xdev->dma_config->dmatype == XDMA_TYPE_CDMA) {
 		dma_cap_set(DMA_MEMCPY, xdev->common.cap_mask);
 		xdev->common.device_prep_dma_memcpy = xilinx_cdma_prep_memcpy;
+		/* Residue calculation is supported by only AXI DMA and CDMA */
+		xdev->common.residue_granularity =
+					DMA_RESIDUE_GRANULARITY_SEGMENT;
 	} else {
 		xdev->common.device_prep_interleaved_dma =
 				xilinx_vdma_dma_prep_interleaved;
-- 
2.7.4

Re: [PATCH -next 3/8] dmaengine: xilinx_dma: Introduce xilinx_dma_get_residue

[Vinod Koul]

On 05-09-19, 22:06, Radhey Shyam Pandey wrote:
> From: Nicholas Graumann <nick.graumann@gmail.com>
> 
> Introduce a function that can calculate residues for IPs that support it:
> AXI DMA and CDMA.
> 
> Signed-off-by: Nicholas Graumann <nick.graumann@gmail.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> ---
>  drivers/dma/xilinx/xilinx_dma.c | 75 ++++++++++++++++++++++++++++++-----------
>  1 file changed, 56 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 9909bfb..4094adb 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -787,6 +787,51 @@ static void xilinx_dma_free_chan_resources(struct dma_chan *dchan)
>  }
>  
>  /**
> + * xilinx_dma_get_residue - Compute residue for a given descriptor
> + * @chan: Driver specific dma channel
> + * @desc: dma transaction descriptor
> + *
> + * Return: The number of residue bytes for the descriptor.
> + */
> +static u32 xilinx_dma_get_residue(struct xilinx_dma_chan *chan,
> +				  struct xilinx_dma_tx_descriptor *desc)
> +{
> +	struct xilinx_cdma_tx_segment *cdma_seg;
> +	struct xilinx_axidma_tx_segment *axidma_seg;
> +	struct xilinx_cdma_desc_hw *cdma_hw;
> +	struct xilinx_axidma_desc_hw *axidma_hw;
> +	struct list_head *entry;
> +	u32 residue = 0;
> +
> +	/**

it should be:
        /*
         * comment...

> +	 * VDMA and simple mode do not support residue reporting, so the
> +	 * residue field will always be 0.
> +	 */
> +	if (chan->xdev->dma_config->dmatype == XDMA_TYPE_VDMA || !chan->has_sg)
> +		return residue;

why not check this in status callback?

> +
> +	list_for_each(entry, &desc->segments) {
> +		if (chan->xdev->dma_config->dmatype == XDMA_TYPE_CDMA) {
> +			cdma_seg = list_entry(entry,
> +					      struct xilinx_cdma_tx_segment,
> +					      node);
> +			cdma_hw = &cdma_seg->hw;
> +			residue += (cdma_hw->control - cdma_hw->status) &
> +				   chan->xdev->max_buffer_len;
> +		} else {
> +			axidma_seg = list_entry(entry,
> +						struct xilinx_axidma_tx_segment,
> +						node);
> +			axidma_hw = &axidma_seg->hw;
> +			residue += (axidma_hw->control - axidma_hw->status) &
> +				   chan->xdev->max_buffer_len;
> +		}
> +	}
> +
> +	return residue;
> +}
> +
> +/**
>   * xilinx_dma_chan_handle_cyclic - Cyclic dma callback
>   * @chan: Driver specific dma channel
>   * @desc: dma transaction descriptor
> @@ -995,33 +1040,22 @@ static enum dma_status xilinx_dma_tx_status(struct dma_chan *dchan,
>  {
>  	struct xilinx_dma_chan *chan = to_xilinx_chan(dchan);
>  	struct xilinx_dma_tx_descriptor *desc;
> -	struct xilinx_axidma_tx_segment *segment;
> -	struct xilinx_axidma_desc_hw *hw;
>  	enum dma_status ret;
>  	unsigned long flags;
> -	u32 residue = 0;
>  
>  	ret = dma_cookie_status(dchan, cookie, txstate);
>  	if (ret == DMA_COMPLETE || !txstate)
>  		return ret;
>  
> -	if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
> -		spin_lock_irqsave(&chan->lock, flags);
> +	spin_lock_irqsave(&chan->lock, flags);
>  
> -		desc = list_last_entry(&chan->active_list,
> -				       struct xilinx_dma_tx_descriptor, node);
> -		if (chan->has_sg) {
> -			list_for_each_entry(segment, &desc->segments, node) {
> -				hw = &segment->hw;
> -				residue += (hw->control - hw->status) &
> -					   chan->xdev->max_buffer_len;
> -			}
> -		}
> -		spin_unlock_irqrestore(&chan->lock, flags);
> +	desc = list_last_entry(&chan->active_list,
> +			       struct xilinx_dma_tx_descriptor, node);
> +	chan->residue = xilinx_dma_get_residue(chan, desc);
>  
> -		chan->residue = residue;
> -		dma_set_residue(txstate, chan->residue);
> -	}
> +	spin_unlock_irqrestore(&chan->lock, flags);
> +
> +	dma_set_residue(txstate, chan->residue);
>  
>  	return ret;
>  }
> @@ -2701,12 +2735,15 @@ static int xilinx_dma_probe(struct platform_device *pdev)
>  					  xilinx_dma_prep_dma_cyclic;
>  		xdev->common.device_prep_interleaved_dma =
>  					xilinx_dma_prep_interleaved;
> -		/* Residue calculation is supported by only AXI DMA */
> +		/* Residue calculation is supported by only AXI DMA and CDMA */
>  		xdev->common.residue_granularity =
>  					  DMA_RESIDUE_GRANULARITY_SEGMENT;
>  	} else if (xdev->dma_config->dmatype == XDMA_TYPE_CDMA) {
>  		dma_cap_set(DMA_MEMCPY, xdev->common.cap_mask);
>  		xdev->common.device_prep_dma_memcpy = xilinx_cdma_prep_memcpy;
> +		/* Residue calculation is supported by only AXI DMA and CDMA */
> +		xdev->common.residue_granularity =
> +					DMA_RESIDUE_GRANULARITY_SEGMENT;
>  	} else {
>  		xdev->common.device_prep_interleaved_dma =
>  				xilinx_vdma_dma_prep_interleaved;
> -- 
> 2.7.4

-- 
~Vinod

RE: [PATCH -next 3/8] dmaengine: xilinx_dma: Introduce xilinx_dma_get_residue

[Radhey Shyam Pandey]

> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: Thursday, September 26, 2019 2:31 AM
> To: Radhey Shyam Pandey <radheys@xilinx.com>
> Cc: dan.j.williams@intel.com; Michal Simek <michals@xilinx.com>;
> nick.graumann@gmail.com; andrea.merello@gmail.com; Appana Durga
> Kedareswara Rao <appanad@xilinx.com>; mcgrof@kernel.org;
> dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH -next 3/8] dmaengine: xilinx_dma: Introduce
> xilinx_dma_get_residue
> 
> On 05-09-19, 22:06, Radhey Shyam Pandey wrote:
> > From: Nicholas Graumann <nick.graumann@gmail.com>
> >
> > Introduce a function that can calculate residues for IPs that support it:
> > AXI DMA and CDMA.
> >
> > Signed-off-by: Nicholas Graumann <nick.graumann@gmail.com>
> > Signed-off-by: Radhey Shyam Pandey
> <radhey.shyam.pandey@xilinx.com>
> > ---
> >  drivers/dma/xilinx/xilinx_dma.c | 75
> > ++++++++++++++++++++++++++++++-----------
> >  1 file changed, 56 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/dma/xilinx/xilinx_dma.c
> > b/drivers/dma/xilinx/xilinx_dma.c index 9909bfb..4094adb 100644
> > --- a/drivers/dma/xilinx/xilinx_dma.c
> > +++ b/drivers/dma/xilinx/xilinx_dma.c
> > @@ -787,6 +787,51 @@ static void xilinx_dma_free_chan_resources(struct
> > dma_chan *dchan)  }
> >
> >  /**
> > + * xilinx_dma_get_residue - Compute residue for a given descriptor
> > + * @chan: Driver specific dma channel
> > + * @desc: dma transaction descriptor
> > + *
> > + * Return: The number of residue bytes for the descriptor.
> > + */
> > +static u32 xilinx_dma_get_residue(struct xilinx_dma_chan *chan,
> > +				  struct xilinx_dma_tx_descriptor *desc) {
> > +	struct xilinx_cdma_tx_segment *cdma_seg;
> > +	struct xilinx_axidma_tx_segment *axidma_seg;
> > +	struct xilinx_cdma_desc_hw *cdma_hw;
> > +	struct xilinx_axidma_desc_hw *axidma_hw;
> > +	struct list_head *entry;
> > +	u32 residue = 0;
> > +
> > +	/**
> 
> it should be:
>         /*
>          * comment...
> 
Thanks for pointing it out. I will fix it in v2.

> > +	 * VDMA and simple mode do not support residue reporting, so the
> > +	 * residue field will always be 0.
> > +	 */
> > +	if (chan->xdev->dma_config->dmatype == XDMA_TYPE_VDMA ||
> !chan->has_sg)
> > +		return residue;
> 
> why not check this in status callback?
Assuming we mean to move vdma and non-sg check to xilinx_dma_tx_status.
Just a thought- Keeping this check in xilinx_dma_get_residue provides
an abstraction and caller can simply call this func with knowing about
IP config specific residue calculation. Considering this point does it
looks ok ? 
> 
> > +
> > +	list_for_each(entry, &desc->segments) {
> > +		if (chan->xdev->dma_config->dmatype ==
> XDMA_TYPE_CDMA) {
> > +			cdma_seg = list_entry(entry,
> > +					      struct xilinx_cdma_tx_segment,
> > +					      node);
> > +			cdma_hw = &cdma_seg->hw;
> > +			residue += (cdma_hw->control - cdma_hw->status)
> &
> > +				   chan->xdev->max_buffer_len;
> > +		} else {
> > +			axidma_seg = list_entry(entry,
> > +						struct
> xilinx_axidma_tx_segment,
> > +						node);
> > +			axidma_hw = &axidma_seg->hw;
> > +			residue += (axidma_hw->control - axidma_hw-
> >status) &
> > +				   chan->xdev->max_buffer_len;
> > +		}
> > +	}
> > +
> > +	return residue;
> > +}
> > +
> > +/**
> >   * xilinx_dma_chan_handle_cyclic - Cyclic dma callback
> >   * @chan: Driver specific dma channel
> >   * @desc: dma transaction descriptor
> > @@ -995,33 +1040,22 @@ static enum dma_status
> > xilinx_dma_tx_status(struct dma_chan *dchan,  {
> >  	struct xilinx_dma_chan *chan = to_xilinx_chan(dchan);
> >  	struct xilinx_dma_tx_descriptor *desc;
> > -	struct xilinx_axidma_tx_segment *segment;
> > -	struct xilinx_axidma_desc_hw *hw;
> >  	enum dma_status ret;
> >  	unsigned long flags;
> > -	u32 residue = 0;
> >
> >  	ret = dma_cookie_status(dchan, cookie, txstate);
> >  	if (ret == DMA_COMPLETE || !txstate)
> >  		return ret;
> >
> > -	if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
> > -		spin_lock_irqsave(&chan->lock, flags);
> > +	spin_lock_irqsave(&chan->lock, flags);
> >
> > -		desc = list_last_entry(&chan->active_list,
> > -				       struct xilinx_dma_tx_descriptor, node);
> > -		if (chan->has_sg) {
> > -			list_for_each_entry(segment, &desc->segments,
> node) {
> > -				hw = &segment->hw;
> > -				residue += (hw->control - hw->status) &
> > -					   chan->xdev->max_buffer_len;
> > -			}
> > -		}
> > -		spin_unlock_irqrestore(&chan->lock, flags);
> > +	desc = list_last_entry(&chan->active_list,
> > +			       struct xilinx_dma_tx_descriptor, node);
> > +	chan->residue = xilinx_dma_get_residue(chan, desc);
> >
> > -		chan->residue = residue;
> > -		dma_set_residue(txstate, chan->residue);
> > -	}
> > +	spin_unlock_irqrestore(&chan->lock, flags);
> > +
> > +	dma_set_residue(txstate, chan->residue);
> >
> >  	return ret;
> >  }
> > @@ -2701,12 +2735,15 @@ static int xilinx_dma_probe(struct
> platform_device *pdev)
> >  					  xilinx_dma_prep_dma_cyclic;
> >  		xdev->common.device_prep_interleaved_dma =
> >  					xilinx_dma_prep_interleaved;
> > -		/* Residue calculation is supported by only AXI DMA */
> > +		/* Residue calculation is supported by only AXI DMA and
> CDMA */
> >  		xdev->common.residue_granularity =
> >
> DMA_RESIDUE_GRANULARITY_SEGMENT;
> >  	} else if (xdev->dma_config->dmatype == XDMA_TYPE_CDMA) {
> >  		dma_cap_set(DMA_MEMCPY, xdev->common.cap_mask);
> >  		xdev->common.device_prep_dma_memcpy =
> xilinx_cdma_prep_memcpy;
> > +		/* Residue calculation is supported by only AXI DMA and
> CDMA */
> > +		xdev->common.residue_granularity =
> > +
> 	DMA_RESIDUE_GRANULARITY_SEGMENT;
> >  	} else {
> >  		xdev->common.device_prep_interleaved_dma =
> >  				xilinx_vdma_dma_prep_interleaved;
> > --
> > 2.7.4
> 
> --
> ~Vinod

Re: [PATCH -next 3/8] dmaengine: xilinx_dma: Introduce xilinx_dma_get_residue

[Vinod Koul]

On 26-09-19, 05:52, Radhey Shyam Pandey wrote:

> > > +	 * VDMA and simple mode do not support residue reporting, so the
> > > +	 * residue field will always be 0.
> > > +	 */
> > > +	if (chan->xdev->dma_config->dmatype == XDMA_TYPE_VDMA ||
> > !chan->has_sg)
> > > +		return residue;
> > 
> > why not check this in status callback?
> Assuming we mean to move vdma and non-sg check to xilinx_dma_tx_status.
> Just a thought- Keeping this check in xilinx_dma_get_residue provides
> an abstraction and caller can simply call this func with knowing about
> IP config specific residue calculation. Considering this point does it
> looks ok ? 

well you are checking either way, so calling the lower level function
only when you need it makes more sense!

-- 
~Vinod

RE: [PATCH -next 3/8] dmaengine: xilinx_dma: Introduce xilinx_dma_get_residue

[Radhey Shyam Pandey]

> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: Thursday, September 26, 2019 10:48 PM
> To: Radhey Shyam Pandey <radheys@xilinx.com>
> Cc: dan.j.williams@intel.com; Michal Simek <michals@xilinx.com>;
> nick.graumann@gmail.com; andrea.merello@gmail.com; Appana Durga
> Kedareswara Rao <appanad@xilinx.com>; mcgrof@kernel.org;
> dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH -next 3/8] dmaengine: xilinx_dma: Introduce
> xilinx_dma_get_residue
> 
> On 26-09-19, 05:52, Radhey Shyam Pandey wrote:
> 
> > > > +	 * VDMA and simple mode do not support residue reporting, so the
> > > > +	 * residue field will always be 0.
> > > > +	 */
> > > > +	if (chan->xdev->dma_config->dmatype == XDMA_TYPE_VDMA ||
> > > !chan->has_sg)
> > > > +		return residue;
> > >
> > > why not check this in status callback?
> > Assuming we mean to move vdma and non-sg check to
> xilinx_dma_tx_status.
> > Just a thought- Keeping this check in xilinx_dma_get_residue provides
> > an abstraction and caller can simply call this func with knowing about
> > IP config specific residue calculation. Considering this point does it
> > looks ok ?
> 
> well you are checking either way, so calling the lower level function
> only when you need it makes more sense!

Sure, will do it in v2.
> 
> --
> ~Vinod

RE: [PATCH -next 3/8] dmaengine: xilinx_dma: Introduce xilinx_dma_get_residue

[Radhey Shyam Pandey]

> -----Original Message-----
> From: Radhey Shyam Pandey
> Sent: Friday, September 27, 2019 10:46 AM
> To: Vinod Koul <vkoul@kernel.org>
> Cc: dan.j.williams@intel.com; Michal Simek <michals@xilinx.com>;
> nick.graumann@gmail.com; andrea.merello@gmail.com; Appana Durga
> Kedareswara Rao <appanad@xilinx.com>; mcgrof@kernel.org;
> dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH -next 3/8] dmaengine: xilinx_dma: Introduce
> xilinx_dma_get_residue
> 
> > -----Original Message-----
> > From: Vinod Koul <vkoul@kernel.org>
> > Sent: Thursday, September 26, 2019 10:48 PM
> > To: Radhey Shyam Pandey <radheys@xilinx.com>
> > Cc: dan.j.williams@intel.com; Michal Simek <michals@xilinx.com>;
> > nick.graumann@gmail.com; andrea.merello@gmail.com; Appana Durga
> > Kedareswara Rao <appanad@xilinx.com>; mcgrof@kernel.org;
> > dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH -next 3/8] dmaengine: xilinx_dma: Introduce
> > xilinx_dma_get_residue
> >
> > On 26-09-19, 05:52, Radhey Shyam Pandey wrote:
> >
> > > > > +	 * VDMA and simple mode do not support residue reporting,
> so the
> > > > > +	 * residue field will always be 0.
> > > > > +	 */
> > > > > +	if (chan->xdev->dma_config->dmatype ==
> XDMA_TYPE_VDMA ||
> > > > !chan->has_sg)
> > > > > +		return residue;
> > > >
> > > > why not check this in status callback?
> > > Assuming we mean to move vdma and non-sg check to
> > xilinx_dma_tx_status.
> > > Just a thought- Keeping this check in xilinx_dma_get_residue
> > > provides an abstraction and caller can simply call this func with
> > > knowing about IP config specific residue calculation. Considering
> > > this point does it looks ok ?
> >
> > well you are checking either way, so calling the lower level function
> > only when you need it makes more sense!
> 
> Sure, will do it in v2.

Just noticed that xilinx_dma_get_residue() is called at the multiple
places i.e one in device_tx_status and other in _complete_descriptor.
To avoid code duplication, I will create a helper function to check
if residue calculation is supported.

> >
> > --
> > ~Vinod

[PATCH -next 4/8] dmaengine: xilinx_dma: Add callback_result support

[Radhey Shyam Pandey]

From: Nicholas Graumann <nick.graumann@gmail.com>

Take advantage of dmaengine_desc_get_callback_invoke which allows either
a callback or callback_result to be specified. This can be useful when
using the AXI DMA transfer unknown quantities of data where the residue
contained in the result can be used to calculate the number of bytes
transferred.

Signed-off-by: Nicholas Graumann <nick.graumann@gmail.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
---
 drivers/dma/xilinx/xilinx_dma.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index 4094adb..3f2e0ad 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -300,12 +300,16 @@ struct xilinx_cdma_tx_segment {
  * @segments: TX segments list
  * @node: Node in the channel descriptors list
  * @cyclic: Check for cyclic transfers.
+ * @err: Whether the descriptor has an error.
+ * @residue: Residue of the completed descriptor
  */
 struct xilinx_dma_tx_descriptor {
 	struct dma_async_tx_descriptor async_tx;
 	struct list_head segments;
 	struct list_head node;
 	bool cyclic;
+	bool err;
+	u32 residue;
 };
 
 /**
@@ -865,6 +869,8 @@ static void xilinx_dma_chan_desc_cleanup(struct xilinx_dma_chan *chan)
 	spin_lock_irqsave(&chan->lock, flags);
 
 	list_for_each_entry_safe(desc, next, &chan->done_list, node) {
+		struct dmaengine_result result;
+
 		if (desc->cyclic) {
 			xilinx_dma_chan_handle_cyclic(chan, desc, &flags);
 			break;
@@ -873,9 +879,20 @@ static void xilinx_dma_chan_desc_cleanup(struct xilinx_dma_chan *chan)
 		/* Remove from the list of running transactions */
 		list_del(&desc->node);
 
+		if (unlikely(desc->err)) {
+			if (chan->direction == DMA_DEV_TO_MEM)
+				result.result = DMA_TRANS_READ_FAILED;
+			else
+				result.result = DMA_TRANS_WRITE_FAILED;
+		} else {
+			result.result = DMA_TRANS_NOERROR;
+		}
+
+		result.residue = desc->residue;
+
 		/* Run the link descriptor callback function */
 		spin_unlock_irqrestore(&chan->lock, flags);
-		dmaengine_desc_get_callback_invoke(&desc->async_tx, NULL);
+		dmaengine_desc_get_callback_invoke(&desc->async_tx, &result);
 		spin_lock_irqsave(&chan->lock, flags);
 
 		/* Run any dependencies, then free the descriptor */
@@ -1424,6 +1441,9 @@ static void xilinx_dma_complete_descriptor(struct xilinx_dma_chan *chan)
 		return;
 
 	list_for_each_entry_safe(desc, next, &chan->active_list, node) {
+		desc->residue = xilinx_dma_get_residue(chan, desc);
+		desc->err = chan->err;
+
 		list_del(&desc->node);
 		if (!desc->cyclic)
 			dma_cookie_complete(&desc->async_tx);
-- 
2.7.4

[PATCH -next 5/8] dmaengine: xilinx_dma: Remove residue from channel data

[Radhey Shyam Pandey]

From: Nicholas Graumann <nick.graumann@gmail.com>

Now that we track residues for each descriptor, there is no need to keep
track of the residue for each channel.

Signed-off-by: Nicholas Graumann <nick.graumann@gmail.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
---
 drivers/dma/xilinx/xilinx_dma.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index 3f2e0ad..bf3fa2e 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -340,7 +340,6 @@ struct xilinx_dma_tx_descriptor {
  * @desc_pendingcount: Descriptor pending count
  * @ext_addr: Indicates 64 bit addressing is supported by dma channel
  * @desc_submitcount: Descriptor h/w submitted count
- * @residue: Residue for AXI DMA
  * @seg_v: Statically allocated segments base
  * @seg_p: Physical allocated segments base
  * @cyclic_seg_v: Statically allocated segment base for cyclic transfers
@@ -377,7 +376,6 @@ struct xilinx_dma_chan {
 	u32 desc_pendingcount;
 	bool ext_addr;
 	u32 desc_submitcount;
-	u32 residue;
 	struct xilinx_axidma_tx_segment *seg_v;
 	dma_addr_t seg_p;
 	struct xilinx_axidma_tx_segment *cyclic_seg_v;
@@ -1068,11 +1066,11 @@ static enum dma_status xilinx_dma_tx_status(struct dma_chan *dchan,
 
 	desc = list_last_entry(&chan->active_list,
 			       struct xilinx_dma_tx_descriptor, node);
-	chan->residue = xilinx_dma_get_residue(chan, desc);
+	desc->residue = xilinx_dma_get_residue(chan, desc);
 
 	spin_unlock_irqrestore(&chan->lock, flags);
 
-	dma_set_residue(txstate, chan->residue);
+	dma_set_residue(txstate, desc->residue);
 
 	return ret;
 }
-- 
2.7.4

[PATCH -next 6/8] dmaengine: xilinx_dma: Print debug message when no free tx segments

[Radhey Shyam Pandey]

From: Nicholas Graumann <nick.graumann@gmail.com>

The driver should not run out of tx segments in normal operation. But,
if the user attempts to prepare a transaction with a large sg list,
the driver may not have enough free segments to accommodate the request.

Log a message at the debug level to inform the user in case they are
experiencing issues.

Signed-off-by: Nicholas Graumann <nick.graumann@gmail.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
---
 drivers/dma/xilinx/xilinx_dma.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index bf3fa2e..b5dd62a 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -612,6 +612,9 @@ xilinx_axidma_alloc_tx_segment(struct xilinx_dma_chan *chan)
 	}
 	spin_unlock_irqrestore(&chan->lock, flags);
 
+	if (!segment)
+		dev_dbg(chan->dev, "Could not find free tx segment\n");
+
 	return segment;
 }
 
-- 
2.7.4

[PATCH -next 7/8] dmaengine: xilinx_dma: Check for both idle and halted state in axidma stop_transfer

[Radhey Shyam Pandey]

From: Nicholas Graumann <nick.graumann@gmail.com>

When polling for a stopped transfer in AXI DMA mode, in some cases the
status of the channel may indicate IDLE instead of HALTED if the
channel was reset due to an error.

Signed-off-by: Nicholas Graumann <nick.graumann@gmail.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
---
 drivers/dma/xilinx/xilinx_dma.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index b5dd62a..0896e07 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -1092,8 +1092,9 @@ static int xilinx_dma_stop_transfer(struct xilinx_dma_chan *chan)
 
 	/* Wait for the hardware to halt */
 	return xilinx_dma_poll_timeout(chan, XILINX_DMA_REG_DMASR, val,
-				       val & XILINX_DMA_DMASR_HALTED, 0,
-				       XILINX_DMA_LOOP_COUNT);
+				       val | (XILINX_DMA_DMASR_IDLE |
+					      XILINX_DMA_DMASR_HALTED),
+				       0, XILINX_DMA_LOOP_COUNT);
 }
 
 /**
-- 
2.7.4

Re: [PATCH -next 7/8] dmaengine: xilinx_dma: Check for both idle and halted state in axidma stop_transfer

[Vinod Koul]

On 05-09-19, 22:07, Radhey Shyam Pandey wrote:
> From: Nicholas Graumann <nick.graumann@gmail.com>
> 
> When polling for a stopped transfer in AXI DMA mode, in some cases the
> status of the channel may indicate IDLE instead of HALTED if the
> channel was reset due to an error.
> 
> Signed-off-by: Nicholas Graumann <nick.graumann@gmail.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> ---
>  drivers/dma/xilinx/xilinx_dma.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index b5dd62a..0896e07 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -1092,8 +1092,9 @@ static int xilinx_dma_stop_transfer(struct xilinx_dma_chan *chan)
>  
>  	/* Wait for the hardware to halt */
>  	return xilinx_dma_poll_timeout(chan, XILINX_DMA_REG_DMASR, val,
> -				       val & XILINX_DMA_DMASR_HALTED, 0,
> -				       XILINX_DMA_LOOP_COUNT);
> +				       val | (XILINX_DMA_DMASR_IDLE |
> +					      XILINX_DMA_DMASR_HALTED),

The condition was bitwise AND and now is OR.. ??

> +				       0, XILINX_DMA_LOOP_COUNT);
>  }
>  
>  /**
> -- 
> 2.7.4

-- 
~Vinod

RE: [PATCH -next 7/8] dmaengine: xilinx_dma: Check for both idle and halted state in axidma stop_transfer

[Radhey Shyam Pandey]

> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: Thursday, September 26, 2019 10:51 PM
> To: Radhey Shyam Pandey <radheys@xilinx.com>
> Cc: dan.j.williams@intel.com; Michal Simek <michals@xilinx.com>;
> nick.graumann@gmail.com; andrea.merello@gmail.com; Appana Durga
> Kedareswara Rao <appanad@xilinx.com>; mcgrof@kernel.org;
> dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH -next 7/8] dmaengine: xilinx_dma: Check for both idle
> and halted state in axidma stop_transfer
> 
> On 05-09-19, 22:07, Radhey Shyam Pandey wrote:
> > From: Nicholas Graumann <nick.graumann@gmail.com>
> >
> > When polling for a stopped transfer in AXI DMA mode, in some cases the
> > status of the channel may indicate IDLE instead of HALTED if the
> > channel was reset due to an error.
> >
> > Signed-off-by: Nicholas Graumann <nick.graumann@gmail.com>
> > Signed-off-by: Radhey Shyam Pandey
> <radhey.shyam.pandey@xilinx.com>
> > ---
> >  drivers/dma/xilinx/xilinx_dma.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/dma/xilinx/xilinx_dma.c
> b/drivers/dma/xilinx/xilinx_dma.c
> > index b5dd62a..0896e07 100644
> > --- a/drivers/dma/xilinx/xilinx_dma.c
> > +++ b/drivers/dma/xilinx/xilinx_dma.c
> > @@ -1092,8 +1092,9 @@ static int xilinx_dma_stop_transfer(struct
> xilinx_dma_chan *chan)
> >
> >  	/* Wait for the hardware to halt */
> >  	return xilinx_dma_poll_timeout(chan, XILINX_DMA_REG_DMASR,
> val,
> > -				       val & XILINX_DMA_DMASR_HALTED, 0,
> > -				       XILINX_DMA_LOOP_COUNT);
> > +				       val | (XILINX_DMA_DMASR_IDLE |
> > +					      XILINX_DMA_DMASR_HALTED),
> 
> The condition was bitwise AND and now is OR.. ??

Ah, it should be same as before . Only _IDLE mask should be in OR.

Also on second thought to this patch- we need to describe which error
scenario "in some cases the status of the channel may indicate IDLE
instead of HALTED" as mentioned in commit description.

@Nick: Can you comment?

> 
> > +				       0, XILINX_DMA_LOOP_COUNT);
> >  }
> >
> >  /**
> > --
> > 2.7.4
> 
> --
> ~Vinod

Re: [PATCH -next 7/8] dmaengine: xilinx_dma: Check for both idle and halted state in axidma stop_transfer

[Nicholas Graumann]

On Fri, Sep 27, 2019 at 06:48:29AM +0000, Radhey Shyam Pandey wrote:
> > -----Original Message-----
> > From: Vinod Koul <vkoul@kernel.org>
> > Sent: Thursday, September 26, 2019 10:51 PM
> > To: Radhey Shyam Pandey <radheys@xilinx.com>
> > Cc: dan.j.williams@intel.com; Michal Simek <michals@xilinx.com>;
> > nick.graumann@gmail.com; andrea.merello@gmail.com; Appana Durga
> > Kedareswara Rao <appanad@xilinx.com>; mcgrof@kernel.org;
> > dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH -next 7/8] dmaengine: xilinx_dma: Check for both idle
> > and halted state in axidma stop_transfer
> > 
> > On 05-09-19, 22:07, Radhey Shyam Pandey wrote:
> > > From: Nicholas Graumann <nick.graumann@gmail.com>
> > >
> > > When polling for a stopped transfer in AXI DMA mode, in some cases the
> > > status of the channel may indicate IDLE instead of HALTED if the
> > > channel was reset due to an error.
> > >
> > > Signed-off-by: Nicholas Graumann <nick.graumann@gmail.com>
> > > Signed-off-by: Radhey Shyam Pandey
> > <radhey.shyam.pandey@xilinx.com>
> > > ---
> > >  drivers/dma/xilinx/xilinx_dma.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/dma/xilinx/xilinx_dma.c
> > b/drivers/dma/xilinx/xilinx_dma.c
> > > index b5dd62a..0896e07 100644
> > > --- a/drivers/dma/xilinx/xilinx_dma.c
> > > +++ b/drivers/dma/xilinx/xilinx_dma.c
> > > @@ -1092,8 +1092,9 @@ static int xilinx_dma_stop_transfer(struct
> > xilinx_dma_chan *chan)
> > >
> > >  	/* Wait for the hardware to halt */
> > >  	return xilinx_dma_poll_timeout(chan, XILINX_DMA_REG_DMASR,
> > val,
> > > -				       val & XILINX_DMA_DMASR_HALTED, 0,
> > > -				       XILINX_DMA_LOOP_COUNT);
> > > +				       val | (XILINX_DMA_DMASR_IDLE |
> > > +					      XILINX_DMA_DMASR_HALTED),
> > 
> > The condition was bitwise AND and now is OR.. ??
> 
> Ah, it should be same as before . Only _IDLE mask should be in OR.
> 
> Also on second thought to this patch- we need to describe which error
> scenario "in some cases the status of the channel may indicate IDLE
> instead of HALTED" as mentioned in commit description.
> 
> @Nick: Can you comment?
> 
In regard to the mask question, yes, this looks like a bug.
We should be AND'ing with the mask like before.

As far as the state, usually when we saw the IDLE state when invoking 
dmaengine_terminate_all on a channel that had errors. I have not
proved this, but I believe what happened was the following:

New transactions were queued when chan->err was set, causing
xilinx_dma_chan_reset to be invoked which ultimately results in the
hardware being in an IDLE state by the time xilinx_dma_terminate_all
gets around to invoking stop_transfer. At that point, stop_transfer is
going to time out waiting for the hardware to indicate it has HALTED and
ultimately will time out.


In any case, xilinx_dma_stop_transfer should be fine with the hardware
being in an IDLE state to indicate that the active transfer is stopped.
Case in point: The CDMA core also covered by this driver only has an
IDLE bit and no HALTED bit in its DMASR, and it checks for just the IDLE
bit in xilinx_cdma_stop_transfer().
> > 
> > > +				       0, XILINX_DMA_LOOP_COUNT);
> > >  }
> > >
> > >  /**
> > > --
> > > 2.7.4
> > 
> > --
> > ~Vinod

RE: [PATCH -next 7/8] dmaengine: xilinx_dma: Check for both idle and halted state in axidma stop_transfer

[Radhey Shyam Pandey]

> -----Original Message-----
> From: Nicholas Graumann <nick.graumann@gmail.com>
> Sent: Friday, September 27, 2019 7:27 PM
> To: Radhey Shyam Pandey <radheys@xilinx.com>
> Cc: Vinod Koul <vkoul@kernel.org>; dan.j.williams@intel.com; Michal Simek
> <michals@xilinx.com>; andrea.merello@gmail.com; Appana Durga
> Kedareswara Rao <appanad@xilinx.com>; mcgrof@kernel.org;
> dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH -next 7/8] dmaengine: xilinx_dma: Check for both idle
> and halted state in axidma stop_transfer
> 
> On Fri, Sep 27, 2019 at 06:48:29AM +0000, Radhey Shyam Pandey wrote:
> > > -----Original Message-----
> > > From: Vinod Koul <vkoul@kernel.org>
> > > Sent: Thursday, September 26, 2019 10:51 PM
> > > To: Radhey Shyam Pandey <radheys@xilinx.com>
> > > Cc: dan.j.williams@intel.com; Michal Simek <michals@xilinx.com>;
> > > nick.graumann@gmail.com; andrea.merello@gmail.com; Appana Durga
> > > Kedareswara Rao <appanad@xilinx.com>; mcgrof@kernel.org;
> > > dmaengine@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH -next 7/8] dmaengine: xilinx_dma: Check for both
> idle
> > > and halted state in axidma stop_transfer
> > >
> > > On 05-09-19, 22:07, Radhey Shyam Pandey wrote:
> > > > From: Nicholas Graumann <nick.graumann@gmail.com>
> > > >
> > > > When polling for a stopped transfer in AXI DMA mode, in some cases
> the
> > > > status of the channel may indicate IDLE instead of HALTED if the
> > > > channel was reset due to an error.
> > > >
> > > > Signed-off-by: Nicholas Graumann <nick.graumann@gmail.com>
> > > > Signed-off-by: Radhey Shyam Pandey
> > > <radhey.shyam.pandey@xilinx.com>
> > > > ---
> > > >  drivers/dma/xilinx/xilinx_dma.c | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/dma/xilinx/xilinx_dma.c
> > > b/drivers/dma/xilinx/xilinx_dma.c
> > > > index b5dd62a..0896e07 100644
> > > > --- a/drivers/dma/xilinx/xilinx_dma.c
> > > > +++ b/drivers/dma/xilinx/xilinx_dma.c
> > > > @@ -1092,8 +1092,9 @@ static int xilinx_dma_stop_transfer(struct
> > > xilinx_dma_chan *chan)
> > > >
> > > >  	/* Wait for the hardware to halt */
> > > >  	return xilinx_dma_poll_timeout(chan, XILINX_DMA_REG_DMASR,
> > > val,
> > > > -				       val & XILINX_DMA_DMASR_HALTED, 0,
> > > > -				       XILINX_DMA_LOOP_COUNT);
> > > > +				       val | (XILINX_DMA_DMASR_IDLE |
> > > > +					      XILINX_DMA_DMASR_HALTED),
> > >
> > > The condition was bitwise AND and now is OR.. ??
> >
> > Ah, it should be same as before . Only _IDLE mask should be in OR.
> >
> > Also on second thought to this patch- we need to describe which error
> > scenario "in some cases the status of the channel may indicate IDLE
> > instead of HALTED" as mentioned in commit description.
> >
> > @Nick: Can you comment?
> >
> In regard to the mask question, yes, this looks like a bug.
> We should be AND'ing with the mask like before.
> 
> As far as the state, usually when we saw the IDLE state when invoking
> dmaengine_terminate_all on a channel that had errors. I have not
> proved this, but I believe what happened was the following:

As per IP produce guide pg021, once DMACR[RS] is set to 0x0
the halted bit in the DMA Status register should asserts to
0x1 when the DMA engine is halted. Also the DMA may be the
in IDLE state, there may be active data on the AXI interface.

I think for now we can skip this patchset in v2 and repost it
when a proper root cause is done.

> 
> New transactions were queued when chan->err was set, causing
> xilinx_dma_chan_reset to be invoked which ultimately results in the
> hardware being in an IDLE state by the time xilinx_dma_terminate_all
> gets around to invoking stop_transfer. At that point, stop_transfer is
> going to time out waiting for the hardware to indicate it has HALTED and
> ultimately will time out.
> 
> 
> In any case, xilinx_dma_stop_transfer should be fine with the hardware
> being in an IDLE state to indicate that the active transfer is stopped.
> Case in point: The CDMA core also covered by this driver only has an
> IDLE bit and no HALTED bit in its DMASR, and it checks for just the IDLE
> bit in xilinx_cdma_stop_transfer().
> > >
> > > > +				       0, XILINX_DMA_LOOP_COUNT);
> > > >  }
> > > >
> > > >  /**
> > > > --
> > > > 2.7.4
> > >
> > > --
> > > ~Vinod

[PATCH -next 8/8] dmaengine: xilinx_dma: Clear desc_pendingcount in xilinx_dma_reset

[Radhey Shyam Pandey]

From: Nicholas Graumann <nick.graumann@gmail.com>

Whenever we reset the channel, we need to clear desc_pendingcount
along with desc_submitcount. Otherwise when a new transaction is
submitted, the irq coalesce level could be programmed to an incorrect
value in the axidma case.

This behavior can be observed when terminating pending transactions
with xilinx_dma_terminate_all() and then submitting new transactions
without releasing and requesting the channel.

Signed-off-by: Nicholas Graumann <nick.graumann@gmail.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
---
 drivers/dma/xilinx/xilinx_dma.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index 0896e07..010baed 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -1480,6 +1480,7 @@ static int xilinx_dma_reset(struct xilinx_dma_chan *chan)
 
 	chan->err = false;
 	chan->idle = true;
+	chan->desc_pendingcount = 0;
 	chan->desc_submitcount = 0;
 
 	return err;
-- 
2.7.4