[PATCH v2 0/2] dma: at_hdmac: Fix residue calculation and add mem to

[PATCH v2 0/2] dma: at_hdmac: Fix residue calculation and add mem to

[Torsten Fleischer]

From: Torsten Fleischer <torfl6749@gmail.com>

This series fixes the calculation of the residual bytes and adds support for
memory to memory scatter-gather transfers.

Changes from V1:
   * Fixed coding style of the multi-line comments.
   * Improved accuracy of the residue calculation.

Torsten Fleischer (2):
  dma: at_hdmac: Fix calculation of the residual bytes
  dma: at_hdmac: Add support for memory to memory sg transfers

 drivers/dma/at_hdmac.c      | 324 ++++++++++++++++++++++++++++++++------------
 drivers/dma/at_hdmac_regs.h |  11 +-
 2 files changed, 245 insertions(+), 90 deletions(-)

-- 
2.1.4

[PATCH v2 1/2] dma: at_hdmac: Fix calculation of the residual bytes

[Torsten Fleischer]

From: Torsten Fleischer <torfl6749@gmail.com>

This patch fixes the following issues regarding to the calculation of the
residue:

1. The residue is always calculated for the current transfer even if the
cookie is associated to a pending transfer.

2. For scatter/gather DMA the calculation of the residue for the current
transfer doesn't include the bytes of the child descriptors that are already
transferred.
It only calculates the difference between the transfer's total length minus
the number of bytes that are already transferred for the current child
descriptor.
For example: There is a scatter/gather DMA transfer with a total length of
1 MByte. Getting the residue several times while the transfer is running shows
something like that:

1: residue = 975584
2: residue = 1002766
3: residue = 992627
4: residue = 983767
5: residue = 985694
6: residue = 1008094
7: residue = 1009741
8: residue = 1011195

3. The driver stores the residue but never resets it when starting a new
transfer.
For example: If there are two subsequent DMA transfers. The first one with
a total length of 1 MByte and the second one with a total length of 1 kByte.
Getting the residue for both transfers shows something like that:

transfer 1: residue = 975584
transfer 2: residue = 1048380

Changes from V1:
   * Fixed coding style of the multi-line comments.
   * Improved accuracy of the residue calculation when the transfer for the
     first descriptor is active.

Signed-off-by: Torsten Fleischer <torfl6749@gmail.com>
---
 drivers/dma/at_hdmac.c      | 155 +++++++++++++++++++++++---------------------
 drivers/dma/at_hdmac_regs.h |  11 ++--
 2 files changed, 87 insertions(+), 79 deletions(-)

diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
index ca9dd26..f46d86d 100644
--- a/drivers/dma/at_hdmac.c
+++ b/drivers/dma/at_hdmac.c
@@ -233,93 +233,104 @@ static void atc_dostart(struct at_dma_chan *atchan, struct at_desc *first)
 }
 
 /*
- * atc_get_current_descriptors -
- * locate the descriptor which equal to physical address in DSCR
- * @atchan: the channel we want to start
- * @dscr_addr: physical descriptor address in DSCR
+ * atc_get_desc_by_cookie - get the descriptor of a cookie
+ * @atchan: the DMA channel
+ * @cookie: the cookie to get the descriptor for
  */
-static struct at_desc *atc_get_current_descriptors(struct at_dma_chan *atchan,
-							u32 dscr_addr)
+static struct at_desc *atc_get_desc_by_cookie(struct at_dma_chan *atchan,
+						dma_cookie_t cookie)
 {
-	struct at_desc  *desc, *_desc, *child, *desc_cur = NULL;
+	struct at_desc *desc, *_desc;
 
-	list_for_each_entry_safe(desc, _desc, &atchan->active_list, desc_node) {
-		if (desc->lli.dscr == dscr_addr) {
-			desc_cur = desc;
-			break;
-		}
+	list_for_each_entry_safe(desc, _desc, &atchan->queue, desc_node) {
+		if (desc->txd.cookie == cookie)
+			return desc;
+	}
 
-		list_for_each_entry(child, &desc->tx_list, desc_node) {
-			if (child->lli.dscr == dscr_addr) {
-				desc_cur = child;
-				break;
-			}
-		}
+	list_for_each_entry_safe(desc, _desc, &atchan->active_list, desc_node) {
+		if (desc->txd.cookie == cookie)
+			return desc;
 	}
 
-	return desc_cur;
+	return NULL;
 }
 
-/*
- * atc_get_bytes_left -
- * Get the number of bytes residue in dma buffer,
- * @chan: the channel we want to start
+/**
+ * atc_get_bytes_left - get the number of bytes residue for a cookie
+ * @chan: DMA channel
+ * @cookie: transaction identifier to check status of
  */
-static int atc_get_bytes_left(struct dma_chan *chan)
+static int atc_get_bytes_left(struct dma_chan *chan, dma_cookie_t cookie)
 {
 	struct at_dma_chan      *atchan = to_at_dma_chan(chan);
-	struct at_dma           *atdma = to_at_dma(chan->device);
-	int	chan_id = atchan->chan_common.chan_id;
 	struct at_desc *desc_first = atc_first_active(atchan);
-	struct at_desc *desc_cur;
+	struct at_desc *desc;
 	int ret = 0, count = 0;
+	u32 ctrla, dscr;
 
 	/*
-	 * Initialize necessary values in the first time.
-	 * remain_desc record remain desc length.
+	 * If the cookie doesn't match to the currently running transfer then
+	 * we can return the total length of the associated DMA transfer,
+	 * because it is still queued.
 	 */
-	if (atchan->remain_desc == 0)
-		/* First descriptor embedds the transaction length */
-		atchan->remain_desc = desc_first->len;
+	desc = atc_get_desc_by_cookie(atchan, cookie);
+	if (desc == NULL)
+		return -EINVAL;
+	else if (desc != desc_first)
+		return desc->total_len;
 
-	/*
-	 * This happens when current descriptor transfer complete.
-	 * The residual buffer size should reduce current descriptor length.
-	 */
-	if (unlikely(test_bit(ATC_IS_BTC, &atchan->status))) {
-		clear_bit(ATC_IS_BTC, &atchan->status);
-		desc_cur = atc_get_current_descriptors(atchan,
-						channel_readl(atchan, DSCR));
-		if (!desc_cur) {
-			ret = -EINVAL;
-			goto out;
+	/* cookie matches to the currently running transfer */
+	ret = desc_first->total_len;
+
+	if (desc_first->lli.dscr) {
+		/* hardware linked list transfer */
+
+		/*
+		 * Calculate the residue by removing the length of the child
+		 * descriptors already transferred from the total length.
+		 * To get the current child descriptor we can use the value of
+		 * the channel's DSCR register and compare it against the value
+		 * of the hardware linked list structure of each child
+		 * descriptor.
+		 */
+
+		ctrla = channel_readl(atchan, CTRLA);
+		rmb(); /* ensure CTRLA is read before DSCR */
+		dscr = channel_readl(atchan, DSCR);
+
+		/* for the first descriptor we can be more accurate */
+		if (desc_first->lli.dscr == dscr) {
+			count = (ctrla & ATC_BTSIZE_MAX);
+			ret -= count << ATC_REG_TO_SRC_WIDTH(ctrla);
+			return ret;
 		}
 
-		count = (desc_cur->lli.ctrla & ATC_BTSIZE_MAX)
-			<< desc_first->tx_width;
-		if (atchan->remain_desc < count) {
-			ret = -EINVAL;
-			goto out;
+		ret -= desc_first->len;
+		list_for_each_entry(desc, &desc_first->tx_list, desc_node) {
+			if (desc->lli.dscr == dscr)
+				break;
+
+			ret -= desc->len;
 		}
 
-		atchan->remain_desc -= count;
-		ret = atchan->remain_desc;
-	} else {
 		/*
-		 * Get residual bytes when current
-		 * descriptor transfer in progress.
+		 * For the last descriptor in the chain we can calculate
+		 * the remaining bytes using the channel's register.
 		 */
-		count = (channel_readl(atchan, CTRLA) & ATC_BTSIZE_MAX)
-				<< (desc_first->tx_width);
-		ret = atchan->remain_desc - count;
+		if (!desc->lli.dscr) {
+			ctrla = channel_readl(atchan, CTRLA);
+			count = (desc->lli.ctrla & ATC_BTSIZE_MAX) -
+				(ctrla & ATC_BTSIZE_MAX);
+
+			ret = count << ATC_REG_TO_SRC_WIDTH(ctrla);
+		}
+	} else {
+		/* single transfer */
+		ctrla = channel_readl(atchan, CTRLA);
+		count = (ctrla & ATC_BTSIZE_MAX) << ATC_REG_TO_SRC_WIDTH(ctrla);
+		ret -= count;
 	}
-	/*
-	 * Check fifo empty.
-	 */
-	if (!(dma_readl(atdma, CHSR) & AT_DMA_EMPT(chan_id)))
-		atc_issue_pending(chan);
 
-out:
 	return ret;
 }
 
@@ -534,8 +545,6 @@ static irqreturn_t at_dma_interrupt(int irq, void *dev_id)
 					/* Give information to tasklet */
 					set_bit(ATC_IS_ERROR, &atchan->status);
 				}
-				if (pending & AT_DMA_BTC(i))
-					set_bit(ATC_IS_BTC, &atchan->status);
 				tasklet_schedule(&atchan->tasklet);
 				ret = IRQ_HANDLED;
 			}
@@ -648,14 +657,14 @@ atc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
 		desc->lli.ctrlb = ctrlb;
 
 		desc->txd.cookie = 0;
+		desc->len = xfer_count << src_width;
 
 		atc_desc_chain(&first, &prev, desc);
 	}
 
 	/* First descriptor of the chain embedds additional information */
 	first->txd.cookie = -EBUSY;
-	first->len = len;
-	first->tx_width = src_width;
+	first->total_len = len;
 
 	/* set end-of-link to the last link descriptor of list*/
 	set_desc_eol(desc);
@@ -747,6 +756,7 @@ atc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 					| ATC_SRC_WIDTH(mem_width)
 					| len >> mem_width;
 			desc->lli.ctrlb = ctrlb;
+			desc->len = len;
 
 			atc_desc_chain(&first, &prev, desc);
 			total_len += len;
@@ -787,6 +797,7 @@ atc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 					| ATC_DST_WIDTH(mem_width)
 					| len >> reg_width;
 			desc->lli.ctrlb = ctrlb;
+			desc->len = len;
 
 			atc_desc_chain(&first, &prev, desc);
 			total_len += len;
@@ -801,8 +812,7 @@ atc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 
 	/* First descriptor of the chain embedds additional information */
 	first->txd.cookie = -EBUSY;
-	first->len = total_len;
-	first->tx_width = reg_width;
+	first->total_len = total_len;
 
 	/* first link descriptor of list is responsible of flags */
 	first->txd.flags = flags; /* client is in control of this ack */
@@ -867,6 +877,7 @@ atc_dma_cyclic_fill_desc(struct dma_chan *chan, struct at_desc *desc,
 				| ATC_FC_MEM2PER
 				| ATC_SIF(atchan->mem_if)
 				| ATC_DIF(atchan->per_if);
+		desc->len = period_len;
 		break;
 
 	case DMA_DEV_TO_MEM:
@@ -878,6 +889,7 @@ atc_dma_cyclic_fill_desc(struct dma_chan *chan, struct at_desc *desc,
 				| ATC_FC_PER2MEM
 				| ATC_SIF(atchan->per_if)
 				| ATC_DIF(atchan->mem_if);
+		desc->len = period_len;
 		break;
 
 	default:
@@ -959,8 +971,7 @@ atc_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
 
 	/* First descriptor of the chain embedds additional information */
 	first->txd.cookie = -EBUSY;
-	first->len = buf_len;
-	first->tx_width = reg_width;
+	first->total_len = buf_len;
 
 	return &first->txd;
 
@@ -1091,7 +1102,7 @@ atc_tx_status(struct dma_chan *chan,
 	spin_lock_irqsave(&atchan->lock, flags);
 
 	/*  Get number of bytes left in the active transactions */
-	bytes = atc_get_bytes_left(chan);
+	bytes = atc_get_bytes_left(chan, cookie);
 
 	spin_unlock_irqrestore(&atchan->lock, flags);
 
@@ -1187,7 +1198,6 @@ static int atc_alloc_chan_resources(struct dma_chan *chan)
 
 	spin_lock_irqsave(&atchan->lock, flags);
 	atchan->descs_allocated = i;
-	atchan->remain_desc = 0;
 	list_splice(&tmp_list, &atchan->free_list);
 	dma_cookie_init(chan);
 	spin_unlock_irqrestore(&atchan->lock, flags);
@@ -1230,7 +1240,6 @@ static void atc_free_chan_resources(struct dma_chan *chan)
 	list_splice_init(&atchan->free_list, &list);
 	atchan->descs_allocated = 0;
 	atchan->status = 0;
-	atchan->remain_desc = 0;
 
 	dev_vdbg(chan2dev(chan), "free_chan_resources: done\n");
 }
diff --git a/drivers/dma/at_hdmac_regs.h b/drivers/dma/at_hdmac_regs.h
index 2787aba..f458642 100644
--- a/drivers/dma/at_hdmac_regs.h
+++ b/drivers/dma/at_hdmac_regs.h
@@ -112,11 +112,13 @@
 #define		ATC_SRC_WIDTH_BYTE	(0x0 << 24)
 #define		ATC_SRC_WIDTH_HALFWORD	(0x1 << 24)
 #define		ATC_SRC_WIDTH_WORD	(0x2 << 24)
+#define		ATC_REG_TO_SRC_WIDTH(x)	(((x) >> 24) & 0x3)
 #define	ATC_DST_WIDTH_MASK	(0x3 << 28)	/* Destination Single Transfer Size */
 #define		ATC_DST_WIDTH(x)	((x) << 28)
 #define		ATC_DST_WIDTH_BYTE	(0x0 << 28)
 #define		ATC_DST_WIDTH_HALFWORD	(0x1 << 28)
 #define		ATC_DST_WIDTH_WORD	(0x2 << 28)
+#define		ATC_REG_TO_DST_WIDTH(x)	(((x) >> 28) & 0x3)
 #define	ATC_DONE		(0x1 << 31)	/* Tx Done (only written back in descriptor) */
 
 /* Bitfields in CTRLB */
@@ -181,8 +183,8 @@ struct at_lli {
  * @at_lli: hardware lli structure
  * @txd: support for the async_tx api
  * @desc_node: node on the channed descriptors list
- * @len: total transaction bytecount
- * @tx_width: transfer width
+ * @len: descriptor byte count
+ * @total_len: total transaction byte count
  */
 struct at_desc {
 	/* FIRST values the hardware uses */
@@ -193,7 +195,7 @@ struct at_desc {
 	struct dma_async_tx_descriptor	txd;
 	struct list_head		desc_node;
 	size_t				len;
-	u32				tx_width;
+	size_t				total_len;
 };
 
 static inline struct at_desc *
@@ -213,7 +215,6 @@ txd_to_at_desc(struct dma_async_tx_descriptor *txd)
 enum atc_status {
 	ATC_IS_ERROR = 0,
 	ATC_IS_PAUSED = 1,
-	ATC_IS_BTC = 2,
 	ATC_IS_CYCLIC = 24,
 };
 
@@ -231,7 +232,6 @@ enum atc_status {
  * @save_cfg: configuration register that is saved on suspend/resume cycle
  * @save_dscr: for cyclic operations, preserve next descriptor address in
  *             the cyclic list on suspend/resume cycle
- * @remain_desc: to save remain desc length
  * @dma_sconfig: configuration for slave transfers, passed via DMA_SLAVE_CONFIG
  * @lock: serializes enqueue/dequeue operations to descriptors lists
  * @active_list: list of descriptors dmaengine is being running on
@@ -250,7 +250,6 @@ struct at_dma_chan {
 	struct tasklet_struct	tasklet;
 	u32			save_cfg;
 	u32			save_dscr;
-	u32			remain_desc;
 	struct dma_slave_config dma_sconfig;
 
 	spinlock_t		lock;
-- 
2.1.4

[PATCH v2 1/2] dma: at_hdmac: Fix calculation of the residual bytes

[Nicolas Ferre]

Le 19/02/2015 18:07, Torsten Fleischer a ?crit :
> From: Torsten Fleischer <torfl6749@gmail.com>
> 
> This patch fixes the following issues regarding to the calculation of the
> residue:
> 
> 1. The residue is always calculated for the current transfer even if the
> cookie is associated to a pending transfer.
> 
> 2. For scatter/gather DMA the calculation of the residue for the current
> transfer doesn't include the bytes of the child descriptors that are already
> transferred.
> It only calculates the difference between the transfer's total length minus
> the number of bytes that are already transferred for the current child
> descriptor.
> For example: There is a scatter/gather DMA transfer with a total length of
> 1 MByte. Getting the residue several times while the transfer is running shows
> something like that:
> 
> 1: residue = 975584
> 2: residue = 1002766
> 3: residue = 992627
> 4: residue = 983767
> 5: residue = 985694
> 6: residue = 1008094
> 7: residue = 1009741
> 8: residue = 1011195
> 
> 3. The driver stores the residue but never resets it when starting a new
> transfer.
> For example: If there are two subsequent DMA transfers. The first one with
> a total length of 1 MByte and the second one with a total length of 1 kByte.
> Getting the residue for both transfers shows something like that:
> 
> transfer 1: residue = 975584
> transfer 2: residue = 1048380
> 
> Changes from V1:
>    * Fixed coding style of the multi-line comments.
>    * Improved accuracy of the residue calculation when the transfer for the
>      first descriptor is active.
> 
> Signed-off-by: Torsten Fleischer <torfl6749@gmail.com>

Torsten,

Thanks a lot for your work and sorry for the delay.
I just do an additional review after Ludovic's one. BTW, I think we can
add his from the v1 of the series:
Acked-by: Ludovic Desroches <ludovic.desroches@atmel.com>

Anyway, I still have a few comments below...

> ---
>  drivers/dma/at_hdmac.c      | 155 +++++++++++++++++++++++---------------------
>  drivers/dma/at_hdmac_regs.h |  11 ++--
>  2 files changed, 87 insertions(+), 79 deletions(-)
> 
> diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
> index ca9dd26..f46d86d 100644
> --- a/drivers/dma/at_hdmac.c
> +++ b/drivers/dma/at_hdmac.c
> @@ -233,93 +233,104 @@ static void atc_dostart(struct at_dma_chan *atchan, struct at_desc *first)
>  }
>  
>  /*
> - * atc_get_current_descriptors -
> - * locate the descriptor which equal to physical address in DSCR
> - * @atchan: the channel we want to start
> - * @dscr_addr: physical descriptor address in DSCR
> + * atc_get_desc_by_cookie - get the descriptor of a cookie
> + * @atchan: the DMA channel
> + * @cookie: the cookie to get the descriptor for
>   */
> -static struct at_desc *atc_get_current_descriptors(struct at_dma_chan *atchan,
> -							u32 dscr_addr)
> +static struct at_desc *atc_get_desc_by_cookie(struct at_dma_chan *atchan,
> +						dma_cookie_t cookie)
>  {
> -	struct at_desc  *desc, *_desc, *child, *desc_cur = NULL;
> +	struct at_desc *desc, *_desc;
>  
> -	list_for_each_entry_safe(desc, _desc, &atchan->active_list, desc_node) {
> -		if (desc->lli.dscr == dscr_addr) {
> -			desc_cur = desc;
> -			break;
> -		}
> +	list_for_each_entry_safe(desc, _desc, &atchan->queue, desc_node) {
> +		if (desc->txd.cookie == cookie)
> +			return desc;
> +	}
>  
> -		list_for_each_entry(child, &desc->tx_list, desc_node) {
> -			if (child->lli.dscr == dscr_addr) {
> -				desc_cur = child;
> -				break;
> -			}
> -		}
> +	list_for_each_entry_safe(desc, _desc, &atchan->active_list, desc_node) {
> +		if (desc->txd.cookie == cookie)
> +			return desc;
>  	}
>  
> -	return desc_cur;
> +	return NULL;
>  }
>  
> -/*
> - * atc_get_bytes_left -
> - * Get the number of bytes residue in dma buffer,
> - * @chan: the channel we want to start
> +/**
> + * atc_get_bytes_left - get the number of bytes residue for a cookie
> + * @chan: DMA channel
> + * @cookie: transaction identifier to check status of
>   */
> -static int atc_get_bytes_left(struct dma_chan *chan)
> +static int atc_get_bytes_left(struct dma_chan *chan, dma_cookie_t cookie)
>  {
>  	struct at_dma_chan      *atchan = to_at_dma_chan(chan);
> -	struct at_dma           *atdma = to_at_dma(chan->device);
> -	int	chan_id = atchan->chan_common.chan_id;
>  	struct at_desc *desc_first = atc_first_active(atchan);
> -	struct at_desc *desc_cur;
> +	struct at_desc *desc;
>  	int ret = 0, count = 0;
> +	u32 ctrla, dscr;
>  
>  	/*
> -	 * Initialize necessary values in the first time.
> -	 * remain_desc record remain desc length.
> +	 * If the cookie doesn't match to the currently running transfer then
> +	 * we can return the total length of the associated DMA transfer,
> +	 * because it is still queued.
>  	 */
> -	if (atchan->remain_desc == 0)
> -		/* First descriptor embedds the transaction length */
> -		atchan->remain_desc = desc_first->len;
> +	desc = atc_get_desc_by_cookie(atchan, cookie);
> +	if (desc == NULL)
> +		return -EINVAL;
> +	else if (desc != desc_first)
> +		return desc->total_len;
>  
> -	/*
> -	 * This happens when current descriptor transfer complete.
> -	 * The residual buffer size should reduce current descriptor length.
> -	 */
> -	if (unlikely(test_bit(ATC_IS_BTC, &atchan->status))) {
> -		clear_bit(ATC_IS_BTC, &atchan->status);
> -		desc_cur = atc_get_current_descriptors(atchan,
> -						channel_readl(atchan, DSCR));
> -		if (!desc_cur) {
> -			ret = -EINVAL;
> -			goto out;
> +	/* cookie matches to the currently running transfer */
> +	ret = desc_first->total_len;
> +
> +	if (desc_first->lli.dscr) {
> +		/* hardware linked list transfer */
> +
> +		/*
> +		 * Calculate the residue by removing the length of the child
> +		 * descriptors already transferred from the total length.
> +		 * To get the current child descriptor we can use the value of
> +		 * the channel's DSCR register and compare it against the value
> +		 * of the hardware linked list structure of each child
> +		 * descriptor.
> +		 */
> +
> +		ctrla = channel_readl(atchan, CTRLA);
> +		rmb(); /* ensure CTRLA is read before DSCR */
> +		dscr = channel_readl(atchan, DSCR);
> +
> +		/* for the first descriptor we can be more accurate */
> +		if (desc_first->lli.dscr == dscr) {
> +			count = (ctrla & ATC_BTSIZE_MAX);
> +			ret -= count << ATC_REG_TO_SRC_WIDTH(ctrla);

I see a little issue here. How are you sure that we must use the *src*
width and not the *dst* width in this computation?

As the register width is different depending on the transfer direction,
I suspect the computation can be wrong in "slave_sg" case.
So, I would recommend to keep the "tx_width" property in the descriptor
and use it here.

Apart from that, it seems that this computation or variants of it are
done twice or maybe 3 times. So, do you think that we can extract a
function from this code?

> +			return ret;
>  		}
>  
> -		count = (desc_cur->lli.ctrla & ATC_BTSIZE_MAX)
> -			<< desc_first->tx_width;
> -		if (atchan->remain_desc < count) {
> -			ret = -EINVAL;
> -			goto out;
> +		ret -= desc_first->len;
> +		list_for_each_entry(desc, &desc_first->tx_list, desc_node) {
> +			if (desc->lli.dscr == dscr)
> +				break;
> +
> +			ret -= desc->len;
>  		}
>  
> -		atchan->remain_desc -= count;
> -		ret = atchan->remain_desc;
> -	} else {
>  		/*
> -		 * Get residual bytes when current
> -		 * descriptor transfer in progress.
> +		 * For the last descriptor in the chain we can calculate
> +		 * the remaining bytes using the channel's register.
>  		 */
> -		count = (channel_readl(atchan, CTRLA) & ATC_BTSIZE_MAX)
> -				<< (desc_first->tx_width);
> -		ret = atchan->remain_desc - count;
> +		if (!desc->lli.dscr) {
> +			ctrla = channel_readl(atchan, CTRLA);
> +			count = (desc->lli.ctrla & ATC_BTSIZE_MAX) -
> +				(ctrla & ATC_BTSIZE_MAX);
> +
> +			ret = count << ATC_REG_TO_SRC_WIDTH(ctrla);

Ditto.

> +		}
> +	} else {
> +		/* single transfer */
> +		ctrla = channel_readl(atchan, CTRLA);
> +		count = (ctrla & ATC_BTSIZE_MAX) << ATC_REG_TO_SRC_WIDTH(ctrla);

Ditto.

> +		ret -= count;
>  	}
> -	/*
> -	 * Check fifo empty.
> -	 */
> -	if (!(dma_readl(atdma, CHSR) & AT_DMA_EMPT(chan_id)))
> -		atc_issue_pending(chan);

Yes, I've never understood why it was needed.

>  
> -out:
>  	return ret;
>  }
>  
> @@ -534,8 +545,6 @@ static irqreturn_t at_dma_interrupt(int irq, void *dev_id)
>  					/* Give information to tasklet */
>  					set_bit(ATC_IS_ERROR, &atchan->status);
>  				}
> -				if (pending & AT_DMA_BTC(i))
> -					set_bit(ATC_IS_BTC, &atchan->status);
>  				tasklet_schedule(&atchan->tasklet);
>  				ret = IRQ_HANDLED;
>  			}
> @@ -648,14 +657,14 @@ atc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
>  		desc->lli.ctrlb = ctrlb;
>  
>  		desc->txd.cookie = 0;
> +		desc->len = xfer_count << src_width;
>  
>  		atc_desc_chain(&first, &prev, desc);
>  	}
>  
>  	/* First descriptor of the chain embedds additional information */
>  	first->txd.cookie = -EBUSY;
> -	first->len = len;
> -	first->tx_width = src_width;

Let's keep tx_width.

> +	first->total_len = len;
>  
>  	/* set end-of-link to the last link descriptor of list*/
>  	set_desc_eol(desc);
> @@ -747,6 +756,7 @@ atc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  					| ATC_SRC_WIDTH(mem_width)
>  					| len >> mem_width;
>  			desc->lli.ctrlb = ctrlb;
> +			desc->len = len;
>  
>  			atc_desc_chain(&first, &prev, desc);
>  			total_len += len;
> @@ -787,6 +797,7 @@ atc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  					| ATC_DST_WIDTH(mem_width)
>  					| len >> reg_width;
>  			desc->lli.ctrlb = ctrlb;
> +			desc->len = len;
>  
>  			atc_desc_chain(&first, &prev, desc);
>  			total_len += len;
> @@ -801,8 +812,7 @@ atc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  
>  	/* First descriptor of the chain embedds additional information */
>  	first->txd.cookie = -EBUSY;
> -	first->len = total_len;
> -	first->tx_width = reg_width;

Let's keep tx_width: this is where tx_width can be different depending
of the transfer direction.

> +	first->total_len = total_len;
>  
>  	/* first link descriptor of list is responsible of flags */
>  	first->txd.flags = flags; /* client is in control of this ack */
> @@ -867,6 +877,7 @@ atc_dma_cyclic_fill_desc(struct dma_chan *chan, struct at_desc *desc,
>  				| ATC_FC_MEM2PER
>  				| ATC_SIF(atchan->mem_if)
>  				| ATC_DIF(atchan->per_if);
> +		desc->len = period_len;
>  		break;
>  
>  	case DMA_DEV_TO_MEM:
> @@ -878,6 +889,7 @@ atc_dma_cyclic_fill_desc(struct dma_chan *chan, struct at_desc *desc,
>  				| ATC_FC_PER2MEM
>  				| ATC_SIF(atchan->per_if)
>  				| ATC_DIF(atchan->mem_if);
> +		desc->len = period_len;
>  		break;
>  
>  	default:
> @@ -959,8 +971,7 @@ atc_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
>  
>  	/* First descriptor of the chain embedds additional information */
>  	first->txd.cookie = -EBUSY;
> -	first->len = buf_len;
> -	first->tx_width = reg_width;

Here again, let's keep tx_width.

> +	first->total_len = buf_len;
>  
>  	return &first->txd;
>  
> @@ -1091,7 +1102,7 @@ atc_tx_status(struct dma_chan *chan,
>  	spin_lock_irqsave(&atchan->lock, flags);
>  
>  	/*  Get number of bytes left in the active transactions */
> -	bytes = atc_get_bytes_left(chan);
> +	bytes = atc_get_bytes_left(chan, cookie);
>  
>  	spin_unlock_irqrestore(&atchan->lock, flags);
>  
> @@ -1187,7 +1198,6 @@ static int atc_alloc_chan_resources(struct dma_chan *chan)
>  
>  	spin_lock_irqsave(&atchan->lock, flags);
>  	atchan->descs_allocated = i;
> -	atchan->remain_desc = 0;

Ok, this is not needed anymore: right.

>  	list_splice(&tmp_list, &atchan->free_list);
>  	dma_cookie_init(chan);
>  	spin_unlock_irqrestore(&atchan->lock, flags);
> @@ -1230,7 +1240,6 @@ static void atc_free_chan_resources(struct dma_chan *chan)
>  	list_splice_init(&atchan->free_list, &list);
>  	atchan->descs_allocated = 0;
>  	atchan->status = 0;
> -	atchan->remain_desc = 0;
>  
>  	dev_vdbg(chan2dev(chan), "free_chan_resources: done\n");
>  }
> diff --git a/drivers/dma/at_hdmac_regs.h b/drivers/dma/at_hdmac_regs.h
> index 2787aba..f458642 100644
> --- a/drivers/dma/at_hdmac_regs.h
> +++ b/drivers/dma/at_hdmac_regs.h
> @@ -112,11 +112,13 @@
>  #define		ATC_SRC_WIDTH_BYTE	(0x0 << 24)
>  #define		ATC_SRC_WIDTH_HALFWORD	(0x1 << 24)
>  #define		ATC_SRC_WIDTH_WORD	(0x2 << 24)
> +#define		ATC_REG_TO_SRC_WIDTH(x)	(((x) >> 24) & 0x3)

Here...

>  #define	ATC_DST_WIDTH_MASK	(0x3 << 28)	/* Destination Single Transfer Size */
>  #define		ATC_DST_WIDTH(x)	((x) << 28)
>  #define		ATC_DST_WIDTH_BYTE	(0x0 << 28)
>  #define		ATC_DST_WIDTH_HALFWORD	(0x1 << 28)
>  #define		ATC_DST_WIDTH_WORD	(0x2 << 28)
> +#define		ATC_REG_TO_DST_WIDTH(x)	(((x) >> 28) & 0x3)

... and here, well, I'm not sure it can be statically called: so maybe
these macros are not needed.


>  #define	ATC_DONE		(0x1 << 31)	/* Tx Done (only written back in descriptor) */
>  
>  /* Bitfields in CTRLB */
> @@ -181,8 +183,8 @@ struct at_lli {
>   * @at_lli: hardware lli structure
>   * @txd: support for the async_tx api
>   * @desc_node: node on the channed descriptors list
> - * @len: total transaction bytecount

Yes for the re-purpose of this.

> - * @tx_width: transfer width

Nack for this, sorry.


> + * @len: descriptor byte count

Ok.

> + * @total_len: total transaction byte count

Ok: this is clearer.

>   */
>  struct at_desc {
>  	/* FIRST values the hardware uses */
> @@ -193,7 +195,7 @@ struct at_desc {
>  	struct dma_async_tx_descriptor	txd;
>  	struct list_head		desc_node;
>  	size_t				len;
> -	u32				tx_width;
> +	size_t				total_len;
>  };
>  
>  static inline struct at_desc *
> @@ -213,7 +215,6 @@ txd_to_at_desc(struct dma_async_tx_descriptor *txd)
>  enum atc_status {
>  	ATC_IS_ERROR = 0,
>  	ATC_IS_PAUSED = 1,
> -	ATC_IS_BTC = 2,

Yes, it's seems not needed anymore.

>  	ATC_IS_CYCLIC = 24,
>  };
>  
> @@ -231,7 +232,6 @@ enum atc_status {
>   * @save_cfg: configuration register that is saved on suspend/resume cycle
>   * @save_dscr: for cyclic operations, preserve next descriptor address in
>   *             the cyclic list on suspend/resume cycle
> - * @remain_desc: to save remain desc length
>   * @dma_sconfig: configuration for slave transfers, passed via DMA_SLAVE_CONFIG
>   * @lock: serializes enqueue/dequeue operations to descriptors lists
>   * @active_list: list of descriptors dmaengine is being running on
> @@ -250,7 +250,6 @@ struct at_dma_chan {
>  	struct tasklet_struct	tasklet;
>  	u32			save_cfg;
>  	u32			save_dscr;
> -	u32			remain_desc;
>  	struct dma_slave_config dma_sconfig;
>  
>  	spinlock_t		lock;
> 


-- 
Nicolas Ferre

Re: [PATCH v2 1/2] dma: at_hdmac: Fix calculation of the residual bytes

[Nicolas Ferre]

Le 19/02/2015 18:07, Torsten Fleischer a écrit :
> From: Torsten Fleischer <torfl6749@gmail.com>
> 
> This patch fixes the following issues regarding to the calculation of the
> residue:
> 
> 1. The residue is always calculated for the current transfer even if the
> cookie is associated to a pending transfer.
> 
> 2. For scatter/gather DMA the calculation of the residue for the current
> transfer doesn't include the bytes of the child descriptors that are already
> transferred.
> It only calculates the difference between the transfer's total length minus
> the number of bytes that are already transferred for the current child
> descriptor.
> For example: There is a scatter/gather DMA transfer with a total length of
> 1 MByte. Getting the residue several times while the transfer is running shows
> something like that:
> 
> 1: residue = 975584
> 2: residue = 1002766
> 3: residue = 992627
> 4: residue = 983767
> 5: residue = 985694
> 6: residue = 1008094
> 7: residue = 1009741
> 8: residue = 1011195
> 
> 3. The driver stores the residue but never resets it when starting a new
> transfer.
> For example: If there are two subsequent DMA transfers. The first one with
> a total length of 1 MByte and the second one with a total length of 1 kByte.
> Getting the residue for both transfers shows something like that:
> 
> transfer 1: residue = 975584
> transfer 2: residue = 1048380
> 
> Changes from V1:
>    * Fixed coding style of the multi-line comments.
>    * Improved accuracy of the residue calculation when the transfer for the
>      first descriptor is active.
> 
> Signed-off-by: Torsten Fleischer <torfl6749@gmail.com>

Torsten,

Thanks a lot for your work and sorry for the delay.
I just do an additional review after Ludovic's one. BTW, I think we can
add his from the v1 of the series:
Acked-by: Ludovic Desroches <ludovic.desroches@atmel.com>

Anyway, I still have a few comments below...

> ---
>  drivers/dma/at_hdmac.c      | 155 +++++++++++++++++++++++---------------------
>  drivers/dma/at_hdmac_regs.h |  11 ++--
>  2 files changed, 87 insertions(+), 79 deletions(-)
> 
> diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
> index ca9dd26..f46d86d 100644
> --- a/drivers/dma/at_hdmac.c
> +++ b/drivers/dma/at_hdmac.c
> @@ -233,93 +233,104 @@ static void atc_dostart(struct at_dma_chan *atchan, struct at_desc *first)
>  }
>  
>  /*
> - * atc_get_current_descriptors -
> - * locate the descriptor which equal to physical address in DSCR
> - * @atchan: the channel we want to start
> - * @dscr_addr: physical descriptor address in DSCR
> + * atc_get_desc_by_cookie - get the descriptor of a cookie
> + * @atchan: the DMA channel
> + * @cookie: the cookie to get the descriptor for
>   */
> -static struct at_desc *atc_get_current_descriptors(struct at_dma_chan *atchan,
> -							u32 dscr_addr)
> +static struct at_desc *atc_get_desc_by_cookie(struct at_dma_chan *atchan,
> +						dma_cookie_t cookie)
>  {
> -	struct at_desc  *desc, *_desc, *child, *desc_cur = NULL;
> +	struct at_desc *desc, *_desc;
>  
> -	list_for_each_entry_safe(desc, _desc, &atchan->active_list, desc_node) {
> -		if (desc->lli.dscr == dscr_addr) {
> -			desc_cur = desc;
> -			break;
> -		}
> +	list_for_each_entry_safe(desc, _desc, &atchan->queue, desc_node) {
> +		if (desc->txd.cookie == cookie)
> +			return desc;
> +	}
>  
> -		list_for_each_entry(child, &desc->tx_list, desc_node) {
> -			if (child->lli.dscr == dscr_addr) {
> -				desc_cur = child;
> -				break;
> -			}
> -		}
> +	list_for_each_entry_safe(desc, _desc, &atchan->active_list, desc_node) {
> +		if (desc->txd.cookie == cookie)
> +			return desc;
>  	}
>  
> -	return desc_cur;
> +	return NULL;
>  }
>  
> -/*
> - * atc_get_bytes_left -
> - * Get the number of bytes residue in dma buffer,
> - * @chan: the channel we want to start
> +/**
> + * atc_get_bytes_left - get the number of bytes residue for a cookie
> + * @chan: DMA channel
> + * @cookie: transaction identifier to check status of
>   */
> -static int atc_get_bytes_left(struct dma_chan *chan)
> +static int atc_get_bytes_left(struct dma_chan *chan, dma_cookie_t cookie)
>  {
>  	struct at_dma_chan      *atchan = to_at_dma_chan(chan);
> -	struct at_dma           *atdma = to_at_dma(chan->device);
> -	int	chan_id = atchan->chan_common.chan_id;
>  	struct at_desc *desc_first = atc_first_active(atchan);
> -	struct at_desc *desc_cur;
> +	struct at_desc *desc;
>  	int ret = 0, count = 0;
> +	u32 ctrla, dscr;
>  
>  	/*
> -	 * Initialize necessary values in the first time.
> -	 * remain_desc record remain desc length.
> +	 * If the cookie doesn't match to the currently running transfer then
> +	 * we can return the total length of the associated DMA transfer,
> +	 * because it is still queued.
>  	 */
> -	if (atchan->remain_desc == 0)
> -		/* First descriptor embedds the transaction length */
> -		atchan->remain_desc = desc_first->len;
> +	desc = atc_get_desc_by_cookie(atchan, cookie);
> +	if (desc == NULL)
> +		return -EINVAL;
> +	else if (desc != desc_first)
> +		return desc->total_len;
>  
> -	/*
> -	 * This happens when current descriptor transfer complete.
> -	 * The residual buffer size should reduce current descriptor length.
> -	 */
> -	if (unlikely(test_bit(ATC_IS_BTC, &atchan->status))) {
> -		clear_bit(ATC_IS_BTC, &atchan->status);
> -		desc_cur = atc_get_current_descriptors(atchan,
> -						channel_readl(atchan, DSCR));
> -		if (!desc_cur) {
> -			ret = -EINVAL;
> -			goto out;
> +	/* cookie matches to the currently running transfer */
> +	ret = desc_first->total_len;
> +
> +	if (desc_first->lli.dscr) {
> +		/* hardware linked list transfer */
> +
> +		/*
> +		 * Calculate the residue by removing the length of the child
> +		 * descriptors already transferred from the total length.
> +		 * To get the current child descriptor we can use the value of
> +		 * the channel's DSCR register and compare it against the value
> +		 * of the hardware linked list structure of each child
> +		 * descriptor.
> +		 */
> +
> +		ctrla = channel_readl(atchan, CTRLA);
> +		rmb(); /* ensure CTRLA is read before DSCR */
> +		dscr = channel_readl(atchan, DSCR);
> +
> +		/* for the first descriptor we can be more accurate */
> +		if (desc_first->lli.dscr == dscr) {
> +			count = (ctrla & ATC_BTSIZE_MAX);
> +			ret -= count << ATC_REG_TO_SRC_WIDTH(ctrla);

I see a little issue here. How are you sure that we must use the *src*
width and not the *dst* width in this computation?

As the register width is different depending on the transfer direction,
I suspect the computation can be wrong in "slave_sg" case.
So, I would recommend to keep the "tx_width" property in the descriptor
and use it here.

Apart from that, it seems that this computation or variants of it are
done twice or maybe 3 times. So, do you think that we can extract a
function from this code?

> +			return ret;
>  		}
>  
> -		count = (desc_cur->lli.ctrla & ATC_BTSIZE_MAX)
> -			<< desc_first->tx_width;
> -		if (atchan->remain_desc < count) {
> -			ret = -EINVAL;
> -			goto out;
> +		ret -= desc_first->len;
> +		list_for_each_entry(desc, &desc_first->tx_list, desc_node) {
> +			if (desc->lli.dscr == dscr)
> +				break;
> +
> +			ret -= desc->len;
>  		}
>  
> -		atchan->remain_desc -= count;
> -		ret = atchan->remain_desc;
> -	} else {
>  		/*
> -		 * Get residual bytes when current
> -		 * descriptor transfer in progress.
> +		 * For the last descriptor in the chain we can calculate
> +		 * the remaining bytes using the channel's register.
>  		 */
> -		count = (channel_readl(atchan, CTRLA) & ATC_BTSIZE_MAX)
> -				<< (desc_first->tx_width);
> -		ret = atchan->remain_desc - count;
> +		if (!desc->lli.dscr) {
> +			ctrla = channel_readl(atchan, CTRLA);
> +			count = (desc->lli.ctrla & ATC_BTSIZE_MAX) -
> +				(ctrla & ATC_BTSIZE_MAX);
> +
> +			ret = count << ATC_REG_TO_SRC_WIDTH(ctrla);

Ditto.

> +		}
> +	} else {
> +		/* single transfer */
> +		ctrla = channel_readl(atchan, CTRLA);
> +		count = (ctrla & ATC_BTSIZE_MAX) << ATC_REG_TO_SRC_WIDTH(ctrla);

Ditto.

> +		ret -= count;
>  	}
> -	/*
> -	 * Check fifo empty.
> -	 */
> -	if (!(dma_readl(atdma, CHSR) & AT_DMA_EMPT(chan_id)))
> -		atc_issue_pending(chan);

Yes, I've never understood why it was needed.

>  
> -out:
>  	return ret;
>  }
>  
> @@ -534,8 +545,6 @@ static irqreturn_t at_dma_interrupt(int irq, void *dev_id)
>  					/* Give information to tasklet */
>  					set_bit(ATC_IS_ERROR, &atchan->status);
>  				}
> -				if (pending & AT_DMA_BTC(i))
> -					set_bit(ATC_IS_BTC, &atchan->status);
>  				tasklet_schedule(&atchan->tasklet);
>  				ret = IRQ_HANDLED;
>  			}
> @@ -648,14 +657,14 @@ atc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
>  		desc->lli.ctrlb = ctrlb;
>  
>  		desc->txd.cookie = 0;
> +		desc->len = xfer_count << src_width;
>  
>  		atc_desc_chain(&first, &prev, desc);
>  	}
>  
>  	/* First descriptor of the chain embedds additional information */
>  	first->txd.cookie = -EBUSY;
> -	first->len = len;
> -	first->tx_width = src_width;

Let's keep tx_width.

> +	first->total_len = len;
>  
>  	/* set end-of-link to the last link descriptor of list*/
>  	set_desc_eol(desc);
> @@ -747,6 +756,7 @@ atc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  					| ATC_SRC_WIDTH(mem_width)
>  					| len >> mem_width;
>  			desc->lli.ctrlb = ctrlb;
> +			desc->len = len;
>  
>  			atc_desc_chain(&first, &prev, desc);
>  			total_len += len;
> @@ -787,6 +797,7 @@ atc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  					| ATC_DST_WIDTH(mem_width)
>  					| len >> reg_width;
>  			desc->lli.ctrlb = ctrlb;
> +			desc->len = len;
>  
>  			atc_desc_chain(&first, &prev, desc);
>  			total_len += len;
> @@ -801,8 +812,7 @@ atc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  
>  	/* First descriptor of the chain embedds additional information */
>  	first->txd.cookie = -EBUSY;
> -	first->len = total_len;
> -	first->tx_width = reg_width;

Let's keep tx_width: this is where tx_width can be different depending
of the transfer direction.

> +	first->total_len = total_len;
>  
>  	/* first link descriptor of list is responsible of flags */
>  	first->txd.flags = flags; /* client is in control of this ack */
> @@ -867,6 +877,7 @@ atc_dma_cyclic_fill_desc(struct dma_chan *chan, struct at_desc *desc,
>  				| ATC_FC_MEM2PER
>  				| ATC_SIF(atchan->mem_if)
>  				| ATC_DIF(atchan->per_if);
> +		desc->len = period_len;
>  		break;
>  
>  	case DMA_DEV_TO_MEM:
> @@ -878,6 +889,7 @@ atc_dma_cyclic_fill_desc(struct dma_chan *chan, struct at_desc *desc,
>  				| ATC_FC_PER2MEM
>  				| ATC_SIF(atchan->per_if)
>  				| ATC_DIF(atchan->mem_if);
> +		desc->len = period_len;
>  		break;
>  
>  	default:
> @@ -959,8 +971,7 @@ atc_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
>  
>  	/* First descriptor of the chain embedds additional information */
>  	first->txd.cookie = -EBUSY;
> -	first->len = buf_len;
> -	first->tx_width = reg_width;

Here again, let's keep tx_width.

> +	first->total_len = buf_len;
>  
>  	return &first->txd;
>  
> @@ -1091,7 +1102,7 @@ atc_tx_status(struct dma_chan *chan,
>  	spin_lock_irqsave(&atchan->lock, flags);
>  
>  	/*  Get number of bytes left in the active transactions */
> -	bytes = atc_get_bytes_left(chan);
> +	bytes = atc_get_bytes_left(chan, cookie);
>  
>  	spin_unlock_irqrestore(&atchan->lock, flags);
>  
> @@ -1187,7 +1198,6 @@ static int atc_alloc_chan_resources(struct dma_chan *chan)
>  
>  	spin_lock_irqsave(&atchan->lock, flags);
>  	atchan->descs_allocated = i;
> -	atchan->remain_desc = 0;

Ok, this is not needed anymore: right.

>  	list_splice(&tmp_list, &atchan->free_list);
>  	dma_cookie_init(chan);
>  	spin_unlock_irqrestore(&atchan->lock, flags);
> @@ -1230,7 +1240,6 @@ static void atc_free_chan_resources(struct dma_chan *chan)
>  	list_splice_init(&atchan->free_list, &list);
>  	atchan->descs_allocated = 0;
>  	atchan->status = 0;
> -	atchan->remain_desc = 0;
>  
>  	dev_vdbg(chan2dev(chan), "free_chan_resources: done\n");
>  }
> diff --git a/drivers/dma/at_hdmac_regs.h b/drivers/dma/at_hdmac_regs.h
> index 2787aba..f458642 100644
> --- a/drivers/dma/at_hdmac_regs.h
> +++ b/drivers/dma/at_hdmac_regs.h
> @@ -112,11 +112,13 @@
>  #define		ATC_SRC_WIDTH_BYTE	(0x0 << 24)
>  #define		ATC_SRC_WIDTH_HALFWORD	(0x1 << 24)
>  #define		ATC_SRC_WIDTH_WORD	(0x2 << 24)
> +#define		ATC_REG_TO_SRC_WIDTH(x)	(((x) >> 24) & 0x3)

Here...

>  #define	ATC_DST_WIDTH_MASK	(0x3 << 28)	/* Destination Single Transfer Size */
>  #define		ATC_DST_WIDTH(x)	((x) << 28)
>  #define		ATC_DST_WIDTH_BYTE	(0x0 << 28)
>  #define		ATC_DST_WIDTH_HALFWORD	(0x1 << 28)
>  #define		ATC_DST_WIDTH_WORD	(0x2 << 28)
> +#define		ATC_REG_TO_DST_WIDTH(x)	(((x) >> 28) & 0x3)

... and here, well, I'm not sure it can be statically called: so maybe
these macros are not needed.


>  #define	ATC_DONE		(0x1 << 31)	/* Tx Done (only written back in descriptor) */
>  
>  /* Bitfields in CTRLB */
> @@ -181,8 +183,8 @@ struct at_lli {
>   * @at_lli: hardware lli structure
>   * @txd: support for the async_tx api
>   * @desc_node: node on the channed descriptors list
> - * @len: total transaction bytecount

Yes for the re-purpose of this.

> - * @tx_width: transfer width

Nack for this, sorry.


> + * @len: descriptor byte count

Ok.

> + * @total_len: total transaction byte count

Ok: this is clearer.

>   */
>  struct at_desc {
>  	/* FIRST values the hardware uses */
> @@ -193,7 +195,7 @@ struct at_desc {
>  	struct dma_async_tx_descriptor	txd;
>  	struct list_head		desc_node;
>  	size_t				len;
> -	u32				tx_width;
> +	size_t				total_len;
>  };
>  
>  static inline struct at_desc *
> @@ -213,7 +215,6 @@ txd_to_at_desc(struct dma_async_tx_descriptor *txd)
>  enum atc_status {
>  	ATC_IS_ERROR = 0,
>  	ATC_IS_PAUSED = 1,
> -	ATC_IS_BTC = 2,

Yes, it's seems not needed anymore.

>  	ATC_IS_CYCLIC = 24,
>  };
>  
> @@ -231,7 +232,6 @@ enum atc_status {
>   * @save_cfg: configuration register that is saved on suspend/resume cycle
>   * @save_dscr: for cyclic operations, preserve next descriptor address in
>   *             the cyclic list on suspend/resume cycle
> - * @remain_desc: to save remain desc length
>   * @dma_sconfig: configuration for slave transfers, passed via DMA_SLAVE_CONFIG
>   * @lock: serializes enqueue/dequeue operations to descriptors lists
>   * @active_list: list of descriptors dmaengine is being running on
> @@ -250,7 +250,6 @@ struct at_dma_chan {
>  	struct tasklet_struct	tasklet;
>  	u32			save_cfg;
>  	u32			save_dscr;
> -	u32			remain_desc;
>  	struct dma_slave_config dma_sconfig;
>  
>  	spinlock_t		lock;
> 


-- 
Nicolas Ferre

[PATCH v2 2/2] dma: at_hdmac: Add support for memory to memory sg transfers

[Torsten Fleischer]

From: Torsten Fleischer <torfl6749@gmail.com>

This patch adds support for memory to memory scatter-gather transfers.

Changes from V1:
   * Fixed coding style of the multi-line comments.

Signed-off-by: Torsten Fleischer <torfl6749@gmail.com>
---
 drivers/dma/at_hdmac.c | 169 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 158 insertions(+), 11 deletions(-)

diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
index f46d86d..6afc02b 100644
--- a/drivers/dma/at_hdmac.c
+++ b/drivers/dma/at_hdmac.c
@@ -60,6 +60,21 @@ static void atc_issue_pending(struct dma_chan *chan);
 
 /*----------------------------------------------------------------------*/
 
+static inline unsigned int atc_get_xfer_width(dma_addr_t src, dma_addr_t dst,
+						size_t len)
+{
+	unsigned int width;
+
+	if (!((src | dst  | len) & 3))
+		width = 2;
+	else if (!((src | dst | len) & 1))
+		width = 1;
+	else
+		width = 0;
+
+	return width;
+}
+
 static struct at_desc *atc_first_active(struct at_dma_chan *atchan)
 {
 	return list_first_entry(&atchan->active_list,
@@ -632,16 +647,10 @@ atc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
 	 * We can be a lot more clever here, but this should take care
 	 * of the most common optimization.
 	 */
-	if (!((src | dest  | len) & 3)) {
-		ctrla = ATC_SRC_WIDTH_WORD | ATC_DST_WIDTH_WORD;
-		src_width = dst_width = 2;
-	} else if (!((src | dest | len) & 1)) {
-		ctrla = ATC_SRC_WIDTH_HALFWORD | ATC_DST_WIDTH_HALFWORD;
-		src_width = dst_width = 1;
-	} else {
-		ctrla = ATC_SRC_WIDTH_BYTE | ATC_DST_WIDTH_BYTE;
-		src_width = dst_width = 0;
-	}
+	src_width = dst_width = atc_get_xfer_width(src, dest, len);
+
+	ctrla = ATC_SRC_WIDTH(src_width) |
+		ATC_DST_WIDTH(dst_width);
 
 	for (offset = 0; offset < len; offset += xfer_count << src_width) {
 		xfer_count = min_t(size_t, (len - offset) >> src_width,
@@ -827,6 +836,138 @@ err:
 }
 
 /**
+ * atc_prep_dma_sg - prepare memory to memory scather-gather operation
+ * @chan: the channel to prepare operation on
+ * @dst_sg: destination scatterlist
+ * @dst_nents: number of destination scatterlist entries
+ * @src_sg: source scatterlist
+ * @src_nents: number of source scatterlist entries
+ * @flags: tx descriptor status flags
+ */
+static struct dma_async_tx_descriptor *
+atc_prep_dma_sg(struct dma_chan *chan,
+		struct scatterlist *dst_sg, unsigned int dst_nents,
+		struct scatterlist *src_sg, unsigned int src_nents,
+		unsigned long flags)
+{
+	struct at_dma_chan	*atchan = to_at_dma_chan(chan);
+	struct at_desc		*desc = NULL;
+	struct at_desc		*first = NULL;
+	struct at_desc		*prev = NULL;
+	unsigned int		src_width;
+	unsigned int		dst_width;
+	size_t			xfer_count;
+	u32			ctrla;
+	u32			ctrlb;
+	size_t			dst_len = 0, src_len = 0;
+	dma_addr_t		dst = 0, src = 0;
+	size_t			len = 0, total_len = 0;
+
+	if (unlikely(dst_nents == 0 || src_nents == 0))
+		return NULL;
+
+	if (unlikely(dst_sg == NULL || src_sg == NULL))
+		return NULL;
+
+	ctrlb =   ATC_DEFAULT_CTRLB | ATC_IEN
+		| ATC_SRC_ADDR_MODE_INCR
+		| ATC_DST_ADDR_MODE_INCR
+		| ATC_FC_MEM2MEM;
+
+	/*
+	 * loop until there is either no more source or no more destination
+	 * scatterlist entry
+	 */
+	while (true) {
+
+		/* prepare the next transfer */
+		if (dst_len == 0) {
+
+			/* no more destination scatterlist entries */
+			if (!dst_sg || !dst_nents)
+				break;
+
+			dst = sg_dma_address(dst_sg);
+			dst_len = sg_dma_len(dst_sg);
+
+			dst_sg = sg_next(dst_sg);
+			dst_nents--;
+		}
+
+		if (src_len == 0) {
+
+			/* no more source scatterlist entries */
+			if (!src_sg || !src_nents)
+				break;
+
+			src = sg_dma_address(src_sg);
+			src_len = sg_dma_len(src_sg);
+
+			src_sg = sg_next(src_sg);
+			src_nents--;
+		}
+
+		len = min_t(size_t, src_len, dst_len);
+		if (len == 0)
+			continue;
+
+		/* take care for the alignment */
+		src_width = dst_width = atc_get_xfer_width(src, dst, len);
+
+		ctrla = ATC_SRC_WIDTH(src_width) |
+			ATC_DST_WIDTH(dst_width);
+
+		/*
+		 * The number of transfers to set up refer to the source width
+		 * that depends on the alignment.
+		 */
+		xfer_count = len >> src_width;
+		if (xfer_count > ATC_BTSIZE_MAX) {
+			xfer_count = ATC_BTSIZE_MAX;
+			len = ATC_BTSIZE_MAX << src_width;
+		}
+
+		/* create the transfer */
+		desc = atc_desc_get(atchan);
+		if (!desc)
+			goto err_desc_get;
+
+		desc->lli.saddr = src;
+		desc->lli.daddr = dst;
+		desc->lli.ctrla = ctrla | xfer_count;
+		desc->lli.ctrlb = ctrlb;
+
+		desc->txd.cookie = 0;
+		desc->len = len;
+
+		atc_desc_chain(&first, &prev, desc);
+
+		/* update the lengths and addresses for the next loop cycle */
+		dst_len -= len;
+		src_len -= len;
+		dst += len;
+		src += len;
+
+		total_len += len;
+	}
+
+	/* First descriptor of the chain embedds additional information */
+	first->txd.cookie = -EBUSY;
+	first->total_len = total_len;
+
+	/* set end-of-link to the last link descriptor of list*/
+	set_desc_eol(desc);
+
+	first->txd.flags = flags; /* client is in control of this ack */
+
+	return &first->txd;
+
+err_desc_get:
+	atc_desc_put(atchan, first);
+	return NULL;
+}
+
+/**
  * atc_dma_cyclic_check_values
  * Check for too big/unaligned periods and unaligned DMA buffer
  */
@@ -1403,8 +1544,10 @@ static int __init at_dma_probe(struct platform_device *pdev)
 
 	/* setup platform data for each SoC */
 	dma_cap_set(DMA_MEMCPY, at91sam9rl_config.cap_mask);
+	dma_cap_set(DMA_SG, at91sam9rl_config.cap_mask);
 	dma_cap_set(DMA_MEMCPY, at91sam9g45_config.cap_mask);
 	dma_cap_set(DMA_SLAVE, at91sam9g45_config.cap_mask);
+	dma_cap_set(DMA_SG, at91sam9g45_config.cap_mask);
 
 	/* get DMA parameters from controller type */
 	plat_dat = at_dma_get_driver_data(pdev);
@@ -1517,11 +1660,15 @@ static int __init at_dma_probe(struct platform_device *pdev)
 		atdma->dma_common.device_control = atc_control;
 	}
 
+	if (dma_has_cap(DMA_SG, atdma->dma_common.cap_mask))
+		atdma->dma_common.device_prep_dma_sg = atc_prep_dma_sg;
+
 	dma_writel(atdma, EN, AT_DMA_ENABLE);
 
-	dev_info(&pdev->dev, "Atmel AHB DMA Controller ( %s%s), %d channels\n",
+	dev_info(&pdev->dev, "Atmel AHB DMA Controller ( %s%s%s), %d channels\n",
 	  dma_has_cap(DMA_MEMCPY, atdma->dma_common.cap_mask) ? "cpy " : "",
 	  dma_has_cap(DMA_SLAVE, atdma->dma_common.cap_mask)  ? "slave " : "",
+	  dma_has_cap(DMA_SG, atdma->dma_common.cap_mask)  ? "sg-cpy " : "",
 	  plat_dat->nr_channels);
 
 	dma_async_device_register(&atdma->dma_common);
-- 
2.1.4