I2CBitController: only detect SCL stretching while in a transaction

hdl/ip/bsv/I2C/I2CBitController.bsv

@@ -155,14 +155,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);
+    rule do_sample_scl_stretch(scl_stretch_sample_strobe && scl_in == 0 && state != AwaitStart);
         scl_stretching              <= scl_out_en == 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 || state == AwaitStart);
         scl_stretching <= False;
     endrule
 

hdl/ip/bsv/I2C/test/I2CPeripheralModel.bsv

@@ -33,6 +33,7 @@ interface I2CPeripheralModel;
     interface Get#(ModelEvent) receive;
     method Action nack_response(Bool ack);
     method Action stretch_next(Bool timeout);
+    method Action bus_pullups(Bool present);
 endinterface
 
 typedef union tagged {
@@ -110,6 +111,8 @@ module mkI2CPeripheralModel #(Bit#(7) i2c_address,
     Reg#(Bool) countdown_reset  <- mkReg(False);
     Reg#(Bool) back_to_rx  <- mkReg(False);
 
+    ConfigReg#(Bool) pullups_lost <- mkConfigReg(False);
+
     (* fire_when_enabled *)
     rule do_detect_scl_fedge;
         scl_in_prev <= scl_in;
@@ -319,13 +322,21 @@ module mkI2CPeripheralModel #(Bit#(7) i2c_address,
     endmethod
 
     method Bit#(1) scl_o();
-        return scl_out;
+        if (pullups_lost) begin
+            return 0;
+        end else begin
+            return scl_out;
+        end
     endmethod
 
     method Action sda_i(Bit#(1) sda_i_next) = sda_in._write(sda_i_next);
 
     method Bit#(1) sda_o();
-        return sda_out;
+        if (pullups_lost) begin
+            return 0;
+        end else begin
+            return sda_out;
+        end
     endmethod
 
     method Action nack_response(Bool ack) = nack_response_._write(ack);
@@ -341,6 +352,8 @@ module mkI2CPeripheralModel #(Bit#(7) i2c_address,
         end
     endmethod
 
+    method Action bus_pullups(Bool present) = pullups_lost._write(!present);
+
     interface Get receive = toGet(outgoing_events);
 endmodule
 

hdl/projects/sidecar/qsfp_x32/QSFPModule/test/QsfpModuleControllerTests.bsv

@@ -33,7 +33,7 @@ QsfpModuleController::Parameters qsfp_test_params =
         core_clk_period_ns: i2c_test_params.core_clk_period_ns,
         i2c_frequency_hz: i2c_test_params.scl_freq_hz,
         power_good_timeout_ms: 10,
-        t_init_ms: 20, // normally 2000, but sped up for simulation
+        t_init_ms: 5, // normally 2000, but sped up for simulation
         t_clock_hold_us: i2c_test_params.max_scl_stretch_us
     };
 
@@ -134,6 +134,14 @@ module mkBench (Bench);
     mkConnection(controller.pins.sda.out, periph.sda_i);
     mkConnection(controller.pins.sda.in, periph.sda_o);
 
+    // We need the ability to simulate the bus losing its pull-ups when a module has not been
+    // inserted since that is how the design behaves. We only apply power to the module (and by the
+    // board design, it's bus) when a module is present. This is kind of janky given we can't
+    // properly simulate tristate logic in bluesim.
+    rule do_pullup_simulation;
+        periph.bus_pullups(controller.pg);
+    endrule
+
     // Used to make dummy data for the DUT to pull from
     Reg#(UInt#(8)) fifo_idx         <- mkReg(0);
 
@@ -289,6 +297,21 @@ function Stmt insert_and_power_module(Bench bench);
     endseq);
 endfunction
 
+function Stmt remove_and_power_down_module(Bench bench);
+    return (seq
+        bench.set_modprsl(1);
+        // modprsl is debounced, so wait for it to transition
+        await(bench.modprsl == 1);
+        delay(5);
+        assert_false(bench.hsc_en(),
+            "Hot swap should be disabled when module is missing");
+        // after some delay, remove power good
+        bench.set_hsc_pg(0);
+        // power good is debounced, so wait for it to transition
+        await(!bench.hsc_pg);
+    endseq);
+endfunction
+
 function Stmt deassert_reset_and_await_init(Bench bench);
     return (seq
         // release reset
@@ -326,6 +349,9 @@ module mkNoModuleTest (Empty);
         assert_eq(unpack(bench.registers.port_status.error[2:0]),
             NoModule,
             "NoModule error should be present when attempting to communicate with a device which is not present.");
+        assert_eq(unpack(bench.registers.port_status.stretching_seen),
+            False,
+            "Should not have observed and SCL stretching.");
         delay(5);
     endseq);
 endmodule
