[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 at 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