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.

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

Jump to bottom

Watchpoint triggers refactor #713

Merged
merged 5 commits into from
Dec 5, 2022
Merged

Watchpoint triggers refactor #713

merged 5 commits into from
Dec 5, 2022

Conversation

silabs-oysteink
Copy link
Contributor

Refactored watchpoint trigger behavior.

  • Instead of gating valid_0_i in the LSU, there is a new module cv32e40x_wpt that sits between the 'aligner' and the MPU in the LSU. This module will let un-triggered accesses through and 'consume' those with watchpoint triggers.
  • WPT will return the trigger match with WB timing, as the MPU returns mpu status today.
  • Controller was updated to handle the new timing of the trigger match/abort_op signals.

@silabs-oysteink
Initroduced new signals in load_store_unit to reflect the signal flow…
0c9e096 
… once the watchpoint module is inserted between the aligner and mpu.

SEC clean.

Signed-off-by: Oystein Knauserud <[email protected]>
@silabs-oysteink
Watchpoint triggers handled in controller after refactoring the LSU/W…
854041d 
…PT module.

- Split pending_debug into asynchronous and synchronous
- Modified the 'sequence_in_progress_wb' to take watchpoint trigger into account, clearing the flag when a trigger is taken.
- Various assertion updates related to the RTL changes.

Signed-off-by: Oystein Knauserud <[email protected]>
@silabs-oysteink
Added assert to check that WPT and MPU may share the outstanding coun…
0862e61 
…ter within the LSU.

Cosmetic changes.

Signed-off-by: Oystein Knauserud <[email protected]>
@silabs-oysteink silabs-oysteink added the Component:RTL For issues in the RTL (e.g. for files in the rtl directory) label Dec 2, 2022
Silabs-ArjanB
Silabs-ArjanB requested changes Dec 2, 2022
View reviewed changes
Copy link
Contributor

@Silabs-ArjanB Silabs-ArjanB left a comment

Choose a reason for hiding this comment

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

I still need to review the 3 sva files (will do that next week), but here is my feedback so far.

rtl/cv32e40x_ex_stage.sv Show resolved Hide resolved
rtl/cv32e40x_ex_stage.sv Outdated Show resolved Hide resolved
rtl/cv32e40x_controller_fsm.sv Show resolved Hide resolved
rtl/cv32e40x_controller_fsm.sv
@@ -458,18 +463,31 @@ module cv32e40x_controller_fsm import cv32e40x_pkg::*;
// - If trigger_match_in_wb is caused by instruction address match, then no side effects will happen for a sequence, and debug mode is entered when the first operation is in WB.
// When a CLIC pointer is in the pipeline stages EX or WB, we must block debug.
// - Debug would otherwise kill the pointer and use the address of the pointer for dpc. A following dret would then return to the mtvt table, losing program progress.
assign debug_allowed = lsu_interruptible_i && !fencei_ongoing && !xif_in_wb && !clic_ptr_in_pipeline && (sequence_interruptible || trigger_match_in_wb);
assign async_debug_allowed = lsu_interruptible_i && !fencei_ongoing && !xif_in_wb && !clic_ptr_in_pipeline && sequence_interruptible;
Copy link
Contributor

Choose a reason for hiding this comment

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

Update above comments

rtl/cv32e40x_load_store_unit.sv Show resolved Hide resolved
rtl/cv32e40x_load_store_unit.sv Outdated
// consume the transaction, not letting it through to the MPU. The triger match will
// be returned with the response with WB timing.

cv32e40x_wpt wpt_i
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a generate (and else branch) around this module for the case when we have 0 debug triggers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, added separate SVA module for WPT to accomodate change

rtl/cv32e40x_wb_stage.sv Outdated
@@ -156,7 +157,7 @@ module cv32e40x_wb_stage import cv32e40x_pkg::*;

// Append any MPU exception to abort_op
// An abort_op_o = 1 will terminate a sequence, either to take an exception or debug due to trigger match.
assign abort_op_o = ex_wb_pipe_i.abort_op || ( ex_wb_pipe_i.lsu_en && lsu_exception);
assign abort_op_o = ex_wb_pipe_i.abort_op || ( ex_wb_pipe_i.lsu_en && lsu_exception) || lsu_wpt_match_i;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is ex_wb_pipe_i.lsu_en needed/used for lsu_exception but not for lsu_wpt_match_i (either it should be needed in both places or in no place I would think)?

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 added ex_wb_pipe_i.lsu_en to align with mpu exception.

rtl/cv32e40x_wpt.sv Outdated
logic wpt_block_bus;
logic wpt_trans_valid;
logic wpt_trans_ready;
logic wpt_match_resp;
Copy link
Contributor

Choose a reason for hiding this comment

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

wpt_match_resp -> wpt_match

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

rtl/include/cv32e40x_pkg.sv
typedef struct packed {
obi_data_resp_t bus_resp;
mpu_status_e mpu_status;
logic wpt_match;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this causes wpt_match to become non-driven in the dataside MPU. It should be tied off there as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tieoff in mpu

@silabs-oysteink
Updates after review.
4b6e380 
Guarded instance of cv32e40x_wpt.sv with DBG_NUM_TRIGGERS>0.
Updated condition for clearing sequence_in_progress_wb when a wpt match occurs.Comment updates
Reintroduced "resp* in LSU, same values as wpt_resp*.

SEC clean.

Signed-off-by: Oystein Knauserud <[email protected]>
@silabs-oysteink
Copy link
Contributor Author

Added an additional commit as the tieoffs within LSU was not correct if DBG_NUM_TRIGGERS is 0. Ready for review.

@Silabs-ArjanB Silabs-ArjanB merged commit a913e8e into openhwgroup:master Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Silabs-ArjanB Silabs-ArjanB Silabs-ArjanB approved these changes

Assignees
No one assigned
Labels
Component:RTL For issues in the RTL (e.g. for files in the rtl directory)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@silabs-oysteink @Silabs-ArjanB