linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V3 0/7] dmaengine/dw_dmac updates
@ 2011-04-27  9:36 Viresh Kumar
  2011-04-27  9:36 ` [PATCH V3 1/7] dmaengine/dw_dmac: call dwc_descriptor_complete from dwc_control with lock held Viresh Kumar
                   ` (7 more replies)
  0 siblings, 8 replies; 16+ messages in thread
From: Viresh Kumar @ 2011-04-27  9:36 UTC (permalink / raw)
  To: linux-arm-kernel

This patchset fixes few issues and extends its support.

Changes in V3:
 - lflags is removed from dw_dma_chan and local flag variables are created.
 - An extra argument is added to routines calling dwc_descriptor_complete()
   directly or indirectly
 - spin_lock() in tasklet is also changed to irqsave variants.

Changes in V2:
 - lflags added in dw_dma_chan instead of dw_dma
 - Patch from Linus Walleij added for pause and resume functionality.

Linus Walleij (1):
  dmaengine/dw_dmac: implement pause and resume in dwc_control

Viresh Kumar (6):
  dmaengine/dw_dmac: call dwc_descriptor_complete from dwc_control with
    lock held
  dmaengine/dw_dmac: Replace spin_lock* with irqsave variants
  dmaengine/dw_dmac: don't call callback routine in case
    dmaengine_terminate_all() is called
  dmaengine/dw_dmac: Enable resubmission from callback routine.
  dmaengine/dw_dmac: set residue as total len in dwc_tx_status if
    status is !DMA_SUCCESS
  dmaengine/dw_dmac: Divide one sg to many desc, if sg len is greater
    than DWC_MAX_COUNT

 drivers/dma/dw_dmac.c      |  260 ++++++++++++++++++++++++++++----------------
 drivers/dma/dw_dmac_regs.h |    1 +
 2 files changed, 165 insertions(+), 96 deletions(-)

-- 
1.7.3.4

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH V3 1/7] dmaengine/dw_dmac: call dwc_descriptor_complete from dwc_control with lock held
  2011-04-27  9:36 [PATCH V3 0/7] dmaengine/dw_dmac updates Viresh Kumar
@ 2011-04-27  9:36 ` Viresh Kumar
  2011-04-27  9:36 ` [PATCH V3 2/7] dmaengine/dw_dmac: Replace spin_lock* with irqsave variants Viresh Kumar
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2011-04-27  9:36 UTC (permalink / raw)
  To: linux-arm-kernel

dwc_descriptor_complete must always be called with channel lock held. This patch
moves unlock code, in dwc_control(), untill after dwc_descriptor_complete is
called.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/dma/dw_dmac.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index b15c32c..b8985af 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -827,12 +827,12 @@ static int dwc_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 	list_splice_init(&dwc->queue, &list);
 	list_splice_init(&dwc->active_list, &list);
 
-	spin_unlock_bh(&dwc->lock);
-
 	/* Flush all pending and queued descriptors */
 	list_for_each_entry_safe(desc, _desc, &list, desc_node)
 		dwc_descriptor_complete(dwc, desc);
 
+	spin_unlock_bh(&dwc->lock);
+
 	return 0;
 }
 
-- 
1.7.3.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH V3 2/7] dmaengine/dw_dmac: Replace spin_lock* with irqsave variants
  2011-04-27  9:36 [PATCH V3 0/7] dmaengine/dw_dmac updates Viresh Kumar
  2011-04-27  9:36 ` [PATCH V3 1/7] dmaengine/dw_dmac: call dwc_descriptor_complete from dwc_control with lock held Viresh Kumar
@ 2011-04-27  9:36 ` Viresh Kumar
  2011-04-28 17:10   ` Russell King - ARM Linux
  2011-04-27  9:36 ` [PATCH V3 3/7] dmaengine/dw_dmac: don't call callback routine in case dmaengine_terminate_all() is called Viresh Kumar
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 16+ messages in thread
From: Viresh Kumar @ 2011-04-27  9:36 UTC (permalink / raw)
  To: linux-arm-kernel

dmaengine routines can be called from interrupt context and with interrupts
disabled.  Whereas spin_unlock_bh can't be called from such contexts. So this
patch converts all spin_*_bh routines to irqsave variants.

Also, spin_lock() used in tasklet is converted to irqsave variants, as tasklet
can be interrupted, and dma requests from such interruptions may also call
spin_lock.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/dma/dw_dmac.c |   82 +++++++++++++++++++++++++++++-------------------
 1 files changed, 49 insertions(+), 33 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index b8985af..b6dfc39 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -93,8 +93,9 @@ static struct dw_desc *dwc_desc_get(struct dw_dma_chan *dwc)
 	struct dw_desc *desc, *_desc;
 	struct dw_desc *ret = NULL;
 	unsigned int i = 0;
+	unsigned long flags;
 
-	spin_lock_bh(&dwc->lock);
+	spin_lock_irqsave(&dwc->lock, flags);
 	list_for_each_entry_safe(desc, _desc, &dwc->free_list, desc_node) {
 		if (async_tx_test_ack(&desc->txd)) {
 			list_del(&desc->desc_node);
@@ -104,7 +105,7 @@ static struct dw_desc *dwc_desc_get(struct dw_dma_chan *dwc)
 		dev_dbg(chan2dev(&dwc->chan), "desc %p not ACKed\n", desc);
 		i++;
 	}
-	spin_unlock_bh(&dwc->lock);
+	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	dev_vdbg(chan2dev(&dwc->chan), "scanned %u descriptors on freelist\n", i);
 
@@ -130,12 +131,14 @@ static void dwc_sync_desc_for_cpu(struct dw_dma_chan *dwc, struct dw_desc *desc)
  */
 static void dwc_desc_put(struct dw_dma_chan *dwc, struct dw_desc *desc)
 {
+	unsigned long flags;
+
 	if (desc) {
 		struct dw_desc *child;
 
 		dwc_sync_desc_for_cpu(dwc, desc);
 
-		spin_lock_bh(&dwc->lock);
+		spin_lock_irqsave(&dwc->lock, flags);
 		list_for_each_entry(child, &desc->tx_list, desc_node)
 			dev_vdbg(chan2dev(&dwc->chan),
 					"moving child desc %p to freelist\n",
@@ -143,7 +146,7 @@ static void dwc_desc_put(struct dw_dma_chan *dwc, struct dw_desc *desc)
 		list_splice_init(&desc->tx_list, &dwc->free_list);
 		dev_vdbg(chan2dev(&dwc->chan), "moving desc %p to freelist\n", desc);
 		list_add(&desc->desc_node, &dwc->free_list);
-		spin_unlock_bh(&dwc->lock);
+		spin_unlock_irqrestore(&dwc->lock, flags);
 	}
 }
 
@@ -407,6 +410,8 @@ EXPORT_SYMBOL(dw_dma_get_dst_addr);
 static void dwc_handle_cyclic(struct dw_dma *dw, struct dw_dma_chan *dwc,
 		u32 status_block, u32 status_err, u32 status_xfer)
 {
+	unsigned long flags;
+
 	if (status_block & dwc->mask) {
 		void (*callback)(void *param);
 		void *callback_param;
@@ -418,9 +423,9 @@ static void dwc_handle_cyclic(struct dw_dma *dw, struct dw_dma_chan *dwc,
 		callback = dwc->cdesc->period_callback;
 		callback_param = dwc->cdesc->period_callback_param;
 		if (callback) {
-			spin_unlock(&dwc->lock);
+			spin_unlock_irqrestore(&dwc->lock, flags);
 			callback(callback_param);
-			spin_lock(&dwc->lock);
+			spin_lock_irqsave(&dwc->lock, flags);
 		}
 	}
 
@@ -470,6 +475,7 @@ static void dw_dma_tasklet(unsigned long data)
 	u32 status_block;
 	u32 status_xfer;
 	u32 status_err;
+	unsigned long flags;
 	int i;
 
 	status_block = dma_readl(dw, RAW.BLOCK);
@@ -481,7 +487,7 @@ static void dw_dma_tasklet(unsigned long data)
 
 	for (i = 0; i < dw->dma.chancnt; i++) {
 		dwc = &dw->chan[i];
-		spin_lock(&dwc->lock);
+		spin_lock_irqsave(&dwc->lock, flags);
 		if (test_bit(DW_DMA_IS_CYCLIC, &dwc->flags))
 			dwc_handle_cyclic(dw, dwc, status_block, status_err,
 					status_xfer);
@@ -489,7 +495,7 @@ static void dw_dma_tasklet(unsigned long data)
 			dwc_handle_error(dw, dwc);
 		else if ((status_block | status_xfer) & (1 << i))
 			dwc_scan_descriptors(dw, dwc);
-		spin_unlock(&dwc->lock);
+		spin_unlock_irqrestore(&dwc->lock, flags);
 	}
 
 	/*
@@ -544,8 +550,9 @@ static dma_cookie_t dwc_tx_submit(struct dma_async_tx_descriptor *tx)
 	struct dw_desc		*desc = txd_to_dw_desc(tx);
 	struct dw_dma_chan	*dwc = to_dw_dma_chan(tx->chan);
 	dma_cookie_t		cookie;
+	unsigned long		flags;
 
-	spin_lock_bh(&dwc->lock);
+	spin_lock_irqsave(&dwc->lock, flags);
 	cookie = dwc_assign_cookie(dwc, desc);
 
 	/*
@@ -565,7 +572,7 @@ static dma_cookie_t dwc_tx_submit(struct dma_async_tx_descriptor *tx)
 		list_add_tail(&desc->desc_node, &dwc->queue);
 	}
 
-	spin_unlock_bh(&dwc->lock);
+	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	return cookie;
 }
@@ -804,6 +811,7 @@ static int dwc_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
 	struct dw_dma		*dw = to_dw_dma(chan->device);
 	struct dw_desc		*desc, *_desc;
+	unsigned long		flags;
 	LIST_HEAD(list);
 
 	/* Only supports DMA_TERMINATE_ALL */
@@ -816,7 +824,7 @@ static int dwc_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 	 * channel. We still have to poll the channel enable bit due
 	 * to AHB/HSB limitations.
 	 */
-	spin_lock_bh(&dwc->lock);
+	spin_lock_irqsave(&dwc->lock, flags);
 
 	channel_clear_bit(dw, CH_EN, dwc->mask);
 
@@ -831,7 +839,7 @@ static int dwc_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 	list_for_each_entry_safe(desc, _desc, &list, desc_node)
 		dwc_descriptor_complete(dwc, desc);
 
-	spin_unlock_bh(&dwc->lock);
+	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	return 0;
 }
@@ -845,15 +853,16 @@ dwc_tx_status(struct dma_chan *chan,
 	dma_cookie_t		last_used;
 	dma_cookie_t		last_complete;
 	int			ret;
+	unsigned long		flags;
 
 	last_complete = dwc->completed;
 	last_used = chan->cookie;
 
 	ret = dma_async_is_complete(cookie, last_complete, last_used);
 	if (ret != DMA_SUCCESS) {
-		spin_lock_bh(&dwc->lock);
+		spin_lock_irqsave(&dwc->lock, flags);
 		dwc_scan_descriptors(to_dw_dma(chan->device), dwc);
-		spin_unlock_bh(&dwc->lock);
+		spin_unlock_irqrestore(&dwc->lock, flags);
 
 		last_complete = dwc->completed;
 		last_used = chan->cookie;
@@ -869,11 +878,12 @@ dwc_tx_status(struct dma_chan *chan,
 static void dwc_issue_pending(struct dma_chan *chan)
 {
 	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
+	unsigned long		flags;
 
-	spin_lock_bh(&dwc->lock);
+	spin_lock_irqsave(&dwc->lock, flags);
 	if (!list_empty(&dwc->queue))
 		dwc_scan_descriptors(to_dw_dma(chan->device), dwc);
-	spin_unlock_bh(&dwc->lock);
+	spin_unlock_irqrestore(&dwc->lock, flags);
 }
 
 static int dwc_alloc_chan_resources(struct dma_chan *chan)
@@ -885,6 +895,7 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
 	int			i;
 	u32			cfghi;
 	u32			cfglo;
+	unsigned long		flags;
 
 	dev_vdbg(chan2dev(chan), "alloc_chan_resources\n");
 
@@ -922,16 +933,16 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
 	 * doesn't mean what you think it means), and status writeback.
 	 */
 
-	spin_lock_bh(&dwc->lock);
+	spin_lock_irqsave(&dwc->lock, flags);
 	i = dwc->descs_allocated;
 	while (dwc->descs_allocated < NR_DESCS_PER_CHANNEL) {
-		spin_unlock_bh(&dwc->lock);
+		spin_unlock_irqrestore(&dwc->lock, flags);
 
 		desc = kzalloc(sizeof(struct dw_desc), GFP_KERNEL);
 		if (!desc) {
 			dev_info(chan2dev(chan),
 				"only allocated %d descriptors\n", i);
-			spin_lock_bh(&dwc->lock);
+			spin_lock_irqsave(&dwc->lock, flags);
 			break;
 		}
 
@@ -943,7 +954,7 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
 				sizeof(desc->lli), DMA_TO_DEVICE);
 		dwc_desc_put(dwc, desc);
 
-		spin_lock_bh(&dwc->lock);
+		spin_lock_irqsave(&dwc->lock, flags);
 		i = ++dwc->descs_allocated;
 	}
 
@@ -952,7 +963,7 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
 	channel_set_bit(dw, MASK.BLOCK, dwc->mask);
 	channel_set_bit(dw, MASK.ERROR, dwc->mask);
 
-	spin_unlock_bh(&dwc->lock);
+	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	dev_dbg(chan2dev(chan),
 		"alloc_chan_resources allocated %d descriptors\n", i);
@@ -965,6 +976,7 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
 	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
 	struct dw_dma		*dw = to_dw_dma(chan->device);
 	struct dw_desc		*desc, *_desc;
+	unsigned long		flags;
 	LIST_HEAD(list);
 
 	dev_dbg(chan2dev(chan), "free_chan_resources (descs allocated=%u)\n",
@@ -975,7 +987,7 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
 	BUG_ON(!list_empty(&dwc->queue));
 	BUG_ON(dma_readl(to_dw_dma(chan->device), CH_EN) & dwc->mask);
 
-	spin_lock_bh(&dwc->lock);
+	spin_lock_irqsave(&dwc->lock, flags);
 	list_splice_init(&dwc->free_list, &list);
 	dwc->descs_allocated = 0;
 
@@ -984,7 +996,7 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
 	channel_clear_bit(dw, MASK.BLOCK, dwc->mask);
 	channel_clear_bit(dw, MASK.ERROR, dwc->mask);
 
-	spin_unlock_bh(&dwc->lock);
+	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	list_for_each_entry_safe(desc, _desc, &list, desc_node) {
 		dev_vdbg(chan2dev(chan), "  freeing descriptor %p\n", desc);
@@ -1009,13 +1021,14 @@ int dw_dma_cyclic_start(struct dma_chan *chan)
 {
 	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
 	struct dw_dma		*dw = to_dw_dma(dwc->chan.device);
+	unsigned long		flags;
 
 	if (!test_bit(DW_DMA_IS_CYCLIC, &dwc->flags)) {
 		dev_err(chan2dev(&dwc->chan), "missing prep for cyclic DMA\n");
 		return -ENODEV;
 	}
 
-	spin_lock(&dwc->lock);
+	spin_lock_irqsave(&dwc->lock, flags);
 
 	/* assert channel is idle */
 	if (dma_readl(dw, CH_EN) & dwc->mask) {
@@ -1028,7 +1041,7 @@ int dw_dma_cyclic_start(struct dma_chan *chan)
 			channel_readl(dwc, LLP),
 			channel_readl(dwc, CTL_HI),
 			channel_readl(dwc, CTL_LO));
-		spin_unlock(&dwc->lock);
+		spin_unlock_irqrestore(&dwc->lock, flags);
 		return -EBUSY;
 	}
 
@@ -1043,7 +1056,7 @@ int dw_dma_cyclic_start(struct dma_chan *chan)
 
 	channel_set_bit(dw, CH_EN, dwc->mask);
 
-	spin_unlock(&dwc->lock);
+	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	return 0;
 }
@@ -1059,14 +1072,15 @@ void dw_dma_cyclic_stop(struct dma_chan *chan)
 {
 	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
 	struct dw_dma		*dw = to_dw_dma(dwc->chan.device);
+	unsigned long		flags;
 
-	spin_lock(&dwc->lock);
+	spin_lock_irqsave(&dwc->lock, flags);
 
 	channel_clear_bit(dw, CH_EN, dwc->mask);
 	while (dma_readl(dw, CH_EN) & dwc->mask)
 		cpu_relax();
 
-	spin_unlock(&dwc->lock);
+	spin_unlock_irqrestore(&dwc->lock, flags);
 }
 EXPORT_SYMBOL(dw_dma_cyclic_stop);
 
@@ -1095,17 +1109,18 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
 	unsigned int			reg_width;
 	unsigned int			periods;
 	unsigned int			i;
+	unsigned long			flags;
 
-	spin_lock_bh(&dwc->lock);
+	spin_lock_irqsave(&dwc->lock, flags);
 	if (!list_empty(&dwc->queue) || !list_empty(&dwc->active_list)) {
-		spin_unlock_bh(&dwc->lock);
+		spin_unlock_irqrestore(&dwc->lock, flags);
 		dev_dbg(chan2dev(&dwc->chan),
 				"queue and/or active list are not empty\n");
 		return ERR_PTR(-EBUSY);
 	}
 
 	was_cyclic = test_and_set_bit(DW_DMA_IS_CYCLIC, &dwc->flags);
-	spin_unlock_bh(&dwc->lock);
+	spin_unlock_irqrestore(&dwc->lock, flags);
 	if (was_cyclic) {
 		dev_dbg(chan2dev(&dwc->chan),
 				"channel already prepared for cyclic DMA\n");
@@ -1219,13 +1234,14 @@ void dw_dma_cyclic_free(struct dma_chan *chan)
 	struct dw_dma		*dw = to_dw_dma(dwc->chan.device);
 	struct dw_cyclic_desc	*cdesc = dwc->cdesc;
 	int			i;
+	unsigned long		flags;
 
 	dev_dbg(chan2dev(&dwc->chan), "cyclic free\n");
 
 	if (!cdesc)
 		return;
 
-	spin_lock_bh(&dwc->lock);
+	spin_lock_irqsave(&dwc->lock, flags);
 
 	channel_clear_bit(dw, CH_EN, dwc->mask);
 	while (dma_readl(dw, CH_EN) & dwc->mask)
@@ -1235,7 +1251,7 @@ void dw_dma_cyclic_free(struct dma_chan *chan)
 	dma_writel(dw, CLEAR.ERROR, dwc->mask);
 	dma_writel(dw, CLEAR.XFER, dwc->mask);
 
-	spin_unlock_bh(&dwc->lock);
+	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	for (i = 0; i < cdesc->periods; i++)
 		dwc_desc_put(dwc, cdesc->desc[i]);
-- 
1.7.3.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH V3 3/7] dmaengine/dw_dmac: don't call callback routine in case dmaengine_terminate_all() is called
  2011-04-27  9:36 [PATCH V3 0/7] dmaengine/dw_dmac updates Viresh Kumar
  2011-04-27  9:36 ` [PATCH V3 1/7] dmaengine/dw_dmac: call dwc_descriptor_complete from dwc_control with lock held Viresh Kumar
  2011-04-27  9:36 ` [PATCH V3 2/7] dmaengine/dw_dmac: Replace spin_lock* with irqsave variants Viresh Kumar
@ 2011-04-27  9:36 ` Viresh Kumar
  2011-04-28 17:11   ` Russell King - ARM Linux
  2011-04-27  9:36 ` [PATCH V3 4/7] dmaengine/dw_dmac: Enable resubmission from callback routine Viresh Kumar
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 16+ messages in thread
From: Viresh Kumar @ 2011-04-27  9:36 UTC (permalink / raw)
  To: linux-arm-kernel

If dmaengine_terminate_all() is called for dma channel, then it doesn't make
much sense to call registered callback routine. While in case of success or
failure it must be called.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/dma/dw_dmac.c |   27 ++++++++++++++-------------
 1 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index b6dfc39..f590109 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -198,7 +198,8 @@ static void dwc_dostart(struct dw_dma_chan *dwc, struct dw_desc *first)
 /*----------------------------------------------------------------------*/
 
 static void
-dwc_descriptor_complete(struct dw_dma_chan *dwc, struct dw_desc *desc)
+dwc_descriptor_complete(struct dw_dma_chan *dwc, struct dw_desc *desc,
+		bool callback_required)
 {
 	dma_async_tx_callback		callback;
 	void				*param;
@@ -208,8 +209,10 @@ dwc_descriptor_complete(struct dw_dma_chan *dwc, struct dw_desc *desc)
 	dev_vdbg(chan2dev(&dwc->chan), "descriptor %u complete\n", txd->cookie);
 
 	dwc->completed = txd->cookie;
-	callback = txd->callback;
-	param = txd->callback_param;
+	if (callback_required) {
+		callback = txd->callback;
+		param = txd->callback_param;
+	}
 
 	dwc_sync_desc_for_cpu(dwc, desc);
 
@@ -241,12 +244,10 @@ dwc_descriptor_complete(struct dw_dma_chan *dwc, struct dw_desc *desc)
 		}
 	}
 
-	/*
-	 * The API requires that no submissions are done from a
-	 * callback, so we don't need to drop the lock here
-	 */
-	if (callback)
-		callback(param);
+	if (callback_required) {
+		if (callback)
+			callback(param);
+	}
 }
 
 static void dwc_complete_all(struct dw_dma *dw, struct dw_dma_chan *dwc)
@@ -275,7 +276,7 @@ static void dwc_complete_all(struct dw_dma *dw, struct dw_dma_chan *dwc)
 	}
 
 	list_for_each_entry_safe(desc, _desc, &list, desc_node)
-		dwc_descriptor_complete(dwc, desc);
+		dwc_descriptor_complete(dwc, desc, 1);
 }
 
 static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
@@ -325,7 +326,7 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 		 * No descriptors so far seem to be in progress, i.e.
 		 * this one must be done.
 		 */
-		dwc_descriptor_complete(dwc, desc);
+		dwc_descriptor_complete(dwc, desc, 1);
 	}
 
 	dev_err(chan2dev(&dwc->chan),
@@ -387,7 +388,7 @@ static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
 		dwc_dump_lli(dwc, &child->lli);
 
 	/* Pretend the descriptor completed successfully */
-	dwc_descriptor_complete(dwc, bad_desc);
+	dwc_descriptor_complete(dwc, bad_desc, 1);
 }
 
 /* --------------------- Cyclic DMA API extensions -------------------- */
@@ -837,7 +838,7 @@ static int dwc_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 
 	/* Flush all pending and queued descriptors */
 	list_for_each_entry_safe(desc, _desc, &list, desc_node)
-		dwc_descriptor_complete(dwc, desc);
+		dwc_descriptor_complete(dwc, desc, 0);
 
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
-- 
1.7.3.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH V3 4/7] dmaengine/dw_dmac: Enable resubmission from callback routine.
  2011-04-27  9:36 [PATCH V3 0/7] dmaengine/dw_dmac updates Viresh Kumar
                   ` (2 preceding siblings ...)
  2011-04-27  9:36 ` [PATCH V3 3/7] dmaengine/dw_dmac: don't call callback routine in case dmaengine_terminate_all() is called Viresh Kumar
@ 2011-04-27  9:36 ` Viresh Kumar
  2011-04-28 17:12   ` Russell King - ARM Linux
  2011-04-27  9:36 ` [PATCH V3 5/7] dmaengine/dw_dmac: set residue as total len in dwc_tx_status if status is !DMA_SUCCESS Viresh Kumar
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 16+ messages in thread
From: Viresh Kumar @ 2011-04-27  9:36 UTC (permalink / raw)
  To: linux-arm-kernel

Resubmission of new transfer must be allowed from callbacks. For this release
lock before calling callback routine and enable them again.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/dma/dw_dmac.c |   37 +++++++++++++++++++++----------------
 1 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index f590109..3285ae9 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -199,10 +199,10 @@ static void dwc_dostart(struct dw_dma_chan *dwc, struct dw_desc *first)
 
 static void
 dwc_descriptor_complete(struct dw_dma_chan *dwc, struct dw_desc *desc,
-		bool callback_required)
+		bool callback_required, unsigned long flags)
 {
-	dma_async_tx_callback		callback;
-	void				*param;
+	dma_async_tx_callback		callback = NULL;
+	void				*param = NULL;
 	struct dma_async_tx_descriptor	*txd = &desc->txd;
 	struct dw_desc			*child;
 
@@ -245,12 +245,15 @@ dwc_descriptor_complete(struct dw_dma_chan *dwc, struct dw_desc *desc,
 	}
 
 	if (callback_required) {
+		spin_unlock_irqrestore(&dwc->lock, flags);
 		if (callback)
 			callback(param);
+		spin_lock_irqsave(&dwc->lock, flags);
 	}
 }
 
-static void dwc_complete_all(struct dw_dma *dw, struct dw_dma_chan *dwc)
+static void dwc_complete_all(struct dw_dma *dw, struct dw_dma_chan *dwc,
+		unsigned long flags)
 {
 	struct dw_desc *desc, *_desc;
 	LIST_HEAD(list);
@@ -276,10 +279,11 @@ static void dwc_complete_all(struct dw_dma *dw, struct dw_dma_chan *dwc)
 	}
 
 	list_for_each_entry_safe(desc, _desc, &list, desc_node)
-		dwc_descriptor_complete(dwc, desc, 1);
+		dwc_descriptor_complete(dwc, desc, 1, flags);
 }
 
-static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
+static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc,
+		unsigned long flags)
 {
 	dma_addr_t llp;
 	struct dw_desc *desc, *_desc;
@@ -298,7 +302,7 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 	if (status_xfer & dwc->mask) {
 		/* Everything we've submitted is done */
 		dma_writel(dw, CLEAR.XFER, dwc->mask);
-		dwc_complete_all(dw, dwc);
+		dwc_complete_all(dw, dwc, flags);
 		return;
 	}
 
@@ -326,7 +330,7 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 		 * No descriptors so far seem to be in progress, i.e.
 		 * this one must be done.
 		 */
-		dwc_descriptor_complete(dwc, desc, 1);
+		dwc_descriptor_complete(dwc, desc, 1, flags);
 	}
 
 	dev_err(chan2dev(&dwc->chan),
@@ -351,12 +355,13 @@ static void dwc_dump_lli(struct dw_dma_chan *dwc, struct dw_lli *lli)
 			lli->ctlhi, lli->ctllo);
 }
 
-static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
+static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc,
+		unsigned long flags)
 {
 	struct dw_desc *bad_desc;
 	struct dw_desc *child;
 
-	dwc_scan_descriptors(dw, dwc);
+	dwc_scan_descriptors(dw, dwc, flags);
 
 	/*
 	 * The descriptor currently at the head of the active list is
@@ -388,7 +393,7 @@ static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
 		dwc_dump_lli(dwc, &child->lli);
 
 	/* Pretend the descriptor completed successfully */
-	dwc_descriptor_complete(dwc, bad_desc, 1);
+	dwc_descriptor_complete(dwc, bad_desc, 1, flags);
 }
 
 /* --------------------- Cyclic DMA API extensions -------------------- */
@@ -493,9 +498,9 @@ static void dw_dma_tasklet(unsigned long data)
 			dwc_handle_cyclic(dw, dwc, status_block, status_err,
 					status_xfer);
 		else if (status_err & (1 << i))
-			dwc_handle_error(dw, dwc);
+			dwc_handle_error(dw, dwc, flags);
 		else if ((status_block | status_xfer) & (1 << i))
-			dwc_scan_descriptors(dw, dwc);
+			dwc_scan_descriptors(dw, dwc, flags);
 		spin_unlock_irqrestore(&dwc->lock, flags);
 	}
 
@@ -838,7 +843,7 @@ static int dwc_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 
 	/* Flush all pending and queued descriptors */
 	list_for_each_entry_safe(desc, _desc, &list, desc_node)
-		dwc_descriptor_complete(dwc, desc, 0);
+		dwc_descriptor_complete(dwc, desc, 0, flags);
 
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
@@ -862,7 +867,7 @@ dwc_tx_status(struct dma_chan *chan,
 	ret = dma_async_is_complete(cookie, last_complete, last_used);
 	if (ret != DMA_SUCCESS) {
 		spin_lock_irqsave(&dwc->lock, flags);
-		dwc_scan_descriptors(to_dw_dma(chan->device), dwc);
+		dwc_scan_descriptors(to_dw_dma(chan->device), dwc, flags);
 		spin_unlock_irqrestore(&dwc->lock, flags);
 
 		last_complete = dwc->completed;
@@ -883,7 +888,7 @@ static void dwc_issue_pending(struct dma_chan *chan)
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	if (!list_empty(&dwc->queue))
-		dwc_scan_descriptors(to_dw_dma(chan->device), dwc);
+		dwc_scan_descriptors(to_dw_dma(chan->device), dwc, flags);
 	spin_unlock_irqrestore(&dwc->lock, flags);
 }
 
-- 
1.7.3.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH V3 5/7] dmaengine/dw_dmac: set residue as total len in dwc_tx_status if status is !DMA_SUCCESS
  2011-04-27  9:36 [PATCH V3 0/7] dmaengine/dw_dmac updates Viresh Kumar
                   ` (3 preceding siblings ...)
  2011-04-27  9:36 ` [PATCH V3 4/7] dmaengine/dw_dmac: Enable resubmission from callback routine Viresh Kumar
@ 2011-04-27  9:36 ` Viresh Kumar
  2011-04-27  9:36 ` [PATCH V3 6/7] dmaengine/dw_dmac: Divide one sg to many desc, if sg len is greater than DWC_MAX_COUNT Viresh Kumar
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2011-04-27  9:36 UTC (permalink / raw)
  To: linux-arm-kernel

If transfer status is !=DMA_SUCCESS, return total transfer len as residue,
instead of zero.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/dma/dw_dmac.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 3285ae9..964d2d6 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -876,7 +876,11 @@ dwc_tx_status(struct dma_chan *chan,
 		ret = dma_async_is_complete(cookie, last_complete, last_used);
 	}
 
-	dma_set_tx_state(txstate, last_complete, last_used, 0);
+	if (ret != DMA_SUCCESS)
+		dma_set_tx_state(txstate, last_complete, last_used,
+				dwc_first_active(dwc)->len);
+	else
+		dma_set_tx_state(txstate, last_complete, last_used, 0);
 
 	return ret;
 }
-- 
1.7.3.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH V3 6/7] dmaengine/dw_dmac: Divide one sg to many desc, if sg len is greater than DWC_MAX_COUNT
  2011-04-27  9:36 [PATCH V3 0/7] dmaengine/dw_dmac updates Viresh Kumar
                   ` (4 preceding siblings ...)
  2011-04-27  9:36 ` [PATCH V3 5/7] dmaengine/dw_dmac: set residue as total len in dwc_tx_status if status is !DMA_SUCCESS Viresh Kumar
@ 2011-04-27  9:36 ` Viresh Kumar
  2011-04-27  9:36 ` [PATCH V3 7/7] dmaengine/dw_dmac: implement pause and resume in dwc_control Viresh Kumar
  2011-04-27  9:40 ` [PATCH V3 0/7] dmaengine/dw_dmac updates viresh kumar
  7 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2011-04-27  9:36 UTC (permalink / raw)
  To: linux-arm-kernel

If len passed in sg for slave_sg transfers is greater than DWC_MAX_COUNT, then
driver programmes controller incorrectly.  This patch adds code to handle this
situation by allocation more than one desc for same sg.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/dma/dw_dmac.c |   65 +++++++++++++++++++++++++++++++++----------------
 1 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 964d2d6..014654d 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -707,9 +707,15 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		reg = dws->tx_reg;
 		for_each_sg(sgl, sg, sg_len, i) {
 			struct dw_desc	*desc;
-			u32		len;
-			u32		mem;
+			u32		len, dlen, mem;
 
+			mem = sg_phys(sg);
+			len = sg_dma_len(sg);
+			mem_width = 2;
+			if (unlikely(mem & 3 || len & 3))
+				mem_width = 0;
+
+slave_sg_todev_fill_desc:
 			desc = dwc_desc_get(dwc);
 			if (!desc) {
 				dev_err(chan2dev(chan),
@@ -717,16 +723,19 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 				goto err_desc_get;
 			}
 
-			mem = sg_phys(sg);
-			len = sg_dma_len(sg);
-			mem_width = 2;
-			if (unlikely(mem & 3 || len & 3))
-				mem_width = 0;
-
 			desc->lli.sar = mem;
 			desc->lli.dar = reg;
 			desc->lli.ctllo = ctllo | DWC_CTLL_SRC_WIDTH(mem_width);
-			desc->lli.ctlhi = len >> mem_width;
+			if ((len >> mem_width) > DWC_MAX_COUNT) {
+				dlen = DWC_MAX_COUNT << mem_width;
+				mem += dlen;
+				len -= dlen;
+			} else {
+				dlen = len;
+				len = 0;
+			}
+
+			desc->lli.ctlhi = dlen >> mem_width;
 
 			if (!first) {
 				first = desc;
@@ -740,7 +749,10 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 						&first->tx_list);
 			}
 			prev = desc;
-			total_len += len;
+			total_len += dlen;
+
+			if (len)
+				goto slave_sg_todev_fill_desc;
 		}
 		break;
 	case DMA_FROM_DEVICE:
@@ -753,15 +765,7 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		reg = dws->rx_reg;
 		for_each_sg(sgl, sg, sg_len, i) {
 			struct dw_desc	*desc;
-			u32		len;
-			u32		mem;
-
-			desc = dwc_desc_get(dwc);
-			if (!desc) {
-				dev_err(chan2dev(chan),
-					"not enough descriptors available\n");
-				goto err_desc_get;
-			}
+			u32		len, dlen, mem;
 
 			mem = sg_phys(sg);
 			len = sg_dma_len(sg);
@@ -769,10 +773,26 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 			if (unlikely(mem & 3 || len & 3))
 				mem_width = 0;
 
+slave_sg_fromdev_fill_desc:
+			desc = dwc_desc_get(dwc);
+			if (!desc) {
+				dev_err(chan2dev(chan),
+						"not enough descriptors available\n");
+				goto err_desc_get;
+			}
+
 			desc->lli.sar = reg;
 			desc->lli.dar = mem;
 			desc->lli.ctllo = ctllo | DWC_CTLL_DST_WIDTH(mem_width);
-			desc->lli.ctlhi = len >> reg_width;
+			if ((len >> reg_width) > DWC_MAX_COUNT) {
+				dlen = DWC_MAX_COUNT << reg_width;
+				mem += dlen;
+				len -= dlen;
+			} else {
+				dlen = len;
+				len = 0;
+			}
+			desc->lli.ctlhi = dlen >> reg_width;
 
 			if (!first) {
 				first = desc;
@@ -786,7 +806,10 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 						&first->tx_list);
 			}
 			prev = desc;
-			total_len += len;
+			total_len += dlen;
+
+			if (len)
+				goto slave_sg_fromdev_fill_desc;
 		}
 		break;
 	default:
-- 
1.7.3.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH V3 7/7] dmaengine/dw_dmac: implement pause and resume in dwc_control
  2011-04-27  9:36 [PATCH V3 0/7] dmaengine/dw_dmac updates Viresh Kumar
                   ` (5 preceding siblings ...)
  2011-04-27  9:36 ` [PATCH V3 6/7] dmaengine/dw_dmac: Divide one sg to many desc, if sg len is greater than DWC_MAX_COUNT Viresh Kumar
@ 2011-04-27  9:36 ` Viresh Kumar
  2011-04-27  9:40 ` [PATCH V3 0/7] dmaengine/dw_dmac updates viresh kumar
  7 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2011-04-27  9:36 UTC (permalink / raw)
  To: linux-arm-kernel

From: Linus Walleij <linus.walleij@linaro.org>

Some peripherals like amba-pl011 needs pause to be implemented in DMA controller
drivers. This also returns correct status from dwc_tx_status() in case chan is
paused.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/dma/dw_dmac.c      |   59 +++++++++++++++++++++++++++++---------------
 drivers/dma/dw_dmac_regs.h |    1 +
 2 files changed, 40 insertions(+), 20 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 014654d..ba1a2a4 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -841,34 +841,50 @@ static int dwc_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 	struct dw_dma		*dw = to_dw_dma(chan->device);
 	struct dw_desc		*desc, *_desc;
 	unsigned long		flags;
+	u32			cfglo;
 	LIST_HEAD(list);
 
-	/* Only supports DMA_TERMINATE_ALL */
-	if (cmd != DMA_TERMINATE_ALL)
-		return -ENXIO;
+	if (cmd == DMA_PAUSE) {
+		spin_lock_irqsave(&dwc->lock, flags);
 
-	/*
-	 * This is only called when something went wrong elsewhere, so
-	 * we don't really care about the data. Just disable the
-	 * channel. We still have to poll the channel enable bit due
-	 * to AHB/HSB limitations.
-	 */
-	spin_lock_irqsave(&dwc->lock, flags);
+		cfglo = channel_readl(dwc, CFG_LO);
+		channel_writel(dwc, CFG_LO, cfglo | DWC_CFGL_CH_SUSP);
+		while (!(channel_readl(dwc, CFG_LO) & DWC_CFGL_FIFO_EMPTY))
+			cpu_relax();
 
-	channel_clear_bit(dw, CH_EN, dwc->mask);
+		dwc->paused = true;
+		spin_unlock_irqrestore(&dwc->lock, flags);
+	} else if (cmd == DMA_RESUME) {
+		if (!dwc->paused)
+			return 0;
 
-	while (dma_readl(dw, CH_EN) & dwc->mask)
-		cpu_relax();
+		spin_lock_irqsave(&dwc->lock, flags);
 
-	/* active_list entries will end up before queued entries */
-	list_splice_init(&dwc->queue, &list);
-	list_splice_init(&dwc->active_list, &list);
+		cfglo = channel_readl(dwc, CFG_LO);
+		channel_writel(dwc, CFG_LO, cfglo & ~DWC_CFGL_CH_SUSP);
+		dwc->paused = false;
 
-	/* Flush all pending and queued descriptors */
-	list_for_each_entry_safe(desc, _desc, &list, desc_node)
-		dwc_descriptor_complete(dwc, desc, 0, flags);
+		spin_unlock_irqrestore(&dwc->lock, flags);
+	} else if (cmd == DMA_TERMINATE_ALL) {
+		spin_lock_irqsave(&dwc->lock, flags);
 
-	spin_unlock_irqrestore(&dwc->lock, flags);
+		channel_clear_bit(dw, CH_EN, dwc->mask);
+		while (dma_readl(dw, CH_EN) & dwc->mask)
+			cpu_relax();
+
+		dwc->paused = false;
+
+		/* active_list entries will end up before queued entries */
+		list_splice_init(&dwc->queue, &list);
+		list_splice_init(&dwc->active_list, &list);
+
+		/* Flush all pending and queued descriptors */
+		list_for_each_entry_safe(desc, _desc, &list, desc_node)
+			dwc_descriptor_complete(dwc, desc, 0, flags);
+
+		spin_unlock_irqrestore(&dwc->lock, flags);
+	} else
+		return -ENXIO;
 
 	return 0;
 }
@@ -905,6 +921,9 @@ dwc_tx_status(struct dma_chan *chan,
 	else
 		dma_set_tx_state(txstate, last_complete, last_used, 0);
 
+	if (dwc->paused)
+		return DMA_PAUSED;
+
 	return ret;
 }
 
diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw_dmac_regs.h
index 720f821..c968597 100644
--- a/drivers/dma/dw_dmac_regs.h
+++ b/drivers/dma/dw_dmac_regs.h
@@ -138,6 +138,7 @@ struct dw_dma_chan {
 	void __iomem		*ch_regs;
 	u8			mask;
 	u8			priority;
+	bool			paused;
 
 	spinlock_t		lock;
 
-- 
1.7.3.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH V3 0/7] dmaengine/dw_dmac updates
  2011-04-27  9:36 [PATCH V3 0/7] dmaengine/dw_dmac updates Viresh Kumar
                   ` (6 preceding siblings ...)
  2011-04-27  9:36 ` [PATCH V3 7/7] dmaengine/dw_dmac: implement pause and resume in dwc_control Viresh Kumar
@ 2011-04-27  9:40 ` viresh kumar
  7 siblings, 0 replies; 16+ messages in thread
From: viresh kumar @ 2011-04-27  9:40 UTC (permalink / raw)
  To: linux-arm-kernel

On 04/27/2011 03:06 PM, Viresh KUMAR wrote:
> This patchset fixes few issues and extends its support.
> 

Sorry for two copies of same pathset :(

-- 
viresh

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH V3 2/7] dmaengine/dw_dmac: Replace spin_lock* with irqsave variants
  2011-04-27  9:36 ` [PATCH V3 2/7] dmaengine/dw_dmac: Replace spin_lock* with irqsave variants Viresh Kumar
@ 2011-04-28 17:10   ` Russell King - ARM Linux
  2011-04-29  9:15     ` Russell King - ARM Linux
  2011-04-29 10:18     ` viresh kumar
  0 siblings, 2 replies; 16+ messages in thread
From: Russell King - ARM Linux @ 2011-04-28 17:10 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Apr 27, 2011 at 03:06:44PM +0530, Viresh Kumar wrote:
> @@ -407,6 +410,8 @@ EXPORT_SYMBOL(dw_dma_get_dst_addr);
>  static void dwc_handle_cyclic(struct dw_dma *dw, struct dw_dma_chan *dwc,
>  		u32 status_block, u32 status_err, u32 status_xfer)
>  {
> +	unsigned long flags;
> +
>  	if (status_block & dwc->mask) {
>  		void (*callback)(void *param);
>  		void *callback_param;
> @@ -418,9 +423,9 @@ static void dwc_handle_cyclic(struct dw_dma *dw, struct dw_dma_chan *dwc,
>  		callback = dwc->cdesc->period_callback;
>  		callback_param = dwc->cdesc->period_callback_param;
>  		if (callback) {
> -			spin_unlock(&dwc->lock);
> +			spin_unlock_irqrestore(&dwc->lock, flags);
>  			callback(callback_param);
> -			spin_lock(&dwc->lock);
> +			spin_lock_irqsave(&dwc->lock, flags);

I'm really not convinced that this is anywhere near correct.  I'm
surprised this doesn't spit out a compiler warning.

spin_unlock_irqrestore() reads the flags argument and puts it into
the PSR.  spin_lock_irqsave() reads the PSR, puts it into the flags
argument, sets the interrupt mask bit and writes back to the PSR.

So, if you do:

	unsigned long flags;

	spin_unlock_irqrestore(&dwc->lock, flags);
	...
	spin_lock_irqsave(&dwc->lock, flags);

you're going to end up corrupting the PSR.

In any case, releasing a spinlock temporarily within a called function
is _really_ not a nice thing to do.  It makes code review rather
difficult as called functions become non-atomic when called within
an atomic region.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH V3 3/7] dmaengine/dw_dmac: don't call callback routine in case dmaengine_terminate_all() is called
  2011-04-27  9:36 ` [PATCH V3 3/7] dmaengine/dw_dmac: don't call callback routine in case dmaengine_terminate_all() is called Viresh Kumar
@ 2011-04-28 17:11   ` Russell King - ARM Linux
  2011-04-29  3:25     ` viresh kumar
  0 siblings, 1 reply; 16+ messages in thread
From: Russell King - ARM Linux @ 2011-04-28 17:11 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Apr 27, 2011 at 03:06:45PM +0530, Viresh Kumar wrote:
> If dmaengine_terminate_all() is called for dma channel, then it doesn't make
> much sense to call registered callback routine. While in case of success or
> failure it must be called.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> ---
>  drivers/dma/dw_dmac.c |   27 ++++++++++++++-------------
>  1 files changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> index b6dfc39..f590109 100644
> --- a/drivers/dma/dw_dmac.c
> +++ b/drivers/dma/dw_dmac.c
> @@ -198,7 +198,8 @@ static void dwc_dostart(struct dw_dma_chan *dwc, struct dw_desc *first)
>  /*----------------------------------------------------------------------*/
>  
>  static void
> -dwc_descriptor_complete(struct dw_dma_chan *dwc, struct dw_desc *desc)
> +dwc_descriptor_complete(struct dw_dma_chan *dwc, struct dw_desc *desc,
> +		bool callback_required)

If you're using 'bool' then using 'true' and 'false' with it rather than
'1' and '0' is a good idea.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH V3 4/7] dmaengine/dw_dmac: Enable resubmission from callback routine.
  2011-04-27  9:36 ` [PATCH V3 4/7] dmaengine/dw_dmac: Enable resubmission from callback routine Viresh Kumar
@ 2011-04-28 17:12   ` Russell King - ARM Linux
  0 siblings, 0 replies; 16+ messages in thread
From: Russell King - ARM Linux @ 2011-04-28 17:12 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Apr 27, 2011 at 03:06:46PM +0530, Viresh Kumar wrote:
>  static void
>  dwc_descriptor_complete(struct dw_dma_chan *dwc, struct dw_desc *desc,
> -		bool callback_required)
> +		bool callback_required, unsigned long flags)
>  {
...
>  	if (callback_required) {
> +		spin_unlock_irqrestore(&dwc->lock, flags);
>  		if (callback)
>  			callback(param);
> +		spin_lock_irqsave(&dwc->lock, flags);

Again, this isn't really on.  It seems to me that this code needs some
serious reworking.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH V3 3/7] dmaengine/dw_dmac: don't call callback routine in case dmaengine_terminate_all() is called
  2011-04-28 17:11   ` Russell King - ARM Linux
@ 2011-04-29  3:25     ` viresh kumar
  0 siblings, 0 replies; 16+ messages in thread
From: viresh kumar @ 2011-04-29  3:25 UTC (permalink / raw)
  To: linux-arm-kernel

On 04/28/2011 10:41 PM, Russell King - ARM Linux wrote:
>> >  static void
>> > -dwc_descriptor_complete(struct dw_dma_chan *dwc, struct dw_desc *desc)
>> > +dwc_descriptor_complete(struct dw_dma_chan *dwc, struct dw_desc *desc,
>> > +		bool callback_required)
> If you're using 'bool' then using 'true' and 'false' with it rather than
> '1' and '0' is a good idea.

Yes, will correct this.

-- 
viresh

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH V3 2/7] dmaengine/dw_dmac: Replace spin_lock* with irqsave variants
  2011-04-28 17:10   ` Russell King - ARM Linux
@ 2011-04-29  9:15     ` Russell King - ARM Linux
  2011-04-29 10:17       ` viresh kumar
  2011-04-29 10:18     ` viresh kumar
  1 sibling, 1 reply; 16+ messages in thread
From: Russell King - ARM Linux @ 2011-04-29  9:15 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Apr 28, 2011 at 06:10:20PM +0100, Russell King - ARM Linux wrote:
> On Wed, Apr 27, 2011 at 03:06:44PM +0530, Viresh Kumar wrote:
> > @@ -407,6 +410,8 @@ EXPORT_SYMBOL(dw_dma_get_dst_addr);
> >  static void dwc_handle_cyclic(struct dw_dma *dw, struct dw_dma_chan *dwc,
> >  		u32 status_block, u32 status_err, u32 status_xfer)
> >  {
> > +	unsigned long flags;
> > +
> >  	if (status_block & dwc->mask) {
> >  		void (*callback)(void *param);
> >  		void *callback_param;
> > @@ -418,9 +423,9 @@ static void dwc_handle_cyclic(struct dw_dma *dw, struct dw_dma_chan *dwc,
> >  		callback = dwc->cdesc->period_callback;
> >  		callback_param = dwc->cdesc->period_callback_param;
> >  		if (callback) {
> > -			spin_unlock(&dwc->lock);
> > +			spin_unlock_irqrestore(&dwc->lock, flags);
> >  			callback(callback_param);
> > -			spin_lock(&dwc->lock);
> > +			spin_lock_irqsave(&dwc->lock, flags);
> 
> I'm really not convinced that this is anywhere near correct.  I'm
> surprised this doesn't spit out a compiler warning.
> 
> spin_unlock_irqrestore() reads the flags argument and puts it into
> the PSR.  spin_lock_irqsave() reads the PSR, puts it into the flags
> argument, sets the interrupt mask bit and writes back to the PSR.
> 
> So, if you do:
> 
> 	unsigned long flags;
> 
> 	spin_unlock_irqrestore(&dwc->lock, flags);
> 	...
> 	spin_lock_irqsave(&dwc->lock, flags);
> 
> you're going to end up corrupting the PSR.
> 
> In any case, releasing a spinlock temporarily within a called function
> is _really_ not a nice thing to do.  It makes code review rather
> difficult as called functions become non-atomic when called within
> an atomic region.

BTW, how this gets handled in other drivers is basically as follows in
the tasklet:

tasklet()
{
	LIST_HEAD(completed);

	spin_lock_irqsave(lock, flags);
	for each txd(txd) {
		if (completed(txd))
			list_move_tail(&txd->node, &completed);
	}
	try to start new txd();
	spin_unlock_irqrestore(lock, flags);

	for each list entry safe(txd, &completed) {
		void (*callback)(void *) = txd->callback;
		void *param = txd->callback_param;

		free_txd(txd);

		if (callback)
			callback(param);
	}
}

I'm not sure how easy it is to move dw_dmac to that kind of structure,
but I think this is what is required rather than dropping locks within
functions which they haven't themselves taken.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH V3 2/7] dmaengine/dw_dmac: Replace spin_lock* with irqsave variants
  2011-04-29  9:15     ` Russell King - ARM Linux
@ 2011-04-29 10:17       ` viresh kumar
  0 siblings, 0 replies; 16+ messages in thread
From: viresh kumar @ 2011-04-29 10:17 UTC (permalink / raw)
  To: linux-arm-kernel

On 04/29/2011 02:45 PM, Russell King - ARM Linux wrote:
> BTW, how this gets handled in other drivers is basically as follows in
> the tasklet:
> 
> tasklet()
> {
> 	LIST_HEAD(completed);
> 
> 	spin_lock_irqsave(lock, flags);
> 	for each txd(txd) {
> 		if (completed(txd))
> 			list_move_tail(&txd->node, &completed);
> 	}
> 	try to start new txd();
> 	spin_unlock_irqrestore(lock, flags);
> 
> 	for each list entry safe(txd, &completed) {
> 		void (*callback)(void *) = txd->callback;
> 		void *param = txd->callback_param;
> 
> 		free_txd(txd);
> 
> 		if (callback)
> 			callback(param);
> 	}
> }
> 
> I'm not sure how easy it is to move dw_dmac to that kind of structure,

I will check that.

-- 
viresh

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH V3 2/7] dmaengine/dw_dmac: Replace spin_lock* with irqsave variants
  2011-04-28 17:10   ` Russell King - ARM Linux
  2011-04-29  9:15     ` Russell King - ARM Linux
@ 2011-04-29 10:18     ` viresh kumar
  1 sibling, 0 replies; 16+ messages in thread
From: viresh kumar @ 2011-04-29 10:18 UTC (permalink / raw)
  To: linux-arm-kernel

On 04/28/2011 10:40 PM, Russell King - ARM Linux wrote:
> On Wed, Apr 27, 2011 at 03:06:44PM +0530, Viresh Kumar wrote:
>> @@ -407,6 +410,8 @@ EXPORT_SYMBOL(dw_dma_get_dst_addr);
>>  static void dwc_handle_cyclic(struct dw_dma *dw, struct dw_dma_chan *dwc,
>>  		u32 status_block, u32 status_err, u32 status_xfer)
>>  {
>> +	unsigned long flags;
>> +
>>  	if (status_block & dwc->mask) {
>>  		void (*callback)(void *param);
>>  		void *callback_param;
>> @@ -418,9 +423,9 @@ static void dwc_handle_cyclic(struct dw_dma *dw, struct dw_dma_chan *dwc,
>>  		callback = dwc->cdesc->period_callback;
>>  		callback_param = dwc->cdesc->period_callback_param;
>>  		if (callback) {
>> -			spin_unlock(&dwc->lock);
>> +			spin_unlock_irqrestore(&dwc->lock, flags);
>>  			callback(callback_param);
>> -			spin_lock(&dwc->lock);
>> +			spin_lock_irqsave(&dwc->lock, flags);
> 
> I'm really not convinced that this is anywhere near correct.  I'm
> surprised this doesn't spit out a compiler warning.
> 

Sorry, this is done by mistake.

-- 
viresh

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2011-04-29 10:18 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-04-27  9:36 [PATCH V3 0/7] dmaengine/dw_dmac updates Viresh Kumar
2011-04-27  9:36 ` [PATCH V3 1/7] dmaengine/dw_dmac: call dwc_descriptor_complete from dwc_control with lock held Viresh Kumar
2011-04-27  9:36 ` [PATCH V3 2/7] dmaengine/dw_dmac: Replace spin_lock* with irqsave variants Viresh Kumar
2011-04-28 17:10   ` Russell King - ARM Linux
2011-04-29  9:15     ` Russell King - ARM Linux
2011-04-29 10:17       ` viresh kumar
2011-04-29 10:18     ` viresh kumar
2011-04-27  9:36 ` [PATCH V3 3/7] dmaengine/dw_dmac: don't call callback routine in case dmaengine_terminate_all() is called Viresh Kumar
2011-04-28 17:11   ` Russell King - ARM Linux
2011-04-29  3:25     ` viresh kumar
2011-04-27  9:36 ` [PATCH V3 4/7] dmaengine/dw_dmac: Enable resubmission from callback routine Viresh Kumar
2011-04-28 17:12   ` Russell King - ARM Linux
2011-04-27  9:36 ` [PATCH V3 5/7] dmaengine/dw_dmac: set residue as total len in dwc_tx_status if status is !DMA_SUCCESS Viresh Kumar
2011-04-27  9:36 ` [PATCH V3 6/7] dmaengine/dw_dmac: Divide one sg to many desc, if sg len is greater than DWC_MAX_COUNT Viresh Kumar
2011-04-27  9:36 ` [PATCH V3 7/7] dmaengine/dw_dmac: implement pause and resume in dwc_control Viresh Kumar
2011-04-27  9:40 ` [PATCH V3 0/7] dmaengine/dw_dmac updates viresh kumar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).