Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 17 commits into from Mar 14, 2020

Conversation

awygle
Copy link
Collaborator

@awygle awygle commented Mar 5, 2020

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
Copy link

codecov bot commented Mar 5, 2020

Codecov Report

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

Impacted file tree graph

@@            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.

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)
Copy link
Member

@whitequark whitequark Mar 5, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@awygle awygle Mar 5, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@whitequark whitequark Mar 5, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@whitequark
Copy link
Member

(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
Copy link
Collaborator Author

awygle commented Mar 5, 2020

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
Copy link
Member

whitequark commented Mar 5, 2020

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
Copy link
Collaborator Author

awygle commented Mar 5, 2020

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
Copy link
Member

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
Copy link
Member

whitequark commented Mar 5, 2020

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
Copy link
Collaborator Author

awygle commented Mar 5, 2020

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
Copy link
Member

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
Copy link
Collaborator Author

awygle commented Mar 5, 2020

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

@whitequark
Copy link
Member

Sure.

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.
@awygle awygle changed the title [WIP] Implement solution to AsyncFIFO resets, as in #328 Implement solution to AsyncFIFO resets, as in #328 Mar 6, 2020
nmigen/hdl/cd.py Outdated
@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

The actual parameter is called clk_edge.

Copy link
Member

@whitequark whitequark Mar 8, 2020

Choose a reason for hiding this comment

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

(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.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

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

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

@awygle
Copy link
Collaborator Author

awygle commented Mar 9, 2020

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

@whitequark
Copy link
Member

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...


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

Choose a reason for hiding this comment

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

Shouldn't this be:

Suggested change
AsyncFFSynchronizer(w_rst, r_rst, domain="rst_cdc")
AsyncFFSynchronizer(w_rst, r_rst, domain=self._r_domain)

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

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 whitequark merged commit 4601dd0 into amaranth-lang:master Mar 14, 2020
@whitequark
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants