Re: [PATCH] i2c-omap: Trigger bus recovery in lockup case

[Vignesh R <vigneshr@ti.com>]

On Monday 18 September 2017 05:31 PM, Claudio Foellmi wrote:
> On 18.09.2017 07:24, Vignesh R wrote:
>>
>>
>> On Saturday 16 September 2017 05:01 AM, Strashko, Grygorii wrote:
>>>
>>>
>>> On 09/14/2017 10:39 AM, Claudio Foellmi wrote:
>>>> A very conservative check for bus activity (to prevent interference
>>>> in multimaster setups) prevented the bus recovery methods from being
>>>> triggered in the case that SDA or SCL was stuck low.
>>>> This defeats the purpose of the recovery mechanism, which was introduced
>>>> for exactly this situation (a slave device keeping SDA pulled down).
>>>>
>>>> Note that bus lockups can persist across reboots. The only other options
>>>> are to reset or power cycle the offending slave device, and many i2c
>>>> slaves do not even have a reset pin.
>>>>
>>>> If we see that one of the lines is low for the entire timeout duration,
>>>> we can actually be sure that there is no other master driving the bus.
>>>> It is therefore save for us to attempt a bus recovery.
>>>>
>>>
>>> Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com>
>>>
>>>> Signed-off-by: Claudio Foellmi <claudio.foellmi@ergon.ch>
>>>> ---
>>>> Caveat: It turns out I don't have the hardware to fully test the
>>>> recovery mechanism. My faulty i2c slave device actually pulls down SCL,
>>>> not SDA (so the recovery will not succeed in my case).
>>
>> Maybe, you could detect SCL stuck low case by reading status of SCL line
>> from OMAP_I2C_SYSTEST_REG and then call IP reset (there is nothing much
>> that can be done) instead of bus recovery.
>>
> 
> I plan on posting a related patch soon, that will print better messages
> if the generic recovery fails. If SCL is stuck low, I think the best we
> can do is make the problem visible in the kernel log.
> 
>>>> But by directly connecting SDA to ground, I could at least make sure
>>>> the recovery function gets called after applying this patch.
>>>>
>>
>> I had seen flood of XRDY & RRDY interrupts as soon as TMODE is set to
>> 0x3 as part of omap_i2c_prepare_recovery() leading to unusable system.
>> Did you observe this behavior on your system? Could you mention the
>> platform on which this experiment done?
>>
> 
> So attempting bus recovery is dangerous on some platforms?
> I did not notice any obvious problems (assuming an 'unusable system'
> would be hard to miss), but then again I only have one target to test on.
> I'm working with a TI AM3352, the slave is a NXP NT3H2211 on i2c-1.
> 

I hit a situation where when communicating with a faulty i2c device, the
last transaction on the bus does not end with proper STOP condition on
the i2c bus. Since, STOP condition was not detected by IP, Bus Busy will
remain set even though both SCL and SDA are high. Thus,
omap_i2c_wait_for_bb() function would end up calling bus recovery. And
as soon as TMODE is set to 0x3 and ST_EN to 0x1, there is a flood of
XRDY & RRDY interrupts.

This spurious irqs can be reproduced easily by setting TMODE to 0x3 and
ST_EN to 0x1 in OMAP_I2C_SYSTEST_REG when both SCL and SDA are high (bus
is idle) even on AM335x.

So, if you are not seeing irq flood when SCL/SDA is stuck low, then
maybe its safe to enter TMODE 0x3 in such cases. Could you modify the
patch to test whether or not SDA is stuck low before initiating bus
recovery?


>>>>   drivers/i2c/busses/i2c-omap.c | 7 ++++++-
>>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>>>> index 1ebb5e9..4b25fd1 100644
>>>> --- a/drivers/i2c/busses/i2c-omap.c
>>>> +++ b/drivers/i2c/busses/i2c-omap.c
>>>> @@ -563,8 +563,13 @@ static int omap_i2c_wait_for_bb_valid(struct omap_i2c_dev *omap)
>>>>   		}
>>>>   
>>>>   		if (time_after(jiffies, timeout)) {
>>>> +			/*
>>>> +			 * SDA or SCL were low for the entire timeout without
>>>> +			 * any activity detected. Most likely, a slave is
>>>> +			 * locking up the bus with no master driving the clock.
>>>> +			 */
>>>>   			dev_warn(omap->dev, "timeout waiting for bus ready\n");
>>>> -			return -ETIMEDOUT;
>>>> +			return i2c_recover_bus(&omap->adapter);
>>>>   		}
>>>>   
>>>>   		msleep(1);
>>>>
>>>
>>
> 
> --
> Regards
> Claudio
> 

-- 
Regards
Vignesh