LKML Archive on lore.kernel.org
 help / Atom feed
* [PATCH v2 1/4] i2c: imx: Fix reinit_completion() use
       [not found] <20180709094304.8814-1-esben.haabendal@gmail.com>
@ 2018-07-09  9:43 ` Esben Haabendal
  2018-07-23 18:08   ` Wolfram Sang
  2018-07-09  9:43 ` [PATCH v2 2/4] i2c: imx: Fix race condition in dma read Esben Haabendal
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: Esben Haabendal @ 2018-07-09  9:43 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Uwe Kleine-König, Rob Herring, Mark Rutland,
	Yuan Yao, Esben Haabendal, Phil Reid, Lucas Stach, Linus Walleij,
	Peter Rosin, Fabio Estevam, linux-kernel

From: Esben Haabendal <eha@deif.com>

Make sure to call reinit_completion() before dma is started to avoid race
condition where reinit_completion() is called after complete() and before
wait_for_completion_timeout().

Signed-off-by: Esben Haabendal <eha@deif.com>
Fixes: ce1a78840ff7 ("i2c: imx: add DMA support for freescale i2c driver")
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/i2c/busses/i2c-imx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 0207e194f84b..39cfd98c7b23 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -368,6 +368,7 @@ static int i2c_imx_dma_xfer(struct imx_i2c_struct *i2c_imx,
 		goto err_desc;
 	}
 
+	reinit_completion(&dma->cmd_complete);
 	txdesc->callback = i2c_imx_dma_callback;
 	txdesc->callback_param = i2c_imx;
 	if (dma_submit_error(dmaengine_submit(txdesc))) {
@@ -622,7 +623,6 @@ static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
 	 * The first byte must be transmitted by the CPU.
 	 */
 	imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
-	reinit_completion(&i2c_imx->dma->cmd_complete);
 	time_left = wait_for_completion_timeout(
 				&i2c_imx->dma->cmd_complete,
 				msecs_to_jiffies(DMA_TIMEOUT));
@@ -681,7 +681,6 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
 	if (result)
 		return result;
 
-	reinit_completion(&i2c_imx->dma->cmd_complete);
 	time_left = wait_for_completion_timeout(
 				&i2c_imx->dma->cmd_complete,
 				msecs_to_jiffies(DMA_TIMEOUT));
-- 
2.18.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 2/4] i2c: imx: Fix race condition in dma read
       [not found] <20180709094304.8814-1-esben.haabendal@gmail.com>
  2018-07-09  9:43 ` [PATCH v2 1/4] i2c: imx: Fix reinit_completion() use Esben Haabendal
@ 2018-07-09  9:43 ` Esben Haabendal
  2018-07-24  7:57   ` Uwe Kleine-König
  2018-07-09  9:43 ` [PATCH v2 3/4] i2c: imx: Simplify stopped state tracking Esben Haabendal
  2018-07-09  9:43 ` [PATCH v2 4/4] arm: dts: ls1021a: Enable I2C DMA support Esben Haabendal
  3 siblings, 1 reply; 13+ messages in thread
From: Esben Haabendal @ 2018-07-09  9:43 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Uwe Kleine-König, Rob Herring, Mark Rutland,
	Yuan Yao, Esben Haabendal, Fabio Estevam, Lucas Stach, Phil Reid,
	Clemens Gruber, Peter Rosin, linux-kernel

From: Esben Haabendal <eha@deif.com>

This fixes a race condition, where the DMAEN bit ends up being set after
I2C slave has transmitted a byte following the dummy read.  When that
happens, an interrupt is generated instead, and no DMA request is generated
to kickstart the DMA read, and a timeout happens after DMA_TIMEOUT (1 sec).

Fixed by setting the DMAEN bit before the dummy read.

Signed-off-by: Esben Haabendal <eha@deif.com>
---
 drivers/i2c/busses/i2c-imx.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 39cfd98c7b23..d86f152176a4 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -668,9 +668,6 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
 	struct imx_i2c_dma *dma = i2c_imx->dma;
 	struct device *dev = &i2c_imx->adapter.dev;
 
-	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
-	temp |= I2CR_DMAEN;
-	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
 
 	dma->chan_using = dma->chan_rx;
 	dma->dma_transfer_dir = DMA_DEV_TO_MEM;
@@ -810,6 +807,11 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
 	if ((msgs->len - 1) || block_data)
 		temp &= ~I2CR_TXAK;
 	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
+	if (i2c_imx->dma && msgs->len >= DMA_THRESHOLD && !block_data) {
+		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+		temp |= I2CR_DMAEN;
+		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
+	}
 	imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); /* dummy read */
 
 	dev_dbg(&i2c_imx->adapter.dev, "<%s> read data\n", __func__);
-- 
2.18.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 3/4] i2c: imx: Simplify stopped state tracking
       [not found] <20180709094304.8814-1-esben.haabendal@gmail.com>
  2018-07-09  9:43 ` [PATCH v2 1/4] i2c: imx: Fix reinit_completion() use Esben Haabendal
  2018-07-09  9:43 ` [PATCH v2 2/4] i2c: imx: Fix race condition in dma read Esben Haabendal
@ 2018-07-09  9:43 ` Esben Haabendal
  2018-07-24  7:59   ` Uwe Kleine-König
  2018-07-09  9:43 ` [PATCH v2 4/4] arm: dts: ls1021a: Enable I2C DMA support Esben Haabendal
  3 siblings, 1 reply; 13+ messages in thread
From: Esben Haabendal @ 2018-07-09  9:43 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Uwe Kleine-König, Rob Herring, Mark Rutland,
	Yuan Yao, Esben Haabendal, Philipp Zabel, Phil Reid, Lucas Stach,
	Clemens Gruber, Peter Rosin, Fabio Estevam, linux-kernel

From: Esben Haabendal <eha@deif.com>

Always update the stopped state when busy status have been checked.
This is identical to what was done before, with the exception of error
handling.
Without this change, some errors cause the stopped state to be left in
incorrect state in i2c_imx_stop(), i2c_imx_dma_read(), i2c_imx_read() and
i2c_imx_xfer().

Signed-off-by: Esben Haabendal <eha@deif.com>
---
 drivers/i2c/busses/i2c-imx.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index d86f152176a4..1db8e6790afc 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -421,10 +421,14 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
 			return -EAGAIN;
 		}
 
-		if (for_busy && (temp & I2SR_IBB))
+		if (for_busy && (temp & I2SR_IBB)) {
+			i2c_imx->stopped = 0;
 			break;
-		if (!for_busy && !(temp & I2SR_IBB))
+		}
+		if (!for_busy && !(temp & I2SR_IBB)) {
+			i2c_imx->stopped = 1;
 			break;
+		}
 		if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
 			dev_dbg(&i2c_imx->adapter.dev,
 				"<%s> I2C bus is busy\n", __func__);
@@ -538,7 +542,6 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
 	result = i2c_imx_bus_busy(i2c_imx, 1);
 	if (result)
 		return result;
-	i2c_imx->stopped = 0;
 
 	temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
 	temp &= ~I2CR_DMAEN;
@@ -569,7 +572,6 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
 
 	if (!i2c_imx->stopped) {
 		i2c_imx_bus_busy(i2c_imx, 0);
-		i2c_imx->stopped = 1;
 	}
 
 	/* Disable I2C controller */
@@ -724,7 +726,6 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
 		temp &= ~(I2CR_MSTA | I2CR_MTX);
 		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
 		i2c_imx_bus_busy(i2c_imx, 0);
-		i2c_imx->stopped = 1;
 	} else {
 		/*
 		 * For i2c master receiver repeat restart operation like:
@@ -852,7 +853,6 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
 				temp &= ~(I2CR_MSTA | I2CR_MTX);
 				imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
 				i2c_imx_bus_busy(i2c_imx, 0);
-				i2c_imx->stopped = 1;
 			} else {
 				/*
 				 * For i2c master receiver repeat restart operation like:
-- 
2.18.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 4/4] arm: dts: ls1021a: Enable I2C DMA support
       [not found] <20180709094304.8814-1-esben.haabendal@gmail.com>
                   ` (2 preceding siblings ...)
  2018-07-09  9:43 ` [PATCH v2 3/4] i2c: imx: Simplify stopped state tracking Esben Haabendal
@ 2018-07-09  9:43 ` Esben Haabendal
  2018-07-24  8:02   ` Uwe Kleine-König
  3 siblings, 1 reply; 13+ messages in thread
From: Esben Haabendal @ 2018-07-09  9:43 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Uwe Kleine-König, Rob Herring, Mark Rutland,
	Yuan Yao, Esben Haabendal, devicetree, linux-kernel

From: Esben Haabendal <eha@deif.com>

Gives substantial performance improvement for transfers larger than 16
bytes (DMA_THRESHOLD).  Smaller transfers are unaffected.

Signed-off-by: Esben Haabendal <eha@deif.com>
---
 arch/arm/boot/dts/ls1021a.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/ls1021a.dtsi b/arch/arm/boot/dts/ls1021a.dtsi
index c55d479971cc..1e5640701c65 100644
--- a/arch/arm/boot/dts/ls1021a.dtsi
+++ b/arch/arm/boot/dts/ls1021a.dtsi
@@ -363,6 +363,8 @@
 			interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
 			clock-names = "i2c";
 			clocks = <&clockgen 4 1>;
+			dma-names = "tx", "rx";
+			dmas = <&edma0 1 39>, <&edma0 1 38>;
 			status = "disabled";
 		};
 
@@ -374,6 +376,8 @@
 			interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
 			clock-names = "i2c";
 			clocks = <&clockgen 4 1>;
+			dma-names = "tx", "rx";
+			dmas = <&edma0 1 37>, <&edma0 1 36>;
 			status = "disabled";
 		};
 
@@ -385,6 +389,8 @@
 			interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
 			clock-names = "i2c";
 			clocks = <&clockgen 4 1>;
+			dma-names = "tx", "rx";
+			dmas = <&edma0 1 35>, <&edma0 1 34>;
 			status = "disabled";
 		};
 
-- 
2.18.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/4] i2c: imx: Fix reinit_completion() use
  2018-07-09  9:43 ` [PATCH v2 1/4] i2c: imx: Fix reinit_completion() use Esben Haabendal
@ 2018-07-23 18:08   ` Wolfram Sang
  0 siblings, 0 replies; 13+ messages in thread
From: Wolfram Sang @ 2018-07-23 18:08 UTC (permalink / raw)
  To: Esben Haabendal
  Cc: linux-i2c, Wolfram Sang, Uwe Kleine-König, Rob Herring,
	Mark Rutland, Yuan Yao, Esben Haabendal, Phil Reid, Lucas Stach,
	Linus Walleij, Peter Rosin, Fabio Estevam, linux-kernel

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

On Mon, Jul 09, 2018 at 11:43:01AM +0200, Esben Haabendal wrote:
> From: Esben Haabendal <eha@deif.com>
> 
> Make sure to call reinit_completion() before dma is started to avoid race
> condition where reinit_completion() is called after complete() and before
> wait_for_completion_timeout().
> 
> Signed-off-by: Esben Haabendal <eha@deif.com>
> Fixes: ce1a78840ff7 ("i2c: imx: add DMA support for freescale i2c driver")
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Applied to for-current and added stable, thanks!

Uwe, do you have comments for the other patches, too?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] i2c: imx: Fix race condition in dma read
  2018-07-09  9:43 ` [PATCH v2 2/4] i2c: imx: Fix race condition in dma read Esben Haabendal
@ 2018-07-24  7:57   ` Uwe Kleine-König
  2018-08-09 11:57     ` Esben Haabendal
  0 siblings, 1 reply; 13+ messages in thread
From: Uwe Kleine-König @ 2018-07-24  7:57 UTC (permalink / raw)
  To: Esben Haabendal
  Cc: linux-i2c, Wolfram Sang, Rob Herring, Mark Rutland, Yuan Yao,
	Esben Haabendal, Fabio Estevam, Lucas Stach, Phil Reid,
	Clemens Gruber, Peter Rosin, linux-kernel

On Mon, Jul 09, 2018 at 11:43:02AM +0200, Esben Haabendal wrote:
> From: Esben Haabendal <eha@deif.com>
> 
> This fixes a race condition, where the DMAEN bit ends up being set after
> I2C slave has transmitted a byte following the dummy read.  When that
> happens, an interrupt is generated instead, and no DMA request is generated
> to kickstart the DMA read, and a timeout happens after DMA_TIMEOUT (1 sec).
> 
> Fixed by setting the DMAEN bit before the dummy read.
> 
> Signed-off-by: Esben Haabendal <eha@deif.com>
> ---
>  drivers/i2c/busses/i2c-imx.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 39cfd98c7b23..d86f152176a4 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -668,9 +668,6 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
>  	struct imx_i2c_dma *dma = i2c_imx->dma;
>  	struct device *dev = &i2c_imx->adapter.dev;
>  
> -	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> -	temp |= I2CR_DMAEN;
> -	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
>  
>  	dma->chan_using = dma->chan_rx;
>  	dma->dma_transfer_dir = DMA_DEV_TO_MEM;
> @@ -810,6 +807,11 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
>  	if ((msgs->len - 1) || block_data)
>  		temp &= ~I2CR_TXAK;
>  	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +	if (i2c_imx->dma && msgs->len >= DMA_THRESHOLD && !block_data) {
> +		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +		temp |= I2CR_DMAEN;
> +		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +	}

Does this need to be a separate write to the I2CR register? Just before
the if there is temp written to this register, so probably this can be
changed to just:

	if (i2c_imx->dma && msgs->len >= DMA_THRESHOLD && !block_data)
		temp |= I2CR_DMAEN;

when moved before up one line.

I don't find documentation for the LS processors where this
register is described though (and the imx family doesn't seem to support
DMA for i2c).

Other than that this looks reasonable and warrants a

	Fixes: ce1a78840ff7 ("i2c: imx: add DMA support for freescale i2c driver")

.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/4] i2c: imx: Simplify stopped state tracking
  2018-07-09  9:43 ` [PATCH v2 3/4] i2c: imx: Simplify stopped state tracking Esben Haabendal
@ 2018-07-24  7:59   ` Uwe Kleine-König
  2018-08-09 12:06     ` Esben Haabendal
  0 siblings, 1 reply; 13+ messages in thread
From: Uwe Kleine-König @ 2018-07-24  7:59 UTC (permalink / raw)
  To: Esben Haabendal
  Cc: linux-i2c, Wolfram Sang, Rob Herring, Mark Rutland, Yuan Yao,
	Esben Haabendal, Philipp Zabel, Phil Reid, Lucas Stach,
	Clemens Gruber, Peter Rosin, Fabio Estevam, linux-kernel

On Mon, Jul 09, 2018 at 11:43:03AM +0200, Esben Haabendal wrote:
> From: Esben Haabendal <eha@deif.com>
> 
> Always update the stopped state when busy status have been checked.
> This is identical to what was done before, with the exception of error
> handling.
> Without this change, some errors cause the stopped state to be left in
> incorrect state in i2c_imx_stop(), i2c_imx_dma_read(), i2c_imx_read() and
> i2c_imx_xfer().
> 
> Signed-off-by: Esben Haabendal <eha@deif.com>
> ---
>  drivers/i2c/busses/i2c-imx.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index d86f152176a4..1db8e6790afc 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -421,10 +421,14 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
>  			return -EAGAIN;
>  		}
>  
> -		if (for_busy && (temp & I2SR_IBB))
> +		if (for_busy && (temp & I2SR_IBB)) {
> +			i2c_imx->stopped = 0;
>  			break;
> -		if (!for_busy && !(temp & I2SR_IBB))
> +		}
> +		if (!for_busy && !(temp & I2SR_IBB)) {
> +			i2c_imx->stopped = 1;
>  			break;
> +		}

Would it make sense to assign to ->stopped independent of for_busy?

>  		if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
>  			dev_dbg(&i2c_imx->adapter.dev,
>  				"<%s> I2C bus is busy\n", __func__);
> @@ -538,7 +542,6 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
>  	result = i2c_imx_bus_busy(i2c_imx, 1);
>  	if (result)
>  		return result;
> -	i2c_imx->stopped = 0;
>  
>  	temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
>  	temp &= ~I2CR_DMAEN;
> @@ -569,7 +572,6 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
>  
>  	if (!i2c_imx->stopped) {
>  		i2c_imx_bus_busy(i2c_imx, 0);
> -		i2c_imx->stopped = 1;
>  	}

The braces can go away here.
>  
>  	/* Disable I2C controller */

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 4/4] arm: dts: ls1021a: Enable I2C DMA support
  2018-07-09  9:43 ` [PATCH v2 4/4] arm: dts: ls1021a: Enable I2C DMA support Esben Haabendal
