[PATCH 0/9] dmaengine: Pl08x and dw_dmac updates

[PATCH 0/9] dmaengine: Pl08x and dw_dmac updates

[Viresh Kumar]

Hi Vinod/Dan,

This patchset does following:
- Adds DMA_SLAVE_CONFIG for dw_dmac driver
- Adds device_fc (device flow control) field in struct DMA_SLAVE_CONFIG
- Updates pl08x driver according to that
- Fixes few issues/bugs in dw_dmac driver
- Includes earlier patchset sent by Rajeev for cleanly applying this patchset

Rajeev KUMAR (1):
  dmaengine/dw_dmac: Hibernation support in dw_dmac

Viresh Kumar (7):
  dmaengine: Add flow controller information to dma_slave_config
  dmaengine/amba-pl08x: Take flow controller info from DMA_SLAVE_CONFIG
  dmaengine/dw_dmac: Don't use magic number for total number of
    channels
  dmaengine/dw_dmac: Use platform_get_drvdata instead of accessing dev
    directly
  dmaengine/dw_dmac: Don't handle block interrupts
  dmaengine/dw_dmac: Unmap all memory buffers after completion of slave
    transfers
  dmaengine/dw_dmac: Add support for DMA_SLAVE_CONFIG

 drivers/dma/amba-pl08x.c   |    4 +-
 drivers/dma/dw_dmac.c      |  171 ++++++++++++++++++++++++++------------------
 drivers/dma/dw_dmac_regs.h |    3 +
 include/linux/amba/pl08x.h |    8 +-
 include/linux/dmaengine.h  |    5 ++
 include/linux/dw_dmac.h    |    6 --
 6 files changed, 115 insertions(+), 82 deletions(-)

-- 
1.7.8.110.g4cb5d

[PATCH 1/8] dmaengine/dw_dmac: Hibernation support in dw_dmac

[Viresh Kumar]

From: Rajeev KUMAR <rajeev-dlh.kumar@st.com>

The suspend and resume implementation is through dev_pm_ops in dmac. So
in order to support hibernation, freeze, thaw, restore and poweroff
features are required.

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

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 9b592b0..3a3d7ed 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -1562,6 +1562,10 @@ static int dw_resume_noirq(struct device *dev)
 static const struct dev_pm_ops dw_dev_pm_ops = {
 	.suspend_noirq = dw_suspend_noirq,
 	.resume_noirq = dw_resume_noirq,
+	.freeze_noirq = dw_suspend_noirq,
+	.thaw_noirq = dw_resume_noirq,
+	.restore_noirq = dw_resume_noirq,
+	.poweroff_noirq = dw_suspend_noirq,
 };
 
 static struct platform_driver dw_driver = {
-- 
1.7.8.110.g4cb5d

[PATCH 2/8] dmaengine: Add flow controller information to dma_slave_config

[Viresh Kumar]

Flow controller is programmable for few controllers and there are few
intelligent peripherals like, Synopsys JPEG controller, that needs to be a flow
controller of DMA transfers on dest side.

For this, currently two drivers, pl08x and dw_dmac, support flow controller to
be passed from platform to these drivers.

Perhaps, this should be a part of struct dma_slave_config. This patch adds
another field device_fc to this structure. User drivers must pass this as true
if they want to be flow controller of certain transfers.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 include/linux/dmaengine.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 5532bb8..dd1915f 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -25,6 +25,7 @@
 #include <linux/uio.h>
 #include <linux/scatterlist.h>
 #include <linux/bitmap.h>
+#include <linux/types.h>
 #include <asm/page.h>
 
 /**
@@ -330,6 +331,9 @@ enum dma_slave_buswidth {
  * may or may not be applicable on memory sources.
  * @dst_maxburst: same as src_maxburst but for destination target
  * mutatis mutandis.
+ * @device_fc: Flow Controller Settings for ccfg register. Only valid for slave
+ * channels. Fill with 'true' if peripheral should be flow controller. Direction
+ * will be selected at Runtime.
  *
  * This struct is passed in as configuration data to a DMA engine
  * in order to set up a certain channel for DMA transport at runtime.
@@ -356,6 +360,7 @@ struct dma_slave_config {
 	enum dma_slave_buswidth dst_addr_width;
 	u32 src_maxburst;
 	u32 dst_maxburst;
+	bool device_fc;
 };
 
 static inline const char *dma_chan_name(struct dma_chan *chan)
-- 
1.7.8.110.g4cb5d

Re: [PATCH 2/8] dmaengine: Add flow controller information to dma_slave_config

[Linus Walleij]

On Mon, Jan 16, 2012 at 10:50 AM, Viresh Kumar <viresh.kumar@st.com> wrote:

> Flow controller is programmable for few controllers and there are few
> intelligent peripherals like, Synopsys JPEG controller, that needs to be a flow
> controller of DMA transfers on dest side.
>
> For this, currently two drivers, pl08x and dw_dmac, support flow controller to
> be passed from platform to these drivers.

OK

> Perhaps, this should be a part of struct dma_slave_config. This patch adds
> another field device_fc to this structure. User drivers must pass this as true
> if they want to be flow controller of certain transfers.

I dma_slave_config is supposed to be about info that

1) Must to be set-up at runtime
2) All DMA controllers need to know

So if it doesn't *have* to be set up by drivers at runtime, please keep it in
platform data.

If it is (1), so you have a use case where you have to switch a certain channel
between DMA master and device flow control at run-time it looks like it could
be useful for others as well, but I doubt this so I'd like some back-up on
that.

Yours,
Linus Walleij

Re: [PATCH 2/8] dmaengine: Add flow controller information to dma_slave_config

[Viresh Kumar]

On 1/17/2012 2:07 PM, Linus Walleij wrote:
>> > Perhaps, this should be a part of struct dma_slave_config. This patch adds
>> > another field device_fc to this structure. User drivers must pass this as true
>> > if they want to be flow controller of certain transfers.
> I dma_slave_config is supposed to be about info that
> 
> 1) Must to be set-up at runtime

Hmmm.. Currently if i check the comments in dmaengine.h, it is written
as you said. But i believe its usage could be more than that.

For example, how amba-pl011 use it today. Nothing dynamic, all static.

Actually the problem scenario is: pl011 is going to use separate DMA drivers
(for ex: dw_dmac and pl08x) for separate platforms. Now, pl011's code shouldn't
be dependent at all on these controllers. So, we have to pass separate data to
these drivers using dma driver specific platform data.

DMA driver's platform data contains two type of stuff: platform specific
(request line, master, etc) and user driver specific (like pl011) (reg address,
burst, width, direction, flow_controller, etc)

Now, its better to have some common struct in dmaengine which can fulfill
requirement of various DMA driver's data.

I find struct dma_slave_config fitting there. All user driver (pl011) specific
data can be passed using this structure. You can call dma_slave_config()
onetime after you got the channel. And after that just submit transfers.

With this i get rid of half of the dw_dmac specific platform data struct and
used already defined struct dma_slave_config.

> 2) All DMA controllers need to know
> 

Sorry i didn't understood it well.

One more thing. I missed few things in this patch:
- Need to update all instances of struct dma_slave_config with
	.device_fc = false

If, this field is not getting initialized to false by default, then these users
will find their code not working atleast with dw_dmac and amba-pl08x.
Can't say about other drivers.

- Also i need to fixup few atmel drivers which are using dw_dmac.

> So if it doesn't *have* to be set up by drivers at runtime, please keep it in
> platform data.
> 
> If it is (1), so you have a use case where you have to switch a certain channel
> between DMA master and device flow control at run-time it looks like it could
> be useful for others as well, but I doubt this so I'd like some back-up on
> that.

I don't have that crazy usecase :)

-- 
viresh

Re: [PATCH 2/8] dmaengine: Add flow controller information to dma_slave_config

[Linus Walleij]

On Tue, Jan 17, 2012 at 10:07 AM, Viresh Kumar <viresh.kumar@st.com> wrote:
> On 1/17/2012 2:07 PM, Linus Walleij wrote:

>> I dma_slave_config is supposed to be about info that
>>
>> 1) Must to be set-up at runtime
>
> Hmmm.. Currently if i check the comments in dmaengine.h, it is written
> as you said. But i believe its usage could be more than that.
>
> For example, how amba-pl011 use it today. Nothing dynamic, all static.

Maybe I should say that it's supposed to transfer information from
the driver to the DMA engine that:

1) The driver naturally "knows", like which physical register address
  the FIFO is in or burst width etc and the DMA engine has no
  business knowing.

2) That needs to change at runtime, like for example how the PL022
 driver request 32, 16 or 8 bit wide transfers depending on bus
 width.

I think master mode could very well be under (1). So the driver knows
if this hardware expects the DMA engine to drive the transaction or
if it's the device itself that should drive it.

So I'm starting to think like you :-)

I wonder what the default in each driver should be though?
I guess it's that the DMA engine drives it, so devcie control
is the exception, and that means the code looks good as it is.

> Actually the problem scenario is: pl011 is going to use separate DMA drivers
> (for ex: dw_dmac and pl08x) for separate platforms. Now, pl011's code shouldn't
> be dependent at all on these controllers. So, we have to pass separate data to
> these drivers using dma driver specific platform data.

OK I buy this.

> Now, its better to have some common struct in dmaengine which can fulfill
> requirement of various DMA driver's data.
> (...)
> With this i get rid of half of the dw_dmac specific platform data struct and
> used already defined struct dma_slave_config.

I tend to agree. But the main reason for my part is that the device
driver has this kind of knowledge, not the DMA driver. You can add:
Acked-by: Linus Walleij <linus.walleij@linaro.org>
to the patch.

> One more thing. I missed few things in this patch:
> - Need to update all instances of struct dma_slave_config with
>        .device_fc = false

All statically defined structs contain zero == false by default
so it's not needed.

Make sure any dynamic allocations (I don't know of any!)
are kzalloc() though.

> If, this field is not getting initialized to false by default, then these users
> will find their code not working atleast with dw_dmac and amba-pl08x.

As per above it will be zero, DMA engine control, by default.

>> If it is (1), so you have a use case where you have to switch a certain channel
>> between DMA master and device flow control at run-time it looks like it could
>> be useful for others as well, but I doubt this so I'd like some back-up on
>> that.
>
> I don't have that crazy usecase :)

Well it doesn't matter, we can support that with this patch though :-)

Yours,
Linus Walleij

Re: [PATCH 2/8] dmaengine: Add flow controller information to dma_slave_config

[Viresh Kumar]

On 1/18/2012 4:06 PM, Linus Walleij wrote:
> On Tue, Jan 17, 2012 at 10:07 AM, Viresh Kumar <viresh.kumar@st.com> wrote:
>> On 1/17/2012 2:07 PM, Linus Walleij wrote:

> Maybe I should say that it's supposed to transfer information from
> the driver to the DMA engine that:
> 
> 1) The driver naturally "knows", like which physical register address
>   the FIFO is in or burst width etc and the DMA engine has no
>   business knowing.
> 
> 2) That needs to change at runtime, like for example how the PL022
>  driver request 32, 16 or 8 bit wide transfers depending on bus
>  width.
> 
> I think master mode could very well be under (1). So the driver knows
> if this hardware expects the DMA engine to drive the transaction or
> if it's the device itself that should drive it.
> 
> So I'm starting to think like you :-)

:)

>> One more thing. I missed few things in this patch:
>> - Need to update all instances of struct dma_slave_config with
>>        .device_fc = false
> 
> All statically defined structs contain zero == false by default
> so it's not needed.
> 
> Make sure any dynamic allocations (I don't know of any!)
> are kzalloc() though.
> 

I already fixed these in V2.
Most of the drivers created inside routines. They are not getting
initialized to _zero_ , so i had to fix it.

There are few though, who used kzalloc. I didn't touch them.

-- 
viresh

[PATCH 3/8] dmaengine/amba-pl08x: Take flow controller info from DMA_SLAVE_CONFIG

[Viresh Kumar]

Flow controller information is passed now from DMA_SLAVE_CONFIG option. This
patch makes changes in pl08x driver to use device_fc from it instead of platform
data.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/dma/amba-pl08x.c   |    4 +++-
 include/linux/amba/pl08x.h |    8 ++++----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 9ebceca..e1a71f0 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1139,6 +1139,8 @@ static int dma_set_runtime_config(struct dma_chan *chan,
 	cctl |= burst << PL080_CONTROL_SB_SIZE_SHIFT;
 	cctl |= burst << PL080_CONTROL_DB_SIZE_SHIFT;
 
+	plchan->device_fc = config->device_fc;
+
 	if (plchan->runtime_direction == DMA_DEV_TO_MEM) {
 		plchan->src_addr = config->src_addr;
 		plchan->src_cctl = pl08x_cctl(cctl) | PL080_CONTROL_DST_INCR |
@@ -1370,7 +1372,7 @@ static struct dma_async_tx_descriptor *pl08x_prep_slave_sg(
 		return NULL;
 	}
 
-	if (plchan->cd->device_fc)
+	if (plchan->device_fc)
 		tmp = (direction == DMA_MEM_TO_DEV) ? PL080_FLOW_MEM2PER_PER :
 			PL080_FLOW_PER2MEM_PER;
 	else
diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h
index 033f6aa..2c58853 100644
--- a/include/linux/amba/pl08x.h
+++ b/include/linux/amba/pl08x.h
@@ -47,9 +47,6 @@ enum {
  * @muxval: a number usually used to poke into some mux regiser to
  * mux in the signal to this channel
  * @cctl_opt: default options for the channel control register
- * @device_fc: Flow Controller Settings for ccfg register. Only valid for slave
- * channels. Fill with 'true' if peripheral should be flow controller. Direction
- * will be selected at Runtime.
  * @addr: source/target address in physical memory for this DMA channel,
  * can be the address of a FIFO register for burst requests for example.
  * This can be left undefined if the PrimeCell API is used for configuring
@@ -68,7 +65,6 @@ struct pl08x_channel_data {
 	int max_signal;
 	u32 muxval;
 	u32 cctl;
-	bool device_fc;
 	dma_addr_t addr;
 	bool circular_buffer;
 	bool single;
@@ -183,6 +179,9 @@ enum pl08x_dma_chan_state {
  * @host: a pointer to the host (internal use)
  * @state: whether the channel is idle, paused, running etc
  * @slave: whether this channel is a device (slave) or for memcpy
+ * @device_fc: Flow Controller Settings for ccfg register. Only valid for slave
+ * channels. Fill with 'true' if peripheral should be flow controller. Direction
+ * will be selected at Runtime.
  * @waiting: a TX descriptor on this channel which is waiting for a physical
  * channel to become available
  */
@@ -205,6 +204,7 @@ struct pl08x_dma_chan {
 	struct pl08x_driver_data *host;
 	enum pl08x_dma_chan_state state;
 	bool slave;
+	bool device_fc;
 	struct pl08x_txd *waiting;
 };
 
-- 
1.7.8.110.g4cb5d

[PATCH 4/8] dmaengine/dw_dmac: Don't use magic number for total number of channels

[Viresh Kumar]

Total number of channels is passed in pdata->nr_channels variable, thus we must
not use magic number '7' for total number of channels.

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

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 3a3d7ed..1577394 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -1432,7 +1432,7 @@ static int __init dw_probe(struct platform_device *pdev)
 
 		/* 7 is highest priority & 0 is lowest. */
 		if (pdata->chan_priority == CHAN_PRIORITY_ASCENDING)
-			dwc->priority = 7 - i;
+			dwc->priority = pdata->nr_channels - i - 1;
 		else
 			dwc->priority = i;
 
-- 
1.7.8.110.g4cb5d

[PATCH 5/8] dmaengine/dw_dmac: Use platform_get_drvdata instead of accessing dev directly

[Viresh Kumar]

Use already defined function platform_get_drvdata() instead of accessing
pdev->dev.data.

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

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 1577394..f061daa 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -1369,7 +1369,7 @@ static int __init dw_probe(struct platform_device *pdev)
 	int			err;
 	int			i;
 
-	pdata = pdev->dev.platform_data;
+	pdata = platform_get_drvdata(pdev);
 	if (!pdata || pdata->nr_channels > DW_DMA_MAX_NR_CHANNELS)
 		return -EINVAL;
 
-- 
1.7.8.110.g4cb5d

Re: [PATCH 5/8] dmaengine/dw_dmac: Use platform_get_drvdata instead of accessing dev directly

[Sergei Shtylyov]

Hello.

On 16-01-2012 13:50, Viresh Kumar wrote:

> Use already defined function platform_get_drvdata() instead of accessing
> pdev->dev.data.

    pdev->dev.platform_data.

> Signed-off-by: Viresh Kumar<viresh.kumar@st.com>

WBR, Sergei

Re: [PATCH 5/8] dmaengine/dw_dmac: Use platform_get_drvdata instead of accessing dev directly

[Viresh Kumar]

On 1/16/2012 4:17 PM, Sergei Shtylyov wrote:
>> > Use already defined function platform_get_drvdata() instead of accessing
>> > pdev->dev.data.
>     pdev->dev.platform_data.
> 

Oops!! Will fix comment once i get comments over other patches too.

-- 
viresh

[PATCH 6/8] dmaengine/dw_dmac: Don't handle block interrupts

[Viresh Kumar]

Block interrupts give interrupt on completion of every LLI, which is actually
too much interrupts. This is just not required for current functioning of
dw_dmac.

So, just don't handle them at all.

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

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index f061daa..8698d3b 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -192,7 +192,6 @@ static void dwc_initialize(struct dw_dma_chan *dwc)
 
 	/* Enable interrupts */
 	channel_set_bit(dw, MASK.XFER, dwc->mask);
-	channel_set_bit(dw, MASK.BLOCK, dwc->mask);
 	channel_set_bit(dw, MASK.ERROR, dwc->mask);
 
 	dwc->initialized = true;
@@ -329,12 +328,6 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 	unsigned long flags;
 
 	spin_lock_irqsave(&dwc->lock, flags);
-	/*
-	 * Clear block interrupt flag before scanning so that we don't
-	 * miss any, and read LLP before RAW_XFER to ensure it is
-	 * valid if we decide to scan the list.
-	 */
-	dma_writel(dw, CLEAR.BLOCK, dwc->mask);
 	llp = channel_readl(dwc, LLP);
 	status_xfer = dma_readl(dw, RAW.XFER);
 
@@ -470,17 +463,16 @@ EXPORT_SYMBOL(dw_dma_get_dst_addr);
 
 /* called with dwc->lock held and all DMAC interrupts disabled */
 static void dwc_handle_cyclic(struct dw_dma *dw, struct dw_dma_chan *dwc,
-		u32 status_block, u32 status_err, u32 status_xfer)
+		u32 status_err, u32 status_xfer)
 {
 	unsigned long flags;
 
-	if (status_block & dwc->mask) {
+	if (dwc->mask) {
 		void (*callback)(void *param);
 		void *callback_param;
 
 		dev_vdbg(chan2dev(&dwc->chan), "new cyclic period llp 0x%08x\n",
 				channel_readl(dwc, LLP));
-		dma_writel(dw, CLEAR.BLOCK, dwc->mask);
 
 		callback = dwc->cdesc->period_callback;
 		callback_param = dwc->cdesc->period_callback_param;
@@ -520,7 +512,6 @@ static void dwc_handle_cyclic(struct dw_dma *dw, struct dw_dma_chan *dwc,
 		channel_writel(dwc, CTL_LO, 0);
 		channel_writel(dwc, CTL_HI, 0);
 
-		dma_writel(dw, CLEAR.BLOCK, dwc->mask);
 		dma_writel(dw, CLEAR.ERROR, dwc->mask);
 		dma_writel(dw, CLEAR.XFER, dwc->mask);
 
@@ -537,36 +528,29 @@ static void dw_dma_tasklet(unsigned long data)
 {
 	struct dw_dma *dw = (struct dw_dma *)data;
 	struct dw_dma_chan *dwc;
-	u32 status_block;
 	u32 status_xfer;
 	u32 status_err;
 	int i;
 
-	status_block = dma_readl(dw, RAW.BLOCK);
 	status_xfer = dma_readl(dw, RAW.XFER);
 	status_err = dma_readl(dw, RAW.ERROR);
 
-	dev_vdbg(dw->dma.dev, "tasklet: status_block=%x status_err=%x\n",
-			status_block, status_err);
+	dev_vdbg(dw->dma.dev, "tasklet: status_err=%x\n", status_err);
 
 	for (i = 0; i < dw->dma.chancnt; i++) {
 		dwc = &dw->chan[i];
 		if (test_bit(DW_DMA_IS_CYCLIC, &dwc->flags))
-			dwc_handle_cyclic(dw, dwc, status_block, status_err,
-					status_xfer);
+			dwc_handle_cyclic(dw, dwc, status_err, status_xfer);
 		else if (status_err & (1 << i))
 			dwc_handle_error(dw, dwc);
-		else if ((status_block | status_xfer) & (1 << i))
+		else if (status_xfer & (1 << i))
 			dwc_scan_descriptors(dw, dwc);
 	}
 
 	/*
-	 * Re-enable interrupts. Block Complete interrupts are only
-	 * enabled if the INT_EN bit in the descriptor is set. This
-	 * will trigger a scan before the whole list is done.
+	 * Re-enable interrupts.
 	 */
 	channel_set_bit(dw, MASK.XFER, dw->all_chan_mask);
-	channel_set_bit(dw, MASK.BLOCK, dw->all_chan_mask);
 	channel_set_bit(dw, MASK.ERROR, dw->all_chan_mask);
 }
 
@@ -583,7 +567,6 @@ static irqreturn_t dw_dma_interrupt(int irq, void *dev_id)
 	 * softirq handler.
 	 */
 	channel_clear_bit(dw, MASK.XFER, dw->all_chan_mask);
-	channel_clear_bit(dw, MASK.BLOCK, dw->all_chan_mask);
 	channel_clear_bit(dw, MASK.ERROR, dw->all_chan_mask);
 
 	status = dma_readl(dw, STATUS_INT);
@@ -594,7 +577,6 @@ static irqreturn_t dw_dma_interrupt(int irq, void *dev_id)
 
 		/* Try to recover */
 		channel_clear_bit(dw, MASK.XFER, (1 << 8) - 1);
-		channel_clear_bit(dw, MASK.BLOCK, (1 << 8) - 1);
 		channel_clear_bit(dw, MASK.SRC_TRAN, (1 << 8) - 1);
 		channel_clear_bit(dw, MASK.DST_TRAN, (1 << 8) - 1);
 		channel_clear_bit(dw, MASK.ERROR, (1 << 8) - 1);
@@ -1068,7 +1050,6 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
 
 	/* Disable interrupts */
 	channel_clear_bit(dw, MASK.XFER, dwc->mask);
-	channel_clear_bit(dw, MASK.BLOCK, dwc->mask);
 	channel_clear_bit(dw, MASK.ERROR, dwc->mask);
 
 	spin_unlock_irqrestore(&dwc->lock, flags);
@@ -1120,7 +1101,6 @@ int dw_dma_cyclic_start(struct dma_chan *chan)
 		return -EBUSY;
 	}
 
-	dma_writel(dw, CLEAR.BLOCK, dwc->mask);
 	dma_writel(dw, CLEAR.ERROR, dwc->mask);
 	dma_writel(dw, CLEAR.XFER, dwc->mask);
 
@@ -1322,7 +1302,6 @@ void dw_dma_cyclic_free(struct dma_chan *chan)
 	while (dma_readl(dw, CH_EN) & dwc->mask)
 		cpu_relax();
 
-	dma_writel(dw, CLEAR.BLOCK, dwc->mask);
 	dma_writel(dw, CLEAR.ERROR, dwc->mask);
 	dma_writel(dw, CLEAR.XFER, dwc->mask);
 
@@ -1347,7 +1326,6 @@ static void dw_dma_off(struct dw_dma *dw)
 	dma_writel(dw, CFG, 0);
 
 	channel_clear_bit(dw, MASK.XFER, dw->all_chan_mask);
-	channel_clear_bit(dw, MASK.BLOCK, dw->all_chan_mask);
 	channel_clear_bit(dw, MASK.SRC_TRAN, dw->all_chan_mask);
 	channel_clear_bit(dw, MASK.DST_TRAN, dw->all_chan_mask);
 	channel_clear_bit(dw, MASK.ERROR, dw->all_chan_mask);
@@ -1449,13 +1427,11 @@ static int __init dw_probe(struct platform_device *pdev)
 
 	/* Clear/disable all interrupts on all channels. */
 	dma_writel(dw, CLEAR.XFER, dw->all_chan_mask);
-	dma_writel(dw, CLEAR.BLOCK, dw->all_chan_mask);
 	dma_writel(dw, CLEAR.SRC_TRAN, dw->all_chan_mask);
 	dma_writel(dw, CLEAR.DST_TRAN, dw->all_chan_mask);
 	dma_writel(dw, CLEAR.ERROR, dw->all_chan_mask);
 
 	channel_clear_bit(dw, MASK.XFER, dw->all_chan_mask);
-	channel_clear_bit(dw, MASK.BLOCK, dw->all_chan_mask);
 	channel_clear_bit(dw, MASK.SRC_TRAN, dw->all_chan_mask);
 	channel_clear_bit(dw, MASK.DST_TRAN, dw->all_chan_mask);
 	channel_clear_bit(dw, MASK.ERROR, dw->all_chan_mask);
-- 
1.7.8.110.g4cb5d

[PATCH 7/8] dmaengine/dw_dmac: Unmap all memory buffers after completion of slave transfers

[Viresh Kumar]

Currently, after completion of transfer, source address or destination address
of only the first LLI descriptor is unmapped. And length passed for unmap is
total length of all descriptors in the list. Which means unmapping code assumed
that the memory buffers pointed to by the descriptors will be physically
contiguous, which might not be the case. It is possible for other drivers to
pass sglist to slave_sg(), in which all buffers are scattered throughout the
memory.

This patch intends to fix this wrong expectation of dw_dmac. Now, first desc
will not contain total length of transfer. But individual descriptors will
contain their individual lengths. Finally, we will call unmap for all
descriptors.

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

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 8698d3b..c33c06b 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -231,6 +231,14 @@ static void dwc_dostart(struct dw_dma_chan *dwc, struct dw_desc *first)
 
 /*----------------------------------------------------------------------*/
 
+#define dw_dmac_unmap(utype, _desc, _child, _buf, _dir)			\
+do {									\
+	list_for_each_entry(_child, &_desc->tx_list, desc_node)		\
+		dma_unmap_##utype(parent, _child->lli._buf,		\
+				_child->len, _dir);			\
+	dma_unmap_##utype(parent, _desc->lli._buf, _desc->len, _dir);	\
+} while (0)
+
 static void
 dwc_descriptor_complete(struct dw_dma_chan *dwc, struct dw_desc *desc,
 		bool callback_required)
@@ -264,19 +272,19 @@ dwc_descriptor_complete(struct dw_dma_chan *dwc, struct dw_desc *desc,
 		struct device *parent = chan2parent(&dwc->chan);
 		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);
+				dw_dmac_unmap(single, desc, child, dar,
+						DMA_FROM_DEVICE);
 			else
-				dma_unmap_page(parent, desc->lli.dar,
-						desc->len, DMA_FROM_DEVICE);
+				dw_dmac_unmap(page, desc, child, dar,
+						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);
+				dw_dmac_unmap(single, desc, child, sar,
+						DMA_TO_DEVICE);
 			else
-				dma_unmap_page(parent, desc->lli.sar,
-						desc->len, DMA_TO_DEVICE);
+				dw_dmac_unmap(page, desc, child, sar,
+						DMA_TO_DEVICE);
 		}
 	}
 
