[PATCH 1/2 V3] MXS: Set I2C timing registers for mxs-i2c

[PATCH 1/2 V3] MXS: Set I2C timing registers for mxs-i2c

[Marek Vasut]

This patch configures the I2C bus timing registers according
to information passed via DT. Currently, 100kHz and 400kHz
modes are supported.

The TIMING2 register value is wrong in the documentation for
i.MX28! This was found and fixed by:
  Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Cc: Detlev Zundel <dzu-ynQEQJNshbs@public.gmane.org>
CC: Dong Aisheng <b29396-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Linux ARM kernel <linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
CC: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
CC: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Stefano Babic <sbabic-ynQEQJNshbs@public.gmane.org>
CC: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Wolfgang Denk <wd-ynQEQJNshbs@public.gmane.org>
Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 Documentation/devicetree/bindings/i2c/i2c-mxs.txt |    3 +
 arch/arm/boot/dts/imx28.dtsi                      |    2 +
 drivers/i2c/busses/i2c-mxs.c                      |   66 +++++++++++++++++++++
 3 files changed, 71 insertions(+)

V2: Fixed static const struct mxs_i2c_speed_config
V3: Use 100kHz by default
    Document support only for 100kHz and 400kHz
    Document why the bus is configured to 95kHz instead of 100kHz
    Replace "Invalid speed ..." message with "Unsupported speed ..." message

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
index 1bfc02d..30ac3a0 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
@@ -4,6 +4,8 @@ Required properties:
 - compatible: Should be "fsl,<chip>-i2c"
 - reg: Should contain registers location and length
 - interrupts: Should contain ERROR and DMA interrupts
+- clock-frequency: Desired I2C bus clock frequency in Hz.
+                   Only 100000Hz and 400000Hz modes are supported.
 
 Examples:
 
@@ -13,4 +15,5 @@ i2c0: i2c@80058000 {
 	compatible = "fsl,imx28-i2c";
 	reg = <0x80058000 2000>;
 	interrupts = <111 68>;
+	clock-frequency = <100000>;
 };
diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
index adb5ffc..e2e9a2b 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -586,6 +586,7 @@
 				compatible = "fsl,imx28-i2c";
 				reg = <0x80058000 2000>;
 				interrupts = <111 68>;
+				clock-frequency = <100000>;
 				status = "disabled";
 			};
 
@@ -595,6 +596,7 @@
 				compatible = "fsl,imx28-i2c";
 				reg = <0x8005a000 2000>;
 				interrupts = <110 69>;
+				clock-frequency = <100000>;
 				status = "disabled";
 			};
 
diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 04eb441..877b169 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -46,6 +46,10 @@
 #define MXS_I2C_CTRL0_DIRECTION			0x00010000
 #define MXS_I2C_CTRL0_XFER_COUNT(v)		((v) & 0x0000FFFF)
 
+#define MXS_I2C_TIMING0		(0x10)
+#define MXS_I2C_TIMING1		(0x20)
+#define MXS_I2C_TIMING2		(0x30)
+
 #define MXS_I2C_CTRL1		(0x40)
 #define MXS_I2C_CTRL1_SET	(0x44)
 #define MXS_I2C_CTRL1_CLR	(0x48)
@@ -97,6 +101,35 @@
 #define MXS_CMD_I2C_READ	(MXS_I2C_CTRL0_SEND_NAK_ON_LAST | \
 				 MXS_I2C_CTRL0_MASTER_MODE)
 
+struct mxs_i2c_speed_config {
+	uint32_t	timing0;
+	uint32_t	timing1;
+	uint32_t	timing2;
+};
+
+/*
+ * Timing values for the default 24MHz clock supplied into the i2c block.
+ *
+ * The bus can operate at 95kHz or at 400kHz with the following timing
+ * register configurations. The 100kHz mode isn't present because it's
+ * values are not stated in the i.MX233/i.MX28 datasheet. The 95kHz mode
+ * shall be close enough replacement. Therefore when the bus is configured
+ * for 100kHz operation, 95kHz timing settings are actually loaded.
+ *
+ * For details, see i.MX233 [25.4.2 - 25.4.4] and i.MX28 [27.5.2 - 27.5.4].
+ */
+static const struct mxs_i2c_speed_config mxs_i2c_95kHz_config = {
+	.timing0	= 0x00780030,
+	.timing1	= 0x00800030,
+	.timing2	= 0x00300030,
+};
+
+static const struct mxs_i2c_speed_config mxs_i2c_400kHz_config = {
+	.timing0	= 0x000f0007,
+	.timing1	= 0x001f000f,
+	.timing2	= 0x00300030,
+};
+
 /**
  * struct mxs_i2c_dev - per device, private MXS-I2C data
  *
@@ -112,11 +145,17 @@ struct mxs_i2c_dev {
 	struct completion cmd_complete;
 	u32 cmd_err;
 	struct i2c_adapter adapter;
+	const struct mxs_i2c_speed_config *speed;
 };
 
 static void mxs_i2c_reset(struct mxs_i2c_dev *i2c)
 {
 	stmp_reset_block(i2c->regs);
+
+	writel(i2c->speed->timing0, i2c->regs + MXS_I2C_TIMING0);
+	writel(i2c->speed->timing1, i2c->regs + MXS_I2C_TIMING1);
+	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,
 			i2c->regs + MXS_I2C_QUEUECTRL_SET);
@@ -319,6 +358,28 @@ static const struct i2c_algorithm mxs_i2c_algo = {
 	.functionality = mxs_i2c_func,
 };
 
+static int mxs_i2c_get_ofdata(struct mxs_i2c_dev *i2c)
+{
+	uint32_t speed;
+	struct device *dev = i2c->dev;
+	struct device_node *node = dev->of_node;
+	int ret;
+
+	if (!node)
+		return -EINVAL;
+
+	i2c->speed = &mxs_i2c_95kHz_config;
+	ret = of_property_read_u32(node, "clock-frequency", &speed);
+	if (ret)
+		dev_warn(dev, "No I2C speed selected, using 100kHz\n");
+	else if (speed == 400000)
+		i2c->speed = &mxs_i2c_400kHz_config;
+	else if (speed != 100000)
+		dev_warn(dev, "Unsupported I2C speed selected, using 100kHz\n");
+
+	return 0;
+}
+
 static int __devinit mxs_i2c_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -358,6 +419,11 @@ static int __devinit mxs_i2c_probe(struct platform_device *pdev)
 		return err;
 
 	i2c->dev = dev;
+
+	err = mxs_i2c_get_ofdata(i2c);
+	if (err)
+		return err;
+
 	platform_set_drvdata(pdev, i2c);
 
 	/* Do reset to enforce correct startup after pinmuxing */
-- 
1.7.10.4

[PATCH 1/2 V3] MXS: Set I2C timing registers for mxs-i2c

[Marek Vasut]

This patch configures the I2C bus timing registers according
to information passed via DT. Currently, 100kHz and 400kHz
modes are supported.

The TIMING2 register value is wrong in the documentation for
i.MX28! This was found and fixed by:
  Shawn Guo <shawn.guo@linaro.org>

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Detlev Zundel <dzu@denx.de>
CC: Dong Aisheng <b29396@freescale.com>
CC: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Linux ARM kernel <linux-arm-kernel@lists.infradead.org>
Cc: linux-i2c at vger.kernel.org
CC: Sascha Hauer <s.hauer@pengutronix.de>
CC: Shawn Guo <shawn.guo@linaro.org>
Cc: Stefano Babic <sbabic@denx.de>
CC: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Wolfram Sang <w.sang@pengutronix.de>
---
 Documentation/devicetree/bindings/i2c/i2c-mxs.txt |    3 +
 arch/arm/boot/dts/imx28.dtsi                      |    2 +
 drivers/i2c/busses/i2c-mxs.c                      |   66 +++++++++++++++++++++
 3 files changed, 71 insertions(+)

V2: Fixed static const struct mxs_i2c_speed_config
V3: Use 100kHz by default
    Document support only for 100kHz and 400kHz
    Document why the bus is configured to 95kHz instead of 100kHz
    Replace "Invalid speed ..." message with "Unsupported speed ..." message

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
index 1bfc02d..30ac3a0 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
@@ -4,6 +4,8 @@ Required properties:
 - compatible: Should be "fsl,<chip>-i2c"
 - reg: Should contain registers location and length
 - interrupts: Should contain ERROR and DMA interrupts
+- clock-frequency: Desired I2C bus clock frequency in Hz.
+                   Only 100000Hz and 400000Hz modes are supported.
 
 Examples:
 
@@ -13,4 +15,5 @@ i2c0: i2c at 80058000 {
 	compatible = "fsl,imx28-i2c";
 	reg = <0x80058000 2000>;
 	interrupts = <111 68>;
+	clock-frequency = <100000>;
 };
diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
index adb5ffc..e2e9a2b 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -586,6 +586,7 @@
 				compatible = "fsl,imx28-i2c";
 				reg = <0x80058000 2000>;
 				interrupts = <111 68>;
+				clock-frequency = <100000>;
 				status = "disabled";
 			};
 
@@ -595,6 +596,7 @@
 				compatible = "fsl,imx28-i2c";
 				reg = <0x8005a000 2000>;
 				interrupts = <110 69>;
+				clock-frequency = <100000>;
 				status = "disabled";
 			};
 
diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 04eb441..877b169 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -46,6 +46,10 @@
 #define MXS_I2C_CTRL0_DIRECTION			0x00010000
 #define MXS_I2C_CTRL0_XFER_COUNT(v)		((v) & 0x0000FFFF)
 
+#define MXS_I2C_TIMING0		(0x10)
+#define MXS_I2C_TIMING1		(0x20)
+#define MXS_I2C_TIMING2		(0x30)
+
 #define MXS_I2C_CTRL1		(0x40)
 #define MXS_I2C_CTRL1_SET	(0x44)
 #define MXS_I2C_CTRL1_CLR	(0x48)
@@ -97,6 +101,35 @@
 #define MXS_CMD_I2C_READ	(MXS_I2C_CTRL0_SEND_NAK_ON_LAST | \
 				 MXS_I2C_CTRL0_MASTER_MODE)
 
+struct mxs_i2c_speed_config {
+	uint32_t	timing0;
+	uint32_t	timing1;
+	uint32_t	timing2;
+};
+
+/*
+ * Timing values for the default 24MHz clock supplied into the i2c block.
+ *
+ * The bus can operate at 95kHz or at 400kHz with the following timing
+ * register configurations. The 100kHz mode isn't present because it's
+ * values are not stated in the i.MX233/i.MX28 datasheet. The 95kHz mode
+ * shall be close enough replacement. Therefore when the bus is configured
+ * for 100kHz operation, 95kHz timing settings are actually loaded.
+ *
+ * For details, see i.MX233 [25.4.2 - 25.4.4] and i.MX28 [27.5.2 - 27.5.4].
+ */
+static const struct mxs_i2c_speed_config mxs_i2c_95kHz_config = {
+	.timing0	= 0x00780030,
+	.timing1	= 0x00800030,
+	.timing2	= 0x00300030,
+};
+
+static const struct mxs_i2c_speed_config mxs_i2c_400kHz_config = {
+	.timing0	= 0x000f0007,
+	.timing1	= 0x001f000f,
+	.timing2	= 0x00300030,
+};
+
 /**
  * struct mxs_i2c_dev - per device, private MXS-I2C data
  *
@@ -112,11 +145,17 @@ struct mxs_i2c_dev {
 	struct completion cmd_complete;
 	u32 cmd_err;
 	struct i2c_adapter adapter;
+	const struct mxs_i2c_speed_config *speed;
 };
 
 static void mxs_i2c_reset(struct mxs_i2c_dev *i2c)
 {
 	stmp_reset_block(i2c->regs);
+
+	writel(i2c->speed->timing0, i2c->regs + MXS_I2C_TIMING0);
+	writel(i2c->speed->timing1, i2c->regs + MXS_I2C_TIMING1);
+	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,
 			i2c->regs + MXS_I2C_QUEUECTRL_SET);
@@ -319,6 +358,28 @@ static const struct i2c_algorithm mxs_i2c_algo = {
 	.functionality = mxs_i2c_func,
 };
 
+static int mxs_i2c_get_ofdata(struct mxs_i2c_dev *i2c)
+{
+	uint32_t speed;
+	struct device *dev = i2c->dev;
+	struct device_node *node = dev->of_node;
+	int ret;
+
+	if (!node)
+		return -EINVAL;
+
+	i2c->speed = &mxs_i2c_95kHz_config;
+	ret = of_property_read_u32(node, "clock-frequency", &speed);
+	if (ret)
+		dev_warn(dev, "No I2C speed selected, using 100kHz\n");
+	else if (speed == 400000)
+		i2c->speed = &mxs_i2c_400kHz_config;
+	else if (speed != 100000)
+		dev_warn(dev, "Unsupported I2C speed selected, using 100kHz\n");
+
+	return 0;
+}
+
 static int __devinit mxs_i2c_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -358,6 +419,11 @@ static int __devinit mxs_i2c_probe(struct platform_device *pdev)
 		return err;
 
 	i2c->dev = dev;
+
+	err = mxs_i2c_get_ofdata(i2c);
+	if (err)
+		return err;
+
 	platform_set_drvdata(pdev, i2c);
 
 	/* Do reset to enforce correct startup after pinmuxing */
-- 
1.7.10.4

[PATCH 2/2 V3] 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: Detlev Zundel <dzu-ynQEQJNshbs@public.gmane.org>
CC: Dong Aisheng <b29396-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Cc: Linux ARM kernel <linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
CC: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
CC: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Stefano Babic <sbabic-ynQEQJNshbs@public.gmane.org>
CC: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Wolfgang Denk <wd-ynQEQJNshbs@public.gmane.org>
Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 Documentation/devicetree/bindings/i2c/i2c-mxs.txt |    5 +
 arch/arm/boot/dts/imx28.dtsi                      |    2 +
 drivers/i2c/busses/i2c-mxs.c                      |  267 +++++++++++++++++++--
 3 files changed, 252 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

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
index 30ac3a0..45b6307 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
@@ -6,6 +6,10 @@ 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
+
+Optional properties:
+- fsl,use-pio: Use PIO transfers instead of DMA, useful for debug
 
 Examples:
 
@@ -16,4 +20,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 e2e9a2b..99bd037 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -587,6 +587,7 @@
 				reg = <0x80058000 2000>;
 				interrupts = <111 68>;
 				clock-frequency = <100000>;
+				fsl,i2c-dma-channel = <6>;
 				status = "disabled";
 			};
 
@@ -597,6 +598,7 @@
 				reg = <0x8005a000 2000>;
 				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 877b169..20290a6 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,6 +29,9 @@
 #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"
 
@@ -146,6 +147,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 +168,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 +263,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 +417,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 +428,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 +468,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 +511,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 +529,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 +554,28 @@ 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 = 1;
+	if (of_find_property(node, "fsl,use-pio", NULL)) {
+		i2c->dma_mode = 0;
+		dev_info(dev, "Using PIO 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!\n");
+		i2c->dma_mode = 0;
+	}
+
 	i2c->speed = &mxs_i2c_95kHz_config;
 	ret = of_property_read_u32(node, "clock-frequency", &speed);
 	if (ret)
@@ -388,7 +596,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 +608,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 +622,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 +632,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 +679,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

[PATCH 2/2 V3] 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@denx.de>
Cc: Detlev Zundel <dzu@denx.de>
CC: Dong Aisheng <b29396@freescale.com>
CC: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Linux ARM kernel <linux-arm-kernel@lists.infradead.org>
Cc: linux-i2c at vger.kernel.org
CC: Sascha Hauer <s.hauer@pengutronix.de>
CC: Shawn Guo <shawn.guo@linaro.org>
Cc: Stefano Babic <sbabic@denx.de>
CC: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Wolfram Sang <w.sang@pengutronix.de>
---
 Documentation/devicetree/bindings/i2c/i2c-mxs.txt |    5 +
 arch/arm/boot/dts/imx28.dtsi                      |    2 +
 drivers/i2c/busses/i2c-mxs.c                      |  267 +++++++++++++++++++--
 3 files changed, 252 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

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
index 30ac3a0..45b6307 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
@@ -6,6 +6,10 @@ 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
+
+Optional properties:
+- fsl,use-pio: Use PIO transfers instead of DMA, useful for debug
 
 Examples:
 
@@ -16,4 +20,5 @@ i2c0: i2c at 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 e2e9a2b..99bd037 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -587,6 +587,7 @@
 				reg = <0x80058000 2000>;
 				interrupts = <111 68>;
 				clock-frequency = <100000>;
+				fsl,i2c-dma-channel = <6>;
 				status = "disabled";
 			};
 
@@ -597,6 +598,7 @@
 				reg = <0x8005a000 2000>;
 				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 877b169..20290a6 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,6 +29,9 @@
 #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"
 
@@ -146,6 +147,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 +168,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 +263,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 +417,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 +428,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 +468,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 +511,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 +529,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 +554,28 @@ 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 = 1;
+	if (of_find_property(node, "fsl,use-pio", NULL)) {
+		i2c->dma_mode = 0;
+		dev_info(dev, "Using PIO 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!\n");
+		i2c->dma_mode = 0;
+	}
+
 	i2c->speed = &mxs_i2c_95kHz_config;
 	ret = of_property_read_u32(node, "clock-frequency", &speed);
 	if (ret)
@@ -388,7 +596,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 +608,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 +622,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 +632,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 +679,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

Re: [PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Wolfram Sang]

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

Hi,

On Mon, Jul 09, 2012 at 06:22:54PM +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>
> Cc: Detlev Zundel <dzu-ynQEQJNshbs@public.gmane.org>
> CC: Dong Aisheng <b29396-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Cc: Linux ARM kernel <linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>
> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> CC: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> CC: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Stefano Babic <sbabic-ynQEQJNshbs@public.gmane.org>
> CC: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Wolfgang Denk <wd-ynQEQJNshbs@public.gmane.org>
> Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-mxs.txt |    5 +
>  arch/arm/boot/dts/imx28.dtsi                      |    2 +
>  drivers/i2c/busses/i2c-mxs.c                      |  267 +++++++++++++++++++--
>  3 files changed, 252 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
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
> index 30ac3a0..45b6307 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
> @@ -6,6 +6,10 @@ 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
> +
> +Optional properties:
> +- fsl,use-pio: Use PIO transfers instead of DMA, useful for debug

Having PIOQUEUE (not PIO) as a fallback is good. I'd rather like to see
this as a module parameter, though. For one reason, this is not a
hardware property or board specific, so not a good device tree property.
Also, it is implicitly deprecated somehow since either we want DMA to
fully work or, even better, somewhen be able to automatically switch
between PIOQUEUE and DMA depending on the i2c_msg size. Deprecated
properties are also troublesome. Third, we don't really need this per
instance, if somebody really has problems with DMA, it will apply to all
i2c busses. Makes sense?

>  
>  Examples:
>  
> @@ -16,4 +20,5 @@ i2c0: i2c@80058000 {
>  	reg = <0x80058000 2000>;
>  	interrupts = <111 68>;
>  	clock-frequency = <100000>;
> +	fsl,i2c-dma-channel = <6>;
>  };

