-
Notifications
You must be signed in to change notification settings - Fork 51
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
Updates due to updated debug spec #858
Updates due to updated debug spec #858
Conversation
silabs-oysteink
commented
May 22, 2023
- Removed tdata3 and tcontrol
- Added hit1, hit0, uncertain and uncertainen to mcontrol6
- Updating all matching mcontrol6 hit fields when a debug trigger is taken
Signed-off-by: Oystein Knauserud <Oystein.Knauserud@silabs.com>
- Added hit1, hit0, uncerteain and uncertainen bits - Propagating trigger matches for all implemented triggers down the pipeline. - Updating all matcing mcontrol6 triggers' hit fields when a trigger debug entry happens. Signed-off-by: Oystein Knauserud <Oystein.Knauserud@silabs.com>
-Removed tdata3 and tcontrol -Updated tinfo to add version field Signed-off-by: Oystein Knauserud <Oystein.Knauserud@silabs.com>
rtl/cv32e40x_debug_triggers.sv
Outdated
2'b00, // zero 26:25 | ||
3'b000, // zero, vs, vu, hit 24:22 | ||
mcontrol6_uncertain_resolve(csr_wdata_i[MCONTROL_6_UNCERTAIN]), // uncertain 26 | ||
mcontrol6_hit1_resolve(csr_wdata_i[MCONTROL_6_HIT1]), // hit1: 25 |
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.
hit1 and hit0 must form a combined 2-bit WARL field (not two 1-bit WARL fields)
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.
Fixed. Note that due to strange issues with bit-slicing a function return value a separate 2-bit logic was used as return value.
rtl/cv32e40x_debug_triggers.sv
Outdated
@@ -226,6 +223,18 @@ import cv32e40x_pkg::*; | |||
tdata1_n[idx] = {TTYPE_DISABLED, 1'b1, {27{1'b0}}}; | |||
end | |||
end // tdata1_we_i | |||
|
|||
// Writes from the controller takes presedence over SW writes |
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.
This comment sounds suspicious. How can there ever be both a software write and a HW write? Can you add an assertion that this is impossible and change this comment?
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.
Fixed
rtl/cv32e40x_ex_stage.sv
Outdated
@@ -420,7 +421,7 @@ module cv32e40x_ex_stage import cv32e40x_pkg::*; | |||
|
|||
// CSR illegal instruction detected in this stage, OR'ing in the status | |||
ex_wb_pipe_o.illegal_insn <= id_ex_pipe_i.illegal_insn || csr_is_illegal; | |||
ex_wb_pipe_o.trigger_match <= id_ex_pipe_i.trigger_match; | |||
ex_wb_pipe_o.trigger_match <= {{(32-DBG_NUM_TRIGGERS){1'b0}}, id_ex_pipe_i.trigger_match[DBG_NUM_TRIGGERS-1:0]}; |
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.
Why this expression?
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.
Just add assertion on all those 32-bit signals that MSBs are always 0
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.
It was a remainder after performing some synthesis checks, removed now.
rtl/cv32e40x_id_stage.sv
Outdated
@@ -639,7 +640,7 @@ module cv32e40x_id_stage import cv32e40x_pkg::*; | |||
id_ex_pipe_o.instr <= if_id_pipe_i.instr; | |||
end | |||
|
|||
id_ex_pipe_o.trigger_match <= if_id_pipe_i.trigger_match; | |||
id_ex_pipe_o.trigger_match <= {{(32-DBG_NUM_TRIGGERS){1'b0}}, if_id_pipe_i.trigger_match[DBG_NUM_TRIGGERS-1:0]}; |
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.
Expression should not be needed (I think)
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.
Same as above
rtl/cv32e40x_if_stage.sv
Outdated
@@ -431,7 +432,7 @@ module cv32e40x_if_stage import cv32e40x_pkg::*; | |||
if_id_pipe_o.illegal_c_insn <= seq_valid ? 1'b0 : illegal_c_insn; | |||
|
|||
if_id_pipe_o.priv_lvl <= prefetch_priv_lvl; | |||
if_id_pipe_o.trigger_match <= trigger_match_i; | |||
if_id_pipe_o.trigger_match <= {{(32-DBG_NUM_TRIGGERS){1'b0}}, trigger_match_i[DBG_NUM_TRIGGERS-1:0]}; |
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.
Why the expression?
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.
Same as the other two
rtl/cv32e40x_load_store_unit.sv
Outdated
@@ -938,7 +938,7 @@ module cv32e40x_load_store_unit import cv32e40x_pkg::*; | |||
assign xif_mem_if.mem_resp.exccode = xif_mpu_err ? ( | |||
trans.we ? EXC_CAUSE_STORE_FAULT : EXC_CAUSE_LOAD_FAULT | |||
) : '0; | |||
assign xif_mem_if.mem_resp.dbg = xif_wpt_match; | |||
assign xif_mem_if.mem_resp.dbg = |xif_wpt_match; // TODO:XIF Is one bit enough? |
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.
I think so. This todo can be removed.
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.
Done
rtl/include/cv32e40x_pkg.sv
Outdated
return 1'b0; | ||
endfunction | ||
|
||
function automatic logic mcontrol6_hit1_resolve |
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.
hit1+hit0 should be combined WARL field
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.
Done
@@ -86,6 +89,32 @@ module cv32e40x_debug_triggers_sva | |||
$stable(tdata2_q)) // Checking stability of ALL tdata2, not just the one selected | |||
else `uvm_error("debug_triggers", "tdata2_q changed after set/clear with rs1==0") | |||
|
|||
// Check that tdata1.hit0 is set when a mcontrol6 trigger matched when debug is entered | |||
generate |
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.
Also check HIT1.
Also check that the triggers not hitting have their HIT* field unchanged.
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.
Done
Signed-off-by: Oystein Knauserud <Oystein.Knauserud@silabs.com>