@@ -352,6 +378,9 @@ module mkNoPowerTest (Empty);
         assert_eq(unpack(bench.registers.port_status.error[2:0]),
             NoPower,
             "NoPower error should be present when attempting to communicate before the hot swap is stable.");
+        assert_eq(unpack(bench.registers.port_status.stretching_seen),
+            False,
+            "Should not have observed and SCL stretching.");
         delay(5);
     endseq);
 endmodule
@@ -385,6 +414,9 @@ module mkRemovePowerEnableTest (Empty);
         await(!bench.hsc_pg);
         delay(3);
         assert_eq(bench.hsc_en(), False, "Expect hot swap to no longer be enabled.");
+        assert_eq(unpack(bench.registers.port_status.stretching_seen),
+            False,
+            "Should not have observed and SCL stretching.");
         delay(5);
     endseq);
 endmodule
@@ -410,6 +442,9 @@ module mkPowerGoodTimeoutTest (Empty);
         assert_eq(unpack(bench.registers.port_status.error[2:0]),
             PowerFault,
             "PowerFault error should be present when attempting to communicate after the hot swap has timed out");
+        assert_eq(unpack(bench.registers.port_status.stretching_seen),
+            False,
+            "Should not have observed and SCL stretching.");
         delay(5);
     endseq);
 endmodule
@@ -437,6 +472,9 @@ module mkPowerGoodLostTest (Empty);
         assert_eq(unpack(bench.registers.port_status.error[2:0]),
             PowerFault,
             "PowerFault error should be present when attempting to communicate after the hot swap has aborted");
+        assert_eq(unpack(bench.registers.port_status.stretching_seen),
+            False,
+            "Should not have observed and SCL stretching.");
         delay(5);
     endseq);
 endmodule
@@ -535,6 +573,9 @@ module mkInitializationTest (Empty);
         assert_eq(unpack(bench.registers.port_status.error[2:0]),
             NotInitialized,
             "NotInitialized error should be present when resetl is asserted.");
+        assert_eq(unpack(bench.registers.port_status.stretching_seen),
+            False,
+            "Should not have observed and SCL stretching.");
         delay(5);
     endseq);
 endmodule
@@ -562,19 +603,18 @@ module mkUninitializationAfterRemovalTest (Empty);
             NoError,
             "NoError should be present when attempting to communicate after t_init has elapsed.");
 
-        bench.set_modprsl(1);
-        // ModPrsL is debounced and thus won't transition immediately
-        await(bench.modprsl == 1);
-        bench.set_modprsl(0);
-        await(bench.modprsl == 0);
-        delay(3); // wait a few cycles for power to re-enable
-        bench.command(read_cmd, False, False);
+        remove_and_power_down_module(bench);
+        insert_and_power_module(bench);
 
+        bench.command(read_cmd, False, False);
         await(!bench.i2c_busy());
         delay(3);
         assert_eq(unpack(bench.registers.port_status.error[2:0]),
             NotInitialized,
             "NotInitialized error should be present when a module has been reseated but not initialized.");
+        assert_eq(unpack(bench.registers.port_status.stretching_seen),
+            False,
+            "Should not have observed and SCL stretching.");
     endseq);
 endmodule
 
@@ -605,6 +645,9 @@ module mkNoLPModeWhenModuleIsUnpoweredTest (Empty);
 
         await(bench.hsc_pg); // wait out debounce
         assert_set(bench.lpmode, "LpMode should be asserted now that 3.3V is up.");
+        assert_eq(unpack(bench.registers.port_status.stretching_seen),
+            False,
+            "Should not have observed and SCL stretching.");
     endseq);
 endmodule
 
@@ -620,6 +663,9 @@ module mkIntLTest (Empty);
         bench.set_intl(0);
         await(bench.intl == 0);
         assert_not_set(bench.intl, "IntL should be low after debounce");
+        assert_eq(unpack(bench.registers.port_status.stretching_seen),
+            False,
+            "Should not have observed and SCL stretching.");
     endseq);
 endmodule
 
