Skip to content

Keeping WFI in WB until SLEEP mode is exited #667

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

Merged
merged 2 commits into from
Sep 22, 2022
Merged

Keeping WFI in WB until SLEEP mode is exited #667

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bde76cb
Keeping WFI active in WB during SLEEP state until we wake up instead …
silabs-oysteink Sep 21, 2022
1eb62f9
Updated comments, added assertions and cleaned up wb_valid naming wit…
silabs-oysteink Sep 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions bhv/cv32e40x_rvfi.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -655,7 +655,7 @@ module cv32e40x_rvfi

logic [63:0] buffer_trans_wdata_ror; // Intermediate rotate signal, as direct part-select not supported in all tools

logic wb_valid_adjusted;
logic wb_valid;
logic wb_valid_subop;

logic pc_mux_debug;
Expand Down Expand Up @@ -730,7 +730,7 @@ module cv32e40x_rvfi
logic in_trap_clr;
// Clear in trap pipeline when it reaches rvfi_intr
// This is done to avoid reporting already signaled triggers as supressed during by debug
assign in_trap_clr = wb_valid_adjusted && in_trap[STAGE_WB].intr;
assign in_trap_clr = wb_valid && in_trap[STAGE_WB].intr;

// Set rvfi_trap for instructions causing exception or debug entry.
rvfi_trap_t rvfi_trap_next;
Expand Down Expand Up @@ -788,8 +788,8 @@ module cv32e40x_rvfi
// WFI instructions retire when their wake-up condition is present.
// The wake-up condition is only checked in the SLEEP state of the controller FSM.
// Other instructions retire when their last suboperation is done in WB.
assign wb_valid_subop = (sys_en_wb_i && sys_wfi_insn_wb_i) ? (ctrl_fsm_cs_i == SLEEP) && (ctrl_fsm_ns_i == FUNCTIONAL) : wb_valid_i;
assign wb_valid_adjusted = (sys_en_wb_i && sys_wfi_insn_wb_i) ? (ctrl_fsm_cs_i == SLEEP) && (ctrl_fsm_ns_i == FUNCTIONAL) : wb_valid_i && (last_op_wb_i || abort_op_wb_i);
assign wb_valid_subop = wb_valid_i;
assign wb_valid = wb_valid_i && (last_op_wb_i || abort_op_wb_i);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we come up with a more descriptive name for wb_valid to avoid confusion with wb_valid_i ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wb_valid_last maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

wb_valid_lastop? to be consistent with wb_valid_subop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that sounds good. I'll change it.



// Pipeline stage model //
Expand Down Expand Up @@ -1002,8 +1002,8 @@ module cv32e40x_rvfi


//// WB Stage ////
rvfi_valid <= wb_valid_adjusted;
if (wb_valid_adjusted) begin
rvfi_valid <= wb_valid;
if (wb_valid) begin

rvfi_order <= rvfi_order + 64'b1;
rvfi_pc_rdata <= pc_wb_i;
Expand Down Expand Up @@ -1114,7 +1114,7 @@ module cv32e40x_rvfi
mhpmcounter_h_during_wb[i] <= 1'b0;
end else begin
// Clear flags on wb_valid
if (wb_valid_adjusted) begin
if (wb_valid) begin
mhpmcounter_l_during_wb[i] <= 1'b0;
mhpmcounter_h_during_wb[i] <= 1'b0;
end else begin
Expand Down
10 changes: 7 additions & 3 deletions rtl/cv32e40x_controller_fsm.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,9 +691,9 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
ctrl_fsm_o.csr_cause.exception_code = exception_cause_wb;
// Special insn
end else if (wfi_in_wb) begin
// Not halting EX/WB to allow insn (interruptible bubble) in EX to pass to WB before sleeping
ctrl_fsm_o.halt_if = 1'b1;
ctrl_fsm_o.halt_id = 1'b1; // Ensures second bubble after WFI (EX is empty while in SLEEP)
// Halt the entire pipeline
Copy link
Contributor

