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

Fix for issues #365 and #361 on cv32e40s. #749

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions bhv/cv32e40x_wrapper.sv
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@ module cv32e40x_wrapper
.ptr_in_if_i (core_i.if_stage_i.ptr_in_if_o),
.instr_req_o (core_i.instr_req_o),
.instr_dbg_o (core_i.instr_dbg_o),
.mstatus_i (core_i.cs_registers_i.mstatus_rdata),
.*);
bind cv32e40x_cs_registers:
core_i.cs_registers_i
Expand Down
2 changes: 1 addition & 1 deletion rtl/cv32e40x_clic_int_controller.sv
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ module cv32e40x_clic_int_controller import cv32e40x_pkg::*;
///////////////////////////

// The outputs for mnxti will only be used within cs_registers when a CSR instruction is accessing mnxti

// The mxnti path to interrupts does not take mstatus.mie or dcsr.stepie into account.
assign mnxti_irq_pending_o = clic_irq_q &&
(clic_irq_level_q > mcause_i.mpil) &&
(clic_irq_level_q > mintthresh_i) &&
Expand Down
26 changes: 23 additions & 3 deletions rtl/cv32e40x_controller_fsm.sv
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,10 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
logic [2:0] debug_cause_n;
logic [2:0] debug_cause_q;

// Flop for remembering causes of wakeup
logic woke_to_debug_q;
logic woke_to_interrupt_q;

logic [10:0] exc_cause; // id of taken interrupt. Max width, unused bits are tied off.

logic fence_req_set;
Expand Down Expand Up @@ -474,7 +478,9 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
// timing is considered haltreq should be considered only asserted on the following
// instruction (i.e. the asynchronous haltreq signal is considered asserted too late to
// impact the current instruction in the pipeline).
assign async_debug_allowed = lsu_interruptible_i && !fencei_ongoing && !xif_in_wb && !clic_ptr_in_pipeline && sequence_interruptible;
// If the core woke up from sleep due to interrupts, the wakeup reason will be honored
// by not allowing async debug the cycle after wakeup.
assign async_debug_allowed = lsu_interruptible_i && !fencei_ongoing && !xif_in_wb && !clic_ptr_in_pipeline && sequence_interruptible && !woke_to_interrupt_q;

// synchronous debug entry have far fewer restrictions than asynchronous entries. In principle synchronous debug entry should have the same
// 'allowed' signal as exceptions - that is it should always be possible.
Expand All @@ -491,7 +497,9 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
// that can currently cause a deadlock if debug_req_i gets asserted while in debug mode, as
// a pending but not allowed async debug will cause the ID stage to halt forever while trying
// to get to an interruptible state.
assign pending_async_debug = debug_req_i && !debug_mode_q;
// When the core wakes up from sleep due to debug_req_i, woke_to_debug_q will be set for exactly one cycle
// to allow the core to enter debug one cycle after waking up, even though debug_req_i may have been deasserted.
assign pending_async_debug = (debug_req_i || woke_to_debug_q) && !debug_mode_q;

// Determine cause of debug. Set for all causes of debug entry.
// In case of ebreak during debug mode, the entry code in DEBUG_TAKEN will
Expand Down Expand Up @@ -532,7 +540,9 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
assign interrupt_allowed = lsu_interruptible_i && debug_interruptible && !fencei_ongoing && !xif_in_wb && !clic_ptr_in_pipeline && sequence_interruptible && !interrupt_blanking_q;

// Allowing NMI's follow the same rule as regular interrupts, except we don't need to regard blanking of NMIs after a load/store.
assign nmi_allowed = lsu_interruptible_i && debug_interruptible && !fencei_ongoing && !xif_in_wb && !clic_ptr_in_pipeline && sequence_interruptible;
// If the core woke up from sleep due to either debug or regular interrupts, the wakeup reason is honored by not allowing NMIs in the cycle after
// waking up to such an event.
assign nmi_allowed = lsu_interruptible_i && debug_interruptible && !fencei_ongoing && !xif_in_wb && !clic_ptr_in_pipeline && sequence_interruptible && !(woke_to_debug_q || woke_to_interrupt_q);

// Do not allow interrupts if in debug mode, or single stepping without dcsr.stepie set.
assign debug_interruptible = !(debug_mode_q || (dcsr_i.step && !dcsr_i.stepie));
Expand Down Expand Up @@ -1332,6 +1342,16 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
end
end

