Constraint writer for Xilinx Vivado (reopened: CDC rework)

[KireinaHoro]

Redoing #1306. I think I accidentally closed this due to some branch dancing errors, now the PR head branch should be safe.

I also dropped from the last state of the PR stuff related to Flow: m2sPipe and FlowCCByToggle (cf. #1360), since these are not inherently safe CDC primitives.

@andreasWallner sorry to bother you again but can you review it (or just a brief look, since most of the stuff has been fixed already)? Thanks a lot!

[andreasWallner]

Looks quite good to me

[Dolu1990]

What's about (keep in mind i'm not skilled into this things):

case class FalsePath(source: Option[BaseType] = None, onData: Boolean = false, onReset: Boolean = false, onEnable: Boolean = false)

[KireinaHoro]

Or should we have a sealed trait for the destination type instead?

[Dolu1990]

Ahhhh yes, that is right, like a enum, yes yes

[KireinaHoro]

fixed

[andreasWallner]

if we build this with an enum and keep the current prohibition for multiple tags then we can' have a path were both data and clock get a false path constraint - I have to admit that I can't think of a use-case for that one ATM, but reset & clock does make some sense...
Not sure having separate booleans isn't the preferable solution here?

[KireinaHoro]

Uh but what you're doing doesn't make sense: you're not actually using it to drive the clock enable/reset signal. If you look at this on the netlist level, clock enable (CE) and reset (PRE, etc.) are always separate pins and thus create separate paths; you can always add the attribute on the non-shared parts of the paths and not having to worry about tag conflict.
UPDATE: Ah I see the reasoning now. I think a better example is like this:

case class TestCC() extends Component {
  val io = new Bundle  {
    val i = in(Bool())
    val o = out(UInt(16 bits))
  }

  val cdWithEnRst = ClockDomain.internal(
    "cdWithEnRst",
    withClockEnable = true,
    config = ClockDomainConfig(
      resetKind = SYNC,
      resetActiveLevel = LOW,
    )
  )

  val inSync = BufferCC(io.i, allBufAttributes = Seq(
    crossClockFalsePath(destType = TimingEndpointType.CLOCK_EN),
    crossClockFalsePath(destType = TimingEndpointType.RESET),
  ))

  cdWithEnRst.clock := clockDomain.clock
  cdWithEnRst.clockEnable := inSync
  cdWithEnRst.reset := inSync

  new ClockingArea(cdWithEnRst) {
    io.o := CounterFreeRun(16 bits)
  }
}

But this is very much an artificial example. While we can surely lift the single tag instance requirement, I really cannot think of a case where this is important enough to warrant dropping the check.

[andreasWallner]

I think the example is actually not that esoteric - Altera recommends a synchronizer using the CE input of a FF, depending on where the reset for that is coming from you need the constraint on both of them.

Looks like this: https://www.intel.com/content/www/us/en/programmable/quartushelp/current/index.htm#da_rules/cdc_50102.htm
Personally I think these should then be delay constraints instead of false path constraints, but I've seen the latter used for that.

Instead of allowing multiple instances I'd go for multiple arguments, much shorter, just as extendible and if used with explicit parameter names just as readable.

[andreasWallner]

@KireinaHoro
How should we continue here? Modify it or leave as it is and drop the uniqueness requirement once we really need it?

[KireinaHoro]

I'm with leaving it as is. We can always come back when such a use case comes up

[andreasWallner]

I'll merge this then, thanks!

[andreasWallner]

@KireinaHoro @Dolu1990 what should we do with the two open threads? Would be great if we could merge this^^

[andreasWallner]

@KireinaHoro Thanks!

[Dolu1990]

@KireinaHoro Hi ^^

I'm currently looking into getting a more generalized aproach to get timings constraints.
Idea being to have some tool analyse the SpinalHDL netlist, and have a abstract software interface to listen at it.
(see https://github.com/SpinalHDL/SpinalHDL/blob/8c1fd9f75c27d97c288d46710c8a695a283ed7d6/lib/src/main/scala/spinal/lib/eda/TimingExtractor.scala)

On the way, i found some issues with the way the netlist is analiysed in the Xilinx version from this PR.
Let me know if you are interrested ^^
trying to get this right :D

[KireinaHoro]

@Dolu1990 hi!

Sounds like a sensible approach, since the logic walking the DSL to derive constraints should be shared across all tool vendors, even though the actual constraint syntax might differ. I'd be happy to test the new approach on my local project :)

Tell me about what's wrong with the current VivadoConstraintWriter :)

[Dolu1990]

Cool ^^

Tell me about what's wrong with the current VivadoConstraintWriter :)

Mostly the def doWalkStatements which match on BaseType instead of Assignement statements
Aswell as the

    var source = s.source.asInstanceOf[BaseType]
    if (!resetIsDriver) {
      AnalysisUtils.seekNonCombDrivers(source) { case b: BaseType =>
        source = b
      }
    }

which assumes the source is a basetype, aswell as assuming only one final source can be reached.

Another missing thing is that crossclock memories are ignored

I'm trying to get those things fixed in https://github.com/SpinalHDL/SpinalHDL/blob/8c1fd9f75c27d97c288d46710c8a695a283ed7d6/lib/src/main/scala/spinal/lib/eda/TimingExtractor.scala + a few fixes in the AnalysisUtils :)

[KireinaHoro]

Mostly the def doWalkStatements which match on BaseType instead of Assignement statements

and

assuming only one final source can be reached

You're right, I didn't (and maybe still don't) have a very clear mental model of how a SpinalHDL netlist looks like.

Excited to see the new design!

[Dolu1990]

no worries ^^