Skip to content

Conversation

@rowanG077
Copy link
Member

@rowanG077 rowanG077 commented Jan 8, 2026

Aimed to address #3115

This updates the documentation of holdReset to indicate the problems it has. It also gives a new function stretchReset that should have the correct behaviour but has slightly different timing characteristics so it's not a drop in replacement.

I have marked the holdReset function with a warning, I don't like it since now we introduce warning into our own project when exposing it and using it in tests. I also thought a deprecation pragma. Both have some implication as that requires a version bump according to haskell PVP specification.

Still TODO:

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

@rowanG077 rowanG077 linked an issue Jan 8, 2026 that may be closed by this pull request
stretchResetAsync clk en SNat rst = rstOut
where
-- This ensures the index at least has one cycle for counting.
counter :: Signal dom (Index ((Max 2 ((Max 1 n) - 1))))
Copy link
Member Author

@rowanG077 rowanG077 Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dislike this a lot but imo it's better than forcing 3 <= n constraint on the user.

Copy link
Member

@christiaanb christiaanb Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not make it so that you create different circuits for different values of n? i.e.

stretchResetAsync clk en sn@SNat rst = rstOut
 where
  rstOut = case snatToInteger sn of
    0 -> rst
    1 -> unsafeToReset (delay clk en isActiveHigh (unsafeFromReset rst))
    _ -> unsafeToReset (if isActiveHigh then fmap (/=maxBound) counter else fmap (==maxBound) counter)

  counter :: Signal dom (Index n)
  counter = register clk rst en 0 (fmap (satSucc SatBound) counter)

  isActiveHigh = case resetPolarity @dom of { SActiveHigh -> True; _ -> False }

I explicitly left out the resetSynchronizer because the incoming reset should already be synchronously deasserted for the counter to work properly.

Copy link
Member Author

@rowanG077 rowanG077 Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need the reset synchronizer to stop the glitches as indicated by the comment in #3115 (comment)

I thought only one FF is enough though since there should be Recovery and Removal timing checks from the counter output FFs to the reset input. But I'm not 100% sure.

My reasoning for not being 100% sure is that the recovery and removal check only apply to the de-assertion path so timing closure will not ensure any glitch assertion will be safe with respect to recovery and removal checks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the linked comment you write assertion, but what you are describing is a glitch deassertion of the outgoing reset. Just FYI; I wondered if you got them mixed up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You just need a simple register on the output to stop the glitching. So in that case rstOut would be something along the lines of:

case snatToInteger sn of
  0 -> rst
  1 -> unsafeToReset (register clk rst en isActiveHigh (pure (not isActiveHigh))
  _ -> unsafeToReset (register clk rst en isActiveHigh (if isActiveHigh then fmap (/=maxBound) counter else fmap (==maxBound) counter)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And even if the outer flop does go meta-stable because CLR/SET port is asserted later on the outer flop than the CLR/SET ports of the counter flops, the circuit will recover, because eventually the assertion will arrive at the CLR/SET port of the outer flop.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if you want to use the standard dual FF reset synchronizer, that's fine by me. The only thing I care about is that stretchReset has a simple semantics. If I want the to stretch the deassertion of the reset by 1 cycle, it should just stretch the deassertion by 1 cycle, regardless of whether I have synchronous or asynchronous resets.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's important that you can configure it precisely, to control the order in which stuff comes out of reset, but the ability to stretch by very small amounts seems less important (not sure, just can't think of a case where that'd matter much). So if it can't stretch by just 1 or 2 cycles, I think that'd be fine, but if you say n it should really mean n.

Copy link
Member Author

@rowanG077 rowanG077 Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes indeed! I now am convinced that this is safe. I was thinking of a possibility where the rst source -> sync FF would be much slower than rst source -> counter FF -> comb logic -> sync FF could induce meta stability. But that's just a standard case with async resets and does not introduce problems since eventually everything will reset. I realized it when I drew it out.

I will update the code and doc. Thank you for the discussion!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is solved now. Pleas re-review.

@rowanG077 rowanG077 force-pushed the 3115-holdreset-output-is-not-registered branch from e5e62ad to 9b6053e Compare January 8, 2026 22:42
@rowanG077 rowanG077 changed the title Add warning to holdReset and add correct implementation Add documentation to holdReset and add correct implementation Jan 8, 2026
@rowanG077 rowanG077 force-pushed the 3115-holdreset-output-is-not-registered branch from 9b6053e to 46bccdb Compare January 8, 2026 22:45
@rowanG077 rowanG077 force-pushed the 3115-holdreset-output-is-not-registered branch 3 times, most recently from 4be1af9 to 7c377d4 Compare January 13, 2026 15:50
@rowanG077 rowanG077 force-pushed the 3115-holdreset-output-is-not-registered branch from 7c377d4 to 087e8bd Compare January 13, 2026 16:00
@rowanG077
Copy link
Member Author

rowanG077 commented Jan 13, 2026

I have removed the WARNING from holdReset. I propose we either replace holdReset with stretchReset on release of 1.10 or:

  1. On release of 1.10 we add a DEPRECATED on holdReset.
  2. On release of 1.12 we remove holdReset.

@rowanG077 rowanG077 force-pushed the 3115-holdreset-output-is-not-registered branch 5 times, most recently from 7400069 to f8eeee6 Compare January 15, 2026 13:44
@rowanG077 rowanG077 changed the title Add documentation to holdReset and add correct implementation Fix holdReset glitches for async resets and registerSyncReset Jan 15, 2026
@rowanG077 rowanG077 force-pushed the 3115-holdreset-output-is-not-registered branch 3 times, most recently from 3dd15c1 to 3957eac Compare January 15, 2026 16:00
@rowanG077
Copy link
Member Author

rowanG077 commented Jan 15, 2026

I ended up keeping the same unregistered implementation for the holdReset function for sync resets while fixing the bugs that it had and adding a registerSyncReset. This way the documentation is simple, it fixes the glitching problem for async resets and user can choose to register the reset themselves.

This also means we don't need a separate implementation or deprecation.

@JvWesterveld Are you satisfied with this?

@rowanG077 rowanG077 force-pushed the 3115-holdreset-output-is-not-registered branch from 3957eac to eb19789 Compare January 15, 2026 17:25
@leonschoorl
Copy link
Member

This now again has the weird quirk that for synchronous resets is it holds it for 1 cycle less then one might expect.

>>> let sampleReset = sampleN 8 . unsafeToActiveHigh
>>> let rst = fromList [False, False, False, False, True, False, False, False]
>>> sampleReset $ holdReset @System clockGen enableGen d2 $ unsafeFromActiveHigh rst
[True,True,False,False,True,True,True,False]
>>> sampleReset $ holdReset @XilinxSystem clockGen enableGen d2 $ unsafeFromActiveHigh rst
[True,True,False,False,False,True,True,False]

Although that is something that holdReset always did.
And hasn't changed in this PR.

So maybe this should be a different issue/PR, but the synchronous behavior is something we should document (with a doctest example).
And possibly change, I'm not sure.

@rowanG077 rowanG077 force-pushed the 3115-holdreset-output-is-not-registered branch from eb19789 to a601dac Compare January 17, 2026 01:17
@rowanG077 rowanG077 force-pushed the 3115-holdreset-output-is-not-registered branch from 425e435 to 1a97376 Compare January 17, 2026 02:27
@rowanG077 rowanG077 changed the title Fix holdReset glitches for async resets and registerSyncReset Fix holdReset glitches for async resets and add registerSyncReset Jan 17, 2026
@rowanG077 rowanG077 force-pushed the 3115-holdreset-output-is-not-registered branch 3 times, most recently from b3898f4 to 7aef9ce Compare January 17, 2026 03:19
@DigitalBrains1
Copy link
Member

Although that is something that holdReset always did.
And hasn't changed in this PR.

While I don't like that behaviour, I would like to fix holdReset in 1.8. So I'd like a version of holdReset that keeps the existing timing but fixes the unsafety of the function.

We could fix that quirk in 1.10, but if we just fix holdReset and make a changelog entry for it, that has a serious risk: we're changing the timing of a reset manager a user wrote, and only if the user notices the changelog entry will they not be caught off guard by that. So we might want to either accept and document the quirk, or make a new function without the quirk.

@rowanG077 Could you please label this PR with backport 1.8? First of all, this will make mergify start a backport PR, and secondly, it will clue in reviewers that this is intended to be backported. Unless of course you do not intend to backport this, in that case don't label it backport 1.8 😜 .

Copy link
Contributor

@JvWesterveld JvWesterveld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rowanG077 Yes, approach LGTM. Thank you for picking it up.

@rowanG077 rowanG077 force-pushed the 3115-holdreset-output-is-not-registered branch 5 times, most recently from 0bc273e to 7d781b7 Compare January 18, 2026 16:16
@rowanG077 rowanG077 force-pushed the 3115-holdreset-output-is-not-registered branch from 7d781b7 to c8ad98c Compare January 18, 2026 17:07
Copy link
Member

@leonschoorl leonschoorl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With that orReset it fixes the weird quirk for synchronous resets, which is very nice!

But since this now creates a (small) behavioral change compared to the previous behavior for synchronous resets, should we be be backporting this to 1.8?

@rowanG077
Copy link
Member Author

@leonschoorl We talked about this in the office and in chat. Since the old behavior is considered broken we think it should be backported.

for async resets

add `registerSyncReset`
@rowanG077 rowanG077 force-pushed the 3115-holdreset-output-is-not-registered branch from c8ad98c to a9a880c Compare February 9, 2026 22:13
@rowanG077
Copy link
Member Author

I didn't get any green review checkmarks yet. But all issues have been resolved. So if anyone has any problems with this PR please let me know.

@rowanG077 rowanG077 merged commit b1b1477 into master Feb 10, 2026
42 of 47 checks passed
@rowanG077 rowanG077 deleted the 3115-holdreset-output-is-not-registered branch February 10, 2026 08:23
mergify bot pushed a commit that referenced this pull request Feb 10, 2026
for async resets

add `registerSyncReset`

(cherry picked from commit b1b1477)

# Conflicts:
#	clash-prelude/src/Clash/Signal.hs
#	tests/shouldwork/Signal/ResetGen.hs
rowanG077 added a commit that referenced this pull request Feb 10, 2026
for async resets

add `registerSyncReset`

(cherry picked from commit b1b1477)

# Conflicts:
#	clash-prelude/src/Clash/Signal.hs
#	tests/shouldwork/Signal/ResetGen.hs
rowanG077 added a commit that referenced this pull request Feb 10, 2026
for async resets

add `registerSyncReset`

(cherry picked from commit b1b1477)

# Conflicts:
#	clash-prelude/src/Clash/Signal.hs
#	tests/shouldwork/Signal/ResetGen.hs
rowanG077 added a commit that referenced this pull request Feb 10, 2026
for async resets

add `registerSyncReset`

(cherry picked from commit b1b1477)

# Conflicts:
#	clash-prelude/src/Clash/Signal.hs
#	tests/shouldwork/Signal/ResetGen.hs

Co-authored-by: Rowan Goemans <goemansrowan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

holdReset output is not registered

5 participants