CDC rework and constraint generation

[andreasWallner]

I have been looking at our CDC stuff and would like to make a few additions/changes:

BufferCC

BufferCC is not a safe primitive to use in general. It's totally fine as a basic building block and to synchronize asynchronous data, but for use in a design it:

• it does not enforce a register in the launching CD, so one can sample an intermediate state from combinatorial propagation
• we currently don't point out the possible pitfalls, i.e. that data does not stay coherent and that BufferCC can't be used for a bus in the general case.

I'd like to:

• Add a SingleBitCDC component (similar to our other ones) for 1 bit CDC.
• Add a SingleBitArrayCDC (better naming ideas are welcome) for the specific case of multiple bits that need not be coherent.
• Keep BufferCC, document its use for synchronizing async signals and as a basic building block for other CDC.

Automatic constraint generation

It's a bit of a chore currently to generate constraints for a bigger design. This is especially true when reusing library components like CDC for AXI: we have to understand the CDC to create correct constraints. E.g. StreamCCByToggle needs to have false-paths on the valid/ready signals, and at least a max-delay constraint on the datapath.

From a discussion with @likewise I mocked this up here: https://github.com/SpinalHDL/SpinalHDL/compare/dev...andreasWallner:SpinalHDL:timing_constraints (the generation part is not yet what it needs to be, but I wanted to start a discussion on this) We should be able to constraint everything with:

• false path (mostly for BufferCC uses)
• max delay (e.g. for multibit as in StreamCCByToggle)
• bus skew (missing in the mockup, needed e.g. for gray counters, configuration registers that are used unsynchronized)

constraints. I considered multi-cycle as well, but for our current set of CDC components there is no need.

Any feedback on these is very welcome.

[Readon]

• Add a SingleBitArrayCDC (better naming ideas are welcome) for the specific case of multiple bits that need not be coherent.

If this refer to a multicycle handshake mechanism has been introduced, I suppose that there might be at least two signals for handshaking, which could be more convenient if a well written facility is provided.

[andreasWallner]

If this refer to a multicycle handshake mechanism has been introduced

Not in that sense: I mean that as a component that just provides n uncorreclated synchronizers and input registers w/o any handshaking - making it explicit what you are getting (compare e.g. Xilinx's https://docs.xilinx.com/r/en-US/pg382-xpm-cdc-generator/XPM_CDC_ARRAY_SINGLE). Or said in another way: BufferCC for Bits but with the input register. Not much there in the sense of engineering, "just" to make explicit what one is getting.

Handshaking is already there with both FlowCCByToggle and StreamCCByToggle

[likewise]

I think BufferCC also needs good timing constraints. Not a false path, because for multi-bit we need to defy bus skew.

Here is an expert background: https://support.xilinx.com/s/article/794116?language=en_US that Andreas shared with us earlier.

This is a state-of-the-art TCL script that generates the correct timing constraints: https://github.com/corundum/corundum/blob/master/fpga/lib/eth/lib/axis/syn/vivado/axis_async_fifo.tcl for this AXI Streaming Cross Clocking FIFO: https://github.com/corundum/corundum/blob/master/fpga/lib/eth/lib/axis/rtl/axis_async_fifo.v

I am currently using a SpinalHDL BlackBox wrapper for the above, but would like to use StreamFifoCC with the correct timing constraints instead.

[andreasWallner]

@likewise I'd disagree on the BufferCC side:

• it's used in places were users just wanted a delay by n cycles (yes, that code should have used Delay)
• I'm pretty sure that we couldn't provide correct timing constraints for all the different use-cases you could use BufferCC in
• it was never a complete synchronization primitive to be begin with

That's why I'd like to introduce the two above mentioned classes, which should then carry attributes for the proper timing constraints - and keep BufferCC as a building block.

Does that sound reasonable?

[Dolu1990]

it was never a complete synchronization primitive to be begin with

Right.

That's why I'd like to introduce the two above mentioned classes, which should then carry attributes for the proper timing constraints - and keep BufferCC as a building block. Does that sound reasonable?

Yes ^^

[KireinaHoro]

@Dolu1990 @andreasWallner any plans on following up on this? I'm not very good at writing these constraints so it would really help if the BufferCC and friends could generate them...

[andreasWallner]

I see you already found the last state when I looked at this: https://github.com/SpinalHDL/SpinalHDL/compare/dev...andreasWallner:SpinalHDL:timing_constraints I was kind of stumped by an issue with Vivado not recognizing the clock itself, I just realized that I forgot to add the flag that get_clocks would also find generated clocks (which is necessary if the SpinalHDL output is an IP component where we don't know the clock at elaboration time) I won't find much time in the near future, feel free if you want to work on this.

I'd think that we should also add some skew constraint tag, otherwise all of the information that is needed for the constraint file should be there in the draft. This and their applications in e.g. BufferCC should be ok to discuss in a PR. The writer still needs some work I think.

[andreasWallner]

FYI: had some not-yet-pushed stuff on my disk, just pushed that (only a small improvement)

[KireinaHoro]

So I have some experiments that I managed to get working (in Vivado); it’s based on your branch but nowhere near upstreaming.  I’ll open a draft PR and tag you.Am 04.02.2024 um 22:24 schrieb Andreas Wallner @.***>: FYI: had some not-yet-pushed stuff on my disk, just pushed that (only a small improvement)

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: @.***>

[KireinaHoro]

@andreasWallner I see that in your code, most of the crossClockMaxDelay tags carry a cycles = 2 argument; does this mean that you intend this to be a set_max_delay of two clock cycles? I'm not very sure why this would be the case, since usually I would use a delay of the smaller period of the two clock domains.

[andreasWallner]

IIRC the data is captured in the receiving CD with the third cycle, so 2 cycles would be OK - but would have done a final check.