Add dma support for i.MX23/28

Add dma support for i.MX23/28

[Shawn Guo]

This patch set is to add dma support for i.MX23/28, including
apbh-dma and apbx-dma.  The apbh-dma was tested with mxs-mmc driver
on mx23evk and mx28evk boards.  And mxs-mmc driver will be posted
for review later.

It's based on Sascha's imx-for-2.6.39 tree since commit:

    22cbba1b82de458028f4aa270e88492b622c1ea8
    ARM: mxs: dynamically register flexcan devices for mx28

Thanks for review.

Regards,
Shawn

Shawn Guo (5):
 [PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28
 [PATCH 2/5] ARM: mxs: add dma channel definitions
 [PATCH 3/5] ARM: mxs: dynamically allocate dma device for mx23/28
 [PATCH 4/5] ARM: mxs/mx23evk: add dma device
 [PATCH 5/5] ARM: mxs/mx28evk: add dma device

 arch/arm/mach-mxs/Kconfig                       |    2 +
 arch/arm/mach-mxs/clock-mx23.c                  |    3 +-
 arch/arm/mach-mxs/clock-mx28.c                  |    4 +-
 arch/arm/mach-mxs/devices-mx23.h                |    6 +
 arch/arm/mach-mxs/devices-mx28.h                |    6 +
 arch/arm/mach-mxs/devices/Kconfig               |    3 +
 arch/arm/mach-mxs/devices/Makefile              |    1 +
 arch/arm/mach-mxs/devices/platform-dma.c        |   50 ++
 arch/arm/mach-mxs/include/mach/devices-common.h |    7 +
 arch/arm/mach-mxs/include/mach/dma.h            |   16 +
 arch/arm/mach-mxs/include/mach/mx23.h           |   24 +
 arch/arm/mach-mxs/include/mach/mx28.h           |   37 ++
 arch/arm/mach-mxs/mach-mx23evk.c                |    8 +
 arch/arm/mach-mxs/mach-mx28evk.c                |    8 +
 drivers/dma/Kconfig                             |    8 +
 drivers/dma/Makefile                            |    1 +
 drivers/dma/mxs-dma.c                           |  702 +++++++++++++++++++++++
 17 files changed, 883 insertions(+), 3 deletions(-)

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Shawn Guo]

This patch adds dma support for Freescale MXS-based SoC i.MX23/28,
including apbh-dma and apbx-dma.

* apbh-dma and apbx-dma are supported in the driver as two instances,
  and have to be filtered by dma clients via device id.  It becomes
  the convention that apbh-dma always gets registered prior to
  apbx-dma.

* apbh-dma is different between mx23 and mx28, hardware version
  register is used to handle the differences.

* Every the mxs dma channel is statically assigned to client device
  by soc design with fixed irq.  The irq number is being passed by
  alloc_chan function with mxs_dma_data, and client driver has to
  filter the correct channel by its channel id.

* mxs-dma supports pio function besides data transfer.  The driver
  uses dma_data_direction DMA_NONE to identify the pio mode, and
  steals sgl and sg_len to get pio words and numbers from clients.

* mxs dmaengine has some very specific features, like sense function
  and the special NAND support (nand_lock, nand_wait4ready).  These
  are too specific to implemented in generic dmaengine driver.

* The parameter "flags" of prep functions is currently being used to
  pass wait4end flag from clients.

* The driver refers to imx-sdma and only a single descriptor is
  statically assigned to each channel.

Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
---
 arch/arm/mach-mxs/include/mach/dma.h |   16 +
 drivers/dma/Kconfig                  |    8 +
 drivers/dma/Makefile                 |    1 +
 drivers/dma/mxs-dma.c                |  702 ++++++++++++++++++++++++++++++++++
 4 files changed, 727 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-mxs/include/mach/dma.h
 create mode 100644 drivers/dma/mxs-dma.c

diff --git a/arch/arm/mach-mxs/include/mach/dma.h b/arch/arm/mach-mxs/include/mach/dma.h
new file mode 100644
index 0000000..429f431
--- /dev/null
+++ b/arch/arm/mach-mxs/include/mach/dma.h
@@ -0,0 +1,16 @@
+/*
+ * Copyright 2011 Freescale Semiconductor, Inc. All Rights Reserved.
+ *
+ * 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.
+ */
+
+#ifndef __MACH_MXS_DMA_H__
+#define __MACH_MXS_DMA_H__
+
+struct mxs_dma_data {
+	int chan_irq;
+};
+
+#endif /* __MACH_MXS_DMA_H__ */
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 1c28816..0b9a5b1 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -227,6 +227,14 @@ config IMX_DMA
 	  Support the i.MX DMA engine. This engine is integrated into
 	  Freescale i.MX1/21/27 chips.
 
+config MXS_DMA
+	tristate "MXS DMA support"
+	depends on SOC_IMX23 || SOC_MX28
+	select DMA_ENGINE
+	help
+	  Support the MXS DMA engine. This engine including APBH-DMA
+	  and APBX-DMA is integrated into Freescale i.MX23/28 chips.
+
 config DMA_ENGINE
 	bool
 
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index 64b21f5..802b557 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_COH901318) += coh901318.o coh901318_lli.o
 obj-$(CONFIG_AMCC_PPC440SPE_ADMA) += ppc4xx/
 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_STE_DMA40) += ste_dma40.o ste_dma40_ll.o
 obj-$(CONFIG_PL330_DMA) += pl330.o
diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
new file mode 100644
index 0000000..8d1e811
--- /dev/null
+++ b/drivers/dma/mxs-dma.c
@@ -0,0 +1,702 @@
+/*
+ * Copyright 2011 Freescale Semiconductor, Inc. All Rights Reserved.
+ *
+ * Refer to drivers/dma/imx-sdma.c
+ *
+ * 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/init.h>
+#include <linux/types.h>
+#include <linux/mm.h>
+#include <linux/interrupt.h>
+#include <linux/clk.h>
+#include <linux/wait.h>
+#include <linux/sched.h>
+#include <linux/semaphore.h>
+#include <linux/spinlock.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/dmaengine.h>
+#include <linux/delay.h>
+
+#include <asm/irq.h>
+#include <mach/mxs.h>
+#include <mach/dma.h>
+#include <mach/common.h>
+
+#define MXS_DMA_APBH		0
+#define MXS_DMA_APBX		1
+#define dma_is_apbh()		(mxs_dma->dev_id == MXS_DMA_APBH)
+
+#define APBH_VERSION_LATEST	3
+#define apbh_is_old()		(mxs_dma->version < APBH_VERSION_LATEST)
+
+#define HW_APBHX_CTRL0				0x000
+#define  BM_APBH_CTRL0_APB_BURST8_EN		(1 << 29)
+#define  BM_APBH_CTRL0_APB_BURST_EN		(1 << 28)
+#define  BP_APBH_CTRL0_CLKGATE_CHANNEL		(8)
+#define  BP_APBH_CTRL0_RESET_CHANNEL		(16)
+#define HW_APBHX_CTRL1				0x010
+#define HW_APBHX_CTRL2				0x020
+#define HW_APBHX_CHANNEL_CTRL			0x030
+#define  BP_APBHX_CHANNEL_CTRL_RESET_CHANNEL	(16)
+#define HW_APBH_VERSION				(cpu_is_mx23() ? 0x3f0 : 0x800)
+#define HW_APBX_VERSION				0x800
+#define  BP_APBHX_VERSION_MAJOR			(24)
+#define HW_APBHX_CHn_NXTCMDAR(n) \
+	(((dma_is_apbh() && apbh_is_old()) ? 0x050 : 0x110) + (n) * 0x70)
+#define HW_APBHX_CHn_SEMA(n) \
+	(((dma_is_apbh() && apbh_is_old()) ? 0x080 : 0x140) + (n) * 0x70)
+
+struct mxs_dma_ccw_bits {
+	unsigned int	command:2;
+#define MXS_DMA_NO_XFER	0x00
+#define MXS_DMA_WRITE	0x01
+#define MXS_DMA_READ	0x02
+#define MXS_DMA_SENSE	0x03			/* not implemented */
+	unsigned int	chain:1;
+	unsigned int	irq:1;
+	unsigned int	nand_lock:1;		/* not implemented */
+	unsigned int	nand_wait4ready:1;	/* not implemented */
+	unsigned int	dec_sem:1;
+	unsigned int	wait4end:1;
+	unsigned int	halt_on_terminate:1;
+	unsigned int	terminate_flush:1;
+	unsigned int	reserved:2;
+	unsigned int	pio_num:4;
+	unsigned int	xfer_bytes:16;
+#define MAX_XFER_BYTES	0xffff
+};
+
+struct mxs_dma_ccw {
+	u32				next;
+	struct mxs_dma_ccw_bits		bits;
+	dma_addr_t bufaddr;
+#define MXS_PIO_WORDS			16
+	u32 pio_words[MXS_PIO_WORDS];
+};
+
+#define NUM_CCW	(int)(PAGE_SIZE / sizeof(struct mxs_dma_ccw))
+
+struct mxs_dma_chan {
+	struct mxs_dma_engine		*mxs_dma;
+	struct dma_chan			chan;
+	struct dma_async_tx_descriptor	desc;
+	spinlock_t			lock;
+	int				chan_irq;
+	struct mxs_dma_ccw		*ccw;
+	dma_addr_t			ccw_phys;
+	dma_cookie_t			last_completed;
+	enum dma_status			status;
+	unsigned int			flags;
+#define MXS_DMA_SG_LOOP			(1 << 0)
+};
+
+struct mxs_dma_engine {
+	struct device			*dev;
+	int				dev_id;
+	unsigned int			version;
+	void __iomem			*base;
+	struct clk			*clk;
+	struct dma_device		dma_device;
+	struct device_dma_parameters 	dma_parms;
+#define MXS_DMA_CHANNELS		16
+	struct mxs_dma_chan		mxs_chans[MXS_DMA_CHANNELS];
+};
+
+static void mxs_dma_reset_chan(struct mxs_dma_chan *mxs_chan)
+{
+	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
+	int chan_id = mxs_chan->chan.chan_id;
+
+	if (dma_is_apbh() && apbh_is_old())
+		__mxs_setl(1 << (chan_id + BP_APBH_CTRL0_RESET_CHANNEL),
+				mxs_dma->base + HW_APBHX_CTRL0);
+	else
+		__mxs_setl(1 << (chan_id + BP_APBHX_CHANNEL_CTRL_RESET_CHANNEL),
+				mxs_dma->base + HW_APBHX_CHANNEL_CTRL);
+}
+
+static void mxs_dma_enable_chan(struct mxs_dma_chan *mxs_chan)
+{
+	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
+	int chan_id = mxs_chan->chan.chan_id;
+
+	/* set cmd_addr up */
+	__raw_writel(mxs_chan->ccw_phys,
+			mxs_dma->base + HW_APBHX_CHn_NXTCMDAR(chan_id));
+
+	/* enable apbh channel clock */
+	if (dma_is_apbh()) {
+		if (apbh_is_old())
+			__mxs_clrl(1 << (chan_id + BP_APBH_CTRL0_CLKGATE_CHANNEL),
+						mxs_dma->base + HW_APBHX_CTRL0);
+		else
+			__mxs_clrl(1 << chan_id, mxs_dma->base + HW_APBHX_CTRL0);
+	}
+
+	/* write 1 to SEMA to kick off the channel */
+	__raw_writel(1, mxs_dma->base + HW_APBHX_CHn_SEMA(chan_id));
+}
+
+static void mxs_dma_disable_chan(struct mxs_dma_chan *mxs_chan)
+{
+	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
+	int chan_id = mxs_chan->chan.chan_id;
+
+	/* disable apbh channel clock */
+	if (dma_is_apbh()) {
+		if (apbh_is_old())
+			__mxs_setl(1 << (chan_id + BP_APBH_CTRL0_CLKGATE_CHANNEL),
+						mxs_dma->base + HW_APBHX_CTRL0);
+		else
+			__mxs_setl(1 << chan_id, mxs_dma->base + HW_APBHX_CTRL0);
+	}
+
+	mxs_chan->status = DMA_SUCCESS;
+}
+
+static void mxs_dma_pause_chan(struct mxs_dma_chan *mxs_chan)
+{
+	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
+	int chan_id = mxs_chan->chan.chan_id;
+
+	/* freeze the channel */
+	if (dma_is_apbh() && apbh_is_old())
+		__mxs_setl(1 << chan_id, mxs_dma->base + HW_APBHX_CTRL0);
+	else
+		__mxs_setl(1 << chan_id, mxs_dma->base + HW_APBHX_CHANNEL_CTRL);
+
+	mxs_chan->status = DMA_PAUSED;
+}
+
+static void mxs_dma_resume_chan(struct mxs_dma_chan *mxs_chan)
+{
+	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
+	int chan_id = mxs_chan->chan.chan_id;
+
+	/* unfreeze the channel */
+	if (dma_is_apbh() && apbh_is_old())
+		__mxs_clrl(1 << chan_id, mxs_dma->base + HW_APBHX_CTRL0);
+	else
+		__mxs_clrl(1 << chan_id, mxs_dma->base + HW_APBHX_CHANNEL_CTRL);
+
+	mxs_chan->status = DMA_IN_PROGRESS;
+}
+
+static dma_cookie_t mxs_dma_assign_cookie(struct mxs_dma_chan *mxs_chan)
+{
+	dma_cookie_t cookie = mxs_chan->chan.cookie;
+
+	if (++cookie < 0)
+		cookie = 1;
+
+	mxs_chan->chan.cookie = cookie;
+	mxs_chan->desc.cookie = cookie;
+
+	return cookie;
+}
+
+static struct mxs_dma_chan *to_mxs_dma_chan(struct dma_chan *chan)
+{
+	return container_of(chan, struct mxs_dma_chan, chan);
+}
+
+static dma_cookie_t mxs_dma_tx_submit(struct dma_async_tx_descriptor *tx)
+{
+	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(tx->chan);
+	dma_cookie_t cookie;
+
+	spin_lock_irq(&mxs_chan->lock);
+
+	cookie = mxs_dma_assign_cookie(mxs_chan);
+
+	mxs_dma_enable_chan(mxs_chan);
+
+	spin_unlock_irq(&mxs_chan->lock);
+
+	return cookie;
+}
+
+static irqreturn_t mxs_dma_int_handler(int irq, void *dev_id)
+{
+	struct mxs_dma_engine *mxs_dma = dev_id;
+	u32 stat1, stat2;
+
+	/* completion status */
+	stat1 = __raw_readl(mxs_dma->base + HW_APBHX_CTRL1);
+	stat1 &= 0xffff;
+	__mxs_clrl(stat1, mxs_dma->base + HW_APBHX_CTRL1);
+
+	/* error status */
+	stat2 = __raw_readl(mxs_dma->base + HW_APBHX_CTRL2);
+	__mxs_clrl(stat2, mxs_dma->base + HW_APBHX_CTRL2);
+
+	/*
+	 * When both completion and error of termination bits set at the
+	 * same time, we do not take it as an error.  IOW, it only becomes
+	 * an error we need to handler here in case of ether it's an bus
+	 * error or a termination error with no completion.
+	 */
+	stat2 = ((stat2 >> 16) & stat2) |	   /* bus error */
+		(~(stat2 >> 16) & stat2 & ~stat1); /* termination with no completion */
+
+	/* combine error and completion status for checking */
+	stat1 = (stat2 << 16) | stat1;
+	while (stat1) {
+		int channel = fls(stat1) - 1;
+		struct mxs_dma_chan *mxs_chan =
+				&mxs_dma->mxs_chans[channel % 16];
+
+		if (channel >= 16) {
+			dev_dbg(mxs_dma->dev, "%s: error in channel %d\n",
+						__func__, channel - 16);
+			mxs_dma_reset_chan(mxs_chan);
+			mxs_chan->status = DMA_ERROR;
+		} else {
+			if (mxs_chan->flags & MXS_DMA_SG_LOOP)
+				mxs_chan->status = DMA_IN_PROGRESS;
+			else
+				mxs_chan->status = DMA_SUCCESS;
+		}
+
+		stat1 &= ~(1 << channel);
+
+		if (mxs_chan->desc.callback)
+			mxs_chan->desc.callback(mxs_chan->desc.callback_param);
+
+		if (mxs_chan->status == DMA_SUCCESS)
+			mxs_chan->last_completed = mxs_chan->desc.cookie;
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
+{
+	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
+	struct mxs_dma_data *data = chan->private;
+	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
+	static unsigned long flags;
+	int ret;
+
+	if (!data)
+		return -EINVAL;
+
+	mxs_chan->chan_irq = data->chan_irq;
+
+	mxs_chan->ccw = dma_alloc_coherent(NULL, PAGE_SIZE,
+				&mxs_chan->ccw_phys, GFP_KERNEL);
+	if (!mxs_chan->ccw) {
+		ret = -ENOMEM;
+		goto err_alloc;
+	}
+
+	memset(mxs_chan->ccw, 0, PAGE_SIZE);
+
+	ret = request_irq(mxs_chan->chan_irq, mxs_dma_int_handler,
+				flags, "mxs-dma", mxs_dma);
+	if (ret)
+		goto err_irq;
+
+	flags = IRQF_SHARED;
+
+	clk_enable(mxs_dma->clk);
+
+	mxs_dma_reset_chan(mxs_chan);
+
+	dma_async_tx_descriptor_init(&mxs_chan->desc, chan);
+	mxs_chan->desc.tx_submit = mxs_dma_tx_submit;
+
+	/* the descriptor is ready */
+	async_tx_ack(&mxs_chan->desc);
+
+	return 0;
+
+err_irq:
+	dma_free_coherent(NULL, PAGE_SIZE, mxs_chan->ccw, mxs_chan->ccw_phys);
+err_alloc:
+	return ret;
+}
+
+static void mxs_dma_free_chan_resources(struct dma_chan *chan)
+{
+	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
+	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
+
+	mxs_dma_disable_chan(mxs_chan);
+
+	free_irq(mxs_chan->chan_irq, mxs_dma);
+
+	dma_free_coherent(NULL, PAGE_SIZE, mxs_chan->ccw, mxs_chan->ccw_phys);
+
+	clk_disable(mxs_dma->clk);
+}
+
+static struct dma_async_tx_descriptor *mxs_dma_prep_slave_sg(
+		struct dma_chan *chan, struct scatterlist *sgl,
+		unsigned int sg_len, enum dma_data_direction direction,
+		unsigned long flags)
+{
+	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
+	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
+	struct mxs_dma_ccw *ccw;
+	struct scatterlist *sg;
+	int ret, i, j;
+	u32 *pio;
+
+	dev_dbg(mxs_dma->dev, "%s: channel %d\n", __func__, chan->chan_id);
+
+	if (mxs_chan->status == DMA_IN_PROGRESS)
+		return NULL;
+
+	mxs_chan->status = DMA_IN_PROGRESS;
+	mxs_chan->flags = 0;
+
+	dev_dbg(mxs_dma->dev, "%s: setting up %d entries\n", __func__, sg_len);
+
+	if (sg_len > ((direction == DMA_NONE) ? MXS_PIO_WORDS : NUM_CCW)) {
+		dev_err(mxs_dma->dev, "maximum number of sg exceeded: %d > %d\n",
+				sg_len, NUM_CCW);
+		ret = -EINVAL;
+		goto err_out;
+	}
+
+	if (direction == DMA_NONE) {
+		ccw = &mxs_chan->ccw[0];
+		pio = (u32 *) sgl;
+
+		for (j = 0; j < sg_len;)
+			ccw->pio_words[j++] = *pio++;
+
+		ccw->next = 0;
+		ccw->bits.chain = 0;
+		ccw->bits.irq = 1;
+		ccw->bits.dec_sem = 1;
+		ccw->bits.wait4end = flags;
+		ccw->bits.halt_on_terminate = 1;
+		ccw->bits.terminate_flush = 1;
+		ccw->bits.pio_num = sg_len;
+		ccw->bits.command = MXS_DMA_NO_XFER;
+	} else {
+		for_each_sg(sgl, sg, sg_len, i) {
+			if (sg->length > MAX_XFER_BYTES) {
+				dev_err(mxs_dma->dev, "maximum bytes for sg entry exceeded: %d > %d\n",
+						sg->length, MAX_XFER_BYTES);
+				ret = -EINVAL;
+				goto err_out;
+			}
+
+			ccw = &mxs_chan->ccw[i];
+
+			ccw->next = mxs_chan->ccw_phys + sizeof(*ccw) * (i + 1);
+			ccw->bufaddr = sg->dma_address;
+			ccw->bits.xfer_bytes = sg->length;
+			ccw->bits.chain = 1;
+			ccw->bits.irq = 0;
+			ccw->bits.dec_sem = 0;
+			ccw->bits.wait4end = 0;
+			ccw->bits.halt_on_terminate = 1;
+			ccw->bits.terminate_flush = 1;
+			ccw->bits.pio_num = 0;
+			ccw->bits.command = (direction == DMA_FROM_DEVICE) ?
+						MXS_DMA_WRITE : MXS_DMA_READ;
+
+			if (i + 1 == sg_len) {
+				ccw->next = 0;
+				ccw->bits.chain = 0;
+				ccw->bits.irq = 1;
+				ccw->bits.dec_sem = 1;
+				ccw->bits.wait4end = 1;
+			}
+		}
+	}
+
+	return &mxs_chan->desc;
+
+err_out:
+	mxs_chan->status = DMA_ERROR;
+	return NULL;
+}
+
+static struct dma_async_tx_descriptor *mxs_dma_prep_dma_cyclic(
+		struct dma_chan *chan, dma_addr_t dma_addr, size_t buf_len,
+		size_t period_len, enum dma_data_direction direction)
+{
+	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
+	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
+	int num_periods = buf_len / period_len;
+	int i = 0, buf = 0;
+
+	dev_dbg(mxs_dma->dev, "%s: channel %d\n", __func__, chan->chan_id);
+
+	if (mxs_chan->status == DMA_IN_PROGRESS)
+		return NULL;
+
+	mxs_chan->status = DMA_IN_PROGRESS;
+	mxs_chan->flags |= MXS_DMA_SG_LOOP;
+
+	if (num_periods > NUM_CCW) {
+		dev_err(mxs_dma->dev, "maximum number of sg exceeded: %d > %d\n",
+				num_periods, NUM_CCW);
+		goto err_out;
+	}
+
+	if (period_len > MAX_XFER_BYTES) {
+		dev_err(mxs_dma->dev, "maximum period size exceeded: %d > %d\n",
+				period_len, MAX_XFER_BYTES);
+		goto err_out;
+	}
+
+	while (buf < buf_len) {
+		struct mxs_dma_ccw *ccw = &mxs_chan->ccw[i];
+
+		if (i + 1 == num_periods)
+			ccw->next = mxs_chan->ccw_phys;
+		else
+			ccw->next = mxs_chan->ccw_phys + sizeof(*ccw) * (i + 1);
+
+		ccw->bufaddr = dma_addr;
+		ccw->bits.xfer_bytes = period_len;
+		ccw->bits.chain = 1;
+		ccw->bits.irq = 1;
+		ccw->bits.dec_sem = 0;
+		ccw->bits.wait4end = 0;
+		ccw->bits.halt_on_terminate = 1;
+		ccw->bits.terminate_flush = 1;
+		ccw->bits.pio_num = 0;
+		ccw->bits.command = (direction == DMA_FROM_DEVICE) ?
+					MXS_DMA_WRITE : MXS_DMA_READ;
+
+		dma_addr += period_len;
+		buf += period_len;
+
+		i++;
+	}
+
+	return &mxs_chan->desc;
+
+err_out:
+	mxs_chan->status = DMA_ERROR;
+	return NULL;
+}
+
+static int mxs_dma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
+		unsigned long arg)
+{
+	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
+
+	switch (cmd) {
+	case DMA_TERMINATE_ALL:
+		mxs_dma_disable_chan(mxs_chan);
+		return 0;
+	case DMA_PAUSE:
+		mxs_dma_pause_chan(mxs_chan);
+		return 0;
+	case DMA_RESUME:
+		mxs_dma_resume_chan(mxs_chan);
+		return 0;
+	default:
+		return -ENOSYS;
+	}
+
+	return -EINVAL;
+}
+
+static enum dma_status mxs_dma_tx_status(struct dma_chan *chan,
+			dma_cookie_t cookie, struct dma_tx_state *txstate)
+{
+	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
+	dma_cookie_t last_used;
+	enum dma_status ret;
+
+	last_used = chan->cookie;
+
+	ret = dma_async_is_complete(cookie, mxs_chan->last_completed, last_used);
+	dma_set_tx_state(txstate, mxs_chan->last_completed, last_used, 0);
+
+	return ret;
+}
+
+static void mxs_dma_issue_pending(struct dma_chan *chan)
+{
+	/*
+	 * Nothing to do. We only have a single descriptor.
+	 */
+}
+
+static int __init mxs_dma_init(struct mxs_dma_engine *mxs_dma)
+{
+	int ret;
+
+	ret = clk_enable(mxs_dma->clk);
+	if (ret)
+		goto err_out;
+
+	ret = mxs_reset_block(mxs_dma->base);
+	if (ret)
+		goto err_out;
+
+	/* only major version matters */
+	mxs_dma->version = __raw_readl(mxs_dma->base +
+				((mxs_dma->dev_id == MXS_DMA_APBX) ?
+				HW_APBX_VERSION : HW_APBH_VERSION)) >>
+				BP_APBHX_VERSION_MAJOR;
+
+	/* enable apbh burst */
+	if (dma_is_apbh()) {
+		__mxs_setl(BM_APBH_CTRL0_APB_BURST_EN,
+				mxs_dma->base + HW_APBHX_CTRL0);
+		__mxs_setl(BM_APBH_CTRL0_APB_BURST8_EN,
+				mxs_dma->base + HW_APBHX_CTRL0);
+	}
+
+	/* enable irq for all the channels */
+	__mxs_setl(0xffff << 16, mxs_dma->base + HW_APBHX_CTRL1);
+
+	clk_disable(mxs_dma->clk);
+
+	return 0;
+
+err_out:
+	return ret;
+}
+
+static int __init mxs_dma_probe(struct platform_device *pdev)
+{
+	const struct platform_device_id *id_entry =
+				platform_get_device_id(pdev);
+	struct mxs_dma_engine *mxs_dma;
+	struct resource *iores;
+	int ret, i;
+
+	mxs_dma = kzalloc(sizeof(*mxs_dma), GFP_KERNEL);
+	if (!mxs_dma)
+		return -ENOMEM;
+
+	mxs_dma->dev = &pdev->dev;
+	mxs_dma->dev_id = id_entry->driver_data;
+
+	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	if (!request_mem_region(iores->start, resource_size(iores),
+				pdev->name)) {
+		ret = -EBUSY;
+		goto err_request_region;
+	}
+
+	mxs_dma->base = ioremap(iores->start, resource_size(iores));
+	if (!mxs_dma->base) {
+		ret = -ENOMEM;
+		goto err_ioremap;
+	}
+
+	mxs_dma->clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(mxs_dma->clk)) {
+		ret = PTR_ERR(mxs_dma->clk);
+		goto err_clk;
+	}
+
+	dma_cap_set(DMA_SLAVE, mxs_dma->dma_device.cap_mask);
+	dma_cap_set(DMA_CYCLIC, mxs_dma->dma_device.cap_mask);
+
+	INIT_LIST_HEAD(&mxs_dma->dma_device.channels);
+
+	/* Initialize channel parameters */
+	for (i = 0; i < MXS_DMA_CHANNELS; i++) {
+		struct mxs_dma_chan *mxs_chan = &mxs_dma->mxs_chans[i];
+
+		mxs_chan->mxs_dma = mxs_dma;
+		spin_lock_init(&mxs_chan->lock);
+
+		mxs_chan->chan.device = &mxs_dma->dma_device;
+
+		/* Add the channel to mxs_chan list */
+		list_add_tail(&mxs_chan->chan.device_node, &mxs_dma->dma_device.channels);
+	}
+
+	ret = mxs_dma_init(mxs_dma);
+	if (ret)
+		goto err_init;
+
+	mxs_dma->dma_device.dev = &pdev->dev;
+
+	/* mxs_dma gets 65535 bytes maximum sg size */
+	mxs_dma->dma_device.dev->dma_parms = &mxs_dma->dma_parms;
+	dma_set_max_seg_size(mxs_dma->dma_device.dev, MAX_XFER_BYTES);
+
+	mxs_dma->dma_device.device_alloc_chan_resources = mxs_dma_alloc_chan_resources;
+	mxs_dma->dma_device.device_free_chan_resources = mxs_dma_free_chan_resources;
+	mxs_dma->dma_device.device_tx_status = mxs_dma_tx_status;
+	mxs_dma->dma_device.device_prep_slave_sg = mxs_dma_prep_slave_sg;
+	mxs_dma->dma_device.device_prep_dma_cyclic = mxs_dma_prep_dma_cyclic;
+	mxs_dma->dma_device.device_control = mxs_dma_control;
+	mxs_dma->dma_device.device_issue_pending = mxs_dma_issue_pending;
+
+	ret = dma_async_device_register(&mxs_dma->dma_device);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to register\n");
+		goto err_dma_register;
+	}
+
+	/*
+	 * dmaengine clients have to use dma_device.dev_id to filter
+	 * dma device between apbh and apbx, so need to ensure it is
+	 * identical to mxs_dma_engine.dev_id.
+	 */
+	if (mxs_dma->dma_device.dev_id != mxs_dma->dev_id) {
+		dev_err(&pdev->dev, "dev_id of dma_device %d differs from mxs_dma_engine %d\n",
+				mxs_dma->dma_device.dev_id, mxs_dma->dev_id);
+		goto err_init;
+	}
+
+	dev_info(mxs_dma->dev, "initialized\n");
+
+	return 0;
+
+err_init:
+	dma_async_device_unregister(&mxs_dma->dma_device);
+err_dma_register:
+	clk_put(mxs_dma->clk);
+err_clk:
+	iounmap(mxs_dma->base);
+err_ioremap:
+	release_mem_region(iores->start, resource_size(iores));
+err_request_region:
+	kfree(mxs_dma);
+	return ret;
+}
+
+static int __exit mxs_dma_remove(struct platform_device *pdev)
+{
+	return -EBUSY;
+}
+
+static struct platform_device_id mxs_dma_type[] = {
+	{
+		.name = "mxs-dma-apbh",
+		.driver_data = MXS_DMA_APBH,
+	}, {
+		.name = "mxs-dma-apbx",
+		.driver_data = MXS_DMA_APBX,
+	}
+};
+
+static struct platform_driver mxs_dma_driver = {
+	.driver		= {
+		.name	= "mxs-dma",
+	},
+	.id_table	= mxs_dma_type,
+	.remove		= __exit_p(mxs_dma_remove),
+};
+
+static int __init mxs_dma_module_init(void)
+{
+	return platform_driver_probe(&mxs_dma_driver, mxs_dma_probe);
+}
+subsys_initcall(mxs_dma_module_init);
-- 
1.7.1

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Russell King - ARM Linux]

On Sat, Feb 05, 2011 at 10:08:12AM +0800, Shawn Guo wrote:
> +struct mxs_dma_ccw_bits {
> +	unsigned int	command:2;
> +#define MXS_DMA_NO_XFER	0x00
> +#define MXS_DMA_WRITE	0x01
> +#define MXS_DMA_READ	0x02
> +#define MXS_DMA_SENSE	0x03			/* not implemented */
> +	unsigned int	chain:1;
> +	unsigned int	irq:1;
> +	unsigned int	nand_lock:1;		/* not implemented */
> +	unsigned int	nand_wait4ready:1;	/* not implemented */
> +	unsigned int	dec_sem:1;
> +	unsigned int	wait4end:1;
> +	unsigned int	halt_on_terminate:1;
> +	unsigned int	terminate_flush:1;
> +	unsigned int	reserved:2;
> +	unsigned int	pio_num:4;
> +	unsigned int	xfer_bytes:16;
> +#define MAX_XFER_BYTES	0xffff
> +};

Bitfields are subject to endianness issues.  Are you sure this is a good
idea?

> +static dma_cookie_t mxs_dma_tx_submit(struct dma_async_tx_descriptor *tx)
> +{
> +	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(tx->chan);
> +	dma_cookie_t cookie;
> +
> +	spin_lock_irq(&mxs_chan->lock);
> +
> +	cookie = mxs_dma_assign_cookie(mxs_chan);
> +
> +	mxs_dma_enable_chan(mxs_chan);
> +
> +	spin_unlock_irq(&mxs_chan->lock);

Do you know that you'll always be called from an IRQs-enabled context?
If not, you should use the irqsave/irqrestore versions.

> +
> +	return cookie;
> +}
> +
> +static irqreturn_t mxs_dma_int_handler(int irq, void *dev_id)
> +{
> +	struct mxs_dma_engine *mxs_dma = dev_id;
> +	u32 stat1, stat2;
> +
> +	/* completion status */
> +	stat1 = __raw_readl(mxs_dma->base + HW_APBHX_CTRL1);
> +	stat1 &= 0xffff;
> +	__mxs_clrl(stat1, mxs_dma->base + HW_APBHX_CTRL1);
> +
> +	/* error status */
> +	stat2 = __raw_readl(mxs_dma->base + HW_APBHX_CTRL2);
> +	__mxs_clrl(stat2, mxs_dma->base + HW_APBHX_CTRL2);

Would it be useful to report error status when it happens?

> +
> +	/*
> +	 * When both completion and error of termination bits set at the
> +	 * same time, we do not take it as an error.  IOW, it only becomes
> +	 * an error we need to handler here in case of ether it's an bus
> +	 * error or a termination error with no completion.
> +	 */
> +	stat2 = ((stat2 >> 16) & stat2) |	   /* bus error */
> +		(~(stat2 >> 16) & stat2 & ~stat1); /* termination with no completion */
> +
> +	/* combine error and completion status for checking */
> +	stat1 = (stat2 << 16) | stat1;
> +	while (stat1) {
> +		int channel = fls(stat1) - 1;
> +		struct mxs_dma_chan *mxs_chan =
> +				&mxs_dma->mxs_chans[channel % 16];
> +
> +		if (channel >= 16) {
> +			dev_dbg(mxs_dma->dev, "%s: error in channel %d\n",
> +						__func__, channel - 16);
> +			mxs_dma_reset_chan(mxs_chan);
> +			mxs_chan->status = DMA_ERROR;
> +		} else {
> +			if (mxs_chan->flags & MXS_DMA_SG_LOOP)
> +				mxs_chan->status = DMA_IN_PROGRESS;
> +			else
> +				mxs_chan->status = DMA_SUCCESS;
> +		}
> +
> +		stat1 &= ~(1 << channel);
> +
> +		if (mxs_chan->desc.callback)
> +			mxs_chan->desc.callback(mxs_chan->desc.callback_param);

Callbacks are supposed to happen from tasklet context, not irq context.

> +
> +		if (mxs_chan->status == DMA_SUCCESS)
> +			mxs_chan->last_completed = mxs_chan->desc.cookie;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
> +{
> +	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> +	struct mxs_dma_data *data = chan->private;
> +	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> +	static unsigned long flags;
> +	int ret;
> +
> +	if (!data)
> +		return -EINVAL;
> +
> +	mxs_chan->chan_irq = data->chan_irq;
> +
> +	mxs_chan->ccw = dma_alloc_coherent(NULL, PAGE_SIZE,
> +				&mxs_chan->ccw_phys, GFP_KERNEL);

Don't you have a struct device for this?  Without a struct device,
dma_alloc_coherent() can only assume that it must give you something
suitable for the smallest DMA mask in your system.  That seems
to be mxs_dma->dev.

> +	if (!mxs_chan->ccw) {
> +		ret = -ENOMEM;
> +		goto err_alloc;
> +	}
> +
> +	memset(mxs_chan->ccw, 0, PAGE_SIZE);
> +
> +	ret = request_irq(mxs_chan->chan_irq, mxs_dma_int_handler,
> +				flags, "mxs-dma", mxs_dma);
> +	if (ret)
> +		goto err_irq;
> +
> +	flags = IRQF_SHARED;

Initialization of flags after use?

> +
> +	clk_enable(mxs_dma->clk);

Error checking?

> +
> +	mxs_dma_reset_chan(mxs_chan);
> +
> +	dma_async_tx_descriptor_init(&mxs_chan->desc, chan);
> +	mxs_chan->desc.tx_submit = mxs_dma_tx_submit;
> +
> +	/* the descriptor is ready */
> +	async_tx_ack(&mxs_chan->desc);
> +
> +	return 0;
> +
> +err_irq:
> +	dma_free_coherent(NULL, PAGE_SIZE, mxs_chan->ccw, mxs_chan->ccw_phys);
> +err_alloc:
> +	return ret;
> +}
> +
> +static void mxs_dma_free_chan_resources(struct dma_chan *chan)
> +{
> +	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> +	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> +
> +	mxs_dma_disable_chan(mxs_chan);
> +
> +	free_irq(mxs_chan->chan_irq, mxs_dma);
> +
> +	dma_free_coherent(NULL, PAGE_SIZE, mxs_chan->ccw, mxs_chan->ccw_phys);

Same comment about struct device.

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Shawn Guo]

Hi Russell,

Thanks for the review and comments.

Hi Sascha,

Some of the comments here also apply on imx-sdma, as mxs-dma closely
followed imx-sdma implementation.  It's appreciated if you can give 
some responses to the comments.

On Fri, Feb 04, 2011 at 06:17:49PM +0000, Russell King - ARM Linux wrote:
> On Sat, Feb 05, 2011 at 10:08:12AM +0800, Shawn Guo wrote:
> > +struct mxs_dma_ccw_bits {
> > +	unsigned int	command:2;
> > +#define MXS_DMA_NO_XFER	0x00
> > +#define MXS_DMA_WRITE	0x01
> > +#define MXS_DMA_READ	0x02
> > +#define MXS_DMA_SENSE	0x03			/* not implemented */
> > +	unsigned int	chain:1;
> > +	unsigned int	irq:1;
> > +	unsigned int	nand_lock:1;		/* not implemented */
> > +	unsigned int	nand_wait4ready:1;	/* not implemented */
> > +	unsigned int	dec_sem:1;
> > +	unsigned int	wait4end:1;
> > +	unsigned int	halt_on_terminate:1;
> > +	unsigned int	terminate_flush:1;
> > +	unsigned int	reserved:2;
> > +	unsigned int	pio_num:4;
> > +	unsigned int	xfer_bytes:16;
> > +#define MAX_XFER_BYTES	0xffff
> > +};
> 
> Bitfields are subject to endianness issues.  Are you sure this is a good
> idea?
> 
Honestly, I'm still a little bit new to kernel development.  Can you
please suggest the correct way to do this?  BP/BM macros like general
register access?

> > +static dma_cookie_t mxs_dma_tx_submit(struct dma_async_tx_descriptor *tx)
> > +{
> > +	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(tx->chan);
> > +	dma_cookie_t cookie;
> > +
> > +	spin_lock_irq(&mxs_chan->lock);
> > +
> > +	cookie = mxs_dma_assign_cookie(mxs_chan);
> > +
> > +	mxs_dma_enable_chan(mxs_chan);
> > +
> > +	spin_unlock_irq(&mxs_chan->lock);
> 
> Do you know that you'll always be called from an IRQs-enabled context?
> If not, you should use the irqsave/irqrestore versions.
> 
OK.

> > +
> > +	return cookie;
> > +}
> > +
> > +static irqreturn_t mxs_dma_int_handler(int irq, void *dev_id)
> > +{
> > +	struct mxs_dma_engine *mxs_dma = dev_id;
> > +	u32 stat1, stat2;
> > +
> > +	/* completion status */
> > +	stat1 = __raw_readl(mxs_dma->base + HW_APBHX_CTRL1);
> > +	stat1 &= 0xffff;
> > +	__mxs_clrl(stat1, mxs_dma->base + HW_APBHX_CTRL1);
> > +
> > +	/* error status */
> > +	stat2 = __raw_readl(mxs_dma->base + HW_APBHX_CTRL2);
> > +	__mxs_clrl(stat2, mxs_dma->base + HW_APBHX_CTRL2);
> 
> Would it be useful to report error status when it happens?
> 
The only useful error info that matters here is the channel which
has error.  The dev_dbg below tells that.  You expect dev_err?

> > +
> > +	/*
> > +	 * When both completion and error of termination bits set at the
> > +	 * same time, we do not take it as an error.  IOW, it only becomes
> > +	 * an error we need to handler here in case of ether it's an bus
> > +	 * error or a termination error with no completion.
> > +	 */
> > +	stat2 = ((stat2 >> 16) & stat2) |	   /* bus error */
> > +		(~(stat2 >> 16) & stat2 & ~stat1); /* termination with no completion */
> > +
> > +	/* combine error and completion status for checking */
> > +	stat1 = (stat2 << 16) | stat1;
> > +	while (stat1) {
> > +		int channel = fls(stat1) - 1;
> > +		struct mxs_dma_chan *mxs_chan =
> > +				&mxs_dma->mxs_chans[channel % 16];
> > +
> > +		if (channel >= 16) {
> > +			dev_dbg(mxs_dma->dev, "%s: error in channel %d\n",
> > +						__func__, channel - 16);

Expect dev_err?

> > +			mxs_dma_reset_chan(mxs_chan);
> > +			mxs_chan->status = DMA_ERROR;
> > +		} else {
> > +			if (mxs_chan->flags & MXS_DMA_SG_LOOP)
> > +				mxs_chan->status = DMA_IN_PROGRESS;
> > +			else
> > +				mxs_chan->status = DMA_SUCCESS;
> > +		}
> > +
> > +		stat1 &= ~(1 << channel);
> > +
> > +		if (mxs_chan->desc.callback)
> > +			mxs_chan->desc.callback(mxs_chan->desc.callback_param);
> 
> Callbacks are supposed to happen from tasklet context, not irq context.
> 

Here is callback copied from mxs-mmc driver.  I expect other mxs-dma
client driver's callbacks are as simple as this one.

static void mxs_mmc_dma_irq_callback(void *param)
{
	struct mxs_mmc_host *host = param;

	host->status = __raw_readl(host->base + HW_SSP_STATUS);
	complete(&host->done);
}

Is a simple callback also required be in tasklet context anyhow?

> > +
> > +		if (mxs_chan->status == DMA_SUCCESS)
> > +			mxs_chan->last_completed = mxs_chan->desc.cookie;
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
> > +{
> > +	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> > +	struct mxs_dma_data *data = chan->private;
> > +	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> > +	static unsigned long flags;
> > +	int ret;
> > +
> > +	if (!data)
> > +		return -EINVAL;
> > +
> > +	mxs_chan->chan_irq = data->chan_irq;
> > +
> > +	mxs_chan->ccw = dma_alloc_coherent(NULL, PAGE_SIZE,
> > +				&mxs_chan->ccw_phys, GFP_KERNEL);
> 
> Don't you have a struct device for this?  Without a struct device,
> dma_alloc_coherent() can only assume that it must give you something
> suitable for the smallest DMA mask in your system.  That seems
> to be mxs_dma->dev.
> 
OK.  With your comment in another reply, this becomes
mxs_dma->dma_device.dev then.

> > +	if (!mxs_chan->ccw) {
> > +		ret = -ENOMEM;
> > +		goto err_alloc;
> > +	}
> > +
> > +	memset(mxs_chan->ccw, 0, PAGE_SIZE);
> > +
> > +	ret = request_irq(mxs_chan->chan_irq, mxs_dma_int_handler,
> > +				flags, "mxs-dma", mxs_dma);
> > +	if (ret)
> > +		goto err_irq;
> > +
> > +	flags = IRQF_SHARED;
> 
> Initialization of flags after use?
> 
I should have flags initialized as 0.  But as Lothar pointed out,
I mistakenly used flag IRQF_SHARED.  My intention is to share the
same handler for different irq lines.

> > +
> > +	clk_enable(mxs_dma->clk);
> 
> Error checking?
> 
OK.

> > +
> > +	mxs_dma_reset_chan(mxs_chan);
> > +
> > +	dma_async_tx_descriptor_init(&mxs_chan->desc, chan);
> > +	mxs_chan->desc.tx_submit = mxs_dma_tx_submit;
> > +
> > +	/* the descriptor is ready */
> > +	async_tx_ack(&mxs_chan->desc);
> > +
> > +	return 0;
> > +
> > +err_irq:
> > +	dma_free_coherent(NULL, PAGE_SIZE, mxs_chan->ccw, mxs_chan->ccw_phys);
> > +err_alloc:
> > +	return ret;
> > +}
> > +
> > +static void mxs_dma_free_chan_resources(struct dma_chan *chan)
> > +{
> > +	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> > +	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> > +
> > +	mxs_dma_disable_chan(mxs_chan);
> > +
> > +	free_irq(mxs_chan->chan_irq, mxs_dma);
> > +
> > +	dma_free_coherent(NULL, PAGE_SIZE, mxs_chan->ccw, mxs_chan->ccw_phys);
> 
> Same comment about struct device.
> 
OK.

Regards,
Shawn

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Russell King - ARM Linux]

On Wed, Feb 09, 2011 at 06:56:36AM +0800, Shawn Guo wrote:
> Hi Russell,
> 
> Thanks for the review and comments.
> 
> Hi Sascha,
> 
> Some of the comments here also apply on imx-sdma, as mxs-dma closely
> followed imx-sdma implementation.  It's appreciated if you can give 
> some responses to the comments.
> 
> On Fri, Feb 04, 2011 at 06:17:49PM +0000, Russell King - ARM Linux wrote:
> > Callbacks are supposed to happen from tasklet context, not irq context.
> > 
> 
> Here is callback copied from mxs-mmc driver.  I expect other mxs-dma
> client driver's callbacks are as simple as this one.
> 
> static void mxs_mmc_dma_irq_callback(void *param)
> {
> 	struct mxs_mmc_host *host = param;
> 
> 	host->status = __raw_readl(host->base + HW_SSP_STATUS);
> 	complete(&host->done);
> }
> 
> Is a simple callback also required be in tasklet context anyhow?

It is part of the DMA engine API specification.  It may not matter
for stuff which currently exists, but how do you know that in the
future you won't be re-using existing drivers which do other stuff
in callbacks and do assume that they're correctly called as per the
DMA engine API?

If you're not going to implement your driver to the DMA engine API
specification, there's no point implementing something which looks
like a DMA engine API but isn't.

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Russell King - ARM Linux]

Shawn,

A couple more points below.

On Sat, Feb 05, 2011 at 10:08:12AM +0800, Shawn Guo wrote:
> +	mxs_dma->dev = &pdev->dev;
...
> +	mxs_dma->dma_device.dev = &pdev->dev;

Do you need mxs_dma->dev, or could you just use mxs_dma->dma_device.dev
throughout?

> +static int __exit mxs_dma_remove(struct platform_device *pdev)
> +{
> +	return -EBUSY;
> +}

As the return code is ignored, it's probably better to omit the remove
function entirely.

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Shawn Guo]

Hi Russell,

On Sat, Feb 05, 2011 at 03:33:00PM +0000, Russell King - ARM Linux wrote:
> Shawn,
> 
> A couple more points below.
> 
> On Sat, Feb 05, 2011 at 10:08:12AM +0800, Shawn Guo wrote:
> > +	mxs_dma->dev = &pdev->dev;
> ...
> > +	mxs_dma->dma_device.dev = &pdev->dev;
> 
> Do you need mxs_dma->dev, or could you just use mxs_dma->dma_device.dev
> throughout?
> 
OK.

> > +static int __exit mxs_dma_remove(struct platform_device *pdev)
> > +{
> > +	return -EBUSY;
> > +}
> 
> As the return code is ignored, it's probably better to omit the remove
> function entirely.
> 
OK.

Regards,
Shawn

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Lothar Waßmann]

Hi,

Shawn Guo writes:
> This patch adds dma support for Freescale MXS-based SoC i.MX23/28,
> including apbh-dma and apbx-dma.
> 
> * apbh-dma and apbx-dma are supported in the driver as two instances,
>   and have to be filtered by dma clients via device id.  It becomes
>   the convention that apbh-dma always gets registered prior to
>   apbx-dma.
> 
> * apbh-dma is different between mx23 and mx28, hardware version
>   register is used to handle the differences.
> 
> * Every the mxs dma channel is statically assigned to client device
>   by soc design with fixed irq.  The irq number is being passed by
>   alloc_chan function with mxs_dma_data, and client driver has to
>   filter the correct channel by its channel id.
> 
> * mxs-dma supports pio function besides data transfer.  The driver
>   uses dma_data_direction DMA_NONE to identify the pio mode, and
>   steals sgl and sg_len to get pio words and numbers from clients.
> 
> * mxs dmaengine has some very specific features, like sense function
>   and the special NAND support (nand_lock, nand_wait4ready).  These
>   are too specific to implemented in generic dmaengine driver.
> 
> * The parameter "flags" of prep functions is currently being used to
>   pass wait4end flag from clients.
> 
> * The driver refers to imx-sdma and only a single descriptor is
>   statically assigned to each channel.
> 
> Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> ---
>  arch/arm/mach-mxs/include/mach/dma.h |   16 +
>  drivers/dma/Kconfig                  |    8 +
>  drivers/dma/Makefile                 |    1 +
>  drivers/dma/mxs-dma.c                |  702 ++++++++++++++++++++++++++++++++++
>  4 files changed, 727 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/mach-mxs/include/mach/dma.h
>  create mode 100644 drivers/dma/mxs-dma.c
> 
> diff --git a/arch/arm/mach-mxs/include/mach/dma.h b/arch/arm/mach-mxs/include/mach/dma.h
> new file mode 100644
> index 0000000..429f431
> --- /dev/null
> +++ b/arch/arm/mach-mxs/include/mach/dma.h
> @@ -0,0 +1,16 @@
> +/*
> + * Copyright 2011 Freescale Semiconductor, Inc. All Rights Reserved.
> + *
> + * 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.
> + */
> +
> +#ifndef __MACH_MXS_DMA_H__
> +#define __MACH_MXS_DMA_H__
> +
> +struct mxs_dma_data {
> +	int chan_irq;
> +};
> +
> +#endif /* __MACH_MXS_DMA_H__ */
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 1c28816..0b9a5b1 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -227,6 +227,14 @@ config IMX_DMA
>  	  Support the i.MX DMA engine. This engine is integrated into
>  	  Freescale i.MX1/21/27 chips.
>  
> +config MXS_DMA
> +	tristate "MXS DMA support"
>
Does it make any sense to build this driver as a module, given the
fact that it does not even support to be removed?

> +	depends on SOC_IMX23 || SOC_MX28
>
SOC_IMX28?

> diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> new file mode 100644
> index 0000000..8d1e811
> --- /dev/null
> +++ b/drivers/dma/mxs-dma.c
> @@ -0,0 +1,702 @@
[...]
> +struct mxs_dma_engine {
> +	struct device			*dev;
> +	int				dev_id;
> +	unsigned int			version;
> +	void __iomem			*base;
> +	struct clk			*clk;
> +	struct dma_device		dma_device;
> +	struct device_dma_parameters 	dma_parms;
                                    ^^^^
mixed space/tab indentation.

[...]
> +static int mxs_dma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
> +		unsigned long arg)
> +{
> +	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> +
> +	switch (cmd) {
> +	case DMA_TERMINATE_ALL:
> +		mxs_dma_disable_chan(mxs_chan);
> +		return 0;
> +	case DMA_PAUSE:
> +		mxs_dma_pause_chan(mxs_chan);
> +		return 0;
> +	case DMA_RESUME:
> +		mxs_dma_resume_chan(mxs_chan);
> +		return 0;
> +	default:
> +		return -ENOSYS;
> +	}
> +
> +	return -EINVAL;
        ^^^^^^^^^^^^^^
This can never be reached. IMO:
|	int ret = 0;
|	switch(cmd) {
|	case DMA_TERMINATE_ALL:
|		mxs_dma_disable_chan(mxs_chan);
|		break;
|	case DMA_PAUSE:
|		mxs_dma_pause_chan(mxs_chan);
|		break;
|	case DMA_RESUME:
|		mxs_dma_resume_chan(mxs_chan);
|		break;
|	default:
|		ret = -ENOSYS;
|	}
|
|	return ret;
would be cleaner (and effectively generates the same code).


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Shawn Guo]

Hi Lothar,

Thanks for the review and all the catches.

On Mon, Feb 07, 2011 at 08:37:14AM +0100, Lothar Wa?mann wrote:
> Hi,
> 
> Shawn Guo writes:
> > This patch adds dma support for Freescale MXS-based SoC i.MX23/28,
> > including apbh-dma and apbx-dma.
> > 
> > * apbh-dma and apbx-dma are supported in the driver as two instances,
> >   and have to be filtered by dma clients via device id.  It becomes
> >   the convention that apbh-dma always gets registered prior to
> >   apbx-dma.
> > 
> > * apbh-dma is different between mx23 and mx28, hardware version
> >   register is used to handle the differences.
> > 
> > * Every the mxs dma channel is statically assigned to client device
> >   by soc design with fixed irq.  The irq number is being passed by
> >   alloc_chan function with mxs_dma_data, and client driver has to
> >   filter the correct channel by its channel id.
> > 
> > * mxs-dma supports pio function besides data transfer.  The driver
> >   uses dma_data_direction DMA_NONE to identify the pio mode, and
> >   steals sgl and sg_len to get pio words and numbers from clients.
> > 
> > * mxs dmaengine has some very specific features, like sense function
> >   and the special NAND support (nand_lock, nand_wait4ready).  These
> >   are too specific to implemented in generic dmaengine driver.
> > 
> > * The parameter "flags" of prep functions is currently being used to
> >   pass wait4end flag from clients.
> > 
> > * The driver refers to imx-sdma and only a single descriptor is
> >   statically assigned to each channel.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > ---
> >  arch/arm/mach-mxs/include/mach/dma.h |   16 +
> >  drivers/dma/Kconfig                  |    8 +
> >  drivers/dma/Makefile                 |    1 +
> >  drivers/dma/mxs-dma.c                |  702 ++++++++++++++++++++++++++++++++++
> >  4 files changed, 727 insertions(+), 0 deletions(-)
> >  create mode 100644 arch/arm/mach-mxs/include/mach/dma.h
> >  create mode 100644 drivers/dma/mxs-dma.c
> > 
> > diff --git a/arch/arm/mach-mxs/include/mach/dma.h b/arch/arm/mach-mxs/include/mach/dma.h
> > new file mode 100644
> > index 0000000..429f431
> > --- /dev/null
> > +++ b/arch/arm/mach-mxs/include/mach/dma.h
> > @@ -0,0 +1,16 @@
> > +/*
> > + * Copyright 2011 Freescale Semiconductor, Inc. All Rights Reserved.
> > + *
> > + * 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.
> > + */
> > +
> > +#ifndef __MACH_MXS_DMA_H__
> > +#define __MACH_MXS_DMA_H__
> > +
> > +struct mxs_dma_data {
> > +	int chan_irq;
> > +};
> > +
> > +#endif /* __MACH_MXS_DMA_H__ */
> > diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> > index 1c28816..0b9a5b1 100644
> > --- a/drivers/dma/Kconfig
> > +++ b/drivers/dma/Kconfig
> > @@ -227,6 +227,14 @@ config IMX_DMA
> >  	  Support the i.MX DMA engine. This engine is integrated into
> >  	  Freescale i.MX1/21/27 chips.
> >  
> > +config MXS_DMA
> > +	tristate "MXS DMA support"
> >
> Does it make any sense to build this driver as a module, given the
> fact that it does not even support to be removed?
> 
Will change it to bool.  BTW, the comment applies on imx-dma and
imx-sdma too, I guess.

> > +	depends on SOC_IMX23 || SOC_MX28
> >
> SOC_IMX28?
> 
Yes.

> > diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> > new file mode 100644
> > index 0000000..8d1e811
> > --- /dev/null
> > +++ b/drivers/dma/mxs-dma.c
> > @@ -0,0 +1,702 @@
> [...]
> > +struct mxs_dma_engine {
> > +	struct device			*dev;
> > +	int				dev_id;
> > +	unsigned int			version;
> > +	void __iomem			*base;
> > +	struct clk			*clk;
> > +	struct dma_device		dma_device;
> > +	struct device_dma_parameters 	dma_parms;
>                                     ^^^^
> mixed space/tab indentation.
> 
Will fix it.

> [...]
> > +static int mxs_dma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
> > +		unsigned long arg)
> > +{
> > +	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> > +
> > +	switch (cmd) {
> > +	case DMA_TERMINATE_ALL:
> > +		mxs_dma_disable_chan(mxs_chan);
> > +		return 0;
> > +	case DMA_PAUSE:
> > +		mxs_dma_pause_chan(mxs_chan);
> > +		return 0;
> > +	case DMA_RESUME:
> > +		mxs_dma_resume_chan(mxs_chan);
> > +		return 0;
> > +	default:
> > +		return -ENOSYS;
> > +	}
> > +
> > +	return -EINVAL;
>         ^^^^^^^^^^^^^^
> This can never be reached. IMO:
> |	int ret = 0;
> |	switch(cmd) {
> |	case DMA_TERMINATE_ALL:
> |		mxs_dma_disable_chan(mxs_chan);
> |		break;
> |	case DMA_PAUSE:
> |		mxs_dma_pause_chan(mxs_chan);
> |		break;
> |	case DMA_RESUME:
> |		mxs_dma_resume_chan(mxs_chan);
> |		break;
> |	default:
> |		ret = -ENOSYS;
> |	}
> |
> |	return ret;
> would be cleaner (and effectively generates the same code).
> 
Nice.  Will take it.

Regards,
Shawn

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Sascha Hauer]

On Sat, Feb 05, 2011 at 10:08:12AM +0800, Shawn Guo wrote:
> This patch adds dma support for Freescale MXS-based SoC i.MX23/28,
> including apbh-dma and apbx-dma.
> 
> * apbh-dma and apbx-dma are supported in the driver as two instances,
>   and have to be filtered by dma clients via device id.  It becomes
>   the convention that apbh-dma always gets registered prior to
>   apbx-dma.
> 
> * apbh-dma is different between mx23 and mx28, hardware version
>   register is used to handle the differences.
> 
> * Every the mxs dma channel is statically assigned to client device
>   by soc design with fixed irq.  The irq number is being passed by
>   alloc_chan function with mxs_dma_data, and client driver has to
>   filter the correct channel by its channel id.
> 
> * mxs-dma supports pio function besides data transfer.  The driver
>   uses dma_data_direction DMA_NONE to identify the pio mode, and
>   steals sgl and sg_len to get pio words and numbers from clients.
> 
> * mxs dmaengine has some very specific features, like sense function
>   and the special NAND support (nand_lock, nand_wait4ready).  These
>   are too specific to implemented in generic dmaengine driver.
> 
> * The parameter "flags" of prep functions is currently being used to
>   pass wait4end flag from clients.
> 
> * The driver refers to imx-sdma and only a single descriptor is
>   statically assigned to each channel.
> 
> Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> ---
>  arch/arm/mach-mxs/include/mach/dma.h |   16 +
>  drivers/dma/Kconfig                  |    8 +
>  drivers/dma/Makefile                 |    1 +
>  drivers/dma/mxs-dma.c                |  702 ++++++++++++++++++++++++++++++++++
>  4 files changed, 727 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/mach-mxs/include/mach/dma.h
>  create mode 100644 drivers/dma/mxs-dma.c
> 
> diff --git a/arch/arm/mach-mxs/include/mach/dma.h b/arch/arm/mach-mxs/include/mach/dma.h
> new file mode 100644
> index 0000000..429f431
> --- /dev/null
> +++ b/arch/arm/mach-mxs/include/mach/dma.h
> +
> +static struct dma_async_tx_descriptor *mxs_dma_prep_slave_sg(
> +		struct dma_chan *chan, struct scatterlist *sgl,
> +		unsigned int sg_len, enum dma_data_direction direction,
> +		unsigned long flags)
> +{
> +	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> +	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> +	struct mxs_dma_ccw *ccw;
> +	struct scatterlist *sg;
> +	int ret, i, j;
> +	u32 *pio;
> +
> +	dev_dbg(mxs_dma->dev, "%s: channel %d\n", __func__, chan->chan_id);
> +
> +	if (mxs_chan->status == DMA_IN_PROGRESS)
> +		return NULL;
> +
> +	mxs_chan->status = DMA_IN_PROGRESS;
> +	mxs_chan->flags = 0;
> +
> +	dev_dbg(mxs_dma->dev, "%s: setting up %d entries\n", __func__, sg_len);
> +
> +	if (sg_len > ((direction == DMA_NONE) ? MXS_PIO_WORDS : NUM_CCW)) {
> +		dev_err(mxs_dma->dev, "maximum number of sg exceeded: %d > %d\n",
> +				sg_len, NUM_CCW);
> +		ret = -EINVAL;
> +		goto err_out;
> +	}
> +
> +	if (direction == DMA_NONE) {
> +		ccw = &mxs_chan->ccw[0];
> +		pio = (u32 *) sgl;
> +
> +		for (j = 0; j < sg_len;)
> +			ccw->pio_words[j++] = *pio++;
> +
> +		ccw->next = 0;
> +		ccw->bits.chain = 0;
> +		ccw->bits.irq = 1;
> +		ccw->bits.dec_sem = 1;
> +		ccw->bits.wait4end = flags;
> +		ccw->bits.halt_on_terminate = 1;
> +		ccw->bits.terminate_flush = 1;
> +		ccw->bits.pio_num = sg_len;
> +		ccw->bits.command = MXS_DMA_NO_XFER;

Does this have a valid usecase? I would just return some error code
here. pio_num and pio_words are unused in the driver and I don't think
a dmaengine driver should have some kind of PIO fallback.

> +	} else {
> +		for_each_sg(sgl, sg, sg_len, i) {
> +			if (sg->length > MAX_XFER_BYTES) {
> +				dev_err(mxs_dma->dev, "maximum bytes for sg entry exceeded: %d > %d\n",
> +						sg->length, MAX_XFER_BYTES);
> +				ret = -EINVAL;
> +				goto err_out;
> +			}
> +
> +			ccw = &mxs_chan->ccw[i];
> +
> +			ccw->next = mxs_chan->ccw_phys + sizeof(*ccw) * (i + 1);
> +			ccw->bufaddr = sg->dma_address;
> +			ccw->bits.xfer_bytes = sg->length;
> +			ccw->bits.chain = 1;
> +			ccw->bits.irq = 0;
> +			ccw->bits.dec_sem = 0;
> +			ccw->bits.wait4end = 0;
> +			ccw->bits.halt_on_terminate = 1;
> +			ccw->bits.terminate_flush = 1;
> +			ccw->bits.pio_num = 0;
> +			ccw->bits.command = (direction == DMA_FROM_DEVICE) ?
> +						MXS_DMA_WRITE : MXS_DMA_READ;
> +
> +			if (i + 1 == sg_len) {
> +				ccw->next = 0;
> +				ccw->bits.chain = 0;
> +				ccw->bits.irq = 1;
> +				ccw->bits.dec_sem = 1;
> +				ccw->bits.wait4end = 1;
> +			}
> +		}
> +	}
> +
> +	return &mxs_chan->desc;
> +
> +err_out:
> +	mxs_chan->status = DMA_ERROR;
> +	return NULL;
> +}
> +
> +
> +static int __init mxs_dma_probe(struct platform_device *pdev)
> +{
> +	const struct platform_device_id *id_entry =
> +				platform_get_device_id(pdev);
> +	struct mxs_dma_engine *mxs_dma;
> +	struct resource *iores;
> +	int ret, i;
> +
> +	mxs_dma = kzalloc(sizeof(*mxs_dma), GFP_KERNEL);
> +	if (!mxs_dma)
> +		return -ENOMEM;
> +
> +	mxs_dma->dev = &pdev->dev;
> +	mxs_dma->dev_id = id_entry->driver_data;
> +
> +	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +	if (!request_mem_region(iores->start, resource_size(iores),
> +				pdev->name)) {
> +		ret = -EBUSY;
> +		goto err_request_region;
> +	}
> +
> +	mxs_dma->base = ioremap(iores->start, resource_size(iores));
> +	if (!mxs_dma->base) {
> +		ret = -ENOMEM;
> +		goto err_ioremap;
> +	}
> +
> +	mxs_dma->clk = clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(mxs_dma->clk)) {
> +		ret = PTR_ERR(mxs_dma->clk);
> +		goto err_clk;
> +	}
> +
> +	dma_cap_set(DMA_SLAVE, mxs_dma->dma_device.cap_mask);
> +	dma_cap_set(DMA_CYCLIC, mxs_dma->dma_device.cap_mask);
> +
> +	INIT_LIST_HEAD(&mxs_dma->dma_device.channels);
> +
> +	/* Initialize channel parameters */
> +	for (i = 0; i < MXS_DMA_CHANNELS; i++) {
> +		struct mxs_dma_chan *mxs_chan = &mxs_dma->mxs_chans[i];
> +
> +		mxs_chan->mxs_dma = mxs_dma;
> +		spin_lock_init(&mxs_chan->lock);
> +
> +		mxs_chan->chan.device = &mxs_dma->dma_device;
> +
> +		/* Add the channel to mxs_chan list */
> +		list_add_tail(&mxs_chan->chan.device_node, &mxs_dma->dma_device.channels);
> +	}
> +
> +	ret = mxs_dma_init(mxs_dma);
> +	if (ret)
> +		goto err_init;
> +
> +	mxs_dma->dma_device.dev = &pdev->dev;
> +
> +	/* mxs_dma gets 65535 bytes maximum sg size */
> +	mxs_dma->dma_device.dev->dma_parms = &mxs_dma->dma_parms;
> +	dma_set_max_seg_size(mxs_dma->dma_device.dev, MAX_XFER_BYTES);
> +
> +	mxs_dma->dma_device.device_alloc_chan_resources = mxs_dma_alloc_chan_resources;
> +	mxs_dma->dma_device.device_free_chan_resources = mxs_dma_free_chan_resources;
> +	mxs_dma->dma_device.device_tx_status = mxs_dma_tx_status;
> +	mxs_dma->dma_device.device_prep_slave_sg = mxs_dma_prep_slave_sg;
> +	mxs_dma->dma_device.device_prep_dma_cyclic = mxs_dma_prep_dma_cyclic;
> +	mxs_dma->dma_device.device_control = mxs_dma_control;
> +	mxs_dma->dma_device.device_issue_pending = mxs_dma_issue_pending;
> +
> +	ret = dma_async_device_register(&mxs_dma->dma_device);
> +	if (ret) {
> +		dev_err(&pdev->dev, "unable to register\n");
> +		goto err_dma_register;
> +	}
> +
> +	/*
> +	 * dmaengine clients have to use dma_device.dev_id to filter
> +	 * dma device between apbh and apbx, so need to ensure it is
> +	 * identical to mxs_dma_engine.dev_id.
> +	 */
> +	if (mxs_dma->dma_device.dev_id != mxs_dma->dev_id) {
> +		dev_err(&pdev->dev, "dev_id of dma_device %d differs from mxs_dma_engine %d\n",
> +				mxs_dma->dma_device.dev_id, mxs_dma->dev_id);
> +		goto err_init;
> +	}

I think it makes more sense to match against device names (apbh vs.
apbx) in the client's filter function than to rely on exact numbering.

> +
> +	dev_info(mxs_dma->dev, "initialized\n");
> +
> +	return 0;
> +
> +err_init:
> +	dma_async_device_unregister(&mxs_dma->dma_device);
> +err_dma_register:
> +	clk_put(mxs_dma->clk);
> +err_clk:
> +	iounmap(mxs_dma->base);
> +err_ioremap:
> +	release_mem_region(iores->start, resource_size(iores));
> +err_request_region:
> +	kfree(mxs_dma);
> +	return ret;
> +}
> +
> +static int __exit mxs_dma_remove(struct platform_device *pdev)
> +{
> +	return -EBUSY;
> +}
> +
> +static struct platform_device_id mxs_dma_type[] = {
> +	{
> +		.name = "mxs-dma-apbh",
> +		.driver_data = MXS_DMA_APBH,
> +	}, {
> +		.name = "mxs-dma-apbx",
> +		.driver_data = MXS_DMA_APBX,
> +	}
> +};
> +
> +static struct platform_driver mxs_dma_driver = {
> +	.driver		= {
> +		.name	= "mxs-dma",
> +	},
> +	.id_table	= mxs_dma_type,
> +	.remove		= __exit_p(mxs_dma_remove),
> +};
> +
> +static int __init mxs_dma_module_init(void)
> +{
> +	return platform_driver_probe(&mxs_dma_driver, mxs_dma_probe);
> +}
> +subsys_initcall(mxs_dma_module_init);
> -- 
> 1.7.1
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Russell King - ARM Linux]

