Implement solution to AsyncFIFO resets, as in #328 #329

awygle · 2020-03-05T04:42:00Z

Mark both read domain counters as reset-less, then synchronize write
domain reset into read domain, On write domain reset, force empty flag
and set read counters == write counters.

Marking this as WIP because the reset functionality isn't really tested yet. Interested in thoughts on how that might be done.

codecov · 2020-03-05T04:55:56Z

Codecov Report

Merging #329 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #329      +/-   ##
==========================================
+ Coverage   82.47%   82.53%   +0.06%     
==========================================
  Files          35       35              
  Lines        5913     5922       +9     
  Branches     1198     1198              
==========================================
+ Hits         4877     4888      +11     
+ Misses        876      875       -1     
+ Partials      160      159       -1

Impacted Files	Coverage Δ
nmigen/hdl/cd.py	`100% <ø> (ø)`	⬆️
nmigen/lib/fifo.py	`92.77% <100%> (+0.38%)`	⬆️
nmigen/back/rtlil.py	`82.95% <0%> (+0.32%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f8669c...8ed0654. Read the comment docs.

whitequark · 2020-03-05T04:58:14Z

nmigen/lib/fifo.py

+        r_rst = Signal()
+
+        # Create synthetic clock domain to work around ResetSynchronizer expecting a domain
+        fake_domain = ClockDomain(name="rst_cdc", async_reset=True, local=True)


I think this characterization isn't correct. You are adding some FFs with a different control set (one that includes w_rst), which in fact form a (very small) new clock domain. This doesn't inherently have anything to do with the way ResetSynchronizer works; nMigen requires you to create a clock domain object for every distinct async control set.

But ResetSynchronizer creates such a clock domain internally (right?), so this one is ~~unnecessary~~ maybe "incidental" is a better word.

You're totally right. I was thinking of ResetSynchronizer delegating to platform code, in which case it generally won't use an nMigen clock domain, but that's a different problem.

Do you feel like fixing it?

Happy to but the right solution wasn't obvious to me, other than making something like a AsyncAssertSyncDeassertSynchronizer module which ResetSynchronizer would use under the hood, and I didn't want to try to create a new cdc module without checking with you.

What about (a) adding a rst attribute (output), alongside arst (input), and (b) allowing passing domain=None to the constructor? Then the platform code would assert reset_sync.rst, which in the case when domain is specified will be assigned to the domain.

That should work. It seems a bit more complex to me, and it doesn't address a case where you might want to use this functionality for something besides a reset (admittedly I don't know what use case that would be), but it's a nice way to enhance ResetSynchronizer without (I think) having to touch all the platform code.

I lean towards creating the module I described (a more reasonable name might be ReleaseSynchronizer), but I'm open to the solution you described as well. I just think a generic module that gets replaced with platform-specific modules if available is a cleaner mental model.

without (I think) having to touch all the platform code.

You have to touch all the platform code with my suggestion.

I just think a generic module that gets replaced with platform-specific modules if available is a cleaner mental model.

I'm a bit confused here. ResetSynchronizer does get replaced with platform-specific modules; that's half the reason for its existence and being in nmigen.lib.

If you mean that you want to make ResetSynchronizer a thin wrapper around some other module that just does ResetSignal(self._domain).eq(...) then I'm not really opposed to it, but it seems like the overhead (having two modules that do almost the same thing, duplicate constructor arguments, documentation, etc) is not worth the benefit (maybe slightly cleaner separation of concerns).

admittedly I don't know what use case that would be

Yes. "Chain of FFs with asynchronous preset and synchronous clear" is a very specific use case. Also, sometime in the future we'll have to extend it to handle negative polarity resets (#185).

Let me just push what I'm talking about and we can discuss it directly instead of each talking about the picture in our head of the picture in the other person's head :) can always roll back.

nmigen/lib/fifo.py

whitequark · 2020-03-05T05:07:30Z

(There's a few style nits I skipped for now.)

Interested in thoughts on how that might be done.

I don't have any especially good ideas. Like you I find testing HDL (especially multiclock) to be pretty frustrating and I don't have silver bullets.

awygle · 2020-03-05T05:27:20Z

Are you willing to merge this without tests of that functionality? If not, it seems like the most logical thing would be to add some properties to FIFOContractSpec, which would also hit the Sync cases, so I'd have to spend time understanding that before I could add the necessary asserts. Totally willing to, it'll just delay this landing (which is also fine with me).

whitequark · 2020-03-05T05:46:36Z

Totally willing to, it'll just delay this landing (which is also fine with me).

There's no rush to support this, so I would prefer if it landed with tests and using the improved ResetSynchronizer interface.

awygle · 2020-03-05T06:21:12Z

In the process of trying to write what I was describing I realized it wouldn't work. I'm not sure what you described works either, because you still need a domain in order to get at the clock for the synchronous release. So I added a reset_less parameter (~~which... I forgot to document, oops~~) whose only function is to prevent hooking up the output of the synchronizer to the clock domain's actual reset. I also added a self.rst signal which gets assigned to the value of the output regardless of the value of reset_less (~~which I need to hook up in the platform case, and document, oops again~~). This makes the change backwards-compatible, since it'll just get ignored by all existing users. How does this look to you?

EDIT: Agreed reset_less isn't a great name. It is kind of analogous if you squint, since this is the only way you could use ResetSynchronizer into a reset_less domain, but it's fairly questionable.

whitequark · 2020-03-05T06:23:44Z

I'm not sure what you described works either, because you still need a domain in order to get at the clock for the synchronous release.

Argh, you're right, it doesn't. But I still think it's really confusing to have it called reset_less. I feel like we need a slightly different approach here then.

whitequark · 2020-03-05T06:27:02Z

What about adding something like AsyncFFSynchronizer that has a clk, i, o ports and a async_edge parameter that can be "pos" or "neg", like in a ClockDomain itself? That would serve the needs of AsyncFIFO here and we could make ResetSynchronizer a thin wrapper around it. It also maps better to what the platform code expects. That's essentially what you suggested, I think.

I did say earlier that adding a new entity here has a cost, but it seems like overloading the functionality of ResetSynchronizer like this has an even higher cost.

awygle · 2020-03-05T06:53:59Z

That's basically what I did except I still took a domain instead of a clk port, I didn't implement async_edge, and I called it ReleaseSynchronizer. (I didn't see your comment until I was almost done making this change, hence the naming differences)

If you agree in principle with this approach I can make those changes. I do think it should remain a domain though, for consistency with FFSynchronizer and friends.

whitequark · 2020-03-05T07:03:32Z

I do think it should remain a domain though, for consistency with FFSynchronizer and friends.

Yes, that make sense. Could you do it as a part of another PR, please?

awygle · 2020-03-05T07:08:39Z

Sure. You want me to pull all the ResetSynchronizer changes out from this PR into a separate one, yes?

whitequark · 2020-03-05T07:22:50Z

Sure.

Also use ReleaseSynchronizer in AsyncFIFO.

Most prominently, modify platform support to produce AsyncFFSynchronizers with configurable edges rather than ResetSynchronizers.

Also document async_edge.

Mark both read domain counters as reset-less, then synchronize write domain reset into read domain, On write domain reset, force empty flag and set read counters == write counters.

Also tone down language describing creation of new domain.

whitequark · 2020-03-08T21:38:30Z

nmigen/hdl/cd.py

@@ -21,6 +21,8 @@ class ClockDomain:
        If ``True``, the domain does not use a reset signal. Registers within this domain are
        still all initialized to their reset state once, e.g. through Verilog `"initial"`
        statements.
+    clock_edge : str


The actual parameter is called clk_edge.

(But maybe it should be called clock_edge instead? We have a rst signal and reset_less parameter. What should we call the parameter that sets reset polarity.)

derp. i fix. that's what i get for adding extras :p

Reset polarity could be rst_edge for consistency... but it's not really an edge-active signal, so rst_pol?

I was thinking rst_active="pos"/="neg", since resets can be both synchronous and asynchronous.

Sure, that's reasonable. I don't have too strong of opinions on this subject.

whitequark

Please document the behavior of AsyncFIFO with regards to reset in the docstring.

whitequark · 2020-03-08T23:01:07Z

nmigen/lib/fifo.py

@@ -331,7 +332,7 @@ def elaborate(self, platform):
        m.d.comb += produce_enc.i.eq(produce_w_nxt),
        m.d[self._w_domain] += produce_w_gry.eq(produce_enc.o)

-        consume_r_gry = Signal(self._ctr_bits)
+        consume_r_gry = Signal(self._ctr_bits, reset_less = True)


Suggested change

consume_r_gry = Signal(self._ctr_bits, reset_less = True)

consume_r_gry = Signal(self._ctr_bits, reset_less=True)

whitequark · 2020-03-08T23:01:19Z

nmigen/lib/fifo.py

+
+        # Async-set-sync-release synchronizer avoids CDC hazards
+        rst_cdc = m.submodules.rst_cdc = \
+                AsyncFFSynchronizer(w_rst, r_rst, domain = "rst_cdc")


Suggested change

AsyncFFSynchronizer(w_rst, r_rst, domain = "rst_cdc")

AsyncFFSynchronizer(w_rst, r_rst, domain="rst_cdc")

awygle · 2020-03-09T00:25:11Z

Oh oops, I didn't realize what Github was asking me - I made the requested formatting changes in the new commit.

whitequark · 2020-03-09T04:01:18Z

Oh oops, I didn't realize what Github was asking me - I made the requested formatting changes in the new commit.

I've never used it before. Wasn't sure if it was more polite to use that or just edit the PR; the latter seems like it'd be a problem if you force pushed, but is less busywork...

whitequark · 2020-03-09T04:19:27Z

nmigen/lib/fifo.py

+
+        # Async-set-sync-release synchronizer avoids CDC hazards
+        rst_cdc = m.submodules.rst_cdc = \
+            AsyncFFSynchronizer(w_rst, r_rst, domain="rst_cdc")


Shouldn't this be:

Suggested change

AsyncFFSynchronizer(w_rst, r_rst, domain="rst_cdc")

AsyncFFSynchronizer(w_rst, r_rst, domain=self._r_domain)

whitequark · 2020-03-09T04:35:55Z

nmigen/lib/fifo.py

@@ -317,7 +321,8 @@ def elaborate(self, platform):
        m.d.comb += produce_w_nxt.eq(produce_w_bin + do_write)
        m.d[self._w_domain] += produce_w_bin.eq(produce_w_nxt)

-        consume_r_bin = Signal(self._ctr_bits)
+        # Note: Both read-domain counters must be reset_less


Could you expand this to explain why the counters must be reset_less?

whitequark · 2020-03-09T04:37:31Z

nmigen/lib/fifo.py

@@ -366,6 +371,25 @@ def elaborate(self, platform):
            self.r_rdy.eq(~r_empty),
        ]

+        # Reset handling to maintain FIFO and CDC invariants in the presence of a write-domain
+        # reset. For further discussion, see https://github.com/nmigen/nmigen/issues/181.


Do you think you could summarize it here? You have all the relevant context right now, whereas that issue is fairly long and meandering. In cases like this I normally describe the problem, the possible solution, and the rationale for choosing the one I implemented, in a comment near the code. This way, anyone who reads the source will know exactly why it works the way it does, without having to parse an entire discussion.

whitequark · 2020-03-14T23:26:12Z

Thanks!

whitequark reviewed Mar 5, 2020

View reviewed changes

nmigen/lib/fifo.py Outdated Show resolved Hide resolved

awygle added 5 commits March 5, 2020 21:00

Proposed solution to ResetSynchronizer redundancy.

Unverified

No user is associated with the committer email.

Learn about vigilant mode

3e7f297

Corrections to reset synchronizer proposal.

e144aaf

Implement ReleaseSynchronizer and make ResetSynchronizer a wrapper.

9477e67

Also use ReleaseSynchronizer in AsyncFIFO.

Rename ReleaseSynchronizer to AsyncFFSynchronizer and add async_edge

e110dcf

Test AsyncFFSynchronizer

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

9f65ebc

awygle mentioned this pull request Mar 6, 2020

Create AsyncFFSynchronizer CDC Primitive #332

Merged

awygle added 8 commits March 5, 2020 22:23

Incorporate review feedback on AsyncFFSynchronizer changes.

b0182fd

Most prominently, modify platform support to produce AsyncFFSynchronizers with configurable edges rather than ResetSynchronizers.

Add test for negedge AsyncFFSynchronizer

562b9f1

Fix inversion of Altera synchronizer.

0ad704b

Also document async_edge.

Throw in Spartan platform on unsupported max input delay

b2f6ab1

Implement solution to AsyncFIFO resets, as in amaranth-lang#328

0b7ddb4

Mark both read domain counters as reset-less, then synchronize write domain reset into read domain, On write domain reset, force empty flag and set read counters == write counters.

Reset CDC domain is never reset-less.

8f9a860

Also tone down language describing creation of new domain.

Use new AsyncFFSynchronizer to synchronize resets

Verified

This commit was signed with the committer’s verified signature. The key has expired.

pstn Philipp

GPG key ID: 2258EE3B85DE1748
Expired

Verified
Learn about vigilant mode

c5abc2e

Add basic assert for FIFO reset property in tests

4733d75

awygle force-pushed the async_fifo_reset branch from e267ccf to 4733d75 Compare March 6, 2020 19:31

awygle changed the title ~~[WIP] Implement solution to AsyncFIFO resets, as in #328~~ Implement solution to AsyncFIFO resets, as in #328 Mar 6, 2020

whitequark reviewed Mar 8, 2020

View reviewed changes

awygle added 2 commits March 8, 2020 15:00

ClockDomain has clk_edge, not clock_edge

d81ae56

Merge branch 'master' into async_fifo_reset

63da364

whitequark requested changes Mar 8, 2020

View reviewed changes

Document AsyncFIFO reset behavior.

939ca68

whitequark reviewed Mar 9, 2020

View reviewed changes

whitequark requested changes Mar 9, 2020

View reviewed changes

Correct clock domain for reset synchronizer, improve documentation

8ed0654

whitequark approved these changes Mar 9, 2020

View reviewed changes

whitequark merged commit 4601dd0 into amaranth-lang:master Mar 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement solution to AsyncFIFO resets, as in #328 #329

Implement solution to AsyncFIFO resets, as in #328 #329

awygle commented Mar 5, 2020

codecov bot commented Mar 5, 2020 •

edited

Loading

whitequark Mar 5, 2020 •

edited

Loading

awygle Mar 5, 2020 •

edited

Loading

whitequark Mar 5, 2020

awygle Mar 5, 2020

whitequark Mar 5, 2020 •

edited

Loading

awygle Mar 5, 2020

whitequark Mar 5, 2020

awygle Mar 5, 2020

whitequark commented Mar 5, 2020

awygle commented Mar 5, 2020

whitequark commented Mar 5, 2020 •

edited

Loading

awygle commented Mar 5, 2020 •

edited

Loading

whitequark commented Mar 5, 2020

whitequark commented Mar 5, 2020 •

edited

Loading

awygle commented Mar 5, 2020 •

edited

Loading

whitequark commented Mar 5, 2020

awygle commented Mar 5, 2020

whitequark commented Mar 5, 2020

whitequark Mar 8, 2020

whitequark Mar 8, 2020 •

edited

Loading

awygle Mar 8, 2020

whitequark Mar 8, 2020

awygle Mar 9, 2020

whitequark left a comment

whitequark Mar 8, 2020

whitequark Mar 8, 2020

awygle commented Mar 9, 2020

whitequark commented Mar 9, 2020

whitequark Mar 9, 2020

whitequark Mar 9, 2020

whitequark Mar 9, 2020

whitequark commented Mar 14, 2020

	consume_r_gry = Signal(self._ctr_bits, reset_less = True)
	consume_r_gry = Signal(self._ctr_bits, reset_less=True)

	AsyncFFSynchronizer(w_rst, r_rst, domain = "rst_cdc")
	AsyncFFSynchronizer(w_rst, r_rst, domain="rst_cdc")

Implement solution to AsyncFIFO resets, as in #328 #329

Implement solution to AsyncFIFO resets, as in #328 #329

Conversation

awygle commented Mar 5, 2020

codecov bot commented Mar 5, 2020 • edited Loading

Codecov Report

whitequark Mar 5, 2020 • edited Loading

Choose a reason for hiding this comment

awygle Mar 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whitequark Mar 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whitequark commented Mar 5, 2020

awygle commented Mar 5, 2020

whitequark commented Mar 5, 2020 • edited Loading

awygle commented Mar 5, 2020 • edited Loading

whitequark commented Mar 5, 2020

whitequark commented Mar 5, 2020 • edited Loading

awygle commented Mar 5, 2020 • edited Loading

whitequark commented Mar 5, 2020

awygle commented Mar 5, 2020

whitequark commented Mar 5, 2020

Choose a reason for hiding this comment

whitequark Mar 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whitequark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awygle commented Mar 9, 2020

whitequark commented Mar 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whitequark commented Mar 14, 2020

codecov bot commented Mar 5, 2020 •

edited

Loading

whitequark Mar 5, 2020 •

edited

Loading

awygle Mar 5, 2020 •

edited

Loading

whitequark Mar 5, 2020 •

edited

Loading

whitequark commented Mar 5, 2020 •

edited

Loading

awygle commented Mar 5, 2020 •

edited

Loading

whitequark commented Mar 5, 2020 •

edited

Loading

awygle commented Mar 5, 2020 •

edited

Loading

whitequark Mar 8, 2020 •

edited

Loading