* [PATCH v3 1/3] i2c: imx: Fix race condition in dma read
[not found] <20180809123207.9732-1-esben.haabendal@gmail.com>
@ 2018-08-09 12:32 ` Esben Haabendal
2018-08-16 8:27 ` Uwe Kleine-König
2018-08-09 12:32 ` [PATCH v3 2/3] i2c: imx: Simplify stopped state tracking Esben Haabendal
2018-08-09 12:32 ` [PATCH v3 3/3] arm: dts: ls1021a: Enable I2C DMA support Esben Haabendal
2 siblings, 1 reply; 6+ messages in thread
From: Esben Haabendal @ 2018-08-09 12:32 UTC (permalink / raw)
To: linux-i2c
Cc: Esben Haabendal, Wolfram Sang, Uwe Kleine-König,
Lucas Stach, Linus Walleij, Peter Rosin, Wei Jinhua, Phil Reid,
Fabio Estevam, 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 | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 498c5e891649..f7bd6c21da27 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;
@@ -809,6 +806,8 @@ 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;
+ if (i2c_imx->dma && msgs->len >= DMA_THRESHOLD && !block_data)
+ temp |= I2CR_DMAEN;
imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); /* dummy read */
--
2.18.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v3 1/3] i2c: imx: Fix race condition in dma read
2018-08-09 12:32 ` [PATCH v3 1/3] i2c: imx: Fix race condition in dma read Esben Haabendal
@ 2018-08-16 8:27 ` Uwe Kleine-König
0 siblings, 0 replies; 6+ messages in thread
From: Uwe Kleine-König @ 2018-08-16 8:27 UTC (permalink / raw)
To: Esben Haabendal
Cc: linux-i2c, Esben Haabendal, Wolfram Sang, Lucas Stach,
Linus Walleij, Peter Rosin, Wei Jinhua, Phil Reid, Fabio Estevam,
linux-kernel
On Thu, Aug 09, 2018 at 02:32:05PM +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 | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 498c5e891649..f7bd6c21da27 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;
> @@ -809,6 +806,8 @@ 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;
> + if (i2c_imx->dma && msgs->len >= DMA_THRESHOLD && !block_data)
> + temp |= I2CR_DMAEN;
Instead of duplicating the if condition I'd do:
int use_dma = i2c_imx->dma && msgs->len >= DMA_THRESHOLD && !block_data;
and then simplify the two conditions to just "use_dma".
With this changed you can add my Ack.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 2/3] i2c: imx: Simplify stopped state tracking
[not found] <20180809123207.9732-1-esben.haabendal@gmail.com>
2018-08-09 12:32 ` [PATCH v3 1/3] i2c: imx: Fix race condition in dma read Esben Haabendal
@ 2018-08-09 12:32 ` Esben Haabendal
2018-08-16 8:23 ` Uwe Kleine-König
2018-08-09 12:32 ` [PATCH v3 3/3] arm: dts: ls1021a: Enable I2C DMA support Esben Haabendal
2 siblings, 1 reply; 6+ messages in thread
From: Esben Haabendal @ 2018-08-09 12:32 UTC (permalink / raw)
To: linux-i2c
Cc: Esben Haabendal, Wolfram Sang, Uwe Kleine-König,
Lucas Stach, Fabio Estevam, Wei Jinhua, Phil Reid, Peter Rosin,
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 | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index f7bd6c21da27..8fb61691e226 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;
@@ -567,10 +570,8 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
udelay(i2c_imx->disable_delay);
}
- if (!i2c_imx->stopped) {
+ if (!i2c_imx->stopped)
i2c_imx_bus_busy(i2c_imx, 0);
- i2c_imx->stopped = 1;
- }
/* Disable I2C controller */
temp = i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN,
@@ -724,7 +725,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:
@@ -849,7 +849,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 related [flat|nested] 6+ messages in thread* Re: [PATCH v3 2/3] i2c: imx: Simplify stopped state tracking
2018-08-09 12:32 ` [PATCH v3 2/3] i2c: imx: Simplify stopped state tracking Esben Haabendal
@ 2018-08-16 8:23 ` Uwe Kleine-König
0 siblings, 0 replies; 6+ messages in thread
From: Uwe Kleine-König @ 2018-08-16 8:23 UTC (permalink / raw)
To: Esben Haabendal
Cc: linux-i2c, Esben Haabendal, Wolfram Sang, Lucas Stach,
Fabio Estevam, Wei Jinhua, Phil Reid, Peter Rosin, linux-kernel
On Thu, Aug 09, 2018 at 02:32:06PM +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>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 3/3] arm: dts: ls1021a: Enable I2C DMA support
[not found] <20180809123207.9732-1-esben.haabendal@gmail.com>
2018-08-09 12:32 ` [PATCH v3 1/3] i2c: imx: Fix race condition in dma read Esben Haabendal
2018-08-09 12:32 ` [PATCH v3 2/3] i2c: imx: Simplify stopped state tracking Esben Haabendal
@ 2018-08-09 12:32 ` Esben Haabendal
2018-08-27 6:51 ` Shawn Guo
2 siblings, 1 reply; 6+ messages in thread
From: Esben Haabendal @ 2018-08-09 12:32 UTC (permalink / raw)
To: linux-i2c
Cc: Esben Haabendal, Shawn Guo, kernel, NXP Linux Team, Rob Herring,
Mark Rutland, 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 related [flat|nested] 6+ messages in thread* Re: [PATCH v3 3/3] arm: dts: ls1021a: Enable I2C DMA support
2018-08-09 12:32 ` [PATCH v3 3/3] arm: dts: ls1021a: Enable I2C DMA support Esben Haabendal
@ 2018-08-27 6:51 ` Shawn Guo
0 siblings, 0 replies; 6+ messages in thread
From: Shawn Guo @ 2018-08-27 6:51 UTC (permalink / raw)
To: Esben Haabendal
Cc: linux-i2c, Esben Haabendal, kernel, NXP Linux Team, Rob Herring,
Mark Rutland, devicetree, linux-kernel
On Thu, Aug 09, 2018 at 02:32:07PM +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>
Applied, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread