[RFC 0/3] i2c: sh_mobile: add DMA support

[RFC 0/3] i2c: sh_mobile: add DMA support

[Wolfram Sang]

Here is my RFC 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. The major
issue currently: This driver uses subsys_initcall() but at that time DMA is not
available, and there is no deferred probe for DMA. So, switching to
module_init() makes DMA work, but this may cause side-effects for older boards
which rely on I2C being available early (to control some PMIC, for example).
This needs some more investigation. Also, the driver (like all I2C DMA drivers
currently) assumes that i2c message buffers are DMA capable. This is not always
true and might need some assistance from the I2C core.

Other than that, 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,

    Wolfram


Wolfram Sang (3):
  i2c: sh_mobile: add DMA support
  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                 | 203 +++++++++++++++++++--
 4 files changed, 203 insertions(+), 19 deletions(-)

-- 
2.0.0

[RFC 1/3] 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>
---
 .../devicetree/bindings/i2c/i2c-sh_mobile.txt      |   5 +
 drivers/i2c/busses/i2c-sh_mobile.c                 | 203 +++++++++++++++++++--
 2 files changed, 189 insertions(+), 19 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 8b5e79cb4468..23664b21ab37 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
@@ -33,6 +35,9 @@
 #include <linux/io.h>
 #include <linux/slab.h>
 #include <linux/of_device.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
+#include <linux/sh_dma.h>
 #include <linux/i2c/i2c-sh_mobile.h>
 
 /* Transmit operation:                                                      */
@@ -114,6 +119,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 +144,12 @@ 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;
+	bool buf_mapped;
 };
 
 struct sh_mobile_dt_config {
@@ -175,6 +187,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 +350,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 +409,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 +474,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 +483,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 +512,82 @@ 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_FROM_DEVICE)
+		dmaengine_terminate_all(pd->dma_rx);
+	if (pd->dma_direction = DMA_TO_DEVICE)
+		dmaengine_terminate_all(pd->dma_tx);
+
+	if (pd->buf_mapped) {
+		dma_unmap_single(pd->dev, sg_dma_address(&pd->sg),
+				 pd->msg->len, pd->dma_direction);
+		pd->buf_mapped = false;
+	}
+
+	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->buf_mapped = false;
+	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_warn(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->buf_mapped = true;
+	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_warn(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_warn(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 +612,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 > 4)
+		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 +698,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 +749,70 @@ static const struct of_device_id sh_mobile_i2c_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, sh_mobile_i2c_dt_ids);
 
+static struct dma_chan *sh_mobile_i2c_request_dma_chan(struct device *dev,
+					      enum dma_transfer_direction dir,
+					      dma_addr_t port_addr)
+{
+	dma_cap_mask_t mask;
+	struct dma_chan *chan;
+	struct dma_slave_config cfg;
+	int ret;
+
+	dma_cap_zero(mask);
+	dma_cap_set(DMA_SLAVE, mask);
+
+	chan = dma_request_slave_channel_compat(mask, shdma_chan_filter,
+				(void *)0UL, dev, dir = DMA_MEM_TO_DEV ? "tx" : "rx");
+
+	if (!chan)
+		return NULL;
+
+	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_warn(dev, "dmaengine_slave_config failed %d\n", ret);
+		dma_release_channel(chan);
+		return NULL;
+	}
+
+	return chan;
+}
+
+static void sh_mobile_i2c_request_dma(struct sh_mobile_i2c_data *pd, const struct resource *res)
+{
+	pd->buf_mapped = false;
+	pd->dma_direction = DMA_NONE;
+	sg_init_table(&pd->sg, 1);
+
+	pd->dma_rx = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
+					       res->start + ICDR);
+
+	pd->dma_tx = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
+					       res->start + ICDR);
+}
+
+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 +899,8 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
 	if (ret)
 		return ret;
 
+	sh_mobile_i2c_request_dma(pd, res);
+
 	/* 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;
 }
@@ -805,19 +981,8 @@ static struct platform_driver sh_mobile_i2c_driver = {
 	.probe		= sh_mobile_i2c_probe,
 	.remove		= sh_mobile_i2c_remove,
 };
+module_platform_driver(sh_mobile_i2c_driver);
 
-static int __init sh_mobile_i2c_adap_init(void)
-{
-	return platform_driver_register(&sh_mobile_i2c_driver);
-}
-
-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");
-- 
2.0.0

Re: [RFC 1/3] i2c: sh_mobile: add DMA support

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Friday 31 October 2014 11:51:16 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>
> ---
>  .../devicetree/bindings/i2c/i2c-sh_mobile.txt      |   5 +
>  drivers/i2c/busses/i2c-sh_mobile.c                 | 203 ++++++++++++++++--
>  2 files changed, 189 insertions(+), 19 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 8b5e79cb4468..23664b21ab37
> 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
> @@ -33,6 +35,9 @@
>  #include <linux/io.h>
>  #include <linux/slab.h>
>  #include <linux/of_device.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/sh_dma.h>

I would have tried to keep the headers alphabetically sorted, if they had been 
sorted in the first place :-)

>  #include <linux/i2c/i2c-sh_mobile.h>
> 
>  /* Transmit operation:                                                     
> */ @@ -114,6 +119,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 +144,12 @@ 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;
> +	bool buf_mapped;
>  };
> 
>  struct sh_mobile_dt_config {
> @@ -175,6 +187,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 +350,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 +409,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 +474,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 +483,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 +512,82 @@ 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_FROM_DEVICE)
> +		dmaengine_terminate_all(pd->dma_rx);
> +	if (pd->dma_direction = DMA_TO_DEVICE)

