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

tc_clk_xilinx.sv: Add IS_FUNCTIONAL parameter on clk_mux #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

CyrilKoe
Copy link
Contributor

@CyrilKoe CyrilKoe commented Jul 4, 2023

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.

@luca-valente
Copy link
Contributor

@CyrilKoe We need to add the parameter also in the RTL target file so that it is actually possible to use it. Here as for the clock gating cell.

Copy link
Collaborator

@niwis niwis left a 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?

/// 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
Copy link
Collaborator

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.

Copy link
Contributor

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.

Copy link
Contributor

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 😄

Copy link
Contributor Author

@CyrilKoe CyrilKoe Jul 6, 2023

Choose a reason for hiding this comment

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

EN_BUF_FPGA ?

Copy link
Collaborator

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.

Copy link
Contributor

@luca-valente luca-valente Jul 6, 2023

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 😄

@CyrilKoe CyrilKoe force-pushed the fix/fpga_clk_mux branch from 6f43739 to 87b59bf Compare July 6, 2023 08:59
@CyrilKoe CyrilKoe force-pushed the fix/fpga_clk_mux branch from 87b59bf to 360fe59 Compare July 6, 2023 09:15
@CyrilKoe
Copy link
Contributor Author

CyrilKoe commented Jul 6, 2023

@niwis @luca-valente
Canged now, if you agree with using this parameter I will start a Carfield CI with the change and I will comment here if it passed.

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