@@ -676,6 +684,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;
@@ -701,7 +710,6 @@ dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
 			DMA_TO_DEVICE);
 
 	first->txd.flags = flags;
-	first->len = len;
 
 	return &first->txd;
 
@@ -725,7 +733,6 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 	unsigned int		mem_width;
 	unsigned int		i;
 	struct scatterlist	*sg;
-	size_t			total_len = 0;
 
 	dev_vdbg(chan2dev(chan), "prep_dma_slave\n");
 
@@ -774,6 +781,7 @@ slave_sg_todev_fill_desc:
 			}
 
 			desc->lli.ctlhi = dlen >> mem_width;
+			desc->len = dlen;
 
 			if (!first) {
 				first = desc;
@@ -787,7 +795,6 @@ slave_sg_todev_fill_desc:
 						&first->tx_list);
 			}
 			prev = desc;
-			total_len += dlen;
 
 			if (len)
 				goto slave_sg_todev_fill_desc;
@@ -831,6 +838,7 @@ slave_sg_fromdev_fill_desc:
 				len = 0;
 			}
 			desc->lli.ctlhi = dlen >> reg_width;
+			desc->len = dlen;
 
 			if (!first) {
 				first = desc;
@@ -844,7 +852,6 @@ slave_sg_fromdev_fill_desc:
 						&first->tx_list);
 			}
 			prev = desc;
-			total_len += dlen;
 
 			if (len)
 				goto slave_sg_fromdev_fill_desc;
@@ -863,8 +870,6 @@ slave_sg_fromdev_fill_desc:
 			prev->txd.phys, sizeof(prev->lli),
 			DMA_TO_DEVICE);
 
-	first->len = total_len;
-
 	return &first->txd;
 
 err_desc_get:
@@ -950,11 +955,19 @@ dwc_tx_status(struct dma_chan *chan,
 		ret = dma_async_is_complete(cookie, last_complete, last_used);
 	}
 
-	if (ret != DMA_SUCCESS)
-		dma_set_tx_state(txstate, last_complete, last_used,
-				dwc_first_active(dwc)->len);
-	else
+	if (ret != DMA_SUCCESS) {
+		struct dw_desc *desc, *child;
+		unsigned int len;
+
+		desc = dwc_first_active(dwc);
+		len = desc->len;
+		list_for_each_entry(child, &desc->tx_list, desc_node)
+			len += child->len;
+
+		dma_set_tx_state(txstate, last_complete, last_used, len);
+	} else {
 		dma_set_tx_state(txstate, last_complete, last_used, 0);
+	}
 
 	if (dwc->paused)
 		return DMA_PAUSED;
-- 
1.7.8.110.g4cb5d

