Skip to content
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

Condition dcache_ctrl to MMU_PRESENT #1568

Merged
merged 5 commits into from
Oct 24, 2023
Merged

Conversation

JeanRochCoulon
Copy link
Contributor

When MMU is not present ldst does not request address translation, that's why related dcache_ctrl is not useful. Condition it to MMU_PRESENT to increase code coverage and decrease the gate count.

When MMU is not present ldst does not request address translation, that's why related dcache_ctrl is not useful. Condition it to MMU_PRESENT to increase code coverage and decrease the gate count.
@github-actions
Copy link
Contributor

✔️ successful run, report available here.

@JeanRochCoulon
Copy link
Contributor Author

@AEzzejjari can you approve the PR ?

@Moschn
Copy link
Contributor

Moschn commented Oct 24, 2023

Why is only the cache_ctrl for miss port 0 tied to MMU_PRESENT? I assume the compiler removes the others during compilation. However, I think the source code would be clearer if all of the instantiations are disabled if MMU_PRESENT is not set. Otherwise, someone who reads the code could be confused as to why the others still remain.

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 it would be better to add the condition as follows

for (genvar k = 0; k < NumPorts - 1; k++) begin : gen_rd_ports
// set these to high prio ports
if (K != 0 || MMU_PRESENT) begin
assign rd_prio[k] = 1'b1;
wt_dcache_ctrl #(
.CVA6Cfg(CVA6Cfg),
.RdTxId (RdAmoTxId)
) i_wt_dcache_ctrl (
.clk_i (clk_i),
.rst_ni (rst_ni),
.cache_en_i (cache_en),
// reqs from core
.req_port_i (req_ports_i[k]),
.req_port_o (req_ports_o[k]),
// miss interface
.miss_req_o (miss_req[k]),
.miss_ack_i (miss_ack[k]),
.miss_we_o (miss_we[k]),
.miss_wdata_o (miss_wdata[k]),
.miss_wuser_o (miss_wuser[k]),
.miss_vld_bits_o(miss_vld_bits_o[k]),
.miss_paddr_o (miss_paddr[k]),
.miss_nc_o (miss_nc[k]),
.miss_size_o (miss_size[k]),
.miss_id_o (miss_id[k]),
.miss_replay_i (miss_replay[k]),
.miss_rtrn_vld_i(miss_rtrn_vld[k]),
// used to detect readout mux collisions
.wr_cl_vld_i (wr_cl_vld),
// cache mem interface
.rd_tag_o (rd_tag[k]),
.rd_idx_o (rd_idx[k]),
.rd_off_o (rd_off[k]),
.rd_req_o (rd_req[k]),
.rd_tag_only_o (rd_tag_only[k]),
.rd_ack_i (rd_ack[k]),
.rd_data_i (rd_data),
.rd_user_i (rd_user),
.rd_vld_bits_i (rd_vld_bits),
.rd_hit_oh_i (rd_hit_oh)
);
end else begin
assign rd_prio[0] = 1'b0;
assign req_ports_o[0] = '0;
assign miss_req[0] = 1'b0;
assign miss_we[0] = 1'b0;
assign miss_wdata[0] = {{riscv::XLEN}{1'b0}};
assign miss_wuser[0] = {{DCACHE_USER_WIDTH}{1'b0}};
assign miss_vld_bits_o[0] = {{DCACHE_SET_ASSOC}{1'b0}};
assign miss_paddr[0] = {{riscv::PLEN}{1'b0}};
assign miss_nc[0] = 1'b0;
assign miss_size[0] = 3'b0;
assign miss_id[0] = {{CACHE_ID_WIDTH}{1'b0}};
assign rd_tag[0] = {{DCACHE_TAG_WIDTH}{1'b0}};
assign rd_idx[0] = {{DCACHE_CL_IDX_WIDTH}{1'b0}};
assign rd_off[0] = {{DCACHE_OFFSET_WIDTH}{1'b0}};
assign rd_req[0] = 1'b0;
assign rd_tag_only[0] = 1'b0;
end

@JeanRochCoulon
Copy link
Contributor Author

@Moschn I do not understand your comment because to my mind dcache_ctrl[0] was dedicated to MMU, [1] to LOAD and [2] to STORE. If MMU is not PRESENT, we cannot disable all configurations but only [0]. Am I wrong ?

@Moschn
Copy link
Contributor

Moschn commented Oct 24, 2023

Ah you probably are correct. I did not look at the details of the cache controller, I just looked at the diff in this PR, and I did not understand why only a part of it is disabled. My mistake.

Maybe it would be best to add this explanation in a comment in the source code to help people like me :)

@JeanRochCoulon
Copy link
Contributor Author

Anyway, thank for your review @Moschn, I have added a comment !

@github-actions
Copy link
Contributor

❌ failed run, report available here.

2 similar comments
@github-actions
Copy link
Contributor

❌ failed run, report available here.

@github-actions
Copy link
Contributor

❌ failed run, report available here.

@github-actions
Copy link
Contributor

✔️ successful run, report available here.

@JeanRochCoulon JeanRochCoulon merged commit 53d3880 into master Oct 24, 2023
23 checks passed
@JeanRochCoulon JeanRochCoulon deleted the JeanRochCoulon-patch-3 branch October 24, 2023 15:29
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.

3 participants