From 139d7644aaf29214f4ea8294fd96b3d6ccb43d89 Mon Sep 17 00:00:00 2001 From: Darrell Harmon Date: Thu, 19 Sep 2019 21:58:01 -0600 Subject: [PATCH 01/14] Xilinx specific MultiReg Vivado was inferring an SRL16 from a MultiReg in some cases --- nmigen/vendor/xilinx_7series.py | 8 ++++++++ nmigen/vendor/xilinx_spartan_3_6.py | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/nmigen/vendor/xilinx_7series.py b/nmigen/vendor/xilinx_7series.py index 6e7d2279..d78581c1 100644 --- a/nmigen/vendor/xilinx_7series.py +++ b/nmigen/vendor/xilinx_7series.py @@ -360,3 +360,11 @@ def get_diff_input_output(self, pin, p_port, n_port, attrs, invert): io_IO=p_port[bit], io_IOB=n_port[bit] ) return m + + def get_multi_reg(self, multireg): + m = Module() + for i, o in zip((multireg.i, *multireg._regs), multireg._regs): + o.attrs["ASYNC_REG"] = "TRUE" + m.d[multireg._o_domain] += o.eq(i) + m.d.comb += multireg.o.eq(multireg._regs[-1]) + return m diff --git a/nmigen/vendor/xilinx_spartan_3_6.py b/nmigen/vendor/xilinx_spartan_3_6.py index 00d6831d..25b2a592 100644 --- a/nmigen/vendor/xilinx_spartan_3_6.py +++ b/nmigen/vendor/xilinx_spartan_3_6.py @@ -411,6 +411,14 @@ def get_diff_input_output(self, pin, p_port, n_port, attrs, invert): ) return m + def get_multi_reg(self, multireg): + m = Module() + for i, o in zip((multireg.i, *multireg._regs), multireg._regs): + o.attrs["ASYNC_REG"] = "TRUE" + m.d[multireg._o_domain] += o.eq(i) + m.d.comb += multireg.o.eq(multireg._regs[-1]) + return m + XilinxSpartan3APlatform = XilinxSpartan3Or6Platform XilinxSpartan6Platform = XilinxSpartan3Or6Platform From a57ca63e9a7ec45829fd9e2ef032d44a41854210 Mon Sep 17 00:00:00 2001 From: Darrell Harmon Date: Sun, 22 Sep 2019 14:46:02 -0600 Subject: [PATCH 02/14] add clock domain crossing constraints on Vivado This tags the first register in each `MultiReg` or `ResetSynchronizer` with the attribute `nmigen_async_ff` and then applies a false path and max delay constraint to all registers tagged with that attribute in the `.xdc` file. The max delay defaults to 5 ns and has an override, `max_delay` where it can be changed for the > whole project. It's possible to make this an argument to `MultiReg` instead, but is more complex. > git commit -m "add clock domain crossing constraints on Vivado This tags the first register in each `MultiReg` or `ResetSynchronizer` with the attribute `nmigen_async_ff` and then applies a false path and max delay constraint to all registers tagged with that attribute in the `.xdc` file. The max delay defaults to 5 ns and has an override, `max_delay` where it can be changed for the whole project. It's possible to make this an optional argument to `MultiReg` instead, but is more complex. It would probably work to set `nmigen_async_ff` to the desired delay rather than just `TRUE`. I'm not sure how hard it would be to extract that in the TCL or if it would be easier to keep a dict of all used delay values and put a line for each into the `.xdc` file. --- nmigen/vendor/xilinx_7series.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/nmigen/vendor/xilinx_7series.py b/nmigen/vendor/xilinx_7series.py index d78581c1..ce83d866 100644 --- a/nmigen/vendor/xilinx_7series.py +++ b/nmigen/vendor/xilinx_7series.py @@ -24,6 +24,7 @@ class Xilinx7SeriesPlatform(TemplatedPlatform): * ``script_after_bitstream``: inserts commands after ``write_bitstream`` in Tcl script. * ``add_constraints``: inserts commands in XDC file. * ``vivado_opts``: adds extra options for ``vivado``. + * ``max_delay``: sets the maximum delay in nanoseconds for clock domain crossing constraints. Build products: * ``{{name}}.log``: Vivado log. @@ -115,6 +116,8 @@ class Xilinx7SeriesPlatform(TemplatedPlatform): {% for signal, frequency in platform.iter_clock_constraints() -%} create_clock -name {{signal.name}} -period {{1000000000/frequency}} [get_nets {{signal|hierarchy("/")}}] {% endfor %} + set_false_path -to [get_cells -hier -filter {nmigen_async_ff == TRUE}] + set_max_delay {{get_override("max_delay")|default("5.0")}} -to [get_cells -hier -filter {nmigen_async_ff == TRUE}] {{get_override("add_constraints")|default("# (add_constraints placeholder)")}} """ } @@ -363,8 +366,27 @@ def get_diff_input_output(self, pin, p_port, n_port, attrs, invert): def get_multi_reg(self, multireg): m = Module() + # the `nmigen_async_ff` attribute is used in the constraints file to find the + # first register in each MultiReg and add false path and max delay constraints + multireg._regs[0].attrs["nmigen_async_ff"]="TRUE" for i, o in zip((multireg.i, *multireg._regs), multireg._regs): + # Vivado uses the `ASYNC_REG` attribute to prevent SRL inferrence, + # in clock domain crossing reporting and for placement o.attrs["ASYNC_REG"] = "TRUE" m.d[multireg._o_domain] += o.eq(i) m.d.comb += multireg.o.eq(multireg._regs[-1]) return m + + def get_reset_sync(self, resetsync): + m = Module() + m.domains += ClockDomain("reset_sync", async_reset=True, local=True) + resetsync._regs[0].attrs["nmigen_async_ff"]="TRUE" + for i, o in zip((0, *resetsync._regs), resetsync._regs): + o.attrs["ASYNC_REG"] = "TRUE" + m.d.reset_sync += o.eq(i) + m.d.comb += [ + ClockSignal("reset_sync").eq(ClockSignal(resetsync._domain)), + ResetSignal("reset_sync").eq(resetsync.arst), + ResetSignal(resetsync._domain).eq(resetsync._regs[-1]) + ] + return m From abad6db924f2b07a348a4e7652e7a005fd5134cc Mon Sep 17 00:00:00 2001 From: Darrell Harmon Date: Sun, 22 Sep 2019 16:47:55 -0600 Subject: [PATCH 03/14] replace false path with min delay the false path had precedence over the max_delay despite the docs the large negative min delay effectively disables hold path timing which was the intended goal of the false path. --- nmigen/vendor/xilinx_7series.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nmigen/vendor/xilinx_7series.py b/nmigen/vendor/xilinx_7series.py index c9336afa..cef3657d 100644 --- a/nmigen/vendor/xilinx_7series.py +++ b/nmigen/vendor/xilinx_7series.py @@ -116,7 +116,7 @@ class Xilinx7SeriesPlatform(TemplatedPlatform): {% for signal, frequency in platform.iter_clock_constraints() -%} create_clock -name {{signal.name}} -period {{1000000000/frequency}} [get_nets {{signal|hierarchy("/")}}] {% endfor %} - set_false_path -to [get_cells -hier -filter {nmigen_async_ff == TRUE}] + set_min_delay -1000.0 -to [get_cells -hier -filter {nmigen_async_ff == TRUE}] set_max_delay {{get_override("max_delay")|default("5.0")}} -to [get_cells -hier -filter {nmigen_async_ff == TRUE}] {{get_override("add_constraints")|default("# (add_constraints placeholder)")}} """ From 6b64f71b3eeabb69e10c19f68cc3586f207a5a10 Mon Sep 17 00:00:00 2001 From: Darrell Harmon Date: Mon, 23 Sep 2019 10:02:23 -0600 Subject: [PATCH 04/14] missed a few variable names in upstream change --- nmigen/vendor/xilinx_7series.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/nmigen/vendor/xilinx_7series.py b/nmigen/vendor/xilinx_7series.py index 1b331c1e..96298284 100644 --- a/nmigen/vendor/xilinx_7series.py +++ b/nmigen/vendor/xilinx_7series.py @@ -368,7 +368,7 @@ def get_ff_sync(self, ff_sync): m = Module() # the `nmigen_async_ff` attribute is used in the constraints file to find the # first register in each MultiReg and add false path and max delay constraints - ff_sync._regs[0].attrs["nmigen_async_ff"]="TRUE" + ff_sync._stages[0].attrs["nmigen_async_ff"]="TRUE" for i, o in zip((ff_sync.i, *ff_sync._stages), ff_sync._stages): # Vivado uses the `ASYNC_REG` attribute to prevent SRL inferrence, # in clock domain crossing reporting and for placement @@ -380,13 +380,13 @@ def get_ff_sync(self, ff_sync): def get_reset_sync(self, resetsync): m = Module() m.domains += ClockDomain("reset_sync", async_reset=True, local=True) - resetsync._regs[0].attrs["nmigen_async_ff"]="TRUE" - for i, o in zip((0, *resetsync._regs), resetsync._regs): + resetsync._stages[0].attrs["nmigen_async_ff"]="TRUE" + for i, o in zip((0, *resetsync._stages), resetsync._stages): o.attrs["ASYNC_REG"] = "TRUE" m.d.reset_sync += o.eq(i) m.d.comb += [ ClockSignal("reset_sync").eq(ClockSignal(resetsync._domain)), ResetSignal("reset_sync").eq(resetsync.arst), - ResetSignal(resetsync._domain).eq(resetsync._regs[-1]) + ResetSignal(resetsync._domain).eq(resetsync._stages[-1]) ] return m From d3580192f79fabfa5d3a5feac6ab95e2c2bb8ea7 Mon Sep 17 00:00:00 2001 From: Darrell Harmon Date: Mon, 23 Sep 2019 10:03:13 -0600 Subject: [PATCH 05/14] change nmigen_async_ff to nmignen.async_ff for consistency --- nmigen/vendor/xilinx_7series.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/nmigen/vendor/xilinx_7series.py b/nmigen/vendor/xilinx_7series.py index 96298284..96dd9821 100644 --- a/nmigen/vendor/xilinx_7series.py +++ b/nmigen/vendor/xilinx_7series.py @@ -116,8 +116,8 @@ class Xilinx7SeriesPlatform(TemplatedPlatform): {% for signal, frequency in platform.iter_clock_constraints() -%} create_clock -name {{signal.name}} -period {{1000000000/frequency}} [get_nets {{signal|hierarchy("/")}}] {% endfor %} - set_min_delay -1000.0 -to [get_cells -hier -filter {nmigen_async_ff == TRUE}] - set_max_delay {{get_override("max_delay")|default("5.0")}} -to [get_cells -hier -filter {nmigen_async_ff == TRUE}] + set_min_delay -1000.0 -to [get_cells -hier -filter {nmigen.async_ff == TRUE}] + set_max_delay {{get_override("max_delay")|default("5.0")}} -to [get_cells -hier -filter {nmigen.async_ff == TRUE}] {{get_override("add_constraints")|default("# (add_constraints placeholder)")}} """ } @@ -366,9 +366,9 @@ def get_diff_input_output(self, pin, p_port, n_port, attrs, invert): def get_ff_sync(self, ff_sync): m = Module() - # the `nmigen_async_ff` attribute is used in the constraints file to find the + # the `nmigen.async_ff` attribute is used in the constraints file to find the # first register in each MultiReg and add false path and max delay constraints - ff_sync._stages[0].attrs["nmigen_async_ff"]="TRUE" + ff_sync._stages[0].attrs["nmigen.async_ff"]="TRUE" for i, o in zip((ff_sync.i, *ff_sync._stages), ff_sync._stages): # Vivado uses the `ASYNC_REG` attribute to prevent SRL inferrence, # in clock domain crossing reporting and for placement @@ -380,7 +380,7 @@ def get_ff_sync(self, ff_sync): def get_reset_sync(self, resetsync): m = Module() m.domains += ClockDomain("reset_sync", async_reset=True, local=True) - resetsync._stages[0].attrs["nmigen_async_ff"]="TRUE" + resetsync._stages[0].attrs["nmigen.async_ff"]="TRUE" for i, o in zip((0, *resetsync._stages), resetsync._stages): o.attrs["ASYNC_REG"] = "TRUE" m.d.reset_sync += o.eq(i) From 11eb6a81b8532b04ca2e60deec02cf3925626962 Mon Sep 17 00:00:00 2001 From: Darrell Harmon Date: Mon, 23 Sep 2019 10:09:58 -0600 Subject: [PATCH 06/14] use set_false_path -hold instead of set_min_delay This is effectively equivalent without the ugly constant There is now no hold path in the timing report, but it retains the max_delay. --- nmigen/vendor/xilinx_7series.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nmigen/vendor/xilinx_7series.py b/nmigen/vendor/xilinx_7series.py index 96dd9821..d5b11d4b 100644 --- a/nmigen/vendor/xilinx_7series.py +++ b/nmigen/vendor/xilinx_7series.py @@ -116,7 +116,7 @@ class Xilinx7SeriesPlatform(TemplatedPlatform): {% for signal, frequency in platform.iter_clock_constraints() -%} create_clock -name {{signal.name}} -period {{1000000000/frequency}} [get_nets {{signal|hierarchy("/")}}] {% endfor %} - set_min_delay -1000.0 -to [get_cells -hier -filter {nmigen.async_ff == TRUE}] + set_false_path -hold -to [get_cells -hier -filter {nmigen.async_ff == TRUE}] set_max_delay {{get_override("max_delay")|default("5.0")}} -to [get_cells -hier -filter {nmigen.async_ff == TRUE}] {{get_override("add_constraints")|default("# (add_constraints placeholder)")}} """ From 2183964a4568c8b4d5bd6ccf2ea86eb399a8cb4d Mon Sep 17 00:00:00 2001 From: Darrell Harmon Date: Mon, 23 Sep 2019 13:02:07 -0600 Subject: [PATCH 07/14] whitequark's suggested change to use TCL loop, pass delay with attribute --- nmigen/vendor/xilinx_7series.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/nmigen/vendor/xilinx_7series.py b/nmigen/vendor/xilinx_7series.py index ab7537e7..953229a3 100644 --- a/nmigen/vendor/xilinx_7series.py +++ b/nmigen/vendor/xilinx_7series.py @@ -79,6 +79,10 @@ class Xilinx7SeriesPlatform(TemplatedPlatform): {% endfor %} {{get_override("script_after_read")|default("# (script_after_read placeholder)")}} synth_design -top {{name}} -part {{platform.device}}{{platform.package}}-{{platform.speed}} + set_false_path -hold -to [get_cells -hier -filter {nmigen.vivado.false_path == TRUE}] + foreach cell [get_cells -hier -filter {nmigen.vivado.max_delay != ""}] { + set_max_delay [get_property nmigen.vivado.max_delay $cell] -to $cell + } {{get_override("script_after_synth")|default("# (script_after_synth placeholder)")}} report_timing_summary -file {{name}}_timing_synth.rpt report_utilization -hierarchical -file {{name}}_utilization_hierachical_synth.rpt @@ -116,8 +120,6 @@ class Xilinx7SeriesPlatform(TemplatedPlatform): {% for signal, frequency in platform.iter_clock_constraints() -%} create_clock -name {{signal.name}} -period {{1000000000/frequency}} [get_nets {{signal|hierarchy("/")}}] {% endfor %} - set_false_path -hold -to [get_cells -hier -filter {nmigen.async_ff == TRUE}] - set_max_delay {{get_override("max_delay")|default("5.0")}} -to [get_cells -hier -filter {nmigen.async_ff == TRUE}] {{get_override("add_constraints")|default("# (add_constraints placeholder)")}} """ } @@ -366,15 +368,12 @@ def get_diff_input_output(self, pin, p_port, n_port, attrs, invert): def get_ff_sync(self, ff_sync): m = Module() - # Vivado uses the `ASYNC_REG` attribute to prevent SRL inferrence, - # in clock domain crossing reporting and for placement - # the `nmigen.async_ff` attribute is used in the constraints file to find the - # first register in each MultiReg and add false path and max delay constraints flops = [Signal(ff_sync.i.shape(), name="stage{}".format(index), reset=ff_sync._reset, reset_less=ff_sync._reset_less, attrs={"ASYNC_REG": "TRUE"}) for index in range(ff_sync._stages)] - flops[0].attrs["nmigen.async_ff"]="TRUE" + flops[0].attrs["nmigen.vivado.false_path"] = "TRUE" + flops[0].attrs["nmigen.vivado.max_delay"] = "5.0" # FIXME for i, o in zip((ff_sync.i, *flops), flops): m.d[ff_sync._o_domain] += o.eq(i) m.d.comb += ff_sync.o.eq(flops[-1]) @@ -386,7 +385,8 @@ def get_reset_sync(self, resetsync): flops = [Signal(1, name="stage{}".format(index), reset=1, attrs={"ASYNC_REG": "TRUE"}) for index in range(reset_sync._stages)] - flops[0].attrs["nmigen.async_ff"]="TRUE" + flops[0].attrs["nmigen.vivado.false_path"] = "TRUE" + flops[0].attrs["nmigen.vivado.max_delay"] = "5.0" # FIXME for i, o in zip((0, *flops), flops): m.d.reset_sync += o.eq(i) m.d.comb += [ From f587c733bc2f30d0a59ff240178c601cac6f0f99 Mon Sep 17 00:00:00 2001 From: Darrell Harmon Date: Mon, 23 Sep 2019 16:46:02 -0600 Subject: [PATCH 08/14] use datapath_only instead of false_path --- nmigen/vendor/xilinx_7series.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/nmigen/vendor/xilinx_7series.py b/nmigen/vendor/xilinx_7series.py index 953229a3..da1b0a27 100644 --- a/nmigen/vendor/xilinx_7series.py +++ b/nmigen/vendor/xilinx_7series.py @@ -24,7 +24,6 @@ class Xilinx7SeriesPlatform(TemplatedPlatform): * ``script_after_bitstream``: inserts commands after ``write_bitstream`` in Tcl script. * ``add_constraints``: inserts commands in XDC file. * ``vivado_opts``: adds extra options for ``vivado``. - * ``max_delay``: sets the maximum delay in nanoseconds for clock domain crossing constraints. Build products: * ``{{name}}.log``: Vivado log. @@ -79,9 +78,15 @@ class Xilinx7SeriesPlatform(TemplatedPlatform): {% endfor %} {{get_override("script_after_read")|default("# (script_after_read placeholder)")}} synth_design -top {{name}} -part {{platform.device}}{{platform.package}}-{{platform.speed}} - set_false_path -hold -to [get_cells -hier -filter {nmigen.vivado.false_path == TRUE}] foreach cell [get_cells -hier -filter {nmigen.vivado.max_delay != ""}] { - set_max_delay [get_property nmigen.vivado.max_delay $cell] -to $cell + set clock [get_clocks -quiet -of_objects \ + [all_fanin -flat -startpoints_only [get_pin $cell/D]]] + if {[llength $clock] != 0} { + set_max_delay -datapath_only -from $clock \ + -to [get_cells $cell] [get_property nmigen.vivado.max_delay $cell] + } else { + set_max_delay -to [get_cells $cell] [get_property nmigen.vivado.max_delay $cell] + } } {{get_override("script_after_synth")|default("# (script_after_synth placeholder)")}} report_timing_summary -file {{name}}_timing_synth.rpt @@ -372,7 +377,6 @@ def get_ff_sync(self, ff_sync): reset=ff_sync._reset, reset_less=ff_sync._reset_less, attrs={"ASYNC_REG": "TRUE"}) for index in range(ff_sync._stages)] - flops[0].attrs["nmigen.vivado.false_path"] = "TRUE" flops[0].attrs["nmigen.vivado.max_delay"] = "5.0" # FIXME for i, o in zip((ff_sync.i, *flops), flops): m.d[ff_sync._o_domain] += o.eq(i) @@ -385,7 +389,6 @@ def get_reset_sync(self, resetsync): flops = [Signal(1, name="stage{}".format(index), reset=1, attrs={"ASYNC_REG": "TRUE"}) for index in range(reset_sync._stages)] - flops[0].attrs["nmigen.vivado.false_path"] = "TRUE" flops[0].attrs["nmigen.vivado.max_delay"] = "5.0" # FIXME for i, o in zip((0, *flops), flops): m.d.reset_sync += o.eq(i) From d7bc91c77b584a3120d34b92b6bef77191f1ffff Mon Sep 17 00:00:00 2001 From: Darrell Harmon Date: Mon, 23 Sep 2019 17:45:13 -0600 Subject: [PATCH 09/14] use false path if no max input delay else max delay --- nmigen/lib/cdc.py | 11 +++++++++-- nmigen/vendor/xilinx_7series.py | 19 +++++++++++++------ nmigen/vendor/xilinx_spartan_3_6.py | 2 ++ 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/nmigen/lib/cdc.py b/nmigen/lib/cdc.py index d21df79a..744c8a96 100644 --- a/nmigen/lib/cdc.py +++ b/nmigen/lib/cdc.py @@ -61,7 +61,8 @@ class FFSynchronizer(Elaboratable): :class:`FFSynchronizer` is reset by the ``o_domain`` reset only. """ - def __init__(self, i, o, *, o_domain="sync", reset=0, reset_less=True, stages=2): + def __init__(self, i, o, *, o_domain="sync", reset=0, reset_less=True, stages=2, + max_input_delay=None): _check_stages(stages) self.i = i @@ -71,11 +72,14 @@ def __init__(self, i, o, *, o_domain="sync", reset=0, reset_less=True, stages=2) self._reset_less = reset_less self._o_domain = o_domain self._stages = stages + self._max_input_delay = max_input_delay def elaborate(self, platform): if hasattr(platform, "get_ff_sync"): return platform.get_ff_sync(self) + assert _max_input_delay is None + m = Module() flops = [Signal(self.i.shape(), name="stage{}".format(index), reset=self._reset, reset_less=self._reset_less) @@ -117,18 +121,21 @@ class ResetSynchronizer(Elaboratable): Define the ``get_reset_sync`` platform method to override the implementation of :class:`ResetSynchronizer`, e.g. to instantiate library cells directly. """ - def __init__(self, arst, *, domain="sync", stages=2): + def __init__(self, arst, *, domain="sync", stages=2, max_input_delay=None): _check_stages(stages) self.arst = arst self._domain = domain self._stages = stages + self._max_input_delay = max_input_delay def elaborate(self, platform): if hasattr(platform, "get_reset_sync"): return platform.get_reset_sync(self) + assert _max_input_delay is None + m = Module() m.domains += ClockDomain("reset_sync", async_reset=True, local=True) flops = [Signal(1, name="stage{}".format(index), reset=1) diff --git a/nmigen/vendor/xilinx_7series.py b/nmigen/vendor/xilinx_7series.py index 292aa83d..7ec30417 100644 --- a/nmigen/vendor/xilinx_7series.py +++ b/nmigen/vendor/xilinx_7series.py @@ -78,14 +78,15 @@ class Xilinx7SeriesPlatform(TemplatedPlatform): {% endfor %} {{get_override("script_after_read")|default("# (script_after_read placeholder)")}} synth_design -top {{name}} -part {{platform.device}}{{platform.package}}-{{platform.speed}} - foreach cell [get_cells -hier -filter {nmigen.vivado.max_delay != ""}] { - set clock [get_clocks -quiet -of_objects \ + foreach cell [get_cells -quiet -hier -filter {nmigen.vivado.false_path == "TRUE"}] { + set_false_path -to $cell + } + foreach cell [get_cells -quiet -hier -filter {nmigen.vivado.max_delay != ""}] { + set clock [get_clocks -of_objects \ [all_fanin -flat -startpoints_only [get_pin $cell/D]]] if {[llength $clock] != 0} { set_max_delay -datapath_only -from $clock \ -to [get_cells $cell] [get_property nmigen.vivado.max_delay $cell] - } else { - set_max_delay -to [get_cells $cell] [get_property nmigen.vivado.max_delay $cell] } } {{get_override("script_after_synth")|default("# (script_after_synth placeholder)")}} @@ -377,7 +378,10 @@ def get_ff_sync(self, ff_sync): reset=ff_sync._reset, reset_less=ff_sync._reset_less, attrs={"ASYNC_REG": "TRUE"}) for index in range(ff_sync._stages)] - flops[0].attrs["nmigen.vivado.max_delay"] = "5.0" # FIXME + if ff_sync._max_input_delay is None: + flops[0].attrs["nmigen.vivado.false_path"] = "TRUE" + else: + flops[0].attrs["nmigen.vivado.max_delay"] = ff_sync._max_input_delay for i, o in zip((ff_sync.i, *flops), flops): m.d[ff_sync._o_domain] += o.eq(i) m.d.comb += ff_sync.o.eq(flops[-1]) @@ -389,7 +393,10 @@ def get_reset_sync(self, reset_sync): flops = [Signal(1, name="stage{}".format(index), reset=1, attrs={"ASYNC_REG": "TRUE"}) for index in range(reset_sync._stages)] - flops[0].attrs["nmigen.vivado.max_delay"] = "5.0" # FIXME + if reset_sync._max_input_delay is None: + flops[0].attrs["nmigen.vivado.false_path"] = "TRUE" + else: + flops[0].attrs["nmigen.vivado.max_delay"] = reset_sync._max_input_delay for i, o in zip((0, *flops), flops): m.d.reset_sync += o.eq(i) m.d.comb += [ diff --git a/nmigen/vendor/xilinx_spartan_3_6.py b/nmigen/vendor/xilinx_spartan_3_6.py index 284cfab1..83b26721 100644 --- a/nmigen/vendor/xilinx_spartan_3_6.py +++ b/nmigen/vendor/xilinx_spartan_3_6.py @@ -412,6 +412,7 @@ def get_diff_input_output(self, pin, p_port, n_port, attrs, invert): return m def get_ff_sync(self, ff_sync): + assert reset_sync._max_input_delay is None m = Module() flops = [Signal(ff_sync.i.shape(), name="stage{}".format(index), reset=ff_sync._reset, reset_less=ff_sync._reset_less, @@ -423,6 +424,7 @@ def get_ff_sync(self, ff_sync): return m def get_reset_sync(self, reset_sync): + assert reset_sync._max_input_delay is None m = Module() m.domains += ClockDomain("reset_sync", async_reset=True, local=True) flops = [Signal(1, name="stage{}".format(index), reset=1, From a0a3b302421c6f84a7e0fba75063d502167c4418 Mon Sep 17 00:00:00 2001 From: Darrell Harmon Date: Mon, 23 Sep 2019 17:51:21 -0600 Subject: [PATCH 10/14] forgot self on _max_input_delay --- nmigen/lib/cdc.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nmigen/lib/cdc.py b/nmigen/lib/cdc.py index 744c8a96..6abc0536 100644 --- a/nmigen/lib/cdc.py +++ b/nmigen/lib/cdc.py @@ -78,7 +78,7 @@ def elaborate(self, platform): if hasattr(platform, "get_ff_sync"): return platform.get_ff_sync(self) - assert _max_input_delay is None + assert self._max_input_delay is None m = Module() flops = [Signal(self.i.shape(), name="stage{}".format(index), @@ -134,7 +134,7 @@ def elaborate(self, platform): if hasattr(platform, "get_reset_sync"): return platform.get_reset_sync(self) - assert _max_input_delay is None + assert self._max_input_delay is None m = Module() m.domains += ClockDomain("reset_sync", async_reset=True, local=True) From 4c266f5ac809f82dc8012a8fd02462ea8af6bc29 Mon Sep 17 00:00:00 2001 From: Darrell Harmon Date: Mon, 23 Sep 2019 18:15:35 -0600 Subject: [PATCH 11/14] merge whitequark's WIP patch error messages and comments did not merge xilinx_7series.py --- nmigen/lib/cdc.py | 22 ++++++++++++++-------- nmigen/vendor/lattice_ecp5.py | 3 +++ nmigen/vendor/lattice_ice40.py | 3 +++ nmigen/vendor/xilinx_spartan_3_6.py | 10 ++++++++-- 4 files changed, 28 insertions(+), 10 deletions(-) diff --git a/nmigen/lib/cdc.py b/nmigen/lib/cdc.py index 6abc0536..3c62b8f5 100644 --- a/nmigen/lib/cdc.py +++ b/nmigen/lib/cdc.py @@ -29,15 +29,15 @@ class FFSynchronizer(Elaboratable): Signal connected to synchroniser output. o_domain : str Name of output clock domain. - stages : int - Number of synchronization stages between input and output. The lowest safe number is 2, - with higher numbers reducing MTBF further, at the cost of increased latency. reset : int Reset value of the flip-flops. On FPGAs, even if ``reset_less`` is True, the :class:`FFSynchronizer` is still set to this value during initialization. reset_less : bool - If True (the default), this :class:`FFSynchronizer` is unaffected by ``o_domain`` reset. - See "Note on Reset" below. + If ``True`` (the default), this :class:`FFSynchronizer` is unaffected by ``o_domain`` + reset. See "Note on Reset" below. + stages : int + Number of synchronization stages between input and output. The lowest safe number is 2, + with higher numbers reducing MTBF further, at the cost of increased latency. Platform override ----------------- @@ -72,13 +72,16 @@ def __init__(self, i, o, *, o_domain="sync", reset=0, reset_less=True, stages=2, self._reset_less = reset_less self._o_domain = o_domain self._stages = stages + self._max_input_delay = max_input_delay def elaborate(self, platform): if hasattr(platform, "get_ff_sync"): return platform.get_ff_sync(self) - assert self._max_input_delay is None + if self._max_input_delay is not None: + raise NotImplementedError("Platform {!r} does not support constraining input delay " + "for FFSynchronizer".format(platform)) m = Module() flops = [Signal(self.i.shape(), name="stage{}".format(index), @@ -128,13 +131,16 @@ def __init__(self, arst, *, domain="sync", stages=2, max_input_delay=None): self._domain = domain self._stages = stages - self._max_input_delay = max_input_delay + + self._max_input_delay = None def elaborate(self, platform): if hasattr(platform, "get_reset_sync"): return platform.get_reset_sync(self) - assert self._max_input_delay is None + if self._max_input_delay is not None: + raise NotImplementedError("Platform {!r} does not support constraining input delay " + "for ResetSynchronizer".format(platform)) m = Module() m.domains += ClockDomain("reset_sync", async_reset=True, local=True) diff --git a/nmigen/vendor/lattice_ecp5.py b/nmigen/vendor/lattice_ecp5.py index f18e87c2..89143ab0 100644 --- a/nmigen/vendor/lattice_ecp5.py +++ b/nmigen/vendor/lattice_ecp5.py @@ -533,3 +533,6 @@ def get_diff_input_output(self, pin, p_port, n_port, attrs, invert): io_B=p_port[bit], ) return m + + # CDC primitives are not currently specialized for ECP5. While Diamond supports the necessary + # attributes (TBD); nextpnr-ecp5 does not. diff --git a/nmigen/vendor/lattice_ice40.py b/nmigen/vendor/lattice_ice40.py index bc1df8f3..ffbbd0c9 100644 --- a/nmigen/vendor/lattice_ice40.py +++ b/nmigen/vendor/lattice_ice40.py @@ -564,3 +564,6 @@ def get_diff_output(self, pin, p_port, n_port, attrs, invert): # Tristate and bidirectional buffers are not supported on iCE40 because it requires external # termination, which is incompatible for input and output differential I/Os. + + # CDC primitives are not currently specialized for iCE40. It is not known if iCECube2 supports + # the necessary attributes; nextpnr-ice40 does not. diff --git a/nmigen/vendor/xilinx_spartan_3_6.py b/nmigen/vendor/xilinx_spartan_3_6.py index 83b26721..eb0aac01 100644 --- a/nmigen/vendor/xilinx_spartan_3_6.py +++ b/nmigen/vendor/xilinx_spartan_3_6.py @@ -412,7 +412,10 @@ def get_diff_input_output(self, pin, p_port, n_port, attrs, invert): return m def get_ff_sync(self, ff_sync): - assert reset_sync._max_input_delay is None + if ff_sync._max_input_delay is not None: + raise NotImplementedError("Platform {!r} does not support constraining input delay " + "for FFSynchronizer".format(self)) + m = Module() flops = [Signal(ff_sync.i.shape(), name="stage{}".format(index), reset=ff_sync._reset, reset_less=ff_sync._reset_less, @@ -424,7 +427,10 @@ def get_ff_sync(self, ff_sync): return m def get_reset_sync(self, reset_sync): - assert reset_sync._max_input_delay is None + if reset_sync._max_input_delay is not None: + raise NotImplementedError("Platform {!r} does not support constraining input delay " + "for ResetSynchronizer".format(self)) + m = Module() m.domains += ClockDomain("reset_sync", async_reset=True, local=True) flops = [Signal(1, name="stage{}".format(index), reset=1, From f5ee5a3beceadc7038f3ef63b6c7c53b0097a7e6 Mon Sep 17 00:00:00 2001 From: Darrell Harmon Date: Mon, 23 Sep 2019 18:20:47 -0600 Subject: [PATCH 12/14] add comments for max_input_delay --- nmigen/lib/cdc.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/nmigen/lib/cdc.py b/nmigen/lib/cdc.py index 3c62b8f5..772d601b 100644 --- a/nmigen/lib/cdc.py +++ b/nmigen/lib/cdc.py @@ -38,6 +38,9 @@ class FFSynchronizer(Elaboratable): stages : int Number of synchronization stages between input and output. The lowest safe number is 2, with higher numbers reducing MTBF further, at the cost of increased latency. + max_input_delay : None or float + Maximum delay from the input signal's clock to the first synchronizer flip flop. Not + supported on all platforms and must be set to None on unsupported platforms. Platform override ----------------- @@ -118,6 +121,9 @@ class ResetSynchronizer(Elaboratable): stages : int, >=2 Number of synchronization stages between input and output. The lowest safe number is 2, with higher numbers reducing MTBF further, at the cost of increased deassertion latency. + max_input_delay : None or float + Maximum delay from the input signal's clock to the first synchronizer flip flop. Not + supported on all platforms and must be set to None on unsupported platforms. Platform override ----------------- From 8e80a84e5d6dc4f2bc7c19c25f1207bc3b1b245a Mon Sep 17 00:00:00 2001 From: whitequark Date: Tue, 24 Sep 2019 00:34:37 +0000 Subject: [PATCH 13/14] minor improvement in wording --- nmigen/lib/cdc.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/nmigen/lib/cdc.py b/nmigen/lib/cdc.py index 772d601b..452814de 100644 --- a/nmigen/lib/cdc.py +++ b/nmigen/lib/cdc.py @@ -39,8 +39,8 @@ class FFSynchronizer(Elaboratable): Number of synchronization stages between input and output. The lowest safe number is 2, with higher numbers reducing MTBF further, at the cost of increased latency. max_input_delay : None or float - Maximum delay from the input signal's clock to the first synchronizer flip flop. Not - supported on all platforms and must be set to None on unsupported platforms. + Maximum delay from the input signal's clock to the first synchronization stage. + If specified and the platform does not support it, elaboration will fail. Platform override ----------------- @@ -122,8 +122,8 @@ class ResetSynchronizer(Elaboratable): Number of synchronization stages between input and output. The lowest safe number is 2, with higher numbers reducing MTBF further, at the cost of increased deassertion latency. max_input_delay : None or float - Maximum delay from the input signal's clock to the first synchronizer flip flop. Not - supported on all platforms and must be set to None on unsupported platforms. + Maximum delay from the input signal's clock to the first synchronization stage. + If specified and the platform does not support it, elaboration will fail. Platform override ----------------- From 3775cbaa62ccae3a19ae62a50e137aac46d24ceb Mon Sep 17 00:00:00 2001 From: whitequark Date: Tue, 24 Sep 2019 00:43:07 +0000 Subject: [PATCH 14/14] add explanation of how the synchronizer overrides work --- nmigen/vendor/xilinx_7series.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/nmigen/vendor/xilinx_7series.py b/nmigen/vendor/xilinx_7series.py index 7ec30417..d423e4cf 100644 --- a/nmigen/vendor/xilinx_7series.py +++ b/nmigen/vendor/xilinx_7series.py @@ -372,6 +372,17 @@ def get_diff_input_output(self, pin, p_port, n_port, attrs, invert): ) return m + # The synchronizer implementations below apply two separate but related timing constraints. + # + # First, the ASYNC_REG attribute prevents inference of shift registers from synchronizer FFs, + # and constraints the FFs to be placed as close as possible, ideally in one CLB. This attribute + # only affects the synchronizer FFs themselves. + # + # Second, the nmigen.vivado.false_path or nmigen.vivado.max_delay attribute affects the path + # into the synchronizer. If maximum input delay is specified, a datapath-only maximum delay + # constraint is applied, limiting routing delay (and therefore skew) at the synchronizer input. + # Otherwise, a false path constraint is used to omit the input path from the timing analysis. + def get_ff_sync(self, ff_sync): m = Module() flops = [Signal(ff_sync.i.shape(), name="stage{}".format(index),