-
Notifications
You must be signed in to change notification settings - Fork 58
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
Please consider an optional reset on MultiReg #37
Comments
On a slightly-related note, are there any plans to flesh out the CDC library with e.g. pulse-to-pulse, Gray code, or ready-ack + multibit? Batteries included! |
Weak yes. I will need to dig in and understand the context here much better. You can help this by providnig compelling real-world examples and extensive documentation. I.e. teach me the use cases.
Yes, all this was in Migen and it will get ported. Gray counters are already in. |
Okay I will try! I don't guarantee any of the below code works, but it's conceptually sound Pulse to pulse is used to signal the occurrence of some event from one domain to another. The idea is that each 1-clock-wide pulse on the input results in a 1-clock wide pulse on the output, even when the clocks are different speeds. For slow->fast you can do 100% duty cycle at the input, otherwise you require some level of sparseness at the input to avoid dropping pulses. One use for this is a credit-based DREQ on e.g. a UART with a DMA interface. If you signal a single pulse for each transfer you require, you can guarantee that a DMA, in the fast system clock domain, will never overflow your FIFO, in the slow peripheral clock domain. OTOH if you use the normal level-sensitive DREQ scheme, you need to deassert the DREQ before completely filling your FIFO, to account for the transfers which may be in-flight in the bus CDC between the DMA and the UART. This wastes FIFO space. (You could alternatively use an async FIFO, but on most SoCs it's normal to run the entire peripheral bus at a much lower frequency than your main interconnect, to save power etc) In Verilog it looks something like this:
Without a reset on the MultiReg, you may randomly get a pulse on If you need full-duty-cycle at the input for fast-slow, and you need backpressure, then one tool is a credit FIFO, which is an async FIFO with no data path. It's used kind of like a semaphore which you Another trick which requires reset on a MultiReg is a req-ack handshake. This allows a multi-bit signal to cross domains safely, but with some delay and infrequent updates. For example, suppose you have an RTC which is clocked off some fixed slow clock, and you want to sample a raw counter value from a processor whose clock varies.
The idea with this one is that you avoid metastability and bit skew by guaranteeing that An async FIFO is unsuitable here: you are interested in sampling a single value of the signal now-ish, not reading in some buffered sequence. Note that this is robust wrt the ordering of reset removal:
However, if there is no reset on the MultiReg, there may be a period after reset removal where the synchronisers contain garbage, and the Sorry for the verbosity :( this (PDF) is a fairly concise document that gives a bit of a who's who of clock crossing tricks, and has diagrams. |
Okay, noted, I will go check out the Migen standard library and Would working on the libraries be helpful, or are people mostly focusing on the core language at the moment? I would be happy to put time into implementing missing features from Migen, or verifying/reviewing the parts that currently exist in nMigen but are unverified. I saw that nMigen has some first-class FV support, which is one reason for my interest! |
Reading back I think I misunderstood what you asked for, and that you already knew 99% of what I said. That's pretty bad, sorry. However, even if e.g. the Migen PulseSynchroniser was FPGA-proven, there are ways for this kind of issue to hide:
None of these are universally true. Also note page 17 on the Cummings paper referenced in |
Working on the libraries is definitely helpful! There's a lot of stuff that needs to be ported from Migen, documentation improved, etc. I will likely have opinions on how it should be implemented, but help is very welcome here!
That's OK. I will now try to summarize what you said. "MultiReg is used for safely sampling signals across clock domains. When the sampled value is used within a sequence of events triggered by something else (which does not happen shortly after a reset), this is OK. But when the sampled value is used itself to trigger other events, this can result in misfires." Is this correct? |
Okay cool! I will start digging into Migen a little bit more. There are plenty of other things which are ubiquitous but kind of tricky, like clock control. Opinionated design is good!
Yes you summed that up much better than I could. MultiReg is dangerous if the output value is observed immediately after reset. |
Thanks. Could you send a PR for reset on MultiReg, together with an explanation of the cases where it would be useful, perhaps excerpted from the above? I would appreciate it. In general, explaining how to use nMigen primitives soundly is very important, and all documentation work on that front is greatly appreciated. You are also good at following existing style, so I feel comfortable knowing that the patches you submit would be quick to review, which I'm very grateful for, given the amount of workload and backlog I have. |
Sorry, I spaced out for an entire week there. Brain has now rebooted. Will add a PR! |
I was browsing the code to learn the standard library, and bumped into this in cdc.py:
I understand there are caveats with resets on CDC capture flops, but sometimes you need it. For example, a pulse-to-pulse synchroniser (or toggle synchroniser) contains a toggling NRZ signal crossing the domains, and the capture flops need a capture-domain reset to avoid spurious toggling observed by the logic when deasserting reset.
platform.get_multi_reg(self)
may contain considerable magic for some synthesis flows, and should only need writing once; ideally, something like a pulse-to-pulse synchroniser should be implementable using the existing MultiReg.A ligh-touch option could be to just add a
reset_less=True
toMultiReg.__init__
, and pass this down.Happy to be argued with, told to go implement it, or just told I'm wrong :)
The text was updated successfully, but these errors were encountered: