Skip to content

Make scrubber work with CVA6 #23

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

Closed
wants to merge 1 commit into from

Conversation

NickLohr
Copy link

@NickLohr NickLohr commented May 6, 2024

Tried to keep implementation as similar as before, major change is that is uses byte enabled and associativity.

Copy link
Member

@micprog micprog left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines +19 to +29
//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

Copy link
Member

Choose a reason for hiding this comment

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

Do you think there is a way to alter the parametrization to remain backward-compatible? E.g.:

Suggested change
//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 bank_we_o,
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,
Copy link
Member

Choose a reason for hiding this comment

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

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;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bank_we_o = scrub_we;
bank_we_o = scrub_we;

scrub_we = 1'b1;

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

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];
Copy link
Member

Choose a reason for hiding this comment

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

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.

@micprog
Copy link
Member

micprog commented Jun 17, 2024

Superseded by #24

@micprog micprog closed this Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants