take over cdc rework branch from andreasWallner #1306

KireinaHoro · 2024-02-08T10:17:51Z

Further work on #1109. This is still currently a draft, since I'm not entirely sure if the resulting constraints work in hardware yet; but seems like Vivado is picking them up (they get listed in report_timing_exceptions and STA does not fail on false paths).

Some notable changes from @andreasWallner's branch:

added crossClockFalsePathSource tag to handle cases where BufferCC is used for generating synchronous reset, e.g.:

SpinalHDL/lib/src/main/scala/spinal/lib/CrossClock.scala

Lines 124 to 137 in 83efd66

    
           val samplerCD = clockDomain.copy( 
        
             reset = input, 
        
             config = clockDomain.config.copy( 
        
               resetKind = ASYNC, 
        
               resetActiveLevel = inputPolarity 
        
             ) 
        
           ) 
        
           val solvedOutputPolarity = if(outputPolarity == null) clockDomain.config.resetActiveLevel else outputPolarity 
        
           samplerCD(BufferCC( 
        
             input       = (if(solvedOutputPolarity == HIGH) False else True) ^ inputSync, 
        
             init        = (if(solvedOutputPolarity == HIGH) True  else False), 
        
             bufferDepth = bufferDepth) 
        
           )

write OOC clock definitions for IP packager
refined get_* queries to reliably find signal in netlist
make ConstraintWriter private and added writeConstraints to SpinalReport, such that we have all signal names

More work needed:

better clock domain determination for set_max_delay: I had to add ClockDomainTag manually in some places to find the correct source and destination clocks
- Maybe a solution for ClockDomain for input/output ports? #1299 would make it easier?
check if the constraints result in a working design
cover all existing CC gadgets (*ByToggle, StreamFifoCC, and more)
implement the goal from @andreasWallner to make CC requirements more explicit, maybe like the style of XPM_CDC_*
support for other EDA tool vendors (other than Xilinx; I have no experience on this one)
documentation on what to use for what situation

@andreasWallner @Dolu1990 please take a look ^^

Closes #1109

andreasWallner

Really cool that you are picking this up^^

I didn't go too much into detail on the code, but I think most of it looks good from a scala POV.

W.r.t. your TODO list: I think the only component really missing is a "safe" version of BufferCC (that buffers the input by default, maybe with a parameter to switch it off, preventing states from comb propagation to be latched to the other CD) - leaving BufferCC as a building block for CDC stuff and sync for external inputs.

W.r.t. #1299: there is a solution to that posted in that Issue?

PS: I don't think we need to go through e.g. implementing this for all tool vendors for this initial PR - only to keep in mind that they also exist ;-)

andreasWallner · 2024-02-13T22:31:58Z

core/src/main/scala/spinal/core/Area.scala

@@ -126,6 +126,17 @@ object ImplicitArea{
 class ClockingArea(val clockDomain: ClockDomain) extends Area with PostInitCallback {
  val ctx = ClockDomainStack.set(clockDomain)

+  /*


I think that's not an approach that will work: a path can be placed in an area connecting FFs that are both outside of that area, where a tag applied by this code would be very misleading. (and I'd say a path doesn't have a CD, only FFs do)

Agreed. I'm thinking that maybe it is possible to automatically derive the actual clockdomain (instead of relying on Component.clockDomain, which is in a lot of cases not the correct domain) from connection relationships? Given that we can specify clock domain for input and output ports, we should be able to propagate this information along the path...

andreasWallner · 2024-02-13T22:42:08Z

lib/src/main/scala/spinal/lib/CrossClock.scala


  io.dataOut := buffers.last
+
+  addAttribute("keep_hierarchy", "TRUE")


in the long run I think we might need something here that generates depending on the toolchain (dont_touch is for vivado, I think yosys uses keep, synplify syn_keep (?), altera preserve (?))

I think we only need a keep_hierarchy here, since what I'm trying to do is prevent flattening to get a predictable port name. I'm not sure if the other tools have the same semantics.

andreasWallner · 2024-02-13T23:27:11Z

lib/src/main/scala/spinal/lib/CrossClock.scala

@@ -97,7 +102,7 @@ class PulseCCByToggle(clockIn: ClockDomain, clockOut: ClockDomain, withOutputBuf

  val finalOutputClock = clockOut.withOptionalBufferedResetFrom(withOutputBufferedReset)(clockIn)
  val outArea = finalOutputClock on new Area {
-    val target = BufferCC(inArea.target, False)
+    val target = BufferCC(inArea.target, False, inputAttributes = List(crossClockFalsePath))


I think I'd personally also use a max_delay constraint here and not remove all constraints on the path - but I know that's an often discussed topic...

lib/src/main/scala/spinal/lib/Flow.scala

andreasWallner · 2024-02-13T23:49:56Z

lib/src/main/scala/spinal/lib/Flow.scala

    val hit = RegNext(target).addTag(noInit)

    val flow = cloneOf(io.input)
    flow.valid := (target =/= hit)
-    flow.payload := inputArea.data
+    // FIXME: general solution such that clock domain propagates during assign?
+    flow.payload.addTag(ClockDomainTag(inputClock)) := inputArea.data


in which case did it not work to search for the original driving circuit, we should be able to find inputArea.data?

We can find the driving circuit no problem, but we cannot determine the clock domain of the driver. As far as I know it's impossible to find the actual clock domain of a FF with the current API (using .clockDomain does not work since that is the default clock domain of the containing component).

I'll have a look, should be able to find some more time tomorrow.

I've had a look at https://github.com/SpinalHDL/SpinalHDL/blob/dev/core/src/main/scala/spinal/core/BaseType.scala#L62 and that should work: it gets initialized with the current CD (not default CD) at creation time, but all ways to place a register in a specific CD will modify that (via the CD stack).

Btw. If you want to print the AST I have somewhat unfinished but often helpful tool here: https://github.com/andreasWallner/spinalStuff/blob/master/src/main/scala/andreasWallner/Utils.scala#L100

I'm holding on fixing this--you mentioned that FlowCCByToggle is not a proper CDCC since it does not involve handshaking.

ah, ok - if this was the only case were you encountered that problem that's one less thing to worry about.

lib/src/main/scala/spinal/lib/eda/ConstraintWriter.scala

andreasWallner · 2024-02-14T01:01:06Z

lib/src/main/scala/spinal/lib/CrossClock.scala

-      bufferDepth = bufferDepth)
+      init        = if(solvedOutputPolarity == HIGH) True  else False,
+      bufferDepth = bufferDepth,
+      allBufAttributes = falsePathAttrs)


we normally have max_delay constraints as well on these, but I could not find any official docs on that (and I guess the async_reg attribute should take care of that anyways, at least for the xilinx case?)

only think that I could find was: https://support.xilinx.com/s/question/0D52E00006hpdDoSAI/ucf-constraints-for-reset-synchronizer?language=en_US - hardly authoritative

lib/src/main/scala/spinal/lib/CrossClock.scala

KireinaHoro · 2024-02-19T08:30:44Z

W.r.t. your TODO list: I think the only component really missing is a "safe" version of BufferCC (that buffers the input by default, maybe with a parameter to switch it off, preventing states from comb propagation to be latched to the other CD) - leaving BufferCC as a building block for CDC stuff and sync for external inputs.

Agreed--should we also deprecate any direct invocations of BufferCC? Since that's rarely what the user actually wants...

W.r.t. #1299: there is a solution to that posted in that Issue?

IMO @Dolu1990 mentioned a possible native API to mark clock domains automatically, so that no manual ClockDomainTags would be needed (if I understood his intentions correctly).

PS: I don't think we need to go through e.g. implementing this for all tool vendors for this initial PR - only to keep in mind that they also exist ;-)

Yeah, I'll do a bit of refactoring to decouple the Xilinx-specific bits; other contributors can fill in the rest ^^

KireinaHoro · 2024-02-20T14:24:04Z

@andreasWallner I made a bunch of small fixes to address most of the issues. Please take another look ^^

andreasWallner · 2024-02-21T01:22:32Z

For two of the points I'll have another look, otherwise this looks quite cool. Have you tried whether the resulting constraint file can be used for IP blocks (so in case the generated spinal top is not the real top)

Regarding your last questions:
I think we should not deprecate BufferCC. It'll still be the component to use when synchronizing an async signal coming from the outside. (And if we did we'd get warnings on the code that rightfully uses BufferCC). But we can mark it accordingly on the documentation.

Ad API: I read it as "we could add some API to make assigning the CD to BB I/Os easier" - but well have to wait for him to clarify (we could add a flag to mapClockDomain maybe?)

One other thing: if we skip the implementation of trustSpinalTimings for now(which is totally fine) we should just revive it for now

KireinaHoro · 2024-02-21T08:36:02Z

Have you tried whether the resulting constraint file can be used for IP blocks (so in case the generated spinal top is not the real top)

Yes! This is why I generated the OOC clock constraints, since I'm using them in the IP packager. The IP-XACT flow automatically scopes the constraints so there's no need to do -hier *compName/... all over the place. This does require the user to package the spinal top as an IP, if they're including it in another module; I'm not sure if we should also support the case without the IP packager. Even better, it would be nice if we can automatically generate the IP-XACT XML file, so that the user does not have to package the IP by hand.

One other thing: if we skip the implementation of trustSpinalTimings for now(which is totally fine) we should just revive it for now

What's trustSpinalTimings?

andreasWallner · 2024-03-04T01:57:58Z

he IP-XACT flow automatically scopes the constraints

great - didn't want to put any -hier stuff in anyways, have used loading a constraint file with a hierarchy a few times manually, but I didn't know that Vivado does it automatically when packaging the IP (though it makes sense, but I haven't done that in a long while...)

What's trustSpinalTimings?

The parameter was initially intended for getting clock domain information (mainly freq) from the CD directly in spinal (a CD can know it's own frequency), instead of tracing in the design in the TCL code.
Had a typo there, revive should have been remove...

Sorry for the delay with the other stuff, had a bit of chaos around me lately - getting back to it.

KireinaHoro · 2024-03-07T08:49:22Z

Great and don't worry! I think this is getting on the right track; it should be ready for review once you get to take a look at the existing CDC stuff e.g. those *CCByToggle. Maybe also the renaming/wrapping BufferCC into the proper CDCC categories? Such as the following (modeled after the Xilinx XPM list)

ArraySingle
SyncReset
AsyncReset
Pulse
Handshake
Gray
Fifo

And then we can deprecate and gut out the existing StreamCCByToggle, StreamCCFifo, etc.

andreasWallner · 2024-03-16T14:24:34Z

Some additional info: FlowCCByToggle is not intended as a general synchronization primitive and is expected to work only if the source is (either in clock or datarate) is slower then the destination. We'll reflect that in the name.
Constraint-wise it should then work with max-delay/skew constraints + sync flags on the valid

W.r.t. renaming: As it stands I don't see a good enough reason to rename all synchronization primitives.

Thanks for investing the time here^^

KireinaHoro · 2024-03-20T09:20:16Z

Wait, I think github closed this automatically since I force-pushed dev in my branch to be in sync with upstream. I'll do a new one in a PR branch and clean up a bit

andreasWallner reviewed Feb 14, 2024

View reviewed changes

KireinaHoro closed this Mar 11, 2024

KireinaHoro force-pushed the dev branch from 82b3fae to 9595865 Compare March 11, 2024 11:05

KireinaHoro mentioned this pull request Mar 20, 2024

Constraint writer for Xilinx Vivado (reopened: CDC rework) #1369

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

take over cdc rework branch from andreasWallner #1306

take over cdc rework branch from andreasWallner #1306

KireinaHoro commented Feb 8, 2024 •

edited

Loading

andreasWallner left a comment

andreasWallner Feb 13, 2024

KireinaHoro Feb 20, 2024

andreasWallner Feb 13, 2024

KireinaHoro Feb 20, 2024

andreasWallner Feb 13, 2024

andreasWallner Feb 13, 2024

KireinaHoro Feb 20, 2024

andreasWallner Feb 21, 2024

andreasWallner Mar 4, 2024

andreasWallner Mar 4, 2024

KireinaHoro Mar 5, 2024

andreasWallner Mar 6, 2024

andreasWallner Feb 14, 2024

KireinaHoro commented Feb 19, 2024

KireinaHoro commented Feb 20, 2024

andreasWallner commented Feb 21, 2024

KireinaHoro commented Feb 21, 2024

andreasWallner commented Mar 4, 2024 •

edited

Loading

KireinaHoro commented Mar 7, 2024 •

edited

Loading

andreasWallner commented Mar 16, 2024

KireinaHoro commented Mar 20, 2024

	val samplerCD = clockDomain.copy(
	reset = input,
	config = clockDomain.config.copy(
	resetKind = ASYNC,
	resetActiveLevel = inputPolarity
	)
	)

	val solvedOutputPolarity = if(outputPolarity == null) clockDomain.config.resetActiveLevel else outputPolarity
	samplerCD(BufferCC(
	input = (if(solvedOutputPolarity == HIGH) False else True) ^ inputSync,
	init = (if(solvedOutputPolarity == HIGH) True else False),
	bufferDepth = bufferDepth)
	)


		io.dataOut := buffers.last

		addAttribute("keep_hierarchy", "TRUE")

take over cdc rework branch from andreasWallner #1306

take over cdc rework branch from andreasWallner #1306

Conversation

KireinaHoro commented Feb 8, 2024 • edited Loading

andreasWallner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KireinaHoro commented Feb 19, 2024

KireinaHoro commented Feb 20, 2024

andreasWallner commented Feb 21, 2024

KireinaHoro commented Feb 21, 2024

andreasWallner commented Mar 4, 2024 • edited Loading

KireinaHoro commented Mar 7, 2024 • edited Loading

andreasWallner commented Mar 16, 2024

KireinaHoro commented Mar 20, 2024

KireinaHoro commented Feb 8, 2024 •

edited

Loading

andreasWallner commented Mar 4, 2024 •

edited

Loading

KireinaHoro commented Mar 7, 2024 •

edited

Loading