Re: [PATCH V5 Resend 1/2] i2c/adapter: Add bus recovery infrastructure

["Uwe Kleine-König" <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

Hello,

> >> +     for (i = 0; i < bri->clock_cnt * 2; i++, val = !val) {
> >> +             set_scl_value(adap, val);
> >> +             ndelay(delay);
> >> +
> >> +             /* break if sda got high, check only when scl line is high */
> >> +             if (!bri->skip_sda_polling && val)
> >> +                     if (unlikely(get_sda_value(adap)))
> >> +                             break;
> >> +     }
> > With clock_cnt usually being 9 (and skipping sda polling) assume a
> > device pulls down SDA because it was just addressed but the last clock
> > pulse to release SDA didn't occur:
> >
> >         SDAout ¯\_/¯¯¯\___/¯¯¯\___________________/¯¯¯¯¯¯
> >         SCL    ¯¯\_/¯\_/¯\_/¯\_/¯\_/¯\_/¯\_/¯\_/¯\_/¯¯¯¯¯
> >                 S   1   2   3   4   5   6   7   8   9
> >
> > This adresses an eeprom with address 0x50. When SCL is released for the
> > 9th clock the eeprom pulls down SDA to ack being addressed. After that
> > the eeprom expects the master to write a byte.
> >
> > Now you do write 0xff:
> >
> >          i      0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17
> >         SDAin  ___/¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯\______
> >         SCL    ¯¯\_/¯\_/¯\_/¯\_/¯\_/¯¯\__/¯¯\__/¯¯\__/¯¯\______/
> >                 9   1   2   3   4    5     6     7     8
> >
> > (assuming the SCL line goes up some time later to its idle state).
> > That is you leave the bus in exactly the same state as before: The 9th
> > clock isn't complete and the eeprom asserts SDA low to ack the byte just
> > written. So the bus is still stalled. The problem is BTW the exact same
> > one I introduced first in my bus clear routine (for barebox though).
> > Even though you count to 2*9 you only do 8 real clock pulses because you
> > have a half cycle at both the start and the end.
> 
> Fantastic explanation!!
Your luck that I just had the same problem ;-)

> So, we actually need to do "Low-High" 9 times instead of "High-Low".
> So, initializing val to 0 should fix it?
I don't think this is enough. If you cut off the last half clock of the
first sequence above doing 9 times low-high isn't enough. So you have to
do high + 9x low-high to assert 9 full cycles.

> >> + * @scl_gpio_flags: flag for gpio_request_one of scl_gpio. 0 implies
> >> + *   GPIOF_OUT_INIT_LOW.
> >> + * @sda_gpio_flags: flag for gpio_request_one of sda_gpio. 0 implies
> >> + *   GPIOF_OUT_INIT_LOW.
> 
> > Didn't you want to change this to GPIOF_OUT_INIT_HIGH |
> > GPIOF_OPEN_DRAIN? Hmm, I just wonder how to distinguish
> > GPIOF_OUT_INIT_LOW from unset. Hmm, maybe assume unset because _LOW is
> > wrong?
> 
> Hmmm.... Hmmmm... Hmmm... :)
> Two things:
> - Why should we default it to GPIOF_OPEN_DRAIN? Open drain would mean,
> gpio_set_value() will not write one for it, but would put it in input mode.
> I don't think that would be correct, as an generic case.
As the i2c-bus is open drain and multi-master capable it's wrong to pull
it to 1 because this can result in a short circuit. Even in a
single-master scenario (where you can pull SCL in both directions) you
must not drive SDA to 1.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |