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

Apply fixes after merging changes from caliptra-rtl #215

Merged
merged 3 commits into from
Jul 22, 2024

Conversation

kiryk
Copy link
Collaborator

@kiryk kiryk commented Jul 18, 2024

This change:

  1. Removes redundant design/el2_pdef.vh file, which is generated by the flow,
  2. Makes veer.config produce parameter struct using logic type, instead of bit,
  3. Makes the RTL handle all valid ICACHE_NUM_WAYS values.

Copy link

Links to coverage and verification reports for this PR (#215) are available at https://chipsalliance.github.io/Cores-VeeR-EL2/

@calebofearth
Copy link
Contributor

What script is responsible for generating el2_pdef.vh? An important recent change was to convert 'bit' types to 'logic' for lint cleanliness. Is that change merged to the generator script?
If not, I'll request that it be added in this PR.
Thanks.

genvar i;
generate
for (i = 0; i < pt.ICACHE_NUM_WAYS; i = i + 1) begin
assign ic_debug_way[i] = (ic_debug_way_enc == i[1:0]);

Choose a reason for hiding this comment

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

[verible-verilog-format] reported by reviewdog 🐶

Suggested change
assign ic_debug_way[i] = (ic_debug_way_enc == i[1:0]);
assign ic_debug_way[i] = (ic_debug_way_enc == i[1:0]);

for (i = 0; i < pt.ICACHE_NUM_WAYS; i = i + 1) begin
assign ic_debug_way[i] = (ic_debug_way_enc == i[1:0]);
end
endgenerate

Choose a reason for hiding this comment

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

[verible-verilog-format] reported by reviewdog 🐶

Suggested change
endgenerate
endgenerate

for (i = 0; i < pt.ICACHE_NUM_WAYS; i = i + 1) begin
assign ic_debug_way[i] = (ic_debug_way_enc == i[1:0]);
end
endgenerate

assign ic_debug_tag_wr_en[pt.ICACHE_NUM_WAYS-1:0] = {pt.ICACHE_NUM_WAYS{ic_debug_wr_en & ic_debug_tag_array}} & ic_debug_way[pt.ICACHE_NUM_WAYS-1:0] ;

Choose a reason for hiding this comment

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

[verible-verilog-format] reported by reviewdog 🐶

Suggested change
assign ic_debug_tag_wr_en[pt.ICACHE_NUM_WAYS-1:0] = {pt.ICACHE_NUM_WAYS{ic_debug_wr_en & ic_debug_tag_array}} & ic_debug_way[pt.ICACHE_NUM_WAYS-1:0] ;
assign ic_debug_tag_wr_en[pt.ICACHE_NUM_WAYS-1:0] = {pt.ICACHE_NUM_WAYS{ic_debug_wr_en & ic_debug_tag_array}} & ic_debug_way[pt.ICACHE_NUM_WAYS-1:0] ;

(ic_debug_way_enc[0] == 1'b0) };
genvar i;
generate
for (i = 0; i < pt.ICACHE_NUM_WAYS; i = i + 1) begin

Choose a reason for hiding this comment

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

⚠️ [verible-verilog-lint] reported by reviewdog 🐶
All generate block statements must have a label [Style: generate-statements] [generate-label]

@kiryk kiryk changed the title Remove redundant el2_pdef.vh Apply fixes after merging changes from caliptra-rtl Jul 19, 2024
@kiryk
Copy link
Collaborator Author

kiryk commented Jul 19, 2024

Hi @calebofearth, the script that generates el2_pdef.vh is in configs/veer.config. I just pushed the change that makes it use logic type, along with the fix of handling the general case of icache set sizes.

@@ -1632,8 +1632,9 @@ assign ic_debug_rd_en = dec_tlu_ic_diag_pkt.icache_rd_valid ;
assign ic_debug_wr_en = dec_tlu_ic_diag_pkt.icache_wr_valid ;


assign ic_debug_way[pt.ICACHE_NUM_WAYS-1:0] = {(ic_debug_way_enc[0] == 1'b1),
(ic_debug_way_enc[0] == 1'b0) };
for (genvar i = 0; i < pt.ICACHE_NUM_WAYS; i = i + 1) begin : ic_debug_way_loop

Choose a reason for hiding this comment

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

⚠️ [verible-verilog-lint] reported by reviewdog 🐶
All generate block labels must start with g_ or gen_ [Style: generate-constructs] [generate-label-prefix]

Copy link

Links to coverage and verification reports for this PR (#215) are available at https://chipsalliance.github.io/Cores-VeeR-EL2/

1 similar comment
Copy link

Links to coverage and verification reports for this PR (#215) are available at https://chipsalliance.github.io/Cores-VeeR-EL2/

Copy link
Collaborator

@tmichalak tmichalak left a comment

Choose a reason for hiding this comment

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

LGTM

@tmichalak tmichalak merged commit a0092d1 into chipsalliance:main Jul 22, 2024
502 checks passed
@tmichalak tmichalak deleted the kiryk/apply-fixes branch October 22, 2024 13:38
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