[i2c,sival] Reduce I2C bitbanging flakiness by separating SDA and SCL…

… write

This commit reduces the flakiness seen in tests that use I2C over GPIO
bitbanging - namely `i2c_target_test` where this was being used.

Currently, writes to SCL and SDA can be performed within a single GPIO
bitbanging sample. This relied on the SDA value being set before the the
SCL value, but in reality these changes were happening microseconds
apart. Physical characteristics of the FPGA such as e.g. the capacitance
on different pins could add delays of a couple of microseconds, causing
the SDA variation to appear after the SCL variation. This could cause a
valid write (setting SDA high before SCL's rising edge) to transmit as a
'None' signal, which was causing a lot of flakiness in tests.

This commit fixes the I2C bitbanging logic to run at half the sampling
rate, extending each sample into two samples - one that sets the new SDA
value whilst the clock is maintained, and one that sets the SCL value
while the SDA is maintained. This ensures "atomic" sequential I2C GPIO
pin writes which will be interpreted correctly, improving reliability.

Signed-off-by: Alex Jones <[email protected]>

sw/host/opentitanlib/src/test_utils/bitbanging/i2c.rs

@@ -24,16 +24,36 @@ impl Symbol {
         Ok(Self::Broken(buffer))
     }
 
+    // We must extend the transaction: in a given sample, if data is written to SDA at the same time as SCL
+    // being driven high, then physical factors e.g. capacitance can cause the SDA write to appear before
+    // the SCL, even though the order of bitbanging is reversed. This can then cause a mistaken "STOP" signal
+    // to be received. We instead run at half frequency: on one cycle write SDA, and on the next write SCL.
+    // This makes GPIO bitbanging of I2C signals reliable.
+    // Assumes that SCL is low (0) when called.
     pub fn bitbanging<const SDA: u8, const SCL: u8>(&self, samples: &mut Vec<u8>) {
         match self {
+            // Each sample is translated into 2 samples: the first changes the SDA, and
+            // the second changes the SCL, to ensure correct ordering.
             Symbol::Start => samples.extend([
+                // SDA high, SCL high
+                0x01 << SDA | 0x00 << SCL,
                 0x01 << SDA | 0x01 << SCL,
+                // SDA low, SCL high
+                0x00 << SDA | 0x01 << SCL,
+                0x00 << SDA | 0x01 << SCL,
+                // SDA low, SCL low
                 0x00 << SDA | 0x01 << SCL,
                 0x00 << SDA | 0x00 << SCL,
             ]),
             Symbol::Stop => samples.extend([
+                // SDA low, SCL low
+                0x00 << SDA | 0x00 << SCL,
+                0x00 << SDA | 0x00 << SCL,
+                // SDA low, SCL high
                 0x00 << SDA | 0x00 << SCL,
                 0x00 << SDA | 0x01 << SCL,
+                // SDA high, SCL high
+                0x01 << SDA | 0x01 << SCL,
                 0x01 << SDA | 0x01 << SCL,
             ]),
             Symbol::Byte { data, nack } => Self::bitbanging_byte::<SDA, SCL>(*data, *nack, samples),
@@ -45,6 +65,10 @@ impl Symbol {
         let data: u16 = (byte as u16) << 1u16 | nack as u16;
         samples.extend((0..9u8).rev().flat_map(|bit| {
             [
+                // Change SDA (to data), SCL high
+                ((((data >> bit) & 0x01) << SDA) | 0x00 << SCL) as u8,
+                ((((data >> bit) & 0x01) << SDA) | 0x01 << SCL) as u8,
+                // Maintain SDA, SCL low
                 ((((data >> bit) & 0x01) << SDA) | 0x01 << SCL) as u8,
                 ((((data >> bit) & 0x01) << SDA) | 0x00 << SCL) as u8,
             ]
@@ -54,6 +78,10 @@ impl Symbol {
     fn bitbanging_bits<const SDA: u8, const SCL: u8>(bits: &[Bit], samples: &mut Vec<u8>) {
         samples.extend(bits.iter().rev().flat_map(|bit| {
             [
+                // Change SDA (to data), SCL high
+                ((*bit as u8) << SDA) | 0x00 << SCL,
+                ((*bit as u8) << SDA) | 0x01 << SCL,
+                // Maintain SDA, SCL low
                 ((*bit as u8) << SDA) | 0x01 << SCL,
                 ((*bit as u8) << SDA) | 0x00 << SCL,
             ]
@@ -115,6 +143,9 @@ pub mod encoder {
 
     pub struct Encoder<const SDA: u8, const SCL: u8> {}
     impl<const SDA: u8, const SCL: u8> Encoder<SDA, SCL> {
+        // Note that this function will run I2C at half of the specified bitbanging sample frequency, because
+        // two cycles must be used per sample to ensure that changes to SDA appear before the rise of SCL,
+        // as otherwise I2C via GPIO bitbanging can be flaky.
         pub fn run(&self, transfer: &[Transfer]) -> Vec<u8> {
             let mut samples: Vec<u8> = Vec::new();
             for window in transfer.windows(2) {