[U-Boot] [PATCH] i2c: fix SDA contention in read_byte()

[Andrew Dyer]

On Sun, Jul 11, 2010 at 5:55 PM, Mike Frysinger <vapier at gentoo.org> wrote:
> On Wednesday, July 07, 2010 00:45:42 Andrew Dyer wrote:
>> On Tue, Jul 6, 2010 at 1:14 AM, Thomas Chou <thomas at wytron.com.tw> wrote:
>> > We should not set SDA after TRISTATE, as it results in contention.
>> >
>> > Signed-off-by: Thomas Chou <thomas at wytron.com.tw>
>> > ---
>> >  drivers/i2c/soft_i2c.c |    2 +-
>> >  1 files changed, 1 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/drivers/i2c/soft_i2c.c b/drivers/i2c/soft_i2c.c
>> > index 847db76..344b7f8 100644
>> > --- a/drivers/i2c/soft_i2c.c
>> > +++ b/drivers/i2c/soft_i2c.c
>> > @@ -305,8 +305,8 @@ static uchar read_byte(int ack)
>> >        /*
>> >         * Read 8 bits, MSB first.
>> >         */
>> > -       I2C_TRISTATE;
>> >        I2C_SDA(1);
>> > +       I2C_TRISTATE;
>> >        data = 0;
>> >        for(j = 0; j < 8; j++) {
>> >                I2C_SCL(0);
>> > --
>>
>> I2C_TRISTATE is supposed to be persistent until I2C_ACTIVE is called,
>> so in the original code it should still be in effect when I2C_SDA(1)
>> is executed and there should be no contention.  This patch causes the
>> code to actively drive SDA high at the same time the addressed device
>> might be driving it low, causing contention until the I2C_TRISTATE
>> takes effect.
>>
>> In some sense the code is misleadingly written, as it is not allowed
>> in the spec to actively drive a '1' on the bus, a chip is only
>> supposed to drive 'z' or '0', and the platform is supposed to provide
>> the pullup current.  This comes more into play if the i2c bus supports
>> clock stretching or arbitration among multiple masters.
>
> how do you propose we get i2c gpio working ?  it works fine under Linux.  we
> cannot tristate a pin (set the gpio to an input) and then turn around and
> attempt to drive it (set the gpio to an output with a specific value).  that
> is what the code currently does.
>
> our end goal is simple: have i2c gpio bitbanging work under u-boot like under
> linux.  we dont really care about the exact way we get there.  this patch was
> just one idea.
> -mike

If you make sure that I2C_SDA(1)/I2C_SCL(1) makes the pins tristate
and I2C_SDA(0)/I2C_SCL(0) means driven actively low, you can define
I2C_ACTIVE and I2C_TRISTATE as nothing, and it should all work out
right.  The trick is in making sure that the order of operations on
the IO buffer data/enables doesn't cause the output to glitch,
especially on the clock line.