> +	/*
> +	 * The MXS I2C DMA mode is prefered and enabled by default.
> +	 * The PIO mode is still supported, but should be used only

PIOQUEUE

> +	 * for debuging purposes etc.
> +	 */
> +	i2c->dma_mode = 1;
> +	if (of_find_property(node, "fsl,use-pio", NULL)) {
> +		i2c->dma_mode = 0;
> +		dev_info(dev, "Using PIO 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.
> +	 */

@Shawn: Any idea when this is going to happen? And why do we need this?
AFAICT it will be always channel 6/7 on mx28?

> +	ret = of_property_read_u32(node, "fsl,i2c-dma-channel",
> +				   &i2c->dma_channel);
> +	if (ret) {
> +		dev_warn(dev, "Failed to get DMA channel!\n");
> +		i2c->dma_mode = 0;
> +	}
> +

Rest looks good, thanks!

   Wolfram

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

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

[PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Wolfram Sang]

Hi,

On Mon, Jul 09, 2012 at 06:22:54PM +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@denx.de>
> Cc: Detlev Zundel <dzu@denx.de>
> CC: Dong Aisheng <b29396@freescale.com>
> CC: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: Linux ARM kernel <linux-arm-kernel@lists.infradead.org>
> Cc: linux-i2c at vger.kernel.org
> CC: Sascha Hauer <s.hauer@pengutronix.de>
> CC: Shawn Guo <shawn.guo@linaro.org>
> Cc: Stefano Babic <sbabic@denx.de>
> CC: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Wolfram Sang <w.sang@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-mxs.txt |    5 +
>  arch/arm/boot/dts/imx28.dtsi                      |    2 +
>  drivers/i2c/busses/i2c-mxs.c                      |  267 +++++++++++++++++++--
>  3 files changed, 252 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
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
> index 30ac3a0..45b6307 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
> @@ -6,6 +6,10 @@ 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
> +
> +Optional properties:
> +- fsl,use-pio: Use PIO transfers instead of DMA, useful for debug

Having PIOQUEUE (not PIO) as a fallback is good. I'd rather like to see
this as a module parameter, though. For one reason, this is not a
hardware property or board specific, so not a good device tree property.
Also, it is implicitly deprecated somehow since either we want DMA to
fully work or, even better, somewhen be able to automatically switch
between PIOQUEUE and DMA depending on the i2c_msg size. Deprecated
properties are also troublesome. Third, we don't really need this per
instance, if somebody really has problems with DMA, it will apply to all
i2c busses. Makes sense?

>  
>  Examples:
>  
> @@ -16,4 +20,5 @@ i2c0: i2c at 80058000 {
>  	reg = <0x80058000 2000>;
>  	interrupts = <111 68>;
>  	clock-frequency = <100000>;
> +	fsl,i2c-dma-channel = <6>;
>  };

> +	/*
> +	 * The MXS I2C DMA mode is prefered and enabled by default.
> +	 * The PIO mode is still supported, but should be used only

PIOQUEUE

> +	 * for debuging purposes etc.
> +	 */
> +	i2c->dma_mode = 1;
> +	if (of_find_property(node, "fsl,use-pio", NULL)) {
> +		i2c->dma_mode = 0;
> +		dev_info(dev, "Using PIO 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.
> +	 */

@Shawn: Any idea when this is going to happen? And why do we need this?
AFAICT it will be always channel 6/7 on mx28?

> +	ret = of_property_read_u32(node, "fsl,i2c-dma-channel",
> +				   &i2c->dma_channel);
> +	if (ret) {
> +		dev_warn(dev, "Failed to get DMA channel!\n");
> +		i2c->dma_mode = 0;
> +	}
> +

Rest looks good, thanks!

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120713/622a36fd/attachment-0001.sig>

Re: [PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> Hi,
> 
> On Mon, Jul 09, 2012 at 06:22:54PM +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>
> > Cc: Detlev Zundel <dzu-ynQEQJNshbs@public.gmane.org>
> > CC: Dong Aisheng <b29396-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > Cc: Linux ARM kernel <linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>
> > Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > CC: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > CC: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > Cc: Stefano Babic <sbabic-ynQEQJNshbs@public.gmane.org>
> > CC: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > Cc: Wolfgang Denk <wd-ynQEQJNshbs@public.gmane.org>
> > Cc: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > ---
> > 
> >  Documentation/devicetree/bindings/i2c/i2c-mxs.txt |    5 +
> >  arch/arm/boot/dts/imx28.dtsi                      |    2 +
> >  drivers/i2c/busses/i2c-mxs.c                      |  267
> >  +++++++++++++++++++-- 3 files changed, 252 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
> > 
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
> > b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt index
> > 30ac3a0..45b6307 100644
> > --- a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
> > +++ b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
> > 
> > @@ -6,6 +6,10 @@ 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
> > +
> > +Optional properties:
> > +- fsl,use-pio: Use PIO transfers instead of DMA, useful for debug
> 
> Having PIOQUEUE (not PIO) as a fallback is good. I'd rather like to see
> this as a module parameter, though.

It'd be cool if someone complained earlier and the responses to new patches 
would be faster. This series has been going on for more than two months and 
noone complained about this part until now. Instead I was made to document this 
and now I have to do it in completely different way? Decide already ...

> For one reason, this is not a
> hardware property or board specific, so not a good device tree property.

Actually, there're still people who might benefit of doing PIOq only, since they 
do small (eg. one byte) transfers. And this is good to have configurable per 
bus.

> Also, it is implicitly deprecated somehow since either we want DMA to
> fully work

And the DMA doesn't fully work?

> or, even better, somewhen be able to automatically switch
> between PIOQUEUE and DMA depending on the i2c_msg size.

That'd be cool. Some months ago, you promised to take a look. I tried it 
recently again with not much luck.

> Deprecated
> properties are also troublesome. Third, we don't really need this per
> instance, if somebody really has problems with DMA, it will apply to all
> i2c busses. Makes sense?

No, it doesn't. See above about small transfers. Consider the easy situation 
where you have sensor on one bus (so you do PIO because you transfer small data) 
and you have EEPROM on other bus, where you use DMA because you transfer large 
data. And the mixed mode isn't there yet.

> >  Examples:
> > @@ -16,4 +20,5 @@ i2c0: i2c@80058000 {
> > 
> >  	reg = <0x80058000 2000>;
> >  	interrupts = <111 68>;
> >  	clock-frequency = <100000>;
> > 
> > +	fsl,i2c-dma-channel = <6>;
> > 
> >  };
> > 
> > +	/*
> > +	 * The MXS I2C DMA mode is prefered and enabled by default.
> > +	 * The PIO mode is still supported, but should be used only
> 
> PIOQUEUE
> 
> > +	 * for debuging purposes etc.
> > +	 */
> > +	i2c->dma_mode = 1;
> > +	if (of_find_property(node, "fsl,use-pio", NULL)) {
> > +		i2c->dma_mode = 0;
> > +		dev_info(dev, "Using PIO 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.
> > +	 */
> 
> @Shawn: Any idea when this is going to happen? And why do we need this?
> AFAICT it will be always channel 6/7 on mx28?
> 
> > +	ret = of_property_read_u32(node, "fsl,i2c-dma-channel",
> > +				   &i2c->dma_channel);
> > +	if (ret) {
> > +		dev_warn(dev, "Failed to get DMA channel!\n");
> > +		i2c->dma_mode = 0;
> > +	}
> > +
> 
> Rest looks good, thanks!
> 
>    Wolfram

Best regards,
Marek Vasut

[PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> Hi,
> 
> On Mon, Jul 09, 2012 at 06:22:54PM +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@denx.de>
> > Cc: Detlev Zundel <dzu@denx.de>
> > CC: Dong Aisheng <b29396@freescale.com>
> > CC: Fabio Estevam <fabio.estevam@freescale.com>
> > Cc: Linux ARM kernel <linux-arm-kernel@lists.infradead.org>
> > Cc: linux-i2c at vger.kernel.org
> > CC: Sascha Hauer <s.hauer@pengutronix.de>
> > CC: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Stefano Babic <sbabic@denx.de>
> > CC: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > Cc: Wolfgang Denk <wd@denx.de>
> > Cc: Wolfram Sang <w.sang@pengutronix.de>
> > ---
> > 
> >  Documentation/devicetree/bindings/i2c/i2c-mxs.txt |    5 +
> >  arch/arm/boot/dts/imx28.dtsi                      |    2 +
> >  drivers/i2c/busses/i2c-mxs.c                      |  267
> >  +++++++++++++++++++-- 3 files changed, 252 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
> > 
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
> > b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt index
> > 30ac3a0..45b6307 100644
> > --- a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
> > +++ b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt
> > 
> > @@ -6,6 +6,10 @@ 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
> > +
> > +Optional properties:
> > +- fsl,use-pio: Use PIO transfers instead of DMA, useful for debug
> 
> Having PIOQUEUE (not PIO) as a fallback is good. I'd rather like to see
> this as a module parameter, though.

It'd be cool if someone complained earlier and the responses to new patches 
would be faster. This series has been going on for more than two months and 
noone complained about this part until now. Instead I was made to document this 
and now I have to do it in completely different way? Decide already ...

> For one reason, this is not a
> hardware property or board specific, so not a good device tree property.

Actually, there're still people who might benefit of doing PIOq only, since they 
do small (eg. one byte) transfers. And this is good to have configurable per 
bus.

> Also, it is implicitly deprecated somehow since either we want DMA to
> fully work

And the DMA doesn't fully work?

> or, even better, somewhen be able to automatically switch
> between PIOQUEUE and DMA depending on the i2c_msg size.

That'd be cool. Some months ago, you promised to take a look. I tried it 
recently again with not much luck.

> Deprecated
> properties are also troublesome. Third, we don't really need this per
> instance, if somebody really has problems with DMA, it will apply to all
> i2c busses. Makes sense?

No, it doesn't. See above about small transfers. Consider the easy situation 
where you have sensor on one bus (so you do PIO because you transfer small data) 
and you have EEPROM on other bus, where you use DMA because you transfer large 
data. And the mixed mode isn't there yet.

> >  Examples:
> > @@ -16,4 +20,5 @@ i2c0: i2c at 80058000 {
> > 
> >  	reg = <0x80058000 2000>;
> >  	interrupts = <111 68>;
> >  	clock-frequency = <100000>;
> > 
> > +	fsl,i2c-dma-channel = <6>;
> > 
> >  };
> > 
> > +	/*
> > +	 * The MXS I2C DMA mode is prefered and enabled by default.
> > +	 * The PIO mode is still supported, but should be used only
> 
> PIOQUEUE
> 
> > +	 * for debuging purposes etc.
> > +	 */
> > +	i2c->dma_mode = 1;
> > +	if (of_find_property(node, "fsl,use-pio", NULL)) {
> > +		i2c->dma_mode = 0;
> > +		dev_info(dev, "Using PIO 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.
> > +	 */
> 
> @Shawn: Any idea when this is going to happen? And why do we need this?
> AFAICT it will be always channel 6/7 on mx28?
> 
> > +	ret = of_property_read_u32(node, "fsl,i2c-dma-channel",
> > +				   &i2c->dma_channel);
> > +	if (ret) {
> > +		dev_warn(dev, "Failed to get DMA channel!\n");
> > +		i2c->dma_mode = 0;
> > +	}
> > +
> 
> Rest looks good, thanks!
> 
>    Wolfram

Best regards,
Marek Vasut

Re: [PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Wolfram Sang]

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

Marek,

> It'd be cool if someone complained earlier and the responses to new patches 
> would be faster. This series has been going on for more than two months and 

I agree it would be cool to response faster. Maintaining being mostly my
priavte fun, I can't make guarantees, though. It just happened that I
didn't have much time for Linux in the last weeks. Actually, I am quite
happy that I managed to work through most of the backlog for the next
merge-window at all (in my holidays!). That's from my side, I can't tell
why other people didn't review; all being too busy is the most likely
guess.

> noone complained about this part until now. Instead I was made to document this 
> and now I have to do it in completely different way? Decide already ...

When we decided to have the fallback mode as a configuration option, the
driver was still considering platform_data. That would have been no
problem...

> 
> > For one reason, this is not a
> > hardware property or board specific, so not a good device tree property.
> 
> Actually, there're still people who might benefit of doing PIOq only, since they 
> do small (eg. one byte) transfers. And this is good to have configurable per 
> bus.

... but we are now on devicetree and things are different there, sadly.
"use-pio" is exposing a driver specific detail which is neither
describing the hardware nor it is OS agnostic. Other drivers from other
OS might not even have the seperation, because they might only use PIO
mode (not even PIOQUEUE). So, the binding is not acceptable AFAIK.
We can add devicetree-discuss to the CC-list.

> > Also, it is implicitly deprecated somehow since either we want DMA to
> > fully work
> 
> And the DMA doesn't fully work?

It probably does. I was just stating the intention.

> > or, even better, somewhen be able to automatically switch
> > between PIOQUEUE and DMA depending on the i2c_msg size.
> 
> That'd be cool. Some months ago, you promised to take a look. I tried it 
> recently again with not much luck.

Yes, I wanted to. I couldn't (see above). I do imagine that it really
might not be possible to switch modes at runtime. This is why I was
trying to get the patch into the next release without the
mode-switching. But for that to happen, there was the binding issue. I
came up with the module parameter as the easiest way to fix it. I am
open to other solutions. Simply dropping the binding might be another
idea if we pay attention that there are no regressions, going from
PIOQUEUE to DMA.

I am also still interested to check the runtime switching, but it might
take another month until I can really hack on it.

> > Deprecated
> > properties are also troublesome. Third, we don't really need this per
> > instance, if somebody really has problems with DMA, it will apply to all
> > i2c busses. Makes sense?
> 
> No, it doesn't. See above about small transfers. Consider the easy situation 
> where you have sensor on one bus (so you do PIO because you transfer small data) 
> and you have EEPROM on other bus, where you use DMA because you transfer large 
> data. And the mixed mode isn't there yet.

I fully understand what you want to configure. I did before. Yet,
devicetree bindings are not platform_data and shouldn't be used like
them.

Regards,

   Wolfram

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

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

[PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Wolfram Sang]

Marek,

> It'd be cool if someone complained earlier and the responses to new patches 
> would be faster. This series has been going on for more than two months and 

I agree it would be cool to response faster. Maintaining being mostly my
priavte fun, I can't make guarantees, though. It just happened that I
didn't have much time for Linux in the last weeks. Actually, I am quite
happy that I managed to work through most of the backlog for the next
merge-window at all (in my holidays!). That's from my side, I can't tell
why other people didn't review; all being too busy is the most likely
guess.

> noone complained about this part until now. Instead I was made to document this 
> and now I have to do it in completely different way? Decide already ...

When we decided to have the fallback mode as a configuration option, the
driver was still considering platform_data. That would have been no
problem...

> 
> > For one reason, this is not a
> > hardware property or board specific, so not a good device tree property.
> 
> Actually, there're still people who might benefit of doing PIOq only, since they 
> do small (eg. one byte) transfers. And this is good to have configurable per 
> bus.

... but we are now on devicetree and things are different there, sadly.
"use-pio" is exposing a driver specific detail which is neither
describing the hardware nor it is OS agnostic. Other drivers from other
OS might not even have the seperation, because they might only use PIO
mode (not even PIOQUEUE). So, the binding is not acceptable AFAIK.
We can add devicetree-discuss to the CC-list.

> > Also, it is implicitly deprecated somehow since either we want DMA to
> > fully work
> 
> And the DMA doesn't fully work?

It probably does. I was just stating the intention.

> > or, even better, somewhen be able to automatically switch
> > between PIOQUEUE and DMA depending on the i2c_msg size.
> 
> That'd be cool. Some months ago, you promised to take a look. I tried it 
> recently again with not much luck.

Yes, I wanted to. I couldn't (see above). I do imagine that it really
might not be possible to switch modes at runtime. This is why I was
trying to get the patch into the next release without the
mode-switching. But for that to happen, there was the binding issue. I
came up with the module parameter as the easiest way to fix it. I am
open to other solutions. Simply dropping the binding might be another
idea if we pay attention that there are no regressions, going from
PIOQUEUE to DMA.

I am also still interested to check the runtime switching, but it might
take another month until I can really hack on it.

> > Deprecated
> > properties are also troublesome. Third, we don't really need this per
> > instance, if somebody really has problems with DMA, it will apply to all
> > i2c busses. Makes sense?
> 
> No, it doesn't. See above about small transfers. Consider the easy situation 
> where you have sensor on one bus (so you do PIO because you transfer small data) 
> and you have EEPROM on other bus, where you use DMA because you transfer large 
> data. And the mixed mode isn't there yet.

I fully understand what you want to configure. I did before. Yet,
devicetree bindings are not platform_data and shouldn't be used like
them.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120714/bcd928c3/attachment.sig>

Re: [PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

[...]

> > That'd be cool. Some months ago, you promised to take a look. I tried it
> > recently again with not much luck.
> 
> Yes, I wanted to. I couldn't (see above). I do imagine that it really
> might not be possible to switch modes at runtime. This is why I was
> trying to get the patch into the next release without the
> mode-switching. But for that to happen, there was the binding issue. I
> came up with the module parameter as the easiest way to fix it. I am
> open to other solutions. Simply dropping the binding might be another
> idea if we pay attention that there are no regressions, going from
> PIOQUEUE to DMA.
> 
> I am also still interested to check the runtime switching, but it might
> take another month until I can really hack on it.

Good, there is some bit that probably needs to be flipped to allow this 
switching. I managed to get this working with SPI, not with i2c though. With 
i2c, if I restarted the controller inbetween each transaction, it worked ... 
which is not what I'd like to see there.

> > > Deprecated
> > > properties are also troublesome. Third, we don't really need this per
> > > instance, if somebody really has problems with DMA, it will apply to
> > > all i2c busses. Makes sense?
> > 
> > No, it doesn't. See above about small transfers. Consider the easy
> > situation where you have sensor on one bus (so you do PIO because you
> > transfer small data) and you have EEPROM on other bus, where you use DMA
> > because you transfer large data. And the mixed mode isn't there yet.
> 
> I fully understand what you want to configure. I did before. Yet,
> devicetree bindings are not platform_data and shouldn't be used like
> them.

But then, how would you configure this detail on a per-bus basis? Well all 
right, screw this, let's make it impotent and go for the module parameter. This 
patch actually fixes a real issue, I'd like to have it in ASAP and it's been 
aboue three months already, which sucks.

> Regards,
> 
>    Wolfram

Best regards,
Marek Vasut

[PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

[...]

> > That'd be cool. Some months ago, you promised to take a look. I tried it
> > recently again with not much luck.
> 
> Yes, I wanted to. I couldn't (see above). I do imagine that it really
> might not be possible to switch modes at runtime. This is why I was
> trying to get the patch into the next release without the
> mode-switching. But for that to happen, there was the binding issue. I
> came up with the module parameter as the easiest way to fix it. I am
> open to other solutions. Simply dropping the binding might be another
> idea if we pay attention that there are no regressions, going from
> PIOQUEUE to DMA.
> 
> I am also still interested to check the runtime switching, but it might
> take another month until I can really hack on it.

Good, there is some bit that probably needs to be flipped to allow this 
switching. I managed to get this working with SPI, not with i2c though. With 
i2c, if I restarted the controller inbetween each transaction, it worked ... 
which is not what I'd like to see there.

> > > Deprecated
> > > properties are also troublesome. Third, we don't really need this per
> > > instance, if somebody really has problems with DMA, it will apply to
> > > all i2c busses. Makes sense?
> > 
> > No, it doesn't. See above about small transfers. Consider the easy
> > situation where you have sensor on one bus (so you do PIO because you
> > transfer small data) and you have EEPROM on other bus, where you use DMA
> > because you transfer large data. And the mixed mode isn't there yet.
> 
> I fully understand what you want to configure. I did before. Yet,
> devicetree bindings are not platform_data and shouldn't be used like
> them.

But then, how would you configure this detail on a per-bus basis? Well all 
right, screw this, let's make it impotent and go for the module parameter. This 
patch actually fixes a real issue, I'd like to have it in ASAP and it's been 
aboue three months already, which sucks.

> Regards,
> 
>    Wolfram

Best regards,
Marek Vasut

Re: [PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Wolfram Sang]

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

Marek,

> > I am also still interested to check the runtime switching, but it might
> > take another month until I can really hack on it.
> 
> Good, there is some bit that probably needs to be flipped to allow this 
> switching. I managed to get this working with SPI, not with i2c though. With 

Ah, hearing that it works with SPI is good news.

> i2c, if I restarted the controller inbetween each transaction, it worked ... 
> which is not what I'd like to see there.

Agreed.

> > > No, it doesn't. See above about small transfers. Consider the easy
> > > situation where you have sensor on one bus (so you do PIO because you
> > > transfer small data) and you have EEPROM on other bus, where you use DMA
> > > because you transfer large data. And the mixed mode isn't there yet.
> > 
> > I fully understand what you want to configure. I did before. Yet,
> > devicetree bindings are not platform_data and shouldn't be used like
> > them.
> 
> But then, how would you configure this detail on a per-bus basis? Well all 

This is a question for devicetree-discuss.

> patch actually fixes a real issue, I'd like to have it in ASAP and it's been 
> aboue three months already, which sucks.

I am open to ideas improving the situation (which is: a lot more
patches, but not a lot more reviewers)

Thanks,

   Wolfram

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

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

[PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Wolfram Sang]

Marek,

> > I am also still interested to check the runtime switching, but it might
> > take another month until I can really hack on it.
> 
> Good, there is some bit that probably needs to be flipped to allow this 
> switching. I managed to get this working with SPI, not with i2c though. With 

Ah, hearing that it works with SPI is good news.

> i2c, if I restarted the controller inbetween each transaction, it worked ... 
> which is not what I'd like to see there.

Agreed.

> > > No, it doesn't. See above about small transfers. Consider the easy
> > > situation where you have sensor on one bus (so you do PIO because you
> > > transfer small data) and you have EEPROM on other bus, where you use DMA
> > > because you transfer large data. And the mixed mode isn't there yet.
> > 
> > I fully understand what you want to configure. I did before. Yet,
> > devicetree bindings are not platform_data and shouldn't be used like
> > them.
> 
> But then, how would you configure this detail on a per-bus basis? Well all 

This is a question for devicetree-discuss.

> patch actually fixes a real issue, I'd like to have it in ASAP and it's been 
> aboue three months already, which sucks.

I am open to ideas improving the situation (which is: a lot more
patches, but not a lot more reviewers)

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120716/ef204b42/attachment.sig>

Re: [PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> Marek,
> 
> > > I am also still interested to check the runtime switching, but it might
> > > take another month until I can really hack on it.
> > 
> > Good, there is some bit that probably needs to be flipped to allow this
> > switching. I managed to get this working with SPI, not with i2c though.
> > With
> 
> Ah, hearing that it works with SPI is good news.
> 
> > i2c, if I restarted the controller inbetween each transaction, it worked
> > ... which is not what I'd like to see there.
> 
> Agreed.
> 
> > > > No, it doesn't. See above about small transfers. Consider the easy
> > > > situation where you have sensor on one bus (so you do PIO because you
> > > > transfer small data) and you have EEPROM on other bus, where you use
> > > > DMA because you transfer large data. And the mixed mode isn't there
> > > > yet.
> > > 
> > > I fully understand what you want to configure. I did before. Yet,
> > > devicetree bindings are not platform_data and shouldn't be used like
> > > them.
> > 
> > But then, how would you configure this detail on a per-bus basis? Well
> > all
> 
> This is a question for devicetree-discuss.

Did you Cc it?

> > patch actually fixes a real issue, I'd like to have it in ASAP and it's
> > been aboue three months already, which sucks.
> 
> I am open to ideas improving the situation (which is: a lot more
> patches, but not a lot more reviewers)
> 
> Thanks,
> 
>    Wolfram

Best regards,
Marek Vasut

[PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> Marek,
> 
> > > I am also still interested to check the runtime switching, but it might
> > > take another month until I can really hack on it.
> > 
> > Good, there is some bit that probably needs to be flipped to allow this
> > switching. I managed to get this working with SPI, not with i2c though.
> > With
> 
> Ah, hearing that it works with SPI is good news.
> 
> > i2c, if I restarted the controller inbetween each transaction, it worked
> > ... which is not what I'd like to see there.
> 
> Agreed.
> 
> > > > No, it doesn't. See above about small transfers. Consider the easy
> > > > situation where you have sensor on one bus (so you do PIO because you
> > > > transfer small data) and you have EEPROM on other bus, where you use
> > > > DMA because you transfer large data. And the mixed mode isn't there
> > > > yet.
> > > 
> > > I fully understand what you want to configure. I did before. Yet,
> > > devicetree bindings are not platform_data and shouldn't be used like
> > > them.
> > 
> > But then, how would you configure this detail on a per-bus basis? Well
> > all
> 
> This is a question for devicetree-discuss.

Did you Cc it?

> > patch actually fixes a real issue, I'd like to have it in ASAP and it's
> > been aboue three months already, which sucks.
> 
> I am open to ideas improving the situation (which is: a lot more
> patches, but not a lot more reviewers)
> 
> Thanks,
> 
>    Wolfram

Best regards,
Marek Vasut

Re: [PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Wolfram Sang]

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


> > > > > No, it doesn't. See above about small transfers. Consider the easy
> > > > > situation where you have sensor on one bus (so you do PIO because you
> > > > > transfer small data) and you have EEPROM on other bus, where you use
> > > > > DMA because you transfer large data. And the mixed mode isn't there
> > > > > yet.
> > > > 
> > > > I fully understand what you want to configure. I did before. Yet,
> > > > devicetree bindings are not platform_data and shouldn't be used like
> > > > them.
> > > 
> > > But then, how would you configure this detail on a per-bus basis? Well
> > > all
> > 
> > This is a question for devicetree-discuss.
> 
> Did you Cc it?

Well, I guess you already checked the mail headers and found out
yourself. The question behind the question probably is "Why didn't you
CC it?". The answer to that is that I didn't have much success by adding
it to $RANDOM_THREAD. It might be more efficient to answer starting a
new thread with explicit "[RFC] $THE_ISSUE". Since a conclusion there
won't make the next merge-window anyhow, I haven't done that now. That
put aside, it might be more efficient if someone with a bigger urge
might strive for a solution (yes, probably, but not necessarily you).
For me, it currently is just one task which needs to be scheduled
inbetween others.

Regards,

   Wolfram

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

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

[PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Wolfram Sang]

> > > > > No, it doesn't. See above about small transfers. Consider the easy
> > > > > situation where you have sensor on one bus (so you do PIO because you
> > > > > transfer small data) and you have EEPROM on other bus, where you use
> > > > > DMA because you transfer large data. And the mixed mode isn't there
> > > > > yet.
> > > > 
> > > > I fully understand what you want to configure. I did before. Yet,
> > > > devicetree bindings are not platform_data and shouldn't be used like
> > > > them.
> > > 
> > > But then, how would you configure this detail on a per-bus basis? Well
> > > all
> > 
> > This is a question for devicetree-discuss.
> 
> Did you Cc it?

Well, I guess you already checked the mail headers and found out
yourself. The question behind the question probably is "Why didn't you
CC it?". The answer to that is that I didn't have much success by adding
it to $RANDOM_THREAD. It might be more efficient to answer starting a
new thread with explicit "[RFC] $THE_ISSUE". Since a conclusion there
won't make the next merge-window anyhow, I haven't done that now. That
put aside, it might be more efficient if someone with a bigger urge
might strive for a solution (yes, probably, but not necessarily you).
For me, it currently is just one task which needs to be scheduled
inbetween others.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120716/d049fd78/attachment.sig>

Re: [PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Shawn Guo]

On Fri, Jul 13, 2012 at 10:22:49AM +0200, Wolfram Sang wrote:
> > +	/*
> > +	 * TODO: This is a temporary solution and should be changed
> > +	 * to use generic DMA binding later when the helpers get in.
> > +	 */
> 
> @Shawn: Any idea when this is going to happen? And why do we need this?

See thread [1] for current statues.  I'm not sure when it's going to
happen though.

> AFAICT it will be always channel 6/7 on mx28?
> 
Yes, but it might be a different channel on mx23.  Just like we define
IO region and interrupt number in device tree, dma channel is just
another resource of hardware block that we choose to define in device
tree.

Regards,
Shawn

[1] http://thread.gmane.org/gmane.linux.ports.arm.omap/75828

[PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Shawn Guo]

On Fri, Jul 13, 2012 at 10:22:49AM +0200, Wolfram Sang wrote:
> > +	/*
> > +	 * TODO: This is a temporary solution and should be changed
> > +	 * to use generic DMA binding later when the helpers get in.
> > +	 */
> 
> @Shawn: Any idea when this is going to happen? And why do we need this?

See thread [1] for current statues.  I'm not sure when it's going to
happen though.

> AFAICT it will be always channel 6/7 on mx28?
> 
Yes, but it might be a different channel on mx23.  Just like we define
IO region and interrupt number in device tree, dma channel is just
another resource of hardware block that we choose to define in device
tree.

Regards,
Shawn

[1] http://thread.gmane.org/gmane.linux.ports.arm.omap/75828

Re: [PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Wolfram Sang]

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

On Sun, Jul 15, 2012 at 04:17:16PM +0800, Shawn Guo wrote:
> On Fri, Jul 13, 2012 at 10:22:49AM +0200, Wolfram Sang wrote:
> > > +	/*
> > > +	 * TODO: This is a temporary solution and should be changed
> > > +	 * to use generic DMA binding later when the helpers get in.
> > > +	 */
> > 
> > @Shawn: Any idea when this is going to happen? And why do we need this?
> 
> See thread [1] for current statues.  I'm not sure when it's going to
> happen though.

Phew, [1] is a bit too much too read. I will just assume there are still
issues.

> > AFAICT it will be always channel 6/7 on mx28?
> > 
> Yes, but it might be a different channel on mx23.  Just like we define
> IO region and interrupt number in device tree, dma channel is just
> another resource of hardware block that we choose to define in device
> tree.

What makes me wonder now that I come to think of it (not necessarily a
question for Shawn but to all):

If I have an I2C slave with an interrupt line tied to something, GPIO or
external IRQ from the SoC, it makes perfect sense to define that in the
devicetree.

Yet, if I know the compatible property for the mxs I2C driver, and also
know the CPU type (be it MX23 or MX28), I can deduce from that a lot of
information, including DMA channel. That is fix. Why encode it?

Regards,

   Wolfram

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

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

[PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Wolfram Sang]

On Sun, Jul 15, 2012 at 04:17:16PM +0800, Shawn Guo wrote:
> On Fri, Jul 13, 2012 at 10:22:49AM +0200, Wolfram Sang wrote:
> > > +	/*
> > > +	 * TODO: This is a temporary solution and should be changed
> > > +	 * to use generic DMA binding later when the helpers get in.
> > > +	 */
> > 
> > @Shawn: Any idea when this is going to happen? And why do we need this?
> 
> See thread [1] for current statues.  I'm not sure when it's going to
> happen though.

Phew, [1] is a bit too much too read. I will just assume there are still
issues.

> > AFAICT it will be always channel 6/7 on mx28?
> > 
> Yes, but it might be a different channel on mx23.  Just like we define
> IO region and interrupt number in device tree, dma channel is just
> another resource of hardware block that we choose to define in device
> tree.

What makes me wonder now that I come to think of it (not necessarily a
question for Shawn but to all):

If I have an I2C slave with an interrupt line tied to something, GPIO or
external IRQ from the SoC, it makes perfect sense to define that in the
devicetree.

Yet, if I know the compatible property for the mxs I2C driver, and also
know the CPU type (be it MX23 or MX28), I can deduce from that a lot of
information, including DMA channel. That is fix. Why encode it?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120721/bd4e8ae8/attachment.sig>

Re: [PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> On Sun, Jul 15, 2012 at 04:17:16PM +0800, Shawn Guo wrote:
> > On Fri, Jul 13, 2012 at 10:22:49AM +0200, Wolfram Sang wrote:
> > > > +	/*
> > > > +	 * TODO: This is a temporary solution and should be changed
> > > > +	 * to use generic DMA binding later when the helpers get in.
> > > > +	 */
> > > 
> > > @Shawn: Any idea when this is going to happen? And why do we need this?
> > 
> > See thread [1] for current statues.  I'm not sure when it's going to
> > happen though.
> 
> Phew, [1] is a bit too much too read. I will just assume there are still
> issues.
> 
> > > AFAICT it will be always channel 6/7 on mx28?
> > 
> > Yes, but it might be a different channel on mx23.  Just like we define
> > IO region and interrupt number in device tree, dma channel is just
> > another resource of hardware block that we choose to define in device
> > tree.
> 
> What makes me wonder now that I come to think of it (not necessarily a
> question for Shawn but to all):
> 
> If I have an I2C slave with an interrupt line tied to something, GPIO or
> external IRQ from the SoC, it makes perfect sense to define that in the
> devicetree.
> 
> Yet, if I know the compatible property for the mxs I2C driver, and also
> know the CPU type (be it MX23 or MX28), I can deduce from that a lot of
> information, including DMA channel. That is fix. Why encode it?

You know the compatible and the "fallback compatible". From the later one, you 
can deduce nothing if that happens to kick in.

btw. the PIO discussion on DT discuss is completely ignored. How shall we 
proceed, this driver is stalled for too long.

> Regards,
> 
>    Wolfram

Best regards,
Marek Vasut

[PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> On Sun, Jul 15, 2012 at 04:17:16PM +0800, Shawn Guo wrote:
> > On Fri, Jul 13, 2012 at 10:22:49AM +0200, Wolfram Sang wrote:
> > > > +	/*
> > > > +	 * TODO: This is a temporary solution and should be changed
> > > > +	 * to use generic DMA binding later when the helpers get in.
> > > > +	 */
> > > 
> > > @Shawn: Any idea when this is going to happen? And why do we need this?
> > 
> > See thread [1] for current statues.  I'm not sure when it's going to
> > happen though.
> 
> Phew, [1] is a bit too much too read. I will just assume there are still
> issues.
> 
> > > AFAICT it will be always channel 6/7 on mx28?
> > 
> > Yes, but it might be a different channel on mx23.  Just like we define
> > IO region and interrupt number in device tree, dma channel is just
> > another resource of hardware block that we choose to define in device
> > tree.
> 
> What makes me wonder now that I come to think of it (not necessarily a
> question for Shawn but to all):
> 
> If I have an I2C slave with an interrupt line tied to something, GPIO or
> external IRQ from the SoC, it makes perfect sense to define that in the
> devicetree.
> 
> Yet, if I know the compatible property for the mxs I2C driver, and also
> know the CPU type (be it MX23 or MX28), I can deduce from that a lot of
> information, including DMA channel. That is fix. Why encode it?

You know the compatible and the "fallback compatible". From the later one, you 
can deduce nothing if that happens to kick in.

btw. the PIO discussion on DT discuss is completely ignored. How shall we 
proceed, this driver is stalled for too long.

> Regards,
> 
>    Wolfram

Best regards,
Marek Vasut

Re: [PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Wolfram Sang]

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


> > Yet, if I know the compatible property for the mxs I2C driver, and also
> > know the CPU type (be it MX23 or MX28), I can deduce from that a lot of
> > information, including DMA channel. That is fix. Why encode it?
> 
> You know the compatible and the "fallback compatible". From the later one, you 
> can deduce nothing if that happens to kick in.

Even if the driver was matched because of an MX23-I2C "compatible"
binding, both devicetree and runtime could provide data that it actually
runs on MX28. That shouldn't be a problem.

> btw. the PIO discussion on DT discuss is completely ignored. How shall we 
> proceed, this driver is stalled for too long.

IIRC I mentioned that a discussion about the bindings won't make the
next merge window. That's why I proposed either module_parameter or
dropping the binding entirely as possible inbetween options.

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

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

[PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Wolfram Sang]

> > Yet, if I know the compatible property for the mxs I2C driver, and also
> > know the CPU type (be it MX23 or MX28), I can deduce from that a lot of
> > information, including DMA channel. That is fix. Why encode it?
> 
> You know the compatible and the "fallback compatible". From the later one, you 
> can deduce nothing if that happens to kick in.

Even if the driver was matched because of an MX23-I2C "compatible"
binding, both devicetree and runtime could provide data that it actually
runs on MX28. That shouldn't be a problem.

> btw. the PIO discussion on DT discuss is completely ignored. How shall we 
> proceed, this driver is stalled for too long.

IIRC I mentioned that a discussion about the bindings won't make the
next merge window. That's why I proposed either module_parameter or
dropping the binding entirely as possible inbetween options.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120721/384e3d24/attachment.sig>

Re: [PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> > > Yet, if I know the compatible property for the mxs I2C driver, and also
> > > know the CPU type (be it MX23 or MX28), I can deduce from that a lot of
> > > information, including DMA channel. That is fix. Why encode it?
> > 
> > You know the compatible and the "fallback compatible". From the later
> > one, you can deduce nothing if that happens to kick in.
> 
> Even if the driver was matched because of an MX23-I2C "compatible"
> binding, both devicetree and runtime could provide data that it actually
> runs on MX28. That shouldn't be a problem.

You mean like ... cpu_is_mx28() ? We got rid of that in favor of DT.

> > btw. the PIO discussion on DT discuss is completely ignored. How shall we
> > proceed, this driver is stalled for too long.
> 
> IIRC I mentioned that a discussion about the bindings won't make the
> next merge window.

Yet another merge window, I have to mention. And only because very long pauses 
inbetween reviews and very minor nitpicks. I'm being annoyed by this patch so 
much I'm thinking of giving up on this. I wasted too much of my free time on 
this and the result is as is.

> That's why I proposed either module_parameter

Which I explained is not a way to go.

> or
> dropping the binding entirely as possible inbetween options.

Which is not an option either. And this discussion is only further stalling the 
patch.

We're adding fsl,something properties all over the DT all the time, yet this one 
is of concern?

Best regards,
Marek Vasut

[PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Marek Vasut]

Dear Wolfram Sang,

> > > Yet, if I know the compatible property for the mxs I2C driver, and also
> > > know the CPU type (be it MX23 or MX28), I can deduce from that a lot of
> > > information, including DMA channel. That is fix. Why encode it?
> > 
> > You know the compatible and the "fallback compatible". From the later
> > one, you can deduce nothing if that happens to kick in.
> 
> Even if the driver was matched because of an MX23-I2C "compatible"
> binding, both devicetree and runtime could provide data that it actually
> runs on MX28. That shouldn't be a problem.

You mean like ... cpu_is_mx28() ? We got rid of that in favor of DT.

> > btw. the PIO discussion on DT discuss is completely ignored. How shall we
> > proceed, this driver is stalled for too long.
> 
> IIRC I mentioned that a discussion about the bindings won't make the
> next merge window.

Yet another merge window, I have to mention. And only because very long pauses 
inbetween reviews and very minor nitpicks. I'm being annoyed by this patch so 
much I'm thinking of giving up on this. I wasted too much of my free time on 
this and the result is as is.

> That's why I proposed either module_parameter

Which I explained is not a way to go.

> or
> dropping the binding entirely as possible inbetween options.

Which is not an option either. And this discussion is only further stalling the 
patch.

We're adding fsl,something properties all over the DT all the time, yet this one 
is of concern?

Best regards,
Marek Vasut

Re: [PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Wolfram Sang]

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


> > Even if the driver was matched because of an MX23-I2C "compatible"
> > binding, both devicetree and runtime could provide data that it actually
> > runs on MX28. That shouldn't be a problem.
> 
> You mean like ... cpu_is_mx28() ? We got rid of that in favor of DT.

Might be. But the information is probably somewhere.

> > IIRC I mentioned that a discussion about the bindings won't make the
> > next merge window.
> 
> Yet another merge window, I have to mention. And only because very long pauses 
> inbetween reviews and very minor nitpicks. I'm being annoyed by this patch so 
> much I'm thinking of giving up on this. I wasted too much of my free time on 
> this and the result is as is.

For you it might be a minor nitpick, for me (as a maintainer) it is not.
You have to deal with just one binding, I have to deal with many. And
since they have to be supported forever, this can easily mess up code
and make the subsystem clumsy and whatnot.

> > That's why I proposed either module_parameter
> 
> Which I explained is not a way to go.

That's why I called it inbetween solution so the patch could go in.
It's fine if you don't like it, I prefer dropping the binding as well.

> > or
> > dropping the binding entirely as possible inbetween options.
> 
> Which is not an option either.

It would enable you to add the binding as an out-of-tree patch.

> And this discussion is only further stalling the 
> patch.


> We're adding fsl,something properties all over the DT all the time, yet this one 
> is of concern?

Yes. Adding all these properties is IMO not the right way, and I have
the impression they often came in because of time pressure like this. If
I think it is wrong for the kernel, I have to reject a patch unless I am
convinced otherwise. Which did not happen yet; as you found out
discussions on devicetree-discuss are slow. Might be another indication
that devictree things happen too much at the time currently. This is not
specific to your patch, there are more which need discussion or had to
be reworked.

Regards,

   Wolfram

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

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

[PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Wolfram Sang]

> > Even if the driver was matched because of an MX23-I2C "compatible"
> > binding, both devicetree and runtime could provide data that it actually
> > runs on MX28. That shouldn't be a problem.
> 
> You mean like ... cpu_is_mx28() ? We got rid of that in favor of DT.

Might be. But the information is probably somewhere.

> > IIRC I mentioned that a discussion about the bindings won't make the
> > next merge window.
> 
> Yet another merge window, I have to mention. And only because very long pauses 
> inbetween reviews and very minor nitpicks. I'm being annoyed by this patch so 
> much I'm thinking of giving up on this. I wasted too much of my free time on 
> this and the result is as is.

For you it might be a minor nitpick, for me (as a maintainer) it is not.
You have to deal with just one binding, I have to deal with many. And
since they have to be supported forever, this can easily mess up code
and make the subsystem clumsy and whatnot.

> > That's why I proposed either module_parameter
> 
> Which I explained is not a way to go.

That's why I called it inbetween solution so the patch could go in.
It's fine if you don't like it, I prefer dropping the binding as well.

> > or
> > dropping the binding entirely as possible inbetween options.
> 
> Which is not an option either.

It would enable you to add the binding as an out-of-tree patch.

> And this discussion is only further stalling the 
> patch.


> We're adding fsl,something properties all over the DT all the time, yet this one 
> is of concern?

Yes. Adding all these properties is IMO not the right way, and I have
the impression they often came in because of time pressure like this. If
I think it is wrong for the kernel, I have to reject a patch unless I am
convinced otherwise. Which did not happen yet; as you found out
discussions on devicetree-discuss are slow. Might be another indication
that devictree things happen too much at the time currently. This is not
specific to your patch, there are more which need discussion or had to
be reworked.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120722/ce0a1af1/attachment.sig>

Re: [PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Shawn Guo]

On Sat, Jul 21, 2012 at 02:44:06PM +0200, Wolfram Sang wrote:
> Yet, if I know the compatible property for the mxs I2C driver, and also
> know the CPU type (be it MX23 or MX28), I can deduce from that a lot of
> information, including DMA channel. That is fix. Why encode it?
> 
The driver shouldn't know the CPU type but just the IP type/version.
We can definitely have "fsl,imx23-i2c" and "fsl,imx28-i2c" compatible
strings for iMX23 and iMX28 respectively if the I2C controller on these
two SoC differs on the IP inside aspects.  But we shouldn't do that
for IO region, interrupt number, DMA channel such differences at SoC
integration level.

-- 
Regards,
Shawn

[PATCH 2/2 V3] MXS: Implement DMA support into mxs-i2c

[Shawn Guo]

On Sat, Jul 21, 2012 at 02:44:06PM +0200, Wolfram Sang wrote:
> Yet, if I know the compatible property for the mxs I2C driver, and also
> know the CPU type (be it MX23 or MX28), I can deduce from that a lot of
> information, including DMA channel. That is fix. Why encode it?
> 
The driver shouldn't know the CPU type but just the IP type/version.
We can definitely have "fsl,imx23-i2c" and "fsl,imx28-i2c" compatible
strings for iMX23 and iMX28 respectively if the I2C controller on these
two SoC differs on the IP inside aspects.  But we shouldn't do that
for IO region, interrupt number, DMA channel such differences at SoC
integration level.

-- 
Regards,
Shawn

Re: [PATCH 1/2 V3] MXS: Set I2C timing registers for mxs-i2c

[Wolfram Sang]

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

On Mon, Jul 09, 2012 at 06:22:53PM +0200, Marek Vasut wrote:
> This patch configures the I2C bus timing registers according
> to information passed via DT. Currently, 100kHz and 400kHz
> modes are supported.
> 
> The TIMING2 register value is wrong in the documentation for
> i.MX28! This was found and fixed by:
>   Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 
> Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>

Applied to next, thanks.

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

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

[PATCH 1/2 V3] MXS: Set I2C timing registers for mxs-i2c

[Wolfram Sang]

On Mon, Jul 09, 2012 at 06:22:53PM +0200, Marek Vasut wrote:
> This patch configures the I2C bus timing registers according
> to information passed via DT. Currently, 100kHz and 400kHz
> modes are supported.
> 
> The TIMING2 register value is wrong in the documentation for
> i.MX28! This was found and fixed by:
>   Shawn Guo <shawn.guo@linaro.org>
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

Applied to next, thanks.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120713/87880d65/attachment.sig>