[i2c] Eliminate multi-item repeated start entry in ACQ FIFO

Change recording of Start symbols to always accompany the address data,
instead of recording a separate R.Start, then Start+address. This helps
maintain certain availability invariants needed for reliable stretching
and NACK handling.

Note that a controller that chooses to do a Repeated Start and address a
*different* target is an unusual case, and it leads to a situation where
the transfer closing symbol won't appear until the Stop or a later
Repeated Start that *does* address this target. While this transaction
style is not explicitly forbidden in the I2C specification, it is not
typically supported in the wild. In any case, software should be able to
resolve this case, and a closing symbol will eventually appear.

Fix up the DV transaction generation to match the expected behavior.

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

hw/ip/i2c/doc/theory_of_operation.md

@@ -124,10 +124,15 @@ The assigned address and mask pairs are set in registers [`TARGET_ID.ADDRESS0`](
 ### Acquired Formatted Data
 
 This section applies to I2C in the target mode.
-When the target accepts a transaction, it inserts the transaction address, read/write bit, and START signal sent by the host into ACQ FIFO where they can be accessed by software.
+When the target accepts a transfer, it inserts the transfer address, read/write bit, and START signal sent by the host into ACQ FIFO where they can be accessed by software.
 ACQ FIFO output corresponds to [`ACQDATA`](registers.md#acqdata).
-If the transaction is a write operation (R/W bit = 0), the target proceeds to read bytes from the bus and insert them into ACQ FIFO until the host terminates the transaction by sending a STOP or a repeated START signal.
-A STOP or repeated START indicator is inserted into ACQ FIFO as the next entry following the last byte received, in which case other bits may be junk.
+If the transfer is a write operation (R/W bit = 0), the target proceeds to read bytes from the bus and insert them into ACQ FIFO until the host terminates the transfer by sending a STOP or a repeated START signal.
+A STOP or repeated START indicator is inserted into ACQ FIFO as the next entry following the last byte received, in which case other bits may be junk or the target address, respectively.
+
+Note that the repeated START indicator only appears if it matches the address.
+Typically, this is the case, and a transaction that changes targets between transfers is strongly discouraged.
+If no additional transfers targeting this target's address occur, an entry representing the end of the transfer will wait for the STOP symbol.
+
 The following diagram shows consecutive entries inserted into ACQ FIFO during a write operation:
 
 ![](../doc/I2C_acq_fifo_write.svg)
@@ -143,14 +148,17 @@ The following diagram shows consecutive entries inserted into ACQ FIFO during a
 
 ![](../doc/I2C_acq_fifo_read.svg)
 
-The ACQ FIFO entry consists of 10 bits:
+The ACQ FIFO entry consists of 11 bits:
 - Address (bits 7:1) and R/W bit (bit 0) or data byte
-- Format flags (bits 9:8)
+- Format flags (bits 10:8)
 The format flags indicate the following signals received from the host:
-- START: 01
-- STOP: 10
-- repeated START: 11
-- No START, or STOP: 00
+- START: 001
+- STOP completing transaction without errors: 010
+- repeated START: 011
+- No START, or STOP: 000
+- NACK during target-receiver transfer: 100
+- NACK of this target's address: 101
+- STOP received for a transaction with errors: 110
 
 ### Timing Control Registers
 

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

@@ -629,7 +629,7 @@ class i2c_base_vseq extends cip_base_vseq #(
     foreach (myq[i]) begin
       if (myq[i].rstart) begin
         if (cfg.m_i2c_agent_cfg.allow_ack_stop & read) begin
-            `uvm_info("seq", $sformatf("eos: %s", (read_ack_nack_q[rd_idx++]) ?
+            `uvm_info("seq", $sformatf("(n)ack: %s", (read_ack_nack_q[rd_idx++]) ?
                                        "NACK" : "ACK"), UVM_MEDIUM)
         end
         read = myq[i].read;
@@ -640,7 +640,7 @@ class i2c_base_vseq extends cip_base_vseq #(
       end
     end
     if (cfg.m_i2c_agent_cfg.allow_ack_stop & read) begin
-      `uvm_info("seq", $sformatf("eos: %s", (read_ack_nack_q[rd_idx]) ?
+      `uvm_info("seq", $sformatf("(n)ack: %s", (read_ack_nack_q[rd_idx]) ?
                                  "NACK" : "ACK"), UVM_MEDIUM)
     end
   endfunction
@@ -750,15 +750,18 @@ class i2c_base_vseq extends cip_base_vseq #(
   // and compared with received transaction at the scoreboard.
   virtual function void fetch_txn(ref i2c_item src_q[$], i2c_item dst_q[$],
                                   input bit force_ack = 0, bit skip_start = 0);
+    // A temporary, representing each item that is pushed to the dst_q intended for the driver.
     i2c_item txn;
     i2c_item rs_txn;
+    // The subset of items that address the DUT that are expected from the ACQ FIFO.
     i2c_item exp_txn;
+    // All items to go on the bus, including those that don't address the DUT.
     i2c_item full_txn;
     int read_size;
     bit is_read = get_read_write();
     bit [6:0] t_addr;
     bit valid_addr;
-    bit got_valid;
+    bit addressed;
 
     `uvm_info("seq", $sformatf("idx %0d:is_read:%0b size:%0d fetch_txn:%0d", start_cnt++, is_read,
                                src_q.size(), full_txn_num++), UVM_MEDIUM)
@@ -785,7 +788,11 @@ class i2c_base_vseq extends cip_base_vseq #(
     `uvm_create_obj(i2c_item, txn)
     `uvm_create_obj(i2c_item, exp_txn)
     txn.drv_type = HostData;
-    txn.start = 1;
+    if (skip_start) begin
+      txn.rstart = 1;
+    end else begin
+      txn.start = 1;
+    end
     txn.wdata[7:1] = get_target_addr(); //target_addr0;
     txn.wdata[0] = is_read;
     valid_addr = is_target_addr(txn.wdata[7:1]);
@@ -801,8 +808,8 @@ class i2c_base_vseq extends cip_base_vseq #(
       p_sequencer.target_mode_wr_exp_port.write(exp_txn);
       cfg.sent_acq_cnt++;
       this.tran_id++;
-      got_valid = 1;
       if (is_read) this.exp_rd_id++;
+      addressed = 1;
     end
     read_size = get_read_data_size(src_q, is_read, read_rcvd);
     cfg.m_i2c_agent_cfg.sent_rd_byte += read_size;
@@ -815,37 +822,19 @@ class i2c_base_vseq extends cip_base_vseq #(
         full_txn.data_q.push_back(txn.wdata);
       end
 
-      // RS creates 2 extra acq entry
-      // one for RS
-      // the other for a new start acq_entry with address
       if (txn.drv_type == HostRStart) begin
         bit prv_read = 0;
         bit prv_valid = valid_addr;
 
-        t_addr = get_target_addr();
-        valid_addr = is_target_addr(t_addr);
-        `uvm_create_obj(i2c_item, exp_txn)
-        exp_txn.drv_type = HostRStart;
-        exp_txn.tran_id = this.tran_id;
-
-        // In 'txn' boundary, only if previous segment is valid,
-        // we create current RS header.
-        if (prv_valid) begin
-           exp_txn.rstart = 1;
-           this.tran_id++;
-           // RS head
-           p_sequencer.target_mode_wr_exp_port.write(exp_txn);
-           cfg.sent_acq_cnt++;
-        end
-        got_valid = valid_addr;
-
         `uvm_create_obj(i2c_item, rs_txn)
         `downcast(rs_txn, txn.clone())
         dst_q.push_back(txn);
 
+        t_addr = get_target_addr();
+        valid_addr = is_target_addr(t_addr);
         rs_txn.drv_type = HostData;
-        rs_txn.start = 1;
-        rs_txn.rstart = 0;
+        rs_txn.start = 0;
+        rs_txn.rstart = 1;
         rs_txn.wdata[7:1] = t_addr;
         prv_read = is_read;
         is_read = rs_txn.read;
@@ -857,6 +846,7 @@ class i2c_base_vseq extends cip_base_vseq #(
         exp_txn.tran_id = this.tran_id;
 
 
+        addressed |= valid_addr;
         // Restart command entry
         if (valid_addr) begin
            p_sequencer.target_mode_wr_exp_port.write(exp_txn);
@@ -881,10 +871,10 @@ class i2c_base_vseq extends cip_base_vseq #(
           `downcast(read_txn, txn.clone())
           full_txn.num_data++;
           if (src_q.size() == 0) begin
-            txn.drv_type = get_eos(.is_stop(1));
+            txn.drv_type = get_ack_nack(.is_stop(1));
           end else begin
             // if your next item is restart Do nack
-            if (src_q[0].drv_type == HostRStart) txn.drv_type = get_eos();
+            if (src_q[0].drv_type == HostRStart) txn.drv_type = get_ack_nack();
             else txn.drv_type = HostAck;
           end
           dst_q.push_back(txn);
@@ -911,7 +901,7 @@ class i2c_base_vseq extends cip_base_vseq #(
     `downcast(exp_txn, txn.clone());
     dst_q.push_back(txn);
     full_txn.stop = 1;
-    if (valid_addr) begin
+    if (addressed) begin
        p_sequencer.target_mode_wr_exp_port.write(exp_txn);
        cfg.sent_acq_cnt++;
        this.tran_id++;
@@ -1111,7 +1101,8 @@ class i2c_base_vseq extends cip_base_vseq #(
     end
   endtask // read_acq_fifo
 
-  function drv_type_e get_eos(bit is_stop = 0);
+  // is_stop: Whether the next symbol is to be a Stop
+  function drv_type_e get_ack_nack(bit is_stop = 0);
     drv_type_e rsp = HostNAck;
     if (cfg.m_i2c_agent_cfg.allow_ack_stop) begin
       if (read_ack_nack_q.pop_front()) begin
@@ -1124,7 +1115,7 @@ class i2c_base_vseq extends cip_base_vseq #(
       end
     end
     return rsp;
-  endfunction // get_eos
+  endfunction // get_ack_nack
 
   // Calling this task will trigger unexp_stop interrupt.
   task send_ack_stop();

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

@@ -199,25 +199,24 @@ class i2c_target_hrst_vseq extends i2c_target_smoke_vseq;
     // Update glitch data
     if (glitch inside {AddressByteStart, AddressByteStop}) begin
       i2c_item  glitch_txn;
-      exp_txn.wdata = 8'hAA;
       txn.wait_cycles = $urandom_range(2,6);
       // Add entry for Address glitch
       `uvm_create_obj(i2c_item, glitch_txn)
       if (glitch == AddressByteStart) begin
         txn.wdata = 8'hFF;
-        exp_txn.rstart = 1;
         // Add Start glitch
         glitch_txn.drv_type = HostRStart;
         skip_start = 1;
       end else begin
         txn.wdata = 8'h00;
-        exp_txn.stop = 1;
         // Add Stop glitch
         glitch_txn.drv_type = HostStop;
       end
       // Push transaction for driver
       driver_q.push_back(glitch_txn);
-      push_exp_txn(exp_txn);
+      // There should be no entry in the ACQ FIFO for an AddressByte*
+      // glitch, since this is a new transaction that never completes
+      // addressing the target.
       return; // return, since glitch entry is done
     end else begin
       // for valid address transaction indicate start bit
@@ -236,11 +235,8 @@ class i2c_target_hrst_vseq extends i2c_target_smoke_vseq;
       txn.wait_cycles = $urandom_range(2, 6);
       if(glitch == WriteDataByteStart) begin
         txn.wdata = 8'hFF;
-        exp_txn.rstart = 1;
-        skip_start = 1;
       end else if (glitch == WriteDataByteStop) begin
         txn.wdata = 8'h00;
-        exp_txn.stop = 1;
       end
       // Push transaction to driver queue
       driver_q.push_back(txn);
@@ -253,12 +249,18 @@ class i2c_target_hrst_vseq extends i2c_target_smoke_vseq;
       end else begin
         txn.drv_type = HostStop;
         txn.stop = 1;
+        exp_txn.stop = 1;
       end
       // Push transaction for driver
       driver_q.push_back(txn);
     end
     // Push transaction for expected ACQ data
-    push_exp_txn(exp_txn);
+    // No need to do this with skip_start, since it would duplicate fetch_txn()
+    // Is this horrible spaghetti code? Why, yes, yes it is.
+    // TODO(): Cleanup?
+    if (!skip_start) begin
+      push_exp_txn(exp_txn);
+    end
   endfunction
 
   task create_read_glitch(ref i2c_item driver_q[$]);