Add known prefix or postfix names on synchronizing registers

[rowanG077]

Clash has a few CDC mechanisms but in practice they will require timing constraints. Industry standard is to do that in an SDC or something similar. In general they support wildcards to select clocks and registers. So what I would like to see is to give our CDC registers constant prefix or postfix so it will be manageable to write timing constraints for them.

[DigitalBrains1]

Note that I was thinking we should have functionality to emit constraints on top-level ports like we can with SynthesisAttributes but more generic. In Quartus, you can use those attributes to specify chip_pin and IO_STANDARD and such, but Vivado has no such functionality. So being able to specify constraints to be emitted in an .SDC would be very useful there. If during HDL generation we can actually get the precise cells a certain part of HDL renders to, we could also add a mechanism to automatically emit these constraints in an SDC file during HDL generation. This would be a nice generic solution. I don't know if we can get at the correct cell names though?

[rowanG077]

I didn't try this but I assume you could use the function in Clash.Magic to give registers a pre- or postfix and then refer to that in the SDC.

Something like this in the asyncFIFOSynchronizer:

...
-- Explicitly gave the state register of the mealy a prefix
(rempty, raddr, rptr) =
    namedMealyB
      @"fifo_grey_r_ptr"
      rclk rrst ren
      readPtrCompareT
      (0, 0, True)
      (s_wptr, rinc)

(wfull, waddr, wptr) =
    namedMealyB
      @"fifo_grey_w_ptr"
      wclk wrst wen
      (writePtrCompareT addrSize)
      (0, 0, False)
      (s_rptr, isJust <$> wdataM)

-- Explicitly gave the synchronization registers a prefix
-- Ex: First sync register would be named fifo_grey_sync_r_ptr0* and the second one fifo_grey_sync_r_ptr1*
s_rptr = namedDualFlipFlopSynchronizer @"fifo_grey_sync_r_ptr"  rclk wclk wrst wen 0 rptr
s_wptr = namedDualFlipFlopSynchronizer @"fifo_grey_sync_w_ptr"  wclk rclk rrst ren 0 wptr
...

And then in the SDC:

  ...
  set write_clk [get_clocks -of_objects [get_pins fifo_grey_w_ptr[*]/C]]
  set read_clk [get_clocks -of_objects [get_pins fifo_grey_r_ptr[*]/C]]

  set_max_delay -from [get_cells fifo_grey_w_ptr[*]] -to [get_cells fifo_grey_sync_w_ptr0[*]] -datapath_only [get_property -min PERIOD write_clk]
  set_max_delay -from [get_cells fifo_grey_r_ptr[*]] -to [get_cells fifo_grey_sync_r_ptr0[*]] -datapath_only [get_property -min PERIOD read_clk]
    ... 

This is not complete or correct set of constraints since I just written it down free hand but it should illustrate what I mean. To generate the SDC I would envision something like a HasConstraints annotation which could refer to a template which get's filled out. There are still potential problems with name clashes you'd need to somehow solve but I think those are details. I wouldn't want to tie it to a specific topEntity you really want to have it depend on instantiation of a synchronizer.

But the scope of this issue is to just generate selectable names for the relevant register in the Clash synchronizers.

[rowanG077]

Actually reading more closely into the Annotation it's doesn't seem to be required for the annotated function to be a topEntity.

[DigitalBrains1]

Right, I think I understand what you are going for. I was noting it would be even better to have a more general mechanism; and I believe what we'd really want is that any use of asyncFIFOSynchronizer automatically emits correct constraints in a generated SDC file.

If we had the general mechanism, your naming mechanism would be superfluous. But your naming mechanism might be a lot simpler to implement, and I don't know how much development time we have to spare.

Actually reading more closely into the Annotation it's doesn't seem to be required for the annotated function to be a topEntity.

Which Annotation are you referring to?

[rowanG077]

Definitely it would be best to do the whole thing but I think at least having consistent "matchable" names is low hanging fruit. As it stands the synchronizers are almost unusable since the names may change at the drop of a hat and you have to sift through the HDL to actually find them.

If we had the general mechanism, your naming mechanism would be superfluous. But your naming mechanism might be a lot simpler to implement, and I don't know how much development time we have to spare.

I think you'd still want the names, or at the very least require some way to link a register/wire in code to a constraint. How would you imagine how to do that without explicit names?

Which Annotation are you referring to?

The SynthesisAttributes you linked.

[DigitalBrains1]

I think you'd still want the names, or at the very least require some way to link a register/wire in code to a constraint. How would you imagine how to do that without explicit names?

To link to a register/wire in code to a constraint, I was hoping we would/could generate the HDL in such a way that we could predict what the cell we wish to constrain is going to be named like, so we can issue the correct SDC commands that refer to all instantiations of asyncFIFOSynchronizer. And that that name was guaranteed to only match the correct cells.

Or come up with some match that is known to be globally unique (unfortunately you might have to account for postfixes, which makes it a unique prefix rather than a unique name). I'd like to avoid the situation that there is the possibility of a name collision with something wholly unrelated.

The SynthesisAttributes you linked.

Right, I think it just needs to be its own HDL file. I suppose this comment I made is what caused the confusion:

we should have functionality to emit constraints on top-level ports like we can with SynthesisAttributes but more generic.

I meant: For a feature unrelated to this issue, I would like to emit constraints on top-level ports. We can sorta do that for Quartus using SynthesisAttributes, but this approach doesn't work for Vivado. So I'd like another mechanism for Vivado. This mechanism that needs to be designed might actually also be a good way to achieve the goal of this issue as well.

Sorry for not being clearer.

[DigitalBrains1]

This issue got rather silent so I picked it up and discussed it in real life with @leonschoorl . He pointed out that the proposed naming of the registers with a constant prefix/suffix is insufficient. If you have an asyncFifoSynchronizer from domain A to domain B and an asyncFifoSynchronizer from domain B to domain A (pretty common, data flowing both ways), you have no way to pick the correct read_clk and write_clk if those synchronizers have the same prefix.

Apart from this complication we both think it would be a welcome addition to Clash.

Perhaps you could implement it as such:

• Make the prefix contain both the name of the read domain and the write domain, so it is uniquely coupled to a specific domain crossing.
• Make a primitive inside asyncFifoSynchronizer that outputs an "include file" with the extension .sdc that will contain the correct SDC statements to set the constraints on any registers having that prefix with read domain and write domain.

Then the user will see several SDC files appear in their HDL directory, one for each combination of read domain and write domain where crossings happen. These need to be added to the design, and presto.

Of course, if the user adds something with the same names, they will also be matched. So the string needs to be fairly unique and unlikely to be picked by a user accidentally (on purpose is fine).

Do you want to give a PR attempting such a thing a go?