@@ -635,6 +681,9 @@ module mkModPrsLTest (Empty);
         bench.set_modprsl(0);
         await(bench.modprsl == 0);
         assert_not_set(bench.modprsl, "ModPrsL should be low after debounce");
+        assert_eq(unpack(bench.registers.port_status.stretching_seen),
+            False,
+            "Should not have observed and SCL stretching.");
     endseq);
 endmodule
 

hdl/projects/sidecar/qsfp_x32/QSFPModule/test/QsfpModuleControllerTests.gtkw

@@ -1,19 +1,19 @@
 [*]
-[*] GTKWave Analyzer v3.3.100 (w)1999-2019 BSI
-[*] Mon Sep 16 21:06:14 2024
+[*] GTKWave Analyzer v3.3.104 (w)1999-2020 BSI
+[*] Thu Dec  5 13:08:47 2024
 [*]
-[dumpfile] "\\wsl$\Ubuntu-20.04\home\aaron\Oxide\git\quartz\build\vcd\QsfpModuleControllerTests_mkI2CSclStretchTimeoutTest.vcd"
-[dumpfile_mtime] "Mon Sep 16 17:50:10 2024"
-[dumpfile_size] 71544432
-[savefile] "\\wsl$\Ubuntu-20.04\home\aaron\Oxide\git\quartz\hdl\projects\sidecar\qsfp_x32\QSFPModule\test\QsfpModuleControllerTests.gtkw"
-[timestart] 15988190
-[size] 2558 1360
+[dumpfile] "/home/aaron/Oxide/git/quartz/build/vcd/QsfpModuleControllerTests_mkUninitializationAfterRemovalTest.vcd"
+[dumpfile_mtime] "Thu Dec  5 13:00:44 2024"
+[dumpfile_size] 93767403
+[savefile] "/home/aaron/Oxide/git/quartz/hdl/projects/sidecar/qsfp_x32/QSFPModule/test/QsfpModuleControllerTests.gtkw"
+[timestart] 13499672
+[size] 2592 1283
 [pos] -1 -1
-*-13.036575 16052670 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1
+*-7.171101 13500140 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1 -1
 [treeopen] main.
 [treeopen] main.top.
-[sst_width] 446
-[signals_width] 526
+[sst_width] 596
+[signals_width] 645
 [sst_expanded] 1
 [sst_vpaned_height] 404
 @200
@@ -22,6 +22,7 @@
 @28
 main.top.bench_controller_lpmode_
 main.top.bench_controller_resetl_
+main.top.bench_modprsl_r
 @200
 -
 @28
@@ -47,22 +48,29 @@ main.top.bench_controller_rdata_fifo_deq
 main.top.bench_controller_wdata_fifo_deq
 @200
 -
--
 -I2CCore
+@28
+main.top.bench_controller_i2c_core_bit_ctrl_sda_in
+main.top.bench_controller_i2c_core_bit_ctrl_scl_in
 @22
 main.top.bench_controller_i2c_core_state_r[3:0]
 @28
 main.top.bench_controller_i2c_core_bit_ctrl_scl_stretch_seen_r
-@29
 main.top.bench_controller_i2c_core_bit_ctrl_scl_stretch_timeout_r
-@28
 main.top.bench_controller_i2c_core_bit_ctrl_scl_stretching
+@24
+main.top.bench_periph_scl_stretch_countdown_count_r[15:0]
+@28
+main.top.bench_periph_scl_stretch_countdown_q
+@24
+main.top.bench_controller_i2c_core_bit_ctrl_scl_stretch_timeout_cntr_count[15:0]
+@28
+main.top.bench_controller_i2c_core_bit_ctrl_scl_stretch_timeout_cntr_q
+@25
+main.top.bench_controller_i2c_core_bit_ctrl_scl_stretch_sample_strobe_count[5:0]
+@28
+main.top.bench_controller_i2c_core_bit_ctrl_scl_stretch_sample_strobe_q
 @200
--FIFO: Next Command
--FIFO: BitControl Incoming Events
--FIFO: BitControl Outgoing Events
--FIFO: RX Data
--FIFO: TX Data
 -
 -WDATA FIFO
 @28
@@ -104,7 +112,6 @@ main.top.bench_controller_rdata_fifo_memory.ADDRB[7:0]
 main.top.bench_fifo_idx[7:0]
 @200
 -
--
 -Model - I2CPeripheralModel
 @28
 main.top.bench_periph_scl_in