Add dma support for i.MX23/28

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.

drivers/dma/mxs-dma.c new file +15 -0

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

drivers/dma/mxs-dma.c new file +24 -0

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

drivers/dma/mxs-dma.c new file +34 -0

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

drivers/dma/mxs-dma.c new file +24 -0

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

drivers/dma/mxs-dma.c new file +5 -0

+static void mxs_dma_tasklet(unsigned long data)
+{
+	struct mxs_dma_chan *mxs_chan = (struct mxs_dma_chan *) data;
+	unsigned long flags;
+

Quoted 5 lines · Shawn Guo

drivers/dma/mxs-dma.c new file +5 -0

+static void mxs_dma_tasklet(unsigned long data)
+{
+	struct mxs_dma_chan *mxs_chan = (struct mxs_dma_chan *) data;
+	unsigned long flags;
+

drivers/dma/mxs-dma.c line 262

Quoted 10 lines

drivers/dma/mxs-dma.c new file +10 -0

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

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

Quoted 6 lines

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

Quoted 5 lines

drivers/dma/mxs-dma.c new file +5 -0

+struct mxs_dma_ccw {
+	u32		next;
+	u16		bits;
+	u16		xfer_bytes;
+#define MAX_XFER_BYTES	0xff00

drivers/dma/mxs-dma.c line 103

Quoted 4 lines

drivers/dma/mxs-dma.c new file +4 -0

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

Quoted 15 lines

drivers/dma/mxs-dma.c new file +15 -0

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

drivers/dma/mxs-dma.c line 131

Quoted 10 lines

drivers/dma/mxs-dma.c new file +10 -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.

Quoted 24 lines

drivers/dma/mxs-dma.c new file +24 -0

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

drivers/dma/mxs-dma.c line 166

Quoted 10 lines

drivers/dma/mxs-dma.c new file +10 -0

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

Quoted 34 lines

drivers/dma/mxs-dma.c new file +34 -0

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

drivers/dma/mxs-dma.c line 211

Quoted 10 lines

drivers/dma/mxs-dma.c new file +10 -0

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

Quoted 24 lines

drivers/dma/mxs-dma.c new file +24 -0

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

drivers/dma/mxs-dma.c line 246

Quoted 10 lines

drivers/dma/mxs-dma.c new file +10 -0

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

Quoted 5 lines

drivers/dma/mxs-dma.c new file +5 -0

+static void mxs_dma_tasklet(unsigned long data)
+{
+	struct mxs_dma_chan *mxs_chan = (struct mxs_dma_chan *) data;
+	unsigned long flags;
+

drivers/dma/mxs-dma.c line 262

Quoted 10 lines

drivers/dma/mxs-dma.c new file +10 -0

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

Quoted 39 lines

Quoted 24 lines

drivers/dma/mxs-dma.c new file +24 -0

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

drivers/dma/mxs-dma.c line 246

Quoted 10 lines

drivers/dma/mxs-dma.c new file +10 -0

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

Quoted 3 lines

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.

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?

Quoted 39 lines · Shawn Guo

Quoted 39 lines

Quoted 24 lines

drivers/dma/mxs-dma.c new file +24 -0

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

drivers/dma/mxs-dma.c line 246

Quoted 10 lines

drivers/dma/mxs-dma.c new file +10 -0

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

Quoted 8 lines

Quoted 3 lines

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.

Quoted 3 lines

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.