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

[ibex,fpga] Apply Ibex FPGA patches #25480

Merged

Conversation

nasahlpa
Copy link
Member

@nasahlpa nasahlpa commented Dec 2, 2024

This PR applies two patches on top of Ibex that fix the following two FPGA related issues:

  • Use WordZeroVal instead of 0 for initializing the FPGA register file
  • Use async reset when not using a DSP for the Ibex FPGA counter

A patch is needed because:

  • Without these fixes the SecureIbex parameter cannot be used on CW340 - this is needed for some SiVal tests
  • These patches only touch the FPGA implementation. If we would vendor in the changes, we would get additional RTL changes that also affect the ASIC

This commit manually pulls over lowRISC/ibex#2224 to fix
the register file for the FPGA configuration. As we do not
want to include other Ibex changes, this is done using a
patch file.

Signed-off-by: Pascal Nasahl <[email protected]>
(commit is original to earlgrey_1.0.0)
@nasahlpa nasahlpa marked this pull request as ready for review December 3, 2024 08:41
@nasahlpa nasahlpa requested a review from vogelpi December 3, 2024 08:41
This commit fixes the reset logic of the Ibex counter
module for the FPGA.

Signed-off-by: Pascal Nasahl <[email protected]>
(commit is original to earlgrey_1.0.0)
@nasahlpa nasahlpa force-pushed the earlgrey_1.0.0_ibex_patch branch from 27ef4b1 to 89ea9f8 Compare December 3, 2024 08:45
Copy link
Contributor

@vogelpi vogelpi left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks @nasahlpa !

For awareness, I am also tagging @andreaskurth and @moidx . We need this FPGA-only RTL changes to enable using the lockstep core on the CW340 board which is required for SiVal. This change is unique to the earlgrey_1.0.0 branch and we will do a different fix (without patches) on the master branch.

@vogelpi vogelpi requested review from andreaskurth and moidx December 4, 2024 00:13
Copy link
Contributor

@moidx moidx left a comment

Choose a reason for hiding this comment

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

I believe we still don't have bitstream caching enabled for earlgrey_1.0.0. @a-will can provide details on how to enable this.

@a-will
Copy link
Contributor

a-will commented Dec 4, 2024

I believe we still don't have bitstream caching enabled for earlgrey_1.0.0. @a-will can provide details on how to enable this.

Bitstreams are being cached, it looks like. The only thing to avoid would be using the latest hacky reference. bitstreams_workspace.py is currently hard-coded to look at master/latest.txt in the bucket for that. In general, though, the default value of "HEAD" should be used from a git repo.

@vogelpi
Copy link
Contributor

vogelpi commented Dec 9, 2024

CHANGE AUTHORIZED: hw/vendor/lowrisc_ibex/rtl/ibex_counter.sv
CHANGE AUTHORIZED: hw/vendor/lowrisc_ibex/rtl/ibex_register_file_fpga.sv

1 similar comment
@nasahlpa
Copy link
Member Author

nasahlpa commented Dec 9, 2024

CHANGE AUTHORIZED: hw/vendor/lowrisc_ibex/rtl/ibex_counter.sv
CHANGE AUTHORIZED: hw/vendor/lowrisc_ibex/rtl/ibex_register_file_fpga.sv

Comment on lines +58 to +63
if (CounterWidth < 49) begin : g_dsp_counter
// DSP output register requires synchronous reset.
`define COUNTER_FLOP_RST posedge clk_i
end else begin : g_no_dsp_counter
`define COUNTER_FLOP_RST posedge clk_i or negedge rst_ni
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note that this code relying on a pre-processor `define isn't generally correct, because the pre-processor would resolve the `define before elaboration resolves the parameterized if-else construct. However, this only affects Xilinx code (it's inside an `ifdef FPGA_XILINX) and @nasahlpa tested that this works as intended in Vivado.

On master a proper fix has been applied (#25458), but that would also modify ASIC RTL, which we don't allow on this branch.

@andreaskurth
Copy link
Contributor

CHANGE AUTHORIZED: hw/vendor/lowrisc_ibex/rtl/ibex_counter.sv
CHANGE AUTHORIZED: hw/vendor/lowrisc_ibex/rtl/ibex_register_file_fpga.sv

This only changes FPGA logic; ASIC logic isn't affected.

@luismarques luismarques merged commit c643d81 into lowRISC:earlgrey_1.0.0 Dec 9, 2024
31 checks passed
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.

6 participants