Keeping WFI in WB until SLEEP mode is exited #667

silabs-oysteink · 2022-09-21T09:12:59Z

Keeping WFI active in WB during SLEEP state until we wake up instead of retiring (wb_valid) the WFI when the core enters SLEEP.
This allows cleanup of the wb_valid/rvfi_valid signals inside RVFI as well.

SEC clean when assuming correct OBI (no rvalid before req)

Prerequisite to adding the WFE instruction.

Signed-off-by: Oystein Knauserud [email protected]

…of retiring (wb_valid) the WFI when the core enters SLEEP. This allows cleanup of the wb_valid/rvfi_valid signals inside RVFI as well. SEC clean when assuming correct OBI (no rvalid before req) Signed-off-by: Oystein Knauserud <[email protected]>

silabs-oivind · 2022-09-21T12:39:21Z

bhv/cv32e40x_rvfi.sv

-  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);


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

wb_valid_last maybe?

wb_valid_lastop? to be consistent with wb_valid_subop

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

Silabs-ArjanB · 2022-09-22T04:20:11Z

rtl/cv32e40x_controller_fsm.sv

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


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?

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.

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.

Silabs-ArjanB · 2022-09-22T04:24:42Z

rtl/cv32e40x_controller_fsm.sv

@@ -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


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

Comments updated

Silabs-ArjanB · 2022-09-22T04:25:43Z

rtl/cv32e40x_controller_fsm.sv

@@ -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
+          // globally disabled interrupts or WFE.
+          ctrl_fsm_o.halt_ex = 1'b1;


Add a comment while EX still needs to be halted.

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

Silabs-ArjanB · 2022-09-22T04:26:24Z

sva/cv32e40x_controller_fsm_sva.sv

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


Add comment about why this bubble is needed.

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)

Comments and assertions added.

…hin RVFI. Signed-off-by: Oystein Knauserud <[email protected]>

silabs-oivind · 2022-09-22T09:51:37Z

sva/cv32e40x_controller_fsm_sva.sv

+                      !lsu_trans_valid_i)
+      else `uvm_error("controller", "LSU trans_valid high when entering SLEEP")
+
+  // When in SLEEP mode, no LSU should perform a request.


Can these two asserts be combined? i.e. change the antecedent to (ctrl_fsm_cs==SLEEP) || (ctrl_fsm_ns==SLEEP)

Yes, I can combine them in the upcoming WFE PR.

silabs-oysteink requested review from Silabs-ArjanB and silabs-oivind September 21, 2022 09:13

silabs-oivind reviewed Sep 21, 2022

View reviewed changes

silabs-oysteink added the Component:RTL For issues in the RTL (e.g. for files in the rtl directory) label Sep 21, 2022

Silabs-ArjanB requested changes Sep 22, 2022

View reviewed changes

Updated comments, added assertions and cleaned up wb_valid naming wit…

1eb62f9

…hin RVFI. Signed-off-by: Oystein Knauserud <[email protected]>

silabs-oivind reviewed Sep 22, 2022

View reviewed changes

silabs-oivind approved these changes Sep 22, 2022

View reviewed changes

silabs-oysteink merged commit a1ca4fc into openhwgroup:master Sep 22, 2022

Keeping WFI in WB until SLEEP mode is exited #667

Keeping WFI in WB until SLEEP mode is exited #667

Uh oh!

Conversation

silabs-oysteink commented Sep 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silabs-oysteink Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

silabs-oysteink Sep 22, 2022 •

edited

Loading