On Mon, Feb 07, 2011 at 09:25:21AM +0100, Sascha Hauer wrote:
> > +		ccw->next = 0;
> > +		ccw->bits.chain = 0;
> > +		ccw->bits.irq = 1;
> > +		ccw->bits.dec_sem = 1;
> > +		ccw->bits.wait4end = flags;
> > +		ccw->bits.halt_on_terminate = 1;
> > +		ccw->bits.terminate_flush = 1;
> > +		ccw->bits.pio_num = sg_len;
> > +		ccw->bits.command = MXS_DMA_NO_XFER;
> 
> Does this have a valid usecase? I would just return some error code
> here. pio_num and pio_words are unused in the driver and I don't think
> a dmaengine driver should have some kind of PIO fallback.

DMA drivers must not perform PIO as a fallback - that's the job of
the driver using the DMA engine API.  The reason is that it buggers up
the DMA buffer ownership rules to the extent that data loss will occur
on ARMv6 and later CPUs.

Also note that the struct device to be used for mapping buffers with
the DMA engine is the dma_device's struct device, not the peripheral
device using the DMA engine.  The DMA engine device is what's
performing the DMA, not the peripheral device.

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Shawn Guo]

Hi Sascha,

Thanks for the review.

On Mon, Feb 07, 2011 at 09:25:21AM +0100, Sascha Hauer wrote:
> On Sat, Feb 05, 2011 at 10:08:12AM +0800, Shawn Guo wrote:
> > This patch adds dma support for Freescale MXS-based SoC i.MX23/28,
> > including apbh-dma and apbx-dma.
> >
> > * apbh-dma and apbx-dma are supported in the driver as two instances,
> >   and have to be filtered by dma clients via device id.  It becomes
> >   the convention that apbh-dma always gets registered prior to
> >   apbx-dma.
> >
> > * apbh-dma is different between mx23 and mx28, hardware version
> >   register is used to handle the differences.
> >
> > * Every the mxs dma channel is statically assigned to client device
> >   by soc design with fixed irq.  The irq number is being passed by
> >   alloc_chan function with mxs_dma_data, and client driver has to
> >   filter the correct channel by its channel id.
> >
> > * mxs-dma supports pio function besides data transfer.  The driver
> >   uses dma_data_direction DMA_NONE to identify the pio mode, and
> >   steals sgl and sg_len to get pio words and numbers from clients.
> >
> > * mxs dmaengine has some very specific features, like sense function
> >   and the special NAND support (nand_lock, nand_wait4ready).  These
> >   are too specific to implemented in generic dmaengine driver.
> >
> > * The parameter "flags" of prep functions is currently being used to
> >   pass wait4end flag from clients.
> >
> > * The driver refers to imx-sdma and only a single descriptor is
> >   statically assigned to each channel.
> >
> > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > ---
> >  arch/arm/mach-mxs/include/mach/dma.h |   16 +
> >  drivers/dma/Kconfig                  |    8 +
> >  drivers/dma/Makefile                 |    1 +
> >  drivers/dma/mxs-dma.c                |  702 ++++++++++++++++++++++++++++++++++
> >  4 files changed, 727 insertions(+), 0 deletions(-)
> >  create mode 100644 arch/arm/mach-mxs/include/mach/dma.h
> >  create mode 100644 drivers/dma/mxs-dma.c
> >
> > diff --git a/arch/arm/mach-mxs/include/mach/dma.h b/arch/arm/mach-mxs/include/mach/dma.h
> > new file mode 100644
> > index 0000000..429f431
> > --- /dev/null
> > +++ b/arch/arm/mach-mxs/include/mach/dma.h
> > +
> > +static struct dma_async_tx_descriptor *mxs_dma_prep_slave_sg(
> > +             struct dma_chan *chan, struct scatterlist *sgl,
> > +             unsigned int sg_len, enum dma_data_direction direction,
> > +             unsigned long flags)
> > +{
> > +     struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> > +     struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> > +     struct mxs_dma_ccw *ccw;
> > +     struct scatterlist *sg;
> > +     int ret, i, j;
> > +     u32 *pio;
> > +
> > +     dev_dbg(mxs_dma->dev, "%s: channel %d\n", __func__, chan->chan_id);
> > +
> > +     if (mxs_chan->status == DMA_IN_PROGRESS)
> > +             return NULL;
> > +
> > +     mxs_chan->status = DMA_IN_PROGRESS;
> > +     mxs_chan->flags = 0;
> > +
> > +     dev_dbg(mxs_dma->dev, "%s: setting up %d entries\n", __func__, sg_len);
> > +
> > +     if (sg_len > ((direction == DMA_NONE) ? MXS_PIO_WORDS : NUM_CCW)) {
> > +             dev_err(mxs_dma->dev, "maximum number of sg exceeded: %d > %d\n",
> > +                             sg_len, NUM_CCW);
> > +             ret = -EINVAL;
> > +             goto err_out;
> > +     }
> > +
> > +     if (direction == DMA_NONE) {
> > +             ccw = &mxs_chan->ccw[0];
> > +             pio = (u32 *) sgl;
> > +
> > +             for (j = 0; j < sg_len;)
> > +                     ccw->pio_words[j++] = *pio++;
> > +
> > +             ccw->next = 0;
> > +             ccw->bits.chain = 0;
> > +             ccw->bits.irq = 1;
> > +             ccw->bits.dec_sem = 1;
> > +             ccw->bits.wait4end = flags;
> > +             ccw->bits.halt_on_terminate = 1;
> > +             ccw->bits.terminate_flush = 1;
> > +             ccw->bits.pio_num = sg_len;
> > +             ccw->bits.command = MXS_DMA_NO_XFER;
> 
> Does this have a valid usecase? I would just return some error code
> here. pio_num and pio_words are unused in the driver and I don't think
> a dmaengine driver should have some kind of PIO fallback.
> 
If you happen to have a look at mxs-mmc patch set, you could find it.

> > +     } else {
> > +             for_each_sg(sgl, sg, sg_len, i) {
> > +                     if (sg->length > MAX_XFER_BYTES) {
> > +                             dev_err(mxs_dma->dev, "maximum bytes for sg entry exceeded: %d > %d\n",
> > +                                             sg->length, MAX_XFER_BYTES);
> > +                             ret = -EINVAL;
> > +                             goto err_out;
> > +                     }
> > +
> > +                     ccw = &mxs_chan->ccw[i];
> > +
> > +                     ccw->next = mxs_chan->ccw_phys + sizeof(*ccw) * (i + 1);
> > +                     ccw->bufaddr = sg->dma_address;
> > +                     ccw->bits.xfer_bytes = sg->length;
> > +                     ccw->bits.chain = 1;
> > +                     ccw->bits.irq = 0;
> > +                     ccw->bits.dec_sem = 0;
> > +                     ccw->bits.wait4end = 0;
> > +                     ccw->bits.halt_on_terminate = 1;
> > +                     ccw->bits.terminate_flush = 1;
> > +                     ccw->bits.pio_num = 0;
> > +                     ccw->bits.command = (direction == DMA_FROM_DEVICE) ?
> > +                                             MXS_DMA_WRITE : MXS_DMA_READ;
> > +
> > +                     if (i + 1 == sg_len) {
> > +                             ccw->next = 0;
> > +                             ccw->bits.chain = 0;
> > +                             ccw->bits.irq = 1;
> > +                             ccw->bits.dec_sem = 1;
> > +                             ccw->bits.wait4end = 1;
> > +                     }
> > +             }
> > +     }
> > +
> > +     return &mxs_chan->desc;
> > +
> > +err_out:
> > +     mxs_chan->status = DMA_ERROR;
> > +     return NULL;
> > +}
> > +
> > +
> > +static int __init mxs_dma_probe(struct platform_device *pdev)
> > +{
> > +     const struct platform_device_id *id_entry =
> > +                             platform_get_device_id(pdev);
> > +     struct mxs_dma_engine *mxs_dma;
> > +     struct resource *iores;
> > +     int ret, i;
> > +
> > +     mxs_dma = kzalloc(sizeof(*mxs_dma), GFP_KERNEL);
> > +     if (!mxs_dma)
> > +             return -ENOMEM;
> > +
> > +     mxs_dma->dev = &pdev->dev;
> > +     mxs_dma->dev_id = id_entry->driver_data;
> > +
> > +     iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +
> > +     if (!request_mem_region(iores->start, resource_size(iores),
> > +                             pdev->name)) {
> > +             ret = -EBUSY;
> > +             goto err_request_region;
> > +     }
> > +
> > +     mxs_dma->base = ioremap(iores->start, resource_size(iores));
> > +     if (!mxs_dma->base) {
> > +             ret = -ENOMEM;
> > +             goto err_ioremap;
> > +     }
> > +
> > +     mxs_dma->clk = clk_get(&pdev->dev, NULL);
> > +     if (IS_ERR(mxs_dma->clk)) {
> > +             ret = PTR_ERR(mxs_dma->clk);
> > +             goto err_clk;
> > +     }
> > +
> > +     dma_cap_set(DMA_SLAVE, mxs_dma->dma_device.cap_mask);
> > +     dma_cap_set(DMA_CYCLIC, mxs_dma->dma_device.cap_mask);
> > +
> > +     INIT_LIST_HEAD(&mxs_dma->dma_device.channels);
> > +
> > +     /* Initialize channel parameters */
> > +     for (i = 0; i < MXS_DMA_CHANNELS; i++) {
> > +             struct mxs_dma_chan *mxs_chan = &mxs_dma->mxs_chans[i];
> > +
> > +             mxs_chan->mxs_dma = mxs_dma;
> > +             spin_lock_init(&mxs_chan->lock);
> > +
> > +             mxs_chan->chan.device = &mxs_dma->dma_device;
> > +
> > +             /* Add the channel to mxs_chan list */
> > +             list_add_tail(&mxs_chan->chan.device_node, &mxs_dma->dma_device.channels);
> > +     }
> > +
> > +     ret = mxs_dma_init(mxs_dma);
> > +     if (ret)
> > +             goto err_init;
> > +
> > +     mxs_dma->dma_device.dev = &pdev->dev;
> > +
> > +     /* mxs_dma gets 65535 bytes maximum sg size */
> > +     mxs_dma->dma_device.dev->dma_parms = &mxs_dma->dma_parms;
> > +     dma_set_max_seg_size(mxs_dma->dma_device.dev, MAX_XFER_BYTES);
> > +
> > +     mxs_dma->dma_device.device_alloc_chan_resources = mxs_dma_alloc_chan_resources;
> > +     mxs_dma->dma_device.device_free_chan_resources = mxs_dma_free_chan_resources;
> > +     mxs_dma->dma_device.device_tx_status = mxs_dma_tx_status;
> > +     mxs_dma->dma_device.device_prep_slave_sg = mxs_dma_prep_slave_sg;
> > +     mxs_dma->dma_device.device_prep_dma_cyclic = mxs_dma_prep_dma_cyclic;
> > +     mxs_dma->dma_device.device_control = mxs_dma_control;
> > +     mxs_dma->dma_device.device_issue_pending = mxs_dma_issue_pending;
> > +
> > +     ret = dma_async_device_register(&mxs_dma->dma_device);
> > +     if (ret) {
> > +             dev_err(&pdev->dev, "unable to register\n");
> > +             goto err_dma_register;
> > +     }
> > +
> > +     /*
> > +      * dmaengine clients have to use dma_device.dev_id to filter
> > +      * dma device between apbh and apbx, so need to ensure it is
> > +      * identical to mxs_dma_engine.dev_id.
> > +      */
> > +     if (mxs_dma->dma_device.dev_id != mxs_dma->dev_id) {
> > +             dev_err(&pdev->dev, "dev_id of dma_device %d differs from mxs_dma_engine %d\n",
> > +                             mxs_dma->dma_device.dev_id, mxs_dma->dev_id);
> > +             goto err_init;
> > +     }
> 
> I think it makes more sense to match against device names (apbh vs.
> apbx) in the client's filter function than to rely on exact numbering.
> 
I agree, if there is already a member like dev_name in dma_device.
There is dev_id but no dev_name.  Suggestion on how to use device
name for matching?

Regards,
Shawn

> > +
> > +     dev_info(mxs_dma->dev, "initialized\n");
> > +
> > +     return 0;
> > +
> > +err_init:
> > +     dma_async_device_unregister(&mxs_dma->dma_device);
> > +err_dma_register:
> > +     clk_put(mxs_dma->clk);
> > +err_clk:
> > +     iounmap(mxs_dma->base);
> > +err_ioremap:
> > +     release_mem_region(iores->start, resource_size(iores));
> > +err_request_region:
> > +     kfree(mxs_dma);
> > +     return ret;
> > +}
> > +
> > +static int __exit mxs_dma_remove(struct platform_device *pdev)
> > +{
> > +     return -EBUSY;
> > +}
> > +
> > +static struct platform_device_id mxs_dma_type[] = {
> > +     {
> > +             .name = "mxs-dma-apbh",
> > +             .driver_data = MXS_DMA_APBH,
> > +     }, {
> > +             .name = "mxs-dma-apbx",
> > +             .driver_data = MXS_DMA_APBX,
> > +     }
> > +};
> > +
> > +static struct platform_driver mxs_dma_driver = {
> > +     .driver         = {
> > +             .name   = "mxs-dma",
> > +     },
> > +     .id_table       = mxs_dma_type,
> > +     .remove         = __exit_p(mxs_dma_remove),
> > +};
> > +
> > +static int __init mxs_dma_module_init(void)
> > +{
> > +     return platform_driver_probe(&mxs_dma_driver, mxs_dma_probe);
> > +}
> > +subsys_initcall(mxs_dma_module_init);
> > --
> > 1.7.1
> >
> >
> >
> 
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Sascha Hauer]

Shawn,

On Wed, Feb 09, 2011 at 07:28:34AM +0800, Shawn Guo wrote:
> > > +
> > > +     if (direction == DMA_NONE) {
> > > +             ccw = &mxs_chan->ccw[0];
> > > +             pio = (u32 *) sgl;
> > > +
> > > +             for (j = 0; j < sg_len;)
> > > +                     ccw->pio_words[j++] = *pio++;
> > > +
> > > +             ccw->next = 0;
> > > +             ccw->bits.chain = 0;
> > > +             ccw->bits.irq = 1;
> > > +             ccw->bits.dec_sem = 1;
> > > +             ccw->bits.wait4end = flags;
> > > +             ccw->bits.halt_on_terminate = 1;
> > > +             ccw->bits.terminate_flush = 1;
> > > +             ccw->bits.pio_num = sg_len;
> > > +             ccw->bits.command = MXS_DMA_NO_XFER;
> > 
> > Does this have a valid usecase? I would just return some error code
> > here. pio_num and pio_words are unused in the driver and I don't think
> > a dmaengine driver should have some kind of PIO fallback.
> > 
> If you happen to have a look at mxs-mmc patch set, you could find it.

As Russell already pointed out, the (mmc-) driver should handle this.

> > > +
> > > +     /*
> > > +      * dmaengine clients have to use dma_device.dev_id to filter
> > > +      * dma device between apbh and apbx, so need to ensure it is
> > > +      * identical to mxs_dma_engine.dev_id.
> > > +      */
> > > +     if (mxs_dma->dma_device.dev_id != mxs_dma->dev_id) {
> > > +             dev_err(&pdev->dev, "dev_id of dma_device %d differs from mxs_dma_engine %d\n",
> > > +                             mxs_dma->dma_device.dev_id, mxs_dma->dev_id);
> > > +             goto err_init;
> > > +     }
> > 
> > I think it makes more sense to match against device names (apbh vs.
> > apbx) in the client's filter function than to rely on exact numbering.
> > 
> I agree, if there is already a member like dev_name in dma_device.
> There is dev_id but no dev_name.  Suggestion on how to use device
> name for matching?

Have a look at the implementation of imx_dma_is_general_purpose() and
imx_dma_is_ipu().

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Shawn Guo]

