[PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver

[PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver

[Barry Song]

From: Rongjun Ying <rongjun.ying@csr.com>

Cc: Jassi Brar <jaswinder.singh@linaro.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Rongjun Ying <rongjun.ying@csr.com>
Signed-off-by: Barry Song <Baohua.Song@csr.com>
---
 -v2:
 use generic xfer API from jassi;
 delete sirf self-defined slave config;
 fix feedback from vinod;
 fix filter function: we have two dmac, clients drivers think the chan id as 0~31;
 rename regs to base;
 delete redundant chan_id initialization in probe since dmaengine core will
 re-write it, refer to my patch too:
 [PATCH] dmaengine: delete redundant chan_id and chancnt initialization in dma drivers
 http://www.spinics.net/lists/kernel/msg1237455.html

 this patch doesn't provide a common way for filter and doesn't use the jassi's v2 patch.

 MAINTAINERS            |    1 +
 drivers/dma/Kconfig    |    7 +
 drivers/dma/Makefile   |    1 +
 drivers/dma/sirf-dma.c |  603 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 612 insertions(+), 0 deletions(-)
 create mode 100644 drivers/dma/sirf-dma.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 28f65c2..c1237ba 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -739,6 +739,7 @@ M:	Barry Song <baohua.song@csr.com>
 L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 S:	Maintained
 F:	arch/arm/mach-prima2/
+F:	drivers/dma/sirf-dma*
 
 ARM/EBSA110 MACHINE SUPPORT
 M:	Russell King <linux@arm.linux.org.uk>
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 2e3b3d3..1341bcd 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -187,6 +187,13 @@ config TIMB_DMA
 	help
 	  Enable support for the Timberdale FPGA DMA engine.
 
+config SIRF_DMA
+	tristate "CSR SiRFprimaII DMA support"
+	depends on ARCH_PRIMA2
+	select DMA_ENGINE
+	help
+	  Enable support for the CSR SiRFprimaII DMA engine.
+
 config ARCH_HAS_ASYNC_TX_FIND_CHANNEL
 	bool
 
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index 30cf3b1..009a222 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_IMX_SDMA) += imx-sdma.o
 obj-$(CONFIG_IMX_DMA) += imx-dma.o
 obj-$(CONFIG_MXS_DMA) += mxs-dma.o
 obj-$(CONFIG_TIMB_DMA) += timb_dma.o
+obj-$(CONFIG_SIRF_DMA) += sirf-dma.o
 obj-$(CONFIG_STE_DMA40) += ste_dma40.o ste_dma40_ll.o
 obj-$(CONFIG_PL330_DMA) += pl330.o
 obj-$(CONFIG_PCH_DMA) += pch_dma.o
diff --git a/drivers/dma/sirf-dma.c b/drivers/dma/sirf-dma.c
new file mode 100644
index 0000000..90a4660
--- /dev/null
+++ b/drivers/dma/sirf-dma.c
@@ -0,0 +1,603 @@
+/*
+ * DMA controller driver for CSR SiRFprimaII
+ *
+ * Copyright (c) 2011 Cambridge Silicon Radio Limited, a CSR plc group company.
+ *
+ * Licensed under GPLv2 or later.
+ */
+
+#include <linux/module.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/of_platform.h>
+#include <linux/sirfsoc_dma.h>
+
+#define SIRFSOC_DMA_DESCRIPTORS                 16
+#define SIRFSOC_DMA_CHANNELS                    16
+
+#define SIRFSOC_DMA_CH_ADDR                     0x00
+#define SIRFSOC_DMA_CH_XLEN                     0x04
+#define SIRFSOC_DMA_CH_YLEN                     0x08
+#define SIRFSOC_DMA_CH_CTRL                     0x0C
+
+#define SIRFSOC_DMA_WIDTH_0                     0x100
+#define SIRFSOC_DMA_CH_VALID                    0x140
+#define SIRFSOC_DMA_CH_INT                      0x144
+#define SIRFSOC_DMA_INT_EN                      0x148
+#define SIRFSOC_DMA_CH_LOOP_CTRL                0x150
+
+#define SIRFSOC_DMA_MODE_CTRL_BIT               4
+#define SIRFSOC_DMA_DIR_CTRL_BIT                5
+
+struct sirfsoc_dma_desc {
+	struct dma_async_tx_descriptor	desc;
+	struct list_head		node;
+
+	/* SiRFprimaII 2D-DMA parameters */
+	int             xlen;           /* DMA xlen */
+	int             ylen;           /* DMA ylen */
+	int             width;          /* DMA width */
+};
+
+struct sirfsoc_dma_chan {
+	struct dma_chan			chan;
+	struct list_head		free;
+	struct list_head		prepared;
+	struct list_head		queued;
+	struct list_head		active;
+	struct list_head		completed;
+	dma_cookie_t			completed_cookie;
+
+	/* Lock for this structure */
+	spinlock_t			lock;
+
+	int             direction;
+	int             mode;
+	u32             addr;
+};
+
+struct sirfsoc_dma {
+	struct dma_device		dma;
+	struct tasklet_struct		tasklet;
+	struct sirfsoc_dma_chan		channels[SIRFSOC_DMA_CHANNELS];
+	void __iomem			*base;
+	int				irq;
+};
+
+#define DRV_NAME	"sirfsoc_dma"
+
+/* Convert struct dma_chan to struct sirfsoc_dma_chan */
+static inline struct sirfsoc_dma_chan *dma_chan_to_sirfsoc_dma_chan(struct dma_chan *c)
+{
+	return container_of(c, struct sirfsoc_dma_chan, chan);
+}
+
+/* Convert struct dma_chan to struct sirfsoc_dma */
+static inline struct sirfsoc_dma *dma_chan_to_sirfsoc_dma(struct dma_chan *c)
+{
+	struct sirfsoc_dma_chan *schan = dma_chan_to_sirfsoc_dma_chan(c);
+	return container_of(schan, struct sirfsoc_dma, channels[c->chan_id]);
+}
+
+/* Execute all queued DMA descriptors */
+static void sirfsoc_dma_execute(struct sirfsoc_dma_chan *schan)
+{
+	struct sirfsoc_dma *sdma = dma_chan_to_sirfsoc_dma(&schan->chan);
+	int cid = schan->chan.chan_id;
+	struct sirfsoc_dma_desc *sdesc = NULL;
+
+	sdesc = list_first_entry(&schan->queued, struct sirfsoc_dma_desc,
+		node);
+	/* Move the first queued descriptor to active list */
+	list_move_tail(&schan->queued, &schan->active);
+
+	/* Start the DMA transfer */
+	writel_relaxed(sdesc->width, sdma->base + SIRFSOC_DMA_WIDTH_0 + cid * 4);
+	writel_relaxed(cid | (schan->mode << SIRFSOC_DMA_MODE_CTRL_BIT) |
+		(schan->direction << SIRFSOC_DMA_DIR_CTRL_BIT),
+		sdma->base + cid * 0x10 + SIRFSOC_DMA_CH_CTRL);
+	writel_relaxed(sdesc->xlen, sdma->base + cid * 0x10 + SIRFSOC_DMA_CH_XLEN);
+	writel_relaxed(sdesc->ylen, sdma->base + cid * 0x10 + SIRFSOC_DMA_CH_YLEN);
+	writel_relaxed(readl_relaxed(sdma->base + SIRFSOC_DMA_INT_EN) | (1 << cid),
+		sdma->base + SIRFSOC_DMA_INT_EN);
+	writel_relaxed(schan->addr >> 2, sdma->base + cid * 0x10 + SIRFSOC_DMA_CH_ADDR);
+}
+
+/* Interrupt handler */
+static irqreturn_t sirfsoc_dma_irq(int irq, void *data)
+{
+	struct sirfsoc_dma *sdma = data;
+	struct sirfsoc_dma_chan *schan;
+	u32 is;
+	int ch;
+
+	is = readl_relaxed(sdma->base + SIRFSOC_DMA_CH_INT);
+	while ((ch = fls(is) - 1) >= 0) {
+		is &= ~(1 << ch);
+		writel_relaxed(1 << ch, sdma->base + SIRFSOC_DMA_CH_INT);
+		schan = &sdma->channels[ch];
+
+		spin_lock(&schan->lock);
+
+		/* Execute queued descriptors */
+		list_splice_tail_init(&schan->active, &schan->completed);
+		if (!list_empty(&schan->queued))
+			sirfsoc_dma_execute(schan);
+
+		spin_unlock(&schan->lock);
+	}
+
+	/* Schedule tasklet */
+	tasklet_schedule(&sdma->tasklet);
+
+	return IRQ_HANDLED;
+}
+
+/* process completed descriptors */
+static void sirfsoc_dma_process_completed(struct sirfsoc_dma *sdma)
+{
+	dma_cookie_t last_cookie = 0;
+	struct sirfsoc_dma_chan *schan;
+	struct sirfsoc_dma_desc *sdesc;
+	struct dma_async_tx_descriptor *desc;
+	unsigned long flags;
+	LIST_HEAD(list);
+	int i;
+
+	for (i = 0; i < sdma->dma.chancnt; i++) {
+		schan = &sdma->channels[i];
+
+		/* Get all completed descriptors */
+		spin_lock_irqsave(&schan->lock, flags);
+		if (!list_empty(&schan->completed))
+			list_splice_tail_init(&schan->completed, &list);
+		spin_unlock_irqrestore(&schan->lock, flags);
+
+		if (list_empty(&list))
+			continue;
+
+		/* Execute callbacks and run dependencies */
+		list_for_each_entry(sdesc, &list, node) {
+			desc = &sdesc->desc;
+
+			if (desc->callback)
+				desc->callback(desc->callback_param);
+
+			last_cookie = desc->cookie;
+			dma_run_dependencies(desc);
+		}
+
+		/* Free descriptors */
+		spin_lock_irqsave(&schan->lock, flags);
+		list_splice_tail_init(&list, &schan->free);
+		schan->completed_cookie = last_cookie;
+		spin_unlock_irqrestore(&schan->lock, flags);
+	}
+}
+
+/* DMA Tasklet */
+static void sirfsoc_dma_tasklet(unsigned long data)
+{
+	struct sirfsoc_dma *sdma = (void *)data;
+
+	sirfsoc_dma_process_completed(sdma);
+}
+
+/* Submit descriptor to hardware */
+static dma_cookie_t sirfsoc_dma_tx_submit(struct dma_async_tx_descriptor *txd)
+{
+	struct sirfsoc_dma_chan *schan = dma_chan_to_sirfsoc_dma_chan(txd->chan);
+	struct sirfsoc_dma_desc *sdesc;
+	unsigned long flags;
+	dma_cookie_t cookie;
+
+	sdesc = container_of(txd, struct sirfsoc_dma_desc, desc);
+
+	spin_lock_irqsave(&schan->lock, flags);
+
+	/* Move descriptor to queue */
+	list_move_tail(&sdesc->node, &schan->queued);
+
+	/* Update cookie */
+	cookie = schan->chan.cookie + 1;
+	if (cookie <= 0)
+		cookie = 1;
+
+	schan->chan.cookie = cookie;
+	sdesc->desc.cookie = cookie;
+
+	spin_unlock_irqrestore(&schan->lock, flags);
+
+	return cookie;
+}
+
+static int sirfsoc_dma_slave_config(struct sirfsoc_dma_chan *schan,
+	struct dma_slave_config *config)
+{
+	u32 addr, direction;
+	unsigned long flags;
+
+	switch (config->direction) {
+	case DMA_FROM_DEVICE:
+		direction = 0;
+		addr = config->dst_addr;
+		break;
+
+	case DMA_TO_DEVICE:
+		direction = 1;
+		addr = config->src_addr;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	if ((config->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES) ||
+		(config->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES))
+		return -EINVAL;
+
+	spin_lock_irqsave(&schan->lock, flags);
+	schan->addr = addr;
+	schan->direction = direction;
+	schan->mode = (config->src_maxburst == 4 ? 1 : 0);
+	spin_unlock_irqrestore(&schan->lock, flags);
+
+	return 0;
+}
+
+static int sirfsoc_dma_terminate_all(struct sirfsoc_dma_chan *schan)
+{
+	struct sirfsoc_dma *sdma = dma_chan_to_sirfsoc_dma(&schan->chan);
+	int cid = schan->chan.chan_id;
+	unsigned long flags;
+
+	writel_relaxed(readl_relaxed(sdma->base + SIRFSOC_DMA_INT_EN) & ~(1 << cid),
+		sdma->base + SIRFSOC_DMA_INT_EN);
+	writel_relaxed(1 << cid, sdma->base + SIRFSOC_DMA_CH_VALID);
+
+	spin_lock_irqsave(&schan->lock, flags);
+	list_splice_tail_init(&schan->active, &schan->free);
+	list_splice_tail_init(&schan->queued, &schan->free);
+	spin_unlock_irqrestore(&schan->lock, flags);
+
+	return 0;
+}
+
+static int sirfsoc_dma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
+	unsigned long arg)
+{
+	struct dma_slave_config *config;
+	struct sirfsoc_dma_chan *schan = dma_chan_to_sirfsoc_dma_chan(chan);
+
+	switch (cmd) {
+	case DMA_TERMINATE_ALL:
+		return sirfsoc_dma_terminate_all(schan);
+	case DMA_SLAVE_CONFIG:
+		config = (struct dma_slave_config *)arg;
+		return sirfsoc_dma_slave_config(schan, config);
+
+	default:
+		break;
+	}
+
+	return -ENOSYS;
+}
+
+/* Alloc channel resources */
+static int sirfsoc_dma_alloc_chan_resources(struct dma_chan *chan)
+{
+	struct sirfsoc_dma *sdma = dma_chan_to_sirfsoc_dma(chan);
+	struct sirfsoc_dma_chan *schan = dma_chan_to_sirfsoc_dma_chan(chan);
+	struct sirfsoc_dma_desc *sdesc;
+	unsigned long flags;
+	LIST_HEAD(descs);
+	int i;
+
+	/* Alloc descriptors for this channel */
+	for (i = 0; i < SIRFSOC_DMA_DESCRIPTORS; i++) {
+		sdesc = kzalloc(sizeof(struct sirfsoc_dma_desc), GFP_KERNEL);
+		if (!sdesc) {
+			dev_notice(sdma->dma.dev, "Memory allocation error. "
+				"Allocated only %u descriptors\n", i);
+			break;
+		}
+
+		dma_async_tx_descriptor_init(&sdesc->desc, chan);
+		sdesc->desc.flags = DMA_CTRL_ACK;
+		sdesc->desc.tx_submit = sirfsoc_dma_tx_submit;
+
+		list_add_tail(&sdesc->node, &descs);
+	}
+
+	/* Return error only if no descriptors were allocated */
+	if (i == 0)
+		return -ENOMEM;
+
+	spin_lock_irqsave(&schan->lock, flags);
+
+	list_splice_tail_init(&descs, &schan->free);
+	spin_unlock_irqrestore(&schan->lock, flags);
+
+	return 0;
+}
+
+/* Free channel resources */
+static void sirfsoc_dma_free_chan_resources(struct dma_chan *chan)
+{
+	struct sirfsoc_dma_chan *schan = dma_chan_to_sirfsoc_dma_chan(chan);
+	struct sirfsoc_dma_desc *sdesc, *tmp;
+	unsigned long flags;
+	LIST_HEAD(descs);
+
+	spin_lock_irqsave(&schan->lock, flags);
+
+	/* Channel must be idle */
+	BUG_ON(!list_empty(&schan->prepared));
+	BUG_ON(!list_empty(&schan->queued));
+	BUG_ON(!list_empty(&schan->active));
+	BUG_ON(!list_empty(&schan->completed));
+
+	/* Move data */
+	list_splice_tail_init(&schan->free, &descs);
+
+	spin_unlock_irqrestore(&schan->lock, flags);
+
+	/* Free descriptors */
+	list_for_each_entry_safe(sdesc, tmp, &descs, node)
+		kfree(sdesc);
+}
+
+/* Send pending descriptor to hardware */
+static void sirfsoc_dma_issue_pending(struct dma_chan *chan)
+{
+	struct sirfsoc_dma_chan *schan = dma_chan_to_sirfsoc_dma_chan(chan);
+	unsigned long flags;
+
+	spin_lock_irqsave(&schan->lock, flags);
+
+	if (list_empty(&schan->active) && !list_empty(&schan->queued))
+		sirfsoc_dma_execute(schan);
+
+	spin_unlock_irqrestore(&schan->lock, flags);
+}
+
+/* Check request completion status */
+static enum dma_status
+sirfsoc_dma_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
+	struct dma_tx_state *txstate)
+{
+	struct sirfsoc_dma_chan *schan = dma_chan_to_sirfsoc_dma_chan(chan);
+	unsigned long flags;
+	dma_cookie_t last_used;
+	dma_cookie_t last_complete;
+
+	spin_lock_irqsave(&schan->lock, flags);
+	last_used = schan->chan.cookie;
+	last_complete = schan->completed_cookie;
+	spin_unlock_irqrestore(&schan->lock, flags);
+
+	dma_set_tx_state(txstate, last_complete, last_used, 0);
+	return dma_async_is_complete(cookie, last_complete, last_used);
+}
+
+static struct dma_async_tx_descriptor *sirfsoc_dma_prep_slave_sg(
+	struct dma_chan *chan, struct scatterlist *sgl,
+	unsigned int sg_len, enum dma_data_direction direction,
+	unsigned long flags)
+{
+	return NULL;
+}
+
+static struct dma_async_tx_descriptor *sirfsoc_dma_prep_genxfer(
+	struct dma_chan *chan, struct xfer_template *xt)
+{
+	struct sirfsoc_dma *sdma = dma_chan_to_sirfsoc_dma(chan);
+	struct sirfsoc_dma_chan *schan = dma_chan_to_sirfsoc_dma_chan(chan);
+	struct sirfsoc_dma_desc *sdesc = NULL;
+	unsigned long iflags;
+	int ret;
+
+	/* Get free descriptor */
+	spin_lock_irqsave(&schan->lock, iflags);
+	if (!list_empty(&schan->free)) {
+		sdesc = list_first_entry(&schan->free, struct sirfsoc_dma_desc,
+			node);
+		list_del(&sdesc->node);
+	}
+	spin_unlock_irqrestore(&schan->lock, iflags);
+
+	if (!sdesc) {
+		/* try to free completed descriptors */
+		sirfsoc_dma_process_completed(sdma);
+		ret = 0;
+		goto no_desc;
+	}
+
+	/* Place descriptor in prepared list */
+	spin_lock_irqsave(&schan->lock, iflags);
+	if ((xt->frame_size == 1) && (xt->numf > 0)) {
+		sdesc->xlen = xt->sgl[0].size;
+		sdesc->width = xt->sgl[0].size + xt->sgl[0].icg;
+		sdesc->ylen = xt->numf - 1;
+		list_add_tail(&sdesc->node, &schan->prepared);
+	} else {
+		pr_err("sirfsoc DMA Invalid xfer\n");
+		ret = -EINVAL;
+		goto err_xfer;
+	}
+	spin_unlock_irqrestore(&schan->lock, iflags);
+
+	return &sdesc->desc;
+err_xfer:
+	spin_unlock_irqrestore(&schan->lock, iflags);
+no_desc:
+	return ERR_PTR(ret);
+}
+
+/*
+ * The DMA controller consists of 16 independent DMA channels.
+ * Each channel is allocated to a different function
+ */
+bool sirfsoc_dma_filter_id(struct dma_chan *chan, void *chan_id)
+{
+	unsigned int ch_nr = (unsigned int) chan_id;
+
+	if (ch_nr == chan->chan_id +
+		chan->device->dev_id * SIRFSOC_DMA_CHANNELS)
+		return true;
+
+	return false;
+}
+EXPORT_SYMBOL(sirfsoc_dma_filter_id);
+
+static int __devinit sirfsoc_dma_probe(struct platform_device *op)
+{
+	struct device_node *dn = op->dev.of_node;
+	struct device *dev = &op->dev;
+	struct dma_device *dma;
+	struct sirfsoc_dma *sdma;
+	struct sirfsoc_dma_chan *schan;
+	struct resource res;
+	ulong regs_start, regs_size;
+	u32 id;
+	int retval, i;
+
+	sdma = devm_kzalloc(dev, sizeof(struct sirfsoc_dma), GFP_KERNEL);
+	if (!sdma) {
+		dev_err(dev, "Memory exhausted!\n");
+		return -ENOMEM;
+	}
+
+	if (of_property_read_u32(dn, "cell-index", &id)) {
+		dev_err(dev, "Fail to get DMAC index\n");
+		return -ENODEV;
+	}
+
+	sdma->irq = irq_of_parse_and_map(dn, 0);
+	if (sdma->irq == NO_IRQ) {
+		dev_err(dev, "Error mapping IRQ!\n");
+		return -EINVAL;
+	}
+
+	retval = of_address_to_resource(dn, 0, &res);
+	if (retval) {
+		dev_err(dev, "Error parsing memory region!\n");
+		return retval;
+	}
+
+	regs_start = res.start;
+	regs_size = resource_size(&res);
+
+	if (!devm_request_mem_region(dev, regs_start, regs_size, DRV_NAME)) {
+		dev_err(dev, "Error requesting memory region!\n");
+		return -EBUSY;
+	}
+
+	sdma->base = devm_ioremap(dev, regs_start, regs_size);
+	if (!sdma->base) {
+		dev_err(dev, "Error mapping memory region!\n");
+		return -ENOMEM;
+	}
+
+	retval = devm_request_irq(dev, sdma->irq, &sirfsoc_dma_irq, 0, DRV_NAME,
+		sdma);
+	if (retval) {
+		dev_err(dev, "Error requesting IRQ!\n");
+		return -EINVAL;
+	}
+
+	dma = &sdma->dma;
+	dma->dev = dev;
+	dma->chancnt = SIRFSOC_DMA_CHANNELS;
+
+	dma->device_alloc_chan_resources = sirfsoc_dma_alloc_chan_resources;
+	dma->device_free_chan_resources = sirfsoc_dma_free_chan_resources;
+	dma->device_issue_pending = sirfsoc_dma_issue_pending;
+	dma->device_control = sirfsoc_dma_control;
+	dma->device_tx_status = sirfsoc_dma_tx_status;
+	dma->device_prep_slave_sg = sirfsoc_dma_prep_slave_sg;
+	dma->device_prep_dma_genxfer = sirfsoc_dma_prep_genxfer;
+
+	INIT_LIST_HEAD(&dma->channels);
+	dma_cap_set(DMA_SLAVE, dma->cap_mask);
+	dma_cap_set(DMA_PRIVATE, dma->cap_mask);
+
+	for (i = 0; i < dma->chancnt; i++) {
+		schan = &sdma->channels[i];
+
+		schan->chan.device = dma;
+		schan->chan.cookie = 1;
+		schan->completed_cookie = schan->chan.cookie;
+
+		INIT_LIST_HEAD(&schan->free);
+		INIT_LIST_HEAD(&schan->prepared);
+		INIT_LIST_HEAD(&schan->queued);
+		INIT_LIST_HEAD(&schan->active);
+		INIT_LIST_HEAD(&schan->completed);
+
+		spin_lock_init(&schan->lock);
+		list_add_tail(&schan->chan.device_node, &dma->channels);
+	}
+
+	tasklet_init(&sdma->tasklet, sirfsoc_dma_tasklet, (unsigned long)sdma);
+
+	/* Register DMA engine */
+	dev_set_drvdata(dev, sdma);
+	retval = dma_async_device_register(dma);
+	if (retval) {
+		devm_free_irq(dev, sdma->irq, sdma);
+		irq_dispose_mapping(sdma->irq);
+	}
+
+	return retval;
+}
+
+static int __devexit sirfsoc_dma_remove(struct platform_device *op)
+{
+	struct device *dev = &op->dev;
+	struct sirfsoc_dma *sdma = dev_get_drvdata(dev);
+
+	dma_async_device_unregister(&sdma->dma);
+	devm_free_irq(dev, sdma->irq, sdma);
+	irq_dispose_mapping(sdma->irq);
+
+	return 0;
+}
+
+static struct of_device_id sirfsoc_dma_match[] = {
+	{ .compatible = "sirf,prima2-dmac", },
+	{},
+};
+
+static struct platform_driver sirfsoc_dma_driver = {
+	.probe		= sirfsoc_dma_probe,
+	.remove		= __devexit_p(sirfsoc_dma_remove),
+	.driver = {
+		.name = DRV_NAME,
+		.owner = THIS_MODULE,
+		.of_match_table	= sirfsoc_dma_match,
+	},
+};
+
+static int __init sirfsoc_dma_init(void)
+{
+	return platform_driver_register(&sirfsoc_dma_driver);
+}
+module_init(sirfsoc_dma_init);
+
+static void __exit sirfsoc_dma_exit(void)
+{
+	platform_driver_unregister(&sirfsoc_dma_driver);
+}
+module_exit(sirfsoc_dma_exit);
+
+MODULE_AUTHOR("Rongjun Ying <rongjun.ying@csr.com>, "
+	"Barry Song <baohua.song@csr.com>");
+MODULE_DESCRIPTION("SIRFSOC DMA control driver");
+MODULE_LICENSE("GPL");
-- 
1.7.1



Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog

Re: [PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver

[Jassi Brar]

On Fri, Sep 16, 2011 at 3:26 PM, Barry Song <Baohua.Song@csr.com> wrote:

> +static int sirfsoc_dma_slave_config(struct sirfsoc_dma_chan *schan,
> +       struct dma_slave_config *config)
> +{
> +       u32 addr, direction;
> +       unsigned long flags;
> +
> +       switch (config->direction) {
> +       case DMA_FROM_DEVICE:
> +               direction = 0;
> +               addr = config->dst_addr;
> +               break;
> +
> +       case DMA_TO_DEVICE:
> +               direction = 1;
> +               addr = config->src_addr;
> +               break;
> +
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       if ((config->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES) ||
> +               (config->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES))
> +               return -EINVAL;
> +
> +       spin_lock_irqsave(&schan->lock, flags);
> +       schan->addr = addr;
> +       schan->direction = direction;
> +       schan->mode = (config->src_maxburst == 4 ? 1 : 0);
> +       spin_unlock_irqrestore(&schan->lock, flags);
> +
> +       return 0;
> +}
You don't need to pass as much info via dma_slave_config as you do here.

dmaxfer_template.src_inc  && !dmaxfer_template.dst_inc  => DMA_TO_DEVICE
!dmaxfer_template.src_inc  && dmaxfer_template.dst_inc  => DMA_FROM_DEVICE

Pass addresses using dmaxfer_template.src_start and dmaxfer_template.dst_start
instead of dma_slave_config.dst_addr and dma_slave_config.src_addr

So, currently you need dma_slave_config only to pass src_addr_width,
dst_addr_width and src_maxburst to the dmac driver.


> +
> +static struct dma_async_tx_descriptor *sirfsoc_dma_prep_slave_sg(
> +       struct dma_chan *chan, struct scatterlist *sgl,
> +       unsigned int sg_len, enum dma_data_direction direction,
> +       unsigned long flags)
> +{
> +       return NULL;
> +}
> +
In v3 I'll remove the BUG_ON check that requires every SLAVE channel
provides device_prep_slave_sg, so you should be able to simply discard
this stub.

Re: [PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver

[Barry Song]

2011/9/17 Jassi Brar <jassisinghbrar@gmail.com>:
> On Fri, Sep 16, 2011 at 3:26 PM, Barry Song <Baohua.Song@csr.com> wrote:
>
>> +static int sirfsoc_dma_slave_config(struct sirfsoc_dma_chan *schan,
>> +       struct dma_slave_config *config)
>> +{
>> +       u32 addr, direction;
>> +       unsigned long flags;
>> +
>> +       switch (config->direction) {
>> +       case DMA_FROM_DEVICE:
>> +               direction = 0;
>> +               addr = config->dst_addr;
>> +               break;
>> +
>> +       case DMA_TO_DEVICE:
>> +               direction = 1;
>> +               addr = config->src_addr;
>> +               break;
>> +
>> +       default:
>> +               return -EINVAL;
>> +       }
>> +
>> +       if ((config->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES) ||
>> +               (config->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES))
>> +               return -EINVAL;
>> +
>> +       spin_lock_irqsave(&schan->lock, flags);
>> +       schan->addr = addr;
>> +       schan->direction = direction;
>> +       schan->mode = (config->src_maxburst == 4 ? 1 : 0);
>> +       spin_unlock_irqrestore(&schan->lock, flags);
>> +
>> +       return 0;
>> +}
> You don't need to pass as much info via dma_slave_config as you do here.
>
> dmaxfer_template.src_inc  && !dmaxfer_template.dst_inc  => DMA_TO_DEVICE
> !dmaxfer_template.src_inc  && dmaxfer_template.dst_inc  => DMA_FROM_DEVICE

it seems direct DMA_TO_DEVICE/DMA_FROM_DEVICE is more feasible and
explict to me. "dmaxfer_template.src_inc  &&
!dmaxfer_template.dst_inc" is probably DMA_TO_DEVICE, but it is not
always true for all scenerios to all kinds of devices. anyway, for my
case, they are always true. so i can use them.

>
> Pass addresses using dmaxfer_template.src_start and dmaxfer_template.dst_start
> instead of dma_slave_config.dst_addr and dma_slave_config.src_addr
>
> So, currently you need dma_slave_config only to pass src_addr_width,
> dst_addr_width and src_maxburst to the dmac driver.

i agree that can decrease the calling of config. let the genxfer
decide the address of every transfer should be more flexible.

>
>
>> +
>> +static struct dma_async_tx_descriptor *sirfsoc_dma_prep_slave_sg(
>> +       struct dma_chan *chan, struct scatterlist *sgl,
>> +       unsigned int sg_len, enum dma_data_direction direction,
>> +       unsigned long flags)
>> +{
>> +       return NULL;
>> +}
>> +
> In v3 I'll remove the BUG_ON check that requires every SLAVE channel
> provides device_prep_slave_sg, so you should be able to simply discard
> this stub.

ok, that is going to make the api better.

-barry

Re: [PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver

[Russell King - ARM Linux]

On Fri, Sep 16, 2011 at 02:56:00AM -0700, Barry Song wrote:
> +static int sirfsoc_dma_slave_config(struct sirfsoc_dma_chan *schan,
> +	struct dma_slave_config *config)
> +{
> +	u32 addr, direction;
> +	unsigned long flags;
> +
> +	switch (config->direction) {

config->direction is deprecated, please don't use it.

> +	case DMA_FROM_DEVICE:
> +		direction = 0;
> +		addr = config->dst_addr;
> +		break;
> +
> +	case DMA_TO_DEVICE:
> +		direction = 1;
> +		addr = config->src_addr;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if ((config->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES) ||
> +		(config->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES))
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&schan->lock, flags);
> +	schan->addr = addr;
> +	schan->direction = direction;
> +	schan->mode = (config->src_maxburst == 4 ? 1 : 0);

Please store the source address, destination address, and mode separately
for each direction.  You should then select from that at prepare time.

Re: [PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver

[Barry Song]

2011/9/17 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Fri, Sep 16, 2011 at 02:56:00AM -0700, Barry Song wrote:
>> +static int sirfsoc_dma_slave_config(struct sirfsoc_dma_chan *schan,
>> +     struct dma_slave_config *config)
>> +{
>> +     u32 addr, direction;
>> +     unsigned long flags;
>> +
>> +     switch (config->direction) {
>
> config->direction is deprecated, please don't use it.

ok.

>
>> +     case DMA_FROM_DEVICE:
>> +             direction = 0;
>> +             addr = config->dst_addr;
>> +             break;
>> +
>> +     case DMA_TO_DEVICE:
>> +             direction = 1;
>> +             addr = config->src_addr;
>> +             break;
>> +
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +
>> +     if ((config->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES) ||
>> +             (config->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES))
>> +             return -EINVAL;
>> +
>> +     spin_lock_irqsave(&schan->lock, flags);
>> +     schan->addr = addr;
>> +     schan->direction = direction;
>> +     schan->mode = (config->src_maxburst == 4 ? 1 : 0);
>
> Please store the source address, destination address, and mode separately
> for each direction.  You should then select from that at prepare time.

the dmac has fixed route. every channel has fixed function. one
channel is for one device and one direction. for example, spi0 can
have two fixed channel: spi0-tx, spi0-rx.
then for every schan, device address is always fixed and we don't care
it at all. we only need to write the memory address to dmac address
register.

>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

-barry

Re: [PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver

[Jassi Brar]

On 17 September 2011 20:27, Barry Song <21cnbao@gmail.com> wrote:
>>> +static int sirfsoc_dma_slave_config(struct sirfsoc_dma_chan *schan,
>>> +       struct dma_slave_config *config)
>>> +{
>>> +       u32 addr, direction;
>>> +       unsigned long flags;
>>> +
>>> +       switch (config->direction) {
>>> +       case DMA_FROM_DEVICE:
>>> +               direction = 0;
>>> +               addr = config->dst_addr;
>>> +               break;
>>> +
>>> +       case DMA_TO_DEVICE:
>>> +               direction = 1;
>>> +               addr = config->src_addr;
>>> +               break;
>>> +
>>> +       default:
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       if ((config->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES) ||
>>> +               (config->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES))
>>> +               return -EINVAL;
>>> +
>>> +       spin_lock_irqsave(&schan->lock, flags);
>>> +       schan->addr = addr;
>>> +       schan->direction = direction;
>>> +       schan->mode = (config->src_maxburst == 4 ? 1 : 0);
>>> +       spin_unlock_irqrestore(&schan->lock, flags);
>>> +
>>> +       return 0;
>>> +}
>> You don't need to pass as much info via dma_slave_config as you do here.
>>
>> dmaxfer_template.src_inc  && !dmaxfer_template.dst_inc  => DMA_TO_DEVICE
>> !dmaxfer_template.src_inc  && dmaxfer_template.dst_inc  => DMA_FROM_DEVICE
>
> it seems direct DMA_TO_DEVICE/DMA_FROM_DEVICE is more feasible and
> explict to me.

> "dmaxfer_template.src_inc  && !dmaxfer_template.dst_inc" is probably
> DMA_TO_DEVICE, but it is not always true for all scenerios to all kinds of devices.
For which scenarios do you think it won't hold (remember that dmac already
knows if the channel is SLAVE or not) ?

Re: [PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver

[Vinod Koul]

On Fri, 2011-09-16 at 02:56 -0700, Barry Song wrote:
> From: Rongjun Ying <rongjun.ying@csr.com>
> 
> Cc: Jassi Brar <jaswinder.singh@linaro.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Rongjun Ying <rongjun.ying@csr.com>
> Signed-off-by: Barry Song <Baohua.Song@csr.com>
> ---
>  -v2:
>  use generic xfer API from jassi;
>  delete sirf self-defined slave config;
>  fix feedback from vinod;
>  fix filter function: we have two dmac, clients drivers think the chan id as 0~31;
>  rename regs to base;
>  delete redundant chan_id initialization in probe since dmaengine core will
>  re-write it, refer to my patch too:
>  [PATCH] dmaengine: delete redundant chan_id and chancnt initialization in dma drivers
>  http://www.spinics.net/lists/kernel/msg1237455.html
> 
>  this patch doesn't provide a common way for filter and doesn't use the jassi's v2 patch.
> +
> +static int sirfsoc_dma_slave_config(struct sirfsoc_dma_chan *schan,
> +	struct dma_slave_config *config)
> +{
> +	u32 addr, direction;
> +	unsigned long flags;
> +
> +	switch (config->direction) {
> +	case DMA_FROM_DEVICE:
> +		direction = 0;
> +		addr = config->dst_addr;
> +		break;
> +
> +	case DMA_TO_DEVICE:
> +		direction = 1;
> +		addr = config->src_addr;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if ((config->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES) ||
> +		(config->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES))
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&schan->lock, flags);
> +	schan->addr = addr;
> +	schan->direction = direction;
> +	schan->mode = (config->src_maxburst == 4 ? 1 : 0);
> +	spin_unlock_irqrestore(&schan->lock, flags);
Not sure why you support this, there seem to be no DMA_SLAVE support in
this version ate least
> +
> +	return 0;
> +}
> +
> +
> +
> +/* Alloc channel resources */
> +static int sirfsoc_dma_alloc_chan_resources(struct dma_chan *chan)
> +{
> +	struct sirfsoc_dma *sdma = dma_chan_to_sirfsoc_dma(chan);
> +	struct sirfsoc_dma_chan *schan = dma_chan_to_sirfsoc_dma_chan(chan);
> +	struct sirfsoc_dma_desc *sdesc;
> +	unsigned long flags;
> +	LIST_HEAD(descs);
> +	int i;
> +
> +	/* Alloc descriptors for this channel */
> +	for (i = 0; i < SIRFSOC_DMA_DESCRIPTORS; i++) {
> +		sdesc = kzalloc(sizeof(struct sirfsoc_dma_desc), GFP_KERNEL);
kernel convention is kzalloc(sizeof(*sdesc),....)

> +		if (!sdesc) {
> +			dev_notice(sdma->dma.dev, "Memory allocation error. "
> +				"Allocated only %u descriptors\n", i);
> +			break;
> +		}
> +
> +		dma_async_tx_descriptor_init(&sdesc->desc, chan);
> +		sdesc->desc.flags = DMA_CTRL_ACK;
> +		sdesc->desc.tx_submit = sirfsoc_dma_tx_submit;
> +
> +		list_add_tail(&sdesc->node, &descs);
> +	}
> +
> +	/* Return error only if no descriptors were allocated */
> +	if (i == 0)
> +		return -ENOMEM;
> +
> +	spin_lock_irqsave(&schan->lock, flags);
> +
> +	list_splice_tail_init(&descs, &schan->free);
> +	spin_unlock_irqrestore(&schan->lock, flags);
> +
> +	return 0;
it should be return i; You are supposed to return the number of desc
allocated.


> +
> +/* Check request completion status */
> +static enum dma_status
> +sirfsoc_dma_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
> +	struct dma_tx_state *txstate)
> +{
> +	struct sirfsoc_dma_chan *schan = dma_chan_to_sirfsoc_dma_chan(chan);
> +	unsigned long flags;
> +	dma_cookie_t last_used;
> +	dma_cookie_t last_complete;
> +
> +	spin_lock_irqsave(&schan->lock, flags);
> +	last_used = schan->chan.cookie;
> +	last_complete = schan->completed_cookie;
> +	spin_unlock_irqrestore(&schan->lock, flags);
> +
> +	dma_set_tx_state(txstate, last_complete, last_used, 0);
> +	return dma_async_is_complete(cookie, last_complete, last_used);
> +}
> +
> +static struct dma_async_tx_descriptor *sirfsoc_dma_prep_slave_sg(
> +	struct dma_chan *chan, struct scatterlist *sgl,
> +	unsigned int sg_len, enum dma_data_direction direction,
> +	unsigned long flags)
> +{
> +	return NULL;
> +}
Please remove this until you support it...


> +}
> +
> +/*
> + * The DMA controller consists of 16 independent DMA channels.
> + * Each channel is allocated to a different function
> + */
> +bool sirfsoc_dma_filter_id(struct dma_chan *chan, void *chan_id)
> +{
> +	unsigned int ch_nr = (unsigned int) chan_id;
> +
> +	if (ch_nr == chan->chan_id +
> +		chan->device->dev_id * SIRFSOC_DMA_CHANNELS)
> +		return true;
> +
> +	return false;
> +}
> +EXPORT_SYMBOL(sirfsoc_dma_filter_id);
> +
> +static int __devinit sirfsoc_dma_probe(struct platform_device *op)
> +{
> +	struct device_node *dn = op->dev.of_node;
> +	struct device *dev = &op->dev;
> +	struct dma_device *dma;
> +	struct sirfsoc_dma *sdma;
> +	struct sirfsoc_dma_chan *schan;
> +	struct resource res;
> +	ulong regs_start, regs_size;
> +	u32 id;
> +	int retval, i;
> +
> +	sdma = devm_kzalloc(dev, sizeof(struct sirfsoc_dma), GFP_KERNEL);
ditto...

> +	if (!sdma) {
> +		dev_err(dev, "Memory exhausted!\n");
> +		return -ENOMEM;
> +	}
> +
> +	if (of_property_read_u32(dn, "cell-index", &id)) {
> +		dev_err(dev, "Fail to get DMAC index\n");
> +		return -ENODEV;
kfree(sdma) ??

> +	}
> +
> +	sdma->irq = irq_of_parse_and_map(dn, 0);
> +	if (sdma->irq == NO_IRQ) {
> +		dev_err(dev, "Error mapping IRQ!\n");
> +		return -EINVAL;
> +	}
> +
> +	retval = of_address_to_resource(dn, 0, &res);
> +	if (retval) {
> +		dev_err(dev, "Error parsing memory region!\n");
> +		return retval;
> +	}
> +
> +	regs_start = res.start;
> +	regs_size = resource_size(&res);
> +
> +	if (!devm_request_mem_region(dev, regs_start, regs_size, DRV_NAME)) {
> +		dev_err(dev, "Error requesting memory region!\n");
> +		return -EBUSY;
> +	}
> +
> +	sdma->base = devm_ioremap(dev, regs_start, regs_size);
> +	if (!sdma->base) {
> +		dev_err(dev, "Error mapping memory region!\n");
> +		return -ENOMEM;
> +	}
> +
> +	retval = devm_request_irq(dev, sdma->irq, &sirfsoc_dma_irq, 0, DRV_NAME,
> +		sdma);
> +	if (retval) {
> +		dev_err(dev, "Error requesting IRQ!\n");
> +		return -EINVAL;
> +	}
> +
> +	dma = &sdma->dma;
> +	dma->dev = dev;
> +	dma->chancnt = SIRFSOC_DMA_CHANNELS;
> +
> +	dma->device_alloc_chan_resources = sirfsoc_dma_alloc_chan_resources;
> +	dma->device_free_chan_resources = sirfsoc_dma_free_chan_resources;
> +	dma->device_issue_pending = sirfsoc_dma_issue_pending;
> +	dma->device_control = sirfsoc_dma_control;
> +	dma->device_tx_status = sirfsoc_dma_tx_status;
> +	dma->device_prep_slave_sg = sirfsoc_dma_prep_slave_sg;
> +	dma->device_prep_dma_genxfer = sirfsoc_dma_prep_genxfer;
> +
> +	INIT_LIST_HEAD(&dma->channels);
> +	dma_cap_set(DMA_SLAVE, dma->cap_mask);
No you don't support DMA_SLAVE yet.


> +MODULE_AUTHOR("Rongjun Ying <rongjun.ying@csr.com>, "
> +	"Barry Song <baohua.song@csr.com>");
> +MODULE_DESCRIPTION("SIRFSOC DMA control driver");
> +MODULE_LICENSE("GPL");
Your header says GPLV2 or later, here its GPL only???


-- 
~Vinod

RE: [PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver

[Barry Song]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 8702 bytes --]

Hi Vinod,
Thanks!

> -----Original Message-----
> From: Vinod Koul [mailto:vinod.koul@intel.com]
> Sent: 2011年9月19日 17:00
> To: Barry Song
> Cc: Arnd Bergmann; Jassi Brar; Linus Walleij; linux-kernel@vger.kernel.org;
> DL-SHA-WorkGroupLinux; Rongjun Ying; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver
> 
> On Fri, 2011-09-16 at 02:56 -0700, Barry Song wrote:
> > From: Rongjun Ying <rongjun.ying@csr.com>
> >
> > Cc: Jassi Brar <jaswinder.singh@linaro.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Rongjun Ying <rongjun.ying@csr.com>
> > Signed-off-by: Barry Song <Baohua.Song@csr.com>
> > ---
> >  -v2:
> >  use generic xfer API from jassi;
> >  delete sirf self-defined slave config;
> >  fix feedback from vinod;
> >  fix filter function: we have two dmac, clients drivers think the chan id as
> 0~31;
> >  rename regs to base;
> >  delete redundant chan_id initialization in probe since dmaengine core will
> >  re-write it, refer to my patch too:
> >  [PATCH] dmaengine: delete redundant chan_id and chancnt initialization in
> dma drivers
> >  http://www.spinics.net/lists/kernel/msg1237455.html
> >
> >  this patch doesn't provide a common way for filter and doesn't use the
> jassi's v2 patch.
> > +
> > +static int sirfsoc_dma_slave_config(struct sirfsoc_dma_chan *schan,
> > +	struct dma_slave_config *config)
> > +{
> > +	u32 addr, direction;
> > +	unsigned long flags;
> > +
> > +	switch (config->direction) {
> > +	case DMA_FROM_DEVICE:
> > +		direction = 0;
> > +		addr = config->dst_addr;
> > +		break;
> > +
> > +	case DMA_TO_DEVICE:
> > +		direction = 1;
> > +		addr = config->src_addr;
> > +		break;
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	if ((config->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES) ||
> > +		(config->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES))
> > +		return -EINVAL;
> > +
> > +	spin_lock_irqsave(&schan->lock, flags);
> > +	schan->addr = addr;
> > +	schan->direction = direction;
> > +	schan->mode = (config->src_maxburst == 4 ? 1 : 0);
> > +	spin_unlock_irqrestore(&schan->lock, flags);
> Not sure why you support this, there seem to be no DMA_SLAVE support in
> this version ate least

Not. I support dma_slave. But I have no prep_slave_sg function since I can use the gen xfer to replace it.

> > +
> > +	return 0;
> > +}
> > +
> > +
> > +
> > +/* Alloc channel resources */
> > +static int sirfsoc_dma_alloc_chan_resources(struct dma_chan *chan)
> > +{
> > +	struct sirfsoc_dma *sdma = dma_chan_to_sirfsoc_dma(chan);
> > +	struct sirfsoc_dma_chan *schan =
> dma_chan_to_sirfsoc_dma_chan(chan);
> > +	struct sirfsoc_dma_desc *sdesc;
> > +	unsigned long flags;
> > +	LIST_HEAD(descs);
> > +	int i;
> > +
> > +	/* Alloc descriptors for this channel */
> > +	for (i = 0; i < SIRFSOC_DMA_DESCRIPTORS; i++) {
> > +		sdesc = kzalloc(sizeof(struct sirfsoc_dma_desc), GFP_KERNEL);
> kernel convention is kzalloc(sizeof(*sdesc),....)

Ok.
> 
> > +		if (!sdesc) {
> > +			dev_notice(sdma->dma.dev, "Memory allocation error. "
> > +				"Allocated only %u descriptors\n", i);
> > +			break;
> > +		}
> > +
> > +		dma_async_tx_descriptor_init(&sdesc->desc, chan);
> > +		sdesc->desc.flags = DMA_CTRL_ACK;
> > +		sdesc->desc.tx_submit = sirfsoc_dma_tx_submit;
> > +
> > +		list_add_tail(&sdesc->node, &descs);
> > +	}
> > +
> > +	/* Return error only if no descriptors were allocated */
> > +	if (i == 0)
> > +		return -ENOMEM;
> > +
> > +	spin_lock_irqsave(&schan->lock, flags);
> > +
> > +	list_splice_tail_init(&descs, &schan->free);
> > +	spin_unlock_irqrestore(&schan->lock, flags);
> > +
> > +	return 0;
> it should be return i; You are supposed to return the number of desc
> allocated.
> 
Ok.
> 
> > +
> > +/* Check request completion status */
> > +static enum dma_status
> > +sirfsoc_dma_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
> > +	struct dma_tx_state *txstate)
> > +{
> > +	struct sirfsoc_dma_chan *schan =
> dma_chan_to_sirfsoc_dma_chan(chan);
> > +	unsigned long flags;
> > +	dma_cookie_t last_used;
> > +	dma_cookie_t last_complete;
> > +
> > +	spin_lock_irqsave(&schan->lock, flags);
> > +	last_used = schan->chan.cookie;
> > +	last_complete = schan->completed_cookie;
> > +	spin_unlock_irqrestore(&schan->lock, flags);
> > +
> > +	dma_set_tx_state(txstate, last_complete, last_used, 0);
> > +	return dma_async_is_complete(cookie, last_complete, last_used);
> > +}
> > +
> > +static struct dma_async_tx_descriptor *sirfsoc_dma_prep_slave_sg(
> > +	struct dma_chan *chan, struct scatterlist *sgl,
> > +	unsigned int sg_len, enum dma_data_direction direction,
> > +	unsigned long flags)
> > +{
> > +	return NULL;
> > +}
> Please remove this until you support it...

Now I am supporting slave by gen xfer. I don't need this function, but a BUG_ON check will cause oops. Jassi would like to delete the BUG_ON check.

         BUG_ON(dma_has_cap(DMA_SLAVE, device->cap_mask) &&
                 !device->device_prep_slave_sg);
> 
> 
> > +}
> > +
> > +/*
> > + * The DMA controller consists of 16 independent DMA channels.
> > + * Each channel is allocated to a different function
> > + */
> > +bool sirfsoc_dma_filter_id(struct dma_chan *chan, void *chan_id)
> > +{
> > +	unsigned int ch_nr = (unsigned int) chan_id;
> > +
> > +	if (ch_nr == chan->chan_id +
> > +		chan->device->dev_id * SIRFSOC_DMA_CHANNELS)
> > +		return true;
> > +
> > +	return false;
> > +}
> > +EXPORT_SYMBOL(sirfsoc_dma_filter_id);
> > +
> > +static int __devinit sirfsoc_dma_probe(struct platform_device *op)
> > +{
> > +	struct device_node *dn = op->dev.of_node;
> > +	struct device *dev = &op->dev;
> > +	struct dma_device *dma;
> > +	struct sirfsoc_dma *sdma;
> > +	struct sirfsoc_dma_chan *schan;
> > +	struct resource res;
> > +	ulong regs_start, regs_size;
> > +	u32 id;
> > +	int retval, i;
> > +
> > +	sdma = devm_kzalloc(dev, sizeof(struct sirfsoc_dma), GFP_KERNEL);
> ditto...

ok. Just copied from mpc, but ignore this :-)

> 
> > +	if (!sdma) {
> > +		dev_err(dev, "Memory exhausted!\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	if (of_property_read_u32(dn, "cell-index", &id)) {
> > +		dev_err(dev, "Fail to get DMAC index\n");
> > +		return -ENODEV;
> kfree(sdma) ??
> 
> > +	}
> > +
> > +	sdma->irq = irq_of_parse_and_map(dn, 0);
> > +	if (sdma->irq == NO_IRQ) {
> > +		dev_err(dev, "Error mapping IRQ!\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	retval = of_address_to_resource(dn, 0, &res);
> > +	if (retval) {
> > +		dev_err(dev, "Error parsing memory region!\n");
> > +		return retval;
> > +	}
> > +
> > +	regs_start = res.start;
> > +	regs_size = resource_size(&res);
> > +
> > +	if (!devm_request_mem_region(dev, regs_start, regs_size, DRV_NAME)) {
> > +		dev_err(dev, "Error requesting memory region!\n");
> > +		return -EBUSY;
> > +	}
> > +
> > +	sdma->base = devm_ioremap(dev, regs_start, regs_size);
> > +	if (!sdma->base) {
> > +		dev_err(dev, "Error mapping memory region!\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	retval = devm_request_irq(dev, sdma->irq, &sirfsoc_dma_irq, 0,
> DRV_NAME,
> > +		sdma);
> > +	if (retval) {
> > +		dev_err(dev, "Error requesting IRQ!\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	dma = &sdma->dma;
> > +	dma->dev = dev;
> > +	dma->chancnt = SIRFSOC_DMA_CHANNELS;
> > +
> > +	dma->device_alloc_chan_resources = sirfsoc_dma_alloc_chan_resources;
> > +	dma->device_free_chan_resources = sirfsoc_dma_free_chan_resources;
> > +	dma->device_issue_pending = sirfsoc_dma_issue_pending;
> > +	dma->device_control = sirfsoc_dma_control;
> > +	dma->device_tx_status = sirfsoc_dma_tx_status;
> > +	dma->device_prep_slave_sg = sirfsoc_dma_prep_slave_sg;
> > +	dma->device_prep_dma_genxfer = sirfsoc_dma_prep_genxfer;
> > +
> > +	INIT_LIST_HEAD(&dma->channels);
> > +	dma_cap_set(DMA_SLAVE, dma->cap_mask);
> No you don't support DMA_SLAVE yet.
> 
> 
> > +MODULE_AUTHOR("Rongjun Ying <rongjun.ying@csr.com>, "
> > +	"Barry Song <baohua.song@csr.com>");
> > +MODULE_DESCRIPTION("SIRFSOC DMA control driver");
> > +MODULE_LICENSE("GPL");
> Your header says GPLV2 or later, here its GPL only???

Gpl v2 actually

> 
> 
> --
> ~Vinod

-barry



Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver

[Vinod Koul]

On Mon, 2011-09-19 at 09:23 +0000, Barry Song wrote:
> Hi Vinod,
> Thanks!
> 
> > -----Original Message-----
> > From: Vinod Koul [mailto:vinod.koul@intel.com]
> > Sent: 2011年9月19日 17:00
> > To: Barry Song
> > Cc: Arnd Bergmann; Jassi Brar; Linus Walleij; linux-kernel@vger.kernel.org;
> > DL-SHA-WorkGroupLinux; Rongjun Ying; linux-arm-kernel@lists.infradead.org
> > Subject: Re: [PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver
> > 
> > On Fri, 2011-09-16 at 02:56 -0700, Barry Song wrote:
> > > From: Rongjun Ying <rongjun.ying@csr.com>
> > >
> > > Cc: Jassi Brar <jaswinder.singh@linaro.org>
> > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > Signed-off-by: Rongjun Ying <rongjun.ying@csr.com>
> > > Signed-off-by: Barry Song <Baohua.Song@csr.com>
> > > ---
> > >  -v2:
> > >  use generic xfer API from jassi;
> > >  delete sirf self-defined slave config;
> > >  fix feedback from vinod;
> > >  fix filter function: we have two dmac, clients drivers think the chan id as
> > 0~31;
> > >  rename regs to base;
> > >  delete redundant chan_id initialization in probe since dmaengine core will
> > >  re-write it, refer to my patch too:
> > >  [PATCH] dmaengine: delete redundant chan_id and chancnt initialization in
> > dma drivers
> > >  http://www.spinics.net/lists/kernel/msg1237455.html
> > >
> > >  this patch doesn't provide a common way for filter and doesn't use the
> > jassi's v2 patch.
> > > +
> > > +static int sirfsoc_dma_slave_config(struct sirfsoc_dma_chan *schan,
> > > +	struct dma_slave_config *config)
> > > +{
> > > +	u32 addr, direction;
> > > +	unsigned long flags;
> > > +
> > > +	switch (config->direction) {
> > > +	case DMA_FROM_DEVICE:
> > > +		direction = 0;
> > > +		addr = config->dst_addr;
> > > +		break;
> > > +
> > > +	case DMA_TO_DEVICE:
> > > +		direction = 1;
> > > +		addr = config->src_addr;
> > > +		break;
> > > +
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if ((config->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES) ||
> > > +		(config->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES))
> > > +		return -EINVAL;
> > > +
> > > +	spin_lock_irqsave(&schan->lock, flags);
> > > +	schan->addr = addr;
> > > +	schan->direction = direction;
> > > +	schan->mode = (config->src_maxburst == 4 ? 1 : 0);
> > > +	spin_unlock_irqrestore(&schan->lock, flags);
> > Not sure why you support this, there seem to be no DMA_SLAVE support in
> > this version ate least
> 
> Not. I support dma_slave. But I have no prep_slave_sg function since I can use the gen xfer to replace it.
Yes thats okay...

Then I have questions on genxfer function...
where are you copying either src or dstn_start address, you seem to
completely ignore them?

Do you support only slave transfers or M2M as well for this driver?
If only slave you might want to check if dma_config_slave is set for
this channel or not.

-- 
~Vinod

RE: [PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver

[Barry Song]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4345 bytes --]



> -----Original Message-----
> From: Vinod Koul [mailto:vinod.koul@intel.com]
> Sent: 2011年9月19日 17:41
> To: Barry Song
> Cc: 21cnbao@gmail.com; Arnd Bergmann; Jassi Brar; Linus Walleij;
> linux-kernel@vger.kernel.org; DL-SHA-WorkGroupLinux; Rongjun Ying;
> linux-arm-kernel@lists.infradead.org
> Subject: RE: [PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver
> 
> On Mon, 2011-09-19 at 09:23 +0000, Barry Song wrote:
> > Hi Vinod,
> > Thanks!
> >
> > > -----Original Message-----
> > > From: Vinod Koul [mailto:vinod.koul@intel.com]
> > > Sent: 2011年9月19日 17:00
> > > To: Barry Song
> > > Cc: Arnd Bergmann; Jassi Brar; Linus Walleij; linux-kernel@vger.kernel.org;
> > > DL-SHA-WorkGroupLinux; Rongjun Ying;
> linux-arm-kernel@lists.infradead.org
> > > Subject: Re: [PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver
> > >
> > > On Fri, 2011-09-16 at 02:56 -0700, Barry Song wrote:
> > > > From: Rongjun Ying <rongjun.ying@csr.com>
> > > >
> > > > Cc: Jassi Brar <jaswinder.singh@linaro.org>
> > > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > > Signed-off-by: Rongjun Ying <rongjun.ying@csr.com>
> > > > Signed-off-by: Barry Song <Baohua.Song@csr.com>
> > > > ---
> > > >  -v2:
> > > >  use generic xfer API from jassi;
> > > >  delete sirf self-defined slave config;
> > > >  fix feedback from vinod;
> > > >  fix filter function: we have two dmac, clients drivers think the chan id as
> > > 0~31;
> > > >  rename regs to base;
> > > >  delete redundant chan_id initialization in probe since dmaengine core
> will
> > > >  re-write it, refer to my patch too:
> > > >  [PATCH] dmaengine: delete redundant chan_id and chancnt initialization
> in
> > > dma drivers
> > > >  http://www.spinics.net/lists/kernel/msg1237455.html
> > > >
> > > >  this patch doesn't provide a common way for filter and doesn't use the
> > > jassi's v2 patch.
> > > > +
> > > > +static int sirfsoc_dma_slave_config(struct sirfsoc_dma_chan *schan,
> > > > +	struct dma_slave_config *config)
> > > > +{
> > > > +	u32 addr, direction;
> > > > +	unsigned long flags;
> > > > +
> > > > +	switch (config->direction) {
> > > > +	case DMA_FROM_DEVICE:
> > > > +		direction = 0;
> > > > +		addr = config->dst_addr;
> > > > +		break;
> > > > +
> > > > +	case DMA_TO_DEVICE:
> > > > +		direction = 1;
> > > > +		addr = config->src_addr;
> > > > +		break;
> > > > +
> > > > +	default:
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	if ((config->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES) ||
> > > > +		(config->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES))
> > > > +		return -EINVAL;
> > > > +
> > > > +	spin_lock_irqsave(&schan->lock, flags);
> > > > +	schan->addr = addr;
> > > > +	schan->direction = direction;
> > > > +	schan->mode = (config->src_maxburst == 4 ? 1 : 0);
> > > > +	spin_unlock_irqrestore(&schan->lock, flags);
> > > Not sure why you support this, there seem to be no DMA_SLAVE support in
> > > this version ate least
> >
> > Not. I support dma_slave. But I have no prep_slave_sg function since I can
> use the gen xfer to replace it.
> Yes thats okay...
> 
> Then I have questions on genxfer function...
> where are you copying either src or dstn_start address, you seem to
> completely ignore them?

Since I only support memory->device or device ->memory, and channel number is fixed to every device. Then I actually don't care device address at all. Either src or dst is fixed to the device's address.

> 
> Do you support only slave transfers or M2M as well for this driver?
> If only slave you might want to check if dma_config_slave is set for
> this channel or not.

I support only slave transfer. Actually I have dma_config_slave. do you mean I need to check whether slave config cmd is really called before executing dma?

> 
> --
> ~Vinod

-barry



Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver

[Vinod Koul]

On Mon, 2011-09-19 at 09:56 +0000, Barry Song wrote:
> > > > Not sure why you support this, there seem to be no DMA_SLAVE
> support in
> > > > this version ate least
> > >
> > > Not. I support dma_slave. But I have no prep_slave_sg function
> since I can
> > use the gen xfer to replace it.
> > Yes thats okay...
> > 
> > Then I have questions on genxfer function...
> > where are you copying either src or dstn_start address, you seem to
> > completely ignore them?
> 
> Since I only support memory->device or device ->memory, and channel
> number is fixed to every device. Then I actually don't care device
> address at all. Either src or dst is fixed to the device's address.
peripheral address can be fixed, not the memory, where do you copy the
memory address?
> 
> > 
> > Do you support only slave transfers or M2M as well for this driver?
> > If only slave you might want to check if dma_config_slave is set for
> > this channel or not.
> 
> I support only slave transfer. Actually I have dma_config_slave. do
> you mean I need to check whether slave config cmd is really called
> before executing dma?
Yes


-- 
~Vinod

Re: [PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver

[Barry Song]

2011/9/19 Vinod Koul <vinod.koul@intel.com>:
> On Mon, 2011-09-19 at 09:56 +0000, Barry Song wrote:
>> > > > Not sure why you support this, there seem to be no DMA_SLAVE
>> support in
>> > > > this version ate least
>> > >
>> > > Not. I support dma_slave. But I have no prep_slave_sg function
>> since I can
>> > use the gen xfer to replace it.
>> > Yes thats okay...
>> >
>> > Then I have questions on genxfer function...
>> > where are you copying either src or dstn_start address, you seem to
>> > completely ignore them?
>>
>> Since I only support memory->device or device ->memory, and channel
>> number is fixed to every device. Then I actually don't care device
>> address at all. Either src or dst is fixed to the device's address.
> peripheral address can be fixed, not the memory, where do you copy the
> memory address?

+static int sirfsoc_dma_slave_config(struct sirfsoc_dma_chan *schan,
+       struct dma_slave_config *config)
+{
+       u32 addr, direction;
+       unsigned long flags;
+
+       switch (config->direction) {
+       case DMA_FROM_DEVICE:
+               direction = 0;
+               addr = config->dst_addr;
+               break;
+
+       case DMA_TO_DEVICE:
+               direction = 1;
+               addr = config->src_addr;
+               break;
+
+       default:
+               return -EINVAL;
+       }
+  ...


+static void sirfsoc_dma_execute(struct sirfsoc_dma_chan *schan)
+{
+       ...
+       writel_relaxed(schan->addr >> 2, sdma->base + cid * 0x10 +
SIRFSOC_DMA_CH_ADDR);
+}
+

>>
>> >
>> > Do you support only slave transfers or M2M as well for this driver?
>> > If only slave you might want to check if dma_config_slave is set for
>> > this channel or not.
>>
>> I support only slave transfer. Actually I have dma_config_slave. do
>> you mean I need to check whether slave config cmd is really called
>> before executing dma?
> Yes
>
>
> --
> ~Vinod
>
>
-Barry

Re: [PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver

[Vinod Koul]

On Mon, 2011-09-19 at 19:25 +0800, Barry Song wrote:
> 2011/9/19 Vinod Koul <vinod.koul@intel.com>:
> > On Mon, 2011-09-19 at 09:56 +0000, Barry Song wrote:
> >> > > > Not sure why you support this, there seem to be no DMA_SLAVE
> >> support in
> >> > > > this version ate least
> >> > >
> >> > > Not. I support dma_slave. But I have no prep_slave_sg function
> >> since I can
> >> > use the gen xfer to replace it.
> >> > Yes thats okay...
> >> >
> >> > Then I have questions on genxfer function...
> >> > where are you copying either src or dstn_start address, you seem to
> >> > completely ignore them?
> >>
> >> Since I only support memory->device or device ->memory, and channel
> >> number is fixed to every device. Then I actually don't care device
> >> address at all. Either src or dst is fixed to the device's address.
> > peripheral address can be fixed, not the memory, where do you copy the
> > memory address?
> 
> +static int sirfsoc_dma_slave_config(struct sirfsoc_dma_chan *schan,
> +       struct dma_slave_config *config)
> +{
> +       u32 addr, direction;
> +       unsigned long flags;
> +
> +       switch (config->direction) {
> +       case DMA_FROM_DEVICE:
> +               direction = 0;
> +               addr = config->dst_addr;
> +               break;
> +
> +       case DMA_TO_DEVICE:
> +               direction = 1;
> +               addr = config->src_addr;
> +               break;
> +
> +       default:
> +               return -EINVAL;
> +       }
> +  ...
This is wrong. You are treating addresses passed in dma_slave_config as
memory address. These should be treating this as peripheral address
(destination in DMA_TO_DEVICE....) and take the memory address from your
prepare function.

-- 
~Vinod

Re: [PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver

[Jassi Brar]

On 19 September 2011 16:55, Barry Song <21cnbao@gmail.com> wrote:
> 2011/9/19 Vinod Koul <vinod.koul@intel.com>:
>> On Mon, 2011-09-19 at 09:56 +0000, Barry Song wrote:
>>> > > > Not sure why you support this, there seem to be no DMA_SLAVE
>>> support in
>>> > > > this version ate least
>>> > >
>>> > > Not. I support dma_slave. But I have no prep_slave_sg function
>>> since I can
>>> > use the gen xfer to replace it.
>>> > Yes thats okay...
>>> >
>>> > Then I have questions on genxfer function...
>>> > where are you copying either src or dstn_start address, you seem to
>>> > completely ignore them?
>>>
>>> Since I only support memory->device or device ->memory, and channel
>>> number is fixed to every device. Then I actually don't care device
>>> address at all. Either src or dst is fixed to the device's address.
>> peripheral address can be fixed, not the memory, where do you copy the
>> memory address?
>
> +static int sirfsoc_dma_slave_config(struct sirfsoc_dma_chan *schan,
> +       struct dma_slave_config *config)
> +{
> +       u32 addr, direction;
> +       unsigned long flags;
> +
> +       switch (config->direction) {
> +       case DMA_FROM_DEVICE:
> +               direction = 0;
> +               addr = config->dst_addr;
> +               break;
> +
> +       case DMA_TO_DEVICE:
> +               direction = 1;
> +               addr = config->src_addr;
> +               break;
> +
> +       default:
> +               return -EINVAL;
> +       }
> +  ...
I repeat
{
Pass addresses using dmaxfer_template.src_start and dmaxfer_template.dst_start
instead of dma_slave_config.dst_addr and dma_slave_config.src_addr
}

Re: [PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver

[Barry Song]

2011/9/20 Jassi Brar <jaswinder.singh@linaro.org>:
> On 19 September 2011 16:55, Barry Song <21cnbao@gmail.com> wrote:
>> 2011/9/19 Vinod Koul <vinod.koul@intel.com>:
>>> On Mon, 2011-09-19 at 09:56 +0000, Barry Song wrote:
>>>> > > > Not sure why you support this, there seem to be no DMA_SLAVE
>>>> support in
>>>> > > > this version ate least
>>>> > >
>>>> > > Not. I support dma_slave. But I have no prep_slave_sg function
>>>> since I can
>>>> > use the gen xfer to replace it.
>>>> > Yes thats okay...
>>>> >
>>>> > Then I have questions on genxfer function...
>>>> > where are you copying either src or dstn_start address, you seem to
>>>> > completely ignore them?
>>>>
>>>> Since I only support memory->device or device ->memory, and channel
>>>> number is fixed to every device. Then I actually don't care device
>>>> address at all. Either src or dst is fixed to the device's address.
>>> peripheral address can be fixed, not the memory, where do you copy the
>>> memory address?
>>
>> +static int sirfsoc_dma_slave_config(struct sirfsoc_dma_chan *schan,
>> +       struct dma_slave_config *config)
>> +{
>> +       u32 addr, direction;
>> +       unsigned long flags;
>> +
>> +       switch (config->direction) {
>> +       case DMA_FROM_DEVICE:
>> +               direction = 0;
>> +               addr = config->dst_addr;
>> +               break;
>> +
>> +       case DMA_TO_DEVICE:
>> +               direction = 1;
>> +               addr = config->src_addr;
>> +               break;
>> +
>> +       default:
>> +               return -EINVAL;
>> +       }
>> +  ...
> I repeat
> {
> Pass addresses using dmaxfer_template.src_start and dmaxfer_template.dst_start
> instead of dma_slave_config.dst_addr and dma_slave_config.src_addr
> }

agree.

-barry

Re: [PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver

[Arnd Bergmann]

Hi Barry,

I just looked at the driver again and stumbled over a potential race:

On Friday 16 September 2011, Barry Song wrote:
> +
> +/* Execute all queued DMA descriptors */
> +static void sirfsoc_dma_execute(struct sirfsoc_dma_chan *schan)
> +{
> +       struct sirfsoc_dma *sdma = dma_chan_to_sirfsoc_dma(&schan->chan);
> +       int cid = schan->chan.chan_id;
> +       struct sirfsoc_dma_desc *sdesc = NULL;
> +
> +       sdesc = list_first_entry(&schan->queued, struct sirfsoc_dma_desc,
> +               node);
> +       /* Move the first queued descriptor to active list */
> +       list_move_tail(&schan->queued, &schan->active);
> +
> +       /* Start the DMA transfer */
> +       writel_relaxed(sdesc->width, sdma->base + SIRFSOC_DMA_WIDTH_0 + cid * 4);
> +       writel_relaxed(cid | (schan->mode << SIRFSOC_DMA_MODE_CTRL_BIT) |
> +               (schan->direction << SIRFSOC_DMA_DIR_CTRL_BIT),
> +               sdma->base + cid * 0x10 + SIRFSOC_DMA_CH_CTRL);
> +       writel_relaxed(sdesc->xlen, sdma->base + cid * 0x10 + SIRFSOC_DMA_CH_XLEN);
> +       writel_relaxed(sdesc->ylen, sdma->base + cid * 0x10 + SIRFSOC_DMA_CH_YLEN);
> +       writel_relaxed(readl_relaxed(sdma->base + SIRFSOC_DMA_INT_EN) | (1 << cid),
> +               sdma->base + SIRFSOC_DMA_INT_EN);
> +       writel_relaxed(schan->addr >> 2, sdma->base + cid * 0x10 + SIRFSOC_DMA_CH_ADDR);
> +}

I think you need to add a memory write barrier somewhere in here, because 
writel_relaxed() does not flush out the CPUs write buffers, unlike writel().

Theoretically, you might be starting a DMA that reads from coherent memory
but the data is still stuck in the CPU. I assume that the last writel_relaxed()
is the access that actually starts the DMA, so it should be airtight once you
replace that with writel().

> +/* Interrupt handler */
> +static irqreturn_t sirfsoc_dma_irq(int irq, void *data)
> +{
> +       struct sirfsoc_dma *sdma = data;
> +       struct sirfsoc_dma_chan *schan;
> +       u32 is;
> +       int ch;
> +
> +       is = readl_relaxed(sdma->base + SIRFSOC_DMA_CH_INT);
> +       while ((ch = fls(is) - 1) >= 0) {
> +               is &= ~(1 << ch);
> +               writel_relaxed(1 << ch, sdma->base + SIRFSOC_DMA_CH_INT);
> +               schan = &sdma->channels[ch];
> +
> +               spin_lock(&schan->lock);
> +
> +               /* Execute queued descriptors */
> +               list_splice_tail_init(&schan->active, &schan->completed);
> +               if (!list_empty(&schan->queued))
> +                       sirfsoc_dma_execute(schan);
> +
> +               spin_unlock(&schan->lock);
> +       }

Similarly, readl_relaxed() does might not force in inbound DMA to be
completed, causing you to call the tasklet before the data is visible
to the CPU. While your hardware might have better guarantees, the
API you are using does not. It should be find when you replace the
first read_relaxed with readl() here.

	Arnd

Re: [PATCH v2] dmaengine: add CSR SiRFprimaII DMAC driver

[Barry Song]

Hi Arnd,

Thanks for review and good comments.

2011/9/21 Arnd Bergmann <arnd@arndb.de>:
> Hi Barry,
>
> I just looked at the driver again and stumbled over a potential race:
>
> On Friday 16 September 2011, Barry Song wrote:
>> +
>> +/* Execute all queued DMA descriptors */
>> +static void sirfsoc_dma_execute(struct sirfsoc_dma_chan *schan)
>> +{
>> +       struct sirfsoc_dma *sdma = dma_chan_to_sirfsoc_dma(&schan->chan);
>> +       int cid = schan->chan.chan_id;
>> +       struct sirfsoc_dma_desc *sdesc = NULL;
>> +
>> +       sdesc = list_first_entry(&schan->queued, struct sirfsoc_dma_desc,
>> +               node);
>> +       /* Move the first queued descriptor to active list */
>> +       list_move_tail(&schan->queued, &schan->active);
>> +
>> +       /* Start the DMA transfer */
>> +       writel_relaxed(sdesc->width, sdma->base + SIRFSOC_DMA_WIDTH_0 + cid * 4);
>> +       writel_relaxed(cid | (schan->mode << SIRFSOC_DMA_MODE_CTRL_BIT) |
>> +               (schan->direction << SIRFSOC_DMA_DIR_CTRL_BIT),
>> +               sdma->base + cid * 0x10 + SIRFSOC_DMA_CH_CTRL);
>> +       writel_relaxed(sdesc->xlen, sdma->base + cid * 0x10 + SIRFSOC_DMA_CH_XLEN);
>> +       writel_relaxed(sdesc->ylen, sdma->base + cid * 0x10 + SIRFSOC_DMA_CH_YLEN);
>> +       writel_relaxed(readl_relaxed(sdma->base + SIRFSOC_DMA_INT_EN) | (1 << cid),
>> +               sdma->base + SIRFSOC_DMA_INT_EN);
>> +       writel_relaxed(schan->addr >> 2, sdma->base + cid * 0x10 + SIRFSOC_DMA_CH_ADDR);
>> +}
>
> I think you need to add a memory write barrier somewhere in here, because
> writel_relaxed() does not flush out the CPUs write buffers, unlike writel().
>
> Theoretically, you might be starting a DMA that reads from coherent memory
> but the data is still stuck in the CPU. I assume that the last writel_relaxed()
> is the access that actually starts the DMA, so it should be airtight once you
> replace that with writel().

yes. ARM_DMA_MEM_BUFFERABLE is forced on for CPU_V7 like primaII. we
used __raw_writel or writel_relaxed
before and haven't gotten any bug reported until now. anyway, actually
i need  the IO barrier.

>
>> +/* Interrupt handler */
>> +static irqreturn_t sirfsoc_dma_irq(int irq, void *data)
>> +{
>> +       struct sirfsoc_dma *sdma = data;
>> +       struct sirfsoc_dma_chan *schan;
>> +       u32 is;
>> +       int ch;
>> +
>> +       is = readl_relaxed(sdma->base + SIRFSOC_DMA_CH_INT);
>> +       while ((ch = fls(is) - 1) >= 0) {
>> +               is &= ~(1 << ch);
>> +               writel_relaxed(1 << ch, sdma->base + SIRFSOC_DMA_CH_INT);
>> +               schan = &sdma->channels[ch];
>> +
>> +               spin_lock(&schan->lock);
>> +
>> +               /* Execute queued descriptors */
>> +               list_splice_tail_init(&schan->active, &schan->completed);
>> +               if (!list_empty(&schan->queued))
>> +                       sirfsoc_dma_execute(schan);
>> +
>> +               spin_unlock(&schan->lock);
>> +       }
>
> Similarly, readl_relaxed() does might not force in inbound DMA to be
> completed, causing you to call the tasklet before the data is visible
> to the CPU. While your hardware might have better guarantees, the
> API you are using does not. It should be find when you replace the
> first read_relaxed with readl() here.
>
>        Arnd
>
-barry