[PATCH v2 0/3] Add dma support for i.MX23/28

[PATCH v2 0/3] Add dma support for i.MX23/28

[Shawn Guo]

The v2 of the patch set is to address the v1 comments given by
Russell, Sascha, and Lothar.

The patch number is down to 3 from 5 (v1) due to the change of dma
device dynamical allocation to the static.

  [PATCH v2 1/3] dmaengine: mxs-dma: add dma support for i.MX23/28
  [PATCH v2 2/3] ARM: mxs: add dma channel definitions
  [PATCH v2 3/3] ARM: mxs: add dma device

Thanks for review.

Regards,
Shawn

[PATCH v2 1/3] 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.

* 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 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 |   26 ++
 drivers/dma/Kconfig                  |    8 +
 drivers/dma/Makefile                 |    1 +
 drivers/dma/mxs-dma.c                |  732 ++++++++++++++++++++++++++++++++++
 4 files changed, 767 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..7f4aeea
--- /dev/null
+++ b/arch/arm/mach-mxs/include/mach/dma.h
@@ -0,0 +1,26 @@
+/*
+ * 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;
+};
+
+static inline int mxs_dma_is_apbh(struct dma_chan *chan)
+{
+	return !strcmp(dev_name(chan->device->dev), "mxs-dma-apbh");
+}
+
+static inline int mxs_dma_is_apbx(struct dma_chan *chan)
+{
+	return !strcmp(dev_name(chan->device->dev), "mxs-dma-apbx");
+}
+
+#endif /* __MACH_MXS_DMA_H__ */
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 1c28816..76f6472 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
+	bool "MXS DMA support"
+	depends on SOC_IMX23 || SOC_IMX28
+	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..b168e29
--- /dev/null
+++ b/drivers/dma/mxs-dma.c
@@ -0,0 +1,732 @@
+/*
+ * 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>
+
+/*
+ * NOTE: The term "PIO" throughout the mxs-dma implementation means
+ * PIO mode of mxs apbh-dma and apbx-dma.  With this working mode,
+ * dma can program the controller registers of peripheral devices.
+ */
+
+#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)
+
+/*
+ * ccw bits definitions
+ *
+ * COMMAND:		0..1	(2)
+ * CHAIN:		2	(1)
+ * IRQ:			3	(1)
+ * NAND_LOCK:		4	(1) - not implemented
+ * NAND_WAIT4READY:	5	(1) - not implemented
+ * DEC_SEM:		6	(1)
+ * WAIT4END:		7	(1)
+ * HALT_ON_TERMINATE:	8	(1)
+ * TERMINATE_FLUSH:	9	(1)
+ * RESERVED:		10..11	(2)
+ * PIO_NUM:		12..15	(4)
+ */
+#define BP_CCW_COMMAND		0
+#define BM_CCW_COMMAND		(3 << 0)
+#define CCW_CHAIN		(1 << 2)
+#define CCW_IRQ			(1 << 3)
+#define CCW_DEC_SEM		(1 << 6)
+#define CCW_WAIT4END		(1 << 7)
+#define CCW_HALT_ON_TERM	(1 << 8)
+#define CCW_TERM_FLUSH		(1 << 9)
+#define BP_CCW_PIO_NUM		12
+#define BM_CCW_PIO_NUM		(0xf << 12)
+
+#define BF_CCW(value, field)	(((value) << BP_CCW_##field) & BM_CCW_##field)
+
+#define MXS_DMA_CMD_NO_XFER	0
+#define MXS_DMA_CMD_WRITE	1
+#define MXS_DMA_CMD_READ	2
+#define MXS_DMA_CMD_DMA_SENSE	3	/* not implemented */
+
+struct mxs_dma_ccw {
+	u32		next;
+	u16		bits;
+	u16		xfer_bytes;
+#define MAX_XFER_BYTES	0xff00
+	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;
+	struct tasklet_struct		tasklet;
+	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 {
+	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;
+	unsigned long flags;
+
+	spin_lock_irqsave(&mxs_chan->lock, flags);
+
+	cookie = mxs_dma_assign_cookie(mxs_chan);
+
+	mxs_dma_enable_chan(mxs_chan);
+
+	spin_unlock_irqrestore(&mxs_chan->lock, flags);
+
+	return cookie;
+}
+
+static void mxs_dma_tasklet(unsigned long data)
+{
+	struct mxs_dma_chan *mxs_chan = (struct mxs_dma_chan *) data;
+	unsigned long flags;
+
+	spin_lock_irqsave(&mxs_chan->lock, flags);
+
+	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;
+
+	spin_unlock_irqrestore(&mxs_chan->lock, flags);
+}
+
+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->dma_device.dev, "%s: error in channel %d\n",
+						__func__, channel - 16);
+			mxs_chan->status = DMA_ERROR;
+			mxs_dma_reset_chan(mxs_chan);
+		} else {
+			if (mxs_chan->flags & MXS_DMA_SG_LOOP)
+				mxs_chan->status = DMA_IN_PROGRESS;
+			else
+				mxs_chan->status = DMA_SUCCESS;
+		}
+
+		stat1 &= ~(1 << channel);
+
+		/* schedule tasklet on this channel */
+		tasklet_schedule(&mxs_chan->tasklet);
+	}
+
+	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;
+	int ret;
+
+	if (!data)
+		return -EINVAL;
+
+	mxs_chan->chan_irq = data->chan_irq;
+
+	mxs_chan->ccw = dma_alloc_coherent(mxs_dma->dma_device.dev, 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,
+				0, "mxs-dma", mxs_dma);
+	if (ret)
+		goto err_irq;
+
+	ret = clk_enable(mxs_dma->clk);
+	if (ret)
+		goto err_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_clk:
+	free_irq(mxs_chan->chan_irq, mxs_dma);
+err_irq:
+	dma_free_coherent(mxs_dma->dma_device.dev, 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(mxs_dma->dma_device.dev, 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 append)
+{
+	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 i, j;
+	u32 *pio;
+	static int idx = 0;
+
+	if (mxs_chan->status == DMA_IN_PROGRESS && !append)
+		return NULL;
+
+	if (sg_len + (append ? idx : 0) > NUM_CCW) {
+		dev_err(mxs_dma->dma_device.dev,
+				"maximum number of sg exceeded: %d > %d\n",
+				sg_len, NUM_CCW);
+		goto err_out;
+	}
+
+	mxs_chan->status = DMA_IN_PROGRESS;
+	mxs_chan->flags = 0;
+
+	/*
+	 * If the sg is prepared with append flag set, the sg
+	 * will be appended to the last prepared sg.
+	 */
+	if (append) {
+		BUG_ON(idx < 1);
+		ccw = &mxs_chan->ccw[idx - 1];
+		ccw->next = mxs_chan->ccw_phys + sizeof(*ccw) * idx;
+		ccw->bits |= CCW_CHAIN;
+		ccw->bits &= ~CCW_IRQ;
+		ccw->bits &= ~CCW_DEC_SEM;
+		ccw->bits &= ~CCW_WAIT4END;
+	} else {
+		idx = 0;
+	}
+
+	if (direction == DMA_NONE) {
+		ccw = &mxs_chan->ccw[idx++];
+		pio = (u32 *) sgl;
+
+		for (j = 0; j < sg_len;)
+			ccw->pio_words[j++] = *pio++;
+
+		ccw->bits = 0;
+		ccw->bits |= CCW_IRQ;
+		ccw->bits |= CCW_DEC_SEM;
+		ccw->bits |= CCW_WAIT4END;
+		ccw->bits |= CCW_HALT_ON_TERM;
+		ccw->bits |= CCW_TERM_FLUSH;
+		ccw->bits |= BF_CCW(sg_len, PIO_NUM);
+		ccw->bits |= BF_CCW(MXS_DMA_CMD_NO_XFER, COMMAND);
+	} else {
+		for_each_sg(sgl, sg, sg_len, i) {
+			if (sg->length > MAX_XFER_BYTES) {
+				dev_err(mxs_dma->dma_device.dev, "maximum bytes for sg entry exceeded: %d > %d\n",
+						sg->length, MAX_XFER_BYTES);
+				goto err_out;
+			}
+
+			ccw = &mxs_chan->ccw[idx++];
+
+			ccw->next = mxs_chan->ccw_phys + sizeof(*ccw) * idx;
+			ccw->bufaddr = sg->dma_address;
+			ccw->xfer_bytes = sg->length;
+
+			ccw->bits = 0;
+			ccw->bits |= CCW_CHAIN;
+			ccw->bits |= CCW_HALT_ON_TERM;
+			ccw->bits |= CCW_TERM_FLUSH;
+			ccw->bits |= BF_CCW(direction == DMA_FROM_DEVICE ?
+					MXS_DMA_CMD_WRITE : MXS_DMA_CMD_READ,
+					COMMAND);
+
+			if (i + 1 == sg_len) {
+				ccw->bits &= ~CCW_CHAIN;
+				ccw->bits |= CCW_IRQ;
+				ccw->bits |= CCW_DEC_SEM;
+				ccw->bits |= CCW_WAIT4END;
+			}
+		}
+	}
+
+	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;
+
+	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->dma_device.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->dma_device.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->xfer_bytes = period_len;
+
+		ccw->bits = 0;
+		ccw->bits |= CCW_CHAIN;
+		ccw->bits |= CCW_IRQ;
+		ccw->bits |= CCW_HALT_ON_TERM;
+		ccw->bits |= CCW_TERM_FLUSH;
+		ccw->bits |= BF_CCW(direction == DMA_FROM_DEVICE ?
+				MXS_DMA_CMD_WRITE : MXS_DMA_CMD_READ, COMMAND);
+
+		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);
+	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;
+}
+
+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_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;
+		mxs_chan->chan.device = &mxs_dma->dma_device;
+
+		spin_lock_init(&mxs_chan->lock);
+		tasklet_init(&mxs_chan->tasklet, mxs_dma_tasklet,
+			     (unsigned long) mxs_chan);
+
+
+		/* 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(mxs_dma->dma_device.dev, "unable to register\n");
+		goto err_init;
+	}
+
+	dev_info(mxs_dma->dma_device.dev, "initialized\n");
+
+	return 0;
+
+err_init:
+	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 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,
+};
+
+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 v2 2/3] 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 v2 3/3] ARM: mxs: add dma device

[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/Makefile       |    1 +
 arch/arm/mach-mxs/devices/platform-dma.c |   99 ++++++++++++++++++++++++++++++
 4 files changed, 104 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/Makefile b/arch/arm/mach-mxs/devices/Makefile
index a8dc8d5..ca7acc4 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-y += 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..9eb976e
--- /dev/null
+++ b/arch/arm/mach-mxs/devices/platform-dma.c
@@ -0,0 +1,99 @@
+/*
+ * 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>
+
+struct mxs_mxs_dma_data {
+	const char *devid;
+	resource_size_t iobase;
+};
+
+#define mxs_dma_data_entry_single(soc, type, _devid)			\
+	{								\
+		.devid = _devid,					\
+		.iobase = soc ## _ ## type ## _DMA ## _BASE_ADDR,	\
+	}
+
+#ifdef CONFIG_SOC_IMX23
+struct mxs_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_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_mxs_dma_data *data)
+{
+	struct resource res[] = {
+		{
+			.start = data->iobase,
+			.end = data->iobase + SZ_8K - 1,
+			.flags = IORESOURCE_MEM,
+		}
+	};
+
+	return mxs_add_platform_device_dmamask(data->devid, -1,
+				res, ARRAY_SIZE(res), NULL, 0,
+				DMA_BIT_MASK(32));
+}
+
+#define mx23_add_apbh_dma() \
+	mxs_add_dma(&mx23_dma_data[0])
+#define mx23_add_apbx_dma() \
+	mxs_add_dma(&mx23_dma_data[1])
+
+#define mx28_add_apbh_dma() \
+	mxs_add_dma(&mx28_dma_data[0])
+#define mx28_add_apbx_dma() \
+	mxs_add_dma(&mx28_dma_data[1])
+
+static int __init mxs_add_mxs_dma(void)
+{
+	struct platform_device *ret;
+
+#ifdef CONFIG_SOC_IMX23
+	if (cpu_is_mx23()) {
+		ret = mx23_add_apbh_dma();
+		if (IS_ERR(ret))
+			goto out;
+
+		ret = mx23_add_apbx_dma();
+	} else
+#endif
+
+#ifdef CONFIG_SOC_IMX28
+	if (cpu_is_mx28()) {
+		ret = mx28_add_apbh_dma();
+		if (IS_ERR(ret))
+			goto out;
+
+		ret = mx28_add_apbx_dma();
+	} else
+#endif
+		ret = ERR_PTR(-ENODEV);
+
+out:
+	if (IS_ERR(ret))
+		return PTR_ERR(ret);
+	else
+		return 0;
+}
+arch_initcall(mxs_add_mxs_dma);
-- 
1.7.1

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

[Arnd Bergmann]

On Monday 14 February 2011, 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.
> 
> * 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 driver refers to imx-sdma and only a single descriptor is
>   statically assigned to each channel.

Hi Shawn,

I took a look at this after our discussion about the MMC driver locking
problem. My feeling is that you still need to get a better understanding
of the locking interfaces in Linux.

> +struct mxs_dma_ccw {
> +	u32		next;
> +	u16		bits;
> +	u16		xfer_bytes;
> +#define MAX_XFER_BYTES	0xff00
> +	dma_addr_t	bufaddr;
> +#define MXS_PIO_WORDS	16
> +	u32		pio_words[MXS_PIO_WORDS];
> +};

Unrelated, but should bufaddr really be dma_addr_t? If this is a hardware
structure, you should make it u32, because dma_addr_t may be either 32 or
64 bit in the future, as we get to multiplatform kernels.

> +struct mxs_dma_chan {
> +	struct mxs_dma_engine		*mxs_dma;
> +	struct dma_chan			chan;
> +	struct dma_async_tx_descriptor	desc;
> +	struct tasklet_struct		tasklet;
> +	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 {
> +	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];
> +};

So you have locking per channel, but there is no lock in the dma engine
structure locking the global fields. This is probably fine.

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

Just like the MMC driver, you also use __raw_writel(). If you have to do
that, please encapsulate the access into a wrapper function that adds
all the necessary serialization and add a comment explaining why you
cannot use writel()/writel_relaxed().

Or just use writel().

> +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;
> +}

Should these take the spinlock? What if they are called simultaneously on
multiple CPUs, or while the interrupt handler is running on another CPU?

Without a lock, another thread might see an incorrect value of mxs_chan->status.

> +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;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&mxs_chan->lock, flags);
> +
> +	cookie = mxs_dma_assign_cookie(mxs_chan);
> +
> +	mxs_dma_enable_chan(mxs_chan);
> +
> +	spin_unlock_irqrestore(&mxs_chan->lock, flags);
> +
> +	return cookie;
> +}

Just like the MMC driver, you use spin_lock_irqsave() in the functions that
are called by other drivers, but then don't actually lock in the interrupt
handler. This is clearly wrong, as I explained before:

The interrupt handler may already be running on another CPU, so it
is not at all serialized with this code.

> +static void mxs_dma_tasklet(unsigned long data)
> +{
> +	struct mxs_dma_chan *mxs_chan = (struct mxs_dma_chan *) data;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&mxs_chan->lock, flags);
> +
> +	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;
> +
> +	spin_unlock_irqrestore(&mxs_chan->lock, flags);
> +}

Here is a different problem. The entire tasklet has interrupts
disabled and holds the spinlock that the interrupt handler does
not hold.

This actually makes the tasklet a stricter context than the
actual interrupt handler, defeating the purpose of tasklets
(i.e. that you can run code without disabling interrupts).

I don't know what the rules for dmaengine callbacks are regarding
the context in which they are called, but it seems you should
either get rid of the spin_lock_irqsave here or do everything
from the interrupt handler.

That leaves the last_completed variable. This apparently
needs to be protected using a spinlock, because
it can be read from other tasks calling mxs_dma_tx_status,
which in turn does not take the lock, so that is broken, too.

	Arnd

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

[Russell King - ARM Linux]

On Thu, Feb 17, 2011 at 03:38:30PM +0100, Arnd Bergmann wrote:
> On Monday 14 February 2011, Shawn Guo wrote:
> > +static void mxs_dma_tasklet(unsigned long data)
> > +{
> > +	struct mxs_dma_chan *mxs_chan = (struct mxs_dma_chan *) data;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&mxs_chan->lock, flags);
> > +
> > +	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;
> > +
> > +	spin_unlock_irqrestore(&mxs_chan->lock, flags);
> > +}
> 
> Here is a different problem. The entire tasklet has interrupts
> disabled and holds the spinlock that the interrupt handler does
> not hold.
> 
> This actually makes the tasklet a stricter context than the
> actual interrupt handler, defeating the purpose of tasklets
> (i.e. that you can run code without disabling interrupts).
> 
> I don't know what the rules for dmaengine callbacks are regarding
> the context in which they are called, but it seems you should
> either get rid of the spin_lock_irqsave here or do everything
> from the interrupt handler.

dmaengine callbacks should be made from tasklet context with no locks
held.

The generally accepted way to do this is:

	dma_async_tx_callback callback;
	void *callback_param;

	spin_lock_irqsave(&mxs_chan->lock, flags);
	callback = mxs_chan->desc.callback;
	callback_param = mxs_chan->desc.callback_param;

	if (mxs_chan->status == DMA_SUCCESS)
		mxs_chan->last_completed = mxs_chan->desc.cookie;
	
	spin_unlock_irqrestore(&mxs_chan->lock, flags);

	if (callback)
		callback(callback_param);

which also avoids the locking issue when the callback starts the next
MMC request.

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

[Shawn Guo]

On Thu, Feb 17, 2011 at 03:38:30PM +0100, Arnd Bergmann wrote:
> On Monday 14 February 2011, 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.
> > 
> > * 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 driver refers to imx-sdma and only a single descriptor is
> >   statically assigned to each channel.
> 
> Hi Shawn,
> 
Hi Arnd,

> I took a look at this after our discussion about the MMC driver locking

Thanks for reviewing the dma code too.

> problem. My feeling is that you still need to get a better understanding
> of the locking interfaces in Linux.
> 
Yes, I'm still learning Linux ;)

> > +struct mxs_dma_ccw {
> > +	u32		next;
> > +	u16		bits;
> > +	u16		xfer_bytes;
> > +#define MAX_XFER_BYTES	0xff00
> > +	dma_addr_t	bufaddr;
> > +#define MXS_PIO_WORDS	16
> > +	u32		pio_words[MXS_PIO_WORDS];
> > +};
> 
> Unrelated, but should bufaddr really be dma_addr_t? If this is a hardware
> structure, you should make it u32, because dma_addr_t may be either 32 or
> 64 bit in the future, as we get to multiplatform kernels.
> 
OK.

> > +struct mxs_dma_chan {
> > +	struct mxs_dma_engine		*mxs_dma;
> > +	struct dma_chan			chan;
> > +	struct dma_async_tx_descriptor	desc;
> > +	struct tasklet_struct		tasklet;
> > +	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 {
> > +	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];
> > +};
> 
> So you have locking per channel, but there is no lock in the dma engine
> structure locking the global fields. This is probably fine.
> 
> > +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));
> > +}
> 
> Just like the MMC driver, you also use __raw_writel(). If you have to do
> that, please encapsulate the access into a wrapper function that adds
> all the necessary serialization and add a comment explaining why you
> cannot use writel()/writel_relaxed().
> 
> Or just use writel().
> 
OK.  Will change to readl/writel pair.

> > +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;
> > +}
> 
> Should these take the spinlock? What if they are called simultaneously on
> multiple CPUs, or while the interrupt handler is running on another CPU?
> 
> Without a lock, another thread might see an incorrect value of mxs_chan->status.
> 
The mxs dma hardware somehow simplified the design to dedicate each
channel to a specific client device, e.g. mmc 0 has to use channel 0.
And I simplified the driver design accordingly with less locking
since only that specific client device will access the channel, in
which case the situation you are concerning very likely will not
happen.

> > +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;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&mxs_chan->lock, flags);
> > +
> > +	cookie = mxs_dma_assign_cookie(mxs_chan);
> > +
> > +	mxs_dma_enable_chan(mxs_chan);
> > +
> > +	spin_unlock_irqrestore(&mxs_chan->lock, flags);
> > +
> > +	return cookie;
> > +}
> 
> Just like the MMC driver, you use spin_lock_irqsave() in the functions that
> are called by other drivers, but then don't actually lock in the interrupt
> handler. This is clearly wrong, as I explained before:
> 
> The interrupt handler may already be running on another CPU, so it
> is not at all serialized with this code.
> 
I may be wrong on this point, but spin_lock_irqsave/irqrestore pair
is only for the protection you described?  I learnt that the pair
should be used to protect the critical region, where I'm not sure
if it's called from interrupt context.  Here I'm intending to protect
the dma registers accessed in mxs_dma_enable_chan.

> > +static void mxs_dma_tasklet(unsigned long data)
> > +{
> > +	struct mxs_dma_chan *mxs_chan = (struct mxs_dma_chan *) data;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&mxs_chan->lock, flags);
> > +
> > +	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;
> > +
> > +	spin_unlock_irqrestore(&mxs_chan->lock, flags);
> > +}
> 
> Here is a different problem. The entire tasklet has interrupts
> disabled and holds the spinlock that the interrupt handler does
> not hold.
> 
> This actually makes the tasklet a stricter context than the
> actual interrupt handler, defeating the purpose of tasklets
> (i.e. that you can run code without disabling interrupts).
> 
> I don't know what the rules for dmaengine callbacks are regarding
> the context in which they are called, but it seems you should
> either get rid of the spin_lock_irqsave here or do everything
> from the interrupt handler.
> 
> That leaves the last_completed variable. This apparently
> needs to be protected using a spinlock, because
> it can be read from other tasks calling mxs_dma_tx_status,
> which in turn does not take the lock, so that is broken, too.
> 
Thanks for teaching.  I intend to take the fix that Russell offered.
(Thanks, Russell).  Does that look good to you, or spin_lock_bh
version should be used than spin_lock_irqsave?

-- 
Regards,
Shawn

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

[Arnd Bergmann]

On Friday 18 February 2011, Shawn Guo wrote:
> > > +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;
> > > +   unsigned long flags;
> > > +
> > > +   spin_lock_irqsave(&mxs_chan->lock, flags);
> > > +
> > > +   cookie = mxs_dma_assign_cookie(mxs_chan);
> > > +
> > > +   mxs_dma_enable_chan(mxs_chan);
> > > +
> > > +   spin_unlock_irqrestore(&mxs_chan->lock, flags);
> > > +
> > > +   return cookie;
> > > +}
> > 
> > Just like the MMC driver, you use spin_lock_irqsave() in the functions that
> > are called by other drivers, but then don't actually lock in the interrupt
> > handler. This is clearly wrong, as I explained before:
> > 
> > The interrupt handler may already be running on another CPU, so it
> > is not at all serialized with this code.
> > 
> I may be wrong on this point, but spin_lock_irqsave/irqrestore pair
> is only for the protection you described?  I learnt that the pair
> should be used to protect the critical region, where I'm not sure
> if it's called from interrupt context.  Here I'm intending to protect
> the dma registers accessed in mxs_dma_enable_chan.

To generalize this further, a lock should protect specific data,
not portions of the code, and ideally you should document
exactly what you protect near the definition of the lock.

If you use the lock to protect all the DMA registers, you should use
it consistently, including inside of the interrupt handler, where
you access some other registers.

If you can prove that the registers used in the IRQ handler are
totally independent of the other ones, you don't need to use the
lock there, but then I'd advise documenting your assumptions.

> Thanks for teaching.  I intend to take the fix that Russell offered.
> (Thanks, Russell).  Does that look good to you, or spin_lock_bh
> version should be used than spin_lock_irqsave?

Russell's change looks correct to me.

Regarding the choice between spin_lock_bh and spin_lock_irqsave,
I always prefer to use the one that expresses best to the reader
what the intention is. spin_lock_irqsave() is stronger than
spin_lock_bh(), and can always be used in its place.

A short form of the strict rules (there are better documentations
out there) is:

* If all users are outside of interrupt or tasklet context,
  use a bare spin_lock().

* If one user is in a tasklet context, use spin_lock() inside
  the tasklet, but spin_lock_bh() outside, to prevent the
  tasklet from interrupting the critical section. Code that
  can be called in either tasklet or regular context needs
  to use spin_lock_bh() as well.

* If one user is in interrupt context, use spin_lock() inside
  of the interrupt handler, but spin_lock_irq() in tasklet
  and normal context (not spin_lock_irqsave()), to prevent
  the interrupt from happening during the critical section.

* Use spin_lock_irqsave() only for functions that can be called
  in either interrupt or non-interrupt context. Most drivers
  don't need this at all.

The simplified rule would be to always use spin_lock_irqsave(),
because that does not require you to understand what you are doing.

My position is that it is better to use the stricter rules, because
that documents that you actually do understand what you are doing ;-)
It's also slightly more efficient, because it avoids having to
save the interrupt status in a variable.

	Arnd

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

[Shawn Guo]

On Fri, Feb 18, 2011 at 02:22:39PM +0100, Arnd Bergmann wrote:
> On Friday 18 February 2011, Shawn Guo wrote:
> > > > +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;
> > > > +   unsigned long flags;
> > > > +
> > > > +   spin_lock_irqsave(&mxs_chan->lock, flags);
> > > > +
> > > > +   cookie = mxs_dma_assign_cookie(mxs_chan);
> > > > +
> > > > +   mxs_dma_enable_chan(mxs_chan);
> > > > +
> > > > +   spin_unlock_irqrestore(&mxs_chan->lock, flags);
> > > > +
> > > > +   return cookie;
> > > > +}
> > > 
> > > Just like the MMC driver, you use spin_lock_irqsave() in the functions that
> > > are called by other drivers, but then don't actually lock in the interrupt
> > > handler. This is clearly wrong, as I explained before:
> > > 
> > > The interrupt handler may already be running on another CPU, so it
> > > is not at all serialized with this code.
> > > 
> > I may be wrong on this point, but spin_lock_irqsave/irqrestore pair
> > is only for the protection you described?  I learnt that the pair
> > should be used to protect the critical region, where I'm not sure
> > if it's called from interrupt context.  Here I'm intending to protect
> > the dma registers accessed in mxs_dma_enable_chan.
> 
> To generalize this further, a lock should protect specific data,
> not portions of the code, and ideally you should document
> exactly what you protect near the definition of the lock.
> 
> If you use the lock to protect all the DMA registers, you should use
> it consistently, including inside of the interrupt handler, where
> you access some other registers.
> 
> If you can prove that the registers used in the IRQ handler are
> totally independent of the other ones, you don't need to use the
> lock there, but then I'd advise documenting your assumptions.
> 
When I change code to use readl/writel pair, I found the register
protection in mxs_dma_enable_chan is actually unnecessary, because
the registers are either channel specific or get set/clear address
to avoid read-modify-write operation.

So I'm removing the locking there.

> > Thanks for teaching.  I intend to take the fix that Russell offered.
> > (Thanks, Russell).  Does that look good to you, or spin_lock_bh
> > version should be used than spin_lock_irqsave?
> 
> Russell's change looks correct to me.
> 
> Regarding the choice between spin_lock_bh and spin_lock_irqsave,
> I always prefer to use the one that expresses best to the reader
> what the intention is. spin_lock_irqsave() is stronger than
> spin_lock_bh(), and can always be used in its place.
> 
> A short form of the strict rules (there are better documentations
> out there) is:
> 
> * If all users are outside of interrupt or tasklet context,
>   use a bare spin_lock().
> 
> * If one user is in a tasklet context, use spin_lock() inside
>   the tasklet, but spin_lock_bh() outside, to prevent the
>   tasklet from interrupting the critical section. Code that
>   can be called in either tasklet or regular context needs
>   to use spin_lock_bh() as well.
> 
> * If one user is in interrupt context, use spin_lock() inside
>   of the interrupt handler, but spin_lock_irq() in tasklet
>   and normal context (not spin_lock_irqsave()), to prevent
>   the interrupt from happening during the critical section.
> 
> * Use spin_lock_irqsave() only for functions that can be called
>   in either interrupt or non-interrupt context. Most drivers
>   don't need this at all.
> 
> The simplified rule would be to always use spin_lock_irqsave(),
> because that does not require you to understand what you are doing.
> 
> My position is that it is better to use the stricter rules, because
> that documents that you actually do understand what you are doing ;-)
> It's also slightly more efficient, because it avoids having to
> save the interrupt status in a variable.
> 
Thanks for this document.  It helps.

I'm changing the tasklet to have callback only, and move others into
interrupt handler, so that I can remove the locking completely from
the driver.

-- 
Regards,
Shawn