Reverted WPT/MPU backpressure and applied sticky LSU bits in WB instead #771
Conversation
This reverts commit 8900390.
…he LSU." This reverts commit f64e4b7. Signed-off-by: Oystein Knauserud <[email protected]>
This reverts commit 1630645. Signed-off-by: Oystein Knauserud <[email protected]>
…sserted. This can currently only happen for watchpoint triggers, in which the wb_valid would have been missed without these sticky bits. Signed-off-by: Oystein Knauserud <[email protected]>
silabs-oysteink
added
the
Component:RTL
rtl/cv32e40x_core.sv
Outdated
| @@ -895,13 +901,13 @@ module cv32e40x_core import cv32e40x_pkg::*; | |||
| .first_op_id_i ( first_op_id ), | |||
|
|
|||
| // LSU | |||
| .lsu_mpu_status_wb_i ( lsu_mpu_status_wb ), | |||
| .lsu_mpu_status_wb_i ( mpu_status_wb ), | |||
There was a problem hiding this comment.
.lsu_mpu_status_wb_i -> .mpu_status_wb_i (and move to WB section)
rtl/cv32e40x_core.sv
Outdated
| .data_stall_wb_i ( data_stall_wb ), | ||
| .lsu_err_wb_i ( lsu_err_wb ), | ||
| .lsu_busy_i ( lsu_busy ), | ||
| .lsu_interruptible_i ( lsu_interruptible ), | ||
| .lsu_valid_wb_i ( lsu_valid_wb ), | ||
| .lsu_wpt_match_wb_i ( lsu_wpt_match_wb ), | ||
| .lsu_wpt_match_wb_i ( wpt_match_wb ), |
There was a problem hiding this comment.
.lsu_wpt_match_wb_i -> .wpt_match_wb_i (and move to WB section)
rtl/cv32e40x_wpt.sv
Outdated
| output data_resp_t core_resp_o, | ||
|
|
||
| // Indication from the core that there will be one pending transaction in the next cycle | ||
| input logic core_one_txn_pend_n, | ||
| input logic core_one_txn_pend_n, |
There was a problem hiding this comment.
Use indentation as in original file (same for all I/O signals)
rtl/cv32e40x_wb_stage.sv
Outdated
| end | ||
|
|
||
| assign lsu_valid = lsu_valid_i || lsu_valid_q; | ||
| assign lsu_wpt_match = lsu_wpt_match_i || lsu_wpt_match_q; |
There was a problem hiding this comment.
The wpt and mpu status assignments should use the same RTL style
rtl/cv32e40x_wb_stage.sv
Outdated
| lsu_wpt_match_q <= 1'b0; | ||
| lsu_mpu_status_q <= MPU_OK; | ||
| end else begin | ||
| lsu_valid_q <= lsu_valid; |
There was a problem hiding this comment.
I think the code would be clearer if you would have something like:
if (lsu_valid) begin
lsu_valid_q <= 1'b1;
lsu_wpt_match_q <= lsu_wpt_match;
lsu_mpu_status_q <= lsu_mpu_status;
end
(as that shows the intention more clearly that we try to capture the state during a 1-cycle rvalid pulse)
assign lsu_valid = lsu_valid_q ? lsu_valid_q : lsu_valid_i;
assign lsu_wpt_match = lsu_valid_q ? lsu_wpt_match_q : lsu_wpt_match_i;
assign lsu_mpu_status = lsu_valid_q ? lsu_mpu_status_q : lsu_mpu_status_i;
rtl/cv32e40x_wb_stage.sv
Outdated
| end | ||
|
|
||
| assign lsu_valid = lsu_valid_i || lsu_valid_q; | ||
| assign lsu_wpt_match = lsu_wpt_match_i || lsu_wpt_match_q; |
There was a problem hiding this comment.
Line 126 should use the sticky version as well (i.e. lsu_mpu_status_i, lsu_wpt_match_i, lsu_valid_i are only allowed to be used in circuit that makes these signals sticky).
lsu_rdata_i usage needs comment why it is save to be used like that (without making it sticky); add assertion proving your assumption.
There was a problem hiding this comment.
Update line 126 and added comments + assertion for lsu_rdata_i.
rtl/cv32e40x_core.sv
Outdated
| .data_stall_wb_i ( data_stall_wb ), | ||
| .lsu_err_wb_i ( lsu_err_wb ), | ||
| .lsu_busy_i ( lsu_busy ), | ||
| .lsu_interruptible_i ( lsu_interruptible ), | ||
| .lsu_valid_wb_i ( lsu_valid_wb ), | ||
| .lsu_wpt_match_wb_i ( lsu_wpt_match_wb ), | ||
| .lsu_wpt_match_wb_i ( wpt_match_wb ), |
There was a problem hiding this comment.
lsu_err_wb signal needs comment why it is treated differently
There was a problem hiding this comment.
Added comments
| // The WB stage will make the mpu_status sticky if wb_walid is not asserted the first cycle in WB. | ||
| // In practice this should never happen as the controller will not allow any halt or kill on a LSU in WB. | ||
| // The only exception to this is the watchpoints which are asserted separately. | ||
| a_no_sticky_mpu_error: |
There was a problem hiding this comment.
Good to have this assertion. Are you only asserting it because this should not happen or is anything relying on this not happening (explain in the comment whether this is just how the design is expected to work or whether other parts of the design depend on this property).
There was a problem hiding this comment.
It is added to check the assumption it should never happen. I do think the existing comments cover that assumption?
Signed-off-by: Oystein Knauserud <[email protected]>
No description provided.