Re: [PATCH 7/8] dmaengine/dw_dmac: Unmap all memory buffers after completion of slave transfers

[Russell King - ARM Linux]

On Mon, Jan 16, 2012 at 03:20:35PM +0530, Viresh Kumar wrote:
> Currently, after completion of transfer, source address or destination address
> of only the first LLI descriptor is unmapped. And length passed for unmap is
> total length of all descriptors in the list. Which means unmapping code assumed
> that the memory buffers pointed to by the descriptors will be physically
> contiguous, which might not be the case. It is possible for other drivers to
> pass sglist to slave_sg(), in which all buffers are scattered throughout the
> memory.
> 
> This patch intends to fix this wrong expectation of dw_dmac. Now, first desc
> will not contain total length of transfer. But individual descriptors will
> contain their individual lengths. Finally, we will call unmap for all
> descriptors.

Note that DMA engine drivers are not responsible for unmapping the buffers
when the transfer completes - that is the responsibility of the caller.

The automatic buffer unmapping is required for the async_tx APIs and
offload APIs.

Re: [PATCH 7/8] dmaengine/dw_dmac: Unmap all memory buffers after completion of slave transfers

[Viresh Kumar]

On 1/16/2012 4:24 PM, Russell King - ARM Linux wrote:
> Note that DMA engine drivers are not responsible for unmapping the buffers
> when the transfer completes - that is the responsibility of the caller.
> 
> The automatic buffer unmapping is required for the async_tx APIs and
> offload APIs.

