Re: [PATCH v2 2/4] i2c: imx: Fix race condition in dma read
Uwe Kleine-König writes:
> On Mon, Jul 09, 2018 at 11:43:02AM +0200, Esben Haabendal wrote:
>> From: Esben Haabendal
>>
>> 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
>> ---
>> 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 = _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
Re: [PATCH v2 2/4] i2c: imx: Fix race condition in dma read
Uwe Kleine-König writes:
> On Mon, Jul 09, 2018 at 11:43:02AM +0200, Esben Haabendal wrote:
>> From: Esben Haabendal
>>
>> 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
>> ---
>> 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 = _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
Re: [PATCH v2 2/4] i2c: imx: Fix race condition in dma read
On Mon, Jul 09, 2018 at 11:43:02AM +0200, Esben Haabendal wrote:
> From: Esben Haabendal
>
> 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
> ---
> 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 = _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/ |
Re: [PATCH v2 2/4] i2c: imx: Fix race condition in dma read
On Mon, Jul 09, 2018 at 11:43:02AM +0200, Esben Haabendal wrote:
> From: Esben Haabendal
>
> 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
> ---
> 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 = _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/ |
[PATCH v2 2/4] i2c: imx: Fix race condition in dma read
From: Esben Haabendal
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
---
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 = _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(_imx->adapter.dev, "<%s> read data\n", __func__);
--
2.18.0
[PATCH v2 2/4] i2c: imx: Fix race condition in dma read
From: Esben Haabendal
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
---
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 = _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(_imx->adapter.dev, "<%s> read data\n", __func__);
--
2.18.0
