Vivado CDC constraints #227
Comments
|
Codecov Report
@@ Coverage Diff @@
## master #227 +/- ##
==========================================
- Coverage 82.38% 82.32% -0.06%
==========================================
Files 33 33
Lines 5483 5489 +6
Branches 1172 1174 +2
==========================================
+ Hits 4517 4519 +2
- Misses 834 836 +2
- Partials 132 134 +2
Continue to review full report at Codecov.
|
|
References: I expect a few more constraint requirements once #40 is merged due to a timing path between the write clock and async outputs on the LUT RAMs. It's a challenge to deal with: https://forums.xilinx.com/t5/Vivado-TCL-Community/set-disable-timing-for-hierarchical-cell/td-p/495808 Alternately, set the clocks to be asynchronous, but getting correct clock names is a challenge. |
|
Xilinx UG903 is self contradictory and the |
|
Ideally, we would use I also noticed that there is a "critical warning" generated when the added lines are present in the Direction would be appreciated. |
|
This isn't directly related to the PR at hand (I'll look more into how Vivado works before commenting here), but I have a question you might answer regarding attributes. nMigen emits some internal attributes like |
|
Can you elaborate on the exact nature of complication here? |
|
Actually, I was wrong. Vivado doesn't mind. ISE complains: |
|
We would either need an I did find that instead of the I think I'll need to add a dict keyed with |
|
As you found, Vivado does seem to ignore any attribute it doesn't use. It seems safe enough to assume it won't use anything starting with I do see Vivado complaining about the |
|
We'll have this information once #4 is implemented, but right now this isn't viable.
I don't understand this statement. We already have
I would personally opt for a simpler solution: grab the attribute with Tcl and set it, again, with Tcl. I'll throw together a snippet soon so you don't have to do it yourself. |
|
Sounds good.
This is #220. Unfortunately an upstream issue. |
|
I was referring to The LUTRAM on 7series has a timing path from the write clock to the async read data. I want to make that a false path / max delay while keeping the path from read address to read data. Having the from clock makes that possible. |
|
My knowledge of Tcl is quite limited, so I'll leave that to you. |
|
@dlharmon I think I understand how to do this in Tcl, but could you please explain what |
|
This one is good and what I based the migen async reset synchronizer constraints on. There are more good posts by that guy in the xilinx forums: http://www.colmryan.org/avrums-clock-domain-crossing-widsom.html |
|
The first link mentioned by @jordens does a decent job explaining it. Simply adding a false path to the flip flop is the easiest, but allows the place and route unlimited delay in that net which is problematic in some cases. For instance, I have a bunch of 250 MHz same frequency, unknown phase shift crossings that need to come out of reset within a few cycles of each other, so a 5 ns max delay works well there. |
|
|
|
Do I understand it correctly that |
|
Yes, that's a great way to put it. |
|
@dlharmon Here is my take on adding false path and max delay constraints for Vivado: diff --git a/nmigen/vendor/xilinx_7series.py b/nmigen/vendor/xilinx_7series.py
index b25c4e9..30dfa64 100644
--- a/nmigen/vendor/xilinx_7series.py
+++ b/nmigen/vendor/xilinx_7series.py
@@ -78,6 +78,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
@@ -367,7 +371,26 @@ class Xilinx7SeriesPlatform(TemplatedPlatform):
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)
m.d.comb += ff_sync.o.eq(flops[-1])
return m
+
+ def get_reset_sync(self, reset_sync):
+ m = Module()
+ m.domains += ClockDomain("reset_sync", async_reset=True, local=True)
+ 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)
+ m.d.comb += [
+ ClockSignal("reset_sync").eq(ClockSignal(reset_sync._domain)),
+ ResetSignal("reset_sync").eq(reset_sync.arst),
+ ResetSignal(reset_sync._domain).eq(flops[-1])
+ ]
+ return mIt looks like you can't use arbitrary Tcl commands in XDC files (AR#54842), so I moved it to the main script. Please let me know if this works properly. |
|
So in the diff above I hardcoded |
|
It limits the skew and allows for metastability resolution. Between two flip flops in a synchronizer chain in the target clock domain, there is |
|
OK. Do I understand correctly that there are two independent purposes served by |
|
I have personally just used Edit: The first flip flop certainly does need I'll try your patch shortly. It looks promising. |
|
I merged this with my local copy and built a bitstream. The timing report and constraints look good. I like how this works. Thanks. |
|
OK, so now the question is how to make the delay configurable. Here's my proposal:
Opinion? |
|
That seems fine. |
|
Allow the option of a full false path, not just hold only: diff --git a/nmigen/vendor/xilinx_7series.py b/nmigen/vendor/xilinx_7series.py
index 953229a..62ab94a 100644
--- a/nmigen/vendor/xilinx_7series.py
+++ b/nmigen/vendor/xilinx_7series.py
@@ -79,7 +79,8 @@ 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}]
+ set_false_path -hold -to [get_cells -hier -filter {nmigen.vivado.false_path == HOLD_ONLY}]
+ set_false_path -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
}
@@ -372,7 +373,7 @@ class Xilinx7SeriesPlatform(TemplatedPlatform):
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.false_path"] = "HOLD_ONLY"
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 +386,7 @@ class Xilinx7SeriesPlatform(TemplatedPlatform):
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.false_path"] = "HOLD_ONLY"
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)```
|
|
The other way around? |
|
My intent was to make it clear that it's a hold only false path in the |
Sunday Sep 22, 2019 at 20:56 GMT
Originally opened as m-labs/nmigen#227
Related: #212
This tags the first register in each
MultiRegorResetSynchronizerwith the attribute
nmigen_async_ffand then applies a false path andmax delay constraint to all registers tagged with that attribute in
the
.xdcfile.The max delay defaults to 5 ns and has an override,
max_delaywhereit can be changed for the > whole project. It's possible to make this
an argument to
MultiReginstead, but is more complex. > git commit-m "add clock domain crossing constraints on Vivado This tags the
first register in each
MultiRegorResetSynchronizerwith theattribute
nmigen_async_ffand then applies a false path and maxdelay constraint to all registers tagged with that attribute in the
.xdcfile.The max delay defaults to 5 ns and has an override,
max_delaywhereit can be changed for the whole project. It's possible to make this an
optional argument to
MultiReginstead, but is more complex. It wouldprobably work to set
nmigen_async_ffto the desired delay ratherthan just
TRUE. I'm not sure how hard it would be to extract that inthe TCL or if it would be easier to keep a dict of all used delay
values and put a line for each into the
.xdcfile.dlharmon included the following code: https://github.com/m-labs/nmigen/pull/227/commits
The text was updated successfully, but these errors were encountered: