Merge pull request #620 from Silabs-ArjanB/ArjanB_todofl

Added todos related to recent PRs

Author: Silabs-ArjanB

bhv/cv32e40x_rvfi.sv

@@ -915,9 +915,9 @@ module cv32e40x_rvfi
         // Jumps may actually use rs1 before (id_valid && ex_ready), an assertion exists to check that
         // the jump target is stable and it should be safe to use rs1/2_rdata at the time of the pipeline handshake.
         // rs1/2 should reflect state of the first operation of any instruction.
-        rs1_addr   [STAGE_EX] <= first_op_id_i ? rs1_addr_id : rs1_addr[STAGE_EX];
-        rs2_addr   [STAGE_EX] <= first_op_id_i ? rs2_addr_id : rs2_addr[STAGE_EX];
-        rs1_rdata  [STAGE_EX] <= first_op_id_i ? rs1_rdata_id : rs1_rdata[STAGE_EX];
+        rs1_addr   [STAGE_EX] <= first_op_id_i ? rs1_addr_id : rs1_addr[STAGE_EX]; // todo: why the first_op_id_i dependency? In https://github.com/openhwgroup/cv32e40x/pull/605 it was stated as not needed.
+        rs2_addr   [STAGE_EX] <= first_op_id_i ? rs2_addr_id : rs2_addr[STAGE_EX]; // todo: why the first_op_id_i dependency? In https://github.com/openhwgroup/cv32e40x/pull/605 it was stated as not needed.
+        rs1_rdata  [STAGE_EX] <= first_op_id_i ? rs1_rdata_id : rs1_rdata[STAGE_EX]; // todo: add good explanation why the select is needed here and not for example on rs1_rdata_subop
         rs2_rdata  [STAGE_EX] <= first_op_id_i ? rs2_rdata_id : rs2_rdata[STAGE_EX];
 
         mem_rmask  [STAGE_EX] <= (lsu_en_id_i && !lsu_we_id_i) ? rvfi_mem_mask_int : '0;
@@ -1035,7 +1035,7 @@ module cv32e40x_rvfi
         end
 
         // Update rvfi_mem
-        rvfi_mem_rdata[(32*(memop_cnt+1))-1 -: 32] <= lsu_rdata_wb_i;
+        rvfi_mem_rdata[(32*(memop_cnt+1))-1 -: 32] <= lsu_rdata_wb_i; // todo: document rvfi_mem_* signals in user manual RVFI section as they differ from the official RVFI. At what index will the 'official' RVFI info be present (also for misaligned loads/stores)?
         rvfi_mem_rmask[ (4*(memop_cnt+1))-1 -:  4] <= mem_rmask [STAGE_WB];
         rvfi_mem_wmask[ (4*(memop_cnt+1))-1 -:  4] <= mem_wmask [STAGE_WB];
         rvfi_mem_addr [(32*(memop_cnt+1))-1 -: 32] <= ex_mem_addr;

rtl/cv32e40x_controller_fsm.sv

@@ -184,7 +184,7 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
 
   // Flag indicating there is a 'live' CLIC pointer in the pipeline
   // Used to block debug until pointer
-  logic pointer_in_pipeline;
+  logic clic_ptr_in_pipeline;
 
   // Internal irq_ack for use when a (clic) pointer reaches ID stage and
   // we have single stepping enabled.
@@ -383,10 +383,10 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
     if(SMCLIC) begin : gen_clic_pointer_flag
       // We only need to check EX and WB, as the FSM will only be in FUNCTIONAL state
       // one cycle after the target CLIC jump has been performed from ID
-      assign pointer_in_pipeline = (id_ex_pipe_i.instr_valid && id_ex_pipe_i.instr_meta.clic_ptr) ||
-                                   (ex_wb_pipe_i.instr_valid && ex_wb_pipe_i.instr_meta.clic_ptr);
+      assign clic_ptr_in_pipeline = (id_ex_pipe_i.instr_valid && id_ex_pipe_i.instr_meta.clic_ptr) ||
+                                    (ex_wb_pipe_i.instr_valid && ex_wb_pipe_i.instr_meta.clic_ptr);
     end else begin : gen_basic_pointer_flag
-      assign pointer_in_pipeline = 1'b0;
+      assign clic_ptr_in_pipeline = 1'b0;
     end
   endgenerate
   // Regular debug will kill insn in WB, do not allow if LSU is not interruptible, a fence.i handshake is taking place
@@ -395,8 +395,8 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
   // a trans_valid has been clocked without ex_valid && wb_ready handshake.
   // The cycle after fencei enters WB, the fencei handshake will be initiated. This must complete and the fencei instruction must retire before allowing debug.
   // Once the first part of a table jump has finished in WB, we are not allowed to take debug before the last part finishes. This can be detected when the last
-  // part of a table jump is in either EX or WB.
-  assign debug_allowed = lsu_interruptible_i && !fencei_ongoing && !xif_in_wb && !pointer_in_pipeline && sequence_interruptible;
+  // part of a table jump is in either EX or WB. // todo: update comments related to table jump (explain general concept and to which instructions it applies)
+  assign debug_allowed = lsu_interruptible_i && !fencei_ongoing && !xif_in_wb && !clic_ptr_in_pipeline && sequence_interruptible;
 
   // Debug pending for any other reason than single step
   assign pending_debug = (trigger_match_in_wb) ||
@@ -475,7 +475,7 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
     end else if (if_id_pipe_i.instr_valid) begin
       sequence_interruptible = first_op_id_i;
     end else begin
-      sequence_interruptible = first_op_if_i;
+      sequence_interruptible = first_op_if_i; // todo: first/last op should always be classified with instr_vali; if not it needs proper explanation.
     end
   end
 
@@ -490,7 +490,7 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
     end else begin
       // IF stage first_op defaults to 1'b1 if the stage does not hold a valid instruction or
       // table jump pointer. Table jump pointers will have first_op set to 0.
-      id_stage_haltable = first_op_if_i;
+      id_stage_haltable = first_op_if_i; // todo: first/last op should always be classified with instr_valid; if not it needs proper explanation (and I would prefer if we get rid of this special reliance on the 1'b1 default)
     end
   end
 

rtl/cv32e40x_if_stage.sv

@@ -313,11 +313,12 @@ module cv32e40x_if_stage import cv32e40x_pkg::*;
   // Trigger matches will have all write enables deasserted in ID, and once the first operation reaches WB the debug entry will be made.
   // Marking as first (and last above) since we know it will not be a true sequence. Sequencer will keep sequencing, but will get killed
   // upon debug entry and no side effect will occur.
+  // todo: The treatement of trigger match causes 1 instruction to possibly have first/last op set multiple times. Can this be avoided (it messes up the semantics of first/last op)?
   // todo: factor in CLIC pointers?
   assign first_op_o = prefetch_is_tbljmp_ptr ? 1'b0 :
                       trigger_match_i        ? 1'b1 : seq_first;
 
-
+  // todo: first/last op need to be treated in the same manner (assignments need to look more similar). Also seq_first/seq_last need cleaner semantics with respect ot how they relate to seq_valid.
 
   // Populate instruction meta data
   // Fields 'compressed' and 'tbljmp' keep their old value by default.
@@ -451,7 +452,7 @@ module cv32e40x_if_stage import cv32e40x_pkg::*;
       assign seq_last  = 1'b0;
       assign seq_instr = '0;
       assign seq_ready = 1'b1;
-      assign seq_first = 1'b1; // Tie high to enable default first_op when ZC_EXT=0
+      assign seq_first = 1'b1; // Tie high to enable default first_op when ZC_EXT=0 // todo: Clean up semantics of seq_first and make this one default 0.
     end
   endgenerate
 
@@ -465,6 +466,9 @@ module cv32e40x_if_stage import cv32e40x_pkg::*;
   assign tbljmp = (instr_decompressed.bus_resp.err || (instr_decompressed.mpu_status != MPU_OK)) ? 1'b0 :
                   trigger_match_i ? 1'b0 : tbljmp_raw;
 
+// todo: The above tbljmp assignment is likely very timing critical. Why is it important to suppress table jumps here, whereas the same thing is not done for non-sequenced compressed instructions nor for sequenced instructions.
+// In fact sequenced instructions seems only gated by trigger match, which also seems inconsistent. Can't all further handling of bus/parity/MPU/trigger match be done in ID?
+
   //---------------------------------------------------------------------------
   // eXtension interface
   //---------------------------------------------------------------------------

rtl/cv32e40x_sequencer.sv

@@ -72,7 +72,7 @@ import cv32e40x_pkg::*;
   assign rlist = instr[7:4];
 
   // Count number of instructions emitted and set next state for FSM.
-  always_ff @( posedge clk, negedge rst_n) begin
+  always_ff @(posedge clk, negedge rst_n) begin
     if (!rst_n) begin
       instr_cnt_q <= '0;
       seq_state_q <= S_IDLE;
@@ -89,7 +89,7 @@ import cv32e40x_pkg::*;
 
       if ((!valid_o && !halt_i) || kill_i) begin
         // Whenever we have no valid outputs and are not halted, reset counter to 0.
-        // In case both halt_i and kill_i are high, kill_i takes presedence.
+        // In case both halt_i and kill_i are high, kill_i takes precedence.
         // Not resetting if halted, as we might have to continue the sequence after being unhalted.
         instr_cnt_q <= '0;
         seq_state_q <= seq_state_n;
@@ -165,7 +165,7 @@ import cv32e40x_pkg::*;
   // In principle this is the same as "seq_en && valid_i"
   //   as the output of the above decode logic is equivalent to seq_en
   // todo: halting IF stage would imply !valid, can this be an issue?
-  // todo: revisit the error conditions below
+  // todo: revisit the error conditions below (they are bad for the critical path; why can't they be done in ID?)
   assign valid_o = (seq_instr != INVALID_INST) && !instr_is_ptr_i && !(instr_i.bus_resp.err || !(instr_i.mpu_status == MPU_OK)) && valid_i && !halt_i && !kill_i;
 
 
@@ -205,7 +205,7 @@ import cv32e40x_pkg::*;
     seq_state_n = seq_state_q;
     seq_last_o = 1'b0;
     // default to 1, used by IF stage regardless of seq_valid
-    // See explanation around combinatorial loops in the if_stage.sv.
+    // See explanation around combinatorial loops in the if_stage.sv. // todo: reconsider, this is not clean enough
     seq_first_o = 1'b1;
     ready_o = 1'b0;
 

sva/cv32e40x_controller_fsm_sva.sv

@@ -304,7 +304,7 @@ module cv32e40x_controller_fsm_sva
 
 // Only include following assertions if X_EXT=1
 generate;
-  if(X_EXT) begin
+  if (X_EXT) begin
 
     // Assert that a CSR instruction that is accepted by both eXtension interface and pipeline is
     // flagged as killed on the eXtension interface
@@ -360,11 +360,11 @@ generate;
         commit_kill_flag  <= 1'b0;
       end else begin
         // Clear flag if EX is killed or instruction goes to WB
-        if(ctrl_fsm_o.kill_ex || (ex_valid_i && wb_ready_i)) begin
+        if (ctrl_fsm_o.kill_ex || (ex_valid_i && wb_ready_i)) begin
           commit_valid_flag <= 1'b0;
           commit_kill_flag  <= 1'b0;
         // Set flag when commit_valid goes high
-        end else if(xif_commit_valid) begin
+        end else if (xif_commit_valid) begin
           commit_valid_flag <= 1'b1;
           commit_kill_flag  <= xif_commit_kill;
         end
@@ -441,11 +441,11 @@ endgenerate
       retire_at_error <= 1'b0;
     end else begin
       // Req, no rvalid
-      if( (m_c_obi_data_if.s_req.req && m_c_obi_data_if.s_gnt.gnt) && !m_c_obi_data_if.s_rvalid.rvalid) begin
+      if (m_c_obi_data_if.s_req.req && m_c_obi_data_if.s_gnt.gnt && !m_c_obi_data_if.s_rvalid.rvalid) begin
         // Increase outstanding counter
         outstanding_count <= outstanding_count + 2'b01;
 
-        if(outstanding_count == 2'b00) begin
+        if (outstanding_count == 2'b00) begin
           // No outstanding, assign first entry
           outstanding_type[0] <= m_c_obi_data_if.req_payload.we;
         end else begin
@@ -472,7 +472,7 @@ endgenerate
       end
 
 
-      if(m_c_obi_data_if.s_rvalid.rvalid && m_c_obi_data_if.resp_payload.err && !bus_error_latched) begin
+      if (m_c_obi_data_if.s_rvalid.rvalid && m_c_obi_data_if.resp_payload.err && !bus_error_latched) begin
         bus_error_is_write <= outstanding_count == 2'b01 ? outstanding_type[0] : outstanding_type[1];
         bus_error_latched <= 1'b1;
         retire_at_error <= wb_valid_i;
@@ -500,8 +500,8 @@ endgenerate
     if (rst_n == 1'b0) begin
       valid_cnt <= '0;
     end else begin
-      if(bus_error_latched) begin
-        if(wb_valid_i && last_op_wb_i && !ctrl_fsm_o.debug_mode && !(dcsr_i.step && !dcsr_i.stepie)) begin
+      if (bus_error_latched) begin
+        if (wb_valid_i && last_op_wb_i && !ctrl_fsm_o.debug_mode && !(dcsr_i.step && !dcsr_i.stepie)) begin
           valid_cnt <= valid_cnt + 1'b1;
         end
       end else begin
@@ -545,32 +545,34 @@ end // SMCLIC
     if (rst_n == 1'b0) begin
       first_op_done_wb <= 1'b0;
     end else begin
-      if(!first_op_done_wb) begin
-        if(wb_valid_i && ex_wb_pipe_i.first_op && !last_op_wb_i) begin
+      if (!first_op_done_wb) begin
+        if (wb_valid_i && ex_wb_pipe_i.first_op && !last_op_wb_i) begin
           first_op_done_wb <= 1'b1;
         end
       end else begin
         // first_op_done_wb is set, clear when last_op retires
-        if(wb_valid_i && last_op_wb_i) begin
+        if (wb_valid_i && last_op_wb_i) begin
           first_op_done_wb <= 1'b0;
         end
       end
     end
   end
 
+// todo: make above code look the same as below code (add kill_wb related logic)
+
   // Helper logic to track first_op and last_op through the ID stage to detect unhaltable sequences
   logic first_op_done_id;
   always_ff @(posedge clk, negedge rst_n) begin
     if (rst_n == 1'b0) begin
       first_op_done_id <= 1'b0;
     end else begin
-      if(!first_op_done_id) begin
-        if(id_valid_i && ex_ready_i && first_op_id_i && !last_op_id_i) begin
+      if (!first_op_done_id) begin
+        if (id_valid_i && ex_ready_i && first_op_id_i && !last_op_id_i) begin
           first_op_done_id <= 1'b1;
         end
       end else begin
         // first_op_done_id is set, clear when last_op retires
-        if(id_valid_i && ex_ready_i && last_op_id_i) begin
+        if (id_valid_i && ex_ready_i && last_op_id_i) begin
           first_op_done_id <= 1'b0;
         end
       end
@@ -586,21 +588,23 @@ end // SMCLIC
                     (first_op_done_wb |-> !ctrl_fsm_o.irq_ack))
     else `uvm_error("controller", "Sequence broken by interrupt")
 
+// todo: add equivalency check related to first_op_done_wb computation and sequence_interruptible
+
   a_no_sequence_nmi:
   assert property (@(posedge clk) disable iff (!rst_n)
                     (first_op_done_wb |-> !(ctrl_fsm_o.pc_set && (ctrl_fsm_o.pc_mux == PC_TRAP_NMI))))
     else `uvm_error("controller", "Sequence broken by NMI")
 
   a_no_sequence_ext_debug:
   assert property (@(posedge clk) disable iff (!rst_n)
-                    (first_op_done_wb |-> !(ctrl_fsm_o.dbg_ack && (debug_cause_q == DBG_CAUSE_HALTREQ))))
+                    (first_op_done_wb |-> !(ctrl_fsm_o.dbg_ack && (debug_cause_q == DBG_CAUSE_HALTREQ)))) // todo: timing of (debug_cause_q == DBG_CAUSE_HALTREQ) seems wrong (and whole calsuse should not be needed)
     else `uvm_error("controller", "Sequence broken by external debug")
 
   // Check that we do not allow ID stage to be halted for pending interrupts/debug if a sequence is not done
   // in the ID stage.
   a_id_stage_haltable:
   assert property (@(posedge clk) disable iff (!rst_n)
-                    (first_op_done_id |-> !id_stage_haltable))
+                    (first_op_done_id |-> !id_stage_haltable)) // todo: proof equivalency, not just implication
     else `uvm_error("controller", "id_stage_haltable not correct")
 
   // Assert that we have no pc_set in the same cycle as a CSR write in WB requires flushing of the pipeline

sva/cv32e40x_core_sva.sv

@@ -83,7 +83,7 @@ module cv32e40x_core_sva
   input mcause_t     cs_registers_mcause_q,    // From cs_registers, flopped mcause
   input mstatus_t    cs_registers_mstatus_q);
 
-if(SMCLIC) begin
+if (SMCLIC) begin
   property p_clic_mie_tieoff;
     @(posedge clk)
     |mie == 1'b0;
@@ -224,7 +224,7 @@ always_ff @(posedge clk , negedge rst_ni)
       end
       else begin
         // Disregard saved CSR due to interrupts when chekcing exceptions
-        if(!cs_registers_csr_cause_i.irq) begin
+        if (!cs_registers_csr_cause_i.irq) begin
           if (!first_cause_illegal_found && (cs_registers_csr_cause_i.exception_code == EXC_CAUSE_ILLEGAL_INSN) && ctrl_fsm.csr_save_cause) begin
             first_cause_illegal_found <= 1'b1;
             actual_illegal_mepc       <= cs_registers_mepc_n;
@@ -296,7 +296,7 @@ always_ff @(posedge clk , negedge rst_ni)
   // For checking single step, ID stage is used as it contains a 'multi_cycle_id_stall' signal.
   // This makes it easy to count misaligned LSU ins as one instruction instead of two.
   logic inst_taken;
-  assign inst_taken = id_stage_id_valid && ex_ready && if_id_pipe.last_op && !id_stage_multi_cycle_id_stall;
+  assign inst_taken = id_stage_id_valid && ex_ready && if_id_pipe.last_op && !id_stage_multi_cycle_id_stall; // todo: the && !id_stage_multi_cycle_id_stall signal should now no longer be needed
 
   // Support for single step assertion
   // In case of single step + taken interrupt, the first instruction
@@ -309,9 +309,9 @@ always_ff @(posedge clk , negedge rst_ni)
         interrupt_taken <= 1'b0;
       end
       else begin
-        if(irq_ack == 1'b1) begin
+        if (irq_ack == 1'b1) begin
           interrupt_taken <= 1'b1;
-        end else if(ctrl_debug_mode_n) begin
+        end else if (ctrl_debug_mode_n) begin
           interrupt_taken <= 1'b0;
         end
       end
@@ -327,6 +327,8 @@ always_ff @(posedge clk , negedge rst_ni)
                      |-> (ctrl_fsm.debug_mode && dcsr.step))
       else `uvm_error("core", "Assertion a_single_step_no_irq failed")
 
+// todo: add similar assertion as above to check that only one instruction moves from IF to ID while taking a single step (rename inst_taken to inst_taken_id and introduce similar inst_taken_if signal)
+
 if (SMCLIC) begin
   // Non-SHV interrupt taken during single stepping.
   // If this happens, no instructions should retire until the core is in debug mode.

sva/cv32e40x_if_stage_sva.sv

@@ -80,6 +80,5 @@ module cv32e40x_if_stage_sva
                       ctrl_fsm_i.kill_if |-> (seq_ready && !seq_valid))
         else `uvm_error("if_stage", "Kill should imply ready and not valid.")
 
-
 endmodule // cv32e40x_if_stage