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

Fix for issue #74; updates to use latest version of CV32E40P #82

Merged
merged 3 commits into from
Oct 26, 2020

Conversation

Silabs-ArjanB
Copy link
Contributor

This pull requests addresses the following:

Note that the original `ifndef VERILATOR to guard assertions causes problems for synthesis tools. Either the synthesis tool would object against the assertions or you would have to define VERILATOR (both of which are not okay). Now binding the assertions to the RTL for a cleaner split between assertions and RTL. Also got rid of the #0 construct in the assertions as that was not a clean solution.

Signed-off-by: Arjan Bink Arjan.Bink@silabs.com

…E40P; moved assertions out of RTL to prevent issues with various tools (e.g. synthesis tools)

Signed-off-by: Arjan Bink <Arjan.Bink@silabs.com>
Signed-off-by: Arjan Bink <Arjan.Bink@silabs.com>
src/dm_sba.sv Outdated
@@ -53,8 +53,7 @@ module dm_sba #(
output logic [2:0] sberror_o // bus error occurred
);

typedef enum logic [2:0] { Idle, Read, Write, WaitRead, WaitWrite } state_e;
state_e state_d, state_q;
dm::sba_state_t state_d, state_q;
Copy link
Contributor

Choose a reason for hiding this comment

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

enum types should end in _e

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a new commit to fix the naming issue

@zarubaf
Copy link
Contributor

zarubaf commented Oct 8, 2020

I think you are just running into the define problem with your synthesis tool because the // pragma translate_off is missing. I would strongly recommend not pulling out the assertions as I don't see any benefit from it - on the contrary - for integration (where you usually don't bind the assertions) you are missing out on any checks.

Signed-off-by: Arjan Bink <Arjan.Bink@silabs.com>
@Silabs-ArjanB
Copy link
Contributor Author

I think you are just running into the define problem with your synthesis tool because the // pragma translate_off is missing. I would strongly recommend not pulling out the assertions as I don't see any benefit from it - on the contrary - for integration (where you usually don't bind the assertions) you are missing out on any checks.

Not all tools use such pragmas. The only work-around we had was to define VERILATOR during ASIC and FPGA synthesis, which is really a hack. Other reason for putting the assertions outside is to get rid of define usage in RTL code and to offer a path for (later) converting to UVM style warnings/errors (as otherwise warnings/errors will go unnoticed in UVM simulations)

Copy link
Collaborator

@bluewww bluewww left a comment

Choose a reason for hiding this comment

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

LGTM.

I didn't have really good experience with bind and various eda tools, but If you think it makes future uvm support easier I can accept it.

@bluewww bluewww merged commit 74c0a42 into pulp-platform:master Oct 26, 2020
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.

None yet

3 participants