Choose a reason for hiding this comment

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

In the original design we were very explicit about not halting EX and WB and even ensuring a second bubble. Why was that originally done and why is that not needed anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the reasoning was that we wanted the WB stage to be empty to guarantee that we could interrupt without killing the WFI on wakeup. In any way (old or new) we don't take the interrupt or kill any stages before the cycle after we leave the SLEEP state, and thus the WFI instruction will finish ahead of any killing anyway. The second bubble was to avoid any issues with LSU instructions in EX while entering/being in/exiting SLEEP state. Since we halt, such instructions could in fact be allowed to enter EX and the halting would keep them from going on the bus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The WFI will get a bubble behind it because the controller_bypass logic will halt ID when the WFI is in EX. This is to ensure that we are in an interruptible state once we wake up from SLEEP (nothing should be in WB). The second bubble is needed to ensure that the instruction that follows (which will then be in ID when WFI is in WB) can perform it's side effects once we wake up in case the interrupt is actually not taken.

// WFI will stay in WB until we exit sleep mode
ctrl_fsm_o.halt_wb = 1'b1;
ctrl_fsm_o.instr_req = 1'b0;
ctrl_fsm_ns = SLEEP;
end else if (fencei_in_wb) begin
Expand Down Expand Up @@ -866,6 +866,10 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
if (ctrl_fsm_o.wake_from_sleep) begin
ctrl_fsm_ns = FUNCTIONAL;
ctrl_fsm_o.ctrl_busy = 1'b1;
// Keep IF/ID/EX halted. May possibly remove this and gain one cycle when waking up to
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the above comments (about the bubbles) as it is not accurate anymore now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comments updated

// globally disabled interrupts or WFE.
ctrl_fsm_o.halt_ex = 1'b1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment while EX still needs to be halted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove this halt_ex and explain why it is ok. I'll also add an assertion to verify that the pipeline is interruptible once we go from SLEEP to FUNCTIONAL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove this halt_ex and explain why it is ok. I'll also add an assertion to verify that the pipeline is interruptible once we go from SLEEP to FUNCTIONAL.

I will actually NOT remove this halt_ex. As explained earlier, this is needed to make sure jumps and mrets perform their jumps when we get back to FUNCTIONAL in case the interrupt is actually not taken.

ctrl_fsm_o.halt_wb = 1'b0; // Unhalt WB to allow WFI to retire when we exit SLEEP mode
end
end
DEBUG_TAKEN: begin
Expand Down
7 changes: 4 additions & 3 deletions sva/cv32e40x_controller_fsm_sva.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,11 +418,12 @@ endgenerate
(ex_wb_pipe_i.instr_valid && ex_wb_pipe_i.lsu_en) |-> !debug_allowed)
else `uvm_error("controller", "debug_allowed high while LSU is in WB")

// Ensure bubbles in EX and WB while in SLEEP mode
// Ensure bubble in EX while in SLEEP mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add comment about why this bubble is needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add assertion that there is no LSU request the cycle before entering the sleep state or while in the sleep state (this is partially implied by below assertion, but not fully). The idea is to check that no loads/stores are in progress when about to block the pipeline (which would prevent proper handling of loads/stores)

Copy link
Contributor Author

@silabs-oysteink silabs-oysteink Sep 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Comments and assertions added.

// WFI instruction will be in WB
a_wfi_bubbles:
assert property (@(posedge clk) disable iff (!rst_n)
(ctrl_fsm_cs == SLEEP) |-> !(ex_wb_pipe_i.instr_valid || id_ex_pipe_i.instr_valid))
else `uvm_error("controller", "EX and WB stages not empty while in SLEEP state")
(ctrl_fsm_cs == SLEEP) |-> !(id_ex_pipe_i.instr_valid))
else `uvm_error("controller", "EX stage not empty while in SLEEP state")

// Assert correct exception cause for mpu load faults (checks default of cause mux)
a_mpu_re_cause_mux:
Expand Down