I2C: A couple of fixes around SCL stretching is handled (#248)

This commit fixes a couple of bugs related to SCL stretching. First off,
in the `I2CBitController` I adjusted how we sample SCL to detect
stretching so we're not errantly sampling. Then in the `I2CCore` I added
a mechanism to handle the SCL stretch timeouts when we see them. Before
we lacked this and that meant if that was seen, we were wedged. The rest
of the changes were to test cases in order to mimic the behavior
observed that brought the problems to our attention.

Fixes #246
Fixes #247

Author: Aaron-Hartwig

hdl/ip/bsv/I2C/I2CBitController.bsv

@@ -48,6 +48,9 @@ interface I2CBitController;
     interface Put#(Event) send;
     interface Get#(Event) receive;
 
+    // way of indicating when there is no bus activity
+    method Bool busy();
+
     // clock stretching information sent sideband from the state machine events
     method Bool scl_stretch_seen();
     method Bool scl_stretch_timeout();
@@ -155,14 +158,14 @@ module mkI2CBitController #(
     // After the delay we know SCL is being stretched if we aren't the ones
     // holding it low.
     (* fire_when_enabled *)
-    rule do_sample_scl_stretch(scl_stretch_sample_strobe && scl_in == 0);
-        scl_stretching              <= scl_out_en == 0;
+    rule do_sample_scl_stretch(scl_stretch_sample_strobe);
+        scl_stretching              <= scl_out_en == 0 && scl_in == 0;
         scl_stretch_sample_delay    <= False;
     endrule
 
     // If SCL is high then no one is holding it
     (* fire_when_enabled *)
-    rule do_clear_scl_stretch(scl_in == 1);
+    rule do_clear_scl_stretch(scl_in == 1 && !scl_stretch_sample_strobe);
         scl_stretching <= False;
     endrule
 
@@ -251,6 +254,8 @@ module mkI2CBitController #(
     (* fire_when_enabled *)
     rule do_scl_stretch_timeout(scl_stretch_timeout_cntr);
         state                   <= AwaitStart;
+        scl_active              <= False;
+        sda_out_en              <= 0;
         scl_stretch_timeout_r   <= True;
         incoming_events.clear();
     endrule
@@ -416,6 +421,8 @@ module mkI2CBitController #(
     endinterface
     interface Get receive = toGet(outgoing_events);
 
+    method busy = state != AwaitStart;
+
     method scl_stretch_seen     = scl_stretch_seen_r;
     method scl_stretch_timeout  = scl_stretch_timeout_r;
 endmodule

hdl/ip/bsv/I2C/I2CCore.bsv

@@ -13,6 +13,7 @@ export Pins(..);
 export I2CCore(..);
 export mkI2CCore;
 
+import ConfigReg::*;
 import DefaultValue::*;
 import DReg::*;
 import FIFO::*;
@@ -105,40 +106,57 @@ module mkI2CCore#(Integer core_clk_freq,
     Reg#(Bool) in_random_read           <- mkReg(False);
     Reg#(Bool) in_write_ack_poll        <- mkReg(False);
     Reg#(Bool) write_acked              <- mkReg(False);
-
+    ConfigReg#(Bool) state_cleared      <- mkConfigReg(False);
+    Reg#(Bool) clearing_state           <- mkReg(False);
+    PulseWire clear_state               <- mkPulseWire;
     PulseWire next_send_data            <- mkPulseWire;
 
-    // relabel this net for brevity
-    let timed_out = bit_ctrl.scl_stretch_timeout;
-
     (* fire_when_enabled, no_implicit_conditions *)
     rule do_valid_command;
         valid_command   <= isValid(cur_command);
     endrule
 
     // when the bit controller has timed out, clear core state
     (* fire_when_enabled *)
-    rule do_handle_stretch_timeout(timed_out);
-        next_command.clear();
-        error_r <= tagged Invalid;
-        state_r <= Idle;
+    rule do_clearing_state_reg;
+        clearing_state <= (bit_ctrl.scl_stretch_timeout && !state_cleared) || clear_state;
     endrule
 
     (* fire_when_enabled *)
-    rule do_register_command(state_r == Idle && !valid_command && !timed_out);
-        cur_command <= tagged Valid next_command.first;
-        error_r     <= tagged Invalid;
-        state_r     <= SendStart;
+    rule do_handle_stretch_timeout(clearing_state);
+        next_command.deq();
+        bytes_done          <= 0;
+        in_random_read      <= False;
+        in_write_ack_poll   <= False;
+        write_acked         <= False;
+        cur_command         <= tagged Invalid;
+        state_r             <= Idle;
     endrule
 
     (* fire_when_enabled *)
-    rule do_send_start (state_r == SendStart && valid_command && !timed_out);
+    rule do_state_cleared_reg;
+        if (clearing_state) begin
+            state_cleared   <= True;
+        end else if (valid_command && bit_ctrl.busy()) begin
+            state_cleared   <= False;
+        end
+    endrule
+
+    (* fire_when_enabled *)
+    rule do_register_command(state_r == Idle && !valid_command && !clearing_state);
+        cur_command     <= tagged Valid next_command.first;
+        error_r         <= tagged Invalid;
+        state_r         <= SendStart;
+    endrule
+
+    (* fire_when_enabled *)
+    rule do_send_start (state_r == SendStart && valid_command && !clearing_state);
         bit_ctrl.send.put(tagged Start);
         state_r <= SendAddr;
     endrule
 
     (* fire_when_enabled *)
-    rule do_send_addr (state_r == SendAddr && valid_command && !timed_out);
+    rule do_send_addr (state_r == SendAddr && valid_command && !clearing_state);
         let cmd = fromMaybe(?, cur_command);
         let is_read = (cmd.op == Read) || in_random_read;
         let addr_byte = {cmd.i2c_addr, pack(is_read)};
@@ -149,21 +167,22 @@ module mkI2CCore#(Integer core_clk_freq,
     (* fire_when_enabled *)
     rule do_await_addr_ack (state_r == AwaitAddrAck
                             && valid_command
-                            && !timed_out);
+                            && !clearing_state);
         let ack_nack <- bit_ctrl.receive.get();
         let cmd = fromMaybe(?, cur_command);
 
         case (ack_nack) matches
             tagged Ack: begin