// Flags for remembering if the core woke up due to a debug request or interrupt.
always_ff @(posedge clk, negedge rst_n) begin
if (rst_n == 1'b0) begin
woke_to_debug_q <= 1'b0;
woke_to_interrupt_q <= 1'b0;
end else begin
woke_to_debug_q <= (ctrl_fsm_cs == SLEEP) && debug_req_i;
woke_to_interrupt_q <= (ctrl_fsm_cs == SLEEP) && irq_wu_ctrl_i;
end
end

/////////////////////
// Debug state FSM //
Expand Down
61 changes: 57 additions & 4 deletions sva/cv32e40x_controller_fsm_sva.sv
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,11 @@ module cv32e40x_controller_fsm_sva
input logic debug_req_i,
input logic fetch_enable_i,
input logic instr_req_o,
input logic instr_dbg_o
input logic instr_dbg_o,
input logic wfe_in_wb,
input mstatus_t mstatus_i,
input logic woke_to_interrupt_q,
input logic woke_to_debug_q
);


Expand Down Expand Up @@ -288,9 +292,11 @@ module cv32e40x_controller_fsm_sva
else `uvm_error("controller", "Fencei handshake active while lsu_busy_o = 1")

// assert that NMI's are not reported on irq_ack
// Exception for the case where the core wakes from SLEEP due to an interrupt
// - in that case the interrupt is honored while there may be a pending nmi.
a_irq_ack_no_nmi :
assert property (@(posedge clk) disable iff (!rst_n)
ctrl_fsm_o.irq_ack |-> !pending_nmi)
ctrl_fsm_o.irq_ack |-> !(pending_nmi && !woke_to_interrupt_q))
else `uvm_error("controller", "irq_ack set while there's a pending NMI")

// Assert that intr_taken is always single cycle. I.e. no double counting
Expand Down Expand Up @@ -875,8 +881,55 @@ end
(abort_op_wb_i && (ctrl_fsm_ns == DEBUG_TAKEN)))
else `uvm_error("controller", "Debug not entered on a WPT match")



// Ensure debug mode is entered if woken up by a debug request
a_sleep_to_debug:
assert property (@(posedge clk) disable iff (!rst_n)
(ctrl_fsm_cs == SLEEP) &&
(ctrl_fsm_ns == FUNCTIONAL) &&
debug_req_i &&
!(pending_nmi || irq_wu_ctrl_i || (wfe_in_wb && wu_wfe_i))
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be logical to add && debug_req_i 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.

Agreed, adding it

|=>
(ctrl_fsm_ns == DEBUG_TAKEN))
else `uvm_error("controller", "Woke from sleep due to debug_req but debug mode not entered")

// Ensure interrupt is taken if woken up by an interrupt
a_sleep_to_irq:
assert property (@(posedge clk) disable iff (!rst_n)
(ctrl_fsm_cs == SLEEP) &&
(ctrl_fsm_ns == FUNCTIONAL) &&
irq_wu_ctrl_i &&
!(pending_nmi || debug_req_i || (wfe_in_wb && wu_wfe_i)) &&
mstatus_i.mie
Copy link
Contributor

Choose a reason for hiding this comment

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

&& irq_wu_ctrl_i seems more logical than the currently used && mstatus_i.mie

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree that it is logical to add the '&& irq_wu_ctrl_i'. mstatus.mie is still needed for this assertion as the core may wake up on a non-globally enabled interrupt. In that case no interrupt will be taken and the core continues ad PC+4.

|=>
(ctrl_fsm_o.pc_mux == PC_TRAP_IRQ) || (ctrl_fsm_o.pc_mux == PC_TRAP_CLICV))
else `uvm_error("controller", "Woke from sleep due to irq but irq not taken")

// Ensure NMI is taken if woken up by an NMI
a_sleep_to_nmi:
assert property (@(posedge clk) disable iff (!rst_n)
(ctrl_fsm_cs == SLEEP) &&
(ctrl_fsm_ns == FUNCTIONAL) &&
pending_nmi &&
!(debug_req_i || irq_wu_ctrl_i || (wfe_in_wb && wu_wfe_i))
Copy link
Contributor

Choose a reason for hiding this comment

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

Add && pending_nmi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add it

|=>
(ctrl_fsm_o.pc_mux == PC_TRAP_NMI))
else `uvm_error("controller", "Woke from sleep due to NMI but NMI not taken")

// woke_to_debug_q shall only be high for a single cycle
a_woke_to_debug_single_cycle:
assert property (@(posedge clk) disable iff (!rst_n)
woke_to_debug_q
|=>
!woke_to_debug_q)
else `uvm_error("controller", "woke_to_debug_q asserted for more than one cycle")

// woke_to_interrupt_q shall only be high for a single cycle
a_woke_to_interrupt_single_cycle:
assert property (@(posedge clk) disable iff (!rst_n)
woke_to_interrupt_q
|=>
!woke_to_interrupt_q)
else `uvm_error("controller", "woke_to_interrupt_q asserted for more than one cycle")

endmodule