[PATCH v2 0/4] dw_dmac: return proper residue value

[PATCH v2 0/4] dw_dmac: return proper residue value

[Andy Shevchenko]

The patch series is targeted for getting proper residue value.

Since v1:
 - everything is rewritten to address Viresh's and Vinod's comments.

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      |   86 ++++++++++++++++++++++++++++++++++++--------
 drivers/dma/dw_dmac_regs.h |    3 +-
 2 files changed, 74 insertions(+), 15 deletions(-)

-- 
1.7.10.4

[PATCH v2 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>
---
 drivers/dma/dw_dmac.c      |   17 ++++++++++++-----
 drivers/dma/dw_dmac_regs.h |    1 -
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 72e6316..82e7c58 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,22 @@ 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);
+			/*
+			 * We are inside first active descriptor.
+			 * Otherwise something is really wrong.
+			 */
+			desc = dwc_first_active(dwc);
+
+			if (dwc->tx_node_active != &desc->tx_list) {
+				child = to_dw_desc(dwc->tx_node_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 v2 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>
---
 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 82e7c58..c3062a5 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);
 		}
 	}
 
@@ -773,7 +773,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;
 
@@ -936,7 +936,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 v2 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>
---
 drivers/dma/dw_dmac.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index c3062a5..026a3c7 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -756,6 +756,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;
@@ -852,8 +853,8 @@ slave_sg_todev_fill_desc:
 				dlen = len;
 				len = 0;
 			}
-
 			desc->lli.ctlhi = dlen >> mem_width;
+			desc->len = dlen;
 
 			if (!first) {
 				first = desc;
@@ -912,6 +913,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 v2 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      |   53 ++++++++++++++++++++++++++++++++++++++++++--
 drivers/dma/dw_dmac_regs.h |    1 +
 2 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 026a3c7..639d4cc 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -387,6 +387,35 @@ 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_update_residue(struct dw_dma_chan *dwc, struct dw_desc *desc)
+{
+	struct list_head *head = &desc->tx_list, *active;
+
+	/* First node is active */
+	if (dwc->tx_node_active == head->next) {
+		dwc->residue = desc->total_len - dwc_get_sent(dwc);
+		return;
+	}
+
+	/* Somewhere in the middle */
+	dwc->residue = desc->total_len - desc->len;
+	list_for_each(active, head) {
+		if (active == dwc->tx_node_active->prev)
+			break;
+		dwc->residue -= to_dw_desc(active)->len;
+	}
+	dwc->residue -= dwc_get_sent(dwc);
+}
+
 static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 {
 	dma_addr_t llp;
@@ -399,6 +428,8 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 	llp = channel_readl(dwc, LLP);
 	status_xfer = dma_readl(dw, RAW.XFER);
 
+	dwc->residue = 0;
+
 	if (status_xfer & dwc->mask) {
 		/* Everything we've submitted is done */
 		dma_writel(dw, CLEAR.XFER, dwc->mask);
@@ -410,6 +441,8 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 			 */
 			desc = dwc_first_active(dwc);
 
+			dwc_update_residue(dwc, desc);
+
 			if (dwc->tx_node_active != &desc->tx_list) {
 				child = to_dw_desc(dwc->tx_node_active);
 
@@ -436,6 +469,9 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 
 	if (test_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags)) {
 		dev_vdbg(chan2dev(&dwc->chan), "%s: soft LLP mode\n", __func__);
+
+		dwc_update_residue(dwc, dwc_first_active(dwc));
+
 		spin_unlock_irqrestore(&dwc->lock, flags);
 		return;
 	}
@@ -444,6 +480,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);
@@ -453,16 +492,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.
@@ -1058,6 +1102,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);
@@ -1067,8 +1112,12 @@ dwc_tx_status(struct dma_chan *chan,
 		ret = dma_cookie_status(chan, cookie, txstate);
 	}
 
+	spin_lock_irqsave(&dwc->lock, flags);
+
 	if (ret != DMA_SUCCESS)
-		dma_set_residue(txstate, dwc_first_active(dwc)->len);
+		dma_set_residue(txstate, dwc->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 v2 1/4] dw_dmac: remove unnecessary tx_list field in dw_dma_chan

[Viresh Kumar]

On Wed, Jan 23, 2013 at 9:07 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> 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>

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

[Viresh Kumar]

On Wed, Jan 23, 2013 at 9:07 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> 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>

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

[Viresh Kumar]

On Wed, Jan 23, 2013 at 9:07 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> 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>
> ---
>  drivers/dma/dw_dmac.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> index c3062a5..026a3c7 100644
> --- a/drivers/dma/dw_dmac.c
> +++ b/drivers/dma/dw_dmac.c
> @@ -756,6 +756,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;
> @@ -852,8 +853,8 @@ slave_sg_todev_fill_desc:
>                                 dlen = len;
>                                 len = 0;
>                         }
> -

:(

>                         desc->lli.ctlhi = dlen >> mem_width;
> +                       desc->len = dlen;
>
>                         if (!first) {
>                                 first = desc;
> @@ -912,6 +913,7 @@ slave_sg_fromdev_fill_desc:
>                                 len = 0;
>                         }
>                         desc->lli.ctlhi = dlen >> reg_width;
> +                       desc->len = dlen;
>
>                         if (!first) {
>                                 first = desc;

After fixing that.

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

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

[Viresh Kumar]

On Wed, Jan 23, 2013 at 9:07 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c

>  static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
>  {
>         dma_addr_t llp;
> @@ -410,6 +441,8 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
>                          */
>                         desc = dwc_first_active(dwc);
>
> +                       dwc_update_residue(dwc, desc);
> +
>                         if (dwc->tx_node_active != &desc->tx_list) {
>                                 child = to_dw_desc(dwc->tx_node_active);

Is there a point updating residue here? I don't have a very good knowledge of
nollp transfers but this is what i know...

The above "if" will pass if we are still doing transfers and fail if
all transfers are done.
After the end of each LLI we receive an interrupt, where we queue next
LLI. Better
would be to initialize dwc->residue at dwc_dostart() with total
length, start decrementing
it with desc->len for every lli interrupt we get and if call for
getting residue comes in
middle of transfer, simple return residue - dwc_get_sent(desc) without
updating residue
field...

> @@ -436,6 +469,9 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
>
>         if (test_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags)) {
>                 dev_vdbg(chan2dev(&dwc->chan), "%s: soft LLP mode\n", __func__);
> +
> +               dwc_update_residue(dwc, dwc_first_active(dwc));
> +

same is applicable here too and so you can get rid of
dwc_update_residue() routine.

>                 spin_unlock_irqrestore(&dwc->lock, flags);
>                 return;
>         }
> @@ -444,6 +480,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);
> @@ -453,16 +492,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.
> @@ -1058,6 +1102,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);
> @@ -1067,8 +1112,12 @@ dwc_tx_status(struct dma_chan *chan,
>                 ret = dma_cookie_status(chan, cookie, txstate);
>         }
>
> +       spin_lock_irqsave(&dwc->lock, flags);
> +

why do you need locking here?

>         if (ret != DMA_SUCCESS)
> -               dma_set_residue(txstate, dwc_first_active(dwc)->len);
> +               dma_set_residue(txstate, dwc->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;

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

[Andy Shevchenko]

On Thu, 2013-01-24 at 10:37 +0530, Viresh Kumar wrote: 
> On Wed, Jan 23, 2013 at 9:07 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> 
> >  static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
> >  {
> >         dma_addr_t llp;
> > @@ -410,6 +441,8 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
> >                          */
> >                         desc = dwc_first_active(dwc);
> >
> > +                       dwc_update_residue(dwc, desc);
> > +
> >                         if (dwc->tx_node_active != &desc->tx_list) {
> >                                 child = to_dw_desc(dwc->tx_node_active);
> 
> Is there a point updating residue here? I don't have a very good knowledge of
> nollp transfers but this is what i know...
> 
> The above "if" will pass if we are still doing transfers and fail if
> all transfers are done.
> After the end of each LLI we receive an interrupt, where we queue next
> LLI. Better
> would be to initialize dwc->residue at dwc_dostart() with total
> length, start decrementing
> it with desc->len for every lli interrupt we get 

It's mostly okay, but we have to handle few cases:
- we have only first (master) descriptor
- we have a chain of the descriptors: master + children
- we have finished last transfer

>From my point of view we can't fully get rid of dwc_update_residue(),
but modify it a bit (drop away for loop).

> and if call for
> getting residue comes in
> middle of transfer, simple return residue - dwc_get_sent(desc) without
> updating residue
> field...

Where? In the tx_status to call something which returns dwc->residue -
dwc_get_sent() ?

> 
> > @@ -436,6 +469,9 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
> >
> >         if (test_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags)) {
> >                 dev_vdbg(chan2dev(&dwc->chan), "%s: soft LLP mode\n", __func__);
> > +
> > +               dwc_update_residue(dwc, dwc_first_active(dwc));
> > +
> 
> same is applicable here too and so you can get rid of
> dwc_update_residue() routine.

Same thoughts as above are applicable to this.

> >                 spin_unlock_irqrestore(&dwc->lock, flags);
> >                 return;
> >         }
> > @@ -444,6 +480,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);
> > @@ -453,16 +492,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.
> > @@ -1058,6 +1102,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);
> > @@ -1067,8 +1112,12 @@ dwc_tx_status(struct dma_chan *chan,
> >                 ret = dma_cookie_status(chan, cookie, txstate);
> >         }
> >
> > +       spin_lock_irqsave(&dwc->lock, flags);
> > +
> 
> why do you need locking here?

What about the case when one CPU is getting an interrupt and runs
scan_descriptors when the other, for example, in the middle of
tx_status? So, I'm afraid the dma_set_residue(txstate, dwc->residue) is
not atomic and we might end up with random numbers here.

> >         if (ret != DMA_SUCCESS)
> > -               dma_set_residue(txstate, dwc_first_active(dwc)->len);
> > +               dma_set_residue(txstate, dwc->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;

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

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

[Viresh Kumar]

On 24 January 2013 13:31, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Thu, 2013-01-24 at 10:37 +0530, Viresh Kumar wrote:

>> Is there a point updating residue here? I don't have a very good knowledge of
>> nollp transfers but this is what i know...
>>
>> The above "if" will pass if we are still doing transfers and fail if
>> all transfers are done.
>> After the end of each LLI we receive an interrupt, where we queue next
>> LLI. Better
>> would be to initialize dwc->residue at dwc_dostart() with total
>> length, start decrementing
>> it with desc->len for every lli interrupt we get
>
> It's mostly okay, but we have to handle few cases:
> - we have only first (master) descriptor
> - we have a chain of the descriptors: master + children
> - we have finished last transfer
>
> From my point of view we can't fully get rid of dwc_update_residue(),
> but modify it a bit (drop away for loop).

I feel the residue mechanism for all three cases can be handled by the code
i asked you to try. Lets see how it goes, after we see first level of
draft from you.

>> and if call for
>> getting residue comes in
>> middle of transfer, simple return residue - dwc_get_sent(desc) without
>> updating residue
>> field...
>
> Where? In the tx_status to call something which returns dwc->residue -
> dwc_get_sent() ?

Yes. My point was not to update dwc->residue with whatever data we get out of
dwc_get_sent().

>> > +       spin_lock_irqsave(&dwc->lock, flags);
>> > +
>>
>> why do you need locking here?
>
> What about the case when one CPU is getting an interrupt and runs
> scan_descriptors when the other, for example, in the middle of
> tx_status? So, I'm afraid the dma_set_residue(txstate, dwc->residue) is
> not atomic and we might end up with random numbers here.

I am not against the lock, but want both of us to know why it is there :)
So, i don't think that we need to protect dma_set_residue() at all. That's
not our job.

What we need to make sure is value read from dwc->residue is consistent.
And so, i believe we need a lock to protect read and write to dwc->residue.