Re: [PATCH 1/3] i2c: Revert "i2c: xiic: Do not reset controller before every transfer"

[Wolfram Sang]

> I think as of now since read is also impacted we can revert it.

I read this as Acked-by.

Applied to for-current, thanks!



signature.asc
Description: Digital signature

Re: [PATCH 1/3] i2c: Revert "i2c: xiic: Do not reset controller before every transfer"

[Lars-Peter Clausen]

On 11/17/2015 03:28 PM, Shubhrajyoti Datta wrote:
> On Tue, Nov 17, 2015 at 1:04 PM, Lars-Peter Clausen  wrote:
>> On 11/17/2015 06:17 AM, Shubhrajyoti Datta wrote:
>>> On Mon, Nov 16, 2015 at 7:12 PM, Lars-Peter Clausen  wrote:
 Commit d701667bb331 ("i2c: xiic: Do not reset controller before every
 transfer") removed the reinitialization of the controller before the start
 of each transfer. Apparently this change is not safe to make and the commit
 results in random I2C bus failures.
>>>
>>> Which is the platform and the ip version that you  saw the issue.
>>> Did you see the issue with read and write as  well?
>>
>> The IP version is the axi-iic v2.0 Revision 8. I've tested this on a few
>> platforms, custom ones and standard ones and I could reproduce it on most.
>> One of them was on the ZED board. The one where I couldn't reproduce it was
>> the ZC706. But that doesn't necessarily mean it doesn't happen there, just
>> that it is not triggered by the testcase.
> All the boards having the same version of the ip is what I have understood.
> 
> Thanks for the info I will try to  reproduce the issue.
> 
>>
>> The problem is that it is random corruption,
> Of registers?

To be honest I don't know if there is corruption during I2C write transfers,
but there is definitely corruption during read transactions.

- Lars

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] i2c: Revert "i2c: xiic: Do not reset controller before every transfer"

[Shubhrajyoti Datta]

On Wed, Nov 18, 2015 at 3:31 PM, Lars-Peter Clausen  wrote:
> On 11/17/2015 03:28 PM, Shubhrajyoti Datta wrote:
>> On Tue, Nov 17, 2015 at 1:04 PM, Lars-Peter Clausen  wrote:
>>> On 11/17/2015 06:17 AM, Shubhrajyoti Datta wrote:
 On Mon, Nov 16, 2015 at 7:12 PM, Lars-Peter Clausen  
 wrote:
> Commit d701667bb331 ("i2c: xiic: Do not reset controller before every
> transfer") removed the reinitialization of the controller before the start
> of each transfer. Apparently this change is not safe to make and the 
> commit
> results in random I2C bus failures.

 Which is the platform and the ip version that you  saw the issue.
 Did you see the issue with read and write as  well?
>>>
>>> The IP version is the axi-iic v2.0 Revision 8. I've tested this on a few
>>> platforms, custom ones and standard ones and I could reproduce it on most.
>>> One of them was on the ZED board. The one where I couldn't reproduce it was
>>> the ZC706. But that doesn't necessarily mean it doesn't happen there, just
>>> that it is not triggered by the testcase.
>> All the boards having the same version of the ip is what I have understood.
>>
>> Thanks for the info I will try to  reproduce the issue.
>>
>>>
>>> The problem is that it is random corruption,
>> Of registers?
>
> To be honest I don't know if there is corruption during I2C write transfers,
> but there is definitely corruption during read transactions.
Ok

Actually  I was thinking it is only an issue with



/* dynamic mode seem to suffer from problems if we just flushes
* fifos and the next message is a TX with len 0 (only addr)
* reset the IP instead of just flush fifos
*/
xiic_reinit(i2c);
<\quote>

I think as of now since read is also impacted we can revert it.


> - Lars
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] i2c: Revert "i2c: xiic: Do not reset controller before every transfer"

[Shubhrajyoti Datta]

On Tue, Nov 17, 2015 at 1:04 PM, Lars-Peter Clausen  wrote:
> On 11/17/2015 06:17 AM, Shubhrajyoti Datta wrote:
>> On Mon, Nov 16, 2015 at 7:12 PM, Lars-Peter Clausen  wrote:
>>> Commit d701667bb331 ("i2c: xiic: Do not reset controller before every
>>> transfer") removed the reinitialization of the controller before the start
>>> of each transfer. Apparently this change is not safe to make and the commit
>>> results in random I2C bus failures.
>>
>> Which is the platform and the ip version that you  saw the issue.
>> Did you see the issue with read and write as  well?
>
> The IP version is the axi-iic v2.0 Revision 8. I've tested this on a few
> platforms, custom ones and standard ones and I could reproduce it on most.
> One of them was on the ZED board. The one where I couldn't reproduce it was
> the ZC706. But that doesn't necessarily mean it doesn't happen there, just
> that it is not triggered by the testcase.
All the boards having the same version of the ip is what I have understood.

Thanks for the info I will try to  reproduce the issue.

>
> The problem is that it is random corruption,
Of registers?

> so some I2C devices might start
> to behave strangely at some point. The only good more or less reliable way
> to reproduce it that I found was to run i2cdetect a couple of times and at
> least one of them will produce strange behavior.
>
I will try to reproduce the issue at my end thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] i2c: Revert "i2c: xiic: Do not reset controller before every transfer"

[Shubhrajyoti Datta]