In dw_dmac, it is only done for slave transfers. Is this Okay ??

-- 
viresh

Re: [PATCH 7/8] dmaengine/dw_dmac: Unmap all memory buffers after completion of slave transfers

[Linus Walleij]

On Mon, Jan 16, 2012 at 12:44 PM, Viresh Kumar <viresh.kumar@st.com> wrote:
> On 1/16/2012 4:24 PM, Russell King - ARM Linux wrote:
>> Note that DMA engine drivers are not responsible for unmapping the buffers
>> when the transfer completes - that is the responsibility of the caller.
>>
>> The automatic buffer unmapping is required for the async_tx APIs and
>> offload APIs.
>
> In dw_dmac, it is only done for slave transfers. Is this Okay ??

Basically the driver using the dmaengine shall map/unmap buffers
used for slave transfers, not the driver.

I usually map them before setting up a transfer and unmaps them
when egtting the callback from the DMA engine that the transfer is
complete.

Why do you want to do this?

Yours,
Linus Walleij

Re: [PATCH 7/8] dmaengine/dw_dmac: Unmap all memory buffers after completion of slave transfers

[Viresh Kumar]

On 1/17/2012 2:22 PM, Linus Walleij wrote:
> Basically the driver using the dmaengine shall map/unmap buffers
> used for slave transfers, not the driver.
> 
> I usually map them before setting up a transfer and unmaps them
> when egtting the callback from the DMA engine that the transfer is
> complete.
> 

You implemented similar stuff in amba-pl08x :) .

if (!plchan->slave)
	pl08x_unmap_buffers(txd);

> Why do you want to do this?

This is because people can pass in flags which are
DMA_COMPL_SKIP_SRC_UNMAP, etc.

If they are not passed, then DMA driver must do it for them.

Sorry if i am understanding the DMA logic incorrectly.

-- 
viresh

Re: [PATCH 7/8] dmaengine/dw_dmac: Unmap all memory buffers after completion of slave transfers

[Russell King - ARM Linux]

On Tue, Jan 17, 2012 at 02:37:10PM +0530, Viresh Kumar wrote:
> On 1/17/2012 2:22 PM, Linus Walleij wrote:
> > Basically the driver using the dmaengine shall map/unmap buffers
> > used for slave transfers, not the driver.
> > 
> > I usually map them before setting up a transfer and unmaps them
> > when egtting the callback from the DMA engine that the transfer is
> > complete.
> > 
> 
> You implemented similar stuff in amba-pl08x :) .
> 
> if (!plchan->slave)
> 	pl08x_unmap_buffers(txd);

Take a look at that.  "If *NOT* slave, unmap the buffers".  That is in
keeping with what I've been saying.  Slave transfers *do* *not* get
automatic buffer unmapping.

Re: [PATCH 7/8] dmaengine/dw_dmac: Unmap all memory buffers after completion of slave transfers

[Linus Walleij]

On Tue, Jan 17, 2012 at 10:07 AM, Viresh Kumar <viresh.kumar@st.com> wrote:

>> Why do you want to do this?
>
> This is because people can pass in flags which are
> DMA_COMPL_SKIP_SRC_UNMAP, etc.
>
> If they are not passed, then DMA driver must do it for them.

OK I see why it's confusing.

That flag is only for memcpy jobs.

Only memcpy jobs maps/unmaps buffers.

Slave transfers are mapped/unmapped by the driver.

Yours,
Linus Walleij

[PATCH 8/8] dmaengine/dw_dmac: Add support for DMA_SLAVE_CONFIG

[Viresh Kumar]

This patch adds support for DMA_SLAVE_CONFIG in dwc DMAC controller.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/dma/dw_dmac.c      |   78 ++++++++++++++++++++++++++++++++------------
 drivers/dma/dw_dmac_regs.h |    3 ++
 include/linux/dw_dmac.h    |    6 ---
 3 files changed, 60 insertions(+), 27 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index c33c06b..6252dce 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -33,12 +33,14 @@
  * which does not support descriptor writeback.
  */
 
-#define DWC_DEFAULT_CTLLO(private) ({				\
-		struct dw_dma_slave *__slave = (private);	\
+#define DWC_DEFAULT_CTLLO(chan) ({				\
+		struct dw_dma_slave *__slave = (chan->private);	\
+		struct dw_dma_chan *dwc = to_dw_dma_chan(chan);	\
+		struct dma_slave_config	*sconfig = &dwc->dma_sconfig;	\
 		int dms = __slave ? __slave->dst_master : 0;	\
 		int sms = __slave ? __slave->src_master : 1;	\