@ 2018-07-24  8:02   ` Uwe Kleine-König
  0 siblings, 0 replies; 13+ messages in thread
From: Uwe Kleine-König @ 2018-07-24  8:02 UTC (permalink / raw)
  To: Esben Haabendal
  Cc: linux-i2c, Wolfram Sang, Rob Herring, Mark Rutland, Yuan Yao,
	Esben Haabendal, devicetree, linux-kernel, Shawn Guo, kernel,
	NXP Linux Team

Hello,

expanding Cc a bit to include Shawn and the kernel teams of Pengutronix
and NXP.

On Mon, Jul 09, 2018 at 11:43:04AM +0200, Esben Haabendal wrote:
> From: Esben Haabendal <eha@deif.com>
> 
> Gives substantial performance improvement for transfers larger than 16
> bytes (DMA_THRESHOLD).  Smaller transfers are unaffected.
> 
> Signed-off-by: Esben Haabendal <eha@deif.com>
> ---
>  arch/arm/boot/dts/ls1021a.dtsi | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/ls1021a.dtsi b/arch/arm/boot/dts/ls1021a.dtsi
> index c55d479971cc..1e5640701c65 100644
> --- a/arch/arm/boot/dts/ls1021a.dtsi
> +++ b/arch/arm/boot/dts/ls1021a.dtsi
> @@ -363,6 +363,8 @@
>  			interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
>  			clock-names = "i2c";
>  			clocks = <&clockgen 4 1>;
> +			dma-names = "tx", "rx";
> +			dmas = <&edma0 1 39>, <&edma0 1 38>;
>  			status = "disabled";
>  		};
>  
> @@ -374,6 +376,8 @@
>  			interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
>  			clock-names = "i2c";
>  			clocks = <&clockgen 4 1>;
> +			dma-names = "tx", "rx";
> +			dmas = <&edma0 1 37>, <&edma0 1 36>;
>  			status = "disabled";
>  		};
>  
> @@ -385,6 +389,8 @@
>  			interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
>  			clock-names = "i2c";
>  			clocks = <&clockgen 4 1>;
> +			dma-names = "tx", "rx";
> +			dmas = <&edma0 1 35>, <&edma0 1 34>;
>  			status = "disabled";
>  		};

This is a bit orthogonal to the other changes in this series and needs
at least an ack from Shawn. Looks ok in my eyes.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] i2c: imx: Fix race condition in dma read
  2018-07-24  7:57   ` Uwe Kleine-König
@ 2018-08-09 11:57     ` Esben Haabendal
  0 siblings, 0 replies; 13+ messages in thread
From: Esben Haabendal @ 2018-08-09 11:57 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: linux-i2c, Wolfram Sang, Rob Herring, Mark Rutland, Yuan Yao,
	Fabio Estevam, Lucas Stach, Phil Reid, Clemens Gruber,
	Peter Rosin, linux-kernel

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:

> On Mon, Jul 09, 2018 at 11:43:02AM +0200, Esben Haabendal wrote:
>> From: Esben Haabendal <eha@deif.com>
>> 
>> This fixes a race condition, where the DMAEN bit ends up being set after
>> I2C slave has transmitted a byte following the dummy read.  When that
>> happens, an interrupt is generated instead, and no DMA request is generated
>> to kickstart the DMA read, and a timeout happens after DMA_TIMEOUT (1 sec).
>> 
>> Fixed by setting the DMAEN bit before the dummy read.
>> 
>> Signed-off-by: Esben Haabendal <eha@deif.com>
>> ---
>>  drivers/i2c/busses/i2c-imx.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
>> index 39cfd98c7b23..d86f152176a4 100644
>> --- a/drivers/i2c/busses/i2c-imx.c
>> +++ b/drivers/i2c/busses/i2c-imx.c
>> @@ -668,9 +668,6 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
>>  	struct imx_i2c_dma *dma = i2c_imx->dma;
>>  	struct device *dev = &i2c_imx->adapter.dev;
>>  
>> -	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>> -	temp |= I2CR_DMAEN;
>> -	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
>>  
>>  	dma->chan_using = dma->chan_rx;
>>  	dma->dma_transfer_dir = DMA_DEV_TO_MEM;
>> @@ -810,6 +807,11 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
>>  	if ((msgs->len - 1) || block_data)
>>  		temp &= ~I2CR_TXAK;
>>  	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
>> +	if (i2c_imx->dma && msgs->len >= DMA_THRESHOLD && !block_data) {
>> +		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>> +		temp |= I2CR_DMAEN;
>> +		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
>> +	}
>
> Does this need to be a separate write to the I2CR register? Just before
> the if there is temp written to this register, so probably this can be
> changed to just:
>
> 	if (i2c_imx->dma && msgs->len >= DMA_THRESHOLD && !block_data)
> 		temp |= I2CR_DMAEN;
>
> when moved before up one line.

Sure, that is clearly better.

> I don't find documentation for the LS processors where this
> register is described though (and the imx family doesn't seem to support
> DMA for i2c).

Yes, unfortunately, I think the LS1021A reference manual requires NDA.

> Other than that this looks reasonable and warrants a
>
> 	Fixes: ce1a78840ff7 ("i2c: imx: add DMA support for freescale i2c driver")

I will add that.

/Esben

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/4] i2c: imx: Simplify stopped state tracking
  2018-07-24  7:59   ` Uwe Kleine-König
