dmaengine: Pl08x and dw_dmac updates

PATCH New Viresh Kumar 5 participants 28 messages Jan 16, 2012 – Jan 19, 2012 dmaengine

lore

Share & tools

lore

Viresh Kumar

author

2012-01-16 09:50 UTC #0 lore raw

cover dmaengine: Pl08x and dw_dmac updates

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(-)

Signature · 1 line

1.7.8.110.g4cb5d

Viresh Kumar

author

2012-01-16 09:50 UTC #1 lore raw

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

From: Rajeev KUMAR <[email protected]>

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 <[email protected]>

Acked-by Viresh Kumar <[email protected]>

---
 drivers/dma/dw_dmac.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

drivers/dma/dw_dmac.c +4 -0

index 9b592b0..3a3d7ed 100644
@@ -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 = {

Signature · 1 line

1.7.8.110.g4cb5d

Viresh Kumar

author

2012-01-16 09:50 UTC #2 lore raw

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

In reply to 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 <[email protected]>

---
 include/linux/dmaengine.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

include/linux/dmaengine.h +5 -0

index 5532bb8..dd1915f 100644
@@ -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)

Signature · 1 line

1.7.8.110.g4cb5d

Linus Walleij

2012-01-17 08:37 UTC #3 lore raw

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

Quoted 6 lines · 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.

Quoted 3 lines

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

Viresh Kumar

author

2012-01-17 09:07 UTC #4 lore raw

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

Quoted 6 lines · Linus Walleij

Quoted 3 lines
Quoted 3 lines
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.

Quoted 2 lines

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.

Quoted 7 lines

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 :)

Signature · 1 line

viresh

Linus Walleij

Acked-by

2012-01-18 10:36 UTC #5 lore raw

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

Quoted 10 lines · Viresh Kumar

Quoted 3 lines · Linus Walleij
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.

Quoted 4 lines

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.

Quoted 5 lines

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 <[email protected]>

to the patch.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 6 lines

Quoted 4 lines
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

Viresh Kumar

author

2012-01-18 10:40 UTC #6 lore raw

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

Quoted 14 lines · Linus Walleij

Quoted 1 line · Viresh Kumar
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.

Quoted 5 lines

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 :-)

Quoted 10 lines

Quoted 3 lines
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.

Signature · 1 line

viresh

Viresh Kumar

author

2012-01-16 09:50 UTC #7 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/dma/amba-pl08x.c   |    4 +++-
 include/linux/amba/pl08x.h |    8 ++++----
 2 files changed, 7 insertions(+), 5 deletions(-)

drivers/dma/amba-pl08x.c +3 -1

index 9ebceca..e1a71f0 100644
@@ -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

include/linux/amba/pl08x.h +4 -4

index 033f6aa..2c58853 100644
@@ -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;
 };

Signature · 1 line

1.7.8.110.g4cb5d

Viresh Kumar

author

2012-01-16 09:50 UTC #8 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/dma/dw_dmac.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/dma/dw_dmac.c +1 -1

index 3a3d7ed..1577394 100644
@@ -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;

Signature · 1 line

1.7.8.110.g4cb5d

Viresh Kumar

author

2012-01-16 09:50 UTC #9 lore raw

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

In reply to Viresh Kumar

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

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/dma/dw_dmac.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/dma/dw_dmac.c +1 -1

index 1577394..f061daa 100644
@@ -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;

Signature · 1 line

1.7.8.110.g4cb5d

Sergei Shtylyov

2012-01-16 10:47 UTC #10 lore raw

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

Hello.

Quoted 2 lines · Viresh Kumar

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

    pdev->dev.platform_data.

Quoted 1 line

Signed-off-by Viresh Kumar<[email protected]>

WBR, Sergei

Viresh Kumar

author

2012-01-16 10:52 UTC #11 lore raw

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

Quoted 4 lines · Sergei Shtylyov

Quoted 2 lines
Quoted 2 lines
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.

Signature · 1 line

viresh

Viresh Kumar

author

2012-01-16 09:50 UTC #12 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/dma/dw_dmac.c |   36 ++++++------------------------------
 1 files changed, 6 insertions(+), 30 deletions(-)

drivers/dma/dw_dmac.c +6 ~~-30~~

index f061daa..8698d3b 100644
@@ -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);

Signature · 1 line

1.7.8.110.g4cb5d

Viresh Kumar

author

2012-01-16 09:50 UTC #13 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/dma/dw_dmac.c |   49 +++++++++++++++++++++++++++++++------------------
 1 files changed, 31 insertions(+), 18 deletions(-)

drivers/dma/dw_dmac.c +31 ~~-18~~

index 8698d3b..c33c06b 100644
@@ -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;

Signature · 1 line

1.7.8.110.g4cb5d

Russell King - ARM Linux

2012-01-16 10:54 UTC #14 lore raw

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

Quoted 8 lines · 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.

Quoted 4 lines

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.

Viresh Kumar

author

2012-01-16 11:44 UTC #15 lore raw

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

Quoted 5 lines · Russell King - ARM Linux

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 ??

Signature · 1 line

viresh

Linus Walleij

2012-01-17 08:52 UTC #16 lore raw

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

Quoted 8 lines · Viresh Kumar

Quoted 5 lines · Russell King - ARM Linux
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

Viresh Kumar

author

2012-01-17 09:07 UTC #17 lore raw

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

Quoted 7 lines · Linus Walleij

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);

Quoted 1 line

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.

Signature · 1 line

viresh

Russell King - ARM Linux

2012-01-18 09:35 UTC #18 lore raw

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

Quoted 9 lines · Viresh Kumar

Quoted 7 lines · Linus Walleij
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.

Quoted 4 lines

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.

Linus Walleij

2012-01-19 17:34 UTC #19 lore raw

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

In reply to Viresh Kumar

Quoted 6 lines · Viresh Kumar

Quoted 1 line
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

Viresh Kumar

author

2012-01-16 09:50 UTC #20 lore raw

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

In reply to Viresh Kumar

This patch adds support for DMA_SLAVE_CONFIG in dwc DMAC controller.

Signed-off-by Viresh Kumar <[email protected]>

---
 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(-)

drivers/dma/dw_dmac.c +57 ~~-21~~

index c33c06b..6252dce 100644
@@ -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:

drivers/dma/dw_dmac_regs.h +3 -0

index 5eef694..2005d30 100644
@@ -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 *

include/linux/dw_dmac.h +0 -6

index f2c64f9..67271d8 100644
@@ -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 */

Signature · 1 line

1.7.8.110.g4cb5d

Viresh Kumar

author

2012-01-16 11:42 UTC #21 lore raw

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

drivers/dma/dw_dmac.c err_desc_get: · line 903

Quoted 8 lines · Viresh KUMAR

drivers/dma/dw_dmac.c +8 -0

+	/*
+	 * 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.

Signature · 1 line

viresh

Linus Walleij

2012-01-17 08:49 UTC #22 lore raw

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

Quoted 10 lines · Viresh Kumar

Quoted 8 lines · Viresh KUMAR
+ ? ? /*
+ ? ? ?* 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;

Quoted 6 lines

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

Viresh Kumar

author

2012-01-17 09:15 UTC #23 lore raw

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

Quoted 4 lines · Linus Walleij

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

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/dma/dw_dmac.c err_desc_get: · line 898
Quoted 2 lines
drivers/dma/dw_dmac.c +2 -0
+      * 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.

Signature · 1 line

viresh

Viresh Kumar

author

2012-01-17 09:19 UTC #24 lore raw

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

Quoted 3 lines · Viresh Kumar

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/dma/dw_dmac.c err_desc_get: · line 898
Quoted 2 lines
drivers/dma/dw_dmac.c +2 -0
+      * 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.

Signature · 1 line

viresh

Viresh Kumar

author

2012-01-17 10:19 UTC #25 lore raw

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

In reply to Viresh Kumar

Quoted 9 lines · Viresh Kumar

Quoted 4 lines · Linus Walleij
Quoted 4 lines
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.

Signature · 1 line

viresh

Jassi Brar

2012-01-17 10:10 UTC #26 lore raw

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

In reply to Viresh Kumar

Quoted 10 lines · Viresh Kumar

Quoted 8 lines · Viresh KUMAR
+ ? ? /*
+ ? ? ?* 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;

Quoted 3 lines

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

Viresh Kumar

author

2012-01-17 10:19 UTC #27 lore raw

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

Quoted 3 lines · Jassi Brar

In case at hand since most likely only valid values are power of 2, maybe you can do
maxburst = ffs(maxburst) - 1

Thanks.

Signature · 1 line

viresh