-		u8 smsize = __slave ? __slave->src_msize : DW_DMA_MSIZE_16; \
-		u8 dmsize = __slave ? __slave->dst_msize : DW_DMA_MSIZE_16; \
+		u8 smsize = __slave ? sconfig->src_maxburst : DW_DMA_MSIZE_16; \
+		u8 dmsize = __slave ? sconfig->dst_maxburst : DW_DMA_MSIZE_16; \
 								\
 		(DWC_CTLL_DST_MSIZE(dmsize)			\
 		 | DWC_CTLL_SRC_MSIZE(smsize)			\
@@ -664,7 +666,7 @@ dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
 	else
 		src_width = dst_width = 0;
 
-	ctllo = DWC_DEFAULT_CTLLO(chan->private)
+	ctllo = DWC_DEFAULT_CTLLO(chan)
 			| DWC_CTLL_DST_WIDTH(dst_width)
 			| DWC_CTLL_SRC_WIDTH(src_width)
 			| DWC_CTLL_DST_INC
@@ -725,6 +727,7 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 {
 	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
 	struct dw_dma_slave	*dws = chan->private;
+	struct dma_slave_config	*sconfig = &dwc->dma_sconfig;
 	struct dw_desc		*prev;
 	struct dw_desc		*first;
 	u32			ctllo;
@@ -739,17 +742,18 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 	if (unlikely(!dws || !sg_len))
 		return NULL;
 
-	reg_width = dws->reg_width;
 	prev = first = NULL;
 
 	switch (direction) {
 	case DMA_MEM_TO_DEV:
-		ctllo = (DWC_DEFAULT_CTLLO(chan->private)
+		reg_width = sconfig->dst_addr_width;
+		reg = sconfig->dst_addr;
+		ctllo = (DWC_DEFAULT_CTLLO(chan)
 				| DWC_CTLL_DST_WIDTH(reg_width)
 				| DWC_CTLL_DST_FIX
 				| DWC_CTLL_SRC_INC
-				| DWC_CTLL_FC(dws->fc));
-		reg = dws->tx_reg;
+				| DWC_CTLL_FC(sconfig->device_fc));
+
 		for_each_sg(sgl, sg, sg_len, i) {
 			struct dw_desc	*desc;
 			u32		len, dlen, mem;
@@ -801,13 +805,14 @@ slave_sg_todev_fill_desc:
 		}
 		break;
 	case DMA_DEV_TO_MEM:
-		ctllo = (DWC_DEFAULT_CTLLO(chan->private)
+		reg_width = sconfig->src_addr_width;
+		reg = sconfig->src_addr;
+		ctllo = (DWC_DEFAULT_CTLLO(chan)
 				| DWC_CTLL_SRC_WIDTH(reg_width)
 				| DWC_CTLL_DST_INC
 				| DWC_CTLL_SRC_FIX
-				| DWC_CTLL_FC(dws->fc));
+				| DWC_CTLL_FC(sconfig->device_fc));
 
-		reg = dws->rx_reg;
 		for_each_sg(sgl, sg, sg_len, i) {
 			struct dw_desc	*desc;
 			u32		len, dlen, mem;
@@ -877,6 +882,29 @@ err_desc_get:
 	return NULL;
 }
 
+static int
+set_runtime_config(struct dma_chan *chan, struct dma_slave_config *sconfig)
+{
+	struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
+
+	/* Check if it is chan is configured for slave transfers */
+	if (!chan->private)
+		return -EINVAL;
+
+	memcpy(&dwc->dma_sconfig, sconfig, sizeof(*sconfig));
+
+	/*
+	 * Fix sconfig's burst size according to dw_dmac. We need to convert
+	 * them as: 1 -> 0, 2 -> 1, 4 -> 2, 8 -> 3, 16 -> 4.
+	 *
+	 * This can be done by findiding least significant bit set: n & (n - 1)
+	 */
+	sconfig->src_maxburst &= sconfig->src_maxburst - 1;
+	sconfig->dst_maxburst &= sconfig->dst_maxburst - 1;
+
+	return 0;
+}
+
 static int dwc_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 		       unsigned long arg)
 {
@@ -926,8 +954,11 @@ 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, false);
-	} else
+	} else if (cmd == DMA_SLAVE_CONFIG) {
+		return set_runtime_config(chan, (struct dma_slave_config *)arg);
+	} else {
 		return -ENXIO;
+	}
 
 	return 0;
 }
@@ -1168,11 +1199,11 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
 		enum dma_transfer_direction direction)
 {
 	struct dw_dma_chan		*dwc = to_dw_dma_chan(chan);
+	struct dma_slave_config		*sconfig = &dwc->dma_sconfig;
 	struct dw_cyclic_desc		*cdesc;
 	struct dw_cyclic_desc		*retval = NULL;
 	struct dw_desc			*desc;
 	struct dw_desc			*last = NULL;
-	struct dw_dma_slave		*dws = chan->private;
 	unsigned long			was_cyclic;
 	unsigned int			reg_width;
 	unsigned int			periods;
@@ -1196,7 +1227,12 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
 	}
 
 	retval = ERR_PTR(-EINVAL);
-	reg_width = dws->reg_width;
+
+	if (direction == DMA_MEM_TO_DEV)
+		reg_width = sconfig->dst_addr_width;
+	else
+		reg_width = sconfig->src_addr_width;
+
 	periods = buf_len / period_len;
 
 	/* Check for too big/unaligned periods and unaligned DMA buffer. */