On Mon, Nov 16, 2015 at 7:12 PM, Lars-Peter Clausen  wrote:
> Commit d701667bb331 ("i2c: xiic: Do not reset controller before every
> transfer") removed the reinitialization of the controller before the start
> of each transfer. Apparently this change is not safe to make and the commit
> results in random I2C bus failures.

Which is the platform and the ip version that you  saw the issue.
Did you see the issue with read and write as  well?
.

>
> An easy way to trigger the issue is to run i2cdetect.
>
> Without the patch applied:
>  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
>  00:  -- -- -- -- -- -- -- -- -- -- -- -- --
>  10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>  20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>  30: -- -- -- -- -- -- -- -- UU UU -- UU 3c -- -- UU
>  40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>  50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>  60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>  70: -- -- -- -- -- -- -- --
>
> With the patch applied every other or so invocation:
>  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
>  00:  03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f
>  10: 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f
>  20: 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f
>  30: -- -- -- -- -- -- -- -- UU UU -- UU 3c -- -- UU
>  40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>  50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>  60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>  70: -- -- -- -- -- -- -- --
>
I assume that you have these many peripherals.
on the bus am I right?


> So revert the commit for now.
>
> Fixes: d701667bb331 ("i2c: xiic: Do not reset controller before every 
> transfer")
> Signed-off-by: Lars-Peter Clausen 
> ---
>  drivers/i2c/busses/i2c-xiic.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
> index e23a7b0..705cf69 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -662,6 +662,9 @@ static void __xiic_start_xfer(struct xiic_i2c *i2c)
>
>  static void xiic_start_xfer(struct xiic_i2c *i2c)
>  {
> +   spin_lock(>lock);
> +   xiic_reinit(i2c);
> +   spin_unlock(>lock);
>
> __xiic_start_xfer(i2c);
>  }
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] i2c: Revert "i2c: xiic: Do not reset controller before every transfer"

[Shubhrajyoti Datta]

On Tue, Nov 17, 2015 at 10:47 AM, Shubhrajyoti Datta
 wrote:
> On Mon, Nov 16, 2015 at 7:12 PM, Lars-Peter Clausen  wrote:
>> Commit d701667bb331 ("i2c: xiic: Do not reset controller before every
>> transfer") removed the reinitialization of the controller before the start
>> of each transfer. Apparently this change is not safe to make and the commit
>> results in random I2C bus failures.
>
> Which is the platform and the ip version that you  saw the issue.
> Did you see the issue with read and write as  well?

Also having a closer look, if we reinit the status registers and bus
busy will be
cleared.

> .
>
>>
>> An easy way to trigger the issue is to run i2cdetect.
>>
>> Without the patch applied:
>>  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
>>  00:  -- -- -- -- -- -- -- -- -- -- -- -- --
>>  10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  30: -- -- -- -- -- -- -- -- UU UU -- UU 3c -- -- UU
>>  40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  70: -- -- -- -- -- -- -- --
>>
>> With the patch applied every other or so invocation:
>>  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
>>  00:  03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f
>>  10: 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f
>>  20: 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f
>>  30: -- -- -- -- -- -- -- -- UU UU -- UU 3c -- -- UU
>>  40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  70: -- -- -- -- -- -- -- --
>>
> I assume that you have these many peripherals.
> on the bus am I right?
>
>
>> So revert the commit for now.
>>
>> Fixes: d701667bb331 ("i2c: xiic: Do not reset controller before every 
>> transfer")
>> Signed-off-by: Lars-Peter Clausen 
>> ---
>>  drivers/i2c/busses/i2c-xiic.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
>> index e23a7b0..705cf69 100644
>> --- a/drivers/i2c/busses/i2c-xiic.c
>> +++ b/drivers/i2c/busses/i2c-xiic.c
>> @@ -662,6 +662,9 @@ static void __xiic_start_xfer(struct xiic_i2c *i2c)
>>
>>  static void xiic_start_xfer(struct xiic_i2c *i2c)
>>  {
>> +   spin_lock(>lock);
>> +   xiic_reinit(i2c);
>> +   spin_unlock(>lock);
>>
>> __xiic_start_xfer(i2c);
>>  }
>> --
>> 2.1.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] i2c: Revert "i2c: xiic: Do not reset controller before every transfer"

[Lars-Peter Clausen]

On 11/17/2015 06:17 AM, Shubhrajyoti Datta wrote:
> On Mon, Nov 16, 2015 at 7:12 PM, Lars-Peter Clausen  wrote:
>> Commit d701667bb331 ("i2c: xiic: Do not reset controller before every
>> transfer") removed the reinitialization of the controller before the start
>> of each transfer. Apparently this change is not safe to make and the commit
>> results in random I2C bus failures.
> 
> Which is the platform and the ip version that you  saw the issue.
> Did you see the issue with read and write as  well?

The IP version is the axi-iic v2.0 Revision 8. I've tested this on a few
platforms, custom ones and standard ones and I could reproduce it on most.
One of them was on the ZED board. The one where I couldn't reproduce it was
the ZC706. But that doesn't necessarily mean it doesn't happen there, just
that it is not triggered by the testcase.

The problem is that it is random corruption, so some I2C devices might start
to behave strangely at some point. The only good more or less reliable way
to reproduce it that I found was to run i2cdetect a couple of times and at
least one of them will produce strange behavior.

>>
>> An easy way to trigger the issue is to run i2cdetect.
>>
>> Without the patch applied:
>>  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
>>  00:  -- -- -- -- -- -- -- -- -- -- -- -- --
>>  10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  30: -- -- -- -- -- -- -- -- UU UU -- UU 3c -- -- UU
>>  40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  70: -- -- -- -- -- -- -- --
>>
>> With the patch applied every other or so invocation:
>>  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
>>  00:  03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f
>>  10: 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f
>>  20: 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f
>>  30: -- -- -- -- -- -- -- -- UU UU -- UU 3c -- -- UU
>>  40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>>  70: -- -- -- -- -- -- -- --
>>
> I assume that you have these many peripherals.
> on the bus am I right?

Sorry, I should have been more clear. The first one is before the patch that
introduces the issue, the second one is with the patch that introduces the
issue.

- Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html