On Wed, Feb 09, 2011 at 09:34:00AM +0100, Sascha Hauer wrote:
> Shawn,
> 
> On Wed, Feb 09, 2011 at 07:28:34AM +0800, Shawn Guo wrote:
> > > > +
> > > > +     if (direction == DMA_NONE) {
> > > > +             ccw = &mxs_chan->ccw[0];
> > > > +             pio = (u32 *) sgl;
> > > > +
> > > > +             for (j = 0; j < sg_len;)
> > > > +                     ccw->pio_words[j++] = *pio++;
> > > > +
> > > > +             ccw->next = 0;
> > > > +             ccw->bits.chain = 0;
> > > > +             ccw->bits.irq = 1;
> > > > +             ccw->bits.dec_sem = 1;
> > > > +             ccw->bits.wait4end = flags;
> > > > +             ccw->bits.halt_on_terminate = 1;
> > > > +             ccw->bits.terminate_flush = 1;
> > > > +             ccw->bits.pio_num = sg_len;
> > > > +             ccw->bits.command = MXS_DMA_NO_XFER;
> > > 
> > > Does this have a valid usecase? I would just return some error code
> > > here. pio_num and pio_words are unused in the driver and I don't think
> > > a dmaengine driver should have some kind of PIO fallback.
> > > 
> > If you happen to have a look at mxs-mmc patch set, you could find it.
> 
> As Russell already pointed out, the (mmc-) driver should handle this.
> 
> > > > +
> > > > +     /*
> > > > +      * dmaengine clients have to use dma_device.dev_id to filter
> > > > +      * dma device between apbh and apbx, so need to ensure it is
> > > > +      * identical to mxs_dma_engine.dev_id.
> > > > +      */
> > > > +     if (mxs_dma->dma_device.dev_id != mxs_dma->dev_id) {
> > > > +             dev_err(&pdev->dev, "dev_id of dma_device %d differs from mxs_dma_engine %d\n",
> > > > +                             mxs_dma->dma_device.dev_id, mxs_dma->dev_id);
> > > > +             goto err_init;
> > > > +     }
> > > 
> > > I think it makes more sense to match against device names (apbh vs.
> > > apbx) in the client's filter function than to rely on exact numbering.
> > > 
> > I agree, if there is already a member like dev_name in dma_device.
> > There is dev_id but no dev_name.  Suggestion on how to use device
> > name for matching?
> 
> Have a look at the implementation of imx_dma_is_general_purpose() and
> imx_dma_is_ipu().
> 
Great example.  Thanks.

Regards
Shawn

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Lothar Waßmann]

Hi,