@@ -1229,25 +1265,25 @@ struct dw_cyclic_desc *dw_dma_cyclic_prep(struct dma_chan *chan,
 
 		switch (direction) {
 		case DMA_MEM_TO_DEV:
-			desc->lli.dar = dws->tx_reg;
+			desc->lli.dar = sconfig->dst_addr;
 			desc->lli.sar = buf_addr + (period_len * i);
-			desc->lli.ctllo = (DWC_DEFAULT_CTLLO(chan->private)
+			desc->lli.ctllo = (DWC_DEFAULT_CTLLO(chan)
 					| DWC_CTLL_DST_WIDTH(reg_width)
 					| DWC_CTLL_SRC_WIDTH(reg_width)
 					| DWC_CTLL_DST_FIX
 					| DWC_CTLL_SRC_INC
-					| DWC_CTLL_FC(dws->fc)
+					| DWC_CTLL_FC(sconfig->device_fc)
 					| DWC_CTLL_INT_EN);
 			break;
 		case DMA_DEV_TO_MEM:
 			desc->lli.dar = buf_addr + (period_len * i);
-			desc->lli.sar = dws->rx_reg;
-			desc->lli.ctllo = (DWC_DEFAULT_CTLLO(chan->private)
+			desc->lli.sar = sconfig->src_addr;
+			desc->lli.ctllo = (DWC_DEFAULT_CTLLO(chan)
 					| DWC_CTLL_SRC_WIDTH(reg_width)
 					| DWC_CTLL_DST_WIDTH(reg_width)
 					| DWC_CTLL_DST_INC
 					| DWC_CTLL_SRC_FIX
-					| DWC_CTLL_FC(dws->fc)
+					| DWC_CTLL_FC(sconfig->device_fc)
 					| DWC_CTLL_INT_EN);
 			break;
 		default:
diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw_dmac_regs.h
index 5eef694..2005d30 100644
--- a/drivers/dma/dw_dmac_regs.h
+++ b/drivers/dma/dw_dmac_regs.h
@@ -153,6 +153,9 @@ struct dw_dma_chan {
 	struct dw_cyclic_desc	*cdesc;
 
 	unsigned int		descs_allocated;
+
+	/* configuration passed via DMA_SLAVE_CONFIG */
+	struct dma_slave_config dma_sconfig;
 };
 
 static inline struct dw_dma_chan_regs __iomem *
diff --git a/include/linux/dw_dmac.h b/include/linux/dw_dmac.h
index f2c64f9..67271d8 100644
--- a/include/linux/dw_dmac.h
+++ b/include/linux/dw_dmac.h
@@ -86,16 +86,10 @@ enum dw_dma_fc {
  */
 struct dw_dma_slave {
 	struct device		*dma_dev;
-	dma_addr_t		tx_reg;
-	dma_addr_t		rx_reg;
-	enum dw_dma_slave_width	reg_width;
 	u32			cfg_hi;
 	u32			cfg_lo;
 	u8			src_master;
 	u8			dst_master;
-	u8			src_msize;
-	u8			dst_msize;
-	u8			fc;
 };
 
 /* Platform-configurable bits in CFG_HI */
-- 
1.7.8.110.g4cb5d

Re: [PATCH 8/8] dmaengine/dw_dmac: Add support for DMA_SLAVE_CONFIG

[Viresh Kumar]

On 1/16/2012 3:20 PM, Viresh KUMAR wrote:
> +	/*
> +	 * Fix sconfig's burst size according to dw_dmac. We need to convert
> +	 * them as: 1 -> 0, 2 -> 1, 4 -> 2, 8 -> 3, 16 -> 4.
> +	 *
> +	 * This can be done by findiding least significant bit set: n & (n - 1)
> +	 */
> +	sconfig->src_maxburst &= sconfig->src_maxburst - 1;
> +	sconfig->dst_maxburst &= sconfig->dst_maxburst - 1;

Perhaps, this looks incorrect. It will always return 0. :(
Can somebody suggest any inbuild function to do this, i think

find_next_bit(sconfig->src_maxburst, sizeof(sconfig->src_maxburst), 0)

will do it.

-- 
viresh

Re: [PATCH 8/8] dmaengine/dw_dmac: Add support for DMA_SLAVE_CONFIG

[Linus Walleij]

On Mon, Jan 16, 2012 at 12:42 PM, Viresh Kumar <viresh.kumar@st.com> wrote:
> On 1/16/2012 3:20 PM, Viresh KUMAR wrote:
>> +     /*
>> +      * Fix sconfig's burst size according to dw_dmac. We need to convert
>> +      * them as: 1 -> 0, 2 -> 1, 4 -> 2, 8 -> 3, 16 -> 4.
>> +      *
>> +      * This can be done by findiding least significant bit set: n & (n - 1)
>> +      */
>> +     sconfig->src_maxburst &= sconfig->src_maxburst - 1;
>> +     sconfig->dst_maxburst &= sconfig->dst_maxburst - 1;
>
> Perhaps, this looks incorrect. It will always return 0. :(
> Can somebody suggest any inbuild function to do this, i think
>
> find_next_bit(sconfig->src_maxburst, sizeof(sconfig->src_maxburst), 0)
>
> will do it.

Are you looking for ffs() from <linus/bitops.h>?

find-first-set (the include boils down to include/asm-generic/bitops/ffs.h
if you want to check the implementation).

Linus Walleij

Re: [PATCH 8/8] dmaengine/dw_dmac: Add support for DMA_SLAVE_CONFIG

[Viresh Kumar]

On 1/17/2012 2:19 PM, Linus Walleij wrote:
> Are you looking for ffs() from <linus/bitops.h>?
> 
> find-first-set (the include boils down to include/asm-generic/bitops/ffs.h
> if you want to check the implementation).

Thanks, but it might not solve the issue. :(
Actually this will retain the first Least Significant Bit (which is 1)
and will mark all other zero. So it will give output 0b10 for 0b110

>>> >> +      * Fix sconfig's burst size according to dw_dmac. We need to convert
>>> >> +      * them as: 1 -> 0, 2 -> 1, 4 -> 2, 8 -> 3, 16 -> 4.

I need above conversion. i.e. finding bit no. of first bit set.

-- 
viresh

Re: [PATCH 8/8] dmaengine/dw_dmac: Add support for DMA_SLAVE_CONFIG

[Viresh Kumar]

On 1/17/2012 2:45 PM, Viresh Kumar wrote:
>>>>>> >>> >> +      * Fix sconfig's burst size according to dw_dmac. We need to convert
>>>>>> >>> >> +      * them as: 1 -> 0, 2 -> 1, 4 -> 2, 8 -> 3, 16 -> 4.
> I need above conversion. i.e. finding bit no. of first bit set.

Something like:

unsigned int v; // 32-bit word
unsigned int r = 0;

while (v >>= 1)
{
  r++;
}

Probably find_first_bit() suits it.

-- 
viresh

Re: [PATCH 8/8] dmaengine/dw_dmac: Add support for DMA_SLAVE_CONFIG

[Viresh Kumar]

On 1/17/2012 2:45 PM, Viresh Kumar wrote:
> On 1/17/2012 2:19 PM, Linus Walleij wrote:
>> > Are you looking for ffs() from <linus/bitops.h>?
>> > 
>> > find-first-set (the include boils down to include/asm-generic/bitops/ffs.h
>> > if you want to check the implementation).
> Thanks, but it might not solve the issue. :(
> Actually this will retain the first Least Significant Bit (which is 1)
> and will mark all other zero. So it will give output 0b10 for 0b110
> 

Oops!! I didn't looked at fls in the implementation. :(
You are right. This will solve my issue.

-- 
viresh

Re: [PATCH 8/8] dmaengine/dw_dmac: Add support for DMA_SLAVE_CONFIG

[Jassi Brar]

On 16 January 2012 17:12, Viresh Kumar <viresh.kumar@st.com> wrote:
> On 1/16/2012 3:20 PM, Viresh KUMAR wrote:
>> +     /*
>> +      * Fix sconfig's burst size according to dw_dmac. We need to convert
>> +      * them as: 1 -> 0, 2 -> 1, 4 -> 2, 8 -> 3, 16 -> 4.
>> +      *
>> +      * This can be done by findiding least significant bit set: n & (n - 1)
>> +      */
>> +     sconfig->src_maxburst &= sconfig->src_maxburst - 1;
>> +     sconfig->dst_maxburst &= sconfig->dst_maxburst - 1;
>
> Perhaps, this looks incorrect. It will always return 0. :(
> Can somebody suggest any inbuild function to do this, i think
>
Calculating log2(n) which isn't very tidy. For that reason usually we make do
the other way around by saving values as power of 2 and simply left shifting
1 by that value.
In case at hand since most likely only valid values are power of 2,
maybe you can do
    maxburst = ffs(maxburst) - 1

-j

Re: [PATCH 8/8] dmaengine/dw_dmac: Add support for DMA_SLAVE_CONFIG

[Viresh Kumar]

On 1/17/2012 3:40 PM, Jassi Brar wrote:
> In case at hand since most likely only valid values are power of 2,
> maybe you can do
>     maxburst = ffs(maxburst) - 1

Thanks.

-- 
viresh