@ 2018-08-09 12:06     ` Esben Haabendal
  2018-08-09 16:26       ` Uwe Kleine-König
  0 siblings, 1 reply; 13+ messages in thread
From: Esben Haabendal @ 2018-08-09 12:06 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: linux-i2c, Wolfram Sang, Rob Herring, Mark Rutland, Yuan Yao,
	Philipp Zabel, Phil Reid, Lucas Stach, Clemens Gruber,
	Peter Rosin, Fabio Estevam, linux-kernel

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:

> On Mon, Jul 09, 2018 at 11:43:03AM +0200, Esben Haabendal wrote:
>> From: Esben Haabendal <eha@deif.com>
>> 
>> Always update the stopped state when busy status have been checked.
>> This is identical to what was done before, with the exception of error
>> handling.
>> Without this change, some errors cause the stopped state to be left in
>> incorrect state in i2c_imx_stop(), i2c_imx_dma_read(), i2c_imx_read() and
>> i2c_imx_xfer().
>> 
>> Signed-off-by: Esben Haabendal <eha@deif.com>
>> ---
>>  drivers/i2c/busses/i2c-imx.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
>> index d86f152176a4..1db8e6790afc 100644
>> --- a/drivers/i2c/busses/i2c-imx.c
>> +++ b/drivers/i2c/busses/i2c-imx.c
>> @@ -421,10 +421,14 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
>>  			return -EAGAIN;
>>  		}
>>  
>> -		if (for_busy && (temp & I2SR_IBB))
>> +		if (for_busy && (temp & I2SR_IBB)) {
>> +			i2c_imx->stopped = 0;
>>  			break;
>> -		if (!for_busy && !(temp & I2SR_IBB))
>> +		}
>> +		if (!for_busy && !(temp & I2SR_IBB)) {
>> +			i2c_imx->stopped = 1;
>>  			break;
>> +		}
>
> Would it make sense to assign to ->stopped independent of for_busy?

What do you mean?

Assigning to ->stopped on each check for I2SR_IBB in loop, independent
of the for_busy argument?  I don't think so.  The additional assignments
would be to the same value as it is set to already.

>>  		if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
>>  			dev_dbg(&i2c_imx->adapter.dev,
>>  				"<%s> I2C bus is busy\n", __func__);
>> @@ -538,7 +542,6 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
>>  	result = i2c_imx_bus_busy(i2c_imx, 1);
>>  	if (result)
>>  		return result;
>> -	i2c_imx->stopped = 0;
>>  
>>  	temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
>>  	temp &= ~I2CR_DMAEN;
>> @@ -569,7 +572,6 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
>>  
>>  	if (!i2c_imx->stopped) {
>>  		i2c_imx_bus_busy(i2c_imx, 0);
>> -		i2c_imx->stopped = 1;
>>  	}
>
> The braces can go away here.

Will do.