-                // Begin a Read
                 if (cmd.op == Read || in_random_read) begin
+                    // begin a Read
                     bytes_done  <= 1;
                     bit_ctrl.send.put(tagged Read (cmd.num_bytes == 1));
                     state_r <= Reading;
                 end else if (in_write_ack_poll) begin
                     write_acked <= True;
                     state_r     <= Stop;
                 end else begin
+                    // begin a Write
                     bit_ctrl.send.put(tagged Write cmd.reg_addr);
                     state_r <= AwaitWriteAck;
                 end
@@ -180,7 +199,7 @@ module mkI2CCore#(Integer core_clk_freq,
     endrule
 
     (* fire_when_enabled *)
-    rule do_writing (state_r == Writing && valid_command && !timed_out);
+    rule do_writing (state_r == Writing && valid_command && !clearing_state);
         bit_ctrl.send.put(tagged Write tx_data);
         next_send_data.send();
         state_r <= AwaitWriteAck;
@@ -189,7 +208,7 @@ module mkI2CCore#(Integer core_clk_freq,
     (* fire_when_enabled *)
     rule do_await_writing_ack (state_r == AwaitWriteAck
                                 && valid_command
-                                && !timed_out);
+                                && !clearing_state);
         let ack_nack <- bit_ctrl.receive.get();
         let cmd = fromMaybe(?, cur_command);
 
@@ -215,7 +234,7 @@ module mkI2CCore#(Integer core_clk_freq,
     endrule
 
     (* fire_when_enabled *)
-    rule do_reading (state_r == Reading && valid_command && !timed_out);
+    rule do_reading (state_r == Reading && valid_command && !clearing_state);
         let rdata   <- bit_ctrl.receive.get();
         let cmd     = fromMaybe(?, cur_command);
 
@@ -233,14 +252,14 @@ module mkI2CCore#(Integer core_clk_freq,
         endcase
     endrule
 
