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

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

[Marek Vasut]

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>
CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
CC: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 Documentation/devicetree/bindings/i2c/i2c-mxs.txt |    2 +
 arch/arm/boot/dts/imx28.dtsi                      |    2 +
 drivers/i2c/busses/i2c-mxs.c                      |  269 +++++++++++++++++++--
 3 files changed, 251 insertions(+), 22 deletions(-)

V2: Fixed return value from mxs_i2c_dma_setup_xfer().
    Fixed coding style nitpicks
    Call mxs_i2c_dma_finish() in failpath only if DMA is active
V3: Align with changes in previous patch
V4: Succumb to nonsense for the greater good ... like working EEPROMs
    Add "use_pioqueue" module parameter

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
index 30ac3a0..7a3fe9e 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
@@ -6,6 +6,7 @@ Required properties:
 - interrupts: Should contain ERROR and DMA interrupts
 - clock-frequency: Desired I2C bus clock frequency in Hz.
                    Only 100000Hz and 400000Hz modes are supported.
+- fsl,i2c-dma-channel: APBX DMA channel for the I2C
 
 Examples:
 
@@ -16,4 +17,5 @@ i2c0: i2c@80058000 {
 	reg = <0x80058000 2000>;
 	interrupts = <111 68>;
 	clock-frequency = <100000>;
+	fsl,i2c-dma-channel = <6>;
 };
diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
index 3fa6d19..ba8227e 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -661,6 +661,7 @@
 				reg = <0x80058000 0x2000>;
 				interrupts = <111 68>;
 				clock-frequency = <100000>;
+				fsl,i2c-dma-channel = <6>;
 				status = "disabled";
 			};
 
@@ -671,6 +672,7 @@
 				reg = <0x8005a000 0x2000>;
 				interrupts = <110 69>;
 				clock-frequency = <100000>;
+				fsl,i2c-dma-channel = <7>;
 				status = "disabled";
 			};
 
diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 088c5c1..83c3a7e 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -7,8 +7,6 @@
  *
  * Copyright (C) 2009-2010 Freescale Semiconductor, Inc. All Rights Reserved.
  *
- * TODO: add dma-support if platform-support for it is available
- *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; either version 2 of the License, or
@@ -31,9 +29,16 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_i2c.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/fsl/mxs-dma.h>
 
 #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)
 
@@ -146,6 +151,16 @@ struct mxs_i2c_dev {
 	u32 cmd_err;
 	struct i2c_adapter adapter;
 	const struct mxs_i2c_speed_config *speed;
+
+	/* DMA support components */
+	bool				dma_mode;
+	int				dma_channel;
+	struct dma_chan         	*dmach;
+	struct mxs_dma_data		dma_data;
+	uint32_t			pio_data[2];
+	uint32_t			addr_data;
+	struct scatterlist		sg_io[2];
+	bool				dma_read;
 };
 
 static void mxs_i2c_reset(struct mxs_i2c_dev *i2c)
@@ -157,7 +172,11 @@ static void mxs_i2c_reset(struct mxs_i2c_dev *i2c)
 	writel(i2c->speed->timing2, i2c->regs + MXS_I2C_TIMING2);
 
 	writel(MXS_I2C_IRQ_MASK << 8, i2c->regs + MXS_I2C_CTRL1_SET);
-	writel(MXS_I2C_QUEUECTRL_PIO_QUEUE_MODE,
+	if (i2c->dma_mode)
+		writel(MXS_I2C_QUEUECTRL_PIO_QUEUE_MODE,
+			i2c->regs + MXS_I2C_QUEUECTRL_CLR);
+	else
+		writel(MXS_I2C_QUEUECTRL_PIO_QUEUE_MODE,
 			i2c->regs + MXS_I2C_QUEUECTRL_SET);
 }
 
@@ -248,6 +267,150 @@ static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf, int len)
 	return 0;
 }
 
+static void mxs_i2c_dma_finish(struct mxs_i2c_dev *i2c)
+{
+	if (i2c->dma_read) {
+		dma_unmap_sg(i2c->dev, &i2c->sg_io[0], 1, DMA_TO_DEVICE);
+		dma_unmap_sg(i2c->dev, &i2c->sg_io[1], 1, DMA_FROM_DEVICE);
+	} else {
+		dma_unmap_sg(i2c->dev, i2c->sg_io, 2, DMA_TO_DEVICE);
+	}
+}
+
+static void mxs_i2c_dma_irq_callback(void *param)
+{
+	struct mxs_i2c_dev *i2c = param;
+
+	complete(&i2c->cmd_complete);
+	mxs_i2c_dma_finish(i2c);
+}
+
+static int mxs_i2c_dma_setup_xfer(struct i2c_adapter *adap,
+			struct i2c_msg *msg, uint32_t flags)
+{
+	struct dma_async_tx_descriptor *desc;
+	struct mxs_i2c_dev *i2c = i2c_get_adapdata(adap);
+
+	if (msg->flags & I2C_M_RD) {
+		i2c->dma_read = 1;
+		i2c->addr_data = (msg->addr << 1) | I2C_SMBUS_READ;
+
+		/*
+		 * SELECT command.
+		 */
+
+		/* Queue the PIO register write transfer. */
+		i2c->pio_data[0] = MXS_CMD_I2C_SELECT;
+		desc = dmaengine_prep_slave_sg(i2c->dmach,
+					(struct scatterlist *)&i2c->pio_data[0],
+					1, DMA_TRANS_NONE, 0);
+		if (!desc) {
+			dev_err(i2c->dev,
+				"Failed to get PIO reg. write descriptor.\n");
+			goto select_init_pio_fail;
+		}
+
+		/* Queue the DMA data transfer. */
+		sg_init_one(&i2c->sg_io[0], &i2c->addr_data, 1);
+		dma_map_sg(i2c->dev, &i2c->sg_io[0], 1, DMA_TO_DEVICE);
+		desc = dmaengine_prep_slave_sg(i2c->dmach, &i2c->sg_io[0], 1,
+					DMA_MEM_TO_DEV,
+					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+		if (!desc) {
+			dev_err(i2c->dev,
+				"Failed to get DMA data write descriptor.\n");
+			goto select_init_dma_fail;
+		}
+
+		/*
+		 * READ command.
+		 */
+
+		/* Queue the PIO register write transfer. */
+		i2c->pio_data[1] = flags | MXS_CMD_I2C_READ |
+				MXS_I2C_CTRL0_XFER_COUNT(msg->len);
+		desc = dmaengine_prep_slave_sg(i2c->dmach,
+					(struct scatterlist *)&i2c->pio_data[1],
+					1, DMA_TRANS_NONE, DMA_PREP_INTERRUPT);
+		if (!desc) {
+			dev_err(i2c->dev,
+				"Failed to get PIO reg. write descriptor.\n");
+			goto select_init_dma_fail;
+		}
+
+		/* Queue the DMA data transfer. */
+		sg_init_one(&i2c->sg_io[1], msg->buf, msg->len);
+		dma_map_sg(i2c->dev, &i2c->sg_io[1], 1, DMA_FROM_DEVICE);
+		desc = dmaengine_prep_slave_sg(i2c->dmach, &i2c->sg_io[1], 1,
+					DMA_DEV_TO_MEM,
+					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+		if (!desc) {
+			dev_err(i2c->dev,
+				"Failed to get DMA data write descriptor.\n");
+			goto read_init_dma_fail;
+		}
+	} else {
+		i2c->dma_read = 0;
+		i2c->addr_data = (msg->addr << 1) | I2C_SMBUS_WRITE;
+
+		/*
+		 * WRITE command.
+		 */
+
+		/* Queue the PIO register write transfer. */
+		i2c->pio_data[0] = flags | MXS_CMD_I2C_WRITE |
+				MXS_I2C_CTRL0_XFER_COUNT(msg->len + 1);
+		desc = dmaengine_prep_slave_sg(i2c->dmach,
+					(struct scatterlist *)&i2c->pio_data[0],
+					1, DMA_TRANS_NONE, 0);
+		if (!desc) {
+			dev_err(i2c->dev,
+				"Failed to get PIO reg. write descriptor.\n");
+			goto write_init_pio_fail;
+		}
+
+		/* Queue the DMA data transfer. */
+		sg_init_table(i2c->sg_io, 2);
+		sg_set_buf(&i2c->sg_io[0], &i2c->addr_data, 1);
+		sg_set_buf(&i2c->sg_io[1], msg->buf, msg->len);
+		dma_map_sg(i2c->dev, i2c->sg_io, 2, DMA_TO_DEVICE);
+		desc = dmaengine_prep_slave_sg(i2c->dmach, i2c->sg_io, 2,
+					DMA_MEM_TO_DEV,
+					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+		if (!desc) {
+			dev_err(i2c->dev,
+				"Failed to get DMA data write descriptor.\n");
+			goto write_init_dma_fail;
+		}
+	}
+
+	/*
+	 * The last descriptor must have this callback,
+	 * to finish the DMA transaction.
+	 */
+	desc->callback = mxs_i2c_dma_irq_callback;
+	desc->callback_param = i2c;
+
+	/* Start the transfer. */
+	dmaengine_submit(desc);
+	dma_async_issue_pending(i2c->dmach);
+	return 0;
+
+/* Read failpath. */
+read_init_dma_fail:
+	dma_unmap_sg(i2c->dev, &i2c->sg_io[1], 1, DMA_FROM_DEVICE);
+select_init_dma_fail:
+	dma_unmap_sg(i2c->dev, &i2c->sg_io[0], 1, DMA_TO_DEVICE);
+select_init_pio_fail:
+	return -EINVAL;
+
+/* Write failpath. */
+write_init_dma_fail:
+	dma_unmap_sg(i2c->dev, i2c->sg_io, 2, DMA_TO_DEVICE);
+write_init_pio_fail:
+	return -EINVAL;
+}
+
 /*
  * Low level master read/write transaction.
  */
@@ -258,6 +421,8 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 	int ret;
 	int flags;
 
+	flags = stop ? MXS_I2C_CTRL0_POST_SEND_STOP : 0;
+
 	dev_dbg(i2c->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",
 		msg->addr, msg->len, msg->flags, stop);
 
@@ -267,23 +432,29 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 	init_completion(&i2c->cmd_complete);
 	i2c->cmd_err = 0;
 
-	flags = stop ? MXS_I2C_CTRL0_POST_SEND_STOP : 0;
-
-	if (msg->flags & I2C_M_RD)
-		mxs_i2c_pioq_setup_read(i2c, msg->addr, msg->len, flags);
-	else
-		mxs_i2c_pioq_setup_write(i2c, msg->addr, msg->buf, msg->len,
-					flags);
+	if (i2c->dma_mode) {
+		ret = mxs_i2c_dma_setup_xfer(adap, msg, flags);
+		if (ret)
+			return ret;
+	} else {
+		if (msg->flags & I2C_M_RD) {
+			mxs_i2c_pioq_setup_read(i2c, msg->addr,
+						msg->len, flags);
+		} else {
+			mxs_i2c_pioq_setup_write(i2c, msg->addr, msg->buf,
+						msg->len, flags);
+		}
 
-	writel(MXS_I2C_QUEUECTRL_QUEUE_RUN,
+		writel(MXS_I2C_QUEUECTRL_QUEUE_RUN,
 			i2c->regs + MXS_I2C_QUEUECTRL_SET);
+	}
 
 	ret = wait_for_completion_timeout(&i2c->cmd_complete,
 						msecs_to_jiffies(1000));
 	if (ret == 0)
 		goto timeout;
 
-	if ((!i2c->cmd_err) && (msg->flags & I2C_M_RD)) {
+	if (!i2c->dma_mode && !i2c->cmd_err && (msg->flags & I2C_M_RD)) {
 		ret = mxs_i2c_finish_read(i2c, msg->buf, msg->len);
 		if (ret)
 			goto timeout;
@@ -301,6 +472,8 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 
 timeout:
 	dev_dbg(i2c->dev, "Timeout!\n");
+	if (i2c->dma_mode)
+		mxs_i2c_dma_finish(i2c);
 	mxs_i2c_reset(i2c);
 	return -ETIMEDOUT;
 }
@@ -342,11 +515,13 @@ static irqreturn_t mxs_i2c_isr(int this_irq, void *dev_id)
 		/* MXS_I2C_CTRL1_OVERSIZE_XFER_TERM_IRQ is only for slaves */
 		i2c->cmd_err = -EIO;
 
-	is_last_cmd = (readl(i2c->regs + MXS_I2C_QUEUESTAT) &
-		MXS_I2C_QUEUESTAT_WRITE_QUEUE_CNT_MASK) == 0;
+	if (!i2c->dma_mode) {
+		is_last_cmd = (readl(i2c->regs + MXS_I2C_QUEUESTAT) &
+			MXS_I2C_QUEUESTAT_WRITE_QUEUE_CNT_MASK) == 0;
 
-	if (is_last_cmd || i2c->cmd_err)
-		complete(&i2c->cmd_complete);
+		if (is_last_cmd || i2c->cmd_err)
+			complete(&i2c->cmd_complete);
+	}
 
 	writel(stat, i2c->regs + MXS_I2C_CTRL1_CLR);
 
@@ -358,6 +533,21 @@ static const struct i2c_algorithm mxs_i2c_algo = {
 	.functionality = mxs_i2c_func,
 };
 
+static bool mxs_i2c_dma_filter(struct dma_chan *chan, void *param)
+{
+	struct mxs_i2c_dev *i2c = param;
+
+	if (!mxs_dma_is_apbx(chan))
+		return false;
+
+	if (chan->chan_id != i2c->dma_channel)
+		return false;
+
+	chan->private = &i2c->dma_data;
+
+	return true;
+}
+
 static int mxs_i2c_get_ofdata(struct mxs_i2c_dev *i2c)
 {
 	uint32_t speed;
@@ -368,6 +558,26 @@ static int mxs_i2c_get_ofdata(struct mxs_i2c_dev *i2c)
 	if (!node)
 		return -EINVAL;
 
+	/*
+	 * 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;
+	}
+
 	i2c->speed = &mxs_i2c_95kHz_config;
 	ret = of_property_read_u32(node, "clock-frequency", &speed);
 	if (ret)
@@ -388,7 +598,8 @@ static int __devinit mxs_i2c_probe(struct platform_device *pdev)
 	struct pinctrl *pinctrl;
 	struct resource *res;
 	resource_size_t res_size;
-	int err, irq;
+	int err, irq, dmairq;
+	dma_cap_mask_t mask;
 
 	pinctrl = devm_pinctrl_get_select_default(dev);
 	if (IS_ERR(pinctrl))
@@ -399,7 +610,10 @@ static int __devinit mxs_i2c_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
+	irq = platform_get_irq(pdev, 0);
+	dmairq = platform_get_irq(pdev, 1);
+
+	if (!res || irq < 0 || dmairq < 0)
 		return -ENOENT;
 
 	res_size = resource_size(res);
@@ -410,10 +624,6 @@ static int __devinit mxs_i2c_probe(struct platform_device *pdev)
 	if (!i2c->regs)
 		return -EBUSY;
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		return irq;
-
 	err = devm_request_irq(dev, irq, mxs_i2c_isr, 0, dev_name(dev), i2c);
 	if (err)
 		return err;
@@ -424,6 +634,18 @@ static int __devinit mxs_i2c_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
+	/* Setup the DMA */
+	if (i2c->dma_mode) {
+		dma_cap_zero(mask);
+		dma_cap_set(DMA_SLAVE, mask);
+		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;
+		}
+	}
+
 	platform_set_drvdata(pdev, i2c);
 
 	/* Do reset to enforce correct startup after pinmuxing */
@@ -459,6 +681,9 @@ static int __devexit mxs_i2c_remove(struct platform_device *pdev)
 	if (ret)
 		return -EBUSY;
 
+	if (i2c->dmach)
+		dma_release_channel(i2c->dmach);
+
 	writel(MXS_I2C_CTRL0_SFTRST, i2c->regs + MXS_I2C_CTRL0_SET);
 
 	platform_set_drvdata(pdev, NULL);
-- 
1.7.10.4


------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

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

[Fabio Estevam]

On Fri, Aug 24, 2012 at 12:44 AM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> 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>
> CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> CC: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

On a mx28evk:

Tested-by: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>

Wolfram,

Adding dma support into mxs-i2c has also the benefit to allow i2c on
mx23 to work.

Please consider applying this one.

Thanks,

Fabio Estevam

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3893 bytes --]

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

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.

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

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 959 bytes --]

On Mon, Aug 27, 2012 at 03:54:30PM -0300, Fabio Estevam wrote:
> On Fri, Aug 24, 2012 at 12:44 AM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> 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>
> > CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > CC: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> 
> On a mx28evk:
> 
> Tested-by: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>

Thanks, may I ask what your tests were?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Marek Vasut]

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

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1740 bytes --]


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

I ran out of ideas. Next thing I'd do is to ask FSL if this is possible
at all. Yet, I'd like to have some solution for 3.7.

> Besides, what about rather > trying PIO + DMA instead of PIOQ + DMA?

That could also be a solution.

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

Please give reasons.

> Besides, someone might simply forget to add the binding, you'd need to
> print a warning.

? It is "on" by default because of the entry in the dtsi. And the status
of DMA will be printed.

> > +	dev_info(dev, "registered. DMA: %s\n", i2c->dma_mode ? "on" : "off");
> 
> dev_debug() ?

Yup.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Marek Vasut]

Dear Wolfram Sang,

> > > 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.
> 
> I ran out of ideas. Next thing I'd do is to ask FSL if this is possible
> at all. Yet, I'd like to have some solution for 3.7.

Why? You now have the module option solution.

> > Besides, what about rather > trying PIO + DMA instead of PIOQ + DMA?
> 
> That could also be a solution.
> 
> > > 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.
> 
> Please give reasons.

It's abuse of proper configuration of hardware property.

> > Besides, someone might simply forget to add the binding, you'd need to
> > print a warning.
> 
> ? It is "on" by default because of the entry in the dtsi. And the status
> of DMA will be printed.
> 
> > > +	dev_info(dev, "registered. DMA: %s\n", i2c->dma_mode ? "on" : "off");
> > 
> > dev_debug() ?
> 
> Yup.

Best regards,
Marek Vasut

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

[Fabio Estevam]

Hi Wolfram,

On Sun, Sep 16, 2012 at 7:44 AM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:

>> Tested-by: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>
> Thanks, may I ask what your tests were?

I used i2c-tools to read/write to an i2c at24 eeprom connected to my mx28evk.

Regards,

Fabio Estevam

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

[Marek Vasut]

Dear Wolfram Sang,

[...]

Didn't you claim you're adding this DMA patch into next some time ago finally? 
;-)

Subject: Re: I2C_FUNC_SMBUS_QUICK on i2c-mxs
Message-ID: <20120830135612.GJ27306-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

Best regards,
Marek Vasut

------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://ad.doubleclick.net/clk;258768047;13503038;j?
http://info.appdynamics.com/FreeJavaPerformanceDownload.html

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 687 bytes --]

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>

Thanks, applied to -next.

I am meanwhile thinking the driver needs massive changes, probably
dropping PIOQUEUE entirely. This is probably best done via incremental
patches to this one. I have sketches which were sadly not ready for this
merge-window :(

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Marek Vasut]

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>
> 
> Thanks, applied to -next.
> 
> I am meanwhile thinking the driver needs massive changes

+1 it does.

> probably dropping PIOQUEUE entirely

Yes, in favor of PIO which can do switching with DMA.

> This is probably best done via incremental
> patches to this one. I have sketches which were sadly not ready for this
> merge-window :(

Cc me when submitting, I'd like to see those.

Best regards,
Marek Vasut

------------------------------------------------------------------------------
Don't let slow site performance ruin your business. Deploy New Relic APM
Deploy New Relic app performance management and know exactly
what is happening inside your Ruby, Python, PHP, Java, and .NET app
Try New Relic at no cost today and get our sweet Data Nerd shirt too!
http://p.sf.net/sfu/newrelic-dev2dev