-
Notifications
You must be signed in to change notification settings - Fork 53
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
Fix for issues #365 and #361 on cv32e40s. #749
Conversation
When waking up from sleep (exiting FSM state SLEEP), the cause of wakeup is now flopped to enable the FSM to take the action according to the wakeup reason, although there may be higher priority events appearing after the wakeup. Signed-off-by: Oystein Knauserud <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only some minor suggestions to make the added assertions look more similar
assert property (@(posedge clk) disable iff (!rst_n) | ||
(ctrl_fsm_cs == SLEEP) && | ||
(ctrl_fsm_ns == FUNCTIONAL) && | ||
!(pending_nmi || irq_wu_ctrl_i || (wfe_in_wb && wu_wfe_i)) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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_cs == SLEEP) && | ||
(ctrl_fsm_ns == FUNCTIONAL) && | ||
!(pending_nmi || debug_req_i || (wfe_in_wb && wu_wfe_i)) && | ||
mstatus_i.mie |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
assert property (@(posedge clk) disable iff (!rst_n) | ||
(ctrl_fsm_cs == SLEEP) && | ||
(ctrl_fsm_ns == FUNCTIONAL) && | ||
!(debug_req_i || irq_wu_ctrl_i || (wfe_in_wb && wu_wfe_i)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add && pending_nmi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add it
sva/cv32e40x_controller_fsm_sva.sv
Outdated
!(pending_nmi || debug_req_i || (wfe_in_wb && wu_wfe_i)) && | ||
mstatus_i.mie | ||
|=> | ||
(ctrl_fsm_o.irq_ack)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use pc_mux instead to make it similar to next assertion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing to pc_mux comparisons.
Signed-off-by: Oystein Knauserud <[email protected]>
When waking up from sleep (exiting FSM state SLEEP), the cause of wakeup is now flopped to enable the FSM to take the action according to the wakeup reason, although there may be higher priority events appearing after the wakeup.
Signed-off-by: Oystein Knauserud [email protected]