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

Remove Verilog delay from DPI outputs and use falling edge instead #2635

Merged
merged 4 commits into from
Sep 15, 2020

Conversation

jackkoenig
Copy link
Contributor

Verilator ignores these delays (after warning the user), so they can
result in race conditions.

This fixes an issue visible on #2617 where some assertions fire erroneously due to some optimizations Verilator makes that are incorrect due to the race conditions.

Thank you to @ernie-sifive for doing this work

Related issue:

Type of change: bug fix

Impact: no functional change

Development Phase: implementation

Release Notes

Remove Verilog delay statements from SimJTAG.v and SimDTM.v

Verilator ignores these delays (after warning the user), so they can
result in race conditions.
@aswaterman
Copy link
Member

In general, using negedge clocking on half of a ready/valid interface is functionally incorrect. It might be the case that this interface is constrained in such a way that it's OK in this case, but I don't know.

jackkoenig and others added 2 commits September 11, 2020 20:05
Co-authored-by: Ernie Edgar <43148441+ernie-sifive@users.noreply.github.com>
Copy link
Contributor

@ernie-sifive ernie-sifive left a comment

Choose a reason for hiding this comment

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

All checks passed on latest commit. Changed output back to posedge to confine any negedge to within the SimDTM module.

@jackkoenig
Copy link
Contributor Author

Is there a fundamental reason why the logic needs to be different for Verilator? Why does the negedge timing work for Verilator but not VCS?

@aswaterman
Copy link
Member

Why does the negedge timing work for Verilator but not VCS?

This is a red flag for me, too.

@aswaterman
Copy link
Member

I haven't thought too hard about the details, but superficially, this looks good to me.

@jackkoenig
Copy link
Contributor Author

Let me verify that this works on the 3.4 branch then hopefully we're good to go!

@jackkoenig
Copy link
Contributor Author

It's working on the 3.4 branch!

@jackkoenig
Copy link
Contributor Author

I believe this is right so I'm going to merge

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