Fix configuration without ECC

[ricted98]

The LLC without ECC had some issues scattered across the design. This PR proposes some fixes.
@Aquaticfuller feel free to push additional commits on this branch if needed.

[yvantor]

Just one comment.

[yvantor]

Is this not needed?

[ricted98]

I guess it was originally intended to cut a path. Functionally, it does not seem fundamental as I am able to run the regression tests without it. However, this observation was somewhat "reverse engineered". If @Aquaticfuller can give his opinion it would be better.

[Aquaticfuller]

Hi, I think I added this shift reg just because I added a timing cut in the ecc_sram module, so the tag sram read latency is added with 1 more cycle, and I also made some change according to this in the hit_miss_unit module. So in order to make sure it is still functionally correct without ECC, I also added this output register for the sram without ecc. However, I test was mainly focused on the with ecc version, so I'm really curious if it passes the regression tests both with and without this shift register?

[Aquaticfuller]

And could you please tell me what kind of regression test you are using? Did you run the axi_llc testbench?

[yvantor]

Ok, for now we have removed the ECC in LLC otherwise Cheshire does not implement on the Genesys. @ricted98 after the current CI finishes without the ECCs, would you reintroduce the ECCs in LLC and repush to verify if such change breaks the CI or not?

[ricted98]

I see that there is some differences between the version with and without ECC in this module. The data split genloop is implemented only for the ECC version. Should it also be replicated for the normal SRAM?

axi_llc/src/axi_llc_sram.sv

Lines 57 to 158 in c7b27b2

	if (EnableEcc) begin: gen_ecc_sram
	logic [NumBanks-1:0][G-1:0] wdata, rdata;
	logic [NumBanks-1:0][BeWidthPerBank-1:0] be;
	logic [NumBanks-1:0] gnt;
	logic gnt_all;
	logic hsk_d, hsk_q;
	logic we_q;
	data_t rdata_d, rdata_q;
	logic rdata_en;
	assign hsk_d = req_i & gnt_all;
	assign gnt_all = &gnt;
	assign gnt_o = gnt_all;
	shift_reg #(
	.dtype(logic),
	.Depth(NumOutputCuts+1)
	) i_hsk_q (
	.clk_i,
	.rst_ni,
	.d_i (hsk_d),
	.d_o (hsk_q)
	);
	shift_reg #(
	.dtype(logic),
	.Depth(NumOutputCuts+1)
	) i_we_q (
	.clk_i,
	.rst_ni,
	.d_i (we_i),
	.d_o (we_q)
	);
	shift_reg_gated #(
	.dtype(data_t),
	.Depth(NumOutputCuts+1)
	) i_rdata_q (
	.clk_i,
	.rst_ni,
	.valid_i(rdata_en),
	.data_i (rdata_d),
	.valid_o(),
	.data_o (rdata_q)
	);
	for (genvar i = 0; i < NumBanks; i++) begin: gen_data_split
	assign wdata[i] = wdata_i[G*i+:G];
	assign rdata_o[G*i+:G] = (hsk_q & ~we_q) ? rdata[i] : rdata_q[G*i+:G];
	assign be[i] = be_i[BeWidthPerBank*(i/NumBankPerInBeBit)+:BeWidthPerBank];
	ecc_sram #(
	.NumWords ( NumWords ),
	.UnprotectedWidth ( G ),
	.ProtectedWidth ( EccDataWidth ),
	.InputECC ( 0 ),
	.NumRMWCuts ( 1 ),
	.NumOutputCuts ( NumOutputCuts ),
	.SimInit ( "zeros" )
	) i_ecc_sram (
	.clk_i,
	.rst_ni,
	.scrub_trigger_i ( scrub_trigger_i [i]),
	.scrubber_fix_o ( scrubber_fix_o [i]),
	.scrub_uncorrectable_o( scrub_uncorrectable_o [i]),
	.wdata_i ( wdata[i]),
	.addr_i ( addr_i ),
	.req_i ( req_i ),
	.we_i ( we_i ),
	.be_i ( {(ByteWidthPerBank/8){be[i]}} ),
	.rdata_o ( rdata[i]),
	.gnt_o ( gnt[i] ),
	.single_error_o ( single_error_o [i] ),
	.multi_error_o ( multi_error_o [i] )
	);
	end
	// always_ff @(posedge clk_i or negedge rst_ni) begin
	// if(~rst_ni) begin
	// hsk_q <= 1'b0;
	// we_q <= 1'b0;
	// rdata_q <= '0;
	// end else begin
	// hsk_q <= hsk_d;
	// we_q <= we_i;
	// if (rdata_en) begin
	// rdata_q <= rdata_d;
	// end
	// end
	// end
	assign rdata_en = hsk_q & ~we_q;
	assign rdata_d = rdata_o;
	end else begin: gen_standard_sram

[yvantor]

Great! We also saw that in the CI the Linux boot in Cheshire breaks (probably because LLC is not configured as Cache). Can you help us having a look in what is going wrong?

[Aquaticfuller]

Hi, I just pushed a commit, which fixed a bug: when running the axi_llc built-in test, it gets stuck at BIST.

[Aquaticfuller]

Great! We also saw that in the CI the Linux boot in Cheshire breaks (probably because LLC is not configured as Cache). Can you help us having a look in what is going wrong?

If I didn't get it wrong, in the cheshire_bootrom.S,

69    li t1, -1
70    sw t1, 0(t0)    // llc.CFG_SPM_LOW
71    sw t1, 4(t0)    // llc.CFG_SPM_HIGH
72    li t1, 1
73    sw t1, 16(t0)   // llc.CFG_COMMIT

this code snippet will config LLC to a SPM, and write 0 to llc.CFG_SPM_LOW and llc.CFG_SPM_HIGH should set the LLC into full cache mode. Or you can do it in your baremetal c program with:

    *reg32(&__base_llc, AXI_LLC_CFG_SPM_LOW_OFFSET) = 0;
    *reg32(&__base_llc, AXI_LLC_CFG_SPM_HIGH_OFFSET) = 0;
    *reg32(&__base_llc, AXI_LLC_COMMIT_CFG_OFFSET) = 1;

to set the LLC into a full cache.

[ricted98]

Hi, I just pushed a commit, which fixed a bug: when running the axi_llc built-in test, it gets stuck at BIST.

Thanks! I am now trying it on Cheshire

Great! We also saw that in the CI the Linux boot in Cheshire breaks (probably because LLC is not configured as Cache). Can you help us having a look in what is going wrong?

If I didn't get it wrong, in the cheshire_bootrom.S,

69    li t1, -1
70    sw t1, 0(t0)    // llc.CFG_SPM_LOW
71    sw t1, 4(t0)    // llc.CFG_SPM_HIGH
72    li t1, 1
73    sw t1, 16(t0)   // llc.CFG_COMMIT

this code snippet will config LLC to a SPM, and write 0 to llc.CFG_SPM_LOW and llc.CFG_SPM_HIGH should set the LLC into full cache mode. Or you can do it in your baremetal c program with:

    *reg32(&__base_llc, AXI_LLC_CFG_SPM_LOW_OFFSET) = 0;
    *reg32(&__base_llc, AXI_LLC_CFG_SPM_HIGH_OFFSET) = 0;
    *reg32(&__base_llc, AXI_LLC_COMMIT_CFG_OFFSET) = 1;

to set the LLC into a full cache.

Yes, as far as I know it it set in cache mode by software as you pointed out.