Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 81 additions & 32 deletions rtl/ecc_wrap/ecc_scrubber.sv
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,20 @@
// - corrects *only* correctable errors

module ecc_scrubber #(
parameter int unsigned WITH_VALID = 0,
parameter int unsigned BankSize = 256,
parameter bit UseExternalECC = 0,
parameter int unsigned DataWidth = 39,
parameter int unsigned ProtWidth = 7
//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

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

) (
input logic clk_i,
input logic rst_ni,
Expand All @@ -26,32 +36,35 @@ module ecc_scrubber #(
output logic uncorrectable_o,

// Input signals from others accessing memory bank
input logic intc_req_i,
input logic [Assoc-1:0] intc_req_i,
input logic intc_we_i,
input logic [$clog2(BankSize)-1:0] intc_add_i,
input logic [ DataWidth-1:0] intc_wdata_i,
output logic [ DataWidth-1:0] intc_rdata_o,
input be_t intc_be_i,
input line_t intc_wdata_i,
output line_t [Assoc-1:0] intc_rdata_o,

// Output directly to bank
output logic bank_req_o,
output logic [Assoc-1:0] bank_req_o,
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?

output line_t bank_wdata_o,
input line_t [Assoc-1:0] bank_rdata_i,

// If using external ECC
output logic [ DataWidth-1:0] ecc_out_o,
input logic [ DataWidth-1:0] ecc_in_i,
input logic [ 2:0] ecc_err_i
output line_t [Assoc-1:0] ecc_out_o,
input line_t [Assoc-1:0] ecc_in_i,
input logic [ 2:0] ecc_err_i
);
int assoc_c = 0;
logic [1:0] ecc_err;


logic [ 1:0] ecc_err;

logic scrub_req;
logic [Assoc-1:0] scrub_req;
logic scrub_we;
logic [$clog2(BankSize)-1:0] scrub_add;
logic [ DataWidth-1:0] scrub_wdata;
logic [ DataWidth-1:0] scrub_rdata;
line_t scrub_wdata;
line_t [Assoc-1:0] scrub_rdata;

typedef enum logic [2:0] {Idle, Read, Write} scrub_state_e;

Expand All @@ -60,7 +73,6 @@ module ecc_scrubber #(
logic [$clog2(BankSize)-1:0] working_add_d, working_add_q;
assign scrub_add = working_add_q;

assign bank_req_o = intc_req_i || scrub_req;
assign intc_rdata_o = bank_rdata_i;
assign scrub_rdata = bank_rdata_i;

Expand All @@ -69,12 +81,16 @@ module ecc_scrubber #(
bank_we_o = intc_we_i;
bank_add_o = intc_add_i;
bank_wdata_o = intc_wdata_i;
bank_be_o = intc_be_i;
bank_req_o = intc_req_i;

// If scrubber active and outside is not, do scrub
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;

bank_add_o = scrub_add;
bank_wdata_o = scrub_wdata;
bank_be_o = '1;
bank_req_o = scrub_req;
end
end

Expand All @@ -84,15 +100,48 @@ module ecc_scrubber #(
assign scrub_wdata = ecc_in_i;
end else begin : gen_internal_ecc
assign ecc_out_o = '0;
hsiao_ecc_cor #(
.DataWidth (DataWidth-ProtWidth),
.ProtWidth (ProtWidth)
) ecc_corrector (
.in ( scrub_rdata ),
.out ( scrub_wdata ),
.syndrome_o(),
.err_o ( ecc_err )
);
logic [Assoc-1:0] [DataDivisions-1:0] [1:0] err_read_data;
logic [Assoc-1:0] [1:0] err_read_tag;
line_t [Assoc-1:0] temp_scrub_result;

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.

assoc_c = assoc;
break;
end
end
end


for (genvar assoc=0; assoc<Assoc; ++assoc)begin
for (genvar j=0; j<DataDivisions;++j) begin
hsiao_ecc_cor #(
.DataWidth (BlockWidth)
) ecc_corrector (
.in ( scrub_rdata[assoc].data[j*BlockWidthECC+:BlockWidthECC] ),
.out ( temp_scrub_result[assoc].data[j*BlockWidthECC+:BlockWidthECC]),
.syndrome_o(),
.err_o ( err_read_data[assoc][j] )
);
end
hsiao_ecc_cor #(
.DataWidth (TagWidth)
) ecc_corrector (
.in ( scrub_rdata[assoc].tag),
.out ( temp_scrub_result[assoc].tag),
.syndrome_o(),
.err_o ( err_read_tag[assoc] )
);
end
assign ecc_err = (scrub_rdata[assoc_c].valid===1'b1)? ((|err_read_data) | (|err_read_tag)): '0;
end

if (WITH_VALID)begin
assign scrub_wdata.dirty =scrub_rdata[assoc_c].dirty;
// if uncorrectable error but not dirty, invalidate line
assign scrub_wdata.valid =scrub_rdata[assoc_c].valid; //(ecc_err[1]===1'b1 && scrub_rdata[assoc_c].dirty=='0)? '0: scrub_rdata[assoc_c].valid;
end

always_comb begin : proc_FSM_logic
Expand All @@ -111,24 +160,24 @@ module ecc_scrubber #(

end else if (state_q == Read) begin
// Request read to scrub
scrub_req = 1'b1;
scrub_req = '1;
// Request only active if outside is inactive
if (intc_req_i == 1'b0) begin
state_d = Write;
end

end else if (state_q == Write) begin
if (ecc_err[0] == 1'b0) begin // No correctable Error
if (ecc_err[0] === 1'b0) begin // No correctable Error
// Return to idle state
state_d = Idle;
working_add_d = (working_add_q + 1) % BankSize; // increment address
uncorrectable_o = ecc_err[1];

end else begin // Correctable Error
// Write corrected version
scrub_req = 1'b1;
scrub_req = 1<<assoc_c;
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

// INTC interference - retry read and write
if (intc_req_i == 1'b1) begin
state_d = Read;
Expand Down