Fix Xilinx clock wizard and add Tcl IP generation

[hiddemoll]

This PR fixes the Xilinx clock generation primitives and adds support for clock primitive instantiation on Xilinx without manually going through the Vivado Clocking Wizard. The updated black boxes generate Tcl code to instantiate an MMCM through the Clocking Wizard. The clock primitives are limited to 1 output clock, and either a single-ended or a differential input clock. The generated Tcl script conforms with the Clash<->Tcl API.
​
Fixed in this PR:

• Broken Haskell simulation
  The Haskell simulation of the Xilinx clock primitives was incorrect due to a polarity mismatch in the lock output of Xilinx clockWizard and clockWizardDifferential: Haskell simulation was the inverse of HDL simulation and hardware. In Haskell simulation, the primitives had an Enable as their output, which was asserted when the PLL lock was deasserted. This corresponds to an asserted reset input to the primitive in the simulation model, meaning the Enable output was asserted iff the PLL reset input was asserted. HDL simulation and hardware meanwhile had an active high lock signal output (which is usually used as an active low reset signal to the circuit). Finally, the simulation model did not account for different frequencies of the input and output domains, instead treating individual input samples as individual output samples without resampling.
  This has been fixed by changing the type to Signal Bool and asserting it when the PLL has locked.
• Wrong port names of blackbox primitives
  The blackbox primitives for clockWizard and clockWizardDifferential had capitalized port names. Also, the clockWizardDifferential primitive had 2 identical port names for the differential input clock signals.
• Remove Asynchronous constraint from Xilinx clockWizard and clockWizardDifferential
  Originally intended to signal that these functions react synchronously to the incoming reset and that the outgoing lock signal is an asynchronous signal. Since synchronous reset signals are a subset of asynchronous reset signals this constraint on the input is vacuous. The constraint on the lock output does not convey this information at all and is wrong. Note that it is still necessary to synchronize the lock output in your design.
  ​

Known problems:

• The locked signal is asynchronous. Therefore, this function is unsafe.
• The Clocking Wizard uses input and output frequencies to determine the multiplier and divider for the MMCM, but Clash uses periods instead. The conversion from period (picoseconds, Natural) to frequency (MHz, Double) is not exact. This is already not exact when using hzToPeriod, e.g. hzToPeriod 300e6 translates to a period of 3333 ps, which becomes 300.03000300030004 in Tcl.
  ​

Still TODO:

​

• Write a changelog entry (see changelog/README.md)
• Check copyright notices are up to date in edited files

[martijnbastiaan]

Nice changes, this definitely makes the primitive more usable.

Two remarks on the output Signal domPllLock Bool:

• The domain domPllLock isn't bound by an argument, so any domain can be instantiated here. This is incredibly unsafe.
• We never produce asynchronous signals in Clash, we shouldn't break this pattern.

So I propose:

• Renaming the current functions to unsafeClockWizard[Differential].
• Rename domPllLock to domOut again. It should be documented that this an asynchronous signal and the user should think twice about using it.
• Create a new function clockWizard[Differential] which calls unsafeClockWizard[Differential] and synchronizes the signal using a dualFlipFlopSynchronizer.

[DigitalBrains1]

Two remarks on the output Signal domPllLock Bool:

• The domain domPllLock isn't bound by an argument, so any domain can be instantiated here. This is incredibly unsafe.
• We never produce asynchronous signals in Clash, we shouldn't break this pattern.

I explicitly requested Hidde to write it like this. This is how we do it and I think have always done it in Clash.Clocks, which provides the constraints and implementation for the Intel PLL alteraPll. It is actually a better way to represent reality. The PLL lock signal is asynchronous and any domain is a lie we have to maintain because Clash doesn't know about asynchronous signals. Same, by the way, for the reset input Reset domIn in these primitives; luckily all these primitives are fine with purely asynchronous reset inputs.

The reason I have requested it is precisely because I'm working on a class to make these PLL's more user friendly. Currently we always have to write

(clk100, pllStable) = clockWizard (SSymbol @"clkWizard50to100") clk50 rstIn
rstSync = resetSynchronizer clk $ unsafeFromLowPolarity pllStable

which means we're forcing the user to remember this and teaching them to use functions that begin with the word unsafe. I'd like to remedy both by changing the invocation to

(clk100, rstSync) = clocksSynchronizedReset (clockWizard (SSymbol @"clkWizard50to100")) clk50 rstIn

but this implementation absolutely requires that pllStable is a Signal domIn Bool to get unsurprising simulation behaviour in Clash (for HDL, it doesn't matter, since the domain is a lie anyway). So I asked Hidde to make the domain as unrestricted as it has always been for alteraPll, so I can restrict it to domIn in clocksSynchronizedReset.

By the way, for multiple clocks, clocksSynchronizedReset works as follows:

instead of

(clk100, clk150, pllStable :: Signal DomIn Bool) = alteraPll (SSymbol @"alterapllmulti") clk50 rst50
rst100 = resetSynchronizer clk100 $ unsafeFromLowPolarity $ unsafeSynchronizer clk50 clk100 pllStable
rst150 = resetSynchronizer clk150 $ unsafeFromLowPolarity $ unsafeSynchronizer clk50 clk150 pllStable

you write

(clk100, rst100, clk150, rst150) = clocksSynchronizedReset (alteraPll (SSymbol @"alterapllmulti")) clk50 rst50

You might actually need to do something more clever with multiple domains (i.e., a reset manager), but in this simple case, you can use the simple implementation.

[DigitalBrains1]

By the way, along with my clocksSynchronizedReset will come improved documentation in both Clash.Xilinx.ClockGen as well as Clash.Intel.ClockGen. The scope of this PR was agreed to be as small as viable: fixing the Xilinx clock wizard so it could be used, and that is that. As far as I'm concerned, further improvements can come in future PR's.

(Nitpick: you wrote synchronizes the signal using a dualFlipFlopSynchronizer, but you probably meant resetSynchronizer, a DFF sync is not what we want to see here).

[martijnbastiaan]

Agreed on keeping the scope of this PR limited.

which means we're forcing the user to remember this and teaching them to use functions that begin with the word unsafe. I'd like to remedy both by changing the invocation to

(clk100, rstSync) = clocksSynchronizedReset (clockWizard (SSymbol @"clkWizard50to100")) clk50 rstIn

I get what you're saying, but this just hides the fact that clockWizard is really unsafe to use. Anything that doesn't have unsafe prepended to its name should be assumed to be safe w.r.t. domain crossing / synchronicity. This is a precedent we set in clash-prelude (though subsequently break in the clock wizard primitives). So my very strong preference would still be to rename the current primitive yielding asynchronous signals to unsafe* and introducing a non-unsafe version that does the synchronization for you.

Got to run, will elaborate on later. Concerns addressed / consensus achieved in the comments below.

[christiaanb]

We could make it safe by adding Not (domOut == pllLock), Not (domIn == pllLock) as a constraint. That way domOut and pllLock cannot unify when clockWizzard is invocated. Meaning you must assign a domain to pllLock that is apart from domOut and domIn; thus forcing you to synchronize if you want to use the signal in either of those domains.

This would make it safe as it prevents accidental synchronization.

It would make it inconvenient to use though, as you have to create a KnownDomain instance for the concrete pllLock domain.

[DigitalBrains1]

Ah, nice work! I just have a few little things.

[DigitalBrains1]

Meaning you must assign a domain to pllLock that is apart from domOut and domIn; thus forcing you to synchronize if you want to use the signal in either of those domains.

This has surprising simulation results in Clash simulation, though. In HDL unsafeSynchronizer is just a wire, but in Clash it resamples. The simulation model of the lock output in Clash is that it is deasserted for as long as the Reset domIn is asserted. So it always starts as a signal in domIn. Suppose domIn has a period of 10 ns and domOut has a period of 2 ns. A 20 ns reset pulse on Reset domIn will keep domOut with a deasserted lock signal for 10 ticks, a period of 20 ns. Now suppose we introduce a domain for the lock signal that has a period of 100 ns. Suddenly a 20 ns pulse on Reset domIn will have the lock signal in domOut, with a period of 2 ns, asserted for 50 ticks which is 100 ns. This just feels weird, to introduce a domain that resamples like that purely to avoid people accidentally treating it as synchronous.

[edit]
You can even miss resets if the domIn reset is fully between edges of the 100 ns domain, meaning the resampling just passes over the asserted bit.
[/edit]

[DigitalBrains1]

The updated Blackboxes generates Tcl to instantiate a PLL or MMC through the Clocking Wizard.

This, by the way, is inaccurate. It always generates an MMCM. The automatic mode which picks the one the wizard likes best is only supported on UltraScale and UltraScale+, so we stuck to the default MMCM so it also works on at least 7 series (I don't think anything older, but not sure).

[DigitalBrains1]

So my very strong preference would still be to rename the current primitive yielding asynchronous signals to unsafe* and introducing a non-unsafe version that does the synchronization for you.

I fully support this (in a new PR).

[DigitalBrains1]

Since we support Vivado in CI, we could (in a future PR) add a simple test that verifies we can instantiate and simulate a clockWizard. I'm talking about a dummy simulation, not actually verifying the output of the primitive, just that Vivado does not throw any errors while doing it. We could just have a counter on the output clock that will count towards test completion (done = cnt .==. pure maxBound). Shall we make a TODO issue for that?

[DigitalBrains1]

I've completed a basic functionality test bench; it is in the branch peter/xilinx-clockwiz. It adds seClkToDiffClk to our test bench functions, which still needs a bit more documentation. The idea is you put that commit first, followed by the Add Xilinx clock wizard support through Tcl commit. You can either include it here in this PR, or we create a separate PR for it if you want this merged quickly without more work.

Now that I see the commit summary, I realise I did not review the commit message. I'd like it to be more explicit. After all, you fix the API, you fix everything else about it (broken Haskell simulation, wrong port names, was there more?), and you add Tcl instantiation support. But the only thing you mention is that last bit :-D. Could you rephrase it to something like "Fix Xilinx clock wizard, add Tcl IP generation"? And also change the title and cover letter of this PR accordingly?

[edit]
I've just renamed the function to seClockToDiffClock and force-pushed. Abbreviating it as much as I did was a mistake given the established naming.
[/edit]

[DigitalBrains1]

Could you reorder the commits? If you put Generalize periodToHz first, you don't need Change frequency calculation to use periodToHz.

[martijnbastiaan]

@DigitalBrains1's suggestions look good. I'm fine with merging after applying them.

[hiddemoll]

I have already applied all Peters changes, with the exception of two:

1. I have not yet applied the infinite-list for the blackbox parameters
2. The periodToHz changes. Peter moved these to their own PR, which got merged over the weekend

I think I have only one more thing to do for this PR, which is removing my periodToHz changes, rebase those from Peter and then remove those commits without breaking other things.

[DigitalBrains1]

Whoops, hit "Approve" too soon.

[DigitalBrains1]

Two remarks on the output Signal domPllLock Bool:

After much internal discussion we decided to leave this part alone in this PR, i.e., leave it at domOut, and tackle this in a future PR where we rename these primitives to unsafe... and introduce safe versions.