Fix Clash.Clocks lock signal

[DigitalBrains1]

The way we tried to generate the lock signal in Clash.Clocks was very wrong. For instance, a Clash.Intel.ClockGen.alteraPll with two clock outputs would boil down to:

alteraPll ::
  ( KnownDomain domIn
  , KnownDomain c1
  , KnownDomain c2
  ) =>
  SSymbol name ->
  Clock domIn ->
  Reset domIn ->
  ( Clock c1
  , Clock c2
  , Signal pllLock Bool
  )
alteraPll !_ = clocks
 where
  clocks clk@(Clock _ Nothing) rst =
    ( unsafeCoerce clk
    , unsafeCoerce clk
    , unsafeCoerce rst
    )
  clocks _ _ = error "clocks: dynamic clocks unsupported" 

unsafeCoerce rst is wrong in three ways:

• You can't coerce a Reset into a Signal, it segfaults.
• The time base is wrong: one input sample becomes one output sample even when the output clock has a different period than the input clock.
• There is no handling of ResetPolarity. The simulation model is that lock is deasserted when reset is asserted, but this was not effected.

This PR changes the implementation to:

alteraPll ::
  ( KnownDomain domIn
  , KnownDomain c1
  , KnownDomain c2
  , KnownDomain pllLock
  ) =>
  SSymbol name ->
  Clock domIn ->
  Reset domIn ->
  ( Clock c1
  , Clock c2
  , Signal pllLock Bool
  )
alteraPll !_ = clocks
 where
  clocks clk@(Clock _ Nothing) rst =
    ( unsafeCoerce clk
    , unsafeCoerce clk
    , unsafeSynchronizer clockGen clockGen (unsafeToLowPolarity rst)
    )
  clocks _ _ = error "clocks: dynamic clocks unsupported" 

Note that we needed a KnownDomain pllLock as well.

Still TODO:

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

[DigitalBrains1]

The extra KnownDomain pllLock is an API change. It could be someone wrote:

myPll ::
  (KnownDomain domIn, KnownDomain domOut) =>
  Clock domIn ->
  Reset domIn ->
  (Clock domOut, Signal pllLock Bool)
myPll = altpll (SSymbol @"mypll")

and now needs to add KnownDomain pllLock to that definition. I feel this should still be backported because the advantage of working (non-segfaulting) Haskell simulation outweighs this disadvantage. But I shall add it to the change log.

[edit]
Note that once source is adjusted to add the KnownDomain constraint, it will continue to compile in older Clash versions as well because functions can be overconstrained without issue.
[/edit]

[leonschoorl]

I think the examples now also require an type annotation on the pllLocked signal to type check:

clash-compiler/clash-prelude/src/Clash/Intel/ClockGen.hs

Lines 144 to 145 in 2a028b3

	-- (clk100 :: 'Clock' Dom100MHz, pllLocked) =
	-- 'alteraPll' ('SSymbol' \@\"alterapll50to100\") clk50 rst50

clash-compiler/clash-prelude/src/Clash/Intel/ClockGen.hs

Lines 154 to 155 in 2a028b3

	-- (clk100 :: 'Clock' Dom100MHz, clk150 :: 'Clock' Dom150MHz, pllLocked) =
	-- 'alteraPll' ('SSymbol' \@\"alterapllmulti\") clk50 rst50

This next one already has all the type annotations, but is missing the = between the pattern and the alteraPll. Maybe you can fix that while you're in there.

clash-compiler/clash-prelude/src/Clash/Intel/ClockGen.hs

Lines 178 to 180 in 2a028b3

	-- (clk :: 'Clock' 'Clash.Signal.System', pllStable :: 'Signal' 'Clash.Signal.System' 'Bool')
	-- 'alteraPll' ('SSymbol' \@\"alterapll50to100\") clkInp
	-- ('Clash.Signal.unsafeFromLowPolarity' rstInp)

And the Xilinx prims clockWizard and clockWizardDifferential don't have the same crashing issues, but do a similar unsafeCoerce between domains with (potentially) different periods.

clash-compiler/clash-prelude/src/Clash/Xilinx/ClockGen.hs

Lines 55 to 56 in 2a028b3

	clockWizard !_ clk@(Clock _ Nothing) rst =
	(unsafeCoerce clk, unsafeCoerce (toEnable (unsafeToHighPolarity rst)))

[DigitalBrains1]

We've talked about the points Leon raises. The documentation now is not incorrect (modulo the missing =) but it can definitely be improved, and I will tackle this in another PR. The Xilinx clockWizard stuff is being worked on by Hidde for Clash master / 1.8, but for 1.6, we're going to keep it as it is but write an issue report that it is... well... basically very weird. It will also not be part of this PR.