[PATCH v3 0/4] dw_dmac: return actual residue value

[PATCH v3 0/4] dw_dmac: return actual residue value

[Andy Shevchenko]

The patch series is targeted for getting proper residue value.

Since v2:
 - get rid of dwc_update_residue(): for soft LLP mode we assign residue in
   dwc_do_start() and decrease it at each interrupt. In the middle of transfer
   we will return that result substracted by amount of sent bytes
Since v1:
 - everything is rewritten to address Viresh's and Vinod's comments.

*** BLURB HERE ***

Andy Shevchenko (4):
  dw_dmac: remove unnecessary tx_list field in dw_dma_chan
  dw_dmac: introduce total_len field in struct dw_desc
  dw_dmac: fill individual length of descriptor
  dw_dmac: return proper residue value

 drivers/dma/dw_dmac.c      |   79 ++++++++++++++++++++++++++++++++++++--------
 drivers/dma/dw_dmac_regs.h |    3 +-
 2 files changed, 67 insertions(+), 15 deletions(-)

-- 
1.7.10.4

[PATCH v3 1/4] dw_dmac: remove unnecessary tx_list field in dw_dma_chan

[Andy Shevchenko]

The soft LLP mode is working for active descriptor only. So, we do not need to
have a copy of its pointer.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/dma/dw_dmac.c      |   20 +++++++++++++++-----
 drivers/dma/dw_dmac_regs.h |    1 -
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 72e6316..e4e4ff2 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -284,9 +284,9 @@ static void dwc_dostart(struct dw_dma_chan *dwc, struct dw_desc *first)
 
 		dwc_initialize(dwc);
 
-		dwc->tx_list = &first->tx_list;
 		dwc->tx_node_active = &first->tx_list;
 
+		/* Submit first block */
 		dwc_do_single_block(dwc, first);
 
 		return;
@@ -404,15 +404,25 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 		dma_writel(dw, CLEAR.XFER, dwc->mask);
 
 		if (test_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags)) {
-			if (dwc->tx_node_active != dwc->tx_list) {
-				desc = to_dw_desc(dwc->tx_node_active);
+			struct list_head *head, *active = dwc->tx_node_active;
+
+			/*
+			 * We are inside first active descriptor.
+			 * Otherwise something is really wrong.
+			 */
+			desc = dwc_first_active(dwc);
+
+			head = &desc->tx_list;
+			if (active != head) {
+				child = to_dw_desc(active);
 
 				/* Submit next block */
-				dwc_do_single_block(dwc, desc);
-				spin_unlock_irqrestore(&dwc->lock, flags);
+				dwc_do_single_block(dwc, child);
 
+				spin_unlock_irqrestore(&dwc->lock, flags);
 				return;
 			}
+
 			/* We are done here */
 			clear_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags);
 		}
diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw_dmac_regs.h
index fef296d..13000d2 100644
--- a/drivers/dma/dw_dmac_regs.h
+++ b/drivers/dma/dw_dmac_regs.h
@@ -194,7 +194,6 @@ struct dw_dma_chan {
 	bool				initialized;
 
 	/* software emulation of the LLP transfers */
-	struct list_head	*tx_list;
 	struct list_head	*tx_node_active;
 
 	spinlock_t		lock;
-- 
1.7.10.4

[PATCH v3 2/4] dw_dmac: introduce total_len field in struct dw_desc

[Andy Shevchenko]

By this new field we distinguish a total length of the chain and the individual
length of each descriptor in the chain.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/dma/dw_dmac.c      |   12 ++++++------
 drivers/dma/dw_dmac_regs.h |    1 +
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index e4e4ff2..7b5a088 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -335,18 +335,18 @@ dwc_descriptor_complete(struct dw_dma_chan *dwc, struct dw_desc *desc,
 		if (!(txd->flags & DMA_COMPL_SKIP_DEST_UNMAP)) {
 			if (txd->flags & DMA_COMPL_DEST_UNMAP_SINGLE)
 				dma_unmap_single(parent, desc->lli.dar,
-						desc->len, DMA_FROM_DEVICE);
+					desc->total_len, DMA_FROM_DEVICE);
 			else
 				dma_unmap_page(parent, desc->lli.dar,
-						desc->len, DMA_FROM_DEVICE);
+					desc->total_len, DMA_FROM_DEVICE);
 		}
 		if (!(txd->flags & DMA_COMPL_SKIP_SRC_UNMAP)) {
 			if (txd->flags & DMA_COMPL_SRC_UNMAP_SINGLE)
 				dma_unmap_single(parent, desc->lli.sar,
-						desc->len, DMA_TO_DEVICE);
+					desc->total_len, DMA_TO_DEVICE);
 			else
 				dma_unmap_page(parent, desc->lli.sar,
-						desc->len, DMA_TO_DEVICE);
+					desc->total_len, DMA_TO_DEVICE);
 		}
 	}
 
@@ -776,7 +776,7 @@ dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
 
 	prev->lli.llp = 0;
 	first->txd.flags = flags;
-	first->len = len;
+	first->total_len = len;
 
 	return &first->txd;
 
@@ -939,7 +939,7 @@ slave_sg_fromdev_fill_desc:
 		prev->lli.ctllo |= DWC_CTLL_INT_EN;
 
 	prev->lli.llp = 0;
-	first->len = total_len;
+	first->total_len = total_len;
 
 	return &first->txd;
 
diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw_dmac_regs.h
index 13000d2..833b4cf 100644
--- a/drivers/dma/dw_dmac_regs.h
+++ b/drivers/dma/dw_dmac_regs.h
@@ -296,6 +296,7 @@ struct dw_desc {
 	struct list_head		tx_list;
 	struct dma_async_tx_descriptor	txd;
 	size_t				len;
+	size_t				total_len;
 };
 
 #define to_dw_desc(h)	list_entry(h, struct dw_desc, desc_node)
-- 
1.7.10.4

[PATCH v3 3/4] dw_dmac: fill individual length of descriptor

[Andy Shevchenko]

It will be useful to have the length of the transfer in the descriptor. The
cyclic transfer functions remained untouched.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/dma/dw_dmac.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 7b5a088..3b788c8 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -759,6 +759,7 @@ dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
 		desc->lli.dar = dest + offset;
 		desc->lli.ctllo = ctllo;
 		desc->lli.ctlhi = xfer_count;
+		desc->len = xfer_count << src_width;
 
 		if (!first) {
 			first = desc;
@@ -857,6 +858,7 @@ slave_sg_todev_fill_desc:
 			}
 
 			desc->lli.ctlhi = dlen >> mem_width;
+			desc->len = dlen;
 
 			if (!first) {
 				first = desc;
@@ -915,6 +917,7 @@ slave_sg_fromdev_fill_desc:
 				len = 0;
 			}
 			desc->lli.ctlhi = dlen >> reg_width;
+			desc->len = dlen;
 
 			if (!first) {
 				first = desc;
-- 
1.7.10.4

[PATCH v3 4/4] dw_dmac: return proper residue value

[Andy Shevchenko]

Currently the driver returns full length of the active descriptor which is
wrong. We have to go throught the active descriptor and substract the length of
each sent children in the chain from the total length along with the actual
data in the DMA channel registers.

The cyclic case is not handled by this patch due to len field in the descriptor
structure is left untouched by the original code.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dw_dmac.c      |   44 +++++++++++++++++++++++++++++++++++++++++---
 drivers/dma/dw_dmac_regs.h |    1 +
 2 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 3b788c8..3925318 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -284,6 +284,7 @@ static void dwc_dostart(struct dw_dma_chan *dwc, struct dw_desc *first)
 
 		dwc_initialize(dwc);
 
+		dwc->residue = first->total_len;
 		dwc->tx_node_active = &first->tx_list;
 
 		/* Submit first block */
@@ -387,6 +388,15 @@ static void dwc_complete_all(struct dw_dma *dw, struct dw_dma_chan *dwc)
 		dwc_descriptor_complete(dwc, desc, true);
 }
 
+/* Returns how many bytes were already received from source */
+static inline u32 dwc_get_sent(struct dw_dma_chan *dwc)
+{
+	u32 ctlhi = channel_readl(dwc, CTL_HI);
+	u32 ctllo = channel_readl(dwc, CTL_LO);
+
+	return (ctlhi & DWC_CTLH_BLOCK_TS_MASK) * (1 << (ctllo >> 4 & 7));
+}
+
 static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 {
 	dma_addr_t llp;
@@ -414,6 +424,12 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 
 			head = &desc->tx_list;
 			if (active != head) {
+				if (active == head->next)
+					dwc->residue -= desc->len;
+				else
+					dwc->residue -=
+						to_dw_desc(active->prev)->len;
+
 				child = to_dw_desc(active);
 
 				/* Submit next block */
@@ -426,6 +442,9 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 			/* We are done here */
 			clear_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags);
 		}
