[PATCH 0/5] i2c: sh_mobile: add DMA support

[PATCH 0/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

Here is v1 of the patch series to support DMA with the i2c-sh_mobile core. DMA
works nicely with my tests so far and we save 1 interrupt per transferred byte,
yay! DMA is opt-in, so if setting it up fails, we will fall back to PIO. The
threshold for selecting DMA is still under test, but probably good enough
already. Since the RFC, I learned about dma_request_slave_channel_reason() and
deferred probing with DMA channels, so the major show stopper is gone now and
this should be ready for inclusion IMO. The other open issues (best threshold
value, ensure DMA capable buffers) can later be done incrementally.

Please test, review, comment. The series is based on
renesas-devel-20141030-v3.18-rc2 with Laurent's series "[PATCH v4 0/5] R-Car
Gen2 DMA Controller driver" on top of it. A git tree can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c-shmobile-dma-experimental

Thanks to Geert and Laurent for reviewing the RFC!

   Wolfram

Wolfram Sang (5):
  i2c: sh_mobile: sort includes alphabetically
  i2c: sh_mobile: add DMA support
  i2c: sh_mobile: improve success message
  ARM: shmobile: r8a7790: add DMA nodes for IIC
  ARM: shmobile: r8a7791: add DMA nodes for IIC

 .../devicetree/bindings/i2c/i2c-sh_mobile.txt      |   5 +
 arch/arm/boot/dts/r8a7790.dtsi                     |   8 +
 arch/arm/boot/dts/r8a7791.dtsi                     |   6 +
 drivers/i2c/busses/i2c-sh_mobile.c                 | 219 ++++++++++++++++++---
 4 files changed, 216 insertions(+), 22 deletions(-)

-- 
2.1.1

[PATCH 0/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

Here is v1 of the patch series to support DMA with the i2c-sh_mobile core. DMA
works nicely with my tests so far and we save 1 interrupt per transferred byte,
yay! DMA is opt-in, so if setting it up fails, we will fall back to PIO. The
threshold for selecting DMA is still under test, but probably good enough
already. Since the RFC, I learned about dma_request_slave_channel_reason() and
deferred probing with DMA channels, so the major show stopper is gone now and
this should be ready for inclusion IMO. The other open issues (best threshold
value, ensure DMA capable buffers) can later be done incrementally.

Please test, review, comment. The series is based on
renesas-devel-20141030-v3.18-rc2 with Laurent's series "[PATCH v4 0/5] R-Car
Gen2 DMA Controller driver" on top of it. A git tree can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c-shmobile-dma-experimental

Thanks to Geert and Laurent for reviewing the RFC!

   Wolfram

Wolfram Sang (5):
  i2c: sh_mobile: sort includes alphabetically
  i2c: sh_mobile: add DMA support
  i2c: sh_mobile: improve success message
  ARM: shmobile: r8a7790: add DMA nodes for IIC
  ARM: shmobile: r8a7791: add DMA nodes for IIC

 .../devicetree/bindings/i2c/i2c-sh_mobile.txt      |   5 +
 arch/arm/boot/dts/r8a7790.dtsi                     |   8 +
 arch/arm/boot/dts/r8a7791.dtsi                     |   6 +
 drivers/i2c/busses/i2c-sh_mobile.c                 | 219 ++++++++++++++++++---
 4 files changed, 216 insertions(+), 22 deletions(-)

-- 
2.1.1

[PATCH 1/5] i2c: sh_mobile: sort includes alphabetically

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

Improves readability and reduces chances of duplicates.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index 8b5e79cb4468..84efe09eb898 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -20,20 +20,20 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/i2c/i2c-sh_mobile.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/init.h>
-#include <linux/delay.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
-#include <linux/interrupt.h>
-#include <linux/i2c.h>
-#include <linux/err.h>
 #include <linux/pm_runtime.h>
-#include <linux/clk.h>
-#include <linux/io.h>
 #include <linux/slab.h>
-#include <linux/of_device.h>
-#include <linux/i2c/i2c-sh_mobile.h>
 
 /* Transmit operation:                                                      */
 /*                                                                          */
-- 
2.1.1

[PATCH 1/5] i2c: sh_mobile: sort includes alphabetically

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

Improves readability and reduces chances of duplicates.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index 8b5e79cb4468..84efe09eb898 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -20,20 +20,20 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/i2c/i2c-sh_mobile.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/init.h>
-#include <linux/delay.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
-#include <linux/interrupt.h>
-#include <linux/i2c.h>
-#include <linux/err.h>
 #include <linux/pm_runtime.h>
-#include <linux/clk.h>
-#include <linux/io.h>
 #include <linux/slab.h>
-#include <linux/of_device.h>
-#include <linux/i2c/i2c-sh_mobile.h>
 
 /* Transmit operation:                                                      */
 /*                                                                          */
-- 
2.1.1

[PATCH 2/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

Make it possible to transfer i2c message buffers via DMA.
Start/Stop/Sending_Slave_Adress is still handled using the old state
machine, it is sending the actual data that is done via DMA. This is
least intrusive and allows us to work with the message buffers directly
instead of preparing a custom buffer which involves copying the data
around.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Changes since RFC:

* use deferred probing for DMA channels; some refactoring of DMA
  initialization came with it
* keep subsys_initcall to avoid regressions with older boards
* drop .buf_mapped flag and exploit use of .dma_direction for that
* headers are sorted now
* turned warnings into debug output; users doesn't need to know about
  every PIO fallback
* minor cleanups & fixes, copyright updates


 .../devicetree/bindings/i2c/i2c-sh_mobile.txt      |   5 +
 drivers/i2c/busses/i2c-sh_mobile.c                 | 195 +++++++++++++++++++--
 2 files changed, 190 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-sh_mobile.txt b/Documentation/devicetree/bindings/i2c/i2c-sh_mobile.txt
index d2153ce36fa8..58dcf7d71e9c 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-sh_mobile.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-sh_mobile.txt
@@ -10,6 +10,11 @@ Required properties:
 
 Optional properties:
 - clock-frequency : frequency of bus clock in Hz. Default 100kHz if unset.
+- dmas            : Must contain a list of two references to DMA
+		    specifiers, one for transmission, and one for
+		    reception.
+- dma-names       : Must contain a list of two DMA names, "tx" and "rx".
+
 
 Pinctrl properties might be needed, too. See there.
 
diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index 84efe09eb898..1e0aa597dd04 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -1,6 +1,8 @@
 /*
  * SuperH Mobile I2C Controller
  *
+ * Copyright (C) 2014 Wolfram Sang <wsa@sang-engineering.com>
+ *
  * Copyright (C) 2008 Magnus Damm
  *
  * Portions of the code based on out-of-tree driver i2c-sh7343.c
@@ -22,6 +24,8 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/i2c/i2c-sh_mobile.h>
@@ -114,6 +118,7 @@ enum sh_mobile_i2c_op {
 	OP_TX_FIRST,
 	OP_TX,
 	OP_TX_STOP,
+	OP_TX_STOP_DATA,
 	OP_TX_TO_RX,
 	OP_RX,
 	OP_RX_STOP,
@@ -138,6 +143,11 @@ struct sh_mobile_i2c_data {
 	int pos;
 	int sr;
 	bool send_stop;
+
+	struct dma_chan *dma_tx;
+	struct dma_chan *dma_rx;
+	struct scatterlist sg;
+	enum dma_data_direction dma_direction;
 };
 
 struct sh_mobile_dt_config {
@@ -175,6 +185,8 @@ struct sh_mobile_dt_config {
 
 #define ICIC_ICCLB8		0x80
 #define ICIC_ICCHB8		0x40
+#define ICIC_TDMAE		0x20
+#define ICIC_RDMAE		0x10
 #define ICIC_ALE		0x08
 #define ICIC_TACKE		0x04
 #define ICIC_WAITE		0x02
@@ -336,8 +348,10 @@ static unsigned char i2c_op(struct sh_mobile_i2c_data *pd,
 	case OP_TX: /* write data */
 		iic_wr(pd, ICDR, data);
 		break;
-	case OP_TX_STOP: /* write data and issue a stop afterwards */
+	case OP_TX_STOP_DATA: /* write data and issue a stop afterwards */
 		iic_wr(pd, ICDR, data);
+		/* fallthrough */
+	case OP_TX_STOP: /* issue a stop */
 		iic_wr(pd, ICCR, pd->send_stop ? ICCR_ICE | ICCR_TRS
 					       : ICCR_ICE | ICCR_TRS | ICCR_BBSY);
 		break;
@@ -393,13 +407,17 @@ static int sh_mobile_i2c_isr_tx(struct sh_mobile_i2c_data *pd)
 {
 	unsigned char data;
 
-	if (pd->pos = pd->msg->len)
+	if (pd->pos = pd->msg->len) {
+		/* Send stop if we haven't yet (DMA case) */
+		if (pd->send_stop && (iic_rd(pd, ICCR) & ICCR_BBSY))
+			i2c_op(pd, OP_TX_STOP, data);
 		return 1;
+	}
 
 	sh_mobile_i2c_get_data(pd, &data);
 
 	if (sh_mobile_i2c_is_last_byte(pd))
-		i2c_op(pd, OP_TX_STOP, data);
+		i2c_op(pd, OP_TX_STOP_DATA, data);
 	else if (sh_mobile_i2c_is_first_byte(pd))
 		i2c_op(pd, OP_TX_FIRST, data);
 	else
@@ -454,7 +472,7 @@ static irqreturn_t sh_mobile_i2c_isr(int irq, void *dev_id)
 	struct platform_device *dev = dev_id;
 	struct sh_mobile_i2c_data *pd = platform_get_drvdata(dev);
 	unsigned char sr;
-	int wakeup;
+	int wakeup = 0;
 
 	sr = iic_rd(pd, ICSR);
 	pd->sr |= sr; /* remember state */
@@ -463,15 +481,21 @@ static irqreturn_t sh_mobile_i2c_isr(int irq, void *dev_id)
 	       (pd->msg->flags & I2C_M_RD) ? "read" : "write",
 	       pd->pos, pd->msg->len);
 
-	if (sr & (ICSR_AL | ICSR_TACK)) {
+	/* Kick off TxDMA after preface was done */
+	if (pd->dma_direction = DMA_TO_DEVICE && pd->pos = 0)
+		iic_set_clr(pd, ICIC, ICIC_TDMAE, 0);
+	else if (sr & (ICSR_AL | ICSR_TACK))
 		/* don't interrupt transaction - continue to issue stop */
 		iic_wr(pd, ICSR, sr & ~(ICSR_AL | ICSR_TACK));
-		wakeup = 0;
-	} else if (pd->msg->flags & I2C_M_RD)
+	else if (pd->msg->flags & I2C_M_RD)
 		wakeup = sh_mobile_i2c_isr_rx(pd);
 	else
 		wakeup = sh_mobile_i2c_isr_tx(pd);
 
+	/* Kick off RxDMA after preface was done */
+	if (pd->dma_direction = DMA_FROM_DEVICE && pd->pos = 1)
+		iic_set_clr(pd, ICIC, ICIC_RDMAE, 0);
+
 	if (sr & ICSR_WAIT) /* TODO: add delay here to support slow acks */
 		iic_wr(pd, ICSR, sr & ~ICSR_WAIT);
 
@@ -486,6 +510,79 @@ static irqreturn_t sh_mobile_i2c_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static void sh_mobile_i2c_cleanup_dma(struct sh_mobile_i2c_data *pd)
+{
+	if (pd->dma_direction = DMA_NONE)
+		return;
+	else if (pd->dma_direction = DMA_FROM_DEVICE)
+		dmaengine_terminate_all(pd->dma_rx);
+	else if (pd->dma_direction = DMA_TO_DEVICE)
+		dmaengine_terminate_all(pd->dma_tx);
+
+	dma_unmap_single(pd->dev, sg_dma_address(&pd->sg),
+			 pd->msg->len, pd->dma_direction);
+
+	pd->dma_direction = DMA_NONE;
+}
+
+static void sh_mobile_i2c_dma_callback(void *data)
+{
+	struct sh_mobile_i2c_data *pd = data;
+
+	dma_unmap_single(pd->dev, sg_dma_address(&pd->sg),
+			 pd->msg->len, pd->dma_direction);
+
+	pd->dma_direction = DMA_NONE;
+	pd->pos = pd->msg->len;
+
+	iic_set_clr(pd, ICIC, 0, ICIC_TDMAE | ICIC_RDMAE);
+}
+
+static void sh_mobile_i2c_xfer_dma(struct sh_mobile_i2c_data *pd)
+{
+	bool read = pd->msg->flags & I2C_M_RD;
+	enum dma_data_direction dir = read ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
+	struct dma_chan *chan = read ? pd->dma_rx : pd->dma_tx;
+	struct dma_async_tx_descriptor *txdesc;
+	dma_addr_t dma_addr;
+	dma_cookie_t cookie;
+
+	if (!chan)
+		return;
+
+	dma_addr = dma_map_single(pd->dev, pd->msg->buf, pd->msg->len, dir);
+	if (dma_mapping_error(pd->dev, dma_addr)) {
+		dev_dbg(pd->dev, "dma map failed, using PIO\n");
+		return;
+	}
+
+	sg_dma_len(&pd->sg) = pd->msg->len;
+	sg_dma_address(&pd->sg) = dma_addr;
+
+	pd->dma_direction = dir;
+
+	txdesc = dmaengine_prep_slave_sg(chan, &pd->sg, 1,
+					 read ? DMA_DEV_TO_MEM : DMA_MEM_TO_DEV,
+					 DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+	if (!txdesc) {
+		dev_dbg(pd->dev, "dma prep slave sg failed, using PIO\n");
+		sh_mobile_i2c_cleanup_dma(pd);
+		return;
+	}
+
+	txdesc->callback = sh_mobile_i2c_dma_callback;
+	txdesc->callback_param = pd;
+
+	cookie = dmaengine_submit(txdesc);
+	if (dma_submit_error(cookie)) {
+		dev_dbg(pd->dev, "submitting dma failed, using PIO\n");
+		sh_mobile_i2c_cleanup_dma(pd);
+		return;
+	}
+
+	dma_async_issue_pending(chan);
+}
+
 static int start_ch(struct sh_mobile_i2c_data *pd, struct i2c_msg *usr_msg,
 		    bool do_init)
 {
@@ -510,6 +607,9 @@ static int start_ch(struct sh_mobile_i2c_data *pd, struct i2c_msg *usr_msg,
 	pd->pos = -1;
 	pd->sr = 0;
 
+	if (pd->msg->len > 8)
+		sh_mobile_i2c_xfer_dma(pd);
+
 	/* Enable all interrupts to begin with */
 	iic_wr(pd, ICIC, ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE);
 	return 0;
@@ -593,6 +693,9 @@ static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter,
 				       5 * HZ);
 		if (!k) {
 			dev_err(pd->dev, "Transfer request timed out\n");
+			if (pd->dma_direction != DMA_NONE)
+				sh_mobile_i2c_cleanup_dma(pd);
+
 			err = -ETIMEDOUT;
 			break;
 		}
@@ -641,6 +744,62 @@ static const struct of_device_id sh_mobile_i2c_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, sh_mobile_i2c_dt_ids);
 
+static int sh_mobile_i2c_request_dma_chan(struct device *dev, enum dma_transfer_direction dir,
+					  dma_addr_t port_addr, struct dma_chan **chan_ptr)
+{
+	dma_cap_mask_t mask;
+	struct dma_chan *chan;
+	struct dma_slave_config cfg;
+	char *chan_name = dir = DMA_MEM_TO_DEV ? "tx" : "rx";
+	int ret;
+
+	dma_cap_zero(mask);
+	dma_cap_set(DMA_SLAVE, mask);
+	*chan_ptr = NULL;
+
+	chan = dma_request_slave_channel_reason(dev, chan_name);
+	if (IS_ERR(chan)) {
+		ret = PTR_ERR(chan);
+		dev_dbg(dev, "request_channel failed for %s (%d)\n", chan_name, ret);
+		return ret;
+	}
+
+	memset(&cfg, 0, sizeof(cfg));
+	cfg.direction = dir;
+	if (dir = DMA_MEM_TO_DEV) {
+		cfg.dst_addr = port_addr;
+		cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	} else {
+		cfg.src_addr = port_addr;
+		cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	}
+
+	ret = dmaengine_slave_config(chan, &cfg);
+	if (ret) {
+		dev_dbg(dev, "slave_config failed for %s (%d)\n", chan_name, ret);
+		dma_release_channel(chan);
+		return ret;
+	}
+
+	*chan_ptr = chan;
+
+	dev_dbg(dev, "got DMA channel for %s\n", chan_name);
+	return 0;
+}
+
+static void sh_mobile_i2c_release_dma(struct sh_mobile_i2c_data *pd)
+{
+	if (pd->dma_tx) {
+		dma_release_channel(pd->dma_tx);
+		pd->dma_tx = NULL;
+	}
+
+	if (pd->dma_rx) {
+		dma_release_channel(pd->dma_rx);
+		pd->dma_rx = NULL;
+	}
+}
+
 static int sh_mobile_i2c_hook_irqs(struct platform_device *dev)
 {
 	struct resource *res;
@@ -727,6 +886,21 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
 	if (ret)
 		return ret;
 
+	/* Init DMA */
+	sg_init_table(&pd->sg, 1);
+	pd->dma_direction = DMA_NONE;
+	ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
+					     res->start + ICDR, &pd->dma_rx);
+	if (ret = -EPROBE_DEFER)
+		return ret;
+
+	ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
+					     res->start + ICDR, &pd->dma_tx);
+	if (ret = -EPROBE_DEFER) {
+		sh_mobile_i2c_release_dma(pd);
+		return ret;
+	}
+
 	/* Enable Runtime PM for this device.
 	 *
 	 * Also tell the Runtime PM core to ignore children
@@ -758,6 +932,7 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
 
 	ret = i2c_add_numbered_adapter(adap);
 	if (ret < 0) {
+		sh_mobile_i2c_release_dma(pd);
 		dev_err(&dev->dev, "cannot add numbered adapter\n");
 		return ret;
 	}
@@ -774,6 +949,7 @@ static int sh_mobile_i2c_remove(struct platform_device *dev)
 	struct sh_mobile_i2c_data *pd = platform_get_drvdata(dev);
 
 	i2c_del_adapter(&pd->adap);
+	sh_mobile_i2c_release_dma(pd);
 	pm_runtime_disable(&dev->dev);
 	return 0;
 }
@@ -810,16 +986,15 @@ static int __init sh_mobile_i2c_adap_init(void)
 {
 	return platform_driver_register(&sh_mobile_i2c_driver);
 }
+subsys_initcall(sh_mobile_i2c_adap_init);
 
 static void __exit sh_mobile_i2c_adap_exit(void)
 {
 	platform_driver_unregister(&sh_mobile_i2c_driver);
 }
-
-subsys_initcall(sh_mobile_i2c_adap_init);
 module_exit(sh_mobile_i2c_adap_exit);
 
 MODULE_DESCRIPTION("SuperH Mobile I2C Bus Controller driver");
-MODULE_AUTHOR("Magnus Damm");
+MODULE_AUTHOR("Magnus Damm and Wolfram Sang");
 MODULE_LICENSE("GPL v2");
 MODULE_ALIAS("platform:i2c-sh_mobile");
-- 
2.1.1

[PATCH 2/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

Make it possible to transfer i2c message buffers via DMA.
Start/Stop/Sending_Slave_Adress is still handled using the old state
machine, it is sending the actual data that is done via DMA. This is
least intrusive and allows us to work with the message buffers directly
instead of preparing a custom buffer which involves copying the data
around.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Changes since RFC:

* use deferred probing for DMA channels; some refactoring of DMA
  initialization came with it
* keep subsys_initcall to avoid regressions with older boards
* drop .buf_mapped flag and exploit use of .dma_direction for that
* headers are sorted now
* turned warnings into debug output; users doesn't need to know about
  every PIO fallback
* minor cleanups & fixes, copyright updates


 .../devicetree/bindings/i2c/i2c-sh_mobile.txt      |   5 +
 drivers/i2c/busses/i2c-sh_mobile.c                 | 195 +++++++++++++++++++--
 2 files changed, 190 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-sh_mobile.txt b/Documentation/devicetree/bindings/i2c/i2c-sh_mobile.txt
index d2153ce36fa8..58dcf7d71e9c 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-sh_mobile.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-sh_mobile.txt
@@ -10,6 +10,11 @@ Required properties:
 
 Optional properties:
 - clock-frequency : frequency of bus clock in Hz. Default 100kHz if unset.
+- dmas            : Must contain a list of two references to DMA
+		    specifiers, one for transmission, and one for
+		    reception.
+- dma-names       : Must contain a list of two DMA names, "tx" and "rx".
+
 
 Pinctrl properties might be needed, too. See there.
 
diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index 84efe09eb898..1e0aa597dd04 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -1,6 +1,8 @@
 /*
  * SuperH Mobile I2C Controller
  *
+ * Copyright (C) 2014 Wolfram Sang <wsa@sang-engineering.com>
+ *
  * Copyright (C) 2008 Magnus Damm
  *
  * Portions of the code based on out-of-tree driver i2c-sh7343.c
@@ -22,6 +24,8 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/i2c/i2c-sh_mobile.h>
@@ -114,6 +118,7 @@ enum sh_mobile_i2c_op {
 	OP_TX_FIRST,
 	OP_TX,
 	OP_TX_STOP,
+	OP_TX_STOP_DATA,
 	OP_TX_TO_RX,
 	OP_RX,
 	OP_RX_STOP,
@@ -138,6 +143,11 @@ struct sh_mobile_i2c_data {
 	int pos;
 	int sr;
 	bool send_stop;
+
+	struct dma_chan *dma_tx;
+	struct dma_chan *dma_rx;
+	struct scatterlist sg;
+	enum dma_data_direction dma_direction;
 };
 
 struct sh_mobile_dt_config {
@@ -175,6 +185,8 @@ struct sh_mobile_dt_config {
 
 #define ICIC_ICCLB8		0x80
 #define ICIC_ICCHB8		0x40
+#define ICIC_TDMAE		0x20
+#define ICIC_RDMAE		0x10
 #define ICIC_ALE		0x08
 #define ICIC_TACKE		0x04
 #define ICIC_WAITE		0x02
@@ -336,8 +348,10 @@ static unsigned char i2c_op(struct sh_mobile_i2c_data *pd,
 	case OP_TX: /* write data */
 		iic_wr(pd, ICDR, data);
 		break;
-	case OP_TX_STOP: /* write data and issue a stop afterwards */
+	case OP_TX_STOP_DATA: /* write data and issue a stop afterwards */
 		iic_wr(pd, ICDR, data);
+		/* fallthrough */
+	case OP_TX_STOP: /* issue a stop */
 		iic_wr(pd, ICCR, pd->send_stop ? ICCR_ICE | ICCR_TRS
 					       : ICCR_ICE | ICCR_TRS | ICCR_BBSY);
 		break;
@@ -393,13 +407,17 @@ static int sh_mobile_i2c_isr_tx(struct sh_mobile_i2c_data *pd)
 {
 	unsigned char data;
 
-	if (pd->pos == pd->msg->len)
+	if (pd->pos == pd->msg->len) {
+		/* Send stop if we haven't yet (DMA case) */
+		if (pd->send_stop && (iic_rd(pd, ICCR) & ICCR_BBSY))
+			i2c_op(pd, OP_TX_STOP, data);
 		return 1;
+	}
 
 	sh_mobile_i2c_get_data(pd, &data);
 
 	if (sh_mobile_i2c_is_last_byte(pd))
-		i2c_op(pd, OP_TX_STOP, data);
+		i2c_op(pd, OP_TX_STOP_DATA, data);
 	else if (sh_mobile_i2c_is_first_byte(pd))
 		i2c_op(pd, OP_TX_FIRST, data);
 	else
@@ -454,7 +472,7 @@ static irqreturn_t sh_mobile_i2c_isr(int irq, void *dev_id)
 	struct platform_device *dev = dev_id;
 	struct sh_mobile_i2c_data *pd = platform_get_drvdata(dev);
 	unsigned char sr;
-	int wakeup;
+	int wakeup = 0;
 
 	sr = iic_rd(pd, ICSR);
 	pd->sr |= sr; /* remember state */
@@ -463,15 +481,21 @@ static irqreturn_t sh_mobile_i2c_isr(int irq, void *dev_id)
 	       (pd->msg->flags & I2C_M_RD) ? "read" : "write",
 	       pd->pos, pd->msg->len);
 
-	if (sr & (ICSR_AL | ICSR_TACK)) {
+	/* Kick off TxDMA after preface was done */
+	if (pd->dma_direction == DMA_TO_DEVICE && pd->pos == 0)
+		iic_set_clr(pd, ICIC, ICIC_TDMAE, 0);
+	else if (sr & (ICSR_AL | ICSR_TACK))
 		/* don't interrupt transaction - continue to issue stop */
 		iic_wr(pd, ICSR, sr & ~(ICSR_AL | ICSR_TACK));
-		wakeup = 0;
-	} else if (pd->msg->flags & I2C_M_RD)
+	else if (pd->msg->flags & I2C_M_RD)
 		wakeup = sh_mobile_i2c_isr_rx(pd);
 	else
 		wakeup = sh_mobile_i2c_isr_tx(pd);
 
+	/* Kick off RxDMA after preface was done */
+	if (pd->dma_direction == DMA_FROM_DEVICE && pd->pos == 1)
+		iic_set_clr(pd, ICIC, ICIC_RDMAE, 0);
+
 	if (sr & ICSR_WAIT) /* TODO: add delay here to support slow acks */
 		iic_wr(pd, ICSR, sr & ~ICSR_WAIT);
 
@@ -486,6 +510,79 @@ static irqreturn_t sh_mobile_i2c_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static void sh_mobile_i2c_cleanup_dma(struct sh_mobile_i2c_data *pd)
+{
+	if (pd->dma_direction == DMA_NONE)
+		return;
+	else if (pd->dma_direction == DMA_FROM_DEVICE)
+		dmaengine_terminate_all(pd->dma_rx);
+	else if (pd->dma_direction == DMA_TO_DEVICE)
+		dmaengine_terminate_all(pd->dma_tx);
+
+	dma_unmap_single(pd->dev, sg_dma_address(&pd->sg),
+			 pd->msg->len, pd->dma_direction);
+
+	pd->dma_direction = DMA_NONE;
+}
+
+static void sh_mobile_i2c_dma_callback(void *data)
+{
+	struct sh_mobile_i2c_data *pd = data;
+
+	dma_unmap_single(pd->dev, sg_dma_address(&pd->sg),
+			 pd->msg->len, pd->dma_direction);
+
+	pd->dma_direction = DMA_NONE;
+	pd->pos = pd->msg->len;
+
+	iic_set_clr(pd, ICIC, 0, ICIC_TDMAE | ICIC_RDMAE);
+}
+
+static void sh_mobile_i2c_xfer_dma(struct sh_mobile_i2c_data *pd)
+{
+	bool read = pd->msg->flags & I2C_M_RD;
+	enum dma_data_direction dir = read ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
+	struct dma_chan *chan = read ? pd->dma_rx : pd->dma_tx;
+	struct dma_async_tx_descriptor *txdesc;
+	dma_addr_t dma_addr;
+	dma_cookie_t cookie;
+
+	if (!chan)
+		return;
+
+	dma_addr = dma_map_single(pd->dev, pd->msg->buf, pd->msg->len, dir);
+	if (dma_mapping_error(pd->dev, dma_addr)) {
+		dev_dbg(pd->dev, "dma map failed, using PIO\n");
+		return;
+	}
+
+	sg_dma_len(&pd->sg) = pd->msg->len;
+	sg_dma_address(&pd->sg) = dma_addr;
+
+	pd->dma_direction = dir;
+
+	txdesc = dmaengine_prep_slave_sg(chan, &pd->sg, 1,
+					 read ? DMA_DEV_TO_MEM : DMA_MEM_TO_DEV,
+					 DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+	if (!txdesc) {
+		dev_dbg(pd->dev, "dma prep slave sg failed, using PIO\n");
+		sh_mobile_i2c_cleanup_dma(pd);
+		return;
+	}
+
+	txdesc->callback = sh_mobile_i2c_dma_callback;
+	txdesc->callback_param = pd;
+
+	cookie = dmaengine_submit(txdesc);
+	if (dma_submit_error(cookie)) {
+		dev_dbg(pd->dev, "submitting dma failed, using PIO\n");
+		sh_mobile_i2c_cleanup_dma(pd);
+		return;
+	}
+
+	dma_async_issue_pending(chan);
+}
+
 static int start_ch(struct sh_mobile_i2c_data *pd, struct i2c_msg *usr_msg,
 		    bool do_init)
 {
@@ -510,6 +607,9 @@ static int start_ch(struct sh_mobile_i2c_data *pd, struct i2c_msg *usr_msg,
 	pd->pos = -1;
 	pd->sr = 0;
 
+	if (pd->msg->len > 8)
+		sh_mobile_i2c_xfer_dma(pd);
+
 	/* Enable all interrupts to begin with */
 	iic_wr(pd, ICIC, ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE);
 	return 0;
@@ -593,6 +693,9 @@ static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter,
 				       5 * HZ);
 		if (!k) {
 			dev_err(pd->dev, "Transfer request timed out\n");
+			if (pd->dma_direction != DMA_NONE)
+				sh_mobile_i2c_cleanup_dma(pd);
+
 			err = -ETIMEDOUT;
 			break;
 		}
@@ -641,6 +744,62 @@ static const struct of_device_id sh_mobile_i2c_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, sh_mobile_i2c_dt_ids);
 
+static int sh_mobile_i2c_request_dma_chan(struct device *dev, enum dma_transfer_direction dir,
+					  dma_addr_t port_addr, struct dma_chan **chan_ptr)
+{
+	dma_cap_mask_t mask;
+	struct dma_chan *chan;
+	struct dma_slave_config cfg;
+	char *chan_name = dir == DMA_MEM_TO_DEV ? "tx" : "rx";
+	int ret;
+
+	dma_cap_zero(mask);
+	dma_cap_set(DMA_SLAVE, mask);
+	*chan_ptr = NULL;
+
+	chan = dma_request_slave_channel_reason(dev, chan_name);
+	if (IS_ERR(chan)) {
+		ret = PTR_ERR(chan);
+		dev_dbg(dev, "request_channel failed for %s (%d)\n", chan_name, ret);
+		return ret;
+	}
+
+	memset(&cfg, 0, sizeof(cfg));
+	cfg.direction = dir;
+	if (dir == DMA_MEM_TO_DEV) {
+		cfg.dst_addr = port_addr;
+		cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	} else {
+		cfg.src_addr = port_addr;
+		cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	}
+
+	ret = dmaengine_slave_config(chan, &cfg);
+	if (ret) {
+		dev_dbg(dev, "slave_config failed for %s (%d)\n", chan_name, ret);
+		dma_release_channel(chan);
+		return ret;
+	}
+
+	*chan_ptr = chan;
+
+	dev_dbg(dev, "got DMA channel for %s\n", chan_name);
+	return 0;
+}
+
+static void sh_mobile_i2c_release_dma(struct sh_mobile_i2c_data *pd)
+{
+	if (pd->dma_tx) {
+		dma_release_channel(pd->dma_tx);
+		pd->dma_tx = NULL;
+	}
+
+	if (pd->dma_rx) {
+		dma_release_channel(pd->dma_rx);
+		pd->dma_rx = NULL;
+	}
+}
+
 static int sh_mobile_i2c_hook_irqs(struct platform_device *dev)
 {
 	struct resource *res;
@@ -727,6 +886,21 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
 	if (ret)
 		return ret;
 
+	/* Init DMA */
+	sg_init_table(&pd->sg, 1);
+	pd->dma_direction = DMA_NONE;
+	ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
+					     res->start + ICDR, &pd->dma_rx);
+	if (ret == -EPROBE_DEFER)
+		return ret;
+
+	ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
+					     res->start + ICDR, &pd->dma_tx);
+	if (ret == -EPROBE_DEFER) {
+		sh_mobile_i2c_release_dma(pd);
+		return ret;
+	}
+
 	/* Enable Runtime PM for this device.
 	 *
 	 * Also tell the Runtime PM core to ignore children
@@ -758,6 +932,7 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
 
 	ret = i2c_add_numbered_adapter(adap);
 	if (ret < 0) {
+		sh_mobile_i2c_release_dma(pd);
 		dev_err(&dev->dev, "cannot add numbered adapter\n");
 		return ret;
 	}
@@ -774,6 +949,7 @@ static int sh_mobile_i2c_remove(struct platform_device *dev)
 	struct sh_mobile_i2c_data *pd = platform_get_drvdata(dev);
 
 	i2c_del_adapter(&pd->adap);
+	sh_mobile_i2c_release_dma(pd);
 	pm_runtime_disable(&dev->dev);
 	return 0;
 }
@@ -810,16 +986,15 @@ static int __init sh_mobile_i2c_adap_init(void)
 {
 	return platform_driver_register(&sh_mobile_i2c_driver);
 }
+subsys_initcall(sh_mobile_i2c_adap_init);
 
 static void __exit sh_mobile_i2c_adap_exit(void)
 {
 	platform_driver_unregister(&sh_mobile_i2c_driver);
 }
-
-subsys_initcall(sh_mobile_i2c_adap_init);
 module_exit(sh_mobile_i2c_adap_exit);
 
 MODULE_DESCRIPTION("SuperH Mobile I2C Bus Controller driver");
-MODULE_AUTHOR("Magnus Damm");
+MODULE_AUTHOR("Magnus Damm and Wolfram Sang");
 MODULE_LICENSE("GPL v2");
 MODULE_ALIAS("platform:i2c-sh_mobile");
-- 
2.1.1

[PATCH 3/5] i2c: sh_mobile: improve success message

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

No user needs magic hex values, makes this debug output. Add DMA info.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index 1e0aa597dd04..903bb22aa5b4 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -293,6 +293,7 @@ static int sh_mobile_i2c_init(struct sh_mobile_i2c_data *pd)
 	else
 		pd->icic &= ~ICIC_ICCHB8;
 
+	dev_dbg(pd->dev, "timing values: L/H=0x%x/0x%x\n", pd->iccl, pd->icch);
 	return 0;
 }
 
@@ -937,9 +938,8 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
 		return ret;
 	}
 
-	dev_info(&dev->dev,
-		 "I2C adapter %d with bus speed %lu Hz (L/H=0x%x/0x%x)\n",
-		 adap->nr, pd->bus_speed, pd->iccl, pd->icch);
+	dev_info(&dev->dev, "I2C adapter %d, bus speed %lu Hz, DMA=%c\n",
+		 adap->nr, pd->bus_speed, (pd->dma_rx || pd->dma_tx) ? 'y' : 'n');
 
 	return 0;
 }
-- 
2.1.1

[PATCH 3/5] i2c: sh_mobile: improve success message

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

No user needs magic hex values, makes this debug output. Add DMA info.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-sh_mobile.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index 1e0aa597dd04..903bb22aa5b4 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -293,6 +293,7 @@ static int sh_mobile_i2c_init(struct sh_mobile_i2c_data *pd)
 	else
 		pd->icic &= ~ICIC_ICCHB8;
 
+	dev_dbg(pd->dev, "timing values: L/H=0x%x/0x%x\n", pd->iccl, pd->icch);
 	return 0;
 }
 
@@ -937,9 +938,8 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
 		return ret;
 	}
 
-	dev_info(&dev->dev,
-		 "I2C adapter %d with bus speed %lu Hz (L/H=0x%x/0x%x)\n",
-		 adap->nr, pd->bus_speed, pd->iccl, pd->icch);
+	dev_info(&dev->dev, "I2C adapter %d, bus speed %lu Hz, DMA=%c\n",
+		 adap->nr, pd->bus_speed, (pd->dma_rx || pd->dma_tx) ? 'y' : 'n');
 
 	return 0;
 }
-- 
2.1.1

[PATCH 4/5] ARM: shmobile: r8a7790: add DMA nodes for IIC

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 arch/arm/boot/dts/r8a7790.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi
index 69b7cd0e7fb3..7e836cc86098 100644
--- a/arch/arm/boot/dts/r8a7790.dtsi
+++ b/arch/arm/boot/dts/r8a7790.dtsi
@@ -359,6 +359,8 @@
 		reg = <0 0xe6500000 0 0x425>;
 		interrupts = <0 174 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&mstp3_clks R8A7790_CLK_IIC0>;
+		dmas = <&dmac0 0x61>, <&dmac0 0x62>;
+		dma-names = "tx", "rx";
 		status = "disabled";
 	};
 
@@ -369,6 +371,8 @@
 		reg = <0 0xe6510000 0 0x425>;
 		interrupts = <0 175 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&mstp3_clks R8A7790_CLK_IIC1>;
+		dmas = <&dmac0 0x65>, <&dmac0 0x66>;
+		dma-names = "tx", "rx";
 		status = "disabled";
 	};
 
@@ -379,6 +383,8 @@
 		reg = <0 0xe6520000 0 0x425>;
 		interrupts = <0 176 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&mstp3_clks R8A7790_CLK_IIC2>;
+		dmas = <&dmac0 0x69>, <&dmac0 0x6a>;
+		dma-names = "tx", "rx";
 		status = "disabled";
 	};
 
@@ -389,6 +395,8 @@
 		reg = <0 0xe60b0000 0 0x425>;
 		interrupts = <0 173 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&mstp9_clks R8A7790_CLK_IICDVFS>;
+		dmas = <&dmac0 0x77>, <&dmac0 0x78>;
+		dma-names = "tx", "rx";
 		status = "disabled";
 	};
 
-- 
2.1.1

[PATCH 4/5] ARM: shmobile: r8a7790: add DMA nodes for IIC

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 arch/arm/boot/dts/r8a7790.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi
index 69b7cd0e7fb3..7e836cc86098 100644
--- a/arch/arm/boot/dts/r8a7790.dtsi
+++ b/arch/arm/boot/dts/r8a7790.dtsi
@@ -359,6 +359,8 @@
 		reg = <0 0xe6500000 0 0x425>;
 		interrupts = <0 174 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&mstp3_clks R8A7790_CLK_IIC0>;
+		dmas = <&dmac0 0x61>, <&dmac0 0x62>;
+		dma-names = "tx", "rx";
 		status = "disabled";
 	};
 
@@ -369,6 +371,8 @@
 		reg = <0 0xe6510000 0 0x425>;
 		interrupts = <0 175 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&mstp3_clks R8A7790_CLK_IIC1>;
+		dmas = <&dmac0 0x65>, <&dmac0 0x66>;
+		dma-names = "tx", "rx";
 		status = "disabled";
 	};
 
@@ -379,6 +383,8 @@
 		reg = <0 0xe6520000 0 0x425>;
 		interrupts = <0 176 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&mstp3_clks R8A7790_CLK_IIC2>;
+		dmas = <&dmac0 0x69>, <&dmac0 0x6a>;
+		dma-names = "tx", "rx";
 		status = "disabled";
 	};
 
@@ -389,6 +395,8 @@
 		reg = <0 0xe60b0000 0 0x425>;
 		interrupts = <0 173 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&mstp9_clks R8A7790_CLK_IICDVFS>;
+		dmas = <&dmac0 0x77>, <&dmac0 0x78>;
+		dma-names = "tx", "rx";
 		status = "disabled";
 	};
 
-- 
2.1.1

[PATCH 5/5] ARM: shmobile: r8a7791: add DMA nodes for IIC

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 arch/arm/boot/dts/r8a7791.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/r8a7791.dtsi b/arch/arm/boot/dts/r8a7791.dtsi
index 9a57215f54f7..213b6bda6201 100644
--- a/arch/arm/boot/dts/r8a7791.dtsi
+++ b/arch/arm/boot/dts/r8a7791.dtsi
@@ -371,6 +371,8 @@
 		reg = <0 0xe60b0000 0 0x425>;
 		interrupts = <0 173 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&mstp9_clks R8A7791_CLK_IICDVFS>;
+		dmas = <&dmac0 0x77>, <&dmac0 0x78>;
+		dma-names = "tx", "rx";
 		status = "disabled";
 	};
 
@@ -381,6 +383,8 @@
 		reg = <0 0xe6500000 0 0x425>;
 		interrupts = <0 174 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&mstp3_clks R8A7791_CLK_IIC0>;
+		dmas = <&dmac0 0x61>, <&dmac0 0x62>;
+		dma-names = "tx", "rx";
 		status = "disabled";
 	};
 
@@ -391,6 +395,8 @@
 		reg = <0 0xe6510000 0 0x425>;
 		interrupts = <0 175 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&mstp3_clks R8A7791_CLK_IIC1>;
+		dmas = <&dmac0 0x65>, <&dmac0 0x66>;
+		dma-names = "tx", "rx";
 		status = "disabled";
 	};
 
-- 
2.1.1

[PATCH 5/5] ARM: shmobile: r8a7791: add DMA nodes for IIC

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 arch/arm/boot/dts/r8a7791.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/r8a7791.dtsi b/arch/arm/boot/dts/r8a7791.dtsi
index 9a57215f54f7..213b6bda6201 100644
--- a/arch/arm/boot/dts/r8a7791.dtsi
+++ b/arch/arm/boot/dts/r8a7791.dtsi
@@ -371,6 +371,8 @@
 		reg = <0 0xe60b0000 0 0x425>;
 		interrupts = <0 173 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&mstp9_clks R8A7791_CLK_IICDVFS>;
+		dmas = <&dmac0 0x77>, <&dmac0 0x78>;
+		dma-names = "tx", "rx";
 		status = "disabled";
 	};
 
@@ -381,6 +383,8 @@
 		reg = <0 0xe6500000 0 0x425>;
 		interrupts = <0 174 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&mstp3_clks R8A7791_CLK_IIC0>;
+		dmas = <&dmac0 0x61>, <&dmac0 0x62>;
+		dma-names = "tx", "rx";
 		status = "disabled";
 	};
 
@@ -391,6 +395,8 @@
 		reg = <0 0xe6510000 0 0x425>;
 		interrupts = <0 175 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&mstp3_clks R8A7791_CLK_IIC1>;
+		dmas = <&dmac0 0x65>, <&dmac0 0x66>;
+		dma-names = "tx", "rx";
 		status = "disabled";
 	};
 
-- 
2.1.1

Re: [PATCH 1/5] i2c: sh_mobile: sort includes alphabetically

[Wolfram Sang]

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

On Fri, Nov 07, 2014 at 11:11:40AM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Improves readability and reduces chances of duplicates.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!


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

Re: [PATCH 1/5] i2c: sh_mobile: sort includes alphabetically

[Wolfram Sang]

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

On Fri, Nov 07, 2014 at 11:11:40AM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Improves readability and reduces chances of duplicates.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!


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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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

On Fri, Nov 07, 2014 at 11:11:41AM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Make it possible to transfer i2c message buffers via DMA.
> Start/Stop/Sending_Slave_Adress is still handled using the old state
> machine, it is sending the actual data that is done via DMA. This is
> least intrusive and allows us to work with the message buffers directly
> instead of preparing a custom buffer which involves copying the data
> around.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---

Applied to for-next, thanks!

One minor change:

-			i2c_op(pd, OP_TX_STOP, data);
+			i2c_op(pd, OP_TX_STOP, 0);

To prevent uninitialzed var usage.

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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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

On Fri, Nov 07, 2014 at 11:11:41AM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Make it possible to transfer i2c message buffers via DMA.
> Start/Stop/Sending_Slave_Adress is still handled using the old state
> machine, it is sending the actual data that is done via DMA. This is
> least intrusive and allows us to work with the message buffers directly
> instead of preparing a custom buffer which involves copying the data
> around.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---

Applied to for-next, thanks!

One minor change:

-			i2c_op(pd, OP_TX_STOP, data);
+			i2c_op(pd, OP_TX_STOP, 0);

To prevent uninitialzed var usage.

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

Re: [PATCH 3/5] i2c: sh_mobile: improve success message

[Wolfram Sang]

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

On Fri, Nov 07, 2014 at 11:11:42AM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> No user needs magic hex values, makes this debug output. Add DMA info.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!

Simon, that means you can queue up patches 4 + 5?


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

Re: [PATCH 3/5] i2c: sh_mobile: improve success message

[Wolfram Sang]

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

On Fri, Nov 07, 2014 at 11:11:42AM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> No user needs magic hex values, makes this debug output. Add DMA info.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!

Simon, that means you can queue up patches 4 + 5?


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

Re: [PATCH 4/5] ARM: shmobile: r8a7790: add DMA nodes for IIC

[Simon Horman]

On Fri, Nov 07, 2014 at 11:11:43AM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks, I have queued this up.

> ---
>  arch/arm/boot/dts/r8a7790.dtsi | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi
> index 69b7cd0e7fb3..7e836cc86098 100644
> --- a/arch/arm/boot/dts/r8a7790.dtsi
> +++ b/arch/arm/boot/dts/r8a7790.dtsi
> @@ -359,6 +359,8 @@
>  		reg = <0 0xe6500000 0 0x425>;
>  		interrupts = <0 174 IRQ_TYPE_LEVEL_HIGH>;
>  		clocks = <&mstp3_clks R8A7790_CLK_IIC0>;
> +		dmas = <&dmac0 0x61>, <&dmac0 0x62>;
> +		dma-names = "tx", "rx";
>  		status = "disabled";
>  	};
>  
> @@ -369,6 +371,8 @@
>  		reg = <0 0xe6510000 0 0x425>;
>  		interrupts = <0 175 IRQ_TYPE_LEVEL_HIGH>;
>  		clocks = <&mstp3_clks R8A7790_CLK_IIC1>;
> +		dmas = <&dmac0 0x65>, <&dmac0 0x66>;
> +		dma-names = "tx", "rx";
>  		status = "disabled";
>  	};
>  
> @@ -379,6 +383,8 @@
>  		reg = <0 0xe6520000 0 0x425>;
>  		interrupts = <0 176 IRQ_TYPE_LEVEL_HIGH>;
>  		clocks = <&mstp3_clks R8A7790_CLK_IIC2>;
> +		dmas = <&dmac0 0x69>, <&dmac0 0x6a>;
> +		dma-names = "tx", "rx";
>  		status = "disabled";
>  	};
>  
> @@ -389,6 +395,8 @@
>  		reg = <0 0xe60b0000 0 0x425>;
>  		interrupts = <0 173 IRQ_TYPE_LEVEL_HIGH>;
>  		clocks = <&mstp9_clks R8A7790_CLK_IICDVFS>;
> +		dmas = <&dmac0 0x77>, <&dmac0 0x78>;
> +		dma-names = "tx", "rx";
>  		status = "disabled";
>  	};
>  
> -- 
> 2.1.1
> 

Re: [PATCH 4/5] ARM: shmobile: r8a7790: add DMA nodes for IIC

[Simon Horman]

On Fri, Nov 07, 2014 at 11:11:43AM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks, I have queued this up.

> ---
>  arch/arm/boot/dts/r8a7790.dtsi | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi
> index 69b7cd0e7fb3..7e836cc86098 100644
> --- a/arch/arm/boot/dts/r8a7790.dtsi
> +++ b/arch/arm/boot/dts/r8a7790.dtsi
> @@ -359,6 +359,8 @@
>  		reg = <0 0xe6500000 0 0x425>;
>  		interrupts = <0 174 IRQ_TYPE_LEVEL_HIGH>;
>  		clocks = <&mstp3_clks R8A7790_CLK_IIC0>;
> +		dmas = <&dmac0 0x61>, <&dmac0 0x62>;
> +		dma-names = "tx", "rx";
>  		status = "disabled";
>  	};
>  
> @@ -369,6 +371,8 @@
>  		reg = <0 0xe6510000 0 0x425>;
>  		interrupts = <0 175 IRQ_TYPE_LEVEL_HIGH>;
>  		clocks = <&mstp3_clks R8A7790_CLK_IIC1>;
> +		dmas = <&dmac0 0x65>, <&dmac0 0x66>;
> +		dma-names = "tx", "rx";
>  		status = "disabled";
>  	};
>  
> @@ -379,6 +383,8 @@
>  		reg = <0 0xe6520000 0 0x425>;
>  		interrupts = <0 176 IRQ_TYPE_LEVEL_HIGH>;
>  		clocks = <&mstp3_clks R8A7790_CLK_IIC2>;
> +		dmas = <&dmac0 0x69>, <&dmac0 0x6a>;
> +		dma-names = "tx", "rx";
>  		status = "disabled";
>  	};
>  
> @@ -389,6 +395,8 @@
>  		reg = <0 0xe60b0000 0 0x425>;
>  		interrupts = <0 173 IRQ_TYPE_LEVEL_HIGH>;
>  		clocks = <&mstp9_clks R8A7790_CLK_IICDVFS>;
> +		dmas = <&dmac0 0x77>, <&dmac0 0x78>;
> +		dma-names = "tx", "rx";
>  		status = "disabled";
>  	};
>  
> -- 
> 2.1.1
> 

Re: [PATCH 5/5] ARM: shmobile: r8a7791: add DMA nodes for IIC

[Simon Horman]

On Fri, Nov 07, 2014 at 11:11:44AM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks, I have queued this up.

> ---
>  arch/arm/boot/dts/r8a7791.dtsi | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/r8a7791.dtsi b/arch/arm/boot/dts/r8a7791.dtsi
> index 9a57215f54f7..213b6bda6201 100644
> --- a/arch/arm/boot/dts/r8a7791.dtsi
> +++ b/arch/arm/boot/dts/r8a7791.dtsi
> @@ -371,6 +371,8 @@
>  		reg = <0 0xe60b0000 0 0x425>;
>  		interrupts = <0 173 IRQ_TYPE_LEVEL_HIGH>;
>  		clocks = <&mstp9_clks R8A7791_CLK_IICDVFS>;
> +		dmas = <&dmac0 0x77>, <&dmac0 0x78>;
> +		dma-names = "tx", "rx";
>  		status = "disabled";
>  	};
>  
> @@ -381,6 +383,8 @@
>  		reg = <0 0xe6500000 0 0x425>;
>  		interrupts = <0 174 IRQ_TYPE_LEVEL_HIGH>;
>  		clocks = <&mstp3_clks R8A7791_CLK_IIC0>;
> +		dmas = <&dmac0 0x61>, <&dmac0 0x62>;
> +		dma-names = "tx", "rx";
>  		status = "disabled";
>  	};
>  
> @@ -391,6 +395,8 @@
>  		reg = <0 0xe6510000 0 0x425>;
>  		interrupts = <0 175 IRQ_TYPE_LEVEL_HIGH>;
>  		clocks = <&mstp3_clks R8A7791_CLK_IIC1>;
> +		dmas = <&dmac0 0x65>, <&dmac0 0x66>;
> +		dma-names = "tx", "rx";
>  		status = "disabled";
>  	};
>  
> -- 
> 2.1.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 5/5] ARM: shmobile: r8a7791: add DMA nodes for IIC

[Simon Horman]

On Fri, Nov 07, 2014 at 11:11:44AM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks, I have queued this up.

> ---
>  arch/arm/boot/dts/r8a7791.dtsi | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/r8a7791.dtsi b/arch/arm/boot/dts/r8a7791.dtsi
> index 9a57215f54f7..213b6bda6201 100644
> --- a/arch/arm/boot/dts/r8a7791.dtsi
> +++ b/arch/arm/boot/dts/r8a7791.dtsi
> @@ -371,6 +371,8 @@
>  		reg = <0 0xe60b0000 0 0x425>;
>  		interrupts = <0 173 IRQ_TYPE_LEVEL_HIGH>;
>  		clocks = <&mstp9_clks R8A7791_CLK_IICDVFS>;
> +		dmas = <&dmac0 0x77>, <&dmac0 0x78>;
> +		dma-names = "tx", "rx";
>  		status = "disabled";
>  	};
>  
> @@ -381,6 +383,8 @@
>  		reg = <0 0xe6500000 0 0x425>;
>  		interrupts = <0 174 IRQ_TYPE_LEVEL_HIGH>;
>  		clocks = <&mstp3_clks R8A7791_CLK_IIC0>;
> +		dmas = <&dmac0 0x61>, <&dmac0 0x62>;
> +		dma-names = "tx", "rx";
>  		status = "disabled";
>  	};
>  
> @@ -391,6 +395,8 @@
>  		reg = <0 0xe6510000 0 0x425>;
>  		interrupts = <0 175 IRQ_TYPE_LEVEL_HIGH>;
>  		clocks = <&mstp3_clks R8A7791_CLK_IIC1>;
> +		dmas = <&dmac0 0x65>, <&dmac0 0x66>;
> +		dma-names = "tx", "rx";
>  		status = "disabled";
>  	};
>  
> -- 
> 2.1.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 3/5] i2c: sh_mobile: improve success message

[Simon Horman]

On Wed, Nov 12, 2014 at 05:12:19PM +0100, Wolfram Sang wrote:
> On Fri, Nov 07, 2014 at 11:11:42AM +0100, Wolfram Sang wrote:
> > From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > 
> > No user needs magic hex values, makes this debug output. Add DMA info.
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Applied to for-next, thanks!
> 
> Simon, that means you can queue up patches 4 + 5?

Thanks, I have now done so.

Re: [PATCH 3/5] i2c: sh_mobile: improve success message

[Simon Horman]

On Wed, Nov 12, 2014 at 05:12:19PM +0100, Wolfram Sang wrote:
> On Fri, Nov 07, 2014 at 11:11:42AM +0100, Wolfram Sang wrote:
> > From: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
> > 
> > No user needs magic hex values, makes this debug output. Add DMA info.
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
> 
> Applied to for-next, thanks!
> 
> Simon, that means you can queue up patches 4 + 5?

Thanks, I have now done so.

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Geert Uytterhoeven]

Hi Wolfram,

On Fri, Nov 7, 2014 at 11:11 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> @@ -727,6 +886,21 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
>         if (ret)
>                 return ret;
>
> +       /* Init DMA */
> +       sg_init_table(&pd->sg, 1);
> +       pd->dma_direction = DMA_NONE;
> +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
> +                                            res->start + ICDR, &pd->dma_rx);
> +       if (ret = -EPROBE_DEFER)
> +               return ret;
> +
> +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
> +                                            res->start + ICDR, &pd->dma_tx);
> +       if (ret = -EPROBE_DEFER) {
> +               sh_mobile_i2c_release_dma(pd);
> +               return ret;
> +       }
> +

If the DTS contains "dma" and "dma-names" properties, but CONFIG_RCAR_DMAC
is disabled, sh_mobile_i2c_request_dma_chan() returns -EPROBE_DEFER,
and the driver fails to initialize.

If I remove the "dma" and "dma-names" properties, the driver does fall back
to PIO mode.

I think this is a regression.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Geert Uytterhoeven]

Hi Wolfram,

On Fri, Nov 7, 2014 at 11:11 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> @@ -727,6 +886,21 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
>         if (ret)
>                 return ret;
>
> +       /* Init DMA */
> +       sg_init_table(&pd->sg, 1);
> +       pd->dma_direction = DMA_NONE;
> +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
> +                                            res->start + ICDR, &pd->dma_rx);
> +       if (ret == -EPROBE_DEFER)
> +               return ret;
> +
> +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
> +                                            res->start + ICDR, &pd->dma_tx);
> +       if (ret == -EPROBE_DEFER) {
> +               sh_mobile_i2c_release_dma(pd);
> +               return ret;
> +       }
> +

If the DTS contains "dma" and "dma-names" properties, but CONFIG_RCAR_DMAC
is disabled, sh_mobile_i2c_request_dma_chan() returns -EPROBE_DEFER,
and the driver fails to initialize.

If I remove the "dma" and "dma-names" properties, the driver does fall back
to PIO mode.

I think this is a regression.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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

On Tue, Dec 09, 2014 at 11:53:45AM +0100, Geert Uytterhoeven wrote:
> Hi Wolfram,
> 
> On Fri, Nov 7, 2014 at 11:11 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> > --- a/drivers/i2c/busses/i2c-sh_mobile.c
> > +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> > @@ -727,6 +886,21 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
> >         if (ret)
> >                 return ret;
> >
> > +       /* Init DMA */
> > +       sg_init_table(&pd->sg, 1);
> > +       pd->dma_direction = DMA_NONE;
> > +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
> > +                                            res->start + ICDR, &pd->dma_rx);
> > +       if (ret == -EPROBE_DEFER)
> > +               return ret;
> > +
> > +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
> > +                                            res->start + ICDR, &pd->dma_tx);
> > +       if (ret == -EPROBE_DEFER) {
> > +               sh_mobile_i2c_release_dma(pd);
> > +               return ret;
> > +       }
> > +
> 
> If the DTS contains "dma" and "dma-names" properties, but CONFIG_RCAR_DMAC
> is disabled, sh_mobile_i2c_request_dma_chan() returns -EPROBE_DEFER,
> and the driver fails to initialize.
> 
> If I remove the "dma" and "dma-names" properties, the driver does fall back
> to PIO mode.
> 
> I think this is a regression.

