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

[Sebastian Reichel <sebastian.reichel@collabora.co.uk>]

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

Hi Claudio,

On Tue, Sep 26, 2017 at 02:24:59PM +0200, Claudio Foellmi wrote:
> On 19.09.2017 12:50, Vignesh R wrote:
> > 
> > 
> > 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?
> > 
> 
> This sounds more like a problem with the interrupt handler than with
> bus recovery, so I'm a bit hesitant to just add such a workaround.
> Instead, I spent a few hours looking through the interrupt handling
> (and poking my i2c bus with a wire to induce random faults), and
> I suspect to have found the underlying cause, or at least part of it:
> 
> We sometimes ignore some interrupts (such as RRDY if we think we are
> not in receiving mode), but don't really deal with their cause.
> As a result, the same interrupt will just be raised again as soon as
> we leave the handler. It will then be ignored again, and raised again...
> 
> I'm still not quite sure how we can reliably get into such situations in
> the first place, but not sending a stop condition seems to be part of it.
> Maybe it is somehow connected to the automatic internal state change
> that happens as part of AL or NACK interrupts.
> 
> 
> Below is a small patch that should test my assumptions.
> It clears the incoming fifo and acks the ignored RRDY interrupts.
> 
> Sebastian, can you please check if this helps with your problems on N950?
> If it does, I'll turn it into a proper standalone patch.

No, it does not. Also no interrupts ignoring messages appearing
in dmesg:

n950# dmesg | grep -E "48072000.i2c|lp5523x"
[    0.791046] omap_i2c 48072000.i2c: bus 1 rev4.4 at 400 kHz
[    4.934265] lp5523x 1-0032: reset command sent (no ACK)!
[    6.003875] omap_i2c 48072000.i2c: controller timed out
[    6.033874] lp5523x 1-0032: device detection err: -110
[    6.039154] lp5523x: probe of 1-0032 failed with error -110

-- Sebastian

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