* [PATCH v2 1/4] dw_dmac: remove unnecessary tx_list field in dw_dma_chan
2013-01-23 15:37 [PATCH v2 0/4] dw_dmac: return proper residue value Andy Shevchenko
@ 2013-01-23 15:37 ` Andy Shevchenko
2013-01-24 4:37 ` Viresh Kumar
2013-01-23 15:37 ` [PATCH v2 2/4] dw_dmac: introduce total_len field in struct dw_desc Andy Shevchenko
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2013-01-23 15:37 UTC (permalink / raw)
To: Viresh Kumar, Vinod Koul, linux-kernel, spear-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v2 2/4] dw_dmac: introduce total_len field in struct dw_desc
2013-01-23 15:37 [PATCH v2 0/4] dw_dmac: return proper residue value Andy Shevchenko
2013-01-23 15:37 ` [PATCH v2 1/4] dw_dmac: remove unnecessary tx_list field in dw_dma_chan Andy Shevchenko
@ 2013-01-23 15:37 ` Andy Shevchenko
2013-01-24 4:39 ` Viresh Kumar
2013-01-23 15:37 ` [PATCH v2 3/4] dw_dmac: fill individual length of descriptor Andy Shevchenko
2013-01-23 15:37 ` [PATCH v2 4/4] dw_dmac: return proper residue value Andy Shevchenko
3 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2013-01-23 15:37 UTC (permalink / raw)
To: Viresh Kumar, Vinod Koul, linux-kernel, spear-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v2 3/4] dw_dmac: fill individual length of descriptor
2013-01-23 15:37 [PATCH v2 0/4] dw_dmac: return proper residue value Andy Shevchenko
2013-01-23 15:37 ` [PATCH v2 1/4] dw_dmac: remove unnecessary tx_list field in dw_dma_chan Andy Shevchenko
2013-01-23 15:37 ` [PATCH v2 2/4] dw_dmac: introduce total_len field in struct dw_desc Andy Shevchenko
@ 2013-01-23 15:37 ` Andy Shevchenko
2013-01-24 4:40 ` Viresh Kumar
2013-01-23 15:37 ` [PATCH v2 4/4] dw_dmac: return proper residue value Andy Shevchenko
3 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2013-01-23 15:37 UTC (permalink / raw)
To: Viresh Kumar, Vinod Koul, linux-kernel, spear-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 3/4] dw_dmac: fill individual length of descriptor
2013-01-23 15:37 ` [PATCH v2 3/4] dw_dmac: fill individual length of descriptor Andy Shevchenko
@ 2013-01-24 4:40 ` Viresh Kumar
0 siblings, 0 replies; 11+ messages in thread
From: Viresh Kumar @ 2013-01-24 4:40 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Vinod Koul, linux-kernel, spear-devel
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 4/4] dw_dmac: return proper residue value
2013-01-23 15:37 [PATCH v2 0/4] dw_dmac: return proper residue value Andy Shevchenko
` (2 preceding siblings ...)
2013-01-23 15:37 ` [PATCH v2 3/4] dw_dmac: fill individual length of descriptor Andy Shevchenko
@ 2013-01-23 15:37 ` Andy Shevchenko
2013-01-24 5:07 ` Viresh Kumar
3 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2013-01-23 15:37 UTC (permalink / raw)
To: Viresh Kumar, Vinod Koul, linux-kernel, spear-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 4/4] dw_dmac: return proper residue value
2013-01-23 15:37 ` [PATCH v2 4/4] dw_dmac: return proper residue value Andy Shevchenko
@ 2013-01-24 5:07 ` Viresh Kumar
2013-01-24 8:01 ` Andy Shevchenko
0 siblings, 1 reply; 11+ messages in thread
From: Viresh Kumar @ 2013-01-24 5:07 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Vinod Koul, linux-kernel, spear-devel
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;
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 4/4] dw_dmac: return proper residue value
2013-01-24 5:07 ` Viresh Kumar
@ 2013-01-24 8:01 ` Andy Shevchenko
2013-01-24 8:14 ` Viresh Kumar
0 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2013-01-24 8:01 UTC (permalink / raw)
To: Viresh Kumar; +Cc: Vinod Koul, linux-kernel, spear-devel
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 4/4] dw_dmac: return proper residue value
2013-01-24 8:01 ` Andy Shevchenko
@ 2013-01-24 8:14 ` Viresh Kumar
0 siblings, 0 replies; 11+ messages in thread
From: Viresh Kumar @ 2013-01-24 8:14 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Vinod Koul, linux-kernel, spear-devel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread