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

vivado: resolve net names for clocks to instance pins #302

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

Conversation

occheung
Copy link
Contributor

As described in m-labs/artiq#2093. Vivado may not keep net names during synthesis.

This PR modifies _convert_clock to resolve signals into its instance driver pin, and assign clock constraints to the pin instead of the signal. Should the signal unable to be resolved into its driving pin, constraints are still added to the signal.

Commits with similar methodology (but for totally different purpose) was also merged into nmigen. Note that ARTIQ build also suffer from TIMING-2 warning (among other methodology warnings) due to some potentially inappropriately used add_period_constraint()/create_clock.

This fixes m-labs/artiq#2093. However, some ARTIQ build may still suffer from explicit critical warning due to vivado not able to resolve net name (as the net was renamed/absorbed/etc. during synthesis). Such warnings look like such.

WARNING: [Vivado 12-1008] No clocks found for command 'get_clocks -include_generated_clocks -of [get_nets main_cdr_clk_buf]'. [/build/artiq_kc705/nist_clock/gateware/top.xdc:1034]
Resolution: Verify the create_clock command was called to create the clock object before it is referenced.
INFO: [Vivado 12-626] No clocks found. Please use 'create_clock' or 'create_generated_clock' command to create clocks. [/build/artiq_kc705/nist_clock/gateware/top.xdc:1034]
CRITICAL WARNING: [Vivado 12-4739] set_clock_groups:No valid object(s) found for '-group [get_clocks -of_objects [get_nets main_cdr_clk_buf]]'. [/build/artiq_kc705/nist_clock/gateware/top.xdc:1034]
Resolution: Check if the specified object(s) exists in the current design. If it does, ensure that the correct design hierarchy was specified for the object. If you are working with clocks, make sure create_clock was used to create the clock object before it is referenced.
CRITICAL WARNING: [Vivado 12-4739] set_clock_groups:No valid object(s) found for '-group '. [/build/artiq_kc705/nist_clock/gateware/top.xdc:1034]
Resolution: Check if the specified object(s) exists in the current design. If it does, ensure that the correct design hierarchy was specified for the object. If you are working with clocks, make sure create_clock was used to create the clock object before it is referenced.
CRITICAL WARNING: [Vivado 12-5201] set_clock_groups: cannot set the clock group when only one non-empty group remains. [/build/artiq_kc705/nist_clock/gateware/top.xdc:1034]

These signals were not declared as clocks (via add_period_constraint()/create_clock).

@occheung
Copy link
Contributor Author

ARTIQ Kasli 2.0 standalone's constraint file now looks like this (without peripherals):

create_clock -name sys_clk -period 8.0 [get_pins BUFG_6/O]

create_clock -name clk125_gtp_p -period 8.0 [get_nets clk125_gtp_p]

create_clock -name main_genericstandalone_txoutclk -period 16.0 [get_pins GTPE2_CHANNEL/TXOUTCLK]

create_clock -name main_genericstandalone_rxoutclk -period 16.0 [get_pins GTPE2_CHANNEL/RXOUTCLK]

create_clock -name cdr_clk_clean_p -period 8.0 [get_nets cdr_clk_clean_p]

set_clock_groups -group [get_clocks -include_generated_clocks sys_clk] -group [get_clocks -include_generated_clocks -of [get_nets bootstrap_clk]] -asynchronous

set_clock_groups -group [get_clocks -include_generated_clocks sys_clk] -group [get_clocks -include_generated_clocks -of [get_nets main_genericstandalone_genericstandalone_crg_clk125_buf]] -asynchronous

set_clock_groups -group [get_clocks -include_generated_clocks sys_clk] -group [get_clocks -include_generated_clocks -of [get_nets main_genericstandalone_genericstandalone_crg_pll_clk_bootstrap]] -asynchronous

set_clock_groups -group [get_clocks -include_generated_clocks sys_clk] -group [get_clocks -include_generated_clocks main_genericstandalone_txoutclk] -asynchronous

set_clock_groups -group [get_clocks -include_generated_clocks sys_clk] -group [get_clocks -include_generated_clocks main_genericstandalone_rxoutclk] -asynchronous

set_clock_groups -group [get_clocks -include_generated_clocks -of [get_nets bootstrap_clk]] -group [get_clocks -include_generated_clocks -of [get_nets main_genericstandalone_genericstandalone_crg_pll_clk_bootstrap]] -asynchronous

set_clock_groups -group [get_clocks -include_generated_clocks -of [get_nets main_genericstandalone_genericstandalone_crg_clk125_buf]] -group [get_clocks -include_generated_clocks -of [get_nets bootstrap_clk]] -asynchronous

set_clock_groups -group [get_clocks -include_generated_clocks -of [get_nets main_genericstandalone_genericstandalone_crg_clk125_buf]] -group [get_clocks -include_generated_clocks -of [get_nets main_genericstandalone_genericstandalone_crg_pll_clk_bootstrap]] -asynchronous

set_clock_groups -group [get_clocks -include_generated_clocks main_genericstandalone_txoutclk] -group [get_clocks -include_generated_clocks main_genericstandalone_rxoutclk] -asynchronous

@sbourdeauducq
Copy link
Member

These signals were not declared as clocks

Should they be declared then?

@occheung
Copy link
Contributor Author

These signals were not declared as clocks

Should they be declared then?

No. I think the false_path_constraints should not even be declared for these signals in this case.
On standalone, this false path constrain is between sys_clk and cdr_clk_buf. cdr_clk_buf drives nothing except the MMCM (clock switch between bootstrap_clk and cdr_clk).
On DRTIO targets, txoutclk is declared as clocks and has false path constraints with sys_clk in ARTIQ separately.

Constraint in question: https://github.com/m-labs/misoc/blob/b3467ff0052f46fd4f15b31441e904b635777683/misoc/targets/kc705.py#L195

@occheung
Copy link
Contributor Author

That being said, there is a need to specify bootstrap_clk and cdr_clk_buf as 2 different clocks due to the clock switch.

https://adaptivesupport.amd.com/s/question/0D54U000088bUL3SAM/timing-constraints-for-twoinputclock-mmcm?language=en_US

Then we should declare both cdr_clk_buf and bootstrap_clk as clocks.

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.

fix vivado set_clock_groups warnings
2 participants