+
+		dwc->residue = 0;
+
 		spin_unlock_irqrestore(&dwc->lock, flags);
 
 		dwc_complete_all(dw, dwc);
@@ -433,6 +452,7 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 	}
 
 	if (list_empty(&dwc->active_list)) {
+		dwc->residue = 0;
 		spin_unlock_irqrestore(&dwc->lock, flags);
 		return;
 	}
@@ -447,6 +467,9 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 			(unsigned long long)llp);
 
 	list_for_each_entry_safe(desc, _desc, &dwc->active_list, desc_node) {
+		/* initial residue value */
+		dwc->residue = desc->total_len;
+
 		/* check first descriptors addr */
 		if (desc->txd.phys == llp) {
 			spin_unlock_irqrestore(&dwc->lock, flags);
@@ -456,16 +479,21 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 		/* check first descriptors llp */
 		if (desc->lli.llp == llp) {
 			/* This one is currently in progress */
+			dwc->residue -= dwc_get_sent(dwc);
 			spin_unlock_irqrestore(&dwc->lock, flags);
 			return;
 		}
 
-		list_for_each_entry(child, &desc->tx_list, desc_node)
+		dwc->residue -= desc->len;
+		list_for_each_entry(child, &desc->tx_list, desc_node) {
 			if (child->lli.llp == llp) {
 				/* Currently in progress */
+				dwc->residue -= dwc_get_sent(dwc);
 				spin_unlock_irqrestore(&dwc->lock, flags);
 				return;
 			}
+			dwc->residue -= child->len;
+		}
 
 		/*
 		 * No descriptors so far seem to be in progress, i.e.
@@ -1062,6 +1090,7 @@ dwc_tx_status(struct dma_chan *chan,
 	      struct dma_tx_state *txstate)
 {
 	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
+	unsigned long		flags;
 	enum dma_status		ret;
 
 	ret = dma_cookie_status(chan, cookie, txstate);
@@ -1071,8 +1100,17 @@ dwc_tx_status(struct dma_chan *chan,
 		ret = dma_cookie_status(chan, cookie, txstate);
 	}
 
-	if (ret != DMA_SUCCESS)
-		dma_set_residue(txstate, dwc_first_active(dwc)->len);
+	spin_lock_irqsave(&dwc->lock, flags);
+
+	if (ret != DMA_SUCCESS) {
+		u32 residue = dwc->residue;
+
+		if (test_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags) && residue)
+			residue -= dwc_get_sent(dwc);
+		dma_set_residue(txstate, residue);
+	}
+
+	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	if (dwc->paused)
 		return DMA_PAUSED;
diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw_dmac_regs.h
index 833b4cf..88dd8eb 100644
--- a/drivers/dma/dw_dmac_regs.h
+++ b/drivers/dma/dw_dmac_regs.h
@@ -203,6 +203,7 @@ struct dw_dma_chan {
 	struct list_head	active_list;
 	struct list_head	queue;
 	struct list_head	free_list;
+	u32			residue;
 	struct dw_cyclic_desc	*cdesc;
 
 	unsigned int		descs_allocated;
-- 
1.7.10.4

Re: [PATCH v3 4/4] dw_dmac: return proper residue value

[Viresh Kumar]

I told you, we can get rid of that routine :)

On Thu, Jan 24, 2013 at 4:04 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

I don't see residue - len from interrupt handler. It isn't required?

> @@ -1062,6 +1090,7 @@ dwc_tx_status(struct dma_chan *chan,
>               struct dma_tx_state *txstate)
>  {
>         struct dw_dma_chan      *dwc = to_dw_dma_chan(chan);
> +       unsigned long           flags;
>         enum dma_status         ret;
>
>         ret = dma_cookie_status(chan, cookie, txstate);
> @@ -1071,8 +1100,17 @@ dwc_tx_status(struct dma_chan *chan,
>                 ret = dma_cookie_status(chan, cookie, txstate);
>         }
>
> -       if (ret != DMA_SUCCESS)
> -               dma_set_residue(txstate, dwc_first_active(dwc)->len);
> +       spin_lock_irqsave(&dwc->lock, flags);
> +
> +       if (ret != DMA_SUCCESS) {
> +               u32 residue = dwc->residue;

If you agree with the explanation that i gave in last mail, you must drop lock
right here. and also take it in this if block.

> +               if (test_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags) && residue)
> +                       residue -= dwc_get_sent(dwc);
> +               dma_set_residue(txstate, residue);
> +       }
> +
> +       spin_unlock_irqrestore(&dwc->lock, flags);

other than that:

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [PATCH v3 4/4] dw_dmac: return proper residue value

[Andy Shevchenko]

On Thu, Jan 24, 2013 at 12:45 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On Thu, Jan 24, 2013 at 4:04 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>
> I don't see residue - len from interrupt handler. It isn't required?

Where exactly?

>> @@ -1062,6 +1090,7 @@ dwc_tx_status(struct dma_chan *chan,
>>               struct dma_tx_state *txstate)
>>  {
>>         struct dw_dma_chan      *dwc = to_dw_dma_chan(chan);
>> +       unsigned long           flags;
>>         enum dma_status         ret;
>>
>>         ret = dma_cookie_status(chan, cookie, txstate);
>> @@ -1071,8 +1100,17 @@ dwc_tx_status(struct dma_chan *chan,
>>                 ret = dma_cookie_status(chan, cookie, txstate);
>>         }
>>
>> -       if (ret != DMA_SUCCESS)
>> -               dma_set_residue(txstate, dwc_first_active(dwc)->len);
>> +       spin_lock_irqsave(&dwc->lock, flags);
>> +
>> +       if (ret != DMA_SUCCESS) {
>> +               u32 residue = dwc->residue;
>
> If you agree with the explanation that i gave in last mail, you must drop lock
> right here. and also take it in this if block.

Yes, I agreed with that. Okay, I will move lock inside this block.

>> +               if (test_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags) && residue)
>> +                       residue -= dwc_get_sent(dwc);
>> +               dma_set_residue(txstate, residue);
>> +       }
>> +
>> +       spin_unlock_irqrestore(&dwc->lock, flags);
>
> other than that:
>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko

[PATCH v3.5] dw_dmac: return proper residue value

[Andy Shevchenko]

Currently the driver returns full length of the active descriptor which is
wrong. We have to go throught the active descriptor and substract the length of
each sent children in the chain from the total length along with the actual
data in the DMA channel registers.

The cyclic case is not handled by this patch due to len field in the descriptor
structure is left untouched by the original code.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dw_dmac.c      |   47 ++++++++++++++++++++++++++++++++++++++++++--
 drivers/dma/dw_dmac_regs.h |    1 +
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 3b788c8..b5c3f3a 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -284,6 +284,7 @@ static void dwc_dostart(struct dw_dma_chan *dwc, struct dw_desc *first)
 
 		dwc_initialize(dwc);
 
+		dwc->residue = first->total_len;
 		dwc->tx_node_active = &first->tx_list;
 
 		/* Submit first block */
@@ -387,6 +388,15 @@ static void dwc_complete_all(struct dw_dma *dw, struct dw_dma_chan *dwc)
 		dwc_descriptor_complete(dwc, desc, true);
 }
 
+/* Returns how many bytes were already received from source */
+static inline u32 dwc_get_sent(struct dw_dma_chan *dwc)
+{
+	u32 ctlhi = channel_readl(dwc, CTL_HI);
+	u32 ctllo = channel_readl(dwc, CTL_LO);
+
+	return (ctlhi & DWC_CTLH_BLOCK_TS_MASK) * (1 << (ctllo >> 4 & 7));
+}
+
 static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 {
 	dma_addr_t llp;
@@ -414,6 +424,12 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 
 			head = &desc->tx_list;
 			if (active != head) {
+				/* Update desc to reflect last sent one */
+				if (active != head->next)
+					desc = to_dw_desc(active->prev);
+
+				dwc->residue -= desc->len;
+
 				child = to_dw_desc(active);
 
 				/* Submit next block */
@@ -426,6 +442,9 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 			/* We are done here */
 			clear_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags);
 		}
+
+		dwc->residue = 0;
+
 		spin_unlock_irqrestore(&dwc->lock, flags);
 
 		dwc_complete_all(dw, dwc);
@@ -433,6 +452,7 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 	}
 
 	if (list_empty(&dwc->active_list)) {
+		dwc->residue = 0;
 		spin_unlock_irqrestore(&dwc->lock, flags);
 		return;
 	}
@@ -447,6 +467,9 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 			(unsigned long long)llp);
 
 	list_for_each_entry_safe(desc, _desc, &dwc->active_list, desc_node) {
+		/* initial residue value */
+		dwc->residue = desc->total_len;
+
 		/* check first descriptors addr */
 		if (desc->txd.phys == llp) {
 			spin_unlock_irqrestore(&dwc->lock, flags);
@@ -456,16 +479,21 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 		/* check first descriptors llp */
 		if (desc->lli.llp == llp) {
 			/* This one is currently in progress */
+			dwc->residue -= dwc_get_sent(dwc);
 			spin_unlock_irqrestore(&dwc->lock, flags);
 			return;
 		}
 
-		list_for_each_entry(child, &desc->tx_list, desc_node)
+		dwc->residue -= desc->len;
+		list_for_each_entry(child, &desc->tx_list, desc_node) {
 			if (child->lli.llp == llp) {
 				/* Currently in progress */
+				dwc->residue -= dwc_get_sent(dwc);
 				spin_unlock_irqrestore(&dwc->lock, flags);
 				return;
 			}
+			dwc->residue -= child->len;
+		}
 
 		/*
 		 * No descriptors so far seem to be in progress, i.e.
@@ -1056,6 +1084,21 @@ static int dwc_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 	return 0;
 }
 
+static inline u32 dwc_get_residue(struct dw_dma_chan *dwc)
+{
+	unsigned long flags;
+	u32 residue;
+
+	spin_lock_irqsave(&dwc->lock, flags);
+
+	residue = dwc->residue;
+	if (test_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags) && residue)
+		residue -= dwc_get_sent(dwc);
+
+	spin_unlock_irqrestore(&dwc->lock, flags);
+	return residue;
+}
+
 static enum dma_status
 dwc_tx_status(struct dma_chan *chan,
 	      dma_cookie_t cookie,
@@ -1072,7 +1115,7 @@ dwc_tx_status(struct dma_chan *chan,
 	}
 
 	if (ret != DMA_SUCCESS)
-		dma_set_residue(txstate, dwc_first_active(dwc)->len);
+		dma_set_residue(txstate, dwc_get_residue(dwc));
 
 	if (dwc->paused)
 		return DMA_PAUSED;
diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw_dmac_regs.h
index 833b4cf..88dd8eb 100644
--- a/drivers/dma/dw_dmac_regs.h
+++ b/drivers/dma/dw_dmac_regs.h
@@ -203,6 +203,7 @@ struct dw_dma_chan {
 	struct list_head	active_list;
 	struct list_head	queue;
 	struct list_head	free_list;
+	u32			residue;
 	struct dw_cyclic_desc	*cdesc;
 
 	unsigned int		descs_allocated;
-- 
1.7.10.4

Re: [PATCH v3.5] dw_dmac: return proper residue value

[Viresh Kumar]

On Thu, Jan 24, 2013 at 6:37 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Currently the driver returns full length of the active descriptor which is
> wrong. We have to go throught the active descriptor and substract the length of
> each sent children in the chain from the total length along with the actual
> data in the DMA channel registers.
>
> The cyclic case is not handled by this patch due to len field in the descriptor
> structure is left untouched by the original code.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Firstly the issue i raised earlier about not updating residue from
tasklet or interrupt
handler was wrong. As i had an older version of code in my mind. This got solved
with following patch:

commit 77bcc497c60ec62dbb84abc809a6e218d53409e9
Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date:   Fri Jan 18 14:14:15 2013 +0200

    dw_dmac: move soft LLP code from tasklet to dwc_scan_descriptors


> ---
> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> +static inline u32 dwc_get_residue(struct dw_dma_chan *dwc)
> +{
> +       unsigned long flags;
> +       u32 residue;
> +
> +       spin_lock_irqsave(&dwc->lock, flags);
> +
> +       residue = dwc->residue;

you need to release the lock just here.

> +       if (test_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags) && residue)
> +               residue -= dwc_get_sent(dwc);

why do you need lock while reading the control registers? It looks you didn't
get what i wanted to say earlier. We are using locks to protect some part for
shared data or critical sections. these are playing with dwc transfer
lists, etc..

Probably we don't need a lock to read the control register as nobody can
guarantee that another access is not happening currently. As hardware is
changing this register continuously for the transfer.

Let me know if i am missing something.

> +       spin_unlock_irqrestore(&dwc->lock, flags);
> +       return residue;
> +}

Re: [PATCH v3.5] dw_dmac: return proper residue value

[Andy Shevchenko]

On Fri, 2013-01-25 at 09:43 +0530, Viresh Kumar wrote: 
> On Thu, Jan 24, 2013 at 6:37 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > Currently the driver returns full length of the active descriptor which is
> > wrong. We have to go throught the active descriptor and substract the length of
> > each sent children in the chain from the total length along with the actual
> > data in the DMA channel registers.
> >
> > The cyclic case is not handled by this patch due to len field in the descriptor
> > structure is left untouched by the original code.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Firstly the issue i raised earlier about not updating residue from
> tasklet or interrupt
> handler was wrong. As i had an older version of code in my mind. This got solved
> with following patch:
> 
> commit 77bcc497c60ec62dbb84abc809a6e218d53409e9
> Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Date:   Fri Jan 18 14:14:15 2013 +0200
> 
>     dw_dmac: move soft LLP code from tasklet to dwc_scan_descriptors
> 
> 
> > ---
> > diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> > +static inline u32 dwc_get_residue(struct dw_dma_chan *dwc)
> > +{
> > +       unsigned long flags;
> > +       u32 residue;
> > +
> > +       spin_lock_irqsave(&dwc->lock, flags);
> > +
> > +       residue = dwc->residue;
> 
> you need to release the lock just here.
> 
> > +       if (test_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags) && residue)
> > +               residue -= dwc_get_sent(dwc);
> 
> why do you need lock while reading the control registers? It looks you didn't
> get what i wanted to say earlier. We are using locks to protect some part for
> shared data or critical sections. these are playing with dwc transfer
> lists, etc..
> 
> Probably we don't need a lock to read the control register as nobody can
> guarantee that another access is not happening currently. As hardware is
> changing this register continuously for the transfer.
> 
> Let me know if i am missing something.

I'm trying to find a proof of any of our opinions.
My intuitive understanding that we have to avoid a concurrency during hw
access.

> 
> > +       spin_unlock_irqrestore(&dwc->lock, flags);
> > +       return residue;
> > +}

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3.5] dw_dmac: return proper residue value

[Andy Shevchenko]

On Fri, Jan 25, 2013 at 10:56 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Fri, 2013-01-25 at 09:43 +0530, Viresh Kumar wrote:
>> On Thu, Jan 24, 2013 at 6:37 PM, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:

>> > diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
>> > +static inline u32 dwc_get_residue(struct dw_dma_chan *dwc)
>> > +{
>> > +       unsigned long flags;
>> > +       u32 residue;
>> > +
>> > +       spin_lock_irqsave(&dwc->lock, flags);
>> > +
>> > +       residue = dwc->residue;
>>
>> you need to release the lock just here.
>>
>> > +       if (test_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags) && residue)
>> > +               residue -= dwc_get_sent(dwc);
>>
>> why do you need lock while reading the control registers? It looks you didn't
>> get what i wanted to say earlier. We are using locks to protect some part for
>> shared data or critical sections. these are playing with dwc transfer
>> lists, etc..
>>
>> Probably we don't need a lock to read the control register as nobody can
>> guarantee that another access is not happening currently. As hardware is
>> changing this register continuously for the transfer.
>>
>> Let me know if i am missing something.

Okay, we have to have a protection here because get_sent reads two
registers consequentially. This means we could end up with scenario
with threads 1 and 2

1. read ctlhi
2. write ctlhi
2. write ctllo
1. read ctllo


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3.5] dw_dmac: return proper residue value

[Viresh Kumar]

On 25 January 2013 14:34, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> Okay, we have to have a protection here because get_sent reads two
> registers consequentially. This means we could end up with scenario
> with threads 1 and 2
>
> 1. read ctlhi
> 2. write ctlhi
> 2. write ctllo
> 1. read ctllo

Who is going to right on ctlhi/lo? we write to ctlhi/lo only when we program new
transfer. and that is not going to happen while we are in middle of a transfer.

Re: [PATCH v3.5] dw_dmac: return proper residue value

[Andy Shevchenko]

On Fri, Jan 25, 2013 at 11:07 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 25 January 2013 14:34, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> Okay, we have to have a protection here because get_sent reads two
>> registers consequentially. This means we could end up with scenario
>> with threads 1 and 2
>>
>> 1. read ctlhi
>> 2. write ctlhi
>> 2. write ctllo
>> 1. read ctllo
>
> Who is going to right on ctlhi/lo?

dwc_do_single_block()

> we write to ctlhi/lo only when we program new
> transfer. and that is not going to happen while we are in middle of a transfer.

We have got a tasklet running inside tx_status call. Isn't possible?
tasklet runs scan_descriptors, that continues transfer in soft LLP mode.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3.5] dw_dmac: return proper residue value

[Viresh Kumar]

On Fri, Jan 25, 2013 at 2:49 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Jan 25, 2013 at 11:07 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> Who is going to right on ctlhi/lo?

Ahh, my English :( (/s/right/write)

> dwc_do_single_block()
>
>> we write to ctlhi/lo only when we program new
>> transfer. and that is not going to happen while we are in middle of a transfer.
>
> We have got a tasklet running inside tx_status call. Isn't possible?
> tasklet runs scan_descriptors, that continues transfer in soft LLP mode.

But we have just executed scan_descriptor() before this function and it doesn't
look possible that transfer wasn't over then and inbetween these calls we got an
interrupt, scheduled an tasklet and called dwc_do_single_block() :)

Re: [PATCH v3.5] dw_dmac: return proper residue value

[Andy Shevchenko]

On Fri, Jan 25, 2013 at 11:30 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On Fri, Jan 25, 2013 at 2:49 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Fri, Jan 25, 2013 at 11:07 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>> Who is going to right on ctlhi/lo?
>
> Ahh, my English :( (/s/right/write)
>
>> dwc_do_single_block()
>>
>>> we write to ctlhi/lo only when we program new
>>> transfer. and that is not going to happen while we are in middle of a transfer.
>>
>> We have got a tasklet running inside tx_status call. Isn't possible?
>> tasklet runs scan_descriptors, that continues transfer in soft LLP mode.
>
> But we have just executed scan_descriptor() before this function and it doesn't
> look possible that transfer wasn't over then and inbetween these calls we got an
> interrupt, scheduled an tasklet and called dwc_do_single_block() :)

Yeah, the keyword is "look". 1 per million cases it could be true. I
think this discussion is going to the dead end.
Anyone else would like to argue for one or the other opinion?

I might obey your way with the commentary that with quite lower
possibility we could have an issue there.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3.5] dw_dmac: return proper residue value

[Viresh Kumar]

On 25 January 2013 15:07, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> Yeah, the keyword is "look". 1 per million cases it could be true. I
> think this discussion is going to the dead end.
> Anyone else would like to argue for one or the other opinion?
>
> I might obey your way with the commentary that with quite lower
> possibility we could have an issue there.

:)
Okay, lets keep your version.

@Vinod: you are free to pick these patches now ;)

Re: [PATCH v3.5] dw_dmac: return proper residue value

[Andy Shevchenko]

On Fri, Jan 25, 2013 at 11:39 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 25 January 2013 15:07, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> Yeah, the keyword is "look". 1 per million cases it could be true. I
>> think this discussion is going to the dead end.
>> Anyone else would like to argue for one or the other opinion?
>>
>> I might obey your way with the commentary that with quite lower
>> possibility we could have an issue there.
>
> :)
> Okay, lets keep your version.
>
> @Vinod: you are free to pick these patches now ;)

I do patch v4 to gather everything together.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/4] dw_dmac: return actual residue value

[Viresh Kumar]

On Thu, Jan 24, 2013 at 4:04 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Since v2:
>  - get rid of dwc_update_residue(): for soft LLP mode we assign residue in
>    dwc_do_start() and decrease it at each interrupt.

I was talking about this comment or design we thought of. Where is this done.
I see residue being updated only from scan_descriptors().