Re: [PATCH V4] MXS: Implement DMA support into mxs-i2c

[Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>]

Dear Wolfram Sang,

> On Fri, Aug 24, 2012 at 05:44:31AM +0200, Marek Vasut wrote:
> > This patch implements DMA support into mxs-i2c. DMA transfers are now
> > enabled via DT. The DMA operation is enabled by default.
> > 
> > Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> 
> Unsurprisingly, I also couldn't get the mode switching between DMA and
> PIOQUEUE to work :(

I'd say more work should be invested into this. Besides, what about rather 
trying PIO + DMA instead of PIOQ + DMA?

> However, I did come up with an idea how to select DMA or PIOQUEUE per
> bus via devicetree. If we change the view from "I want PIOQUEUE" to "I
> don't want DMA", we could simply override the default DMA configuration
> in the devicetree with, e.g.:
> 
> 			i2c0: i2c@80058000 {
> 				...
> 				fsl,i2c-dma-channel = <>;
> 			};
> 
> So, no DMA channel set up will fall back to PIOQUEUE. This is more
> generic than a custom binding, because if one doesn't want DMA, don't
> configure it. That should also work with other devices which can fall
> back to something. I think this is worth trying, another advantage is
> that it doesn't expose something new to the user. So, there is no legacy
> support needed in case there will be a better way of configuration.

Ewww ... that's such an ad-hoc hack. Besides, someone might simply forget to add 
the binding, you'd need to print a warning.

> So, here is a proof-of-concept patch which adds that to Marek's DMA
> implementation which I'd like to fold into it. I am about to prepare a
> proper series and will post it later today. Comments appreciated.
> 
> Thanks,
> 
>    Wolfram
> 
> diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
> index 1f58197..0198a43 100644
> --- a/drivers/i2c/busses/i2c-mxs.c
> +++ b/drivers/i2c/busses/i2c-mxs.c
> @@ -35,10 +35,6 @@
> 
>  #define DRIVER_NAME "mxs-i2c"
> 
> -static bool use_pioqueue;
> -module_param(use_pioqueue, bool, 0);
> -MODULE_PARM_DESC(use_pioqueue, "Use PIOQUEUE mode for transfer instead of
> DMA"); -
>  #define MXS_I2C_CTRL0		(0x00)
>  #define MXS_I2C_CTRL0_SET	(0x04)
> 
> @@ -556,24 +552,13 @@ static int mxs_i2c_get_ofdata(struct mxs_i2c_dev
> *i2c) int ret;
> 
>  	/*
> -	 * The MXS I2C DMA mode is prefered and enabled by default.
> -	 * The PIO mode is still supported, but should be used only
> -	 * for debuging purposes etc.
> -	 */
> -	i2c->dma_mode = !use_pioqueue;
> -	if (!i2c->dma_mode)
> -		dev_info(dev, "Using PIOQUEUE mode for I2C transfers!\n");
> -
> -	/*
>  	 * TODO: This is a temporary solution and should be changed
>  	 * to use generic DMA binding later when the helpers get in.
>  	 */
>  	ret = of_property_read_u32(node, "fsl,i2c-dma-channel",
>  				   &i2c->dma_channel);
> -	if (ret) {
> -		dev_warn(dev, "Failed to get DMA channel, using PIOQUEUE!\n");
> -		i2c->dma_mode = 0;
> -	}
> +	if (ret)
> +		i2c->dma_mode = false;
> 
>  	ret = of_property_read_u32(node, "clock-frequency", &speed);
>  	if (ret)
> @@ -624,6 +609,13 @@ static int __devinit mxs_i2c_probe(struct
> platform_device *pdev) if (err)
>  		return err;
> 
> +	/*
> +	 * The MXS I2C DMA mode is prefered and enabled by default.
> +	 * Ideally, we'd switch between PIOQUEUE and DMA depending on the
> +	 * message size, but neither Marek Vasut nor Wolfram Sang got this
> +	 * to work without resetting the controller completely :(
> +	 */
> +	i2c->dma_mode = true;
>  	i2c->dev = dev;
>  	i2c->speed = &mxs_i2c_95kHz_config;
> 
> @@ -640,8 +632,8 @@ static int __devinit mxs_i2c_probe(struct
> platform_device *pdev) i2c->dma_data.chan_irq = dmairq;
>  		i2c->dmach = dma_request_channel(mask, mxs_i2c_dma_filter, i2c);
>  		if (!i2c->dmach) {
> -			dev_err(dev, "Failed to request dma\n");
> -			return -ENODEV;
> +			dev_err(dev, "Failed to request dma, falling back\n");
> +			i2c->dma_mode = false;
>  		}
>  	}
> 
> @@ -668,6 +660,8 @@ static int __devinit mxs_i2c_probe(struct
> platform_device *pdev)
> 
>  	of_i2c_register_devices(adap);
> 
> +	dev_info(dev, "registered. DMA: %s\n", i2c->dma_mode ? "on" : "off");

dev_debug() ?

> +
>  	return 0;
>  }