Shawn Guo writes:
[...]
> diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> new file mode 100644
> index 0000000..8d1e811
> --- /dev/null
> +++ b/drivers/dma/mxs-dma.c
> @@ -0,0 +1,702 @@
[...]
> +static irqreturn_t mxs_dma_int_handler(int irq, void *dev_id)
> +{
> +	struct mxs_dma_engine *mxs_dma = dev_id;
> +	u32 stat1, stat2;
> +
> +	/* completion status */
> +	stat1 = __raw_readl(mxs_dma->base + HW_APBHX_CTRL1);
> +	stat1 &= 0xffff;
> +	__mxs_clrl(stat1, mxs_dma->base + HW_APBHX_CTRL1);
> +
> +	/* error status */
> +	stat2 = __raw_readl(mxs_dma->base + HW_APBHX_CTRL2);
> +	__mxs_clrl(stat2, mxs_dma->base + HW_APBHX_CTRL2);
> +
> +	/*
> +	 * When both completion and error of termination bits set at the
> +	 * same time, we do not take it as an error.  IOW, it only becomes
> +	 * an error we need to handler here in case of ether it's an bus
> +	 * error or a termination error with no completion.
> +	 */
> +	stat2 = ((stat2 >> 16) & stat2) |	   /* bus error */
> +		(~(stat2 >> 16) & stat2 & ~stat1); /* termination with no completion */
> +
> +	/* combine error and completion status for checking */
> +	stat1 = (stat2 << 16) | stat1;
> +	while (stat1) {
> +		int channel = fls(stat1) - 1;
> +		struct mxs_dma_chan *mxs_chan =
> +				&mxs_dma->mxs_chans[channel % 16];
> +
> +		if (channel >= 16) {
> +			dev_dbg(mxs_dma->dev, "%s: error in channel %d\n",
> +						__func__, channel - 16);
> +			mxs_dma_reset_chan(mxs_chan);
> +			mxs_chan->status = DMA_ERROR;
> +		} else {
> +			if (mxs_chan->flags & MXS_DMA_SG_LOOP)
> +				mxs_chan->status = DMA_IN_PROGRESS;
> +			else
> +				mxs_chan->status = DMA_SUCCESS;
> +		}
> +
> +		stat1 &= ~(1 << channel);
> +
> +		if (mxs_chan->desc.callback)
> +			mxs_chan->desc.callback(mxs_chan->desc.callback_param);
> +
> +		if (mxs_chan->status == DMA_SUCCESS)
> +			mxs_chan->last_completed = mxs_chan->desc.cookie;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
> +{
> +	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> +	struct mxs_dma_data *data = chan->private;
> +	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> +	static unsigned long flags;
> +	int ret;
> +
> +	if (!data)
> +		return -EINVAL;
> +
> +	mxs_chan->chan_irq = data->chan_irq;
> +
> +	mxs_chan->ccw = dma_alloc_coherent(NULL, PAGE_SIZE,
> +				&mxs_chan->ccw_phys, GFP_KERNEL);
> +	if (!mxs_chan->ccw) {
> +		ret = -ENOMEM;
> +		goto err_alloc;
> +	}
> +
> +	memset(mxs_chan->ccw, 0, PAGE_SIZE);
> +
> +	ret = request_irq(mxs_chan->chan_irq, mxs_dma_int_handler,
> +				flags, "mxs-dma", mxs_dma);
> +	if (ret)
> +		goto err_irq;
> +
> +	flags = IRQF_SHARED;
> +
Apart from the fact, that this is initilized after use, the use of
IRQF_SHARED is wrong here. Shared interrupt handlers are for
multiple handlers sharing a single interrupt source, not for multiple
interrupt sources sharing the same handler!
A shared handler must return IRQ_NONE, if it detects that the
interrupt was from a source it does not handle.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Shawn Guo]

Hi Lothar,

On Mon, Feb 07, 2011 at 03:13:50PM +0100, Lothar Wa?mann wrote:
> Hi,
> 
> Shawn Guo writes:
> [...]
> > diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> > new file mode 100644
> > index 0000000..8d1e811
> > --- /dev/null
> > +++ b/drivers/dma/mxs-dma.c
> > @@ -0,0 +1,702 @@
> [...]
> > +static irqreturn_t mxs_dma_int_handler(int irq, void *dev_id)
> > +{
> > +	struct mxs_dma_engine *mxs_dma = dev_id;
> > +	u32 stat1, stat2;
> > +
> > +	/* completion status */
> > +	stat1 = __raw_readl(mxs_dma->base + HW_APBHX_CTRL1);
> > +	stat1 &= 0xffff;
> > +	__mxs_clrl(stat1, mxs_dma->base + HW_APBHX_CTRL1);
> > +
> > +	/* error status */
> > +	stat2 = __raw_readl(mxs_dma->base + HW_APBHX_CTRL2);
> > +	__mxs_clrl(stat2, mxs_dma->base + HW_APBHX_CTRL2);
> > +
> > +	/*
> > +	 * When both completion and error of termination bits set at the
> > +	 * same time, we do not take it as an error.  IOW, it only becomes
> > +	 * an error we need to handler here in case of ether it's an bus
> > +	 * error or a termination error with no completion.
> > +	 */
> > +	stat2 = ((stat2 >> 16) & stat2) |	   /* bus error */
> > +		(~(stat2 >> 16) & stat2 & ~stat1); /* termination with no completion */
> > +
> > +	/* combine error and completion status for checking */
> > +	stat1 = (stat2 << 16) | stat1;
> > +	while (stat1) {
> > +		int channel = fls(stat1) - 1;
> > +		struct mxs_dma_chan *mxs_chan =
> > +				&mxs_dma->mxs_chans[channel % 16];
> > +
> > +		if (channel >= 16) {
> > +			dev_dbg(mxs_dma->dev, "%s: error in channel %d\n",
> > +						__func__, channel - 16);
> > +			mxs_dma_reset_chan(mxs_chan);
> > +			mxs_chan->status = DMA_ERROR;
> > +		} else {
> > +			if (mxs_chan->flags & MXS_DMA_SG_LOOP)
> > +				mxs_chan->status = DMA_IN_PROGRESS;
> > +			else
> > +				mxs_chan->status = DMA_SUCCESS;
> > +		}
> > +
> > +		stat1 &= ~(1 << channel);
> > +
> > +		if (mxs_chan->desc.callback)
> > +			mxs_chan->desc.callback(mxs_chan->desc.callback_param);
> > +
> > +		if (mxs_chan->status == DMA_SUCCESS)
> > +			mxs_chan->last_completed = mxs_chan->desc.cookie;
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
> > +{
> > +	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> > +	struct mxs_dma_data *data = chan->private;
> > +	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> > +	static unsigned long flags;
> > +	int ret;
> > +
> > +	if (!data)
> > +		return -EINVAL;
> > +
> > +	mxs_chan->chan_irq = data->chan_irq;
> > +
> > +	mxs_chan->ccw = dma_alloc_coherent(NULL, PAGE_SIZE,
> > +				&mxs_chan->ccw_phys, GFP_KERNEL);
> > +	if (!mxs_chan->ccw) {
> > +		ret = -ENOMEM;
> > +		goto err_alloc;
> > +	}
> > +
> > +	memset(mxs_chan->ccw, 0, PAGE_SIZE);
> > +
> > +	ret = request_irq(mxs_chan->chan_irq, mxs_dma_int_handler,
> > +				flags, "mxs-dma", mxs_dma);
> > +	if (ret)
> > +		goto err_irq;
> > +
> > +	flags = IRQF_SHARED;
> > +
> Apart from the fact, that this is initilized after use, the use of
> IRQF_SHARED is wrong here. Shared interrupt handlers are for
> multiple handlers sharing a single interrupt source, not for multiple
> interrupt sources sharing the same handler!
> A shared handler must return IRQ_NONE, if it detects that the
> interrupt was from a source it does not handle.
> 
My bad.  Thanks for pointing this out.

Regards,
Shawn

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Lothar Waßmann]

Hi,

Shawn Guo writes:
> This patch adds dma support for Freescale MXS-based SoC i.MX23/28,
> including apbh-dma and apbx-dma.
> 
> * apbh-dma and apbx-dma are supported in the driver as two instances,
>   and have to be filtered by dma clients via device id.  It becomes
>   the convention that apbh-dma always gets registered prior to
>   apbx-dma.
> 
> * apbh-dma is different between mx23 and mx28, hardware version
>   register is used to handle the differences.
> 
> * Every the mxs dma channel is statically assigned to client device
>   by soc design with fixed irq.  The irq number is being passed by
>   alloc_chan function with mxs_dma_data, and client driver has to
>   filter the correct channel by its channel id.
> 
> * mxs-dma supports pio function besides data transfer.  The driver
>   uses dma_data_direction DMA_NONE to identify the pio mode, and
>   steals sgl and sg_len to get pio words and numbers from clients.
> 
> * mxs dmaengine has some very specific features, like sense function
>   and the special NAND support (nand_lock, nand_wait4ready).  These
>   are too specific to implemented in generic dmaengine driver.
> 
> * The parameter "flags" of prep functions is currently being used to
>   pass wait4end flag from clients.
> 
> * The driver refers to imx-sdma and only a single descriptor is
>   statically assigned to each channel.
> 
> Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> ---
>  arch/arm/mach-mxs/include/mach/dma.h |   16 +
>  drivers/dma/Kconfig                  |    8 +
>  drivers/dma/Makefile                 |    1 +
>  drivers/dma/mxs-dma.c                |  702 ++++++++++++++++++++++++++++++++++
>  4 files changed, 727 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/mach-mxs/include/mach/dma.h
>  create mode 100644 drivers/dma/mxs-dma.c
> 
> diff --git a/arch/arm/mach-mxs/include/mach/dma.h b/arch/arm/mach-mxs/include/mach/dma.h
> new file mode 100644
> index 0000000..429f431
> --- /dev/null
> +++ b/arch/arm/mach-mxs/include/mach/dma.h
> @@ -0,0 +1,16 @@
> +/*
> + * Copyright 2011 Freescale Semiconductor, Inc. All Rights Reserved.
> + *
> + * 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.
> + */
> +
> +#ifndef __MACH_MXS_DMA_H__
> +#define __MACH_MXS_DMA_H__
> +
> +struct mxs_dma_data {
> +	int chan_irq;
> +};
>
There is a name clash of this structure with the name of a different
structure defined in arch/arm/mach-mxs/include/mach/devices-common.h
(your patch 3/5):
|diff --git a/arch/arm/mach-mxs/include/mach/devices-common.h b/arch/arm/mach-mxs/include/mach/devices-common.h
|index e7aefb4..acd74b4 100644
|--- a/arch/arm/mach-mxs/include/mach/devices-common.h
|+++ b/arch/arm/mach-mxs/include/mach/devices-common.h
|@@ -40,6 +40,13 @@ struct mxs_auart_data {
| struct platform_device *__init mxs_add_auart(
| 		const struct mxs_auart_data *data);
| 
|+/* dma */
|+struct mxs_dma_data {
|+	const char *devid;
|+	resource_size_t iobase;
|+};
|+struct platform_device *__init mxs_add_dma(const struct mxs_dma_data *data);


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Shawn Guo]

On Mon, Feb 07, 2011 at 03:31:04PM +0100, Lothar Wa?mann wrote:
> Hi,
> 
> Shawn Guo writes:
> > This patch adds dma support for Freescale MXS-based SoC i.MX23/28,
> > including apbh-dma and apbx-dma.
> > 
> > * apbh-dma and apbx-dma are supported in the driver as two instances,
> >   and have to be filtered by dma clients via device id.  It becomes
> >   the convention that apbh-dma always gets registered prior to
> >   apbx-dma.
> > 
> > * apbh-dma is different between mx23 and mx28, hardware version
> >   register is used to handle the differences.
> > 
> > * Every the mxs dma channel is statically assigned to client device
> >   by soc design with fixed irq.  The irq number is being passed by
> >   alloc_chan function with mxs_dma_data, and client driver has to
> >   filter the correct channel by its channel id.
> > 
> > * mxs-dma supports pio function besides data transfer.  The driver
> >   uses dma_data_direction DMA_NONE to identify the pio mode, and
> >   steals sgl and sg_len to get pio words and numbers from clients.
> > 
> > * mxs dmaengine has some very specific features, like sense function
> >   and the special NAND support (nand_lock, nand_wait4ready).  These
> >   are too specific to implemented in generic dmaengine driver.
> > 
> > * The parameter "flags" of prep functions is currently being used to
> >   pass wait4end flag from clients.
> > 
> > * The driver refers to imx-sdma and only a single descriptor is
> >   statically assigned to each channel.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > ---
> >  arch/arm/mach-mxs/include/mach/dma.h |   16 +
> >  drivers/dma/Kconfig                  |    8 +
> >  drivers/dma/Makefile                 |    1 +
> >  drivers/dma/mxs-dma.c                |  702 ++++++++++++++++++++++++++++++++++
> >  4 files changed, 727 insertions(+), 0 deletions(-)
> >  create mode 100644 arch/arm/mach-mxs/include/mach/dma.h
> >  create mode 100644 drivers/dma/mxs-dma.c
> > 
> > diff --git a/arch/arm/mach-mxs/include/mach/dma.h b/arch/arm/mach-mxs/include/mach/dma.h
> > new file mode 100644
> > index 0000000..429f431
> > --- /dev/null
> > +++ b/arch/arm/mach-mxs/include/mach/dma.h
> > @@ -0,0 +1,16 @@
> > +/*
> > + * Copyright 2011 Freescale Semiconductor, Inc. All Rights Reserved.
> > + *
> > + * 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.
> > + */
> > +
> > +#ifndef __MACH_MXS_DMA_H__
> > +#define __MACH_MXS_DMA_H__
> > +
> > +struct mxs_dma_data {
> > +	int chan_irq;
> > +};
> >
> There is a name clash of this structure with the name of a different
> structure defined in arch/arm/mach-mxs/include/mach/devices-common.h
> (your patch 3/5):
> |diff --git a/arch/arm/mach-mxs/include/mach/devices-common.h b/arch/arm/mach-mxs/include/mach/devices-common.h
> |index e7aefb4..acd74b4 100644
> |--- a/arch/arm/mach-mxs/include/mach/devices-common.h
> |+++ b/arch/arm/mach-mxs/include/mach/devices-common.h
> |@@ -40,6 +40,13 @@ struct mxs_auart_data {
> | struct platform_device *__init mxs_add_auart(
> | 		const struct mxs_auart_data *data);
> | 
> |+/* dma */
> |+struct mxs_dma_data {
> |+	const char *devid;
> |+	resource_size_t iobase;
> |+};
> |+struct platform_device *__init mxs_add_dma(const struct mxs_dma_data *data);
> 
Good catch.  Thanks.

Regards,
Shawn

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Lothar Waßmann]

Hi,

> On Sat, Feb 05, 2011 at 10:08:12AM +0800, Shawn Guo wrote:
>> This patch adds dma support for Freescale MXS-based SoC i.MX23/28,
>> including apbh-dma and apbx-dma.
>> 
>> * apbh-dma and apbx-dma are supported in the driver as two instances,
>>   and have to be filtered by dma clients via device id.  It becomes
>>   the convention that apbh-dma always gets registered prior to
>>   apbx-dma.
>> 
>> * apbh-dma is different between mx23 and mx28, hardware version
>>   register is used to handle the differences.
>> 
>> * Every the mxs dma channel is statically assigned to client device
>>   by soc design with fixed irq.  The irq number is being passed by
>>   alloc_chan function with mxs_dma_data, and client driver has to
>>   filter the correct channel by its channel id.
>> 
>> * mxs-dma supports pio function besides data transfer.  The driver
>>   uses dma_data_direction DMA_NONE to identify the pio mode, and
>>   steals sgl and sg_len to get pio words and numbers from clients.
>> 
>> * mxs dmaengine has some very specific features, like sense function
>>   and the special NAND support (nand_lock, nand_wait4ready).  These
>>   are too specific to implemented in generic dmaengine driver.
>> 
>> * The parameter "flags" of prep functions is currently being used to
>>   pass wait4end flag from clients.
>> 
>> * The driver refers to imx-sdma and only a single descriptor is
>>   statically assigned to each channel.
>> 
>> Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
>> ---
>>  arch/arm/mach-mxs/include/mach/dma.h |   16 +
>>  drivers/dma/Kconfig                  |    8 +
>>  drivers/dma/Makefile                 |    1 +
>>  drivers/dma/mxs-dma.c                |  702 ++++++++++++++++++++++++++++++++++
>>  4 files changed, 727 insertions(+), 0 deletions(-)
>>  create mode 100644 arch/arm/mach-mxs/include/mach/dma.h
>>  create mode 100644 drivers/dma/mxs-dma.c
>> 
>> diff --git a/arch/arm/mach-mxs/include/mach/dma.h b/arch/arm/mach-mxs/include/mach/dma.h
>> new file mode 100644
>> index 0000000..429f431
>> --- /dev/null
>> +++ b/arch/arm/mach-mxs/include/mach/dma.h
>> +
>> +static struct dma_async_tx_descriptor *mxs_dma_prep_slave_sg(
>> +		struct dma_chan *chan, struct scatterlist *sgl,
>> +		unsigned int sg_len, enum dma_data_direction direction,
>> +		unsigned long flags)
>> +{
>> +	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
>> +	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
>> +	struct mxs_dma_ccw *ccw;
>> +	struct scatterlist *sg;
>> +	int ret, i, j;
>> +	u32 *pio;
>> +
>> +	dev_dbg(mxs_dma->dev, "%s: channel %d\n", __func__, chan->chan_id);
>> +
>> +	if (mxs_chan->status == DMA_IN_PROGRESS)
>> +		return NULL;
>> +
>> +	mxs_chan->status = DMA_IN_PROGRESS;
>> +	mxs_chan->flags = 0;
>> +
>> +	dev_dbg(mxs_dma->dev, "%s: setting up %d entries\n", __func__, sg_len);
>> +
>> +	if (sg_len > ((direction == DMA_NONE) ? MXS_PIO_WORDS : NUM_CCW)) {
>> +		dev_err(mxs_dma->dev, "maximum number of sg exceeded: %d > %d\n",
>> +				sg_len, NUM_CCW);
>> +		ret = -EINVAL;
>> +		goto err_out;
>> +	}
>> +
>> +	if (direction == DMA_NONE) {
>> +		ccw = &mxs_chan->ccw[0];
>> +		pio = (u32 *) sgl;
>> +
>> +		for (j = 0; j < sg_len;)
>> +			ccw->pio_words[j++] = *pio++;
>> +
>> +		ccw->next = 0;
>> +		ccw->bits.chain = 0;
>> +		ccw->bits.irq = 1;
>> +		ccw->bits.dec_sem = 1;
>> +		ccw->bits.wait4end = flags;
>> +		ccw->bits.halt_on_terminate = 1;
>> +		ccw->bits.terminate_flush = 1;
>> +		ccw->bits.pio_num = sg_len;
>> +		ccw->bits.command = MXS_DMA_NO_XFER;
>
> Does this have a valid usecase? I would just return some error code
> here. pio_num and pio_words are unused in the driver and I don't think
> a dmaengine driver should have some kind of PIO fallback.
>
Actually 'PIO' is a misnomer here. It's the free scaled way of
implementing a simple feature (chained DMA with mixed transfer modes)
in a complicated and obfuscated way.

What's behinde the 'PIO' transfers is programming controller registers
via DMA along with the actual DMA data transfer. DMA_NONE simply
means, that the DMA transfer does only the register programming but
does not transfer any payload. The 'pio_words' are the values that are
being written to consecutive locations of e.g. the SPI controller
register address space.
The programming is actually done by DMA, in any case.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Shawn Guo]

On Tue, Feb 08, 2011 at 03:41:55PM +0100, Lothar Wa?mann wrote:
> Hi,
> 
> > On Sat, Feb 05, 2011 at 10:08:12AM +0800, Shawn Guo wrote:
> >> This patch adds dma support for Freescale MXS-based SoC i.MX23/28,
> >> including apbh-dma and apbx-dma.
> >> 
> >> * apbh-dma and apbx-dma are supported in the driver as two instances,
> >>   and have to be filtered by dma clients via device id.  It becomes
> >>   the convention that apbh-dma always gets registered prior to
> >>   apbx-dma.
> >> 
> >> * apbh-dma is different between mx23 and mx28, hardware version
> >>   register is used to handle the differences.
> >> 
> >> * Every the mxs dma channel is statically assigned to client device
> >>   by soc design with fixed irq.  The irq number is being passed by
> >>   alloc_chan function with mxs_dma_data, and client driver has to
> >>   filter the correct channel by its channel id.
> >> 
> >> * mxs-dma supports pio function besides data transfer.  The driver
> >>   uses dma_data_direction DMA_NONE to identify the pio mode, and
> >>   steals sgl and sg_len to get pio words and numbers from clients.
> >> 
> >> * mxs dmaengine has some very specific features, like sense function
> >>   and the special NAND support (nand_lock, nand_wait4ready).  These
> >>   are too specific to implemented in generic dmaengine driver.
> >> 
> >> * The parameter "flags" of prep functions is currently being used to
> >>   pass wait4end flag from clients.
> >> 
> >> * The driver refers to imx-sdma and only a single descriptor is
> >>   statically assigned to each channel.
> >> 
> >> Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> >> ---
> >>  arch/arm/mach-mxs/include/mach/dma.h |   16 +
> >>  drivers/dma/Kconfig                  |    8 +
> >>  drivers/dma/Makefile                 |    1 +
> >>  drivers/dma/mxs-dma.c                |  702 ++++++++++++++++++++++++++++++++++
> >>  4 files changed, 727 insertions(+), 0 deletions(-)
> >>  create mode 100644 arch/arm/mach-mxs/include/mach/dma.h
> >>  create mode 100644 drivers/dma/mxs-dma.c
> >> 
> >> diff --git a/arch/arm/mach-mxs/include/mach/dma.h b/arch/arm/mach-mxs/include/mach/dma.h
> >> new file mode 100644
> >> index 0000000..429f431
> >> --- /dev/null
> >> +++ b/arch/arm/mach-mxs/include/mach/dma.h
> >> +
> >> +static struct dma_async_tx_descriptor *mxs_dma_prep_slave_sg(
> >> +		struct dma_chan *chan, struct scatterlist *sgl,
> >> +		unsigned int sg_len, enum dma_data_direction direction,
> >> +		unsigned long flags)
> >> +{
> >> +	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> >> +	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> >> +	struct mxs_dma_ccw *ccw;
> >> +	struct scatterlist *sg;
> >> +	int ret, i, j;
> >> +	u32 *pio;
> >> +
> >> +	dev_dbg(mxs_dma->dev, "%s: channel %d\n", __func__, chan->chan_id);
> >> +
> >> +	if (mxs_chan->status == DMA_IN_PROGRESS)
> >> +		return NULL;
> >> +
> >> +	mxs_chan->status = DMA_IN_PROGRESS;
> >> +	mxs_chan->flags = 0;
> >> +
> >> +	dev_dbg(mxs_dma->dev, "%s: setting up %d entries\n", __func__, sg_len);
> >> +
> >> +	if (sg_len > ((direction == DMA_NONE) ? MXS_PIO_WORDS : NUM_CCW)) {
> >> +		dev_err(mxs_dma->dev, "maximum number of sg exceeded: %d > %d\n",
> >> +				sg_len, NUM_CCW);
> >> +		ret = -EINVAL;
> >> +		goto err_out;
> >> +	}
> >> +
> >> +	if (direction == DMA_NONE) {
> >> +		ccw = &mxs_chan->ccw[0];
> >> +		pio = (u32 *) sgl;
> >> +
> >> +		for (j = 0; j < sg_len;)
> >> +			ccw->pio_words[j++] = *pio++;
> >> +
> >> +		ccw->next = 0;
> >> +		ccw->bits.chain = 0;
> >> +		ccw->bits.irq = 1;
> >> +		ccw->bits.dec_sem = 1;
> >> +		ccw->bits.wait4end = flags;
> >> +		ccw->bits.halt_on_terminate = 1;
> >> +		ccw->bits.terminate_flush = 1;
> >> +		ccw->bits.pio_num = sg_len;
> >> +		ccw->bits.command = MXS_DMA_NO_XFER;
> >
> > Does this have a valid usecase? I would just return some error code
> > here. pio_num and pio_words are unused in the driver and I don't think
> > a dmaengine driver should have some kind of PIO fallback.
> >
> Actually 'PIO' is a misnomer here. It's the free scaled way of
> implementing a simple feature (chained DMA with mixed transfer modes)
> in a complicated and obfuscated way.
> 
> What's behinde the 'PIO' transfers is programming controller registers
> via DMA along with the actual DMA data transfer. DMA_NONE simply
> means, that the DMA transfer does only the register programming but
> does not transfer any payload. The 'pio_words' are the values that are
> being written to consecutive locations of e.g. the SPI controller
> register address space.
> The programming is actually done by DMA, in any case.
> 
I'm waiting for this reply ;)

i.MX23/28 Reference Manual uses word "PIO" for the working mode that
Lothar has explained.  It seems that "PIO" in mxs-dma needs some more
documents.

It's true that mxs dma hardware is designed to program peripheral
registers along with data transfer with ccw chain.  But it's hard
for generic dmaengine model to implement that.  The client device
driver gets the data in scatter-gather list to transfer.  It requires
client driver to manipulate the sgl to get pio ccw inserted properly
to get the "along with" implemented.  This is not a reasonable
implementation to me.

I still chose to keep the pio mode in the implementation in "single
step" rather than "along with" way.  That means client driver has to
issue one dma request to program client device registers, and issue
another one to transfer data.  The natural thought is that the pio
support can totally be saved with cpu programming.  But looking at
any mxs dma client device in reference manual, you will find it gets
two irq lines, irq_dma and irq_error.  For ssp (mmc) example, when
one mmc command is issued and completed without error, you have to
either polling ssp status register or use pio dma and irq_dma
interrupt to know the completion.  That's to say I keep the pio
support in single ccw way to help client device driver utilize the
interrupt capability somehow.

Actually, besides the pio mode, mxs dma hardware has some other
supports that are incompatible with dmaengine driver model, like
sense command and some nand specific supports.  I simply chose not
implement them.

Regards,
Shawn

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Lothar Waßmann]

Hi Shawn,

Shawn Guo writes:
> On Tue, Feb 08, 2011 at 03:41:55PM +0100, Lothar Wa?mann wrote:
[...]
> > What's behinde the 'PIO' transfers is programming controller registers
> > via DMA along with the actual DMA data transfer. DMA_NONE simply
> > means, that the DMA transfer does only the register programming but
> > does not transfer any payload. The 'pio_words' are the values that are
> > being written to consecutive locations of e.g. the SPI controller
> > register address space.
> > The programming is actually done by DMA, in any case.
> > 
> I'm waiting for this reply ;)
> 
> i.MX23/28 Reference Manual uses word "PIO" for the working mode that
> Lothar has explained.  It seems that "PIO" in mxs-dma needs some more
> documents.
> 
> It's true that mxs dma hardware is designed to program peripheral
> registers along with data transfer with ccw chain.  But it's hard
> for generic dmaengine model to implement that.  The client device
> driver gets the data in scatter-gather list to transfer.  It requires
> client driver to manipulate the sgl to get pio ccw inserted properly
> to get the "along with" implemented.  This is not a reasonable
> implementation to me.
> 
> I still chose to keep the pio mode in the implementation in "single
> step" rather than "along with" way.  That means client driver has to
> issue one dma request to program client device registers, and issue
> another one to transfer data.  The natural thought is that the pio
>
which defeats the purpose of the whole thing. If the software has to
issue a single request for DMA to program the registers it could as
well directly write the registers without any DMA. The purpose of the
embedded 'PIO' transfers is, that you can set up a whole chain of
commands and associated data transfers and have that executed
without any further software intervention.

> support can totally be saved with cpu programming.  But looking at
> any mxs dma client device in reference manual, you will find it gets
> two irq lines, irq_dma and irq_error.  For ssp (mmc) example, when
> one mmc command is issued and completed without error, you have to
> either polling ssp status register or use pio dma and irq_dma
> interrupt to know the completion.  That's to say I keep the pio
>
You could use the SSP IRQ as well.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Shawn Guo]

On Wed, Feb 09, 2011 at 09:06:16AM +0100, Lothar Wa?mann wrote:
> Hi Shawn,
> 
> Shawn Guo writes:
> > On Tue, Feb 08, 2011 at 03:41:55PM +0100, Lothar Wa?mann wrote:
> [...]
> > > What's behinde the 'PIO' transfers is programming controller registers
> > > via DMA along with the actual DMA data transfer. DMA_NONE simply
> > > means, that the DMA transfer does only the register programming but
> > > does not transfer any payload. The 'pio_words' are the values that are
> > > being written to consecutive locations of e.g. the SPI controller
> > > register address space.
> > > The programming is actually done by DMA, in any case.
> > > 
> > I'm waiting for this reply ;)
> > 
> > i.MX23/28 Reference Manual uses word "PIO" for the working mode that
> > Lothar has explained.  It seems that "PIO" in mxs-dma needs some more
> > documents.
> > 
> > It's true that mxs dma hardware is designed to program peripheral
> > registers along with data transfer with ccw chain.  But it's hard
> > for generic dmaengine model to implement that.  The client device
> > driver gets the data in scatter-gather list to transfer.  It requires
> > client driver to manipulate the sgl to get pio ccw inserted properly
> > to get the "along with" implemented.  This is not a reasonable
> > implementation to me.
> > 
> > I still chose to keep the pio mode in the implementation in "single
> > step" rather than "along with" way.  That means client driver has to
> > issue one dma request to program client device registers, and issue
> > another one to transfer data.  The natural thought is that the pio
> >
> which defeats the purpose of the whole thing. If the software has to
> issue a single request for DMA to program the registers it could as
> well directly write the registers without any DMA. The purpose of the
> embedded 'PIO' transfers is, that you can set up a whole chain of
> commands and associated data transfers and have that executed
> without any further software intervention.
> 
> > support can totally be saved with cpu programming.  But looking at
> > any mxs dma client device in reference manual, you will find it gets
> > two irq lines, irq_dma and irq_error.  For ssp (mmc) example, when
> > one mmc command is issued and completed without error, you have to
> > either polling ssp status register or use pio dma and irq_dma
> > interrupt to know the completion.  That's to say I keep the pio
> >
> You could use the SSP IRQ as well.
> 
No, I could not.  The SSP IRQ (e.g. MX28_INT_SSP0_ERROR) is designed
to indicate an error condition than the completion of mmc command.

Regards,
Shawn

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Lothar Waßmann]

Hi,

Shawn Guo writes:
> On Wed, Feb 09, 2011 at 09:06:16AM +0100, Lothar Wa?mann wrote:
[...]
> > You could use the SSP IRQ as well.
> > 
> No, I could not.  The SSP IRQ (e.g. MX28_INT_SSP0_ERROR) is designed
> to indicate an error condition than the completion of mmc command.
> 
Ah, ok. That's the 'advantage' of not having a dedicated MMC/SD
controller, but adding some MMC capabilities to the SSP controller.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Shawn Guo]

On Wed, Feb 09, 2011 at 09:06:16AM +0100, Lothar Wa?mann wrote:
> Hi Shawn,
> 
> Shawn Guo writes:
> > On Tue, Feb 08, 2011 at 03:41:55PM +0100, Lothar Wa?mann wrote:
> [...]
> > > What's behinde the 'PIO' transfers is programming controller registers
> > > via DMA along with the actual DMA data transfer. DMA_NONE simply
> > > means, that the DMA transfer does only the register programming but
> > > does not transfer any payload. The 'pio_words' are the values that are
> > > being written to consecutive locations of e.g. the SPI controller
> > > register address space.
> > > The programming is actually done by DMA, in any case.
> > > 
> > I'm waiting for this reply ;)
> > 
> > i.MX23/28 Reference Manual uses word "PIO" for the working mode that
> > Lothar has explained.  It seems that "PIO" in mxs-dma needs some more
> > documents.
> > 
> > It's true that mxs dma hardware is designed to program peripheral
> > registers along with data transfer with ccw chain.  But it's hard
> > for generic dmaengine model to implement that.  The client device
> > driver gets the data in scatter-gather list to transfer.  It requires
> > client driver to manipulate the sgl to get pio ccw inserted properly
> > to get the "along with" implemented.  This is not a reasonable
> > implementation to me.
> > 
> > I still chose to keep the pio mode in the implementation in "single
> > step" rather than "along with" way.  That means client driver has to
> > issue one dma request to program client device registers, and issue
> > another one to transfer data.  The natural thought is that the pio
> >
> which defeats the purpose of the whole thing. If the software has to
> issue a single request for DMA to program the registers it could as
> well directly write the registers without any DMA. The purpose of the
> embedded 'PIO' transfers is, that you can set up a whole chain of
> commands and associated data transfers and have that executed
> without any further software intervention.
> 

I'm redefining the flags of dma prep function as whether the current
sg list should be appended to the last one in the ccw chain, so that
data transfer can be done along with pio controller register.  Please
see mxs-dma v2 coming soon for details.

Regards,
Shawn

> > support can totally be saved with cpu programming.  But looking at
> > any mxs dma client device in reference manual, you will find it gets
> > two irq lines, irq_dma and irq_error.  For ssp (mmc) example, when
> > one mmc command is issued and completed without error, you have to
> > either polling ssp status register or use pio dma and irq_dma
> > interrupt to know the completion.  That's to say I keep the pio
> >
> You could use the SSP IRQ as well.
> 
> 
> Lothar Wa?mann
> -- 
> ___________________________________________________________
> 
> Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
> Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
> Gesch?ftsf?hrer: Matthias Kaussen
> Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
> 
> www.karo-electronics.de | info at karo-electronics.de
> ___________________________________________________________
> 

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Sascha Hauer]

On Tue, Feb 08, 2011 at 03:41:55PM +0100, Lothar Wa?mann wrote:
> Hi,
> 
> > On Sat, Feb 05, 2011 at 10:08:12AM +0800, Shawn Guo wrote:
> >> This patch adds dma support for Freescale MXS-based SoC i.MX23/28,
> >> including apbh-dma and apbx-dma.
> >> 
> >> * apbh-dma and apbx-dma are supported in the driver as two instances,
> >>   and have to be filtered by dma clients via device id.  It becomes
> >>   the convention that apbh-dma always gets registered prior to
> >>   apbx-dma.
> >> 
> >> * apbh-dma is different between mx23 and mx28, hardware version
> >>   register is used to handle the differences.
> >> 
> >> * Every the mxs dma channel is statically assigned to client device
> >>   by soc design with fixed irq.  The irq number is being passed by
> >>   alloc_chan function with mxs_dma_data, and client driver has to
> >>   filter the correct channel by its channel id.
> >> 
> >> * mxs-dma supports pio function besides data transfer.  The driver
> >>   uses dma_data_direction DMA_NONE to identify the pio mode, and
> >>   steals sgl and sg_len to get pio words and numbers from clients.
> >> 
> >> * mxs dmaengine has some very specific features, like sense function
> >>   and the special NAND support (nand_lock, nand_wait4ready).  These
> >>   are too specific to implemented in generic dmaengine driver.
> >> 
> >> * The parameter "flags" of prep functions is currently being used to
> >>   pass wait4end flag from clients.
> >> 
> >> * The driver refers to imx-sdma and only a single descriptor is
> >>   statically assigned to each channel.
> >> 
> >> Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> >> ---
> >>  arch/arm/mach-mxs/include/mach/dma.h |   16 +
> >>  drivers/dma/Kconfig                  |    8 +
> >>  drivers/dma/Makefile                 |    1 +
> >>  drivers/dma/mxs-dma.c                |  702 ++++++++++++++++++++++++++++++++++
> >>  4 files changed, 727 insertions(+), 0 deletions(-)
> >>  create mode 100644 arch/arm/mach-mxs/include/mach/dma.h
> >>  create mode 100644 drivers/dma/mxs-dma.c
> >> 
> >> diff --git a/arch/arm/mach-mxs/include/mach/dma.h b/arch/arm/mach-mxs/include/mach/dma.h
> >> new file mode 100644
> >> index 0000000..429f431
> >> --- /dev/null
> >> +++ b/arch/arm/mach-mxs/include/mach/dma.h
> >> +
> >> +static struct dma_async_tx_descriptor *mxs_dma_prep_slave_sg(
> >> +		struct dma_chan *chan, struct scatterlist *sgl,
> >> +		unsigned int sg_len, enum dma_data_direction direction,
> >> +		unsigned long flags)
> >> +{
> >> +	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> >> +	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> >> +	struct mxs_dma_ccw *ccw;
> >> +	struct scatterlist *sg;
> >> +	int ret, i, j;
> >> +	u32 *pio;
> >> +
> >> +	dev_dbg(mxs_dma->dev, "%s: channel %d\n", __func__, chan->chan_id);
> >> +
> >> +	if (mxs_chan->status == DMA_IN_PROGRESS)
> >> +		return NULL;
> >> +
> >> +	mxs_chan->status = DMA_IN_PROGRESS;
> >> +	mxs_chan->flags = 0;
> >> +
> >> +	dev_dbg(mxs_dma->dev, "%s: setting up %d entries\n", __func__, sg_len);
> >> +
> >> +	if (sg_len > ((direction == DMA_NONE) ? MXS_PIO_WORDS : NUM_CCW)) {
> >> +		dev_err(mxs_dma->dev, "maximum number of sg exceeded: %d > %d\n",
> >> +				sg_len, NUM_CCW);
> >> +		ret = -EINVAL;
> >> +		goto err_out;
> >> +	}
> >> +
> >> +	if (direction == DMA_NONE) {
> >> +		ccw = &mxs_chan->ccw[0];
> >> +		pio = (u32 *) sgl;
> >> +
> >> +		for (j = 0; j < sg_len;)
> >> +			ccw->pio_words[j++] = *pio++;
> >> +
> >> +		ccw->next = 0;
> >> +		ccw->bits.chain = 0;
> >> +		ccw->bits.irq = 1;
> >> +		ccw->bits.dec_sem = 1;
> >> +		ccw->bits.wait4end = flags;
> >> +		ccw->bits.halt_on_terminate = 1;
> >> +		ccw->bits.terminate_flush = 1;
> >> +		ccw->bits.pio_num = sg_len;
> >> +		ccw->bits.command = MXS_DMA_NO_XFER;
> >
> > Does this have a valid usecase? I would just return some error code
> > here. pio_num and pio_words are unused in the driver and I don't think
> > a dmaengine driver should have some kind of PIO fallback.
> >
> Actually 'PIO' is a misnomer here. It's the free scaled way of
> implementing a simple feature (chained DMA with mixed transfer modes)
> in a complicated and obfuscated way.
> 
> What's behinde the 'PIO' transfers is programming controller registers
> via DMA along with the actual DMA data transfer. DMA_NONE simply
> means, that the DMA transfer does only the register programming but
> does not transfer any payload. The 'pio_words' are the values that are
> being written to consecutive locations of e.g. the SPI controller
> register address space.
> The programming is actually done by DMA, in any case.

OK, got it now.

The NAND programming example in the reference manual looks...
interesting. So it's possible to program the DMA engine in a way that it
autonomously sends commands to the NAND controller, reads blocks from
NAND, polls for status, make branches depending on status bits...
This looks more like a DMA programming language.

I think it does not make much sense to try to implement these features
in the dmaengine API. What the dmaengine offers is only a special
usecase of what the controller provides. Also these features are so
hardware specific that it won't make sense to extend the dmaengine API
for them.

So what should we do? Not use these features at all? Create a bypass to
the dmaengine API? Make the dmaengine driver a user of another, i.MX28
specific driver which provides the additional features?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 1/5] dmaengine: mxs-dma: add dma support for i.MX23/28

[Shawn Guo]

On Wed, Feb 09, 2011 at 10:09:15AM +0100, Sascha Hauer wrote:
> On Tue, Feb 08, 2011 at 03:41:55PM +0100, Lothar Wa?mann wrote:
> > Hi,
> >
> > > On Sat, Feb 05, 2011 at 10:08:12AM +0800, Shawn Guo wrote:
> > >> This patch adds dma support for Freescale MXS-based SoC i.MX23/28,
> > >> including apbh-dma and apbx-dma.
> > >>
> > >> * apbh-dma and apbx-dma are supported in the driver as two instances,
> > >>   and have to be filtered by dma clients via device id.  It becomes
> > >>   the convention that apbh-dma always gets registered prior to
> > >>   apbx-dma.
> > >>
> > >> * apbh-dma is different between mx23 and mx28, hardware version
> > >>   register is used to handle the differences.
> > >>
> > >> * Every the mxs dma channel is statically assigned to client device
> > >>   by soc design with fixed irq.  The irq number is being passed by
> > >>   alloc_chan function with mxs_dma_data, and client driver has to
> > >>   filter the correct channel by its channel id.
> > >>
> > >> * mxs-dma supports pio function besides data transfer.  The driver
> > >>   uses dma_data_direction DMA_NONE to identify the pio mode, and
> > >>   steals sgl and sg_len to get pio words and numbers from clients.
> > >>
> > >> * mxs dmaengine has some very specific features, like sense function
> > >>   and the special NAND support (nand_lock, nand_wait4ready).  These
> > >>   are too specific to implemented in generic dmaengine driver.
> > >>
> > >> * The parameter "flags" of prep functions is currently being used to
> > >>   pass wait4end flag from clients.
> > >>
> > >> * The driver refers to imx-sdma and only a single descriptor is
> > >>   statically assigned to each channel.
> > >>
> > >> Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > >> ---
> > >>  arch/arm/mach-mxs/include/mach/dma.h |   16 +
> > >>  drivers/dma/Kconfig                  |    8 +
> > >>  drivers/dma/Makefile                 |    1 +
> > >>  drivers/dma/mxs-dma.c                |  702 ++++++++++++++++++++++++++++++++++
> > >>  4 files changed, 727 insertions(+), 0 deletions(-)
> > >>  create mode 100644 arch/arm/mach-mxs/include/mach/dma.h
> > >>  create mode 100644 drivers/dma/mxs-dma.c
> > >>
> > >> diff --git a/arch/arm/mach-mxs/include/mach/dma.h b/arch/arm/mach-mxs/include/mach/dma.h
> > >> new file mode 100644
> > >> index 0000000..429f431
> > >> --- /dev/null
> > >> +++ b/arch/arm/mach-mxs/include/mach/dma.h
> > >> +
> > >> +static struct dma_async_tx_descriptor *mxs_dma_prep_slave_sg(
> > >> +          struct dma_chan *chan, struct scatterlist *sgl,
> > >> +          unsigned int sg_len, enum dma_data_direction direction,
> > >> +          unsigned long flags)
> > >> +{
> > >> +  struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> > >> +  struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> > >> +  struct mxs_dma_ccw *ccw;
> > >> +  struct scatterlist *sg;
> > >> +  int ret, i, j;
> > >> +  u32 *pio;
> > >> +
> > >> +  dev_dbg(mxs_dma->dev, "%s: channel %d\n", __func__, chan->chan_id);
> > >> +
> > >> +  if (mxs_chan->status == DMA_IN_PROGRESS)
> > >> +          return NULL;
> > >> +
> > >> +  mxs_chan->status = DMA_IN_PROGRESS;
> > >> +  mxs_chan->flags = 0;
> > >> +
> > >> +  dev_dbg(mxs_dma->dev, "%s: setting up %d entries\n", __func__, sg_len);
> > >> +
> > >> +  if (sg_len > ((direction == DMA_NONE) ? MXS_PIO_WORDS : NUM_CCW)) {
> > >> +          dev_err(mxs_dma->dev, "maximum number of sg exceeded: %d > %d\n",
> > >> +                          sg_len, NUM_CCW);
> > >> +          ret = -EINVAL;
> > >> +          goto err_out;
> > >> +  }
> > >> +
> > >> +  if (direction == DMA_NONE) {
> > >> +          ccw = &mxs_chan->ccw[0];
> > >> +          pio = (u32 *) sgl;
> > >> +
> > >> +          for (j = 0; j < sg_len;)
> > >> +                  ccw->pio_words[j++] = *pio++;
> > >> +
> > >> +          ccw->next = 0;
> > >> +          ccw->bits.chain = 0;
> > >> +          ccw->bits.irq = 1;
> > >> +          ccw->bits.dec_sem = 1;
> > >> +          ccw->bits.wait4end = flags;
> > >> +          ccw->bits.halt_on_terminate = 1;
> > >> +          ccw->bits.terminate_flush = 1;
> > >> +          ccw->bits.pio_num = sg_len;
> > >> +          ccw->bits.command = MXS_DMA_NO_XFER;
> > >
> > > Does this have a valid usecase? I would just return some error code
> > > here. pio_num and pio_words are unused in the driver and I don't think
> > > a dmaengine driver should have some kind of PIO fallback.
> > >
> > Actually 'PIO' is a misnomer here. It's the free scaled way of
> > implementing a simple feature (chained DMA with mixed transfer modes)
> > in a complicated and obfuscated way.
> >
> > What's behinde the 'PIO' transfers is programming controller registers
> > via DMA along with the actual DMA data transfer. DMA_NONE simply
> > means, that the DMA transfer does only the register programming but
> > does not transfer any payload. The 'pio_words' are the values that are
> > being written to consecutive locations of e.g. the SPI controller
> > register address space.
> > The programming is actually done by DMA, in any case.
> 
> OK, got it now.
> 
> The NAND programming example in the reference manual looks...
> interesting. So it's possible to program the DMA engine in a way that it
> autonomously sends commands to the NAND controller, reads blocks from
> NAND, polls for status, make branches depending on status bits...

This is what sense command does.  I scanned the dma client drivers
implemented in Freescale BSP and found only NAND driver uses it.
All others use pio and data transfer.

> This looks more like a DMA programming language.
> 
> I think it does not make much sense to try to implement these features
> in the dmaengine API. What the dmaengine offers is only a special
> usecase of what the controller provides. Also these features are so
> hardware specific that it won't make sense to extend the dmaengine API
> for them.
> 
> So what should we do? Not use these features at all? Create a bypass to
> the dmaengine API? Make the dmaengine driver a user of another, i.MX28
> specific driver which provides the additional features?
> 
My preference is not use these NAND specific features at all.  And
I think NAND driver can also be implemented with pio and data
commands only.

Regards,
Shawn

[PATCH 2/5] ARM: mxs: add dma channel definitions

[Shawn Guo]

Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
---
 arch/arm/mach-mxs/include/mach/mx23.h |   24 +++++++++++++++++++++
 arch/arm/mach-mxs/include/mach/mx28.h |   37 +++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-mxs/include/mach/mx23.h b/arch/arm/mach-mxs/include/mach/mx23.h
index 9edd02e..1730c9c 100644
--- a/arch/arm/mach-mxs/include/mach/mx23.h
+++ b/arch/arm/mach-mxs/include/mach/mx23.h
@@ -142,4 +142,28 @@
 #define MX23_INT_VDD5V_DROOP		64
 #define MX23_INT_DCDC4P2_BO		65
 
+/*
+ * APBH DMA
+ */
+#define MX23_DMA_SSP1			1
+#define MX23_DMA_SSP2			2
+#define MX23_DMA_GPMI0			4
+#define MX23_DMA_GPMI1			5
+#define MX23_DMA_GPMI2			6
+#define MX23_DMA_GPMI3			7
+
+/*
+ * APBX DMA
+ */
+#define MX23_DMA_ADC			0
+#define MX23_DMA_DAC			1
+#define MX23_DMA_SPDIF			2
+#define MX23_DMA_I2C			3
+#define MX23_DMA_SAIF0			4
+#define MX23_DMA_UART0_RX		6
+#define MX23_DMA_UART0_TX		7
+#define MX23_DMA_UART1_RX		8
+#define MX23_DMA_UART1_TX		9
+#define MX23_DMA_SAIF1			10
+
 #endif /* __MACH_MX23_H__ */
diff --git a/arch/arm/mach-mxs/include/mach/mx28.h b/arch/arm/mach-mxs/include/mach/mx28.h
index 0716745..3f3485a 100644
--- a/arch/arm/mach-mxs/include/mach/mx28.h
+++ b/arch/arm/mach-mxs/include/mach/mx28.h
@@ -185,4 +185,41 @@
 #define MX28_INT_GPIO1			126
 #define MX28_INT_GPIO0			127
 
+/*
+ * APBH DMA
+ */
+#define MX28_DMA_SSP0			0
+#define MX28_DMA_SSP1			1
+#define MX28_DMA_SSP2			2
+#define MX28_DMA_SSP3			3
+#define MX28_DMA_GPMI0			4
+#define MX28_DMA_GPMI1			5
+#define MX28_DMA_GPMI2			6
+#define MX28_DMA_GPMI3			7
+#define MX28_DMA_GPMI4			8
+#define MX28_DMA_GPMI5			9
+#define MX28_DMA_GPMI6			10
+#define MX28_DMA_GPMI7			11
+#define MX28_DMA_HSADC			12
+#define MX28_DMA_LCDIF			13
+
+/*
+ * APBX DMA
+ */
+#define MX28_DMA_AUART4_RX		0
+#define MX28_DMA_AUART4_TX		1
+#define MX28_DMA_SPDIF_TX		2
+#define MX28_DMA_SAIF0			4
+#define MX28_DMA_SAIF1			5
+#define MX28_DMA_I2C0			6
+#define MX28_DMA_I2C1			7
+#define MX28_DMA_AUART0_RX		8
+#define MX28_DMA_AUART0_TX		9
+#define MX28_DMA_AUART1_RX		10
+#define MX28_DMA_AUART1_TX		11
+#define MX28_DMA_AUART2_RX		12
+#define MX28_DMA_AUART2_TX		13
+#define MX28_DMA_AUART3_RX		14
+#define MX28_DMA_AUART3_TX		15
+
 #endif /* __MACH_MX28_H__ */
-- 
1.7.1

[PATCH 3/5] ARM: mxs: dynamically allocate dma device for mx23/28

[Shawn Guo]

Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
---
 arch/arm/mach-mxs/clock-mx23.c                  |    3 +-
 arch/arm/mach-mxs/clock-mx28.c                  |    4 +-
 arch/arm/mach-mxs/devices-mx23.h                |    6 +++
 arch/arm/mach-mxs/devices-mx28.h                |    6 +++
 arch/arm/mach-mxs/devices/Kconfig               |    3 +
 arch/arm/mach-mxs/devices/Makefile              |    1 +
 arch/arm/mach-mxs/devices/platform-dma.c        |   50 +++++++++++++++++++++++
 arch/arm/mach-mxs/include/mach/devices-common.h |    7 +++
 8 files changed, 77 insertions(+), 3 deletions(-)
 create mode 100644 arch/arm/mach-mxs/devices/platform-dma.c

diff --git a/arch/arm/mach-mxs/clock-mx23.c b/arch/arm/mach-mxs/clock-mx23.c
index b1a362e..350b28c 100644
--- a/arch/arm/mach-mxs/clock-mx23.c
+++ b/arch/arm/mach-mxs/clock-mx23.c
@@ -443,7 +443,8 @@ static struct clk_lookup lookups[] = {
 	/* for amba-pl011 driver */
 	_REGISTER_CLOCK("duart", NULL, uart_clk)
 	_REGISTER_CLOCK("rtc", NULL, rtc_clk)
-	_REGISTER_CLOCK(NULL, "hclk", hbus_clk)
+	_REGISTER_CLOCK("mxs-dma-apbh", NULL, hbus_clk)
+	_REGISTER_CLOCK("mxs-dma-apbx", NULL, xbus_clk)
 	_REGISTER_CLOCK(NULL, "usb", usb_clk)
 	_REGISTER_CLOCK(NULL, "audio", audio_clk)
 	_REGISTER_CLOCK(NULL, "pwm", pwm_clk)
diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c
index c9d7951..a3b4787 100644
--- a/arch/arm/mach-mxs/clock-mx28.c
+++ b/arch/arm/mach-mxs/clock-mx28.c
@@ -617,8 +617,8 @@ static struct clk_lookup lookups[] = {
 	_REGISTER_CLOCK("fec.0", NULL, fec_clk)
 	_REGISTER_CLOCK("rtc", NULL, rtc_clk)
 	_REGISTER_CLOCK("pll2", NULL, pll2_clk)
-	_REGISTER_CLOCK(NULL, "hclk", hbus_clk)
-	_REGISTER_CLOCK(NULL, "xclk", xbus_clk)
+	_REGISTER_CLOCK("mxs-dma-apbh", NULL, hbus_clk)
+	_REGISTER_CLOCK("mxs-dma-apbx", NULL, xbus_clk)
 	_REGISTER_CLOCK("flexcan.0", NULL, can0_clk)
 	_REGISTER_CLOCK("flexcan.1", NULL, can1_clk)
 	_REGISTER_CLOCK(NULL, "usb0", usb0_clk)
diff --git a/arch/arm/mach-mxs/devices-mx23.h b/arch/arm/mach-mxs/devices-mx23.h
index 1256788..d5595ce 100644
--- a/arch/arm/mach-mxs/devices-mx23.h
+++ b/arch/arm/mach-mxs/devices-mx23.h
@@ -14,3 +14,9 @@
 extern const struct amba_device mx23_duart_device __initconst;
 #define mx23_add_duart() \
 	mxs_add_duart(&mx23_duart_device)
+
+extern const struct mxs_dma_data mx23_dma_data[] __initconst;
+#define mx23_add_apbh_dma() \
+	mxs_add_dma(&mx23_dma_data[0])
+#define mx23_add_apbx_dma() \
+	mxs_add_dma(&mx23_dma_data[1])
diff --git a/arch/arm/mach-mxs/devices-mx28.h b/arch/arm/mach-mxs/devices-mx28.h
index 3b18304..e3a3bbc 100644
--- a/arch/arm/mach-mxs/devices-mx28.h
+++ b/arch/arm/mach-mxs/devices-mx28.h
@@ -23,6 +23,12 @@ extern const struct mxs_auart_data mx28_auart_data[] __initconst;
 #define mx28_add_auart3()		mx28_add_auart(3)
 #define mx28_add_auart4()		mx28_add_auart(4)
 
+extern const struct mxs_dma_data mx28_dma_data[] __initconst;
+#define mx28_add_apbh_dma() \
+	mxs_add_dma(&mx28_dma_data[0])
+#define mx28_add_apbx_dma() \
+	mxs_add_dma(&mx28_dma_data[1])
+
 extern const struct mxs_fec_data mx28_fec_data[] __initconst;
 #define mx28_add_fec(id, pdata) \
 	mxs_add_fec(&mx28_fec_data[id], pdata)
diff --git a/arch/arm/mach-mxs/devices/Kconfig b/arch/arm/mach-mxs/devices/Kconfig
index 6c65b67..c2ddbc5 100644
--- a/arch/arm/mach-mxs/devices/Kconfig
+++ b/arch/arm/mach-mxs/devices/Kconfig
@@ -5,6 +5,9 @@ config MXS_HAVE_AMBA_DUART
 config MXS_HAVE_PLATFORM_AUART
 	bool
 
+config MXS_HAVE_PLATFORM_DMA
+	bool
+
 config MXS_HAVE_PLATFORM_FEC
 	bool
 
diff --git a/arch/arm/mach-mxs/devices/Makefile b/arch/arm/mach-mxs/devices/Makefile
index a8dc8d5..510849d 100644
--- a/arch/arm/mach-mxs/devices/Makefile
+++ b/arch/arm/mach-mxs/devices/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_MXS_HAVE_AMBA_DUART) += amba-duart.o
 obj-$(CONFIG_MXS_HAVE_PLATFORM_AUART) += platform-auart.o
+obj-$(CONFIG_MXS_HAVE_PLATFORM_DMA) += platform-dma.o
 obj-$(CONFIG_MXS_HAVE_PLATFORM_FEC) += platform-fec.o
 obj-$(CONFIG_MXS_HAVE_PLATFORM_FLEXCAN) += platform-flexcan.o
diff --git a/arch/arm/mach-mxs/devices/platform-dma.c b/arch/arm/mach-mxs/devices/platform-dma.c
new file mode 100644
index 0000000..0c15a26
--- /dev/null
+++ b/arch/arm/mach-mxs/devices/platform-dma.c
@@ -0,0 +1,50 @@
+/*
+ * Copyright (C) 2010 Pengutronix
+ * Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>
+ *
+ * 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/compiler.h>
+#include <linux/err.h>
+#include <linux/init.h>
+
+#include <mach/mx23.h>
+#include <mach/mx28.h>
+#include <mach/devices-common.h>
+
+#define mxs_dma_data_entry_single(soc, type, _devid)			\
+	{								\
+		.devid = _devid,					\
+		.iobase = soc ## _ ## type ## _DMA ## _BASE_ADDR,	\
+	}
+
+#ifdef CONFIG_SOC_IMX23
+struct mxs_dma_data mx23_dma_data[] __initconst = {
+	mxs_dma_data_entry_single(MX23, APBH, "mxs-dma-apbh"),
+	mxs_dma_data_entry_single(MX23, APBX, "mxs-dma-apbx"),
+};
+#endif
+
+#ifdef CONFIG_SOC_IMX28
+struct mxs_dma_data mx28_dma_data[] __initconst = {
+	mxs_dma_data_entry_single(MX28, APBH, "mxs-dma-apbh"),
+	mxs_dma_data_entry_single(MX28, APBX, "mxs-dma-apbx"),
+};
+#endif
+
+struct platform_device *__init mxs_add_dma(
+			const struct mxs_dma_data *data)
+{
+	struct resource res[] = {
+		{
+			.start = data->iobase,
+			.end = data->iobase + SZ_8K - 1,
+			.flags = IORESOURCE_MEM,
+		}
+	};
+
+	return mxs_add_platform_device(data->devid, -1,
+				res, ARRAY_SIZE(res), NULL, 0);
+}
diff --git a/arch/arm/mach-mxs/include/mach/devices-common.h b/arch/arm/mach-mxs/include/mach/devices-common.h
index e7aefb4..acd74b4 100644
--- a/arch/arm/mach-mxs/include/mach/devices-common.h
+++ b/arch/arm/mach-mxs/include/mach/devices-common.h
@@ -40,6 +40,13 @@ struct mxs_auart_data {
 struct platform_device *__init mxs_add_auart(
 		const struct mxs_auart_data *data);
 
+/* dma */
+struct mxs_dma_data {
+	const char *devid;
+	resource_size_t iobase;
+};
+struct platform_device *__init mxs_add_dma(const struct mxs_dma_data *data);
+
 /* fec */
 #include <linux/fec.h>
 struct mxs_fec_data {
-- 
1.7.1

[PATCH 3/5] ARM: mxs: dynamically allocate dma device for mx23/28

[Sascha Hauer]

On Sat, Feb 05, 2011 at 10:08:14AM +0800, Shawn Guo wrote:
> Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> ---
>  arch/arm/mach-mxs/clock-mx23.c                  |    3 +-
>  arch/arm/mach-mxs/clock-mx28.c                  |    4 +-
>  arch/arm/mach-mxs/devices-mx23.h                |    6 +++
>  arch/arm/mach-mxs/devices-mx28.h                |    6 +++
>  arch/arm/mach-mxs/devices/Kconfig               |    3 +
>  arch/arm/mach-mxs/devices/Makefile              |    1 +
>  arch/arm/mach-mxs/devices/platform-dma.c        |   50 +++++++++++++++++++++++
>  arch/arm/mach-mxs/include/mach/devices-common.h |    7 +++
>  8 files changed, 77 insertions(+), 3 deletions(-)
>  create mode 100644 arch/arm/mach-mxs/devices/platform-dma.c
> 
> diff --git a/arch/arm/mach-mxs/devices-mx23.h b/arch/arm/mach-mxs/devices-mx23.h
> index 1256788..d5595ce 100644
> --- a/arch/arm/mach-mxs/devices-mx23.h
> +++ b/arch/arm/mach-mxs/devices-mx23.h
> @@ -14,3 +14,9 @@
>  extern const struct amba_device mx23_duart_device __initconst;
>  #define mx23_add_duart() \
>  	mxs_add_duart(&mx23_duart_device)
> +
> +extern const struct mxs_dma_data mx23_dma_data[] __initconst;
> +#define mx23_add_apbh_dma() \
> +	mxs_add_dma(&mx23_dma_data[0])
> +#define mx23_add_apbx_dma() \
> +	mxs_add_dma(&mx23_dma_data[1])
> diff --git a/arch/arm/mach-mxs/devices-mx28.h b/arch/arm/mach-mxs/devices-mx28.h
> index 3b18304..e3a3bbc 100644
> --- a/arch/arm/mach-mxs/devices-mx28.h
> +++ b/arch/arm/mach-mxs/devices-mx28.h
> @@ -23,6 +23,12 @@ extern const struct mxs_auart_data mx28_auart_data[] __initconst;
>  #define mx28_add_auart3()		mx28_add_auart(3)
>  #define mx28_add_auart4()		mx28_add_auart(4)
>  
> +extern const struct mxs_dma_data mx28_dma_data[] __initconst;
> +#define mx28_add_apbh_dma() \
> +	mxs_add_dma(&mx28_dma_data[0])
> +#define mx28_add_apbx_dma() \
> +	mxs_add_dma(&mx28_dma_data[1])
> +

Given that the DMA device is fully internal to the SoC and always
present, does it make sense to add it dynamically and to leave
registration to the boards?

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 3/5] ARM: mxs: dynamically allocate dma device for mx23/28

[Shawn Guo]

On Mon, Feb 07, 2011 at 09:09:45AM +0100, Sascha Hauer wrote:
> On Sat, Feb 05, 2011 at 10:08:14AM +0800, Shawn Guo wrote:
> > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > ---
> >  arch/arm/mach-mxs/clock-mx23.c                  |    3 +-
> >  arch/arm/mach-mxs/clock-mx28.c                  |    4 +-
> >  arch/arm/mach-mxs/devices-mx23.h                |    6 +++
> >  arch/arm/mach-mxs/devices-mx28.h                |    6 +++
> >  arch/arm/mach-mxs/devices/Kconfig               |    3 +
> >  arch/arm/mach-mxs/devices/Makefile              |    1 +
> >  arch/arm/mach-mxs/devices/platform-dma.c        |   50 +++++++++++++++++++++++
> >  arch/arm/mach-mxs/include/mach/devices-common.h |    7 +++
> >  8 files changed, 77 insertions(+), 3 deletions(-)
> >  create mode 100644 arch/arm/mach-mxs/devices/platform-dma.c
> > 
> > diff --git a/arch/arm/mach-mxs/devices-mx23.h b/arch/arm/mach-mxs/devices-mx23.h
> > index 1256788..d5595ce 100644
> > --- a/arch/arm/mach-mxs/devices-mx23.h
> > +++ b/arch/arm/mach-mxs/devices-mx23.h
> > @@ -14,3 +14,9 @@
> >  extern const struct amba_device mx23_duart_device __initconst;
> >  #define mx23_add_duart() \
> >  	mxs_add_duart(&mx23_duart_device)
> > +
> > +extern const struct mxs_dma_data mx23_dma_data[] __initconst;
> > +#define mx23_add_apbh_dma() \
> > +	mxs_add_dma(&mx23_dma_data[0])
> > +#define mx23_add_apbx_dma() \
> > +	mxs_add_dma(&mx23_dma_data[1])
> > diff --git a/arch/arm/mach-mxs/devices-mx28.h b/arch/arm/mach-mxs/devices-mx28.h
> > index 3b18304..e3a3bbc 100644
> > --- a/arch/arm/mach-mxs/devices-mx28.h
> > +++ b/arch/arm/mach-mxs/devices-mx28.h
> > @@ -23,6 +23,12 @@ extern const struct mxs_auart_data mx28_auart_data[] __initconst;
> >  #define mx28_add_auart3()		mx28_add_auart(3)
> >  #define mx28_add_auart4()		mx28_add_auart(4)
> >  
> > +extern const struct mxs_dma_data mx28_dma_data[] __initconst;
> > +#define mx28_add_apbh_dma() \
> > +	mxs_add_dma(&mx28_dma_data[0])
> > +#define mx28_add_apbx_dma() \
> > +	mxs_add_dma(&mx28_dma_data[1])
> > +
> 
> Given that the DMA device is fully internal to the SoC and always
> present, does it make sense to add it dynamically and to leave
> registration to the boards?
> 
OK.  Will make it in initcall.  Correct me if this is not what you
expect.

Regards,
Shawn

[PATCH 4/5] ARM: mxs/mx23evk: add dma device

[Shawn Guo]

Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
---
 arch/arm/mach-mxs/Kconfig        |    1 +
 arch/arm/mach-mxs/mach-mx23evk.c |    8 ++++++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-mxs/Kconfig b/arch/arm/mach-mxs/Kconfig
index cd2fbdf..0b9bb03 100644
--- a/arch/arm/mach-mxs/Kconfig
+++ b/arch/arm/mach-mxs/Kconfig
@@ -19,6 +19,7 @@ config MACH_MX23EVK
 	bool "Support MX23EVK Platform"
 	select SOC_IMX23
 	select MXS_HAVE_AMBA_DUART
+	select MXS_HAVE_PLATFORM_DMA
 	default y
 	help
 	  Include support for MX23EVK platform. This includes specific
diff --git a/arch/arm/mach-mxs/mach-mx23evk.c b/arch/arm/mach-mxs/mach-mx23evk.c
index aa06400..13c22a0 100644
--- a/arch/arm/mach-mxs/mach-mx23evk.c
+++ b/arch/arm/mach-mxs/mach-mx23evk.c
@@ -37,6 +37,14 @@ static void __init mx23evk_init(void)
 	mxs_iomux_setup_multiple_pads(mx23evk_pads, ARRAY_SIZE(mx23evk_pads));
 
 	mx23_add_duart();
+
+	/*
+	 * the order of adding dma device matters here, otherwise
+	 * dma_device.dev_id may differ from mxs_dma_engine.dev_id,
+	 * in which case mxs-dma will fail to probe.
+	 */
+	mx23_add_apbh_dma();
+	mx23_add_apbx_dma();
 }
 
 static void __init mx23evk_timer_init(void)
-- 
1.7.1

[PATCH 5/5] ARM: mxs/mx28evk: add dma device

[Shawn Guo]

Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
---
 arch/arm/mach-mxs/Kconfig        |    1 +
 arch/arm/mach-mxs/mach-mx28evk.c |    8 ++++++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-mxs/Kconfig b/arch/arm/mach-mxs/Kconfig
index 0b9bb03..c9ac415 100644
--- a/arch/arm/mach-mxs/Kconfig
+++ b/arch/arm/mach-mxs/Kconfig
@@ -29,6 +29,7 @@ config MACH_MX28EVK
 	bool "Support MX28EVK Platform"
 	select SOC_IMX28
 	select MXS_HAVE_AMBA_DUART
+	select MXS_HAVE_PLATFORM_DMA
 	select MXS_HAVE_PLATFORM_FEC
 	select MXS_OCOTP
 	default y
diff --git a/arch/arm/mach-mxs/mach-mx28evk.c b/arch/arm/mach-mxs/mach-mx28evk.c
index e8db99f..04ddec1 100644
--- a/arch/arm/mach-mxs/mach-mx28evk.c
+++ b/arch/arm/mach-mxs/mach-mx28evk.c
@@ -165,6 +165,14 @@ static void __init mx28evk_init(void)
 
 	mx28_add_duart();
 
+	/*
+	 * the order of adding dma device matters here, otherwise
+	 * dma_device.dev_id may differ from mxs_dma_engine.dev_id,
+	 * in which case mxs-dma will fail to probe.
+	 */
+	mx28_add_apbh_dma();
+	mx28_add_apbx_dma();
+
 	if (mx28evk_fec_get_mac())
 		pr_warn("%s: failed on fec mac setup\n", __func__);
 
-- 
1.7.1