-    rule do_next_read (state_r == NextRead && valid_command && !timed_out);
+    rule do_next_read (state_r == NextRead && valid_command && !clearing_state);
         let cmd     = fromMaybe(?, cur_command);
         bit_ctrl.send.put(tagged Read (cmd.num_bytes == bytes_done));
         state_r <= Reading;
     endrule
 
     (* fire_when_enabled *)
-    rule do_stop (state_r == Stop && valid_command && !timed_out);
+    rule do_stop (state_r == Stop && valid_command && !clearing_state);
         bit_ctrl.send.put(tagged Stop);
 
         let cmd     = fromMaybe(?, cur_command);
@@ -253,14 +272,8 @@ module mkI2CCore#(Integer core_clk_freq,
     endrule
 
     (* fire_when_enabled *)
-    rule do_done (state_r == Done && valid_command && !timed_out);
-        next_command.deq();
-        bytes_done          <= 0;
-        in_random_read      <= False;
-        in_write_ack_poll   <= False;
-        write_acked         <= False;
-        cur_command         <= tagged Invalid;
-        state_r             <= Idle;
+    rule do_done (state_r == Done && valid_command && !clearing_state && !bit_ctrl.busy());
+        clear_state.send();
     endrule
 
     interface pins = bit_ctrl.pins;

hdl/ip/bsv/I2C/test/I2CBitControllerTests.bsv

@@ -89,7 +89,6 @@ module mkBench (Bench);
     Reg#(Vector#(3,Bit#(8))) prev_written_bytes <- mkReg(replicate(0));
     Reg#(UInt#(2)) bytes_done                   <- mkReg(0);
     Reg#(Bool) is_last_byte                     <- mkReg(False);
-    Reg#(Bool) stretch_timeout                  <- mkReg(False);
 
     FSM write_seq <- mkFSMWithPred(seq
         dut.send.put(tagged Start);
@@ -120,7 +119,7 @@ module mkBench (Bench);
 
         dut.send.put(tagged Stop);
         check_peripheral_event(periph, tagged ReceivedStop, "Expected to receive STOP");
-    endseq, command_r.op == Write && !stretch_timeout);
+    endseq, command_r.op == Write && !dut.scl_stretch_timeout);
 
     FSM read_seq <- mkFSMWithPred(seq
         dut.send.put(tagged Start);
@@ -153,7 +152,7 @@ module mkBench (Bench);
 
         dut.send.put(tagged Stop);
         check_peripheral_event(periph, tagged ReceivedStop, "Expected to receive STOP");
-    endseq, command_r.op == Read && !stretch_timeout);
+    endseq, command_r.op == Read && !dut.scl_stretch_timeout);
 
     FSM rnd_read_seq <- mkFSMWithPred(seq
         dut.send.put(tagged Start);

hdl/ip/bsv/I2C/test/I2CCoreTests.bsv

@@ -135,7 +135,7 @@ module mkBench (Bench);
             endaction
         endseq
         check_peripheral_event(periph, tagged ReceivedStop, "Expected to receive STOP");
-    endseq, command_r.op == Write);
+    endseq, command_r.op == Write && !dut.scl_stretch_timeout);
 
     FSM read_seq <- mkFSMWithPred(seq
         dut.send_command.put(command_r);
@@ -156,7 +156,7 @@ module mkBench (Bench);
         check_peripheral_event(periph, tagged ReceivedNack, "Expected to receive NACK to end the Read");
         check_peripheral_event(periph, tagged ReceivedStop, "Expected to receive STOP");
         bytes_done  <= 0;
-    endseq, command_r.op == Read);
+    endseq, command_r.op == Read && !dut.scl_stretch_timeout);
 
     FSM rand_read_seq <- mkFSMWithPred(seq
         dut.send_command.put(command_r);
@@ -181,7 +181,7 @@ module mkBench (Bench);
         check_peripheral_event(periph, tagged ReceivedNack, "Expected to receive NACK to end the Read");
         check_peripheral_event(periph, tagged ReceivedStop, "Expected to receive STOP");
         bytes_done  <= 0;
-    endseq, command_r.op == RandomRead);
+    endseq, command_r.op == RandomRead && !dut.scl_stretch_timeout);
 
     rule do_handle_stretch_timeout(dut.scl_stretch_timeout);
         write_seq.abort();