else ?

> +		dmaengine_terminate_all(pd->dma_tx);
> +
> +	if (pd->buf_mapped) {
> +		dma_unmap_single(pd->dev, sg_dma_address(&pd->sg),
> +				 pd->msg->len, pd->dma_direction);
> +		pd->buf_mapped = false;
> +	}
> +
> +	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->buf_mapped = false;
> +	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_warn(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->buf_mapped = true;

Can't you use dma_direction != DMA_NONE to detect whether the buffer has been 
mapped, instead of adding a new field to struct sh_mobile_i2c_data ? You could 
then simplify sh_mobile_i2c_cleanup_dma() by returning immediately at the 
beginning of the function if dma_direction = DMA_NONE.

> +	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_warn(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_warn(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 +612,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 > 4)
> +		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 +698,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 +749,70 @@ static const struct of_device_id sh_mobile_i2c_dt_ids[]
> = { };
>  MODULE_DEVICE_TABLE(of, sh_mobile_i2c_dt_ids);
> 
> +static struct dma_chan *sh_mobile_i2c_request_dma_chan(struct device *dev,
> +					      enum dma_transfer_direction dir,
> +					      dma_addr_t port_addr)
> +{
> +	dma_cap_mask_t mask;
> +	struct dma_chan *chan;
> +	struct dma_slave_config cfg;
> +	int ret;
> +
> +	dma_cap_zero(mask);
> +	dma_cap_set(DMA_SLAVE, mask);
> +
> +	chan = dma_request_slave_channel_compat(mask, shdma_chan_filter,
> +				(void *)0UL, dev, dir = DMA_MEM_TO_DEV ? "tx" : "rx");
> +
> +	if (!chan)
> +		return NULL;
> +
> +	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_warn(dev, "dmaengine_slave_config failed %d\n", ret);
> +		dma_release_channel(chan);
> +		return NULL;
> +	}
> +
> +	return chan;
> +}
> +
> +static void sh_mobile_i2c_request_dma(struct sh_mobile_i2c_data *pd, const
> struct resource *res)
> +{
> +	pd->buf_mapped = false;
> +	pd->dma_direction = DMA_NONE;
> +	sg_init_table(&pd->sg, 1);
> +
> +	pd->dma_rx = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_DEV_TO_MEM,
> +					       res->start + ICDR);
> +
> +	pd->dma_tx = sh_mobile_i2c_request_dma_chan(pd->dev, DMA_MEM_TO_DEV,
> +					       res->start + ICDR);
> +}
> +
> +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 +899,8 @@ static int sh_mobile_i2c_probe(struct platform_device
> *dev) if (ret)
>  		return ret;
> 
> +	sh_mobile_i2c_request_dma(pd, res);
> +
>  	/* 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;
>  }
> @@ -805,19 +981,8 @@ static struct platform_driver sh_mobile_i2c_driver = {
>  	.probe		= sh_mobile_i2c_probe,
>  	.remove		= sh_mobile_i2c_remove,
>  };
> +module_platform_driver(sh_mobile_i2c_driver);
> 
> -static int __init sh_mobile_i2c_adap_init(void)
> -{
> -	return platform_driver_register(&sh_mobile_i2c_driver);
> -}
> -
> -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);
> 

Extra blank line here.

>  MODULE_DESCRIPTION("SuperH Mobile I2C Bus Controller driver");
>  MODULE_AUTHOR("Magnus Damm");

-- 
Regards,

Laurent Pinchart

Re: [RFC 1/3] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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

> > @@ -33,6 +35,9 @@
> >  #include <linux/io.h>
> >  #include <linux/slab.h>
> >  #include <linux/of_device.h>
> > +#include <linux/dmaengine.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/sh_dma.h>
> 
> I would have tried to keep the headers alphabetically sorted, if they had been 
> sorted in the first place :-)

Sure, I can do that as a seperate patch.

> > +static void sh_mobile_i2c_cleanup_dma(struct sh_mobile_i2c_data *pd)
> > +{
> > +	if (pd->dma_direction == DMA_FROM_DEVICE)
> > +		dmaengine_terminate_all(pd->dma_rx);
> > +	if (pd->dma_direction == DMA_TO_DEVICE)
> 
> else ?

Yes, why not.

> > +	pd->buf_mapped = true;
> 
> Can't you use dma_direction != DMA_NONE to detect whether the buffer has been 
> mapped, instead of adding a new field to struct sh_mobile_i2c_data ? You could 
> then simplify sh_mobile_i2c_cleanup_dma() by returning immediately at the 
> beginning of the function if dma_direction == DMA_NONE.

I thought having this explicit might be more readable. Will try your
idea though and check.

> 
> Extra blank line here.

Yes.

Thanks for the review!


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

Re: [RFC 1/3] i2c: sh_mobile: add DMA support

[Geert Uytterhoeven]

Hi Wolfram,

On Fri, Oct 31, 2014 at 11:51 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> +++ b/drivers/i2c/busses/i2c-sh_mobile.c

> +static struct dma_chan *sh_mobile_i2c_request_dma_chan(struct device *dev,
> +                                             enum dma_transfer_direction dir,
> +                                             dma_addr_t port_addr)
> +{
> +       dma_cap_mask_t mask;
> +       struct dma_chan *chan;
> +       struct dma_slave_config cfg;
> +       int ret;
> +
> +       dma_cap_zero(mask);
> +       dma_cap_set(DMA_SLAVE, mask);
> +
> +       chan = dma_request_slave_channel_compat(mask, shdma_chan_filter,
> +                               (void *)0UL, dev, dir = DMA_MEM_TO_DEV ? "tx" : "rx");

Given you only support DT DMA configuration (you always pass (void *)0UL
as the channel ID), I think you can call dma_request_slave_channel(dev,
dir = DMA_MEM_TO_DEV ? "tx" : "rx") directly instead of using the
dma_request_slave_channel_compat() wrapper.

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: [RFC 1/3] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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


> Given you only support DT DMA configuration (you always pass (void *)0UL
> as the channel ID), I think you can call dma_request_slave_channel(dev,
> dir == DMA_MEM_TO_DEV ? "tx" : "rx") directly instead of using the
> dma_request_slave_channel_compat() wrapper.

Yay, true. Thanks for the review!


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

[RFC 2/3] 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>
---
 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.0.0

Re: [RFC 2/3] ARM: shmobile: r8a7790: add DMA nodes for IIC

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Friday 31 October 2014 11:51:17 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>

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

-- 
Regards,

Laurent Pinchart

Re: [RFC 2/3] ARM: shmobile: r8a7790: add DMA nodes for IIC

[Geert Uytterhoeven]

On Fri, Oct 31, 2014 at 11:51 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

[RFC 3/3] 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>
---
 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.0.0

Re: [RFC 3/3] ARM: shmobile: r8a7791: add DMA nodes for IIC

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Friday 31 October 2014 11:51:18 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>

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

-- 
Regards,

Laurent Pinchart

Re: [RFC 3/3] ARM: shmobile: r8a7791: add DMA nodes for IIC

[Geert Uytterhoeven]

On Fri, Oct 31, 2014 at 11:51 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>

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: [RFC 0/3] i2c: sh_mobile: add DMA support

[Laurent Pinchart]

Hi Wolfram,

On Friday 31 October 2014 11:51:15 Wolfram Sang wrote:
> Here is my RFC 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!

Do we have an idea of how much power (or CPU time ?) DMA support could save ?

> 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. The major issue currently: This driver uses subsys_initcall() but
> at that time DMA is not available, and there is no deferred probe for DMA.
> So, switching to module_init() makes DMA work, but this may cause
> side-effects for older boards which rely on I2C being available early (to
> control some PMIC, for example). This needs some more investigation. Also,
> the driver (like all I2C DMA drivers currently) assumes that i2c message
> buffers are DMA capable. This is not always true and might need some
> assistance from the I2C core.

Given the amount of data we could probably use bounce buffers.

> Other than that, 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,
> 
>     Wolfram
> 
> 
> Wolfram Sang (3):
>   i2c: sh_mobile: add DMA support
>   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                 | 203 ++++++++++++++++--
>  4 files changed, 203 insertions(+), 19 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [RFC 0/3] i2c: sh_mobile: add DMA support

[Wolfram Sang]

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


> > Here is my RFC 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!
> 
> Do we have an idea of how much power (or CPU time ?) DMA support could save ?

I have some numbers from the function tracer, but I don't trust them.
According to them, DMA is always better (50us per interrupt vs. 23us for
DMA setup). This is why I say the threshold is still under test.


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