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 empty statement in sequential block warning #17

Closed
wants to merge 5 commits into from

Conversation

d953i
Copy link

@d953i d953i commented Dec 12, 2019

Delete extra semicolon.

No perfect solution for Verilog2001 - can't use localparam for module ports, so have to either  change localparam to parameters or use expression in ports definition.
Doing latter in this commit.
@ZipCPU
Copy link
Owner

ZipCPU commented Dec 12, 2019

Looking over your proposed changes, I like the removal of the pair of semicolons. Thanks for catching that! I'm running regression tests on that change now, but don't foresee any problems to accepting that change.

As for the second change request, I'm going to say, no thanks. localparam has been around for over ten years. It's been around long enough that it is supported by open source synthesis tools such as yosys, even when using ISE as a back end, and so using it shouldn't be a problem with any synthesis tool that I am aware of.

@ZipCPU ZipCPU closed this Dec 12, 2019
@d953i
Copy link
Author

d953i commented Dec 12, 2019

Heh. Looks like Github just combined 2 commits to 1 pull request - I actually didn't intent to submit second one. Sorry.

localparam's can't be used for module port in Vivado project mode/block diagram even thou synth have no problem synthesizing it. Not a big deal.
localparam_warning

@ZipCPU
Copy link
Owner

ZipCPU commented Dec 12, 2019

Wait, seriously??? Vivado won't accept it?

Can you tell me which Vivado version you tried this with?

@ZipCPU ZipCPU reopened this Dec 12, 2019
@d953i
Copy link
Author

d953i commented Dec 12, 2019

Vivado 2019.2
But then again - only in project/block diagram mode.

May need to increase it for a slave with large latency
@d953i d953i closed this Dec 17, 2019
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