[i2c] Reject Start/Stop if SCL changes too soon

Because the spec's data hold time is nominally 0, there are situations
where an SDA change may arrive before an SCL change after CDC. This can
give the appearance that a controller has sent a Start or Stop symbol,
when the next bit was intended to be data, leading to unintended
outcomes.

However, Start and Stop symbols have a significant required hold time, a
period after the SDA transition when SCL is not allowed to change. Use
some of this period to disambiguate data vs. control symbols. Require
SCL to be held stable for at least thd_dat for a control symbol to
complete decoding.

Note that thd_sta and t_buf must be longer than thd_dat, and thd_dat
must be at least 1 cycle. Adjust the randomization of timing parameters
in DV so this constraint is properly observed.

Signed-off-by: Alexander Williams <[email protected]>

hw/ip/i2c/doc/programmers_guide.md

@@ -45,6 +45,8 @@ $$ \textrm{TSU_DAT_MIN}= \lceil{t\_{HD,DAT,min}/t\_{clk}}\rceil $$
 $$ \textrm{T_BUF_MIN}= \lceil{t\_{BUF,min}/t\_{clk}}\rceil $$
 $$ \textrm{T_STO_MIN}= \lceil{t\_{STO,min}/t\_{clk}}\rceil $$
 
+Note that `T_HD_DAT_MIN` must be at least 1, and `T_HD_STA_MIN` and `T_BUF_MIN` must be greater than `T_HD_DAT_MIN`.
+
 1. Input the integer timing parameters, THD_STA_MIN, TSU_STA_MIN, THD_DAT_MIN, TSU_DAT_MIN, T_BUF_MIN and T_STO_MIN into their corresponding registers (`TIMING2.THD_STA`, `TIMING2.TSU_STA`, `TIMING3.THD_DAT`, `TIMING3.TSU_DAT`, `TIMING4.T_BUF`, `TIMING4.T_STO`)
     - This step allows the firmware to manage SDA signal delays to ensure that the SDA outputs are compliant with the specification.
     - The registers `TIMING0.THIGH` and `TIMING0.TLOW` will be taken care of in a later step.

hw/ip/i2c/doc/theory_of_operation.md

@@ -154,6 +154,8 @@ Taken to be synonymous with T<sub>SU,ACK</sub>
 - t<sub>HD,DAT</sub>: set in register [`TIMING3.THD_DAT`](registers.md#timing3).
 Taken to be synonymous with T<sub>HD,ACK</sub>.
 Moreover, since the pin driver fall time is likely to be less then one clock cycle, this parameter is also taken to be synonymous with the parameters T<sub>VD,DAT</sub> and T<sub>VD,ACK</sub>
+This parameter controls the number of cycles after the falling edge of SCL that SDA is driven.
+In addition, when the IP operates as a target, the parameter specifies the required SCL high time after SDA changes to satisfy the Start and Stop symbol decoders.
 - t<sub>SU,STO</sub>: set in register [`TIMING4.TSU_STO`](registers.md#timing4).
 - t<sub>BUF</sub>: set in register [`TIMING4.T_BUF`](registers.md#timing4)
 

hw/ip/i2c/dv/env/seq_lib/i2c_base_vseq.sv

@@ -188,6 +188,10 @@ class i2c_base_vseq extends cip_base_vseq #(
     solve t_r, tsu_dat, thd_dat before tlow;
     solve t_r                   before t_buf;
     solve t_f, thigh            before t_sda_unstable, t_sda_interference;
+
+    thd_sta > thd_dat + 1;
+    t_buf > thd_dat + 1;
+
     if (program_incorrect_regs) {
       // force derived timing parameters to be negative (incorrect DUT config)
       tsu_sta == t_r + t_buf + 1;  // negative tHoldStop

hw/ip/i2c/dv/env/seq_lib/i2c_host_rx_oversample_vseq.sv

@@ -14,11 +14,13 @@ class i2c_host_rx_oversample_vseq extends i2c_rx_tx_vseq;
     thigh   == 1;
     t_r     == 1;
     t_f     == 1;
-    thd_sta == 1;
+    thd_sta == 3;
     tsu_sto == 1;
     tsu_dat == 1;
     thd_dat == 1;
 
+    t_buf > thd_dat + 1;
+
     solve t_r, tsu_dat, thd_dat before tlow;
     solve t_r                   before t_buf;
     solve t_f, thigh            before t_sda_unstable, t_sda_interference;

hw/ip/i2c/dv/env/seq_lib/i2c_target_perf_vseq.sv

@@ -12,7 +12,7 @@ class i2c_target_perf_vseq extends i2c_target_smoke_vseq;
   constraint timing_val_c {
     t_r       == 1;
     t_f       == 1;
-    thd_sta   == 1;
+    thd_sta   == 3;
     tsu_sto   == 1;
     tsu_dat   == 1;
     thd_dat   == 1;
@@ -24,7 +24,11 @@ class i2c_target_perf_vseq extends i2c_target_smoke_vseq;
     tlow      == 8;
     // tHoldStop must be at least 2 cycles which implies, t_r + t_buf - tsu_sta >= 2
     // in order for stop condition to propogate to internal FSM via prim flop
-    t_buf     == tsu_sta - t_r + 2;
+    t_buf     >= tsu_sta - t_r + 2;
+    // In addition, t_buf > thd_dat + 1 to satisfy the Start/Stop decoder,
+    // which rejects decoding Start/Stop symbols if SCL changes after SDA within
+    // thd_dat cycles (+1 for CDC skew)
+    t_buf     == thd_dat + 2;
   }
 
 endclass

hw/ip/i2c/dv/env/seq_lib/i2c_target_smoke_vseq.sv

@@ -20,6 +20,10 @@ class i2c_target_smoke_vseq extends i2c_base_vseq;
     solve t_r                   before t_buf;
     solve tsu_sta               before t_buf;
     solve t_f, thigh            before t_sda_unstable, t_sda_interference;
+
+    thd_sta > thd_dat + 1;
+    t_buf > thd_dat + 1;
+
     if (program_incorrect_regs) {
       // force derived timing parameters to be negative (incorrect DUT config)
       tsu_sta == t_r + t_buf + 1;  // negative tHoldStop

hw/ip/i2c/rtl/i2c_fsm.sv

@@ -109,6 +109,9 @@ module i2c_fsm import i2c_pkg::*;
   logic        byte_decr;     // indicates byte_index must be decremented by 1
   logic        byte_clr;      // indicates byte_index must be reset to byte_num
 
+  // Stop / Start detection counter
+  logic [15:0] ctrl_det_count;
+
   // Other internal variables
   logic        scl_q;         // scl internal flopped
   logic        sda_q;         // data internal flopped
@@ -126,7 +129,9 @@ module i2c_fsm import i2c_pkg::*;
   logic        log_stop;      // indicates stop is been issued
 
   // Target specific variables
+  logic        start_det_trigger, start_det_pending;
   logic        start_det;     // indicates start or repeated start is detected on the bus
+  logic        stop_det_trigger, stop_det_pending;
   logic        stop_det;      // indicates stop is detected on the bus
   logic        address0_match;// indicates target's address0 matches the one sent by host
   logic        address1_match;// indicates target's address1 matches the one sent by host
@@ -328,11 +333,55 @@ module i2c_fsm import i2c_pkg::*;
     end
   end
 
+  // To resolve ambiguity with early SDA arrival, reject control symbols when
+  // SCL goes low too soon. The hold time for ordinary data/ACK bits is too
+  // short to reliably see SCL change before SDA. Use the hold time for
+  // control signals to ensure a Start or Stop symbol was actually received.
+  // Requirements: thd_dat + 1 < thd_sta
+  //   The extra (+1) here is to account for a late SDA arrival due to CDC
+  //   skew.
+  //
+  // Note that this counter combines Start and Stop detection into one
+  // counter. A controller-only reset scenario could end up with a Stop
+  // following shortly after a Start, with the requisite setup time not
+  // observed.
+  always_ff @(posedge clk_i or negedge rst_ni) begin
+    if (!rst_ni) begin
+      ctrl_det_count <= '0;
+    end else if (start_det_trigger || stop_det_trigger) begin
+      ctrl_det_count <= 16'd1;
+    end else if (start_det_pending || stop_det_pending) begin
+      ctrl_det_count <= ctrl_det_count + 1'b1;
+    end
+  end
+
+  always_ff @(posedge clk_i or negedge rst_ni) begin
+    if (!rst_ni) begin
+      start_det_pending <= 1'b0;
+    end else if (start_det_trigger) begin
+      start_det_pending <= 1'b1;
+    end else if (!target_enable_i || !scl_i || start_det || stop_det_trigger) begin
+      start_det_pending <= 1'b0;
+    end
+  end
+
+  always_ff @(posedge clk_i or negedge rst_ni) begin
+    if (!rst_ni) begin
+      stop_det_pending <= 1'b0;
+    end else if (stop_det_trigger) begin
+      stop_det_pending <= 1'b1;
+    end else if (!target_enable_i || !scl_i || stop_det || start_det_trigger) begin
+      stop_det_pending <= 1'b0;
+    end
+  end
+
   // (Repeated) Start condition detection by target
-  assign start_det = target_enable_i && (scl_i_q && scl_i) & (sda_i_q && !sda_i);
+  assign start_det_trigger = target_enable_i && (scl_i_q && scl_i) & (sda_i_q && !sda_i);
+  assign start_det = target_enable_i && start_det_pending && (ctrl_det_count >= thd_dat_i);
 
   // Stop condition detection by target
-  assign stop_det = target_enable_i && (scl_i_q && scl_i) & (!sda_i_q && sda_i);
+  assign stop_det_trigger = target_enable_i && (scl_i_q && scl_i) & (!sda_i_q && sda_i);
+  assign stop_det = target_enable_i && stop_det_pending && (ctrl_det_count >= thd_dat_i);
 
   // Bit counter on the target side
   assign bit_ack = (bit_idx == 4'd8); // ack