Fixed convertReset

[rowanG077]

convertReset was not converting synchronous signals correctly. I added an additional check to never apply a synchronizer if the domains are equal. I also exposed the newly created sameDomain for users to use.

[martijnbastiaan]

Thanks for the changes @rowanG077!

And thanks for digging in, because that made me think: I believe we also need to synchronize whenever the target domain is asynchronous, as Clash assumes resets asynchronous resets are deasserted synchronously. (Not doing this might lead to meta-stability.) Note that calling resetSynchronizer in this case would still let the original asynchronous reset assert asynchronously.

In the future we'd like to make this more explicit by allowing something like:

topEntity :: Clock dom -> Reset Any -> ....
topEntity clk rstNotSync = ..
  where
    rst = resetSynchronizer clk rstNotSync

..but this would require KnownDomain properties to be stored in Reset/Clock/.. for simulation purposes.

[rowanG077]

Yes clear. If asynchronous resets are a precondition for clash we always need a synchronizer. The implementation now uses the resetSynchronizer when going to an asynchronous domain and the original dual flip flop synchronizer when going to a synchronous domain.

[christiaanb]

Well, to be clear: Clash can cannot model the asynchronous deassertion. So while the resetSynchronizer isn't strictly needed, it is recommended to prevent meta-stability. So I'm not too sure on adding the resetSynchronizer inside of convertReset when doing a to-asynchronous conversion: the circuit consuming this reset might already have a resetSynchronizer, and so having it inside convertReset would mean double work. So I'm leaning in favour of not adding the synchronizer in the to-asynchronous case.

[martijnbastiaan]

Christaan, don't you think the negatives outweigh the positives in this case? In the worst case, you'll add two superfluous 1-bit flipflops to your design, and in the best case you prevent (I assume extremely hard to debug) meta stability.

In any case, if we do decide to add resetSynchronizer just to the synchronous case, I think it'd be a good idea to add a big disclaimer to covertResets documentation :).

[christiaanb]

Rowan, Martijn: I can see Martijn's point, but then I would like the reset assertion and de-assertion delay documented. Something along the lines:

* When the outgoing reset belongs to a domain with memory elements that have a synchronous clear/set, both the assertion and de-assertion of the reset are delayed by two cycles.
* When the outgoing reset belongs to a domain with memory elements that have an asynchronous clear/set, reset assert is immediate, but reset de-assertion is delayed by two cycles. 

In addition, this change cannot be backported to 1.2, as it is an API change.

[martijnbastiaan]

Alright, thanks for the discussion, I think @rowanG077 can make an informed decision now :-).

When both synchronization cases would use resetSynchronizer resets would be asserted immediately for both. I.e., documentation could be simplified to:

 reset assert is immediate, but reset de-assertion is delayed by two cycles. 

[christiaanb]

But resetSynchronizer should not be used for domains with memory elements that have synchronous set/clear ports! Because resetSynchronizer still uses an asynchronous assertion, which could lead to meta-stability when used on memory elements with synchronous clear/set.

[martijnbastiaan]

Jep, makes sense @christiaanb

[rowanG077]

Yes that's why I already had a normal flip-flop when going to a synchronous domain. I have updated the documentation to reflect this.

@christiaanb @martijnbastiaan

[martijnbastiaan]

Looks good @rowanG077.

Seems like I made a mistake while implementing resetSynchronizer: it should basically do exactly what convertReset does, except that it would always insert a synchronizer (due to the cumbersome nature of creating domains). Then convertReset would become:

convertReset clkA clkB rstA0 =
  case sameDomain @domA @domB of
    Just Refl -> rstA1
    _ -> resetSynchronizer clkB rstA1 enableGen
 where
  rstA1 = unsafeToReset (unsafeSynchronizer clkA clkB (unsafeFromReset rstA0))

and resetSynchronizer:

resetSynchronizer clk rst ena =
    unsafeToReset 
  $ register clk rst ena isActiveHigh
  $ register clk rst ena isActiveHigh
  $ pure (not isActiveHigh)
 where
  isActiveHigh = case resetPolarity @dom of { SActiveHigh -> True; _ -> False }

This PR looks good though, so I wouldn't want to burden you with any more work :). Are you happy with it? If so, this can be merged.

edit: resetSynchronizer should use dflipflop clk (dflipflop clk rst) for synchronous target domains, like you did for this PR.

[rowanG077]

@martijnbastiaan I rather fix it now then run the risk of this conversation being forgotten :). I have applied the changes you suggested. I updated the docs for resetSynchronizer to include the way synchronous signals are handled. But be aware that dflipflop have an undefined initial value whereas delay does include a default value.

[christiaanb]

@rowanG077 I think @martijnbastiaan copied that from an internal discussion: please do use delay instead of dflipflop, but just connect the enable argument to enableGen.

We intended to get rid of that Enable argument anyhow, as you can see from that haddock comment.

[rowanG077]

@christiaanb Clear! I have pushed the changes!

[martijnbastiaan]

Not sure if you can see the CircleCI logs, but the test ResetSynchronizerSync fails. You can run that test with:

cabal run testsuite -- -p ResetSynchronizerSync.VHDL

You can replace VHDL with Verilog or SystemVerilog. Simulators needed:

• VHDL: GHDL
• Verilog: iverilog
• SystemVerilog: modelsim

In case you can figure it out from the error message alone:

              Stdout was:
              @630001ns: outputVerifier, expected: 0xxx, actual: 1001

[martijnbastiaan]

And yeah, forgot about the initial value 😓

[martijnbastiaan]

@rowanG077 I had some train time so I looked in to it :-), this should do:

diff --git a/tests/shouldwork/Signal/ResetSynchronizer.hs b/tests/shouldwork/Signal/ResetSynchronizer.hs
index aa2af3de..400fe19c 100644
--- a/tests/shouldwork/Signal/ResetSynchronizer.hs
+++ b/tests/shouldwork/Signal/ResetSynchronizer.hs
@@ -28,24 +28,25 @@ testReset tbClk tbRst cClk =
   $ stimuliGenerator tbClk tbRst
     ( True
 
-      -- Reset synchronizer introduces a delay of two, but lets asynchronous
-      -- asserts through untouched. This means that for 'topEntity' we'll see
-      -- [R, R, R, R] for these cycles. Where the first two zeroes are caused by
-      -- 'resetSynchronizer's induced latency, the next zero by the first cycle
-      -- not in reset, and the last zero because we're in reset again (async
-      -- behavior).
+      -- Asynchronous resets:
+      --
+      --   Resets are asserted asynchronously and deasserted synchronously with
+      --   a delay of two cycles. This means that for 'topEntity' we'll see
+      --   [R, R, R, R] for the first batch of reset cycles. The first two Rs
+      --   are caused by  'resetSynchronizer's induced latency, the next zero
+      --   by the first cycle not in reset, and the last zero because we're in
+      --   reset again (async assertion behavior).
+      --
+      --   The second batch of resets is similar to ^, but the reset is held
+      --   a cycle longer so we should be able to see a count output.
+      --
+      -- Synchronous resets:
+      --
+      --   Synchronous resets are simply delayed for two cycles. Hence, we
+      --   should count up to the number of deasserted cycles.
       --
-      -- We should be able to see a difference between asynchronous and synchronous
-      -- resets here: the counter is driven by a register whose synchronicity
-      -- is imposed by the 'KnownDomain' constraint. Synchronous counters will
-      -- only see the reset asserted on the /next/ cycle, hence outputting
-      -- [R, R, R, 0] instead.
    :> False :> False :> False :> True
-
-      -- Similar to ^, but now we're out of of reset one cycle longer so we should
-      -- start seeing [R, R, R, 0, R] for asynchronous counters, and
-      -- [R, R, R, 0, 1] for synchronous ones.
-   :> False :> False :> False :>  False :> True
+   :> False :> False :> False :> False :> True
    :> replicate d20 False )
 
 polyTopEntity
@@ -76,10 +77,12 @@ polyTestBench ::
 polyTestBench Proxy = (done, sampleN 20 (polyTopEntity cClk cRst))
  where
   rKind = resetKind @circuitDom
+  rOr' = flip rOr rKind
+
   expectedOutput = outputVerifier tbClk tbRst
-    ( RRRRRRR :> RRRRRRR :> RRRRRRR :> RRRRRRR :> rOr 0 rKind
-   :> RRRRRRR :> RRRRRRR :> RRRRRRR :> Count 0 :> rOr 1 rKind
-   :> RRRRRRR :> RRRRRRR :> RRRRRRR :> Count 0 :> Count 1 :> Count 2
+    ( RRRRRRR :> RRRRRRR :> RRRRRRR :> RRRRRRR :> rOr' 0
+   :> rOr' 1  :> rOr' 2  :> RRRRRRR :> Count 0 :> rOr' 1
+   :> rOr' 2  :> rOr' 3  :> RRRRRRR :> Count 0 :> Count 1 :> Count 2
    :> Nil )
   done = expectedOutput (polyTopEntity cClk cRst)
   (tbClk, cClk) = biTbClockGen @System @circuitDom (not <$> done)

[rowanG077]

@martijnbastiaan Thanks! I have some issues getting GHDL working on NixOS :(... I will add your changes today.

[martijnbastiaan]

Great :). Just ping me when it's done, I'll pack the changes in 1.2.5 (#1585).

[rowanG077]

@martijnbastiaan Once CI finishes succesfully this can be merged.

[martijnbastiaan]

Forgot that this is an API change, so it won't up in 1.2.5. I'll merge it to master when the CI succeeds :)