On Tue, 2007-05-22 at 18:23 +0200, Joakim Tjernlund wrote:

On Mon, 2007-05-21 at 17:04 -0500, Andrew Dyer wrote:

...

Hi, I noticed a bug in soft_i2c.c (or in the README depending on how you think about it).

If CONFIG_SOFT_I2C is defined and using open-collector I/O, then the README says that I2C_TRISTATE can be defined as null. If this is the case, then doing:

imd 29 0.0 2

will cause the following chain of calls in soft_i2c.c:

send_start() write_byte() read_byte() read_byte() send_stop()

The first read_byte() calls send_ack(0). send_ack(0) drives the data line low without releasing it. The following call to read_byte() calls I2C_TRISTATE, but in this case it is defined as NULL per the README. This leaves the data line driven low during the read of the 2nd byte causing data^h^h^h^hhair loss that I can ill afford :-)

I propose the following fix - modify the README to make say that I2C_TRISTATE should be defined even for open-collector setups, and add code to soft_i2c.c to explictly tristate the data at the end of send_ack(0)

Signed-off-by: Andrew Dyer amdyer@gmail.com

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.

I have tested the non open-collector on my HW and that works fine. If someone can test the open-collector case that would be good so this can go into u-boot.

Jocke

Signed-off-by: Joakim Tjernlund joakim.tjernlund@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;

}

/*----------------------------------------------------------------------- @@ -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;

}

@@ -152,9 +156,9 @@ static void send_stop(void) I2C_DELAY; I2C_SCL(1); I2C_DELAY;

• I2C_TRISTATE; I2C_SDA(1); I2C_DELAY;

• I2C_TRISTATE;

}

@@ -172,14 +176,17 @@ static void send_ack(int ack)

I2C_SCL(0); I2C_DELAY;

• I2C_ACTIVE; I2C_SDA(ack);

• I2C_ACTIVE; I2C_DELAY; I2C_SCL(1); I2C_DELAY; I2C_DELAY; I2C_SCL(0); I2C_DELAY;
• I2C_TRISTATE;
• I2C_SDA(1);
• I2C_DELAY;

}

@@ -215,8 +222,8 @@ static int write_byte(uchar data) */ I2C_SCL(0); I2C_DELAY;

• I2C_SDA(1); I2C_TRISTATE;

• I2C_SDA(1); I2C_DELAY; I2C_SCL(1); I2C_DELAY;

@@ -224,7 +231,6 @@ static int write_byte(uchar data) nack = I2C_READ; I2C_SCL(0); I2C_DELAY;

• I2C_ACTIVE;

  return(nack); /* not a nack is an ack */

} @@ -249,6 +255,7 @@ static uchar read_byte(int ack) * Read 8 bits, MSB first. */ I2C_TRISTATE;

• I2C_SDA(1); data = 0; for(j = 0; j < 8; j++) { I2C_SCL(0);

---

This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ _______________________________________________ U-Boot-Users mailing list U-Boot-Users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/u-boot-users

-----Original Message----- From: Andrew Dyer [mailto:amdyer@gmail.com] Sent: den 6 juni 2007 01:04 To: joakim.tjernlund@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@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@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]