[PATCH v2 0/2] dmaengine: add k3dma

[PATCH v2 0/2] dmaengine: add k3dma

[Zhangfei Gao]

change since v1:
Follow Arnd suggestion:
using dma_get_slave_channel interface
Correct dma-channels and dma-requests for phy and virt channel

Follow Vinod suggestion:
add name space
use SIMPLE_DEV_PM_OPS

Besides:
add pause/resume
Using kzalloc instead of dma pool

Zhangfei Gao (2):
  dmaengine: add interface of dma_get_slave_channel
  dmaengine: Add hisilicon k3 DMA engine driver

 Documentation/devicetree/bindings/dma/k3dma.txt |   46 ++
 drivers/dma/Kconfig                             |    9 +
 drivers/dma/Makefile                            |    1 +
 drivers/dma/dmaengine.c                         |   26 +
 drivers/dma/k3dma.c                             |  853 +++++++++++++++++++++++
 include/linux/dmaengine.h                       |    1 +
 6 files changed, 936 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/k3dma.txt
 create mode 100644 drivers/dma/k3dma.c

-- 
1.7.9.5

[PATCH v2 1/2] dmaengine: add interface of dma_get_slave_channel

[Zhangfei Gao]

Suggested by Arnd, add dma_get_slave_channel interface
Dma host driver could get specific channel specificied by request line, rather than filter.

host example:
static struct dma_chan *xx_of_dma_simple_xlate(struct of_phandle_args *dma_spec,
		struct of_dma *ofdma)
{
	struct xx_dma_dev *d = ofdma->of_dma_data;
	unsigned int request = dma_spec->args[0];

	if (request > d->dma_requests)
		return NULL;

	return dma_get_slave_channel(&(d->chans[request].vc.chan));
}

probe:
of_dma_controller_register((&op->dev)->of_node, xx_of_dma_simple_xlate, d);

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 drivers/dma/dmaengine.c   |   26 ++++++++++++++++++++++++++
 include/linux/dmaengine.h |    1 +
 2 files changed, 27 insertions(+)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 93f7992..78dbbe0 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -504,6 +504,32 @@ static struct dma_chan *private_candidate(const dma_cap_mask_t *mask,
 }
 
 /**
+ * dma_request_channel - try to get specific channel exclusively
+ * @chan: target channel
+ */
+struct dma_chan *dma_get_slave_channel(struct dma_chan *chan)
+{
+	int err = -EBUSY;
+
+	/* lock against __dma_request_channel */
+	mutex_lock(&dma_list_mutex);
+
+	if (chan->client_count == 0)
+		err = dma_chan_get(chan);
+	else
+		chan = NULL;
+
+	mutex_unlock(&dma_list_mutex);
+
+	if (err)
+		pr_debug("%s: failed to get %s: (%d)\n",
+			__func__, dma_chan_name(chan), err);
+
+	return chan;
+}
+EXPORT_SYMBOL_GPL(dma_get_slave_channel);
+
+/**
  * dma_request_channel - try to allocate an exclusive channel
  * @mask: capabilities that the channel must satisfy
  * @fn: optional callback to disposition available channels
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 96d3e4a..4e1c843 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -1000,6 +1000,7 @@ int dma_async_device_register(struct dma_device *device);
 void dma_async_device_unregister(struct dma_device *device);
 void dma_run_dependencies(struct dma_async_tx_descriptor *tx);
 struct dma_chan *dma_find_channel(enum dma_transaction_type tx_type);
+struct dma_chan *dma_get_slave_channel(struct dma_chan *chan);
 struct dma_chan *net_dma_find_channel(void);
 #define dma_request_channel(mask, x, y) __dma_request_channel(&(mask), x, y)
 #define dma_request_slave_channel_compat(mask, x, y, dev, name) \
-- 
1.7.9.5

[PATCH v2 2/2] dmaengine: Add hisilicon k3 DMA engine driver

[Zhangfei Gao]

Add dmaengine driver for hisilicon k3 platform based on virt_dma

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
Tested-by: Kai Yang <jean.yangkai@huawei.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
 Documentation/devicetree/bindings/dma/k3dma.txt |   46 ++
 drivers/dma/Kconfig                             |    9 +
 drivers/dma/Makefile                            |    1 +
 drivers/dma/k3dma.c                             |  849 +++++++++++++++++++++++
 4 files changed, 905 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/k3dma.txt
 create mode 100644 drivers/dma/k3dma.c

diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
new file mode 100644
index 0000000..23f8d71
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/k3dma.txt
@@ -0,0 +1,46 @@
+* Hisilicon K3 DMA controller
+
+See dma.txt first
+
+Required properties:
+- compatible: Should be "hisilicon,k3-dma-1.0"
+- reg: Should contain DMA registers location and length.
+- interrupts: Should contain one interrupt shared by all channel
+- #dma-cells: see dma.txt, should be 1, para number
+- dma-channels: physical channels supported
+- dma-requests: virtual channels supported, each virtual channel
+		have specific request line
+- clocks: clock required
+
+Example:
+
+Controller:
+		dma0: dma at fcd02000 {
+			compatible = "hisilicon,k3-dma-1.0";
+			reg = <0xfcd02000 0x1000>;
+			#dma-cells = <1>;
+			dma-channels = <16>;
+			dma-requests = <27>;
+			interrupts = <0 12 4>;
+			clocks = <&pclk>;
+			status = "disable";
+		};
+
+Client:
+Use specific request line passing from dmax
+For example, i2c0 read channel request line is 18, while write channel use 19
+
+		i2c0: i2c at fcb08000 {
+			compatible = "snps,designware-i2c";
+			dmas =	<&dma0 18          /* read channel */
+				 &dma0 19>;        /* write channel */
+			dma-names = "rx", "tx";
+		};
+
+		i2c1: i2c at fcb09000 {
+			compatible = "snps,designware-i2c";
+			dmas =	<&dma0 20          /* read channel */
+				 &dma0 21>;        /* write channel */
+			dma-names = "rx", "tx";
+		};
+
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index e992489..4e7f4bf 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -312,6 +312,15 @@ config MMP_PDMA
 	help
 	  Support the MMP PDMA engine for PXA and MMP platfrom.
 
+config K3_DMA
+	tristate "Hisilicon K3 DMA support"
+	depends on ARCH_HI3xxx
+	select DMA_ENGINE
+	select DMA_VIRTUAL_CHANNELS
+	help
+	  Support the DMA engine for Hisilicon K3 platform
+	  devices.
+
 config DMA_ENGINE
 	bool
 
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index a2b0df5..3b05b15 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -38,3 +38,4 @@ obj-$(CONFIG_DMA_SA11X0) += sa11x0-dma.o
 obj-$(CONFIG_MMP_TDMA) += mmp_tdma.o
 obj-$(CONFIG_DMA_OMAP) += omap-dma.o
 obj-$(CONFIG_MMP_PDMA) += mmp_pdma.o
+obj-$(CONFIG_K3_DMA) += k3dma.o
diff --git a/drivers/dma/k3dma.c b/drivers/dma/k3dma.c
new file mode 100644
index 0000000..1ba08bb
--- /dev/null
+++ b/drivers/dma/k3dma.c
@@ -0,0 +1,849 @@
+/*
+ * Copyright (c) 2013 Linaro Ltd.
+ * Copyright (c) 2013 Hisilicon Limited.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include <linux/sched.h>
+#include <linux/device.h>
+#include <linux/dmaengine.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/of_device.h>
+#include <linux/of.h>
+#include <linux/clk.h>
+#include <linux/of_dma.h>
+
+#include "virt-dma.h"
+
+#define DRIVER_NAME		"k3-dma"
+#define DMA_ALIGN		3
+#define DMA_MAX_SIZE		0x1ffc
+
+#define INT_STAT		0x00
+#define INT_TC1			0x04
+#define INT_ERR1		0x0c
+#define INT_ERR2		0x10
+#define INT_TC1_MASK		0x18
+#define INT_ERR1_MASK		0x20
+#define INT_ERR2_MASK		0x24
+#define INT_TC1_RAW		0x600
+#define INT_ERR1_RAW		0x608
+#define INT_ERR2_RAW		0x610
+#define CH_PRI			0x688
+#define CH_STAT			0x690
+#define CX_CUR_CNT		0x704
+#define CX_LLI			0x800
+#define CX_CNT			0x810
+#define CX_SRC			0x814
+#define CX_DST			0x818
+#define CX_CONFIG		0x81c
+#define AXI_CONFIG		0x820
+#define DEF_AXI_CONFIG		0x201201
+
+#define CX_LLI_CHAIN_EN		0x2
+#define CCFG_EN			0x1
+#define CCFG_MEM2PER		(0x1 << 2)
+#define CCFG_PER2MEM		(0x2 << 2)
+#define CCFG_SRCINCR		(0x1 << 31)
+#define CCFG_DSTINCR		(0x1 << 30)
+
+struct k3_desc_hw {
+	u32 lli;
+	u32 reserved[3];
+	u32 count;
+	u32 saddr;
+	u32 daddr;
+	u32 config;
+} __aligned(32);
+
+struct k3_dma_desc_sw {
+	struct virt_dma_desc	vd;
+	dma_addr_t		desc_hw_lli;
+	size_t			desc_num;
+	size_t			size;
+	struct k3_desc_hw	desc_hw[0];
+};
+
+struct k3_dma_phy;
+
+struct k3_dma_chan {
+	u32			ccfg;
+	struct virt_dma_chan	vc;
+	struct k3_dma_phy	*phy;
+	struct list_head	node;
+	enum dma_transfer_direction dir;
+	dma_addr_t		dev_addr;
+	enum dma_status		status;
+};
+
+struct k3_dma_phy {
+	u32			idx;
+	void __iomem		*base;
+	struct k3_dma_chan	*vchan;
+	struct k3_dma_desc_sw	*ds_run;
+	struct k3_dma_desc_sw	*ds_done;
+};
+
+struct k3_dma_dev {
+	struct dma_device	slave;
+	void __iomem		*base;
+	struct tasklet_struct	task;
+	spinlock_t		lock;
+	struct list_head	chan_pending;
+	struct k3_dma_phy	*phy;
+	struct k3_dma_chan	*chans;
+	struct clk		*clk;
+	u32			dma_channels;
+	u32			dma_requests;
+};
+
+#define to_k3_dma(dmadev) container_of(dmadev, struct k3_dma_dev, slave)
+
+static struct k3_dma_chan *to_k3_chan(struct dma_chan *chan)
+{
+	return container_of(chan, struct k3_dma_chan, vc.chan);
+}
+
+static void k3_dma_pause_dma(struct k3_dma_phy *phy, bool on)
+{
+	u32 val = 0;
+
+	if (on) {
+		val = readl_relaxed(phy->base + CX_CONFIG);
+		val |= CCFG_EN;
+		writel_relaxed(val, phy->base + CX_CONFIG);
+	} else {
+		val = readl_relaxed(phy->base + CX_CONFIG);
+		val &= ~CCFG_EN;
+		writel_relaxed(val, phy->base + CX_CONFIG);
+	}
+}
+
+static void k3_dma_terminate_chan(struct k3_dma_phy *phy, struct k3_dma_dev *d)
+{
+	u32 val = 0;
+
+	k3_dma_pause_dma(phy, false);
+
+	val = 0x1 << phy->idx;
+	writel_relaxed(val, d->base + INT_TC1_RAW);
+	writel_relaxed(val, d->base + INT_ERR1_RAW);
+	writel_relaxed(val, d->base + INT_ERR2_RAW);
+}
+
+static void k3_dma_set_desc(struct k3_dma_phy *phy, struct k3_desc_hw *hw)
+{
+	writel_relaxed(hw->lli, phy->base + CX_LLI);
+	writel_relaxed(hw->count, phy->base + CX_CNT);
+	writel_relaxed(hw->saddr, phy->base + CX_SRC);
+	writel_relaxed(hw->daddr, phy->base + CX_DST);
+	writel_relaxed(DEF_AXI_CONFIG, phy->base + AXI_CONFIG);
+	writel_relaxed(hw->config, phy->base + CX_CONFIG);
+}
+
+static u32 k3_dma_get_curr_cnt(struct k3_dma_dev *d, struct k3_dma_phy *phy)
+{
+	u32 cnt = 0;
+
+	cnt = readl_relaxed(d->base + CX_CUR_CNT + phy->idx * 0x10);
+	cnt &= 0xffff;
+	return cnt;
+}
+
+static u32 k3_dma_get_curr_lli(struct k3_dma_phy *phy)
+{
+	return readl_relaxed(phy->base + CX_LLI);
+}
+
+static u32 k3_dma_get_chan_stat(struct k3_dma_dev *d)
+{
+	return readl_relaxed(d->base + CH_STAT);
+}
+
+static void k3_dma_enable_dma(struct k3_dma_dev *d, bool on)
+{
+	if (on) {
+		/* set same priority */
+		writel_relaxed(0x0, d->base + CH_PRI);
+
+		/* unmask irq */
+		writel_relaxed(0xffff, d->base + INT_TC1_MASK);
+		writel_relaxed(0xffff, d->base + INT_ERR1_MASK);
+		writel_relaxed(0xffff, d->base + INT_ERR2_MASK);
+	} else {
+		/* mask irq */
+		writel_relaxed(0x0, d->base + INT_TC1_MASK);
+		writel_relaxed(0x0, d->base + INT_ERR1_MASK);
+		writel_relaxed(0x0, d->base + INT_ERR2_MASK);
+	}
+}
+
+static irqreturn_t k3_dma_int_handler(int irq, void *dev_id)
+{
+	struct k3_dma_dev *d = (struct k3_dma_dev *)dev_id;
+	struct k3_dma_phy *p;
+	struct k3_dma_chan *c;
+	u32 stat = readl_relaxed(d->base + INT_STAT);
+	u32 tc1  = readl_relaxed(d->base + INT_TC1);
+	u32 err1 = readl_relaxed(d->base + INT_ERR1);
+	u32 err2 = readl_relaxed(d->base + INT_ERR2);
+	u32 i, irq_chan = 0;
+
+	while (stat) {
+		i = __ffs(stat);
+		stat &= (stat - 1);
+		if (likely(tc1 & BIT(i))) {
+			p = &d->phy[i];
+			c = p->vchan;
+			if (c) {
+				unsigned long flags;
+
+				spin_lock_irqsave(&c->vc.lock, flags);
+				vchan_cookie_complete(&p->ds_run->vd);
+				p->ds_done = p->ds_run;
+				spin_unlock_irqrestore(&c->vc.lock, flags);
+			}
+			irq_chan |= BIT(i);
+		}
+		if (unlikely((err1 & BIT(i)) || (err2 & BIT(i))))
+			dev_warn(d->slave.dev, "DMA ERR\n");
+	}
+
+	writel_relaxed(irq_chan, d->base + INT_TC1_RAW);
+	writel_relaxed(err1, d->base + INT_ERR1_RAW);
+	writel_relaxed(err2, d->base + INT_ERR2_RAW);
+
+	if (irq_chan) {
+		tasklet_schedule(&d->task);
+		return IRQ_HANDLED;
+	} else
+		return IRQ_NONE;
+}
+
+static int k3_dma_start_txd(struct k3_dma_chan *c)
+{
+	struct k3_dma_dev *d = to_k3_dma(c->vc.chan.device);
+	struct virt_dma_desc *vd = vchan_next_desc(&c->vc);
+
+	if (!c->phy)
+		return -EAGAIN;
+
+	if (BIT(c->phy->idx) & k3_dma_get_chan_stat(d))
+		return -EAGAIN;
+
+	if (vd) {
+		struct k3_dma_desc_sw *ds =
+			container_of(vd, struct k3_dma_desc_sw, vd);
+		/*
+		 * fetch and remove request from vc->desc_issued
+		 * so vc->desc_issued only contains desc pending
+		 */
+		list_del(&ds->vd.node);
+		c->phy->ds_run = ds;
+		c->phy->ds_done = NULL;
+		/* start dma */
+		k3_dma_set_desc(c->phy, &ds->desc_hw[0]);
+		return 0;
+	}
+	c->phy->ds_done = NULL;
+	c->phy->ds_run = NULL;
+	return -EAGAIN;
+}
+
+static void k3_dma_tasklet(unsigned long arg)
+{
+	struct k3_dma_dev *d = (struct k3_dma_dev *)arg;
+	struct k3_dma_phy *p;
+	struct k3_dma_chan *c, *cn;
+	unsigned pch, pch_alloc = 0;
+
+	/* check new dma request of running channel in vc->desc_issued */
+	list_for_each_entry_safe(c, cn, &d->slave.channels, vc.chan.device_node) {
+		spin_lock_irq(&c->vc.lock);
+		p = c->phy;
+		if (p && p->ds_done) {
+			if (k3_dma_start_txd(c)) {
+				/* No current txd associated with this channel */
+				dev_dbg(d->slave.dev, "pchan %u: free\n", p->idx);
+				/* Mark this channel free */
+				c->phy = NULL;
+				p->vchan = NULL;
+			}
+		}
+		spin_unlock_irq(&c->vc.lock);
+	}
+
+	/* check new channel request in d->chan_pending */
+	spin_lock_irq(&d->lock);
+	for (pch = 0; pch < d->dma_channels; pch++) {
+		p = &d->phy[pch];
+
+		if (p->vchan == NULL && !list_empty(&d->chan_pending)) {
+			c = list_first_entry(&d->chan_pending,
+				struct k3_dma_chan, node);
+			/* remove from d->chan_pending */
+			list_del_init(&c->node);
+			pch_alloc |= 1 << pch;
+			/* Mark this channel allocated */
+			p->vchan = c;
+			c->phy = p;
+			dev_dbg(d->slave.dev, "pchan %u: alloc vchan %p\n", pch, &c->vc);
+		}
+	}
+	spin_unlock_irq(&d->lock);
+
+	for (pch = 0; pch < d->dma_channels; pch++) {
+		if (pch_alloc & (1 << pch)) {
+			p = &d->phy[pch];
+			c = p->vchan;
+			if (c) {
+				spin_lock_irq(&c->vc.lock);
+				k3_dma_start_txd(c);
+				spin_unlock_irq(&c->vc.lock);
+			}
+		}
+	}
+}
+
+static int k3_dma_alloc_chan_resources(struct dma_chan *chan)
+{
+	return 0;
+}
+
+static void k3_dma_free_chan_resources(struct dma_chan *chan)
+{
+	struct k3_dma_chan *c = to_k3_chan(chan);
+	struct k3_dma_dev *d = to_k3_dma(chan->device);
+	unsigned long flags;
+
+	spin_lock_irqsave(&d->lock, flags);
+	list_del_init(&c->node);
+	spin_unlock_irqrestore(&d->lock, flags);
+
+	vchan_free_chan_resources(&c->vc);
+	c->ccfg = 0;
+}
+
+static enum dma_status k3_dma_tx_status(struct dma_chan *chan,
+	dma_cookie_t cookie, struct dma_tx_state *state)
+{
+	struct k3_dma_chan *c = to_k3_chan(chan);
+	struct k3_dma_dev *d = to_k3_dma(chan->device);
+	struct k3_dma_phy *p;
+	struct virt_dma_desc *vd;
+	unsigned long flags;
+	enum dma_status ret;
+	size_t bytes = 0;
+
+	ret = dma_cookie_status(&c->vc.chan, cookie, state);
+	if (ret == DMA_SUCCESS)
+		return ret;
+
+	spin_lock_irqsave(&c->vc.lock, flags);
+	p = c->phy;
+	ret = c->status;
+
+	/*
+	 * If the cookie is on our issue queue, then the residue is
+	 * its total size.
+	 */
+	vd = vchan_find_desc(&c->vc, cookie);
+	if (vd) {
+		bytes = container_of(vd, struct k3_dma_desc_sw, vd)->size;
+	} else if ((!p) || (!p->ds_run)) {
+		bytes = 0;
+	} else {
+		struct k3_dma_desc_sw *ds = p->ds_run;
+		u32 clli = 0, index = 0;
+
+		bytes = k3_dma_get_curr_cnt(d, p);
+		clli = k3_dma_get_curr_lli(p);
+		index = (clli - ds->desc_hw_lli) / sizeof(struct k3_desc_hw);
+		for (; index < ds->desc_num; index++) {
+			bytes += ds->desc_hw[index].count;
+			/* end of lli */
+			if (!ds->desc_hw[index].lli)
+				break;
+		}
+	}
+	spin_unlock_irqrestore(&c->vc.lock, flags);
+	dma_set_residue(state, bytes);
+	return ret;
+}
+
+static void k3_dma_issue_pending(struct dma_chan *chan)
+{
+	struct k3_dma_chan *c = to_k3_chan(chan);
+	struct k3_dma_dev *d = to_k3_dma(chan->device);
+	unsigned long flags;
+
+	spin_lock_irqsave(&c->vc.lock, flags);
+	/* add request to vc->desc_issued */
+	if (vchan_issue_pending(&c->vc)) {
+		spin_lock(&d->lock);
+		if (!c->phy) {
+			if (list_empty(&c->node)) {
+				/* if new channel, add chan_pending */
+				list_add_tail(&c->node, &d->chan_pending);
+				/* check in tasklet */
+				tasklet_schedule(&d->task);
+				dev_dbg(d->slave.dev, "vchan %p: issued\n", &c->vc);
+			}
+		}
+		spin_unlock(&d->lock);
+	} else
+		dev_dbg(d->slave.dev, "vchan %p: nothing to issue\n", &c->vc);
+	spin_unlock_irqrestore(&c->vc.lock, flags);
+}
+
+static void k3_dma_fill_desc(struct k3_dma_desc_sw *ds, dma_addr_t dst,
+			dma_addr_t src, size_t len, u32 num, u32 ccfg)
+{
+	if ((num + 1) < ds->desc_num)
+		ds->desc_hw[num].lli = ds->desc_hw_lli + (num + 1) *
+			sizeof(struct k3_desc_hw);
+	ds->desc_hw[num].lli |= CX_LLI_CHAIN_EN;
+	ds->desc_hw[num].count = len;
+	ds->desc_hw[num].saddr = src;
+	ds->desc_hw[num].daddr = dst;
+	ds->desc_hw[num].config = ccfg;
+}
+
+static struct dma_async_tx_descriptor *k3_dma_prep_memcpy(
+	struct dma_chan *chan,	dma_addr_t dst, dma_addr_t src,
+	size_t len, unsigned long flags)
+{
+	struct k3_dma_chan *c = to_k3_chan(chan);
+	struct k3_dma_desc_sw *ds;
+	size_t copy = 0;
+	int num = 0;
+
+	if (!len)
+		return NULL;
+
+	num = DIV_ROUND_UP(len, DMA_MAX_SIZE);
+	ds = kzalloc(sizeof(*ds) + num * sizeof(ds->desc_hw[0]), GFP_ATOMIC);
+	if (!ds) {
+		dev_dbg(chan->device->dev, "vchan %p: kzalloc fail\n", &c->vc);
+		return NULL;
+	}
+	ds->desc_hw_lli = __virt_to_phys((unsigned long)&ds->desc_hw[0]);
+	ds->size = len;
+	ds->desc_num = num;
+	num = 0;
+
+	if (!c->ccfg) {
+		/* default is memtomem, without calling device_control */
+		c->ccfg = CCFG_SRCINCR | CCFG_DSTINCR | CCFG_EN;
+		c->ccfg |= (0xf << 20) | (0xf << 24);	/* burst = 16 */
+		c->ccfg |= (0x3 << 12) | (0x3 << 16);	/* width = 64 bit */
+	}
+
+	do {
+		copy = min_t(size_t, len, DMA_MAX_SIZE);
+		k3_dma_fill_desc(ds, dst, src, copy, num++, c->ccfg);
+
+		if (c->dir == DMA_MEM_TO_DEV) {
+			src += copy;
+		} else if (c->dir == DMA_DEV_TO_MEM) {
+			dst += copy;
+		} else {
+			src += copy;
+			dst += copy;
+		}
+		len -= copy;
+	} while (len);
+
+	ds->desc_hw[num-1].lli = 0;	/* end of link */
+	return vchan_tx_prep(&c->vc, &ds->vd, flags);
+}
+
+static struct dma_async_tx_descriptor *k3_dma_prep_slave_sg(
+	struct dma_chan *chan, struct scatterlist *sgl, unsigned int sglen,
+	enum dma_transfer_direction dir, unsigned long flags, void *context)
+{
+	struct k3_dma_chan *c = to_k3_chan(chan);
+	struct k3_dma_desc_sw *ds;
+	size_t len, avail, total = 0;
+	struct scatterlist *sg;
+	dma_addr_t addr, src = 0, dst = 0;
+	int num = sglen, i;
+
+	if (sgl == 0)
+		return NULL;
+
+	for_each_sg(sgl, sg, sglen, i) {
+		avail = sg_dma_len(sg);
+		if (avail > DMA_MAX_SIZE)
+			num += DIV_ROUND_UP(avail, DMA_MAX_SIZE) - 1;
+	}
+
+	ds = kzalloc(sizeof(*ds) + num * sizeof(ds->desc_hw[0]), GFP_ATOMIC);
+	if (!ds) {
+		dev_dbg(chan->device->dev, "vchan %p: kzalloc fail\n", &c->vc);
+		return NULL;
+	}
+	ds->desc_hw_lli = __virt_to_phys((unsigned long)&ds->desc_hw[0]);
+	ds->desc_num = num;
+	num = 0;
+
+	for_each_sg(sgl, sg, sglen, i) {
+		addr = sg_dma_address(sg);
+		avail = sg_dma_len(sg);
+		total += avail;
+
+		do {
+			len = min_t(size_t, avail, DMA_MAX_SIZE);
+
+			if (dir == DMA_MEM_TO_DEV) {
+				src = addr;
+				dst = c->dev_addr;
+			} else if (dir == DMA_DEV_TO_MEM) {
+				src = c->dev_addr;
+				dst = addr;
+			}
+
+			k3_dma_fill_desc(ds, dst, src, len, num++, c->ccfg);
+
+			addr += len;
+			avail -= len;
+		} while (avail);
+	}
+
+	ds->desc_hw[num-1].lli = 0;	/* end of link */
+	ds->size = total;
+	return vchan_tx_prep(&c->vc, &ds->vd, flags);
+}
+
+static int k3_dma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
+	unsigned long arg)
+{
+	struct k3_dma_chan *c = to_k3_chan(chan);
+	struct k3_dma_dev *d = to_k3_dma(chan->device);
+	struct dma_slave_config *cfg = (void *)arg;
+	struct k3_dma_phy *p = NULL;
+	unsigned long flags;
+	u32 maxburst = 0, val = 0;
+	enum dma_slave_buswidth width = DMA_SLAVE_BUSWIDTH_UNDEFINED;
+	LIST_HEAD(head);
+
+	if (c)
+		p = c->phy;
+
+	switch (cmd) {
+	case DMA_SLAVE_CONFIG:
+		if (cfg == NULL)
+			return -EINVAL;
+		c->dir = cfg->direction;
+		if (c->dir == DMA_DEV_TO_MEM) {
+			c->ccfg = CCFG_DSTINCR;
+			c->dev_addr = cfg->src_addr;
+			maxburst = cfg->src_maxburst;
+			width = cfg->src_addr_width;
+		} else if (c->dir == DMA_MEM_TO_DEV) {
+			c->ccfg = CCFG_SRCINCR;
+			c->dev_addr = cfg->dst_addr;
+			maxburst = cfg->dst_maxburst;
+			width = cfg->dst_addr_width;
+		}
+		switch (width) {
+		case DMA_SLAVE_BUSWIDTH_1_BYTE:
+			val = 0;
+			break;
+		case DMA_SLAVE_BUSWIDTH_2_BYTES:
+			val = 1;
+			break;
+		case DMA_SLAVE_BUSWIDTH_4_BYTES:
+			val = 2;
+			break;
+		case DMA_SLAVE_BUSWIDTH_8_BYTES:
+			val = 3;
+			break;
+		default:
+			break;
+		}
+		c->ccfg |= (val << 12) | (val << 16);
+
+		if ((maxburst == 0) || (maxburst > 16))
+			val = 16;
+		else
+			val = maxburst - 1;
+		c->ccfg |= (val << 20) | (val << 24);
+		c->ccfg |= CCFG_MEM2PER | CCFG_EN;
+
+		/* specific request line */
+		c->ccfg |= c->vc.chan.chan_id << 4;
+		break;
+
+	case DMA_TERMINATE_ALL:
+		dev_dbg(d->slave.dev, "vchan %p: terminate all\n", &c->vc);
+
+		/* Prevent this channel being scheduled */
+		spin_lock(&d->lock);
+		list_del_init(&c->node);
+		spin_unlock(&d->lock);
+
+		/* Clear the tx descriptor lists */
+		spin_lock_irqsave(&c->vc.lock, flags);
+		vchan_get_all_descriptors(&c->vc, &head);
+		if (p) {
+			/* vchan is assigned to a pchan - stop the channel */
+			k3_dma_terminate_chan(p, d);
+			c->phy = NULL;
+			p->vchan = NULL;
+			p->ds_run = p->ds_done = NULL;
+		}
+		spin_unlock_irqrestore(&c->vc.lock, flags);
+		vchan_dma_desc_free_list(&c->vc, &head);
+		break;
+
+	case DMA_PAUSE:
+		dev_dbg(d->slave.dev, "vchan %p: pause\n", &c->vc);
+		if (c->status == DMA_IN_PROGRESS) {
+			c->status = DMA_PAUSED;
+			if (p) {
+				k3_dma_pause_dma(p, false);
+			} else {
+				spin_lock(&d->lock);
+				list_del_init(&c->node);
+				spin_unlock(&d->lock);
+			}
+		}
+		break;
+
+	case DMA_RESUME:
+		dev_dbg(d->slave.dev, "vchan %p: resume\n", &c->vc);
+		spin_lock_irqsave(&c->vc.lock, flags);
+		if (c->status == DMA_PAUSED) {
+			c->status = DMA_IN_PROGRESS;
+			if (p) {
+				k3_dma_pause_dma(p, true);
+			} else if (!list_empty(&c->vc.desc_issued)) {
+				spin_lock(&d->lock);
+				list_add_tail(&c->node, &d->chan_pending);
+				spin_unlock(&d->lock);
+			}
+		}
+		spin_unlock_irqrestore(&c->vc.lock, flags);
+		break;
+	default:
+		return -ENXIO;
+	}
+	return 0;
+}
+
+static void k3_dma_free_desc(struct virt_dma_desc *vd)
+{
+	struct k3_dma_desc_sw *ds =
+		container_of(vd, struct k3_dma_desc_sw, vd);
+
+	kfree(ds);
+}
+
+static struct of_device_id k3_pdma_dt_ids[] = {
+	{ .compatible = "hisilicon,k3-dma-1.0", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, k3_pdma_dt_ids);
+
+static struct dma_chan *k3_of_dma_simple_xlate(struct of_phandle_args *dma_spec,
+						struct of_dma *ofdma)
+{
+	struct k3_dma_dev *d = ofdma->of_dma_data;
+	unsigned int request = dma_spec->args[0];
+
+	if (request > d->dma_requests)
+		return NULL;
+
+	return dma_get_slave_channel(&(d->chans[request].vc.chan));
+}
+
+static int k3_dma_probe(struct platform_device *op)
+{
+	struct k3_dma_dev *d;
+	const struct of_device_id *of_id;
+	struct resource *iores;
+	int i, ret, irq = 0;
+
+	iores = platform_get_resource(op, IORESOURCE_MEM, 0);
+	if (!iores)
+		return -EINVAL;
+
+	d = devm_kzalloc(&op->dev, sizeof(*d), GFP_KERNEL);
+	if (!d)
+		return -ENOMEM;
+
+	d->base = devm_request_and_ioremap(&op->dev, iores);
+	if (!d->base)
+		return -EADDRNOTAVAIL;
+
+	of_id = of_match_device(k3_pdma_dt_ids, &op->dev);
+	if (of_id) {
+		of_property_read_u32((&op->dev)->of_node,
+				"dma-channels", &d->dma_channels);
+		of_property_read_u32((&op->dev)->of_node,
+				"dma-requests", &d->dma_requests);
+	}
+
+	d->clk = devm_clk_get(&op->dev, NULL);
+	if (IS_ERR(d->clk)) {
+		dev_err(&op->dev, "no dma clk\n");
+		return PTR_ERR(d->clk);
+	}
+
+	irq = platform_get_irq(op, 0);
+	ret = devm_request_irq(&op->dev, irq,
+			k3_dma_int_handler, IRQF_DISABLED, DRIVER_NAME, d);
+	if (ret)
+		return ret;
+
+	/* init phy channel */
+	d->phy = devm_kzalloc(&op->dev,
+		d->dma_channels * sizeof(struct k3_dma_phy), GFP_KERNEL);
+	if (d->phy == NULL)
+		return -ENOMEM;
+
+	for (i = 0; i < d->dma_channels; i++) {
+		struct k3_dma_phy *p = &d->phy[i];
+
+		p->idx = i;
+		p->base = d->base + i * 0x40;
+	}
+
+	INIT_LIST_HEAD(&d->slave.channels);
+	dma_cap_set(DMA_SLAVE, d->slave.cap_mask);
+	dma_cap_set(DMA_MEMCPY, d->slave.cap_mask);
+	d->slave.dev = &op->dev;
+	d->slave.device_alloc_chan_resources = k3_dma_alloc_chan_resources;
+	d->slave.device_free_chan_resources = k3_dma_free_chan_resources;
+	d->slave.device_tx_status = k3_dma_tx_status;
+	d->slave.device_prep_dma_memcpy = k3_dma_prep_memcpy;
+	d->slave.device_prep_slave_sg = k3_dma_prep_slave_sg;
+	d->slave.device_issue_pending = k3_dma_issue_pending;
+	d->slave.device_control = k3_dma_control;
+	d->slave.copy_align = DMA_ALIGN;
+	d->slave.chancnt = d->dma_requests;
+
+	/* init virtual channel */
+	d->chans = devm_kzalloc(&op->dev,
+		d->dma_requests * sizeof(struct k3_dma_chan), GFP_KERNEL);
+	if (d->chans == NULL)
+		return -ENOMEM;
+
+	for (i = 0; i < d->dma_requests; i++) {
+		struct k3_dma_chan *c = &d->chans[i];
+
+		c->status = DMA_IN_PROGRESS;
+		INIT_LIST_HEAD(&c->node);
+		c->vc.desc_free = k3_dma_free_desc;
+		vchan_init(&c->vc, &d->slave);
+	}
+
+	/* Enable clock before accessing registers */
+	ret = clk_prepare_enable(d->clk);
+	if (ret < 0) {
+		dev_err(&op->dev, "clk_prepare_enable failed: %d\n", ret);
+		return -EINVAL;
+	}
+
+	k3_dma_enable_dma(d, true);
+
+	ret = dma_async_device_register(&d->slave);
+	if (ret)
+		return ret;
+
+	ret = of_dma_controller_register((&op->dev)->of_node,
+					k3_of_dma_simple_xlate, d);
+	if (ret)
+		goto of_dma_register_fail;
+
+	spin_lock_init(&d->lock);
+	INIT_LIST_HEAD(&d->chan_pending);
+	tasklet_init(&d->task, k3_dma_tasklet, (unsigned long)d);
+	platform_set_drvdata(op, d);
+	dev_info(&op->dev, "initialized\n");
+
+	return 0;
+
+of_dma_register_fail:
+	dma_async_device_unregister(&d->slave);
+	return ret;
+}
+
+static int k3_dma_remove(struct platform_device *op)
+{
+	struct k3_dma_chan *c, *cn;
+	struct k3_dma_dev *d = platform_get_drvdata(op);
+
+	dma_async_device_unregister(&d->slave);
+	of_dma_controller_free((&op->dev)->of_node);
+
+	list_for_each_entry_safe(c, cn, &d->slave.channels, vc.chan.device_node) {
+		list_del(&c->vc.chan.device_node);
+		tasklet_kill(&c->vc.task);
+	}
+	tasklet_kill(&d->task);
+	clk_disable_unprepare(d->clk);
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int k3_dma_pltfm_suspend(struct device *dev)
+{
+	struct k3_dma_dev *d = dev_get_drvdata(dev);
+	u32 stat = 0;
+
+	stat = k3_dma_get_chan_stat(d);
+	if (stat) {
+		dev_warn(d->slave.dev,
+			"chan %d is running fail to suspend\n", stat);
+		return -1;
+	}
+	k3_dma_enable_dma(d, false);
+	clk_disable_unprepare(d->clk);
+	return 0;
+}
+
+static int k3_dma_pltfm_resume(struct device *dev)
+{
+	struct k3_dma_dev *d = dev_get_drvdata(dev);
+	int ret = 0;
+
+	ret = clk_prepare_enable(d->clk);
+	if (ret < 0) {
+		dev_err(d->slave.dev, "clk_prepare_enable failed: %d\n", ret);
+		return -EINVAL;
+	}
+	k3_dma_enable_dma(d, true);
+	return 0;
+}
+#else
+#define k3_dma_pltfm_suspend	NULL
+#define k3_dma_pltfm_resume	NULL
+#endif /* CONFIG_PM_SLEEP */
+
+SIMPLE_DEV_PM_OPS(k3_dma_pltfm_pmops, k3_dma_pltfm_suspend, k3_dma_pltfm_resume);
+
+static struct platform_driver k3_pdma_driver = {
+	.driver		= {
+		.name	= DRIVER_NAME,
+		.owner  = THIS_MODULE,
+		.pm	= &k3_dma_pltfm_pmops,
+		.of_match_table = k3_pdma_dt_ids,
+	},
+	.probe		= k3_dma_probe,
+	.remove		= k3_dma_remove,
+};
+
+module_platform_driver(k3_pdma_driver);
+
+MODULE_DESCRIPTION("Hisilicon k3 DMA Driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

[PATCH v2 1/2] dmaengine: add interface of dma_get_slave_channel

[Arnd Bergmann]

On Friday 28 June 2013, Zhangfei Gao wrote:
> Suggested by Arnd, add dma_get_slave_channel interface
> Dma host driver could get specific channel specificied by request line, rather than filter.
> 
> host example:
> static struct dma_chan *xx_of_dma_simple_xlate(struct of_phandle_args *dma_spec,
>                 struct of_dma *ofdma)
> {
>         struct xx_dma_dev *d = ofdma->of_dma_data;
>         unsigned int request = dma_spec->args[0];
> 
>         if (request > d->dma_requests)
>                 return NULL;
> 
>         return dma_get_slave_channel(&(d->chans[request].vc.chan));
> }
> 
> probe:
> of_dma_controller_register((&op->dev)->of_node, xx_of_dma_simple_xlate, d);
> 
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH v2 1/2] dmaengine: add interface of dma_get_slave_channel

[Vinod Koul]

On Fri, Jun 28, 2013 at 08:39:12PM +0800, Zhangfei Gao wrote:
> Suggested by Arnd, add dma_get_slave_channel interface
> Dma host driver could get specific channel specificied by request line, rather than filter.
> 
> host example:
> static struct dma_chan *xx_of_dma_simple_xlate(struct of_phandle_args *dma_spec,
> 		struct of_dma *ofdma)
> {
> 	struct xx_dma_dev *d = ofdma->of_dma_data;
> 	unsigned int request = dma_spec->args[0];
> 
> 	if (request > d->dma_requests)
> 		return NULL;
> 
> 	return dma_get_slave_channel(&(d->chans[request].vc.chan));
> }
> 
> probe:
> of_dma_controller_register((&op->dev)->of_node, xx_of_dma_simple_xlate, d);
> 
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
Applied, thanks

~Vinod

[PATCH v2 2/2] dmaengine: Add hisilicon k3 DMA engine driver

[Vinod Koul]

On Fri, Jun 28, 2013 at 08:39:13PM +0800, Zhangfei Gao wrote:
> Add dmaengine driver for hisilicon k3 platform based on virt_dma
> 
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> Tested-by: Kai Yang <jean.yangkai@huawei.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> ---

> +#define DRIVER_NAME		"k3-dma"
> +#define DMA_ALIGN		3
> +#define DMA_MAX_SIZE		0x1ffc
> +
> +#define INT_STAT		0x00
> +#define INT_TC1			0x04
> +#define INT_ERR1		0x0c
> +#define INT_ERR2		0x10
> +#define INT_TC1_MASK		0x18
> +#define INT_ERR1_MASK		0x20
> +#define INT_ERR2_MASK		0x24
> +#define INT_TC1_RAW		0x600
> +#define INT_ERR1_RAW		0x608
> +#define INT_ERR2_RAW		0x610
> +#define CH_PRI			0x688
> +#define CH_STAT			0x690
> +#define CX_CUR_CNT		0x704
> +#define CX_LLI			0x800
> +#define CX_CNT			0x810
> +#define CX_SRC			0x814
> +#define CX_DST			0x818
> +#define CX_CONFIG		0x81c
> +#define AXI_CONFIG		0x820
> +#define DEF_AXI_CONFIG		0x201201
> +
> +#define CX_LLI_CHAIN_EN		0x2
> +#define CCFG_EN			0x1
> +#define CCFG_MEM2PER		(0x1 << 2)
> +#define CCFG_PER2MEM		(0x2 << 2)
> +#define CCFG_SRCINCR		(0x1 << 31)
> +#define CCFG_DSTINCR		(0x1 << 30)
I see these are not namespace aptly and can collide..

> +static int k3_dma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
> +	unsigned long arg)
> +{
> +	struct k3_dma_chan *c = to_k3_chan(chan);
> +	struct k3_dma_dev *d = to_k3_dma(chan->device);
> +	struct dma_slave_config *cfg = (void *)arg;
> +	struct k3_dma_phy *p = NULL;
> +	unsigned long flags;
> +	u32 maxburst = 0, val = 0;
> +	enum dma_slave_buswidth width = DMA_SLAVE_BUSWIDTH_UNDEFINED;
> +	LIST_HEAD(head);
> +
> +	if (c)
> +		p = c->phy;
> +
> +	switch (cmd) {
> +	case DMA_SLAVE_CONFIG:
> +		if (cfg == NULL)
> +			return -EINVAL;
> +		c->dir = cfg->direction;
> +		if (c->dir == DMA_DEV_TO_MEM) {
> +			c->ccfg = CCFG_DSTINCR;
> +			c->dev_addr = cfg->src_addr;
> +			maxburst = cfg->src_maxburst;
> +			width = cfg->src_addr_width;
> +		} else if (c->dir == DMA_MEM_TO_DEV) {
> +			c->ccfg = CCFG_SRCINCR;
> +			c->dev_addr = cfg->dst_addr;
> +			maxburst = cfg->dst_maxburst;
> +			width = cfg->dst_addr_width;
> +		}
> +		switch (width) {
> +		case DMA_SLAVE_BUSWIDTH_1_BYTE:
> +			val = 0;
> +			break;
> +		case DMA_SLAVE_BUSWIDTH_2_BYTES:
> +			val = 1;
> +			break;
> +		case DMA_SLAVE_BUSWIDTH_4_BYTES:
> +			val = 2;
> +			break;
> +		case DMA_SLAVE_BUSWIDTH_8_BYTES:
> +			val = 3;
DMA_SLAVE_BUSWIDTHS are 1, 2, 4, 8...
so if you can do val = ffs(width) as well?


> +	case DMA_PAUSE:
> +		dev_dbg(d->slave.dev, "vchan %p: pause\n", &c->vc);
> +		if (c->status == DMA_IN_PROGRESS) {
> +			c->status = DMA_PAUSED;
> +			if (p) {
> +				k3_dma_pause_dma(p, false);
> +			} else {
> +				spin_lock(&d->lock);
> +				list_del_init(&c->node);
> +				spin_unlock(&d->lock);
> +			}
why do we need the else part here?

> +		}
> +		break;
> +
> +	case DMA_RESUME:
> +		dev_dbg(d->slave.dev, "vchan %p: resume\n", &c->vc);
> +		spin_lock_irqsave(&c->vc.lock, flags);
> +		if (c->status == DMA_PAUSED) {
> +			c->status = DMA_IN_PROGRESS;
> +			if (p) {
> +				k3_dma_pause_dma(p, true);
> +			} else if (!list_empty(&c->vc.desc_issued)) {
> +				spin_lock(&d->lock);
> +				list_add_tail(&c->node, &d->chan_pending);
> +				spin_unlock(&d->lock);
> +			}
ditto?

> +		}
> +		spin_unlock_irqrestore(&c->vc.lock, flags);
> +		break;
> +	default:
> +		return -ENXIO;
> +	}
> +	return 0;
> +}
> +

> +
> +#ifdef CONFIG_PM_SLEEP
> +static int k3_dma_pltfm_suspend(struct device *dev)
> +{
> +	struct k3_dma_dev *d = dev_get_drvdata(dev);
> +	u32 stat = 0;
> +
> +	stat = k3_dma_get_chan_stat(d);
> +	if (stat) {
> +		dev_warn(d->slave.dev,
> +			"chan %d is running fail to suspend\n", stat);
> +		return -1;
> +	}
> +	k3_dma_enable_dma(d, false);
> +	clk_disable_unprepare(d->clk);
> +	return 0;
> +}
> +
> +static int k3_dma_pltfm_resume(struct device *dev)
> +{
> +	struct k3_dma_dev *d = dev_get_drvdata(dev);
> +	int ret = 0;
> +
> +	ret = clk_prepare_enable(d->clk);
> +	if (ret < 0) {
> +		dev_err(d->slave.dev, "clk_prepare_enable failed: %d\n", ret);
> +		return -EINVAL;
> +	}
> +	k3_dma_enable_dma(d, true);
> +	return 0;
> +}
> +#else
> +#define k3_dma_pltfm_suspend	NULL
> +#define k3_dma_pltfm_resume	NULL
> +#endif /* CONFIG_PM_SLEEP */
you dont need #ifdef here, then whats the use of SIMPLE_DEV_PM_OPS below?
> +
> +SIMPLE_DEV_PM_OPS(k3_dma_pltfm_pmops, k3_dma_pltfm_suspend, k3_dma_pltfm_resume);
pltfm... can we skip this or use platform, plat...
> +
> +static struct platform_driver k3_pdma_driver = {
> +	.driver		= {
> +		.name	= DRIVER_NAME,
> +		.owner  = THIS_MODULE,
> +		.pm	= &k3_dma_pltfm_pmops,
> +		.of_match_table = k3_pdma_dt_ids,
> +	},
> +	.probe		= k3_dma_probe,
> +	.remove		= k3_dma_remove,
> +};
> +
> +module_platform_driver(k3_pdma_driver);
> +
> +MODULE_DESCRIPTION("Hisilicon k3 DMA Driver");
> +MODULE_LICENSE("GPL v2");
MODULE_ALIAS?

~Vinod

[PATCH v2 2/2] dmaengine: Add hisilicon k3 DMA engine driver

[zhangfei gao]

On Tue, Aug 13, 2013 at 7:20 PM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Fri, Jun 28, 2013 at 08:39:13PM +0800, Zhangfei Gao wrote:
>> Add dmaengine driver for hisilicon k3 platform based on virt_dma
>>
>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>> Tested-by: Kai Yang <jean.yangkai@huawei.com>
>> Acked-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>
>> +#define DRIVER_NAME          "k3-dma"
>> +#define DMA_ALIGN            3
>> +#define DMA_MAX_SIZE         0x1ffc
>> +
>> +#define INT_STAT             0x00
>> +#define INT_TC1                      0x04
>> +#define INT_ERR1             0x0c
>> +#define INT_ERR2             0x10
>> +#define INT_TC1_MASK         0x18
>> +#define INT_ERR1_MASK                0x20
>> +#define INT_ERR2_MASK                0x24
>> +#define INT_TC1_RAW          0x600
>> +#define INT_ERR1_RAW         0x608
>> +#define INT_ERR2_RAW         0x610
>> +#define CH_PRI                       0x688
>> +#define CH_STAT                      0x690
>> +#define CX_CUR_CNT           0x704
>> +#define CX_LLI                       0x800
>> +#define CX_CNT                       0x810
>> +#define CX_SRC                       0x814
>> +#define CX_DST                       0x818
>> +#define CX_CONFIG            0x81c
>> +#define AXI_CONFIG           0x820
>> +#define DEF_AXI_CONFIG               0x201201
>> +
>> +#define CX_LLI_CHAIN_EN              0x2
>> +#define CCFG_EN                      0x1
>> +#define CCFG_MEM2PER         (0x1 << 2)
>> +#define CCFG_PER2MEM         (0x2 << 2)
>> +#define CCFG_SRCINCR         (0x1 << 31)
>> +#define CCFG_DSTINCR         (0x1 << 30)
> I see these are not namespace aptly and can collide..
OK,
How about change name to
#define CX_CFG                 0x81c

#define CX_CFG_EN              0x1
#define CX_CFG_MEM2PER         (0x1 << 2)
~

>> +             switch (width) {
>> +             case DMA_SLAVE_BUSWIDTH_1_BYTE:
>> +                     val = 0;
>> +                     break;
>> +             case DMA_SLAVE_BUSWIDTH_2_BYTES:
>> +                     val = 1;
>> +                     break;
>> +             case DMA_SLAVE_BUSWIDTH_4_BYTES:
>> +                     val = 2;
>> +                     break;
>> +             case DMA_SLAVE_BUSWIDTH_8_BYTES:
>> +                     val = 3;
> DMA_SLAVE_BUSWIDTHS are 1, 2, 4, 8...
> so if you can do val = ffs(width) as well?

Good idea, will use __ffs(width) here.

>
>
>> +     case DMA_PAUSE:
>> +             dev_dbg(d->slave.dev, "vchan %p: pause\n", &c->vc);
>> +             if (c->status == DMA_IN_PROGRESS) {
>> +                     c->status = DMA_PAUSED;
>> +                     if (p) {
>> +                             k3_dma_pause_dma(p, false);
>> +                     } else {
>> +                             spin_lock(&d->lock);
>> +                             list_del_init(&c->node);
>> +                             spin_unlock(&d->lock);
>> +                     }
> why do we need the else part here?
Since asynchronous mode is supported.
Desc is submitted to list but may not get physical channel to run.

>
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int k3_dma_pltfm_suspend(struct device *dev)
>> +{
>> +     struct k3_dma_dev *d = dev_get_drvdata(dev);
>> +     u32 stat = 0;
>> +
>> +     stat = k3_dma_get_chan_stat(d);
>> +     if (stat) {
>> +             dev_warn(d->slave.dev,
>> +                     "chan %d is running fail to suspend\n", stat);
>> +             return -1;
>> +     }
>> +     k3_dma_enable_dma(d, false);
>> +     clk_disable_unprepare(d->clk);
>> +     return 0;
>> +}
>> +
>> +static int k3_dma_pltfm_resume(struct device *dev)
>> +{
>> +     struct k3_dma_dev *d = dev_get_drvdata(dev);
>> +     int ret = 0;
>> +
>> +     ret = clk_prepare_enable(d->clk);
>> +     if (ret < 0) {
>> +             dev_err(d->slave.dev, "clk_prepare_enable failed: %d\n", ret);
>> +             return -EINVAL;
>> +     }
>> +     k3_dma_enable_dma(d, true);
>> +     return 0;
>> +}
>> +#else
>> +#define k3_dma_pltfm_suspend NULL
>> +#define k3_dma_pltfm_resume  NULL
>> +#endif /* CONFIG_PM_SLEEP */
> you dont need #ifdef here, then whats the use of SIMPLE_DEV_PM_OPS below?
Yes.

>> +
>> +SIMPLE_DEV_PM_OPS(k3_dma_pltfm_pmops, k3_dma_pltfm_suspend, k3_dma_pltfm_resume);
> pltfm... can we skip this or use platform, plat...
Got it.

>> +
>> +static struct platform_driver k3_pdma_driver = {
>> +     .driver         = {
>> +             .name   = DRIVER_NAME,
>> +             .owner  = THIS_MODULE,
>> +             .pm     = &k3_dma_pltfm_pmops,
>> +             .of_match_table = k3_pdma_dt_ids,
>> +     },
>> +     .probe          = k3_dma_probe,
>> +     .remove         = k3_dma_remove,
>> +};
>> +
>> +module_platform_driver(k3_pdma_driver);
>> +
>> +MODULE_DESCRIPTION("Hisilicon k3 DMA Driver");
>> +MODULE_LICENSE("GPL v2");
> MODULE_ALIAS?

OK.
>
> ~Vinod
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/2] dmaengine: Add hisilicon k3 DMA engine driver

[Vinod Koul]

On Thu, Aug 15, 2013 at 01:54:28PM +0800, zhangfei gao wrote:
> On Tue, Aug 13, 2013 at 7:20 PM, Vinod Koul <vinod.koul@intel.com> wrote:
> > On Fri, Jun 28, 2013 at 08:39:13PM +0800, Zhangfei Gao wrote:
> >> Add dmaengine driver for hisilicon k3 platform based on virt_dma
> >>
> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> >> Tested-by: Kai Yang <jean.yangkai@huawei.com>
> >> Acked-by: Arnd Bergmann <arnd@arndb.de>
> >> ---
> >
> >> +#define DRIVER_NAME          "k3-dma"
> >> +#define DMA_ALIGN            3
> >> +#define DMA_MAX_SIZE         0x1ffc
> >> +
> >> +#define INT_STAT             0x00
> >> +#define INT_TC1                      0x04
> >> +#define INT_ERR1             0x0c
> >> +#define INT_ERR2             0x10
> >> +#define INT_TC1_MASK         0x18
> >> +#define INT_ERR1_MASK                0x20
> >> +#define INT_ERR2_MASK                0x24
> >> +#define INT_TC1_RAW          0x600
> >> +#define INT_ERR1_RAW         0x608
> >> +#define INT_ERR2_RAW         0x610
> >> +#define CH_PRI                       0x688
> >> +#define CH_STAT                      0x690
> >> +#define CX_CUR_CNT           0x704
> >> +#define CX_LLI                       0x800
> >> +#define CX_CNT                       0x810
> >> +#define CX_SRC                       0x814
> >> +#define CX_DST                       0x818
> >> +#define CX_CONFIG            0x81c
> >> +#define AXI_CONFIG           0x820
> >> +#define DEF_AXI_CONFIG               0x201201
> >> +
> >> +#define CX_LLI_CHAIN_EN              0x2
> >> +#define CCFG_EN                      0x1
> >> +#define CCFG_MEM2PER         (0x1 << 2)
> >> +#define CCFG_PER2MEM         (0x2 << 2)
> >> +#define CCFG_SRCINCR         (0x1 << 31)
> >> +#define CCFG_DSTINCR         (0x1 << 30)
> > I see these are not namespace aptly and can collide..
> OK,
> How about change name to
> #define CX_CFG                 0x81c
since you are calling your driver K3_DMA it would be apt to namespace all of the
above with K3_INT_STAT to K3_CFG_EN and so on...

> 
> #define CX_CFG_EN              0x1
> #define CX_CFG_MEM2PER         (0x1 << 2)
> ~
> 
> >> +             switch (width) {
> >> +             case DMA_SLAVE_BUSWIDTH_1_BYTE:
> >> +                     val = 0;
> >> +                     break;
> >> +             case DMA_SLAVE_BUSWIDTH_2_BYTES:
> >> +                     val = 1;
> >> +                     break;
> >> +             case DMA_SLAVE_BUSWIDTH_4_BYTES:
> >> +                     val = 2;
> >> +                     break;
> >> +             case DMA_SLAVE_BUSWIDTH_8_BYTES:
> >> +                     val = 3;
> > DMA_SLAVE_BUSWIDTHS are 1, 2, 4, 8...
> > so if you can do val = ffs(width) as well?
> 
> Good idea, will use __ffs(width) here.
> 
> >
> >
> >> +     case DMA_PAUSE:
> >> +             dev_dbg(d->slave.dev, "vchan %p: pause\n", &c->vc);
> >> +             if (c->status == DMA_IN_PROGRESS) {
> >> +                     c->status = DMA_PAUSED;
> >> +                     if (p) {
> >> +                             k3_dma_pause_dma(p, false);
> >> +                     } else {
> >> +                             spin_lock(&d->lock);
> >> +                             list_del_init(&c->node);
> >> +                             spin_unlock(&d->lock);
> >> +                     }
> > why do we need the else part here?
> Since asynchronous mode is supported.
> Desc is submitted to list but may not get physical channel to run.
But when you pause you pause the running channel. You dont pause a descriptor.
So whatever you are trying to imply doesnt make sense to me.

~Vinod

[PATCH v2 2/2] dmaengine: Add hisilicon k3 DMA engine driver

[zhangfei gao]

On Mon, Aug 19, 2013 at 1:35 PM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Thu, Aug 15, 2013 at 01:54:28PM +0800, zhangfei gao wrote:
>> On Tue, Aug 13, 2013 at 7:20 PM, Vinod Koul <vinod.koul@intel.com> wrote:
>> > On Fri, Jun 28, 2013 at 08:39:13PM +0800, Zhangfei Gao wrote:
>> >> Add dmaengine driver for hisilicon k3 platform based on virt_dma
>> >>
>> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>> >> Tested-by: Kai Yang <jean.yangkai@huawei.com>
>> >> Acked-by: Arnd Bergmann <arnd@arndb.de>
>> >> ---
>> >
>> >> +#define DRIVER_NAME          "k3-dma"
>> >> +#define DMA_ALIGN            3
>> >> +#define DMA_MAX_SIZE         0x1ffc
>> >> +
>> >> +#define INT_STAT             0x00
>> >> +#define INT_TC1                      0x04
>> >> +#define INT_ERR1             0x0c
>> >> +#define INT_ERR2             0x10
>> >> +#define INT_TC1_MASK         0x18
>> >> +#define INT_ERR1_MASK                0x20
>> >> +#define INT_ERR2_MASK                0x24
>> >> +#define INT_TC1_RAW          0x600
>> >> +#define INT_ERR1_RAW         0x608
>> >> +#define INT_ERR2_RAW         0x610
>> >> +#define CH_PRI                       0x688
>> >> +#define CH_STAT                      0x690
>> >> +#define CX_CUR_CNT           0x704
>> >> +#define CX_LLI                       0x800
>> >> +#define CX_CNT                       0x810
>> >> +#define CX_SRC                       0x814
>> >> +#define CX_DST                       0x818
>> >> +#define CX_CONFIG            0x81c
>> >> +#define AXI_CONFIG           0x820
>> >> +#define DEF_AXI_CONFIG               0x201201
>> >> +
>> >> +#define CX_LLI_CHAIN_EN              0x2
>> >> +#define CCFG_EN                      0x1
>> >> +#define CCFG_MEM2PER         (0x1 << 2)
>> >> +#define CCFG_PER2MEM         (0x2 << 2)
>> >> +#define CCFG_SRCINCR         (0x1 << 31)
>> >> +#define CCFG_DSTINCR         (0x1 << 30)
>> > I see these are not namespace aptly and can collide..
>> OK,
>> How about change name to
>> #define CX_CFG                 0x81c
> since you are calling your driver K3_DMA it would be apt to namespace all of the
> above with K3_INT_STAT to K3_CFG_EN and so on...

Sorry, does define of register need add name space?
Is it looks too long?

There are many example keep define simple.
drivers/dma/sa11x0-dma.c
#define DCSR_RUN        (1 << 0)
#define DCSR_IE         (1 << 1)
#define DCSR_ERROR      (1 << 2)

drivers/dma/mxs-dma.c
#define CCW_CHAIN               (1 << 2)
#define CCW_IRQ                 (1 << 3)
#define CCW_DEC_SEM             (1 << 6)

>
>>
>> #define CX_CFG_EN              0x1
>> #define CX_CFG_MEM2PER         (0x1 << 2)
>> ~

>> >
>> >> +     case DMA_PAUSE:
>> >> +             dev_dbg(d->slave.dev, "vchan %p: pause\n", &c->vc);
>> >> +             if (c->status == DMA_IN_PROGRESS) {
>> >> +                     c->status = DMA_PAUSED;
>> >> +                     if (p) {
>> >> +                             k3_dma_pause_dma(p, false);
>> >> +                     } else {
>> >> +                             spin_lock(&d->lock);
>> >> +                             list_del_init(&c->node);
>> >> +                             spin_unlock(&d->lock);
>> >> +                     }
>> > why do we need the else part here?
>> Since asynchronous mode is supported.
>> Desc is submitted to list but may not get physical channel to run.
> But when you pause you pause the running channel. You dont pause a descriptor.
> So whatever you are trying to imply doesnt make sense to me.

Here delete node from chan_pending, which will be quired from tasklet.

The physical channel is free matched.
dma_issue_pending will put node to d->k3_dma_issue_pending if no phy allocated.
Tasklet do two jobs
1, check running channel for new request form desc_issued.
2, check any new chan_pending and alloc phy if available.

If no phy, the node will be put in chan_pending.
If pause does not remove from chan_pending, it may be got from tasklet
to start a new transaction.
So it's safe to remove from chan_pending when pause, and add back when resume.

[PATCH v2 2/2] dmaengine: Add hisilicon k3 DMA engine driver

[Vinod Koul]

On Tue, Aug 20, 2013 at 03:55:05PM +0800, zhangfei gao wrote:
> There are many example keep define simple.
and there are others examples which have done the otherway. I would prefer that
way please!


> >> > why do we need the else part here?
> >> Since asynchronous mode is supported.
> >> Desc is submitted to list but may not get physical channel to run.
> > But when you pause you pause the running channel. You dont pause a descriptor.
> > So whatever you are trying to imply doesnt make sense to me.
> 
> Here delete node from chan_pending, which will be quired from tasklet.
> 
> The physical channel is free matched.
> dma_issue_pending will put node to d->k3_dma_issue_pending if no phy allocated.
> Tasklet do two jobs
> 1, check running channel for new request form desc_issued.
> 2, check any new chan_pending and alloc phy if available.
> 
> If no phy, the node will be put in chan_pending.
> If pause does not remove from chan_pending, it may be got from tasklet
> to start a new transaction.
> So it's safe to remove from chan_pending when pause, and add back when resume.
But when you add, where do you start from, from the start of the descriptor or
the previosu position.

The point is pause-resume you dont need to do all that. Channel is paused so
just pause it by stopping exuection, not more. Then you resume by asking
controller to start from where it left off.

~Vinod

[PATCH v2 2/2] dmaengine: Add hisilicon k3 DMA engine driver

[Vinod Koul]

On Tue, Aug 20, 2013 at 05:23:39PM +0800, zhangfei wrote:
> >>>>>why do we need the else part here?
> >>>>Since asynchronous mode is supported.
> >>>>Desc is submitted to list but may not get physical channel to run.
> >>>But when you pause you pause the running channel. You dont pause a descriptor.
> >>>So whatever you are trying to imply doesnt make sense to me.
> >>
> >>Here delete node from chan_pending, which will be quired from tasklet.
> >>
> >>The physical channel is free matched.
> >>dma_issue_pending will put node to d->k3_dma_issue_pending if no phy allocated.
> >>Tasklet do two jobs
> >>1, check running channel for new request form desc_issued.
> >>2, check any new chan_pending and alloc phy if available.
> >>
> >>If no phy, the node will be put in chan_pending.
> >>If pause does not remove from chan_pending, it may be got from tasklet
> >>to start a new transaction.
> >>So it's safe to remove from chan_pending when pause, and add back when resume.
> >But when you add, where do you start from, from the start of the descriptor or
> >the previosu position.
> >
> >The point is pause-resume you dont need to do all that. Channel is paused so
> >just pause it by stopping exuection, not more. Then you resume by asking
> >controller to start from where it left off.
> 
> Since it is async mode, it does not know the physical channel is
> really started or not.
> 
> When desc is submitted, it can be
> a). get phy and run, pause can stop and resume where it stops.
> b). Dot not start at all since no phy available (16 phys vs 27
> request line), if pause do nothing it will stay in chan_pending, if
> tasklet happens again, it will be fetched and started, while upper
> layer thought it is already paused.
> 
> Do we need consider case b?
and how do you handle when the desc is running on a phy.

~Vinod

[PATCH v2 2/2] dmaengine: Add hisilicon k3 DMA engine driver

[zhangfei]

On 13-08-20 04:27 PM, Vinod Koul wrote:
> On Tue, Aug 20, 2013 at 03:55:05PM +0800, zhangfei gao wrote:
>> There are many example keep define simple.
> and there are others examples which have done the otherway. I would prefer that
> way please!
>
Personally, I prefer simple is better, it is only used in the file.
But is it fine to change.


>
>>>>> why do we need the else part here?
>>>> Since asynchronous mode is supported.
>>>> Desc is submitted to list but may not get physical channel to run.
>>> But when you pause you pause the running channel. You dont pause a descriptor.
>>> So whatever you are trying to imply doesnt make sense to me.
>>
>> Here delete node from chan_pending, which will be quired from tasklet.
>>
>> The physical channel is free matched.
>> dma_issue_pending will put node to d->k3_dma_issue_pending if no phy allocated.
>> Tasklet do two jobs
>> 1, check running channel for new request form desc_issued.
>> 2, check any new chan_pending and alloc phy if available.
>>
>> If no phy, the node will be put in chan_pending.
>> If pause does not remove from chan_pending, it may be got from tasklet
>> to start a new transaction.
>> So it's safe to remove from chan_pending when pause, and add back when resume.
> But when you add, where do you start from, from the start of the descriptor or
> the previosu position.
>
> The point is pause-resume you dont need to do all that. Channel is paused so
> just pause it by stopping exuection, not more. Then you resume by asking
> controller to start from where it left off.

Since it is async mode, it does not know the physical channel is really 
started or not.

When desc is submitted, it can be
a). get phy and run, pause can stop and resume where it stops.
b). Dot not start at all since no phy available (16 phys vs 27 request 
line), if pause do nothing it will stay in chan_pending, if tasklet 
happens again, it will be fetched and started, while upper layer thought 
it is already paused.

Do we need consider case b?

[PATCH v2 2/2] dmaengine: Add hisilicon k3 DMA engine driver

[zhangfei gao]

On Tue, Aug 20, 2013 at 4:50 PM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Tue, Aug 20, 2013 at 05:23:39PM +0800, zhangfei wrote:
>> >>>>>why do we need the else part here?
>> >>>>Since asynchronous mode is supported.
>> >>>>Desc is submitted to list but may not get physical channel to run.
>> >>>But when you pause you pause the running channel. You dont pause a descriptor.
>> >>>So whatever you are trying to imply doesnt make sense to me.
>> >>
>> >>Here delete node from chan_pending, which will be quired from tasklet.
>> >>
>> >>The physical channel is free matched.
>> >>dma_issue_pending will put node to d->k3_dma_issue_pending if no phy allocated.
>> >>Tasklet do two jobs
>> >>1, check running channel for new request form desc_issued.
>> >>2, check any new chan_pending and alloc phy if available.
>> >>
>> >>If no phy, the node will be put in chan_pending.
>> >>If pause does not remove from chan_pending, it may be got from tasklet
>> >>to start a new transaction.
>> >>So it's safe to remove from chan_pending when pause, and add back when resume.
>> >But when you add, where do you start from, from the start of the descriptor or
>> >the previosu position.
>> >
>> >The point is pause-resume you dont need to do all that. Channel is paused so
>> >just pause it by stopping exuection, not more. Then you resume by asking
>> >controller to start from where it left off.
>>
>> Since it is async mode, it does not know the physical channel is
>> really started or not.
>>
>> When desc is submitted, it can be
>> a). get phy and run, pause can stop and resume where it stops.
>> b). Dot not start at all since no phy available (16 phys vs 27
>> request line), if pause do nothing it will stay in chan_pending, if
>> tasklet happens again, it will be fetched and started, while upper
>> layer thought it is already paused.
>>
>> Do we need consider case b?
> and how do you handle when the desc is running on a phy.

Currently k3dma is used with uart.

drivers/tty/serial/amba-pl011.c
pl011_dma_rx_irq: dmaengine_pause -> device_tx_status
First pause channel, then get residue.
k3_dma_tx_status:
a) if desc in issue queue, then it still not started, return total size.
b) otherwise get the residue via counting descriptor.
pl011 get the residue and recaculate the pending.

Have pass the stress test with amba-pl011.

Thanks

[PATCH v2 2/2] dmaengine: Add hisilicon k3 DMA engine driver

[Vinod Koul]

On Tue, Aug 20, 2013 at 09:36:50PM +0800, zhangfei gao wrote:
> > and how do you handle when the desc is running on a phy.
> 
> Currently k3dma is used with uart.
> 
> drivers/tty/serial/amba-pl011.c
> pl011_dma_rx_irq: dmaengine_pause -> device_tx_status
> First pause channel, then get residue.
> k3_dma_tx_status:
> a) if desc in issue queue, then it still not started, return total size.
> b) otherwise get the residue via counting descriptor.
> pl011 get the residue and recaculate the pending.
Okay thanks, While looks like for uart this is usage (note this driver doesnt not
call resume. So for that you dont need resume to be implemented.

Also, the driver will call the residue and then terminate always. So again I dont
see a point of yours with what your are doing with descriptors.

And since you support PAUSE/RESUME, if someone tries to use this with sound,
this will break as sound will call pause and then resume. No resubmitting or
terminating.

> Have pass the stress test with amba-pl011.
See the example of amba-pl08x.c, I think Linux W had this in mind when he added
this and other exapmple of PAUSE and RESUME implemented in drivers

Also, again you PAUSE a channel not a trancation, so it doesnt not really matter
where the descriptor is. In phy or somehwere else! We shouldnt care

~Vinod

[PATCH v2 2/2] dmaengine: Add hisilicon k3 DMA engine driver

[zhangfei gao]

On Wed, Aug 21, 2013 at 12:58 PM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Tue, Aug 20, 2013 at 09:36:50PM +0800, zhangfei gao wrote:
>> > and how do you handle when the desc is running on a phy.
>>
>> Currently k3dma is used with uart.
>>
>> drivers/tty/serial/amba-pl011.c
>> pl011_dma_rx_irq: dmaengine_pause -> device_tx_status
>> First pause channel, then get residue.
>> k3_dma_tx_status:
>> a) if desc in issue queue, then it still not started, return total size.
>> b) otherwise get the residue via counting descriptor.
>> pl011 get the residue and recaculate the pending.
> Okay thanks, While looks like for uart this is usage (note this driver doesnt not
> call resume. So for that you dont need resume to be implemented.
>
> Also, the driver will call the residue and then terminate always. So again I dont
> see a point of yours with what your are doing with descriptors.
>
> And since you support PAUSE/RESUME, if someone tries to use this with sound,
> this will break as sound will call pause and then resume. No resubmitting or
> terminating.

In k3v2 platform, k3dma is shared dma, used by peripherals, like uart, ssp.
While audio has special dma, can not used by others, even not use
dmaengine, which expose to system.
PAUSE & RESUME is added ONLY because we found amba-pl011 need when enable dma.

>
>> Have pass the stress test with amba-pl011.
> See the example of amba-pl08x.c, I think Linux W had this in mind when he added
> this and other exapmple of PAUSE and RESUME implemented in drivers
>
> Also, again you PAUSE a channel not a trancation, so it doesnt not really matter
> where the descriptor is. In phy or somehwere else! We shouldnt care

Excuse me for my bad English.
I am really lost and don't know what to do now.
Are you concern:
1. Why need else in case DMA_PAUSE, no need?
2. device_tx_status is not called in audio and directly
dmaengine_pause -> dmaengine_resume?
3. ?

Besides, where I should take example of amba-pl08x.c, pl08x_control,
case DMA_PAUSE & DMA_RESUME or pl08x_dma_tx_status?

Some difference I see is k3dma.c only has one tasklet for all channels,
The tasklet will check pending channel and alloc physical channel.
While the issue_pending does not really alloc physical channel to
support more request then physical channel.
So when suspend we have to consider remove the channel from chan_pending.

When trying use virt-dma, have study drivers/dma/sa11x0-dma.c as good reference.

Sorry for my bad understanding.

[PATCH v2 2/2] dmaengine: Add hisilicon k3 DMA engine driver

[zhangfei gao]

On Wed, Aug 21, 2013 at 12:58 PM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Tue, Aug 20, 2013 at 09:36:50PM +0800, zhangfei gao wrote:
>> > and how do you handle when the desc is running on a phy.
>>
>> Currently k3dma is used with uart.
>>
>> drivers/tty/serial/amba-pl011.c
>> pl011_dma_rx_irq: dmaengine_pause -> device_tx_status
>> First pause channel, then get residue.
>> k3_dma_tx_status:
>> a) if desc in issue queue, then it still not started, return total size.
>> b) otherwise get the residue via counting descriptor.
>> pl011 get the residue and recaculate the pending.
> Okay thanks, While looks like for uart this is usage (note this driver doesnt not
> call resume. So for that you dont need resume to be implemented.
>
> Also, the driver will call the residue and then terminate always. So again I dont
> see a point of yours with what your are doing with descriptors.
>
> And since you support PAUSE/RESUME, if someone tries to use this with sound,
> this will break as sound will call pause and then resume. No resubmitting or
> terminating.
>
>> Have pass the stress test with amba-pl011.
> See the example of amba-pl08x.c, I think Linux W had this in mind when he added
> this and other exapmple of PAUSE and RESUME implemented in drivers
>
> Also, again you PAUSE a channel not a trancation, so it doesnt not really matter
> where the descriptor is. In phy or somehwere else! We shouldnt care
>
> ~Vinod

Dear Vinod

I think your concern is device_tx_status does not return right pos.

prep_slave_sg -> vchan_tx_prep -> vd->tx.tx_submit = vchan_tx_submit;
vchan_tx_submit -> cookie = dma_cookie_assign(tx)
So one cookie is assigned for one tx_submit

sound/soc/soc-dmaengine-pcm.c:
dmaengine_pcm_prepare_and_submit -> dmaengine_prep_dma_cyclic ->
prtd->cookie = dmaengine_submit
The cookie is get for one pare prepare and submit.

k3_dma_tx_status:
If cookie is in the issue_queue, means sg list not send out, return
total size of the prepare, including all descriptor.
If the sg list already run, the count descriptor and return the left value.
Since only one cookie match all descriptor, or descriptor is organized
inside, transparent to upper layer, return total left bytes.

Is this fine?

Thanks

[PATCH v2 2/2] dmaengine: Add hisilicon k3 DMA engine driver

[Vinod Koul]

On Wed, Aug 21, 2013 at 04:02:03PM +0800, zhangfei gao wrote:
> Excuse me for my bad English.
> I am really lost and don't know what to do now.
> Are you concern:
> 1. Why need else in case DMA_PAUSE, no need?
> 2. device_tx_status is not called in audio and directly
> dmaengine_pause -> dmaengine_resume?
> 3. ?
> 
> Besides, where I should take example of amba-pl08x.c, pl08x_control,
> case DMA_PAUSE & DMA_RESUME or pl08x_dma_tx_status?
> 
> Some difference I see is k3dma.c only has one tasklet for all channels,
> The tasklet will check pending channel and alloc physical channel.
> While the issue_pending does not really alloc physical channel to
> support more request then physical channel.
> So when suspend we have to consider remove the channel from chan_pending.
> 
> When trying use virt-dma, have study drivers/dma/sa11x0-dma.c as good reference.
> 
> Sorry for my bad understanding.
No issues...

I think i am okay with else part looking at the driver again.

Can you resend by rebasing on my next and fxing any other issues

~Vinod
--