You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
CDC primitives are particularly troublesome for synthesizers because they interact badly with features like retiming. Because of this, toolchains require them to be marked in some way, often by instantiating them in a specific way, adding attributes, etc.
This was handled in oMigen in a particularly awkward way, essentially privileging ISE and Vivado above any other toolchain, and tailoring the generic primitives towards them as well. (The attr_translate mechanism looks generic, but in practice it was only barely flexible enough to support these two toolchains.)
In this issue I'd like to collect toolchain requirements so that a more flexible implementation can be done, one that would not be tied to Xilinx.
The following are the uses of attr.add and attr= in oMigen, assuming they are the only places where such attributes originate:
In MultiRegImpl, no_retiming is added on internal registers.
In BusSynchronizer, something similar is done to the buffer. (Can this be done in terms of MultiReg?)
In XilinxMultiRegImpl, mr_ff, async_reg and no_shreg_extract are added on internal registers.
In XilinxAsyncResetSynchronizerImpl, ars_ff1 and ars_ff2 are added on the internal registers.
(Aside: since RTLIL can directly represent async flops, nMigen's [Async]ResetSynchronizer in principle does not have to be lowered to a platform-specific implementation.)
Each toolchain that has special requirements for MultiReg, ResetSynchronizer and possibly other primitives, should make sure they are lowered to an appropriate platform-specific implementation. Once that happens, it is free to add false path, etc, constraints in any way it considers appropriate.
Beyond that I'd like to read reference material, particularly from Xilinx, explaining exactly how to add constraints to CDC primitives. In particular the Vivado implementation seems complex and perhaps overly fragile, with the hardcoded 2ns delta:
XilinxVivadoToolchain._constrain
def_constrain(self, platform):
# The asynchronous input to a MultiReg is a false pathplatform.add_platform_command(
"set_false_path -quiet ""-to [get_nets -filter {{mr_ff == TRUE}}]"
)
# The asychronous reset input to the AsyncResetSynchronizer is a false# pathplatform.add_platform_command(
"set_false_path -quiet ""-to [get_pins -filter {{REF_PIN_NAME == PRE}} ""-of [get_cells -filter {{ars_ff1 == TRUE || ars_ff2 == TRUE}}]]"
)
# clock_period-2ns to resolve metastability on the wire between the# AsyncResetSynchronizer FFsplatform.add_platform_command(
"set_max_delay 2 -quiet ""-from [get_pins -filter {{REF_PIN_NAME == Q}} ""-of [get_cells -filter {{ars_ff1 == TRUE}}]] ""-to [get_pins -filter {{REF_PIN_NAME == D}} ""-of [get_cells -filter {{ars_ff2 == TRUE}}]]"
)
The text was updated successfully, but these errors were encountered:
Based on @dlharmon's investigation in #227, I feel confident that per-platform overrides for CDC primitives are as good or better than the attr_translate mechanism.
CDC primitives are particularly troublesome for synthesizers because they interact badly with features like retiming. Because of this, toolchains require them to be marked in some way, often by instantiating them in a specific way, adding attributes, etc.
This was handled in oMigen in a particularly awkward way, essentially privileging ISE and Vivado above any other toolchain, and tailoring the generic primitives towards them as well. (The
attr_translate
mechanism looks generic, but in practice it was only barely flexible enough to support these two toolchains.)In this issue I'd like to collect toolchain requirements so that a more flexible implementation can be done, one that would not be tied to Xilinx.
The following are the uses of
attr.add
andattr=
in oMigen, assuming they are the only places where such attributes originate:MultiRegImpl
,no_retiming
is added on internal registers.BusSynchronizer
, something similar is done to the buffer. (Can this be done in terms of MultiReg?)XilinxMultiRegImpl
,mr_ff
,async_reg
andno_shreg_extract
are added on internal registers.XilinxAsyncResetSynchronizerImpl
,ars_ff1
andars_ff2
are added on the internal registers.Here is my overall proposal:
lib.cdc
primitives should not make any attempt to influence toolchain behavior. In particular, explicitly marked CDC (Require signals crossing clock domains to be explicitly marked #4) should be required and sufficient to inhibit retiming across CDC primitives. (With the design proposed in Require signals crossing clock domains to be explicitly marked #4, only the first flop in a chain will be marked as performing CDC. This is a problem for extracting no_retiming attributes that needs to be solved.)[Async]ResetSynchronizer
in principle does not have to be lowered to a platform-specific implementation.)MultiReg
,ResetSynchronizer
and possibly other primitives, should make sure they are lowered to an appropriate platform-specific implementation. Once that happens, it is free to add false path, etc, constraints in any way it considers appropriate.@sbourdeauducq @jordens Feedback? Objections?
Beyond that I'd like to read reference material, particularly from Xilinx, explaining exactly how to add constraints to CDC primitives. In particular the Vivado implementation seems complex and perhaps overly fragile, with the hardcoded 2ns delta:
XilinxVivadoToolchain._constrain
The text was updated successfully, but these errors were encountered: