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

hw: Replace non-resettable FFs #154

Merged
merged 6 commits into from
Jul 23, 2024
Merged

hw: Replace non-resettable FFs #154

merged 6 commits into from
Jul 23, 2024

Conversation

fischeti
Copy link
Contributor

@fischeti fischeti commented Jul 3, 2024

Replace all non-resettable FlipFlops with resettable ones. Non-resettable FlipFlops have a marginally smaller area but can cause undefined behavior, and the common agreement is that non-resettable FlipFlops should be replaced everywhere if possible.

This PR also replaces all deprecated macros e.g. FFARN with FF, and adapts the parameters in the snitch_regfile modules to the CamelCase naming convention.

TODO

  • Credit original authors before merging
Co-authored-by: Milos Hirsl <[email protected]>
Co-authored-by: Thierry Dubochet <[email protected]>

@fischeti fischeti force-pushed the non-rst-ffs branch 2 times, most recently from 23a0335 to c619022 Compare July 3, 2024 13:02
@fischeti fischeti force-pushed the non-rst-ffs branch 2 times, most recently from 5de3bcf to 39fd927 Compare July 19, 2024 12:20
@fischeti fischeti marked this pull request as ready for review July 19, 2024 13:44
Copy link
Collaborator

@colluca colluca left a comment

Choose a reason for hiding this comment

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

LGTM, except for the change suggested.

@@ -601,20 +601,21 @@ module snitch_int_ss import riscv_instr::*; import snitch_ipu_pkg::*; import sni
);

for (genvar i = 0; i < 2; i++) begin : gen_multi_cycle_buffer
`FFLNR(imd_val_q[i], imd_val_q[i], imd_val_we[i], clk_i)
`FFL(imd_val_q[i], imd_val_q[i], imd_val_we[i], '0, clk_i, rst_i)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As an active-low FF, this should use rst_ni, or be changed to `FFLAR. Please double-check everywhere that `FFL gets an active-low reset, and viceversa.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks! I double-checked everything else again.

@colluca colluca merged commit c12ce9b into main Jul 23, 2024
27 checks passed
@colluca colluca deleted the non-rst-ffs branch July 23, 2024 13:31
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.

2 participants