Vivado CDC constraints

[nmigen-issue-migration]

Issue by dlharmon
Sunday Sep 22, 2019 at 20:56 GMT
Originally opened as m-labs/nmigen#227

---

Related: #212

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.

---

dlharmon included the following code: https://github.com/m-labs/nmigen/pull/227/commits

[nmigen-issue-migration]

Comment by codecov[bot]
Sunday Sep 22, 2019 at 20:58 GMT

---

Codecov Report

Merging #227 into master will decrease coverage by 0.05%.
The diff coverage is 50%.

@@            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

Impacted Files	Coverage Δ
nmigen/lib/cdc.py	84% <50%> (-6.91%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da53048...b78ef18. Read the comment docs.

[nmigen-issue-migration]

Comment by dlharmon
Sunday Sep 22, 2019 at 21:04 GMT

---

References:
https://forums.xilinx.com/t5/Timing-Analysis/how-to-constrain-a-CDC/td-p/748174

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.

[nmigen-issue-migration]

Comment by dlharmon
Sunday Sep 22, 2019 at 22:15 GMT

---

Xilinx UG903 is self contradictory and the set_false_path constraint appears to override the set_max_delay constraint. I'm looking into how to solve this.

[nmigen-issue-migration]

Comment by dlharmon
Monday Sep 23, 2019 at 02:14 GMT

---

Ideally, we would use set_max_delay -datapath_only, but that option requires providing -from with the source clock greatly complicating things.

I also noticed that there is a "critical warning" generated when the added lines are present in the .xdc file and there are no flip flops with the nmigen_async_ff attribute.

Direction would be appreciated.

[nmigen-issue-migration]

Comment by whitequark
Monday Sep 23, 2019 at 06:55 GMT

---

This tags the first register in each MultiReg or ResetSynchronizer
with the attribute nmigen_async_ff

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 nmigen.hierarchy, nmigen.decoding, src (which should really be nmigen.src), etc. Vivado complains about these. Is here a way to tell it "just ignore any attribute that starts with nmigen", other than the nmigen_async_ff and other semantically important ones?

[nmigen-issue-migration]

Comment by whitequark
Monday Sep 23, 2019 at 07:07 GMT

---

Ideally, we would use set_max_delay -datapath_only, but that option requires providing -from with the source clock greatly complicating things.

Can you elaborate on the exact nature of complication here?

[nmigen-issue-migration]

Comment by whitequark
Monday Sep 23, 2019 at 07:48 GMT

---

Vivado complains about these

Actually, I was wrong. Vivado doesn't mind. ISE complains:

WARNING:Xst:37 - Detected unknown constraint/property "src". This constraint/property is not supported by the current software release and will be ignored.
WARNING:Xst:37 - Detected unknown constraint/property "src". This constraint/property is not supported by the current software release and will be ignored.
WARNING:Xst:37 - Detected unknown constraint/property "nmigen.hierarchy". This constraint/property is not supported by the current software release and will be ignored.

[nmigen-issue-migration]

Comment by dlharmon
Monday Sep 23, 2019 at 15:18 GMT

---

Ideally, we would use set_max_delay -datapath_only, but that option requires providing -from with the source clock greatly complicating things.

Can you elaborate on the exact nature of complication here?

We would either need an i_domain argument to MultiReg and to know the final Verilog name of the clock or some way to find the clock source of the i argument (may not exist if it came from a pin which is OK or could be combinatorial logic from multiple domains). I assumed the latter to be difficult and possibly would add much more complexity than required. If that's not the case, it may provide a bit more flexibility, especially for things like a LUTRAM based FIFO. It might be worth an optional i_domain argument for the LUTRAM FIFO case.

I did find that instead of the set_min_delay -1000 -to <REG>, a better way to do it is to add set_false_path -hold -to <REG>. I'll push that change to this branch after testing it.

I think I'll need to add a dict keyed with (source_clk, max_delay) to platform for keeping track of which constraints need to be added to the .xdc file. Of course, if we don't do per CDC max_delay or set_max_delay -datapath_only -from <X> -to <Y> it could simply be a bool to enable adding to the constraints file.

[nmigen-issue-migration]

Comment by dlharmon
Monday Sep 23, 2019 at 15:25 GMT

---

nMigen emits some internal attributes like nmigen.hierarchy, nmigen.decoding, src (which should really be nmigen.src), etc. Vivado complains about these. Is here a way to tell it "just ignore any attribute that starts with nmigen", other than the nmigen_async_ff and other semantically important ones?

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 nmigen. Perhaps, I should swap my nmigen_async_ff to nmigen.async_ff for consistency.

I do see Vivado complaining about the init attribute on Verilog wires.

[nmigen-issue-migration]

Comment by whitequark
Monday Sep 23, 2019 at 15:30 GMT

---

We would either need an i_domain argument to MultiReg and to know the final Verilog name of the clock or some way to find the clock source of the i argument (may not exist if it came from a pin which is OK or could be combinatorial logic from multiple domains). I assumed the latter to be difficult and possibly would add much more complexity than required.

We'll have this information once #4 is implemented, but right now this isn't viable.

If that's not the case, it may provide a bit more flexibility, especially for things like a LUTRAM based FIFO. It might be worth an optional i_domain argument for the LUTRAM FIFO case.

I don't understand this statement. We already have r_domain and w_domain in AsyncFIFO. Or do you mean some other kind of FIFO?

I think I'll need to add a dict keyed with (source_clk, max_delay) to platform for keeping track of which constraints need to be added to the .xdc file. Of course, if we don't do per CDC max_delay or set_max_delay -datapath_only -from <X> -to <Y> it could simply be a bool to enable adding to the constraints file.

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.

[nmigen-issue-migration]

Comment by whitequark
Monday Sep 23, 2019 at 15:30 GMT

---

Perhaps, I should swap my nmigen_async_ff to nmigen.async_ff for consistency.

Sounds good.

I do see Vivado complaining about the init attribute on Verilog wires.

This is #220. Unfortunately an upstream issue.

[nmigen-issue-migration]

Comment by dlharmon
Monday Sep 23, 2019 at 15:44 GMT

---

If that's not the case, it may provide a bit more flexibility, especially for things like a LUTRAM based FIFO. It might be worth an optional i_domain argument for the LUTRAM FIFO case.

I don't understand this statement. We already have r_domain and w_domain in AsyncFIFO. Or do you mean some other kind of FIFO?

I was referring to MultiReg/FFSynchronizer which is used within AsyncFIFO and doesn't currently get passed the input clock. It's simple enough to add an argument i_domain = None to FFSynchronizer if it's not None add the -from and -datapath_only arguments to set_max_delay, omit set_false_path.

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.

[nmigen-issue-migration]

Comment by dlharmon
Monday Sep 23, 2019 at 15:46 GMT

---

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.

My knowledge of Tcl is quite limited, so I'll leave that to you.

[nmigen-issue-migration]

Comment by whitequark
Monday Sep 23, 2019 at 16:50 GMT

---

@dlharmon I think I understand how to do this in Tcl, but could you please explain what set_max_delay actually does here? I've been looking at various documentation for quite some time and I still do not understand why it is useful.

[nmigen-issue-migration]

Comment by jordens
Monday Sep 23, 2019 at 17:05 GMT

---

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