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.

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

Implemented custom WFE instruction #669

Merged
merged 7 commits into from
Sep 28, 2022

Conversation

silabs-oysteink
Copy link
Contributor

Instruction behaves exactly as WFI, but can wake up to the new input wu_wfe_i in addition to interrupts and debug. WFI will never wake up due to wu_wfe_i.

misa.X is set to 1. Note that this seems to break the ISS step and compare - we might want to wait with this merge until verification can verify this behaviour and come up with a fix that avoids breaking regressions.

@silabs-oysteink silabs-oysteink added the Component:RTL For issues in the RTL (e.g. for files in the rtl directory) label Sep 22, 2022
@@ -138,7 +138,7 @@ module cv32e40x_controller_bypass import cv32e40x_pkg::*;

// Stall ID when WFI is active in EX.
// Prevent load/store following a WFI in the pipeline
assign ctrl_byp_o.wfi_stall = (id_ex_pipe_i.sys_en && id_ex_pipe_i.sys_wfi_insn && id_ex_pipe_i.instr_valid);
assign ctrl_byp_o.wfi_stall = (id_ex_pipe_i.sys_en && (id_ex_pipe_i.sys_wfi_insn || id_ex_pipe_i.sys_wfe_insn) && id_ex_pipe_i.instr_valid);
Copy link
Contributor

Choose a reason for hiding this comment

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

wfi_stall -> wfi_wfe_stall

Copy link
Contributor

Choose a reason for hiding this comment

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

Update comments as well.

@@ -991,7 +998,8 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
end

// Wakeup from sleep
assign ctrl_fsm_o.wake_from_sleep = irq_wu_ctrl_i || pending_debug || debug_mode_q;
assign ctrl_fsm_o.wake_from_sleep = irq_wu_ctrl_i || pending_debug || debug_mode_q ||
(ex_wb_pipe_i.sys_en && ex_wb_pipe_i.sys_wfe_insn && ex_wb_pipe_i.instr_valid && wu_wfe_i); // Only WFE wakes up for wfe_wu_i
Copy link
Contributor

Choose a reason for hiding this comment

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

(ex_wb_pipe_i.sys_en && ex_wb_pipe_i.sys_wfe_insn && ex_wb_pipe_i.instr_valid && wu_wfe_i) ->(wfe_in_wb && wu_wfe_i)

@@ -991,7 +998,8 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
end

// Wakeup from sleep
assign ctrl_fsm_o.wake_from_sleep = irq_wu_ctrl_i || pending_debug || debug_mode_q;
assign ctrl_fsm_o.wake_from_sleep = irq_wu_ctrl_i || pending_debug || debug_mode_q ||
(ex_wb_pipe_i.sys_en && ex_wb_pipe_i.sys_wfe_insn && ex_wb_pipe_i.instr_valid && wu_wfe_i); // Only WFE wakes up for wfe_wu_i
Copy link
Contributor

Choose a reason for hiding this comment

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

debug_wfi_no_sleep -> debug_wfi_wfe_no_sleep

@@ -43,6 +43,7 @@ module cv32e40x_decoder import cv32e40x_pkg::*;
output logic sys_dret_insn_o, // Return from debug (M)
output logic sys_ecall_insn_o, // Environment call (syscall) instruction encountered
output logic sys_wfi_insn_o, // Pipeline flush is requested
output logic sys_wfe_insn_o,
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove above flush comment (not accurate)

assign sys_mret_insn_o = decoder_i_ctrl.sys_mret_insn; // Only I decoder handles SYS
assign sys_dret_insn_o = decoder_i_ctrl.sys_dret_insn; // Only I decoder handles SYS
assign sys_ebrk_insn_o = decoder_i_ctrl.sys_ebrk_insn; // Only I decoder handles SYS
assign sys_ecall_insn_o = decoder_i_ctrl.sys_ecall_insn; // Only I decoder handles SYS
assign sys_wfi_insn_o = decoder_i_ctrl.sys_wfi_insn; // Only I decoder handles SYS
assign sys_wfe_insn_o = decoder_x_ctrl.sys_wfe_insn; // Only X decoder handles WFE
Copy link
Contributor

Choose a reason for hiding this comment

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

Update all above comments similarly, e.g. on line 236:

// Only I decoder handles MRET

@@ -219,12 +232,13 @@ module cv32e40x_decoder import cv32e40x_pkg::*;
assign lsu_size_o = decoder_ctrl_mux.lsu_size;
assign lsu_sext_o = decoder_ctrl_mux.lsu_sext;
assign lsu_atop_o = decoder_a_ctrl.lsu_atop; // Only A decoder handles atomics
assign sys_en = decoder_i_ctrl.sys_en; // Only I decoder handles SYS
assign sys_en = decoder_ctrl_mux.sys_en;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you do a synthesis and see if this change led to unwanted timing paths (as that was the reason to use decoder_i_ctrl instead of decoder_ctrl_mux). If the wfe related path shows up, then we should make WFE part if the I decoder instead (although it is very clean how you did it now)

module cv32e40x_x_decoder import cv32e40x_pkg::*;
(
// from IF/ID pipeline
input logic [31:0] instr_rdata_i,
Copy link
Contributor

Choose a reason for hiding this comment

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

Align types and signal names

decoder_ctrl_o = DECODER_CTRL_ILLEGAL_INSN;
decoder_ctrl_o.illegal_insn = 1'b0;

unique case (instr_rdata_i[6:0])
Copy link
Contributor

Choose a reason for hiding this comment

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

Just compare all 32 bits at once against 32'h8C000073 to save some lines of code

Copy link
Contributor

Choose a reason for hiding this comment

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

Another PR needs to be done for the 40S to add privilege related code as for WFI.

// Bubble is needed to avoid any LSU instructions to go on the bus while handling the WFI, as this
// could cause the pipeline not to be interruptible when we wake up to an interrupt that should be taken.
a_wfi_bubbles:
assert property (@(posedge clk) disable iff (!rst_n)
(ctrl_fsm_cs == SLEEP) |-> !(id_ex_pipe_i.instr_valid))
(ctrl_fsm_cs == SLEEP)
Copy link
Contributor

Choose a reason for hiding this comment

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

a_wfi_bubbles -> a_wfi_wfe_bubbles

@@ -138,10 +138,10 @@ module cv32e40x_decoder_sva
else `uvm_error("decoder", "CSR related signals driven from unexpected decoder")

// Check that SYS related signals can be used from I decoder directly (bypassing other decoders)
// Note that with the WFE instruction, sys_en may be driven by both I-decoder and X-decoder.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add similar assertion for sys_wfe_insn

…input pin in addition to interrupts.

Signed-off-by: Oystein Knauserud <[email protected]>
…sa.X=1.

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

Cleanup of indentation and removed commented assertion code.

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

Added missing file for custom decoder.

Signed-off-by: Oystein Knauserud <[email protected]>
…lean change.

Updated some assertions.

Signed-off-by: Oystein Knauserud <[email protected]>
Signed-off-by: Oystein Knauserud <[email protected]>
@Silabs-ArjanB Silabs-ArjanB merged commit 707899d into openhwgroup:master Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component:RTL For issues in the RTL (e.g. for files in the rtl directory)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants