[U-Boot-Users] [PATCH] bug in README and soft_i2c.c

[Joakim Tjernlund]

> -----Original Message-----
> From: Andrew Dyer [mailto:amdyer at gmail.com] 
> Sent: den 6 juni 2007 01:04
> To: joakim.tjernlund at transmode.se
> Cc: U-Boot list
> Subject: Re: [U-Boot-Users] [PATCH] bug in README and soft_i2c.c
> 
> On 5/22/07, Joakim Tjernlund <joakim.tjernlund at transmode.se> wrote:
> > I think the README w.r.t I2C_TRISTATE is OK as is(don't 
> change it). I do
> > think the soft i2c driver is broken in several places w.r.t
> > IC2_ACTIVE/I2C_TRISTATE and I2C reset sequence. The below 
> patch is an
> > attempt to fix it, but I havn't tested it.
> >
> > Signed-off-by: Joakim Tjernlund <joakim.tjernlund at transmode.se>
> >
> > diff --git a/common/soft_i2c.c b/common/soft_i2c.c
> > index edad51b..ed3f8f8 100644
> > --- a/common/soft_i2c.c
> > +++ b/common/soft_i2c.c
> > @@ -100,15 +100,18 @@ static void send_reset(void)
> >  #endif
> >         I2C_TRISTATE;
> >         for(j = 0; j < 9; j++) {
> > +               if(I2C_READ)
> > +                       send_start();
> >                 I2C_SCL(0);
> >                 I2C_DELAY;
> > +               I2C_TRISTATE;
> > +               I2C_SDA(1);
> >                 I2C_DELAY;
> >                 I2C_SCL(1);
> >                 I2C_DELAY;
> >                 I2C_DELAY;
> >         }
> >         send_stop();
> > -       I2C_TRISTATE;
> >  }
> >
> >  
> /*------------------------------------------------------------
> -----------
> 
> I'm not sure about this part.  I think the idea was to send a full
> transaction.  You've added a bunch of repeated start commands in here
> which may not work as the data is low when it shouldn't be.  I think

Data isnt low since I test for that first.

> the best you can hope for is to try and clock through whatever state
> machines might be out there.
> 
> A better solution might be to test the state of data (and clock would
> be good too), and repeat the clearing loop some number of times.  If
> the data never goes high or clock is stuck low, print a message and
> bail out.

I got the current sequence into ppcboot years ago, but a few years later
I found it was incomplete. The current sequence will only "unlock" a slave
that's stuck in a READ, the new one will also unlock a dev that's stuck
in a WRITE.

> 
> > @@ -124,12 +127,13 @@ static void send_start(void)
> >  #endif
> >
> >         I2C_DELAY;
> > +       I2C_TRISTATE;
> >         I2C_SDA(1);
> > -       I2C_ACTIVE;
> >         I2C_DELAY;
> >         I2C_SCL(1);
> >         I2C_DELAY;
> >         I2C_SDA(0);
> > +       I2C_ACTIVE;
> >         I2C_DELAY;
> >  }
> >
> 
> this looks funky to me for the case where you have tri-state I/O - you
> are specifically not driving sda high, instead letting it float up
> with the pullup.  If you have active drivers, IMHO you should use them
> to make nice clean edges, not let the pullup slowly drag the line high
> :-)

Well, this is how I2C is supposed to work. Why would I violate that
in generic code? You could end up in a state, where the
slave is driving SDA low while the master is driving SDA high, that's
not an ideal situation.

Did you test the non TRISTATE case?

[SNIP more of the same]