-
Notifications
You must be signed in to change notification settings - Fork 30
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
tc_clk_xilinx.sv: Add IS_FUNCTIONAL
parameter on clk_mux
#28
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CyrilKoe I'm not sure if IS_FUNCTIONAL
makes sense here. A multiplexer is always functional. Why is the tc_clk_mux
used for non-clock signals?
src/fpga/tc_clk_xilinx.sv
Outdated
/// Using BUFGMUX on FPGA can allocate limited clock ressources | ||
/// to non clock signals. It can be disabled with | ||
/// IS_FUNCTIONAL = 0 | ||
parameter bit IS_FUNCTIONAL = 1'b0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default needs to remain 1'b1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@niwis the goal is to disable the default instantiation of the BUFGMUX
, as it consumes lots of clock resources when it is not actually needed. When needed, who's instantiating the IP has to explicitly set it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can find a different name for the parameter 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EN_BUF_FPGA ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand from the comment that the BUFGMUX
is not wanted if you multiplex a non-clock signal. My question is: why do you use tc_clk_mux
for these non-clock signals in the first place? Maybe the place to solve this would be one hierarchy level above (by not instantiating this module). But I might be missing something; happy to discuss this offline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of "legacy" code using the tc_clk_mux
... let's discuss it offline 😄
6f43739
to
87b59bf
Compare
87b59bf
to
360fe59
Compare
@niwis @luca-valente |
The goal is to disable the default instantiation of the BUFGMUX, as it consumes lots of clock resources when it is not actually needed. It can also create long BUF chains that can fail DRCs.
This can often fail Vivado implementations.
When buffering is needed, who's instantiating the IP has to explicitly set it.