-
Notifications
You must be signed in to change notification settings - Fork 55
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
WIP: Expand and document lib.cdc #40
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #40 +/- ##
==========================================
+ Coverage 80.70% 81.34% +0.64%
==========================================
Files 32 32
Lines 4976 5158 +182
Branches 1071 1105 +34
==========================================
+ Hits 4016 4196 +180
- Misses 825 829 +4
+ Partials 135 133 -2
Continue to review full report at Codecov.
|
Last commit is just to fix a small coverage blackspot in the existing code. I was having far too much fun with the graphs on Codecov |
BusSynchronizer is as per Migen except:
Again it needs formal checks, and tests are devalued by running them in one clock domain. Edit: updated the commit. Previously there was a clock-enabled odomain register driven by a clock-enabled idomain register. This is ok for CDC purposes, because the CDC-safe handshake controls the clock enables, but I've also put in a (shortened) Edit: squashed in another small change: making sure that sync_stages gets passed on to the MultiReg for the width=1 special case. |
502b138
to
c592d35
Compare
Add Edit: also this elastic buffer is unusual, usually there is some provision for slipping the pointers to account for long-term unbounded phase change. This would be more light-weight than an Force push due to finding small issues in earlier commits, e.g. gearbox increment didn't work for non-pow-2 chunk counts (which can't be tested in one clock domain, I just spotted it while reading the code). I've tried to keep it so that each commit can be separately checked out, tested and reviewed. If you'd rather I just push fixes, let me know :) Also, I've copied the |
I'll try to fix DomainRenamer to work on Modules as soon as possible!
I use the second style, with some exceptions like MultiReg. The reason is that the signals aren't always completely independent, for example you might want to pass a width and have a set of signals with that width, double that width, etc created for you. Also, constructors with a large number of arguments are unwieldy and bug-prone, especially if you change the constructor later. |
Ok, no sweat, I'll rebase + fix as and when :) |
Rebased on latest master and fixed up docstrings:
I also stared at the code for a while, only one minor fix which was something that really oughtn't be retimed, but didn't have the Domains are passed as strings, will fix this after the |
@Wren6991 Done! |
Thanks :) I've fixed up PulseSynchronizer and Gearbox so far. Unfortunately I'm away on a trip at the moment and don't have all the necessary tools set up on my work laptop, so it'll take me a while to get the fixes tested and pushed. For consistency I'm using "read" and "write" for default domain names (matching AsyncFIFO), but there is a not-totally-frivolous argument to use something like "i" and "o" instead:
Was just something that occurred to me when editing the files, it's not up to me. |
Now that I think more about it, I think it doesn't make sense to use There's a good argument that WDYT? |
So a flow could be:
If so, that feels cleaner, yeah! |
Yep!
Incredible. |
Awesome. I guess the next step for this PR is a lot more testing then. I did note #28, which I guess I could start to take a look at at some point, as this bumps against it. It would be good to learn about that part. Formal would be good too. For CDC it would also be cool to do some soak testing on real FPGAs, to catch genuine CDC bugs rather than just state machine bugs (albeit they're difficult to debug once found). Not sure if this is something you already had ideas on. |
The CDC primitives in oMigen are extremely well tested. I think we mainly need to make sure the nMigen ones don't stray from them, like it happened with AsyncFIFO... |
Ok that's a good point. Will read through the oMigen library again later today. For the most part it's pretty faithful but mistakes creep in! There are things in the old library that seemed odd though, e.g. for Gearbox with I'll definitely make sure the logic is all ok, and then maybe raise questions about things like the above. |
Sounds good! |
I did a side-by-side of oMigen vs current state of my code
BusSynchronizer datapath I drew a rough box diagram of oMigen My problem with this is that there is a 3-flop delay for both the request and the data. (1 flop in I've fixed the race by removing one stage from the datapath |
@Wren6991 Sorry, I completely missed your update, GitHub decided to not send me an email for it for some reason. |
if modulo == 2 ** len(signal): | ||
return signal + 1 | ||
else: | ||
return Mux(signal == modulo - 1, 0, signal + 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's already one in lib.fifo
and this is getting ridiculous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, it should be either inlined at my callsites, or preferably factored out. Wanted to keep the scope of this PR well-defined though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you think of a nicer place to put this? It's a common pattern (as is saturating increment/decrement)
Do you agree on my diagram of oMigen BusSynchronizer? I might have misunderstood e.g. the PulseSynchronizer logic. Haven't looked at it for a while now. |
I'll need to think more about it as I wasn't the one who wrote it. @sbourdeauducq ? |
But then, how do you preserve the MultiReg constraints between the anti-metastabilty registers? Sounds easier to add one register of delay into the request path. |
@sbourdeauducq You're 100% right -- it's possible to just falsepath/MCP the connection between write data buffer and the MultiReg, but it's cleaner if it can just rely on constraints on paths internal to the MultiReg. Added a patch which restores the data path to its original length, and lengthens the request path to compensate. |
…ath, so that data path benefits from MCP/falsepath constraints from MultiReg.
Fixed merge conflict and made classes derive from Elaboratable (sorry for force-push) |
Thanks, I'll take another look soon. |
Updated naming conventions according to #97. I've also updated the only use of |
That's fine, I have a lot of other work here, so you're not really blocking anything. |
I saw this in #113
And have also noted #87. Wanted to mention that falsepath is not necessarily safe for all of these primitives: in particular, BusSynchronizer is unsafe if there is too much skew between the request path and the datapath, and false paths can accumulate huge routing delays when FPGA utilisation is high. Since we want the widest possible toolchain support, one option is a maxdelay constraint of capture period minus setup. This isn't perfectly safe (clock skew), and does make closure harder, but might be worth considering. |
That seems very troublesome, and certainly something we want to loudly warn about. Would you be able to prepare a detailed evaluation of all of our CDC primitives with this in mind?
Are there any ways to work around this?
This seems like a dramatic increase in complexity of platform code. It's an option, but maybe we can avoid it completely... |
That is an interesting handshake, but seems to overlap quite a lot with BusSynchronizer. I have had the occasional shower thought that BusSynchronizer should have an optional valid/ready handshake in the launching domain (and perhaps a valid pulse in the capturing domain), which would give you something similar to this BlindTransfer. I'm also happy to just port BlindTransfer wholesale! |
I'm OK with merging them, but |
Moving to 0.2, which has the theme of "standard library improvements" anyway. |
|
||
MultiReg is reset by the ``odomain`` reset only. | ||
MultiReg is reset by the ``cd_o`` reset only. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MultiReg
is renamed to FFSynchronizer
in 8deb13c.
Very much a work in progress, but I wanted to open early to get feedback on the approach/direction. I'm porting/rewriting some of the missing modules from migen.genlib.cdc.
Currently in this patch set:
Known issues:
I definitely still intend to port:
And we probably ought to do something about GrayCounter at some point, but I think it's less important than the others.
As a general style question, there seem to be two ways of doing module wiring in
nmigen.lib
at the moment. One is to pass external signals into__init__
, which gives you more of a Verilog-style instantiation. The other is to create "port" signals in__init__
, which the parent then wires up during elaboration. I've used the second style because it seems a bit cleaner, and doesn't require creating extraneous signals like in Verilog, but not sure if this is the right thing to do?