Make scrubber work with CVA6 #23
Conversation
micprog
left a comment
micprog
left a comment
There was a problem hiding this comment.
I added a few comments with a few tricks and nitpicks in an attempt to make this compatible with the existing wrapper, but upon further inspection, this does not seem reasonable.
| //parameter int unsigned DataWidth = 39, | ||
| parameter int unsigned BlockWidth = 32, | ||
| parameter int unsigned BlockWidthECC = 39, | ||
| parameter int unsigned DataDivisions = 1, | ||
| parameter int unsigned TagWidth = 1, | ||
| //parameter int unsigned ProtWidth = 7, | ||
| parameter int unsigned Assoc = 2, | ||
|
|
||
| parameter type be_t = logic, // need to have a data and tag element | ||
| parameter type line_t = logic // need to have a data and tag element | ||
|
|
There was a problem hiding this comment.
Do you think there is a way to alter the parametrization to remain backward-compatible? E.g.:
| //parameter int unsigned DataWidth = 39, | |
| parameter int unsigned BlockWidth = 32, | |
| parameter int unsigned BlockWidthECC = 39, | |
| parameter int unsigned DataDivisions = 1, | |
| parameter int unsigned TagWidth = 1, | |
| //parameter int unsigned ProtWidth = 7, | |
| parameter int unsigned Assoc = 2, | |
| parameter type be_t = logic, // need to have a data and tag element | |
| parameter type line_t = logic // need to have a data and tag element | |
| parameter int unsigned DataWidth = 39, | |
| parameter int unsigned ProtWidth = 7, | |
| parameter int unsigned BlockWidth = DataWidth-ProtWidth, | |
| parameter int unsigned BlockWidthECC = DataWidth, | |
| parameter int unsigned DataDivisions = DataWidth/BlockWidthECC, | |
| parameter int unsigned TagWidth = 1, | |
| parameter int unsigned Assoc = 2, | |
| parameter type be_t = logic [DataDivisions-1:0], // need to have a data and tag element | |
| parameter type line_t = logic[DataWidth-1:0] // need to have a data and tag element | |
| output logic [$clog2(BankSize)-1:0] bank_add_o, | ||
| output logic [ DataWidth-1:0] bank_wdata_o, | ||
| input logic [ DataWidth-1:0] bank_rdata_i, | ||
| output be_t bank_be_o, |
There was a problem hiding this comment.
I see this will have an impact on backward compatibility. Do you think this is necessary, i.e., would re-writing the entire data line impact performance?
| if ( (state_q == Read || state_q == Write) && intc_req_i == 1'b0) begin | ||
| bank_we_o = scrub_we; | ||
| if ( (state_q == Read || state_q == Write) && (|intc_req_i === 1'b0)) begin | ||
| bank_we_o = scrub_we; |
There was a problem hiding this comment.
| bank_we_o = scrub_we; | |
| bank_we_o = scrub_we; |
| scrub_req = 1<<assoc_c; | ||
| scrub_we = 1'b1; | ||
|
|
||
| always_comb begin | ||
| for (int assoc=0; assoc<Assoc; ++assoc)begin | ||
| if ((|err_read_data[assoc])=='0 && (|err_read_tag[assoc])=='0)begin end else begin | ||
| scrub_wdata.data = temp_scrub_result[assoc]; |
There was a problem hiding this comment.
I think this may also be a challenge for backward compatibility, as you are making assumptions about the structure of line_t. As far as I understand, you need it, so we may just need to write a separate module for your use case, as integrating with this one will break other parts.
|
Superseded by #24 |
2 participants
Tried to keep implementation as similar as before, major change is that is uses byte enabled and associativity.