Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Changed logic for sequence progress detection #638

Merged
merged 3 commits into from Aug 5, 2022
Merged

Changed logic for sequence progress detection #638

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aefdf1e
Changed mid-sequence detection. Controller tracks sequence state of W…
silabs-oysteink Aug 4, 2022
d47b51d
Turned off ZC_EXT as it should stay off until Zc is complete.
silabs-oysteink Aug 5, 2022
7723120
Updates after review. Comments and renameing of port in controller_sva.
silabs-oysteink Aug 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 11 additions & 7 deletions bhv/cv32e40x_wrapper.sv
View file
Open in desktop
Expand Up @@ -203,13 +203,17 @@ module cv32e40x_wrapper
cv32e40x_controller_fsm_sva
#(.X_EXT(X_EXT))
controller_fsm_sva (
.lsu_outstanding_cnt (core_i.load_store_unit_i.cnt_q),
.rf_we_wb_i (core_i.wb_stage_i.rf_we_wb_o ),
.csr_we_i (core_i.cs_registers_i.csr_we_int ),
.csr_illegal_i (core_i.cs_registers_i.csr_illegal_o),
.xif_commit_kill (core_i.xif_commit_if.commit.commit_kill),
.xif_commit_valid (core_i.xif_commit_if.commit_valid),
.last_op_id_i (core_i.if_id_pipe.last_op),
.lsu_outstanding_cnt (core_i.load_store_unit_i.cnt_q),
.rf_we_wb_i (core_i.wb_stage_i.rf_we_wb_o ),
.csr_we_i (core_i.cs_registers_i.csr_we_int ),
.csr_illegal_i (core_i.cs_registers_i.csr_illegal_o),
.xif_commit_kill (core_i.xif_commit_if.commit.commit_kill),
.xif_commit_valid (core_i.xif_commit_if.commit_valid),
.last_op_id_i (core_i.if_id_pipe.last_op),
.first_op_if_i (core_i.first_op_if),
.first_op_ex_i (core_i.first_op_ex),
.prefetch_valid_if_i (core_i.if_stage_i.prefetch_valid),
.prefetch_is_tbljmp_ptr_if_i (core_i.if_stage_i.prefetch_is_tbljmp_ptr),
.*);
bind cv32e40x_cs_registers: core_i.cs_registers_i cv32e40x_cs_registers_sva #(.SMCLIC(SMCLIC)) cs_registers_sva (.*);

Expand Down
6 changes: 2 additions & 4 deletions rtl/cv32e40x_controller.sv
View file
Open in desktop
Expand Up @@ -47,7 +47,6 @@ module cv32e40x_controller import cv32e40x_pkg::*;

// From IF stage
input logic [31:0] pc_if_i,
input logic first_op_if_i,
input logic last_op_if_i,

// from IF/ID pipeline
Expand All @@ -60,9 +59,9 @@ module cv32e40x_controller import cv32e40x_pkg::*;
input logic csr_en_raw_id_i,
input csr_opcode_e csr_op_id_i,
input logic first_op_id_i,
input logic last_op_id_i,

input id_ex_pipe_t id_ex_pipe_i,
input logic first_op_ex_i,

input ex_wb_pipe_t ex_wb_pipe_i,

Expand Down Expand Up @@ -146,7 +145,6 @@ module cv32e40x_controller import cv32e40x_pkg::*;

.if_valid_i ( if_valid_i ),
.pc_if_i ( pc_if_i ),
.first_op_if_i ( first_op_if_i ),
.last_op_if_i ( last_op_if_i ),

// From ID stage
Expand All @@ -158,14 +156,14 @@ module cv32e40x_controller import cv32e40x_pkg::*;
.alu_en_id_i ( alu_en_id_i ),
.sys_en_id_i ( sys_en_id_i ),
.first_op_id_i ( first_op_id_i ),
.last_op_id_i ( last_op_id_i ),

// From EX stage
.id_ex_pipe_i ( id_ex_pipe_i ),
.branch_decision_ex_i ( branch_decision_ex_i ),
.ex_ready_i ( ex_ready_i ),
.ex_valid_i ( ex_valid_i ),
.last_op_ex_i ( last_op_ex_i ),
.first_op_ex_i ( first_op_ex_i ),

// From WB stage
.ex_wb_pipe_i ( ex_wb_pipe_i ),
Expand Down
85 changes: 54 additions & 31 deletions rtl/cv32e40x_controller_fsm.sv
View file
Open in desktop
Expand Up @@ -48,7 +48,6 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;

// From IF stage
input logic [31:0] pc_if_i,
input logic first_op_if_i, // IF stage is handling the first operation of a sequence.
input logic last_op_if_i, // IF stage is handling the last operation of a sequence.

// From ID stage
Expand All @@ -58,12 +57,12 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
input logic alu_en_id_i, // alu_en qualifier for jumps
input logic sys_en_id_i, // sys_en qualifier for mret
input logic first_op_id_i, // ID stage is handling the first operation of a sequence
input logic last_op_id_i, // ID stage is handling the last operation of a sequence

// From EX stage
input id_ex_pipe_t id_ex_pipe_i,
input logic branch_decision_ex_i, // branch decision signal from EX ALU
input logic last_op_ex_i, // EX stage contains the last operation of an instruction
input logic first_op_ex_i, // EX stage is handling the first operation of a sequence

// From WB stage
input ex_wb_pipe_t ex_wb_pipe_i,
Expand Down Expand Up @@ -218,12 +217,18 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
logic csr_flush_ack_q;

// Flag for checking if multi op instructions are in an interruptible state
logic sequence_in_progress_wb;
logic sequence_interruptible;

// Flag for checking if ID stage can be halted
// Used to not halt sequences in the middle, potentially causing deadlocks
logic sequence_in_progress_id;
logic id_stage_haltable;

assign sequence_interruptible = !sequence_in_progress_wb;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!


assign id_stage_haltable = !sequence_in_progress_id;

assign fencei_ready = !lsu_busy_i;

// Once the fencei handshake is initiated, it must complete and the instruction must retire.
Expand Down Expand Up @@ -463,36 +468,7 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
assign ctrl_fsm_o.mhpmevent.id_ld_stall = ctrl_byp_i.load_stall && !id_valid_i && ex_ready_i;
assign ctrl_fsm_o.mhpmevent.wb_data_stall = data_stall_wb_i;

// Detect if we are in the middle of a multi operation sequence
// Basically check if the oldest instruction in the pipeline is a 'first_op'
// - if not, we have an unfinished sequence and cannot interrupt it.
// Used to determine if we are allowed to take debug or interrupts
always_comb begin
if (ex_wb_pipe_i.instr_valid) begin
sequence_interruptible = ex_wb_pipe_i.first_op;
end else if (id_ex_pipe_i.instr_valid) begin
sequence_interruptible = first_op_ex_i;
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; // todo: first/last op should always be classified with instr_vali; if not it needs proper explanation.
end
end

// Check if we are allowed to halt the ID stage.
// If ID stage contains a non-first operation, we cannot halt ID as that
// could cause a deadlock because a sequence cannot finish through ID.
// If ID stage does not contain a valid instruction, the same check is performed
// for the IF stage (although this is likely not needed).
always_comb begin
if (if_id_pipe_i.instr_valid) begin
id_stage_haltable = first_op_id_i;
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; // 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

// Mux used to select PC from the different pipeline stages
always_comb begin
Expand Down Expand Up @@ -1082,6 +1058,53 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
end
end

// Instruction sequences may not be interrupted/killed if the first operation has already been retired.
// Flop set when first_op is done in WB, and cleared when last_op is done, or WB is killed
always_ff @(posedge clk, negedge rst_n) begin
if (rst_n == 1'b0) begin
sequence_in_progress_wb <= 1'b0;
end else begin
if (!sequence_in_progress_wb) begin
if (wb_valid_i && ex_wb_pipe_i.first_op && !last_op_wb_i) begin // wb_valid implies ex_wb_pipe.instr_valid
sequence_in_progress_wb <= 1'b1;
end
end else begin
// sequence_in_progress_wb is set, clear when last_op retires
if (wb_valid_i && last_op_wb_i) begin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reply as above

sequence_in_progress_wb <= 1'b0;
end
// Needed in the future, but commented out to make a SEC clean PR.
//if (ctrl_fsm_o.kill_wb) begin
// sequence_in_progress_wb <= 1'b0;
//end
end
end
end

// Logic to track first_op and last_op through the ID stage to detect unhaltable sequences
// Flop set when first_op is done in ID, and cleared when last_op is done, or ID is killed
always_ff @(posedge clk, negedge rst_n) begin
if (rst_n == 1'b0) begin
sequence_in_progress_id <= 1'b0;
end else begin
if (!sequence_in_progress_id) begin
if (id_valid_i && ex_ready_i && first_op_id_i && !last_op_id_i) begin // id_valid implies if_id_pipe.instr.valid
sequence_in_progress_id <= 1'b1;
end
end else begin
// sequence_in_progress_id is set, clear when last_op retires
if (id_valid_i && ex_ready_i && last_op_id_i) begin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remarks as above related to usage of isntr_valid. Is adding the instr_valid signals a SEC clean change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would likely be SEC clean, but as for the others the instr_valid is baked into the valid

sequence_in_progress_id <= 1'b0;
end
end

// Reset flag if ID stage is killed
if (ctrl_fsm_o.kill_id) begin
sequence_in_progress_id <= 1'b0;
end
end
end


/////////////////////
// Debug state FSM //
Expand Down
5 changes: 3 additions & 2 deletions rtl/cv32e40x_core.sv
View file
Open in desktop
Expand Up @@ -181,6 +181,7 @@ module cv32e40x_core import cv32e40x_pkg::*;

// Detect last_op
logic last_op_if;
logic last_op_id;
logic last_op_ex;
logic last_op_wb;

Expand Down Expand Up @@ -506,6 +507,7 @@ module cv32e40x_core import cv32e40x_pkg::*;
.sys_en_o ( sys_en_id ),

.first_op_o ( first_op_id ),
.last_op_o ( last_op_id ),

.rf_re_o ( rf_re_id ),
.rf_raddr_o ( rf_raddr_id ),
Expand Down Expand Up @@ -800,7 +802,6 @@ module cv32e40x_core import cv32e40x_pkg::*;

// From ID/EX pipeline
.id_ex_pipe_i ( id_ex_pipe ),
.first_op_ex_i ( first_op_ex ),

.csr_counter_read_i ( csr_counter_read ),
.csr_mnxti_read_i ( csr_mnxti_read ),
Expand All @@ -809,12 +810,12 @@ module cv32e40x_core import cv32e40x_pkg::*;
.ex_wb_pipe_i ( ex_wb_pipe ),

// last_op bits
.last_op_id_i ( last_op_id ),
.last_op_ex_i ( last_op_ex ),
.last_op_wb_i ( last_op_wb ),

.if_valid_i ( if_valid ),
.pc_if_i ( pc_if ),
.first_op_if_i ( first_op_if ),
.last_op_if_i ( last_op_if ),
// from IF/ID pipeline
.if_id_pipe_i ( if_id_pipe ),
Expand Down
2 changes: 2 additions & 0 deletions rtl/cv32e40x_id_stage.sv
View file
Open in desktop
Expand Up @@ -77,6 +77,7 @@ module cv32e40x_id_stage import cv32e40x_pkg::*;
output logic sys_en_o,

output logic first_op_o,
output logic last_op_o,

// RF interface -> controller
output logic [REGFILE_NUM_READ_PORTS-1:0] rf_re_o,
Expand Down Expand Up @@ -679,6 +680,7 @@ module cv32e40x_id_stage import cv32e40x_pkg::*;
assign id_valid_o = (instr_valid && !xif_waiting) || (multi_cycle_id_stall && !ctrl_fsm_i.kill_id && !ctrl_fsm_i.halt_id);

assign first_op_o = if_id_pipe_i.first_op;
assign last_op_o = if_id_pipe_i.last_op;
//---------------------------------------------------------------------------
// eXtension interface
//---------------------------------------------------------------------------
Expand Down
5 changes: 2 additions & 3 deletions rtl/cv32e40x_if_stage.sv
View file
Open in desktop
Expand Up @@ -291,8 +291,6 @@ module cv32e40x_if_stage import cv32e40x_pkg::*;

// Acknowledge prefetcher when IF stage is ready. This factors in seq_ready to avoid ack'ing the
// prefetcher in the middle of a Zc sequence.
// No need to ack the prefetcher when tbljmp==1, as this is will generate a pc_set and kill_if when
// the table jump is taken in ID and the pointer fetch is initiated (or the table jump is killed).
assign prefetch_ready = if_ready && !tbljmp;

// Last operation of table jumps are set when the pointer is fed to ID stage
Expand All @@ -315,7 +313,8 @@ module cv32e40x_if_stage import cv32e40x_pkg::*;
// 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;
trigger_match_i ? 1'b1 :
seq_valid ? seq_first : 1'b1;

// 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.

Expand Down