The only solution I can think of is to not bail out here and retry again
before every transfer? Doesn't sound elegant, though...


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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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

On Tue, Dec 09, 2014 at 11:53:45AM +0100, Geert Uytterhoeven wrote:
> Hi Wolfram,
> 
> On Fri, Nov 7, 2014 at 11:11 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> > --- a/drivers/i2c/busses/i2c-sh_mobile.c
> > +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> > @@ -727,6 +886,21 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
> >         if (ret)
> >                 return ret;
> >
> > +       /* Init DMA */
> > +       sg_init_table(&pd->sg, 1);
> > +       pd->dma_direction = DMA_NONE;
> > +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
> > +                                            res->start + ICDR, &pd->dma_rx);
> > +       if (ret == -EPROBE_DEFER)
> > +               return ret;
> > +
> > +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
> > +                                            res->start + ICDR, &pd->dma_tx);
> > +       if (ret == -EPROBE_DEFER) {
> > +               sh_mobile_i2c_release_dma(pd);
> > +               return ret;
> > +       }
> > +
> 
> If the DTS contains "dma" and "dma-names" properties, but CONFIG_RCAR_DMAC
> is disabled, sh_mobile_i2c_request_dma_chan() returns -EPROBE_DEFER,
> and the driver fails to initialize.
> 
> If I remove the "dma" and "dma-names" properties, the driver does fall back
> to PIO mode.
> 
> I think this is a regression.

The only solution I can think of is to not bail out here and retry again
before every transfer? Doesn't sound elegant, though...


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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Magnus Damm]

Hi Wolfram, Geert,

On Tue, Dec 9, 2014 at 11:09 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> On Tue, Dec 09, 2014 at 11:53:45AM +0100, Geert Uytterhoeven wrote:
>> Hi Wolfram,
>>
>> On Fri, Nov 7, 2014 at 11:11 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> > --- a/drivers/i2c/busses/i2c-sh_mobile.c
>> > +++ b/drivers/i2c/busses/i2c-sh_mobile.c
>> > @@ -727,6 +886,21 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
>> >         if (ret)
>> >                 return ret;
>> >
>> > +       /* Init DMA */
>> > +       sg_init_table(&pd->sg, 1);
>> > +       pd->dma_direction = DMA_NONE;
>> > +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
>> > +                                            res->start + ICDR, &pd->dma_rx);
>> > +       if (ret = -EPROBE_DEFER)
>> > +               return ret;
>> > +
>> > +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
>> > +                                            res->start + ICDR, &pd->dma_tx);
>> > +       if (ret = -EPROBE_DEFER) {
>> > +               sh_mobile_i2c_release_dma(pd);
>> > +               return ret;
>> > +       }
>> > +
>>
>> If the DTS contains "dma" and "dma-names" properties, but CONFIG_RCAR_DMAC
>> is disabled, sh_mobile_i2c_request_dma_chan() returns -EPROBE_DEFER,
>> and the driver fails to initialize.
>>
>> If I remove the "dma" and "dma-names" properties, the driver does fall back
>> to PIO mode.
>>
>> I think this is a regression.
>
> The only solution I can think of is to not bail out here and retry again
> before every transfer? Doesn't sound elegant, though...

I think we have to request for each and every transfer. And fall back
to PIO as default in a transparent way. This because the number of DMA
channels are limited compared to number of potential consumers, so
request failure may happen at any time.

Thanks,

/ magnus

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Magnus Damm]

Hi Wolfram, Geert,

On Tue, Dec 9, 2014 at 11:09 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> On Tue, Dec 09, 2014 at 11:53:45AM +0100, Geert Uytterhoeven wrote:
>> Hi Wolfram,
>>
>> On Fri, Nov 7, 2014 at 11:11 AM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
>> > --- a/drivers/i2c/busses/i2c-sh_mobile.c
>> > +++ b/drivers/i2c/busses/i2c-sh_mobile.c
>> > @@ -727,6 +886,21 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
>> >         if (ret)
>> >                 return ret;
>> >
>> > +       /* Init DMA */
>> > +       sg_init_table(&pd->sg, 1);
>> > +       pd->dma_direction = DMA_NONE;
>> > +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
>> > +                                            res->start + ICDR, &pd->dma_rx);
>> > +       if (ret == -EPROBE_DEFER)
>> > +               return ret;
>> > +
>> > +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
>> > +                                            res->start + ICDR, &pd->dma_tx);
>> > +       if (ret == -EPROBE_DEFER) {
>> > +               sh_mobile_i2c_release_dma(pd);
>> > +               return ret;
>> > +       }
>> > +
>>
>> If the DTS contains "dma" and "dma-names" properties, but CONFIG_RCAR_DMAC
>> is disabled, sh_mobile_i2c_request_dma_chan() returns -EPROBE_DEFER,
>> and the driver fails to initialize.
>>
>> If I remove the "dma" and "dma-names" properties, the driver does fall back
>> to PIO mode.
>>
>> I think this is a regression.
>
> The only solution I can think of is to not bail out here and retry again
> before every transfer? Doesn't sound elegant, though...

I think we have to request for each and every transfer. And fall back
to PIO as default in a transparent way. This because the number of DMA
channels are limited compared to number of potential consumers, so
request failure may happen at any time.

Thanks,

/ magnus

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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

On Wed, Dec 10, 2014 at 02:44:01PM +0900, Magnus Damm wrote:
> Hi Wolfram, Geert,
> 
> On Tue, Dec 9, 2014 at 11:09 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> > On Tue, Dec 09, 2014 at 11:53:45AM +0100, Geert Uytterhoeven wrote:
> >> Hi Wolfram,
> >>
> >> On Fri, Nov 7, 2014 at 11:11 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> >> > --- a/drivers/i2c/busses/i2c-sh_mobile.c
> >> > +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> >> > @@ -727,6 +886,21 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
> >> >         if (ret)
> >> >                 return ret;
> >> >
> >> > +       /* Init DMA */
> >> > +       sg_init_table(&pd->sg, 1);
> >> > +       pd->dma_direction = DMA_NONE;
> >> > +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
> >> > +                                            res->start + ICDR, &pd->dma_rx);
> >> > +       if (ret == -EPROBE_DEFER)
> >> > +               return ret;
> >> > +
> >> > +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
> >> > +                                            res->start + ICDR, &pd->dma_tx);
> >> > +       if (ret == -EPROBE_DEFER) {
> >> > +               sh_mobile_i2c_release_dma(pd);
> >> > +               return ret;
> >> > +       }
> >> > +
> >>
> >> If the DTS contains "dma" and "dma-names" properties, but CONFIG_RCAR_DMAC
> >> is disabled, sh_mobile_i2c_request_dma_chan() returns -EPROBE_DEFER,
> >> and the driver fails to initialize.
> >>
> >> If I remove the "dma" and "dma-names" properties, the driver does fall back
> >> to PIO mode.
> >>
> >> I think this is a regression.
> >
> > The only solution I can think of is to not bail out here and retry again
> > before every transfer? Doesn't sound elegant, though...
> 
> I think we have to request for each and every transfer. And fall back
> to PIO as default in a transparent way. This because the number of DMA
> channels are limited compared to number of potential consumers, so
> request failure may happen at any time.

AFAIR this scenario happens when submitting the transfer. The check
for this is already in place. Requesting the channel is a different
matter. Still, I'll cook up a patch and we will see what it looks
like...


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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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

On Wed, Dec 10, 2014 at 02:44:01PM +0900, Magnus Damm wrote:
> Hi Wolfram, Geert,
> 
> On Tue, Dec 9, 2014 at 11:09 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> > On Tue, Dec 09, 2014 at 11:53:45AM +0100, Geert Uytterhoeven wrote:
> >> Hi Wolfram,
> >>
> >> On Fri, Nov 7, 2014 at 11:11 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> >> > --- a/drivers/i2c/busses/i2c-sh_mobile.c
> >> > +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> >> > @@ -727,6 +886,21 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
> >> >         if (ret)
> >> >                 return ret;
> >> >
> >> > +       /* Init DMA */
> >> > +       sg_init_table(&pd->sg, 1);
> >> > +       pd->dma_direction = DMA_NONE;
> >> > +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
> >> > +                                            res->start + ICDR, &pd->dma_rx);
> >> > +       if (ret == -EPROBE_DEFER)
> >> > +               return ret;
> >> > +
> >> > +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
> >> > +                                            res->start + ICDR, &pd->dma_tx);
> >> > +       if (ret == -EPROBE_DEFER) {
> >> > +               sh_mobile_i2c_release_dma(pd);
> >> > +               return ret;
> >> > +       }
> >> > +
> >>
> >> If the DTS contains "dma" and "dma-names" properties, but CONFIG_RCAR_DMAC
> >> is disabled, sh_mobile_i2c_request_dma_chan() returns -EPROBE_DEFER,
> >> and the driver fails to initialize.
> >>
> >> If I remove the "dma" and "dma-names" properties, the driver does fall back
> >> to PIO mode.
> >>
> >> I think this is a regression.
> >
> > The only solution I can think of is to not bail out here and retry again
> > before every transfer? Doesn't sound elegant, though...
> 
> I think we have to request for each and every transfer. And fall back
> to PIO as default in a transparent way. This because the number of DMA
> channels are limited compared to number of potential consumers, so
> request failure may happen at any time.

AFAIR this scenario happens when submitting the transfer. The check
for this is already in place. Requesting the channel is a different
matter. Still, I'll cook up a patch and we will see what it looks
like...


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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Laurent Pinchart]

(CC'ing the dmaengine mailing list)

On Wednesday 10 December 2014 09:01:55 Wolfram Sang wrote:
> On Wed, Dec 10, 2014 at 02:44:01PM +0900, Magnus Damm wrote:
> > On Tue, Dec 9, 2014 at 11:09 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> >> On Tue, Dec 09, 2014 at 11:53:45AM +0100, Geert Uytterhoeven wrote:
> >>> On Fri, Nov 7, 2014 at 11:11 AM, Wolfram Sang wrote:
> >>>> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> >>>> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> >>>> @@ -727,6 +886,21 @@ static int sh_mobile_i2c_probe(struct
> >>>> platform_device *dev)
> >>>>         if (ret)
> >>>>                 return ret;
> >>>> 
> >>>> +       /* Init DMA */
> >>>> +       sg_init_table(&pd->sg, 1);
> >>>> +       pd->dma_direction = DMA_NONE;
> >>>> +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
> >>>> +                                            res->start + ICDR,
> >>>> &pd->dma_rx);
> >>>> +       if (ret = -EPROBE_DEFER)
> >>>> +               return ret;
> >>>> +
> >>>> +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
> >>>> +                                            res->start + ICDR,
> >>>> &pd->dma_tx);
> >>>> +       if (ret = -EPROBE_DEFER) {
> >>>> +               sh_mobile_i2c_release_dma(pd);
> >>>> +               return ret;
> >>>> +       }
> >>>> +
> >>> 
> >>> If the DTS contains "dma" and "dma-names" properties, but
> >>> CONFIG_RCAR_DMAC is disabled, sh_mobile_i2c_request_dma_chan() returns
> >>> -EPROBE_DEFER, and the driver fails to initialize.
> >>> 
> >>> If I remove the "dma" and "dma-names" properties, the driver does fall
> >>> back to PIO mode.
> >>> 
> >>> I think this is a regression.
> >> 
> >> The only solution I can think of is to not bail out here and retry again
> >> before every transfer? Doesn't sound elegant, though...
> > 
> > I think we have to request for each and every transfer. And fall back
> > to PIO as default in a transparent way. This because the number of DMA
> > channels are limited compared to number of potential consumers, so
> > request failure may happen at any time.
> 
> AFAIR this scenario happens when submitting the transfer. The check
> for this is already in place. Requesting the channel is a different
> matter. Still, I'll cook up a patch and we will see what it looks
> like...

We could fix part of the issue by using virtual dma channels. In that case 
channel requests wouldn't fail anymore due to resource starvation with a large 
number of consumers. However, the request could still fail with -EPROBE_DEFER. 
For a driver that wants to fall back to PIO when DMA is unavailable I 
currently don't see another way than moving the channel request at the time of 
the transfer.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Laurent Pinchart]

(CC'ing the dmaengine mailing list)

On Wednesday 10 December 2014 09:01:55 Wolfram Sang wrote:
> On Wed, Dec 10, 2014 at 02:44:01PM +0900, Magnus Damm wrote:
> > On Tue, Dec 9, 2014 at 11:09 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> >> On Tue, Dec 09, 2014 at 11:53:45AM +0100, Geert Uytterhoeven wrote:
> >>> On Fri, Nov 7, 2014 at 11:11 AM, Wolfram Sang wrote:
> >>>> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> >>>> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> >>>> @@ -727,6 +886,21 @@ static int sh_mobile_i2c_probe(struct
> >>>> platform_device *dev)
> >>>>         if (ret)
> >>>>                 return ret;
> >>>> 
> >>>> +       /* Init DMA */
> >>>> +       sg_init_table(&pd->sg, 1);
> >>>> +       pd->dma_direction = DMA_NONE;
> >>>> +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
> >>>> +                                            res->start + ICDR,
> >>>> &pd->dma_rx);
> >>>> +       if (ret == -EPROBE_DEFER)
> >>>> +               return ret;
> >>>> +
> >>>> +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
> >>>> +                                            res->start + ICDR,
> >>>> &pd->dma_tx);
> >>>> +       if (ret == -EPROBE_DEFER) {
> >>>> +               sh_mobile_i2c_release_dma(pd);
> >>>> +               return ret;
> >>>> +       }
> >>>> +
> >>> 
> >>> If the DTS contains "dma" and "dma-names" properties, but
> >>> CONFIG_RCAR_DMAC is disabled, sh_mobile_i2c_request_dma_chan() returns
> >>> -EPROBE_DEFER, and the driver fails to initialize.
> >>> 
> >>> If I remove the "dma" and "dma-names" properties, the driver does fall
> >>> back to PIO mode.
> >>> 
> >>> I think this is a regression.
> >> 
> >> The only solution I can think of is to not bail out here and retry again
> >> before every transfer? Doesn't sound elegant, though...
> > 
> > I think we have to request for each and every transfer. And fall back
> > to PIO as default in a transparent way. This because the number of DMA
> > channels are limited compared to number of potential consumers, so
> > request failure may happen at any time.
> 
> AFAIR this scenario happens when submitting the transfer. The check
> for this is already in place. Requesting the channel is a different
> matter. Still, I'll cook up a patch and we will see what it looks
> like...

We could fix part of the issue by using virtual dma channels. In that case 
channel requests wouldn't fail anymore due to resource starvation with a large 
number of consumers. However, the request could still fail with -EPROBE_DEFER. 
For a driver that wants to fall back to PIO when DMA is unavailable I 
currently don't see another way than moving the channel request at the time of 
the transfer.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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

On Wed, Dec 10, 2014 at 04:19:36PM +0200, Laurent Pinchart wrote:
> (CC'ing the dmaengine mailing list)

Thanks!

> On Wednesday 10 December 2014 09:01:55 Wolfram Sang wrote:
> > On Wed, Dec 10, 2014 at 02:44:01PM +0900, Magnus Damm wrote:
> > > On Tue, Dec 9, 2014 at 11:09 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> > >> On Tue, Dec 09, 2014 at 11:53:45AM +0100, Geert Uytterhoeven wrote:
> > >>> On Fri, Nov 7, 2014 at 11:11 AM, Wolfram Sang wrote:
> > >>>> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> > >>>> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> > >>>> @@ -727,6 +886,21 @@ static int sh_mobile_i2c_probe(struct
> > >>>> platform_device *dev)
> > >>>>         if (ret)
> > >>>>                 return ret;
> > >>>> 
> > >>>> +       /* Init DMA */
> > >>>> +       sg_init_table(&pd->sg, 1);
> > >>>> +       pd->dma_direction = DMA_NONE;
> > >>>> +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
> > >>>> +                                            res->start + ICDR,
> > >>>> &pd->dma_rx);
> > >>>> +       if (ret == -EPROBE_DEFER)
> > >>>> +               return ret;
> > >>>> +
> > >>>> +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
> > >>>> +                                            res->start + ICDR,
> > >>>> &pd->dma_tx);
> > >>>> +       if (ret == -EPROBE_DEFER) {
> > >>>> +               sh_mobile_i2c_release_dma(pd);
> > >>>> +               return ret;
> > >>>> +       }
> > >>>> +
> > >>> 
> > >>> If the DTS contains "dma" and "dma-names" properties, but
> > >>> CONFIG_RCAR_DMAC is disabled, sh_mobile_i2c_request_dma_chan() returns
> > >>> -EPROBE_DEFER, and the driver fails to initialize.
> > >>> 
> > >>> If I remove the "dma" and "dma-names" properties, the driver does fall
> > >>> back to PIO mode.
> > >>> 
> > >>> I think this is a regression.
> > >> 
> > >> The only solution I can think of is to not bail out here and retry again
> > >> before every transfer? Doesn't sound elegant, though...
> > > 
> > > I think we have to request for each and every transfer. And fall back
> > > to PIO as default in a transparent way. This because the number of DMA
> > > channels are limited compared to number of potential consumers, so
> > > request failure may happen at any time.
> > 
> > AFAIR this scenario happens when submitting the transfer. The check
> > for this is already in place. Requesting the channel is a different
> > matter. Still, I'll cook up a patch and we will see what it looks
> > like...
> 
> We could fix part of the issue by using virtual dma channels. In that case 
> channel requests wouldn't fail anymore due to resource starvation with a large 
> number of consumers. However, the request could still fail with -EPROBE_DEFER. 
> For a driver that wants to fall back to PIO when DMA is unavailable I 
> currently don't see another way than moving the channel request at the time of 
> the transfer.

Note that the I2C drives uses subsys_initcall() for historic reasons,
while the DMA driver uses module_init(). This is hard to revert without
introducing potential regressions on older boards. So, the I2C DMA
support needs to handle deferred probe definately. I am with Laurent, I
don't see any other way, but I'd be glad to be enlightened...


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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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

On Wed, Dec 10, 2014 at 04:19:36PM +0200, Laurent Pinchart wrote:
> (CC'ing the dmaengine mailing list)

Thanks!

> On Wednesday 10 December 2014 09:01:55 Wolfram Sang wrote:
> > On Wed, Dec 10, 2014 at 02:44:01PM +0900, Magnus Damm wrote:
> > > On Tue, Dec 9, 2014 at 11:09 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> > >> On Tue, Dec 09, 2014 at 11:53:45AM +0100, Geert Uytterhoeven wrote:
> > >>> On Fri, Nov 7, 2014 at 11:11 AM, Wolfram Sang wrote:
> > >>>> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> > >>>> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> > >>>> @@ -727,6 +886,21 @@ static int sh_mobile_i2c_probe(struct
> > >>>> platform_device *dev)
> > >>>>         if (ret)
> > >>>>                 return ret;
> > >>>> 
> > >>>> +       /* Init DMA */
> > >>>> +       sg_init_table(&pd->sg, 1);
> > >>>> +       pd->dma_direction = DMA_NONE;
> > >>>> +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
> > >>>> +                                            res->start + ICDR,
> > >>>> &pd->dma_rx);
> > >>>> +       if (ret == -EPROBE_DEFER)
> > >>>> +               return ret;
> > >>>> +
> > >>>> +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
> > >>>> +                                            res->start + ICDR,
> > >>>> &pd->dma_tx);
> > >>>> +       if (ret == -EPROBE_DEFER) {
> > >>>> +               sh_mobile_i2c_release_dma(pd);
> > >>>> +               return ret;
> > >>>> +       }
> > >>>> +
> > >>> 
> > >>> If the DTS contains "dma" and "dma-names" properties, but
> > >>> CONFIG_RCAR_DMAC is disabled, sh_mobile_i2c_request_dma_chan() returns
> > >>> -EPROBE_DEFER, and the driver fails to initialize.
> > >>> 
> > >>> If I remove the "dma" and "dma-names" properties, the driver does fall
> > >>> back to PIO mode.
> > >>> 
> > >>> I think this is a regression.
> > >> 
> > >> The only solution I can think of is to not bail out here and retry again
> > >> before every transfer? Doesn't sound elegant, though...
> > > 
> > > I think we have to request for each and every transfer. And fall back
> > > to PIO as default in a transparent way. This because the number of DMA
> > > channels are limited compared to number of potential consumers, so
> > > request failure may happen at any time.
> > 
> > AFAIR this scenario happens when submitting the transfer. The check
> > for this is already in place. Requesting the channel is a different
> > matter. Still, I'll cook up a patch and we will see what it looks
> > like...
> 
> We could fix part of the issue by using virtual dma channels. In that case 
> channel requests wouldn't fail anymore due to resource starvation with a large 
> number of consumers. However, the request could still fail with -EPROBE_DEFER. 
> For a driver that wants to fall back to PIO when DMA is unavailable I 
> currently don't see another way than moving the channel request at the time of 
> the transfer.

Note that the I2C drives uses subsys_initcall() for historic reasons,
while the DMA driver uses module_init(). This is hard to revert without
introducing potential regressions on older boards. So, the I2C DMA
support needs to handle deferred probe definately. I am with Laurent, I
don't see any other way, but I'd be glad to be enlightened...


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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Vinod Koul]

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

On Wed, Dec 10, 2014 at 03:23:15PM +0100, Wolfram Sang wrote:
> On Wed, Dec 10, 2014 at 04:19:36PM +0200, Laurent Pinchart wrote:
> > (CC'ing the dmaengine mailing list)
> 
> Thanks!
> 
> > On Wednesday 10 December 2014 09:01:55 Wolfram Sang wrote:
> > > On Wed, Dec 10, 2014 at 02:44:01PM +0900, Magnus Damm wrote:
> > > > On Tue, Dec 9, 2014 at 11:09 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> > > >> On Tue, Dec 09, 2014 at 11:53:45AM +0100, Geert Uytterhoeven wrote:
> > > >>> On Fri, Nov 7, 2014 at 11:11 AM, Wolfram Sang wrote:
> > > >>>> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> > > >>>> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> > > >>>> @@ -727,6 +886,21 @@ static int sh_mobile_i2c_probe(struct
> > > >>>> platform_device *dev)
> > > >>>>         if (ret)
> > > >>>>                 return ret;
> > > >>>> 
> > > >>>> +       /* Init DMA */
> > > >>>> +       sg_init_table(&pd->sg, 1);
> > > >>>> +       pd->dma_direction = DMA_NONE;
> > > >>>> +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
> > > >>>> +                                            res->start + ICDR,
> > > >>>> &pd->dma_rx);
> > > >>>> +       if (ret == -EPROBE_DEFER)
> > > >>>> +               return ret;
> > > >>>> +
> > > >>>> +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
> > > >>>> +                                            res->start + ICDR,
> > > >>>> &pd->dma_tx);
> > > >>>> +       if (ret == -EPROBE_DEFER) {
> > > >>>> +               sh_mobile_i2c_release_dma(pd);
> > > >>>> +               return ret;
> > > >>>> +       }
> > > >>>> +
> > > >>> 
> > > >>> If the DTS contains "dma" and "dma-names" properties, but
> > > >>> CONFIG_RCAR_DMAC is disabled, sh_mobile_i2c_request_dma_chan() returns
> > > >>> -EPROBE_DEFER, and the driver fails to initialize.
> > > >>> 
> > > >>> If I remove the "dma" and "dma-names" properties, the driver does fall
> > > >>> back to PIO mode.
> > > >>> 
> > > >>> I think this is a regression.
> > > >> 
> > > >> The only solution I can think of is to not bail out here and retry again
> > > >> before every transfer? Doesn't sound elegant, though...
> > > > 
> > > > I think we have to request for each and every transfer. And fall back
> > > > to PIO as default in a transparent way. This because the number of DMA
> > > > channels are limited compared to number of potential consumers, so
> > > > request failure may happen at any time.
> > > 
> > > AFAIR this scenario happens when submitting the transfer. The check
> > > for this is already in place. Requesting the channel is a different
> > > matter. Still, I'll cook up a patch and we will see what it looks
> > > like...
I think this is a limitation of driver may not be for HW. The right model for
dma_chan is to be viewed as SW channels and not the ones of HW (yes that is
how most of the drivers use that, but we can improve upon)

If we rework the driver to view dma_chan as SW channels, then you can accept
multiple channel requests and accept based on if we are able link the channel to
that peripheral or not. Here one txn maybe for peripheral A, subsequent one
for peripheral B. The driver needs to ensure the proper mux registers etc are
programmed while issuing next descriptor.

-- 
~Vinod

> > 
> > We could fix part of the issue by using virtual dma channels. In that case 
> > channel requests wouldn't fail anymore due to resource starvation with a large 
> > number of consumers. However, the request could still fail with -EPROBE_DEFER. 
> > For a driver that wants to fall back to PIO when DMA is unavailable I 
> > currently don't see another way than moving the channel request at the time of 
> > the transfer.
> 
> Note that the I2C drives uses subsys_initcall() for historic reasons,
> while the DMA driver uses module_init(). This is hard to revert without
> introducing potential regressions on older boards. So, the I2C DMA
> support needs to handle deferred probe definately. I am with Laurent, I
> don't see any other way, but I'd be glad to be enlightened...
> 



-- 

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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Vinod Koul]

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