/Esben

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/4] i2c: imx: Simplify stopped state tracking
  2018-08-09 12:06     ` Esben Haabendal
@ 2018-08-09 16:26       ` Uwe Kleine-König
  2018-08-10  9:25         ` Esben Haabendal
  0 siblings, 1 reply; 13+ messages in thread
From: Uwe Kleine-König @ 2018-08-09 16:26 UTC (permalink / raw)
  To: Esben Haabendal
  Cc: linux-i2c, Wolfram Sang, Rob Herring, Mark Rutland, Yuan Yao,
	Philipp Zabel, Phil Reid, Lucas Stach, Clemens Gruber,
	Peter Rosin, Fabio Estevam, linux-kernel

On Thu, Aug 09, 2018 at 02:06:43PM +0200, Esben Haabendal wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> 
> > On Mon, Jul 09, 2018 at 11:43:03AM +0200, Esben Haabendal wrote:
> >> From: Esben Haabendal <eha@deif.com>
> >> 
> >> Always update the stopped state when busy status have been checked.
> >> This is identical to what was done before, with the exception of error
> >> handling.
> >> Without this change, some errors cause the stopped state to be left in
> >> incorrect state in i2c_imx_stop(), i2c_imx_dma_read(), i2c_imx_read() and
> >> i2c_imx_xfer().
> >> 
> >> Signed-off-by: Esben Haabendal <eha@deif.com>
> >> ---
> >>  drivers/i2c/busses/i2c-imx.c | 12 ++++++------
> >>  1 file changed, 6 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> >> index d86f152176a4..1db8e6790afc 100644
> >> --- a/drivers/i2c/busses/i2c-imx.c
> >> +++ b/drivers/i2c/busses/i2c-imx.c
> >> @@ -421,10 +421,14 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
> >>  			return -EAGAIN;
> >>  		}
> >>  
> >> -		if (for_busy && (temp & I2SR_IBB))
> >> +		if (for_busy && (temp & I2SR_IBB)) {
> >> +			i2c_imx->stopped = 0;
> >>  			break;
> >> -		if (!for_busy && !(temp & I2SR_IBB))
> >> +		}
> >> +		if (!for_busy && !(temp & I2SR_IBB)) {
> >> +			i2c_imx->stopped = 1;
> >>  			break;
> >> +		}
> >
> > Would it make sense to assign to ->stopped independent of for_busy?
> 
> What do you mean?
> 
> Assigning to ->stopped on each check for I2SR_IBB in loop, independent
> of the for_busy argument?  I don't think so.  The additional assignments
> would be to the same value as it is set to already.

Currently you have:

	if (for_busy && (temp & I2SR_IBB)) {
		i2c_imx->stopped = 0;
		break;
	}

	if (!for_busy && !(temp & I2SR_IBB)) {
		i2c_imx->stopped = 1;
		break;
	}

The semantic of this is the same (apart from always updating .stopped)
but is imho easier:

	i2c_imx->stopped = !(temp & I2SR_IBB);

	if (for_busy != i2c_imx->stopped)
		break;

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/4] i2c: imx: Simplify stopped state tracking
  2018-08-09 16:26       ` Uwe Kleine-König
@ 2018-08-10  9:25         ` Esben Haabendal
  2018-08-10 12:25           ` Uwe Kleine-König
  0 siblings, 1 reply; 13+ messages in thread
From: Esben Haabendal @ 2018-08-10  9:25 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: linux-i2c, Wolfram Sang, Rob Herring, Mark Rutland, Yuan Yao,
	Philipp Zabel, Phil Reid, Lucas Stach, Clemens Gruber,
	Peter Rosin, Fabio Estevam, linux-kernel

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:

> On Thu, Aug 09, 2018 at 02:06:43PM +0200, Esben Haabendal wrote:
>> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> 
>> > On Mon, Jul 09, 2018 at 11:43:03AM +0200, Esben Haabendal wrote:
>> >> From: Esben Haabendal <eha@deif.com>
>> >> 
>> >> Always update the stopped state when busy status have been checked.
>> >> This is identical to what was done before, with the exception of error
>> >> handling.
>> >> Without this change, some errors cause the stopped state to be left in
>> >> incorrect state in i2c_imx_stop(), i2c_imx_dma_read(), i2c_imx_read() and
>> >> i2c_imx_xfer().
>> >> 
>> >> Signed-off-by: Esben Haabendal <eha@deif.com>
>> >> ---
>> >>  drivers/i2c/busses/i2c-imx.c | 12 ++++++------
>> >>  1 file changed, 6 insertions(+), 6 deletions(-)
>> >> 
>> >> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
>> >> index d86f152176a4..1db8e6790afc 100644
>> >> --- a/drivers/i2c/busses/i2c-imx.c
>> >> +++ b/drivers/i2c/busses/i2c-imx.c
>> >> @@ -421,10 +421,14 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
>> >>  			return -EAGAIN;
>> >>  		}
>> >>  
>> >> -		if (for_busy && (temp & I2SR_IBB))
>> >> +		if (for_busy && (temp & I2SR_IBB)) {
>> >> +			i2c_imx->stopped = 0;
>> >>  			break;
>> >> -		if (!for_busy && !(temp & I2SR_IBB))
>> >> +		}
>> >> +		if (!for_busy && !(temp & I2SR_IBB)) {
>> >> +			i2c_imx->stopped = 1;
>> >>  			break;
>> >> +		}
>> >
>> > Would it make sense to assign to ->stopped independent of for_busy?
>> 
>> What do you mean?
>> 
>> Assigning to ->stopped on each check for I2SR_IBB in loop, independent
>> of the for_busy argument?  I don't think so.  The additional assignments
>> would be to the same value as it is set to already.
>
> Currently you have:
>
> 	if (for_busy && (temp & I2SR_IBB)) {
> 		i2c_imx->stopped = 0;
> 		break;
> 	}
>
> 	if (!for_busy && !(temp & I2SR_IBB)) {
> 		i2c_imx->stopped = 1;
> 		break;
> 	}
>
> The semantic of this is the same (apart from always updating .stopped)
> but is imho easier:
>
> 	i2c_imx->stopped = !(temp & I2SR_IBB);
>
> 	if (for_busy != i2c_imx->stopped)
> 		break;

Yes, that should work also.
Shorter, but IMHO a bit more convoluted to read.
Let me know if I should send a new version with this change.

/Esben

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/4] i2c: imx: Simplify stopped state tracking
  2018-08-10  9:25         ` Esben Haabendal
@ 2018-08-10 12:25           ` Uwe Kleine-König
  0 siblings, 0 replies; 13+ messages in thread
From: Uwe Kleine-König @ 2018-08-10 12:25 UTC (permalink / raw)
  To: Esben Haabendal
  Cc: linux-i2c, Wolfram Sang, Rob Herring, Mark Rutland, Yuan Yao,
	Philipp Zabel, Phil Reid, Lucas Stach, Clemens Gruber,
	Peter Rosin, Fabio Estevam, linux-kernel

Hello Esben,

On Fri, Aug 10, 2018 at 11:25:34AM +0200, Esben Haabendal wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> > Currently you have:
> >
> > 	if (for_busy && (temp & I2SR_IBB)) {
> > 		i2c_imx->stopped = 0;
> > 		break;
> > 	}
> >
> > 	if (!for_busy && !(temp & I2SR_IBB)) {
> > 		i2c_imx->stopped = 1;
> > 		break;
> > 	}
> >
> > The semantic of this is the same (apart from always updating .stopped)
> > but is imho easier:
> >
> > 	i2c_imx->stopped = !(temp & I2SR_IBB);
> >
> > 	if (for_busy != i2c_imx->stopped)
> > 		break;
> 
> Yes, that should work also.
> Shorter, but IMHO a bit more convoluted to read.
> Let me know if I should send a new version with this change.

unless someone else chimes in I'd say keep it as is. I'd prefer my
variant, but I accept that this is something subjective.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20180709094304.8814-1-esben.haabendal@gmail.com>
2018-07-09  9:43 ` [PATCH v2 1/4] i2c: imx: Fix reinit_completion() use Esben Haabendal
2018-07-23 18:08   ` Wolfram Sang
2018-07-09  9:43 ` [PATCH v2 2/4] i2c: imx: Fix race condition in dma read Esben Haabendal
2018-07-24  7:57   ` Uwe Kleine-König
2018-08-09 11:57     ` Esben Haabendal
2018-07-09  9:43 ` [PATCH v2 3/4] i2c: imx: Simplify stopped state tracking Esben Haabendal
2018-07-24  7:59   ` Uwe Kleine-König
2018-08-09 12:06     ` Esben Haabendal
2018-08-09 16:26       ` Uwe Kleine-König
2018-08-10  9:25         ` Esben Haabendal
2018-08-10 12:25           ` Uwe Kleine-König
2018-07-09  9:43 ` [PATCH v2 4/4] arm: dts: ls1021a: Enable I2C DMA support Esben Haabendal
2018-07-24  8:02   ` Uwe Kleine-König

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox