From 6da6eb9e4ee2ac5f96d5bed40c4c46d57a64c79f Mon Sep 17 00:00:00 2001
From: Rutherther <rutherther@proton.me>
Date: Fri, 15 Dec 2023 19:27:21 +0100
Subject: [PATCH] docs: better document the stage code, organize it better

---
 src/cpu.sv                  | 10 ++++++++++
 src/forwarder.sv            |  1 +
 src/stages/decode.sv        | 10 ++++++----
 src/stages/execute.sv       | 12 +++++++-----
 src/stages/fetch.sv         |  3 ++-
 src/stages/memory_access.sv |  3 ++-
 6 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/src/cpu.sv b/src/cpu.sv
index 9446adb..2ab97b8 100755
--- a/src/cpu.sv
+++ b/src/cpu.sv
@@ -33,6 +33,12 @@ module cpu(
   wire [31:0] jumping_pc_next;
 
   stage_status_t stages_in[1:4];
+
+  /// verilator doesn't like that data taken from stages_out[i]
+  // are used in stages_out[i + 1]. But that shouldn't really matter
+  // as there is not really a cyclic dependency.
+  // It just seems that verilator is not very good at "separating"
+  // array elements
 /* verilator lint_off UNOPTFLAT */
   stage_status_t stages_out[0:3];
 /* verilator lint_on UNOPTFLAT */
@@ -76,6 +82,10 @@ module cpu(
       pc_next = pc;
   end
 
+  // data for forwarding from the stages
+  // Note: this is a record instead of an array
+  // just because verilator didn't like it as an array
+  // consider switching back to array.
   forwarding_data_status_t data_in_pipeline;
   assign data_in_pipeline.execute_out = stages_out[EXECUTE].data;
   assign data_in_pipeline.access_out = stages_out[ACCESS].data;
diff --git a/src/forwarder.sv b/src/forwarder.sv
index 5e3b5b8..20b35c0 100644
--- a/src/forwarder.sv
+++ b/src/forwarder.sv
@@ -14,6 +14,7 @@ module forwarder(
   // these will be used instead of the register_file_data
   //
   // if there are multiple matches, the first one is taken
+  // to get the most recent data
 
   always_comb begin
     stall = 0;
diff --git a/src/stages/decode.sv b/src/stages/decode.sv
index 7a99c4f..e832a67 100644
--- a/src/stages/decode.sv
+++ b/src/stages/decode.sv
@@ -17,8 +17,6 @@ module decode(
   output        stage_status_t stage_out
 );
 
-  parameter FORWARDING_STAGES = 3; // , execute(out), memory(out), writeback(in)
-
   wire [2:0] alu_op;
   wire       alu_add_one;
   wire       alu_negate;
@@ -42,6 +40,7 @@ module decode(
   wire        memory_we;
 
   assign stage_out.data.address = reg_we && !stalling ? reg_rd : 0;
+  assign stage_out.data.valid = 0; // the data cannot be valid at this point;
 
   assign stage_out.pc = stage_in.pc;
 
@@ -110,6 +109,8 @@ module decode(
     .data(forwarded_reg_rd2)
   );
 
+  // TODO: this is there twice instead of just once
+  // the second is in execute stage. Maybe merge these?
   // alu source 1
   reg [31:0] alu_1, alu_2;
   always_comb begin
@@ -158,6 +159,7 @@ module decode(
 
   wire stalling;
   assign stalling = (uses_reg_rd1 && stall_1) || (uses_reg_rd2 && stall_2);
-  assign stage_out.valid = !stalling;
-  assign stage_out.ready = !stalling;
+  assign stage_out.valid = !stalling && stage_in.valid;
+  assign stage_out.ready = !stalling || !stage_in.valid;
+    // if input is not valid, do not care about stalling...
 endmodule
diff --git a/src/stages/execute.sv b/src/stages/execute.sv
index a7a921e..fd3702f 100644
--- a/src/stages/execute.sv
+++ b/src/stages/execute.sv
@@ -9,15 +9,17 @@ module execute(
   reg [31:0] alu_1, alu_2;
   wire [31:0] alu_out;
 
+  assign stage_out.instruction = stage_in.instruction;
+  assign stage_out.pc = stage_in.pc;
   assign stage_out.reg_rd1 = stage_in.reg_rd1;
   assign stage_out.reg_rd2 = stage_in.reg_rd2;
-  assign stage_out.pc = stage_in.pc;
-  assign stage_out.instruction = stage_in.instruction;
-  assign stage_out.valid = stage_in.valid;
-  assign stage_out.ready = 1;
-  assign stage_out.data.valid = stage_in.valid && (stage_in.instruction.reg_rd_src != RD_MEMORY);
+
   assign stage_out.data.address = stage_in.valid ? stage_in.data.address : 0;
   assign stage_out.data.data = stage_in.instruction.reg_rd_src == RD_PC_PLUS ? stage_in.pc + 4 : alu_out;
+  assign stage_out.data.valid = stage_in.valid && (stage_in.instruction.reg_rd_src != RD_MEMORY);
+
+  assign stage_out.valid = stage_in.valid;
+  assign stage_out.ready = 1;
 
   // alu source 1
   always_comb begin
diff --git a/src/stages/fetch.sv b/src/stages/fetch.sv
index c455f27..11bcf95 100644
--- a/src/stages/fetch.sv
+++ b/src/stages/fetch.sv
@@ -9,7 +9,8 @@ module fetch(
   output       stage_status_t stage_out
 );
   assign stage_out.instruction.instruction = mem_instruction;
-  assign stage_out.valid = !jump;
   assign stage_out.pc = pc;
+
+  assign stage_out.valid = !jump;
   assign stage_out.ready = 1;
 endmodule
diff --git a/src/stages/memory_access.sv b/src/stages/memory_access.sv
index 6b7e4d5..054fc63 100644
--- a/src/stages/memory_access.sv
+++ b/src/stages/memory_access.sv
@@ -49,6 +49,7 @@ module memory_access(
   assign stage_out.reg_rd1 = stage_in.reg_rd1;
   assign stage_out.reg_rd2 = stage_in.reg_rd2;
 
+  assign stage_out.data.valid = stage_in.valid;
   assign stage_out.data.address = stage_in.valid ? stage_in.data.address : 0;
   assign stage_out.data.data =
     stage_in.instruction.reg_rd_src == RD_MEMORY ?
@@ -58,7 +59,7 @@ module memory_access(
             .sext(stage_in.instruction.memory_sign_extension)
         ) :
         stage_in.data.data;
-  assign stage_out.data.valid = stage_in.valid;
+
   assign stage_out.valid = stage_in.valid;
   assign stage_out.ready = 1;
 endmodule
-- 
2.48.1