I2C SDA/SCL race #83

New Issue

sb10q · 2021-06-20T21:53:24+08:00

sb10q commented

2021-06-20 21:53:24 +08:00

The current I2C code switches SDA and SCL almost at the same time, which can create false start/stop conditions.

Quick fix (not well thought through):

diff --git a/libboard_zynq/src/i2c/mod.rs b/libboard_zynq/src/i2c/mod.rs
index 8290903..28b9405 100644
--- a/libboard_zynq/src/i2c/mod.rs
+++ b/libboard_zynq/src/i2c/mod.rs
@@ -150,6 +150,7 @@ impl I2c {
         self.sda_oe(true);
         self.half_period();
         self.scl_oe(true);
+        self.half_period();
         // postcondition: SCL and SDA low
         Ok(())
     }
@@ -188,6 +189,7 @@ impl I2c {
             self.scl_oe(false);
             self.half_period();
             self.scl_oe(true);
+            self.half_period();
         }
         self.sda_oe(false);
         self.half_period();
@@ -196,6 +198,7 @@ impl I2c {
         // Read ack/nack
         let ack = !self.sda_i();
         self.scl_oe(true);
+        self.half_period();
         self.sda_oe(true);
         // postcondition: SCL and SDA low

The bug was inherited from the main ARTIQ repos and is also present there, but only became symptomatic on Kasli-SoC.

The current I2C code switches SDA and SCL almost at the same time, which can create false start/stop conditions. Quick fix (not well thought through): ``` diff --git a/libboard_zynq/src/i2c/mod.rs b/libboard_zynq/src/i2c/mod.rs index 8290903..28b9405 100644 --- a/libboard_zynq/src/i2c/mod.rs +++ b/libboard_zynq/src/i2c/mod.rs @@ -150,6 +150,7 @@ impl I2c { self.sda_oe(true); self.half_period(); self.scl_oe(true); + self.half_period(); // postcondition: SCL and SDA low Ok(()) } @@ -188,6 +189,7 @@ impl I2c { self.scl_oe(false); self.half_period(); self.scl_oe(true); + self.half_period(); } self.sda_oe(false); self.half_period(); @@ -196,6 +198,7 @@ impl I2c { // Read ack/nack let ack = !self.sda_i(); self.scl_oe(true); + self.half_period(); self.sda_oe(true); // postcondition: SCL and SDA low ``` The bug was inherited from the main ARTIQ repos and is also present there, but only became symptomatic on Kasli-SoC.

harry commented

2021-06-21 14:47:49 +08:00

So there is this issue where, after NACK (i.e. SDA does NOT get pulled down by the receiver), SCL might not go low before a transition happens on SDA, meaning a "repeated" START could be triggered, is that correct?

In addition to your suggestion, we can also loop to read from SCL before the transmtter pull down SDA, and that should consume less than half an SCL period.

So there is this issue where, after NACK (i.e. SDA does NOT get pulled down by the receiver), SCL might not go low before a transition happens on SDA, meaning a "repeated" START could be triggered, is that correct? In addition to your suggestion, we can also loop to read from SCL before the transmtter pull down SDA, and that should consume less than half an SCL period.

harry commented

2021-06-21 17:37:56 +08:00

Yes, I also just noticed that SCL might not be held low long enough before the next bit is shifted in/out on the SDA line. If my understanding is right, the I2C spec requires that no transition shall take place when SCL is not pulled LOW.

sb10q commented

2021-06-21 17:44:42 +08:00

Poster

So there is this issue where, after NACK (i.e. SDA does NOT get pulled down by the receiver), SCL might not go low before a transition happens on SDA, meaning a "repeated" START could be triggered, is that correct?

See the patch, there are at least 3 potential places where SCL/SDA races could happen.

the I2C spec requires that no transition shall take place when SCL is not pulled LOW.

Correct, unless you are doing START or STOP, you can only toggle SDA while SCL is low (and there should be some time margins).

> So there is this issue where, after NACK (i.e. SDA does NOT get pulled down by the receiver), SCL might not go low before a transition happens on SDA, meaning a "repeated" START could be triggered, is that correct? See the patch, there are at least 3 potential places where SCL/SDA races could happen. > the I2C spec requires that no transition shall take place when SCL is not pulled LOW. Correct, unless you are doing START or STOP, you can only toggle SDA while SCL is low (and there should be some time margins).

sb10q referenced this issue from a commit

2021-06-25 16:27:03 +08:00

i2c: conservative timing, avoid SCL/SDA races. Closes #83

sb10q closed this issue

2021-06-25 16:27:03 +08:00

Sign in to join this conversation.