On Wed, Dec 10, 2014 at 03:23:15PM +0100, Wolfram Sang wrote:
> On Wed, Dec 10, 2014 at 04:19:36PM +0200, Laurent Pinchart wrote:
> > (CC'ing the dmaengine mailing list)
> 
> Thanks!
> 
> > On Wednesday 10 December 2014 09:01:55 Wolfram Sang wrote:
> > > On Wed, Dec 10, 2014 at 02:44:01PM +0900, Magnus Damm wrote:
> > > > On Tue, Dec 9, 2014 at 11:09 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> > > >> On Tue, Dec 09, 2014 at 11:53:45AM +0100, Geert Uytterhoeven wrote:
> > > >>> On Fri, Nov 7, 2014 at 11:11 AM, Wolfram Sang wrote:
> > > >>>> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> > > >>>> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> > > >>>> @@ -727,6 +886,21 @@ static int sh_mobile_i2c_probe(struct
> > > >>>> platform_device *dev)
> > > >>>>         if (ret)
> > > >>>>                 return ret;
> > > >>>> 
> > > >>>> +       /* Init DMA */
> > > >>>> +       sg_init_table(&pd->sg, 1);
> > > >>>> +       pd->dma_direction = DMA_NONE;
> > > >>>> +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
> > > >>>> +                                            res->start + ICDR,
> > > >>>> &pd->dma_rx);
> > > >>>> +       if (ret == -EPROBE_DEFER)
> > > >>>> +               return ret;
> > > >>>> +
> > > >>>> +       ret = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
> > > >>>> +                                            res->start + ICDR,
> > > >>>> &pd->dma_tx);
> > > >>>> +       if (ret == -EPROBE_DEFER) {
> > > >>>> +               sh_mobile_i2c_release_dma(pd);
> > > >>>> +               return ret;
> > > >>>> +       }
> > > >>>> +
> > > >>> 
> > > >>> If the DTS contains "dma" and "dma-names" properties, but
> > > >>> CONFIG_RCAR_DMAC is disabled, sh_mobile_i2c_request_dma_chan() returns
> > > >>> -EPROBE_DEFER, and the driver fails to initialize.
> > > >>> 
> > > >>> If I remove the "dma" and "dma-names" properties, the driver does fall
> > > >>> back to PIO mode.
> > > >>> 
> > > >>> I think this is a regression.
> > > >> 
> > > >> The only solution I can think of is to not bail out here and retry again
> > > >> before every transfer? Doesn't sound elegant, though...
> > > > 
> > > > I think we have to request for each and every transfer. And fall back
> > > > to PIO as default in a transparent way. This because the number of DMA
> > > > channels are limited compared to number of potential consumers, so
> > > > request failure may happen at any time.
> > > 
> > > AFAIR this scenario happens when submitting the transfer. The check
> > > for this is already in place. Requesting the channel is a different
> > > matter. Still, I'll cook up a patch and we will see what it looks
> > > like...
I think this is a limitation of driver may not be for HW. The right model for
dma_chan is to be viewed as SW channels and not the ones of HW (yes that is
how most of the drivers use that, but we can improve upon)

If we rework the driver to view dma_chan as SW channels, then you can accept
multiple channel requests and accept based on if we are able link the channel to
that peripheral or not. Here one txn maybe for peripheral A, subsequent one
for peripheral B. The driver needs to ensure the proper mux registers etc are
programmed while issuing next descriptor.

-- 
~Vinod

> > 
> > We could fix part of the issue by using virtual dma channels. In that case 
> > channel requests wouldn't fail anymore due to resource starvation with a large 
> > number of consumers. However, the request could still fail with -EPROBE_DEFER. 
> > For a driver that wants to fall back to PIO when DMA is unavailable I 
> > currently don't see another way than moving the channel request at the time of 
> > the transfer.
> 
> Note that the I2C drives uses subsys_initcall() for historic reasons,
> while the DMA driver uses module_init(). This is hard to revert without
> introducing potential regressions on older boards. So, the I2C DMA
> support needs to handle deferred probe definately. I am with Laurent, I
> don't see any other way, but I'd be glad to be enlightened...
> 



-- 

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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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


> I think this is a limitation of driver may not be for HW. The right model for
> dma_chan is to be viewed as SW channels and not the ones of HW (yes that is
> how most of the drivers use that, but we can improve upon)
> 
> If we rework the driver to view dma_chan as SW channels, then you can accept
> multiple channel requests and accept based on if we are able link the channel to
> that peripheral or not.

In my understanding, the DMA driver does exactly that. However, it is
not even loaded at the time the I2C driver wants a channel, so the
dmaengine core defers the probe. That is the problem for optional DMA
channels: we can't know when deferring probe won't help anymore and
don't know when it is time to fall back to PIO.


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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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


> I think this is a limitation of driver may not be for HW. The right model for
> dma_chan is to be viewed as SW channels and not the ones of HW (yes that is
> how most of the drivers use that, but we can improve upon)
> 
> If we rework the driver to view dma_chan as SW channels, then you can accept
> multiple channel requests and accept based on if we are able link the channel to
> that peripheral or not.

In my understanding, the DMA driver does exactly that. However, it is
not even loaded at the time the I2C driver wants a channel, so the
dmaengine core defers the probe. That is the problem for optional DMA
channels: we can't know when deferring probe won't help anymore and
don't know when it is time to fall back to PIO.


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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Laurent Pinchart]

On Thursday 11 December 2014 08:37:27 Wolfram Sang wrote:
> > I think this is a limitation of driver may not be for HW. The right model
> > for dma_chan is to be viewed as SW channels and not the ones of HW (yes
> > that is how most of the drivers use that, but we can improve upon)
> > 
> > If we rework the driver to view dma_chan as SW channels, then you can
> > accept multiple channel requests and accept based on if we are able link
> > the channel to that peripheral or not.
> 
> In my understanding, the DMA driver does exactly that.

Actually it doesn't at the moment, I should implement that.

> However, it is not even loaded at the time the I2C driver wants a channel,
> so the dmaengine core defers the probe. That is the problem for optional DMA
> channels: we can't know when deferring probe won't help anymore and don't
> know when it is time to fall back to PIO.

This is true regardless of the whether the driver exposes HW or SW channels.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Laurent Pinchart]

On Thursday 11 December 2014 08:37:27 Wolfram Sang wrote:
> > I think this is a limitation of driver may not be for HW. The right model
> > for dma_chan is to be viewed as SW channels and not the ones of HW (yes
> > that is how most of the drivers use that, but we can improve upon)
> > 
> > If we rework the driver to view dma_chan as SW channels, then you can
> > accept multiple channel requests and accept based on if we are able link
> > the channel to that peripheral or not.
> 
> In my understanding, the DMA driver does exactly that.

Actually it doesn't at the moment, I should implement that.

> However, it is not even loaded at the time the I2C driver wants a channel,
> so the dmaengine core defers the probe. That is the problem for optional DMA
> channels: we can't know when deferring probe won't help anymore and don't
> know when it is time to fall back to PIO.

This is true regardless of the whether the driver exposes HW or SW channels.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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

On Thu, Dec 11, 2014 at 09:47:29AM +0200, Laurent Pinchart wrote:
> On Thursday 11 December 2014 08:37:27 Wolfram Sang wrote:
> > > I think this is a limitation of driver may not be for HW. The right model
> > > for dma_chan is to be viewed as SW channels and not the ones of HW (yes
> > > that is how most of the drivers use that, but we can improve upon)
> > > 
> > > If we rework the driver to view dma_chan as SW channels, then you can
> > > accept multiple channel requests and accept based on if we are able link
> > > the channel to that peripheral or not.
> > 
> > In my understanding, the DMA driver does exactly that.
> 
> Actually it doesn't at the moment, I should implement that.

OK, so I was misinterpreting the overcommitment. Thanks for the correction!

> > However, it is not even loaded at the time the I2C driver wants a channel,
> > so the dmaengine core defers the probe. That is the problem for optional DMA
> > channels: we can't know when deferring probe won't help anymore and don't
> > know when it is time to fall back to PIO.
> 
> This is true regardless of the whether the driver exposes HW or SW channels.

Yup.


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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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

On Thu, Dec 11, 2014 at 09:47:29AM +0200, Laurent Pinchart wrote:
> On Thursday 11 December 2014 08:37:27 Wolfram Sang wrote:
> > > I think this is a limitation of driver may not be for HW. The right model
> > > for dma_chan is to be viewed as SW channels and not the ones of HW (yes
> > > that is how most of the drivers use that, but we can improve upon)
> > > 
> > > If we rework the driver to view dma_chan as SW channels, then you can
> > > accept multiple channel requests and accept based on if we are able link
> > > the channel to that peripheral or not.
> > 
> > In my understanding, the DMA driver does exactly that.
> 
> Actually it doesn't at the moment, I should implement that.

OK, so I was misinterpreting the overcommitment. Thanks for the correction!

> > However, it is not even loaded at the time the I2C driver wants a channel,
> > so the dmaengine core defers the probe. That is the problem for optional DMA
> > channels: we can't know when deferring probe won't help anymore and don't
> > know when it is time to fall back to PIO.
> 
> This is true regardless of the whether the driver exposes HW or SW channels.

Yup.


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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Laurent Pinchart]

Hi Wolfram,

On Wednesday 10 December 2014 15:23:15 Wolfram Sang wrote:
> On Wed, Dec 10, 2014 at 04:19:36PM +0200, Laurent Pinchart wrote:
> > (CC'ing the dmaengine mailing list)
> 
> Thanks!
> 
> > On Wednesday 10 December 2014 09:01:55 Wolfram Sang wrote:
> > > On Wed, Dec 10, 2014 at 02:44:01PM +0900, Magnus Damm wrote:
> > > > On Tue, Dec 9, 2014 at 11:09 PM, Wolfram Sang wrote:
> > > >> On Tue, Dec 09, 2014 at 11:53:45AM +0100, Geert Uytterhoeven wrote:
> > > >>> On Fri, Nov 7, 2014 at 11:11 AM, Wolfram Sang wrote:
> > > >>>> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> > > >>>> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> > > >>>> @@ -727,6 +886,21 @@ static int sh_mobile_i2c_probe(struct
> > > >>>> platform_device *dev)
> > > >>>> 
> > > >>>>         if (ret)
> > > >>>>         
> > > >>>>                 return ret;
> > > >>>> 
> > > >>>> +       /* Init DMA */
> > > >>>> +       sg_init_table(&pd->sg, 1);
> > > >>>> +       pd->dma_direction = DMA_NONE;
> > > >>>> +       ret = sh_mobile_i2c_request_dma_chan(pd->dev,
> > > >>>> DMA_DEV_TO_MEM,
> > > >>>> +                                            res->start + ICDR,
> > > >>>> &pd->dma_rx);
> > > >>>> +       if (ret = -EPROBE_DEFER)
> > > >>>> +               return ret;
> > > >>>> +
> > > >>>> +       ret = sh_mobile_i2c_request_dma_chan(pd->dev,
> > > >>>> DMA_MEM_TO_DEV,
> > > >>>> +                                            res->start + ICDR,
> > > >>>> &pd->dma_tx);
> > > >>>> +       if (ret = -EPROBE_DEFER) {
> > > >>>> +               sh_mobile_i2c_release_dma(pd);
> > > >>>> +               return ret;
> > > >>>> +       }
> > > >>>> +
> > > >>> 
> > > >>> If the DTS contains "dma" and "dma-names" properties, but
> > > >>> CONFIG_RCAR_DMAC is disabled, sh_mobile_i2c_request_dma_chan()
> > > >>> returns -EPROBE_DEFER, and the driver fails to initialize.
> > > >>> 
> > > >>> If I remove the "dma" and "dma-names" properties, the driver does
> > > >>> fall back to PIO mode.
> > > >>> 
> > > >>> I think this is a regression.
> > > >> 
> > > >> The only solution I can think of is to not bail out here and retry
> > > >> again before every transfer? Doesn't sound elegant, though...
> > > > 
> > > > I think we have to request for each and every transfer. And fall back
> > > > to PIO as default in a transparent way. This because the number of DMA
> > > > channels are limited compared to number of potential consumers, so
> > > > request failure may happen at any time.
> > > 
> > > AFAIR this scenario happens when submitting the transfer. The check
> > > for this is already in place. Requesting the channel is a different
> > > matter. Still, I'll cook up a patch and we will see what it looks
> > > like...
> > 
> > We could fix part of the issue by using virtual dma channels. In that case
> > channel requests wouldn't fail anymore due to resource starvation with a
> > large number of consumers. However, the request could still fail with
> > -EPROBE_DEFER. For a driver that wants to fall back to PIO when DMA is
> > unavailable I currently don't see another way than moving the channel
> > request at the time of the transfer.
> 
> Note that the I2C drives uses subsys_initcall() for historic reasons,
> while the DMA driver uses module_init(). This is hard to revert without
> introducing potential regressions on older boards. So, the I2C DMA
> support needs to handle deferred probe definately. I am with Laurent, I
> don't see any other way, but I'd be glad to be enlightened...

While I believe that requesting the channel at transfer time is the good 
solution, I think we should still try to move to module initcalls where 
possible. The risk of regressions is real so proper testing is needed. My 
question is, have you tried it ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Laurent Pinchart]

Hi Wolfram,

On Wednesday 10 December 2014 15:23:15 Wolfram Sang wrote:
> On Wed, Dec 10, 2014 at 04:19:36PM +0200, Laurent Pinchart wrote:
> > (CC'ing the dmaengine mailing list)
> 
> Thanks!
> 
> > On Wednesday 10 December 2014 09:01:55 Wolfram Sang wrote:
> > > On Wed, Dec 10, 2014 at 02:44:01PM +0900, Magnus Damm wrote:
> > > > On Tue, Dec 9, 2014 at 11:09 PM, Wolfram Sang wrote:
> > > >> On Tue, Dec 09, 2014 at 11:53:45AM +0100, Geert Uytterhoeven wrote:
> > > >>> On Fri, Nov 7, 2014 at 11:11 AM, Wolfram Sang wrote:
> > > >>>> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> > > >>>> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> > > >>>> @@ -727,6 +886,21 @@ static int sh_mobile_i2c_probe(struct
> > > >>>> platform_device *dev)
> > > >>>> 
> > > >>>>         if (ret)
> > > >>>>         
> > > >>>>                 return ret;
> > > >>>> 
> > > >>>> +       /* Init DMA */
> > > >>>> +       sg_init_table(&pd->sg, 1);
> > > >>>> +       pd->dma_direction = DMA_NONE;
> > > >>>> +       ret = sh_mobile_i2c_request_dma_chan(pd->dev,
> > > >>>> DMA_DEV_TO_MEM,
> > > >>>> +                                            res->start + ICDR,
> > > >>>> &pd->dma_rx);
> > > >>>> +       if (ret == -EPROBE_DEFER)
> > > >>>> +               return ret;
> > > >>>> +
> > > >>>> +       ret = sh_mobile_i2c_request_dma_chan(pd->dev,
> > > >>>> DMA_MEM_TO_DEV,
> > > >>>> +                                            res->start + ICDR,
> > > >>>> &pd->dma_tx);
> > > >>>> +       if (ret == -EPROBE_DEFER) {
> > > >>>> +               sh_mobile_i2c_release_dma(pd);
> > > >>>> +               return ret;
> > > >>>> +       }
> > > >>>> +
> > > >>> 
> > > >>> If the DTS contains "dma" and "dma-names" properties, but
> > > >>> CONFIG_RCAR_DMAC is disabled, sh_mobile_i2c_request_dma_chan()
> > > >>> returns -EPROBE_DEFER, and the driver fails to initialize.
> > > >>> 
> > > >>> If I remove the "dma" and "dma-names" properties, the driver does
> > > >>> fall back to PIO mode.
> > > >>> 
> > > >>> I think this is a regression.
> > > >> 
> > > >> The only solution I can think of is to not bail out here and retry
> > > >> again before every transfer? Doesn't sound elegant, though...
> > > > 
> > > > I think we have to request for each and every transfer. And fall back
> > > > to PIO as default in a transparent way. This because the number of DMA
> > > > channels are limited compared to number of potential consumers, so
> > > > request failure may happen at any time.
> > > 
> > > AFAIR this scenario happens when submitting the transfer. The check
> > > for this is already in place. Requesting the channel is a different
> > > matter. Still, I'll cook up a patch and we will see what it looks
> > > like...
> > 
> > We could fix part of the issue by using virtual dma channels. In that case
> > channel requests wouldn't fail anymore due to resource starvation with a
> > large number of consumers. However, the request could still fail with
> > -EPROBE_DEFER. For a driver that wants to fall back to PIO when DMA is
> > unavailable I currently don't see another way than moving the channel
> > request at the time of the transfer.
> 
> Note that the I2C drives uses subsys_initcall() for historic reasons,
> while the DMA driver uses module_init(). This is hard to revert without
> introducing potential regressions on older boards. So, the I2C DMA
> support needs to handle deferred probe definately. I am with Laurent, I
> don't see any other way, but I'd be glad to be enlightened...

While I believe that requesting the channel at transfer time is the good 
solution, I think we should still try to move to module initcalls where 
possible. The risk of regressions is real so proper testing is needed. My 
question is, have you tried it ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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


> > Note that the I2C drives uses subsys_initcall() for historic reasons,
> > while the DMA driver uses module_init(). This is hard to revert without
> > introducing potential regressions on older boards. So, the I2C DMA
> > support needs to handle deferred probe definately. I am with Laurent, I
> > don't see any other way, but I'd be glad to be enlightened...
> 
> While I believe that requesting the channel at transfer time is the good 
> solution, I think we should still try to move to module initcalls where 
> possible. The risk of regressions is real so proper testing is needed. My 
> question is, have you tried it ?

I would need to test all boards using this driver to not fail booting.
Usually I2C drivers are moved to subsys_initcall because they need
access to something critical (PMIC, GPIOs...) early. I don't see a sane
way to do that testing.

Other than that, even if we move to module_init, we reduce the chance of
getting a deferred probe, but we do not eliminate it...


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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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


> > Note that the I2C drives uses subsys_initcall() for historic reasons,
> > while the DMA driver uses module_init(). This is hard to revert without
> > introducing potential regressions on older boards. So, the I2C DMA
> > support needs to handle deferred probe definately. I am with Laurent, I
> > don't see any other way, but I'd be glad to be enlightened...
> 
> While I believe that requesting the channel at transfer time is the good 
> solution, I think we should still try to move to module initcalls where 
> possible. The risk of regressions is real so proper testing is needed. My 
> question is, have you tried it ?

I would need to test all boards using this driver to not fail booting.
Usually I2C drivers are moved to subsys_initcall because they need
access to something critical (PMIC, GPIOs...) early. I don't see a sane
way to do that testing.

Other than that, even if we move to module_init, we reduce the chance of
getting a deferred probe, but we do not eliminate it...


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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Laurent Pinchart]

On Thursday 11 December 2014 22:47:32 Wolfram Sang wrote:
> > > Note that the I2C drives uses subsys_initcall() for historic reasons,
> > > while the DMA driver uses module_init(). This is hard to revert without
> > > introducing potential regressions on older boards. So, the I2C DMA
> > > support needs to handle deferred probe definately. I am with Laurent, I
> > > don't see any other way, but I'd be glad to be enlightened...
> > 
> > While I believe that requesting the channel at transfer time is the good
> > solution, I think we should still try to move to module initcalls where
> > possible. The risk of regressions is real so proper testing is needed. My
> > question is, have you tried it ?
> 
> I would need to test all boards using this driver to not fail booting.
> Usually I2C drivers are moved to subsys_initcall because they need
> access to something critical (PMIC, GPIOs...) early. I don't see a sane
> way to do that testing.

Still, I would like to get a better view on the problems we should expect, by 
testing this on the latest boards for instance.

> Other than that, even if we move to module_init, we reduce the chance of
> getting a deferred probe, but we do not eliminate it...

Sure, but reducing the chance of deferred probe is a good idea in my opinion 
:-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Laurent Pinchart]

On Thursday 11 December 2014 22:47:32 Wolfram Sang wrote:
> > > Note that the I2C drives uses subsys_initcall() for historic reasons,
> > > while the DMA driver uses module_init(). This is hard to revert without
> > > introducing potential regressions on older boards. So, the I2C DMA
> > > support needs to handle deferred probe definately. I am with Laurent, I
> > > don't see any other way, but I'd be glad to be enlightened...
> > 
> > While I believe that requesting the channel at transfer time is the good
> > solution, I think we should still try to move to module initcalls where
> > possible. The risk of regressions is real so proper testing is needed. My
> > question is, have you tried it ?
> 
> I would need to test all boards using this driver to not fail booting.
> Usually I2C drivers are moved to subsys_initcall because they need
> access to something critical (PMIC, GPIOs...) early. I don't see a sane
> way to do that testing.

Still, I would like to get a better view on the problems we should expect, by 
testing this on the latest boards for instance.

> Other than that, even if we move to module_init, we reduce the chance of
> getting a deferred probe, but we do not eliminate it...

Sure, but reducing the chance of deferred probe is a good idea in my opinion 
:-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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


> > Other than that, even if we move to module_init, we reduce the chance of
> > getting a deferred probe, but we do not eliminate it...
> 
> Sure, but reducing the chance of deferred probe is a good idea in my opinion 
> :-)

Yes, that's a thing nice to have. It is just, I ought to work on "must
have" tasks first ;)


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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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


> > Other than that, even if we move to module_init, we reduce the chance of
> > getting a deferred probe, but we do not eliminate it...
> 
> Sure, but reducing the chance of deferred probe is a good idea in my opinion 
> :-)

Yes, that's a thing nice to have. It is just, I ought to work on "must
have" tasks first ;)


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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Vinod Koul]

On Thu, Dec 11, 2014 at 11:52:10PM +0200, Laurent Pinchart wrote:
> On Thursday 11 December 2014 22:47:32 Wolfram Sang wrote:
> > > > Note that the I2C drives uses subsys_initcall() for historic reasons,
> > > > while the DMA driver uses module_init(). This is hard to revert without
> > > > introducing potential regressions on older boards. So, the I2C DMA
> > > > support needs to handle deferred probe definately. I am with Laurent, I
> > > > don't see any other way, but I'd be glad to be enlightened...
> > > 
> > > While I believe that requesting the channel at transfer time is the good
> > > solution, I think we should still try to move to module initcalls where
> > > possible. The risk of regressions is real so proper testing is needed. My
> > > question is, have you tried it ?
> > 
> > I would need to test all boards using this driver to not fail booting.
> > Usually I2C drivers are moved to subsys_initcall because they need
> > access to something critical (PMIC, GPIOs...) early. I don't see a sane
> > way to do that testing.
> 
> Still, I would like to get a better view on the problems we should expect, by 
> testing this on the latest boards for instance.
> 
> > Other than that, even if we move to module_init, we reduce the chance of
> > getting a deferred probe, but we do not eliminate it...
> 
> Sure, but reducing the chance of deferred probe is a good idea in my opinion 
> :-)
Okay so what is the issue here, this statement is a bit worrying. Why do
you guys want to reduce the chance of deferred probe? I assumed that issue was
the channels availability...

-- 
~Vinod

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Vinod Koul]

On Thu, Dec 11, 2014 at 11:52:10PM +0200, Laurent Pinchart wrote:
> On Thursday 11 December 2014 22:47:32 Wolfram Sang wrote:
> > > > Note that the I2C drives uses subsys_initcall() for historic reasons,
> > > > while the DMA driver uses module_init(). This is hard to revert without
> > > > introducing potential regressions on older boards. So, the I2C DMA
> > > > support needs to handle deferred probe definately. I am with Laurent, I
> > > > don't see any other way, but I'd be glad to be enlightened...
> > > 
> > > While I believe that requesting the channel at transfer time is the good
> > > solution, I think we should still try to move to module initcalls where
> > > possible. The risk of regressions is real so proper testing is needed. My
> > > question is, have you tried it ?
> > 
> > I would need to test all boards using this driver to not fail booting.
> > Usually I2C drivers are moved to subsys_initcall because they need
> > access to something critical (PMIC, GPIOs...) early. I don't see a sane
> > way to do that testing.
> 
> Still, I would like to get a better view on the problems we should expect, by 
> testing this on the latest boards for instance.
> 
> > Other than that, even if we move to module_init, we reduce the chance of
> > getting a deferred probe, but we do not eliminate it...
> 
> Sure, but reducing the chance of deferred probe is a good idea in my opinion 
> :-)
Okay so what is the issue here, this statement is a bit worrying. Why do
you guys want to reduce the chance of deferred probe? I assumed that issue was
the channels availability...

-- 
~Vinod

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Geert Uytterhoeven]

Hi Vinod,

On Mon, Dec 15, 2014 at 7:43 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Thu, Dec 11, 2014 at 11:52:10PM +0200, Laurent Pinchart wrote:
>> On Thursday 11 December 2014 22:47:32 Wolfram Sang wrote:
>> > > > Note that the I2C drives uses subsys_initcall() for historic reasons,
>> > > > while the DMA driver uses module_init(). This is hard to revert without
>> > > > introducing potential regressions on older boards. So, the I2C DMA
>> > > > support needs to handle deferred probe definately. I am with Laurent, I
>> > > > don't see any other way, but I'd be glad to be enlightened...
>> > >
>> > > While I believe that requesting the channel at transfer time is the good
>> > > solution, I think we should still try to move to module initcalls where
>> > > possible. The risk of regressions is real so proper testing is needed. My
>> > > question is, have you tried it ?
>> >
>> > I would need to test all boards using this driver to not fail booting.
>> > Usually I2C drivers are moved to subsys_initcall because they need
>> > access to something critical (PMIC, GPIOs...) early. I don't see a sane
>> > way to do that testing.
>>
>> Still, I would like to get a better view on the problems we should expect, by
>> testing this on the latest boards for instance.
>>
>> > Other than that, even if we move to module_init, we reduce the chance of
>> > getting a deferred probe, but we do not eliminate it...
>>
>> Sure, but reducing the chance of deferred probe is a good idea in my opinion
>> :-)
> Okay so what is the issue here, this statement is a bit worrying. Why do
> you guys want to reduce the chance of deferred probe? I assumed that issue was
> the channels availability...

Please let me summarize...

During probe of a DMA client driver, the DMA engine driver may not be available,
causing dma_request_slave_channel*() to return -EPROBE-DEFER.
There are actually two different reasons that the DMA engine driver may not
be available:
  1. The DMA engine driver hasn't been initialized yet, due to probe order.
     This is more likely to happen with i2c client drivers, as they
are initialized
     from subsys_initcall() instead of module_init() (E.g. I never saw
it with the
     spi-rspi driver).
     => The DMA client driver wants to return -EPROBE_DEFER too, and
       retry later.
  2. The DMA engine driver is not included in the kernel build.
     => The DMA client driver wants to fall back to PIO.

Now, how to distinguish between the two cases above?

Currently, e.g. i2c-sh_mobile always returns -EPROBE_DEFER, never falling
back to PIO, breaking case 2. While e.g. spi-rspi always falls back to PIO,
which is suboptimal in case 1 (but I never encountered that case with spi).

Solutions under consideration:
  1. Wolfram posted a patch to make i2c-sh_mobile fall back to PIO, and retry
     DMA initialization in every request, so it will switch to DMA
when it becomes
     available. But this is suboptimal, as it adds overhead to every
request (and
     DMA may never become available in case 2).

  2. Delay i2c initialization, by moving from subsys_initcall() to
module_init(),
      in the hope the i2c client driver will be initialized after the
DMA engine.
     This is being discussed in the thread you quoted above.

I hope this explains the problem well.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Geert Uytterhoeven]

Hi Vinod,

On Mon, Dec 15, 2014 at 7:43 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Thu, Dec 11, 2014 at 11:52:10PM +0200, Laurent Pinchart wrote:
>> On Thursday 11 December 2014 22:47:32 Wolfram Sang wrote:
>> > > > Note that the I2C drives uses subsys_initcall() for historic reasons,
>> > > > while the DMA driver uses module_init(). This is hard to revert without
>> > > > introducing potential regressions on older boards. So, the I2C DMA
>> > > > support needs to handle deferred probe definately. I am with Laurent, I
>> > > > don't see any other way, but I'd be glad to be enlightened...
>> > >
>> > > While I believe that requesting the channel at transfer time is the good
>> > > solution, I think we should still try to move to module initcalls where
>> > > possible. The risk of regressions is real so proper testing is needed. My
>> > > question is, have you tried it ?
>> >
>> > I would need to test all boards using this driver to not fail booting.
>> > Usually I2C drivers are moved to subsys_initcall because they need
>> > access to something critical (PMIC, GPIOs...) early. I don't see a sane
>> > way to do that testing.
>>
>> Still, I would like to get a better view on the problems we should expect, by
>> testing this on the latest boards for instance.
>>
>> > Other than that, even if we move to module_init, we reduce the chance of
>> > getting a deferred probe, but we do not eliminate it...
>>
>> Sure, but reducing the chance of deferred probe is a good idea in my opinion
>> :-)
> Okay so what is the issue here, this statement is a bit worrying. Why do
> you guys want to reduce the chance of deferred probe? I assumed that issue was
> the channels availability...

Please let me summarize...

During probe of a DMA client driver, the DMA engine driver may not be available,
causing dma_request_slave_channel*() to return -EPROBE-DEFER.
There are actually two different reasons that the DMA engine driver may not
be available:
  1. The DMA engine driver hasn't been initialized yet, due to probe order.
     This is more likely to happen with i2c client drivers, as they
are initialized
     from subsys_initcall() instead of module_init() (E.g. I never saw
it with the
     spi-rspi driver).
     => The DMA client driver wants to return -EPROBE_DEFER too, and
       retry later.
  2. The DMA engine driver is not included in the kernel build.
     => The DMA client driver wants to fall back to PIO.

Now, how to distinguish between the two cases above?

Currently, e.g. i2c-sh_mobile always returns -EPROBE_DEFER, never falling
back to PIO, breaking case 2. While e.g. spi-rspi always falls back to PIO,
which is suboptimal in case 1 (but I never encountered that case with spi).

Solutions under consideration:
  1. Wolfram posted a patch to make i2c-sh_mobile fall back to PIO, and retry
     DMA initialization in every request, so it will switch to DMA
when it becomes
     available. But this is suboptimal, as it adds overhead to every
request (and
     DMA may never become available in case 2).

  2. Delay i2c initialization, by moving from subsys_initcall() to
module_init(),
      in the hope the i2c client driver will be initialized after the
DMA engine.
     This is being discussed in the thread you quoted above.

I hope this explains the problem well.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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


> Please let me summarize...

Thanks for the summary, Geert!

> Solutions under consideration:
>   1. Wolfram posted a patch to make i2c-sh_mobile fall back to PIO,
>   and retry DMA initialization in every request, so it will switch to
>   DMA when it becomes available. But this is suboptimal, as it adds
>   overhead to every request (and DMA may never become available in
>   case 2).

Still, I'd think I should repost my patches with your comments
addressed. It does add a bit of overhead IF the dmaengine core is
compiled in AND the driver for the DMA hardware is not. Well, yeah. On
the other hand, it fixes the regression that the driver is not even
loaded in that case (because it currently will be deferred endlessly).

What do you think?

All the best,

   Wolfram


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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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


> Please let me summarize...

Thanks for the summary, Geert!

> Solutions under consideration:
>   1. Wolfram posted a patch to make i2c-sh_mobile fall back to PIO,
>   and retry DMA initialization in every request, so it will switch to
>   DMA when it becomes available. But this is suboptimal, as it adds
>   overhead to every request (and DMA may never become available in
>   case 2).

Still, I'd think I should repost my patches with your comments
addressed. It does add a bit of overhead IF the dmaengine core is
compiled in AND the driver for the DMA hardware is not. Well, yeah. On
the other hand, it fixes the regression that the driver is not even
loaded in that case (because it currently will be deferred endlessly).

What do you think?

All the best,

   Wolfram


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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Vinod Koul]

On Mon, Dec 15, 2014 at 09:31:01AM +0100, Geert Uytterhoeven wrote:
> Please let me summarize...
Thanks :)

> During probe of a DMA client driver, the DMA engine driver may not be available,
> causing dma_request_slave_channel*() to return -EPROBE-DEFER.
> There are actually two different reasons that the DMA engine driver may not
> be available:
>   1. The DMA engine driver hasn't been initialized yet, due to probe order.
>      This is more likely to happen with i2c client drivers, as they
> are initialized
>      from subsys_initcall() instead of module_init() (E.g. I never saw
> it with the
>      spi-rspi driver).
>      => The DMA client driver wants to return -EPROBE_DEFER too, and
>        retry later.
>   2. The DMA engine driver is not included in the kernel build.
>      => The DMA client driver wants to fall back to PIO.
> 
> Now, how to distinguish between the two cases above?
Quite right, this is a good question. Today we cannot distinguish between the
two. Should we improve the deferred probe to tell us when the init
is complete and all the modules have been initialized? If we ever have such
a mechanism to check then we know no modules are to be inserted then we can
fall back to PIO mode. Without that we should use some timeout counter to
fall back on, say try requesting 5 times and give up and move to PIO after that

> Currently, e.g. i2c-sh_mobile always returns -EPROBE_DEFER, never falling
> back to PIO, breaking case 2. While e.g. spi-rspi always falls back to PIO,
> which is suboptimal in case 1 (but I never encountered that case with spi).
> 
> Solutions under consideration:
>   1. Wolfram posted a patch to make i2c-sh_mobile fall back to PIO, and retry
>      DMA initialization in every request, so it will switch to DMA
> when it becomes
>      available. But this is suboptimal, as it adds overhead to every
> request (and
>      DMA may never become available in case 2).
> 
>   2. Delay i2c initialization, by moving from subsys_initcall() to
> module_init(),
>       in the hope the i2c client driver will be initialized after the
> DMA engine.
>      This is being discussed in the thread you quoted above.
> 
> I hope this explains the problem well.
Yes and it has nothing to do with channels being exhausted, the problem I
assumed earlier!!

-- 
~Vinod

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Vinod Koul]

On Mon, Dec 15, 2014 at 09:31:01AM +0100, Geert Uytterhoeven wrote:
> Please let me summarize...
Thanks :)

> During probe of a DMA client driver, the DMA engine driver may not be available,
> causing dma_request_slave_channel*() to return -EPROBE-DEFER.
> There are actually two different reasons that the DMA engine driver may not
> be available:
>   1. The DMA engine driver hasn't been initialized yet, due to probe order.
>      This is more likely to happen with i2c client drivers, as they
> are initialized
>      from subsys_initcall() instead of module_init() (E.g. I never saw
> it with the
>      spi-rspi driver).
>      => The DMA client driver wants to return -EPROBE_DEFER too, and
>        retry later.
>   2. The DMA engine driver is not included in the kernel build.
>      => The DMA client driver wants to fall back to PIO.
> 
> Now, how to distinguish between the two cases above?
Quite right, this is a good question. Today we cannot distinguish between the
two. Should we improve the deferred probe to tell us when the init
is complete and all the modules have been initialized? If we ever have such
a mechanism to check then we know no modules are to be inserted then we can
fall back to PIO mode. Without that we should use some timeout counter to
fall back on, say try requesting 5 times and give up and move to PIO after that

> Currently, e.g. i2c-sh_mobile always returns -EPROBE_DEFER, never falling
> back to PIO, breaking case 2. While e.g. spi-rspi always falls back to PIO,
> which is suboptimal in case 1 (but I never encountered that case with spi).
> 
> Solutions under consideration:
>   1. Wolfram posted a patch to make i2c-sh_mobile fall back to PIO, and retry
>      DMA initialization in every request, so it will switch to DMA
> when it becomes
>      available. But this is suboptimal, as it adds overhead to every
> request (and
>      DMA may never become available in case 2).
> 
>   2. Delay i2c initialization, by moving from subsys_initcall() to
> module_init(),
>       in the hope the i2c client driver will be initialized after the
> DMA engine.
>      This is being discussed in the thread you quoted above.
> 
> I hope this explains the problem well.
Yes and it has nothing to do with channels being exhausted, the problem I
assumed earlier!!

-- 
~Vinod

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Geert Uytterhoeven]

Hi Wolfram,

On Mon, Dec 15, 2014 at 10:06 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> Solutions under consideration:
>>   1. Wolfram posted a patch to make i2c-sh_mobile fall back to PIO,
>>   and retry DMA initialization in every request, so it will switch to
>>   DMA when it becomes available. But this is suboptimal, as it adds
>>   overhead to every request (and DMA may never become available in
>>   case 2).
>
> Still, I'd think I should repost my patches with your comments
> addressed. It does add a bit of overhead IF the dmaengine core is
> compiled in AND the driver for the DMA hardware is not. Well, yeah. On
> the other hand, it fixes the regression that the driver is not even
> loaded in that case (because it currently will be deferred endlessly).
>
> What do you think?

I think it doesn't hurt to repost, now more people understand the intrinsics
of the problem.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Geert Uytterhoeven]

Hi Wolfram,

On Mon, Dec 15, 2014 at 10:06 AM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
>> Solutions under consideration:
>>   1. Wolfram posted a patch to make i2c-sh_mobile fall back to PIO,
>>   and retry DMA initialization in every request, so it will switch to
>>   DMA when it becomes available. But this is suboptimal, as it adds
>>   overhead to every request (and DMA may never become available in
>>   case 2).
>
> Still, I'd think I should repost my patches with your comments
> addressed. It does add a bit of overhead IF the dmaengine core is
> compiled in AND the driver for the DMA hardware is not. Well, yeah. On
> the other hand, it fixes the regression that the driver is not even
> loaded in that case (because it currently will be deferred endlessly).
>
> What do you think?

I think it doesn't hurt to repost, now more people understand the intrinsics
of the problem.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Laurent Pinchart]

Hi Vinod,

On Monday 15 December 2014 14:43:14 Vinod Koul wrote:
> On Mon, Dec 15, 2014 at 09:31:01AM +0100, Geert Uytterhoeven wrote:
> > Please let me summarize...
> 
> Thanks :)
> 
> > During probe of a DMA client driver, the DMA engine driver may not be
> > available, causing dma_request_slave_channel*() to return -EPROBE-DEFER.
> > There are actually two different reasons that the DMA engine driver may
> > not be available:
> >
> > 1. The DMA engine driver hasn't been initialized yet, due to probe order.
> >    This is more likely to happen with i2c client drivers, as they are
> >    initialized from subsys_initcall() instead of module_init() (E.g. I
> >    never saw it with the spi-rspi driver).
> >    => The DMA client driver wants to return -EPROBE_DEFER too, and
> >       retry later.
> >   
> > 2. The DMA engine driver is not included in the kernel build.
> >    => The DMA client driver wants to fall back to PIO.
> > 
> > Now, how to distinguish between the two cases above?
> 
> Quite right, this is a good question. Today we cannot distinguish between
> the two. Should we improve the deferred probe to tell us when the init is
> complete and all the modules have been initialized?

I don't think that's possible, as you can never know when a module will be 
loaded.

> If we ever have such a mechanism to check then we know no modules are to be
> inserted then we can fall back to PIO mode. Without that we should use some
> timeout counter to fall back on, say try requesting 5 times and give up and
> move to PIO after that

That could be a performance improvement, but I wonder whether it's worth it. 
If DT specifies DMA channels for the I2C controller, and the DMA core is 
compiled in, and the DMA engine driver is compiled as a module and never 
loaded, then yes, there will be a small overhead for each I2C transaction, but 
I'd argue that such a combination of conditions is asking for trouble anyway 
:-)

> > Currently, e.g. i2c-sh_mobile always returns -EPROBE_DEFER, never falling
> > back to PIO, breaking case 2. While e.g. spi-rspi always falls back to
> > PIO, which is suboptimal in case 1 (but I never encountered that case with
> > spi).
> > 
> > Solutions under consideration:
> > 1. Wolfram posted a patch to make i2c-sh_mobile fall back to PIO, and
> >    retry DMA initialization in every request, so it will switch to DMA
> >    when it becomes available. But this is suboptimal, as it adds overhead
> >    to every request (and DMA may never become available in case 2).
> >   
> > 2. Delay i2c initialization, by moving from subsys_initcall() to
> >    module_init(), in the hope the i2c client driver will be initialized
> >    after the DMA engine.
> > 
> >    This is being discussed in the thread you quoted above.
> > 
> > I hope this explains the problem well.
> 
> Yes and it has nothing to do with channels being exhausted, the problem I
> assumed earlier!!

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Laurent Pinchart]

Hi Vinod,

On Monday 15 December 2014 14:43:14 Vinod Koul wrote:
> On Mon, Dec 15, 2014 at 09:31:01AM +0100, Geert Uytterhoeven wrote:
> > Please let me summarize...
> 
> Thanks :)
> 
> > During probe of a DMA client driver, the DMA engine driver may not be
> > available, causing dma_request_slave_channel*() to return -EPROBE-DEFER.
> > There are actually two different reasons that the DMA engine driver may
> > not be available:
> >
> > 1. The DMA engine driver hasn't been initialized yet, due to probe order.
> >    This is more likely to happen with i2c client drivers, as they are
> >    initialized from subsys_initcall() instead of module_init() (E.g. I
> >    never saw it with the spi-rspi driver).
> >    => The DMA client driver wants to return -EPROBE_DEFER too, and
> >       retry later.
> >   
> > 2. The DMA engine driver is not included in the kernel build.
> >    => The DMA client driver wants to fall back to PIO.
> > 
> > Now, how to distinguish between the two cases above?
> 
> Quite right, this is a good question. Today we cannot distinguish between
> the two. Should we improve the deferred probe to tell us when the init is
> complete and all the modules have been initialized?

I don't think that's possible, as you can never know when a module will be 
loaded.

> If we ever have such a mechanism to check then we know no modules are to be
> inserted then we can fall back to PIO mode. Without that we should use some
> timeout counter to fall back on, say try requesting 5 times and give up and
> move to PIO after that

That could be a performance improvement, but I wonder whether it's worth it. 
If DT specifies DMA channels for the I2C controller, and the DMA core is 
compiled in, and the DMA engine driver is compiled as a module and never 
loaded, then yes, there will be a small overhead for each I2C transaction, but 
I'd argue that such a combination of conditions is asking for trouble anyway 
:-)

> > Currently, e.g. i2c-sh_mobile always returns -EPROBE_DEFER, never falling
> > back to PIO, breaking case 2. While e.g. spi-rspi always falls back to
> > PIO, which is suboptimal in case 1 (but I never encountered that case with
> > spi).
> > 
> > Solutions under consideration:
> > 1. Wolfram posted a patch to make i2c-sh_mobile fall back to PIO, and
> >    retry DMA initialization in every request, so it will switch to DMA
> >    when it becomes available. But this is suboptimal, as it adds overhead
> >    to every request (and DMA may never become available in case 2).
> >   
> > 2. Delay i2c initialization, by moving from subsys_initcall() to
> >    module_init(), in the hope the i2c client driver will be initialized
> >    after the DMA engine.
> > 
> >    This is being discussed in the thread you quoted above.
> > 
> > I hope this explains the problem well.
> 
> Yes and it has nothing to do with channels being exhausted, the problem I
> assumed earlier!!

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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


> Quite right, this is a good question. Today we cannot distinguish between the
> two. Should we improve the deferred probe to tell us when the init
> is complete and all the modules have been initialized? If we ever have such
> a mechanism to check then we know no modules are to be inserted then we can
> fall back to PIO mode.

Well, the user can load modules from userspace. And I could imagine
there are people wanting to defer things until they load a module
manually. And surely there will be a case where, for some reason, the
deferred code has to be built-in. I trust Murphy's law here...

> Without that we should use some timeout counter to fall back on, say
> try requesting 5 times and give up and move to PIO after that

Won't work. In my case, the I2C driver gets deferred two times and then
the list becomes static already.

On top of that, arbitrary retry values don't make me happy. Mentioning
Murphy's law again...


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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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


> Quite right, this is a good question. Today we cannot distinguish between the
> two. Should we improve the deferred probe to tell us when the init
> is complete and all the modules have been initialized? If we ever have such
> a mechanism to check then we know no modules are to be inserted then we can
> fall back to PIO mode.

Well, the user can load modules from userspace. And I could imagine
there are people wanting to defer things until they load a module
manually. And surely there will be a case where, for some reason, the
deferred code has to be built-in. I trust Murphy's law here...

> Without that we should use some timeout counter to fall back on, say
> try requesting 5 times and give up and move to PIO after that

Won't work. In my case, the I2C driver gets deferred two times and then
the list becomes static already.

On top of that, arbitrary retry values don't make me happy. Mentioning
Murphy's law again...


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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Vinod Koul]

On Mon, Dec 15, 2014 at 11:42:46AM +0200, Laurent Pinchart wrote:
> Hi Vinod,
> 
> On Monday 15 December 2014 14:43:14 Vinod Koul wrote:
> > On Mon, Dec 15, 2014 at 09:31:01AM +0100, Geert Uytterhoeven wrote:
> > > Please let me summarize...
> > 
> > Thanks :)
> > 
> > > During probe of a DMA client driver, the DMA engine driver may not be
> > > available, causing dma_request_slave_channel*() to return -EPROBE-DEFER.
> > > There are actually two different reasons that the DMA engine driver may
> > > not be available:
> > >
> > > 1. The DMA engine driver hasn't been initialized yet, due to probe order.
> > >    This is more likely to happen with i2c client drivers, as they are
> > >    initialized from subsys_initcall() instead of module_init() (E.g. I
> > >    never saw it with the spi-rspi driver).
> > >    => The DMA client driver wants to return -EPROBE_DEFER too, and
> > >       retry later.
> > >   
> > > 2. The DMA engine driver is not included in the kernel build.
> > >    => The DMA client driver wants to fall back to PIO.
> > > 
> > > Now, how to distinguish between the two cases above?
> > 
> > Quite right, this is a good question. Today we cannot distinguish between
> > the two. Should we improve the deferred probe to tell us when the init is
> > complete and all the modules have been initialized?
> 
> I don't think that's possible, as you can never know when a module will be 
> loaded.
In a production system it is quite reasonable to assume that usermode will
find all the modules available for the devices detected and insert them post
boot within a reasonable amount of time.

> > If we ever have such a mechanism to check then we know no modules are to be
> > inserted then we can fall back to PIO mode. Without that we should use some
> > timeout counter to fall back on, say try requesting 5 times and give up and
> > move to PIO after that
> 
> That could be a performance improvement, but I wonder whether it's worth it. 
> If DT specifies DMA channels for the I2C controller, and the DMA core is 
> compiled in, and the DMA engine driver is compiled as a module and never 
> loaded, then yes, there will be a small overhead for each I2C transaction, but 
> I'd argue that such a combination of conditions is asking for trouble anyway 
> :-)
Yes that is quite reasonable. For a proper production system it should be a
fair assumption that drivers for hardware available should be available and if
not possibly a buggy configuration which must be fixed anyway.

-- 
~Vinod

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Vinod Koul]

On Mon, Dec 15, 2014 at 11:42:46AM +0200, Laurent Pinchart wrote:
> Hi Vinod,
> 
> On Monday 15 December 2014 14:43:14 Vinod Koul wrote:
> > On Mon, Dec 15, 2014 at 09:31:01AM +0100, Geert Uytterhoeven wrote:
> > > Please let me summarize...
> > 
> > Thanks :)
> > 
> > > During probe of a DMA client driver, the DMA engine driver may not be
> > > available, causing dma_request_slave_channel*() to return -EPROBE-DEFER.
> > > There are actually two different reasons that the DMA engine driver may
> > > not be available:
> > >
> > > 1. The DMA engine driver hasn't been initialized yet, due to probe order.
> > >    This is more likely to happen with i2c client drivers, as they are
> > >    initialized from subsys_initcall() instead of module_init() (E.g. I
> > >    never saw it with the spi-rspi driver).
> > >    => The DMA client driver wants to return -EPROBE_DEFER too, and
> > >       retry later.
> > >   
> > > 2. The DMA engine driver is not included in the kernel build.
> > >    => The DMA client driver wants to fall back to PIO.
> > > 
> > > Now, how to distinguish between the two cases above?
> > 
> > Quite right, this is a good question. Today we cannot distinguish between
> > the two. Should we improve the deferred probe to tell us when the init is
> > complete and all the modules have been initialized?
> 
> I don't think that's possible, as you can never know when a module will be 
> loaded.
In a production system it is quite reasonable to assume that usermode will
find all the modules available for the devices detected and insert them post
boot within a reasonable amount of time.

> > If we ever have such a mechanism to check then we know no modules are to be
> > inserted then we can fall back to PIO mode. Without that we should use some
> > timeout counter to fall back on, say try requesting 5 times and give up and
> > move to PIO after that
> 
> That could be a performance improvement, but I wonder whether it's worth it. 
> If DT specifies DMA channels for the I2C controller, and the DMA core is 
> compiled in, and the DMA engine driver is compiled as a module and never 
> loaded, then yes, there will be a small overhead for each I2C transaction, but 
> I'd argue that such a combination of conditions is asking for trouble anyway 
> :-)
Yes that is quite reasonable. For a proper production system it should be a
fair assumption that drivers for hardware available should be available and if
not possibly a buggy configuration which must be fixed anyway.

-- 
~Vinod

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Vinod Koul]

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

On Mon, Dec 15, 2014 at 10:45:33AM +0100, Wolfram Sang wrote:
> 
> > Quite right, this is a good question. Today we cannot distinguish between the
> > two. Should we improve the deferred probe to tell us when the init
> > is complete and all the modules have been initialized? If we ever have such
> > a mechanism to check then we know no modules are to be inserted then we can
> > fall back to PIO mode.
> 
> Well, the user can load modules from userspace. And I could imagine
> there are people wanting to defer things until they load a module
> manually. And surely there will be a case where, for some reason, the
> deferred code has to be built-in. I trust Murphy's law here...
in a dev system yes, but in a production system there wont be manual step!

-- 
~Vinod


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

Re: [PATCH 2/5] i2c: sh_mobile: add DMA support

[Vinod Koul]

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

On Mon, Dec 15, 2014 at 10:45:33AM +0100, Wolfram Sang wrote:
> 
> > Quite right, this is a good question. Today we cannot distinguish between the
> > two. Should we improve the deferred probe to tell us when the init
> > is complete and all the modules have been initialized? If we ever have such
> > a mechanism to check then we know no modules are to be inserted then we can
> > fall back to PIO mode.
> 
> Well, the user can load modules from userspace. And I could imagine
> there are people wanting to defer things until they load a module
> manually. And surely there will be a case where, for some reason, the
> deferred code has to be built-in. I trust Murphy's law here...
in a dev system yes, but in a production system there wont be manual step!

-- 
~Vinod


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