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

Reconsidering AsyncFIFO and resets #180

Open
whitequark opened this issue Aug 22, 2019 · 7 comments
Open

Reconsidering AsyncFIFO and resets #180

whitequark opened this issue Aug 22, 2019 · 7 comments

Comments

@whitequark
Copy link
Contributor

Both ports of a SyncFIFO belong to the same control set, which means that the entire FIFO is always reset at once.

However, the ports of AsyncFIFO belong to different control sets, which means that, if either if the control sets includes a reset, only half of the FIFO will get reset; and as a result, stale data will be read.

In some cases, this problem may be alleviated by using a reset synchronizer: one port is chosen as the one that resets the entire FIFO, its reset signal is brought into the other clock domain, and used to reset either the entire clock domain, or only the port state. However, if the duration of the reset pulse at the leading port is shorter than the clock period of the following port, the following port will not be reset properly, and stale data will be read. In particular this happens if the clock to one of the ports is gated completely.

In some cases, this problem may be further alleviated by using asynchronous reset (of course, with synchronous release) for the entire clock domain of the following port. However, this carries the restriction that the entire domain has to be reset: since the assertion of the reset is not synchronized to the clock of the following port, it incurs metastability. Further, many FPGA elements will not be inferred from logic that is asynchronously reset, notably BRAMs and DSP blocks, severely penalizing this design.


It seems that there is no easy solution to the issue of resetting AsyncFIFO reliably in all circumstances, making it a completely safe design element that never spuriously produces stale data. I would suggest the following:

  1. AsyncFIFO should not have a reset at all, that is, it should not have a signal whose stated purpose is to ensure that one or both pointers becomes zero.
  2. The read side only of AsyncFIFO should have a synchronous clear signal that sets the read pointer to the same value as the write pointer, therefore flushing the buffer of any stale data that might be stuck there. Because AsyncFIFO reads are synchronous, this clear signal works regardless of how the read side is clocked. Because this clear signal operates entirely in the read domain, it works correctly regardless of how the write domain is clocked--the write clock could even be stopped completely.

This solves everything described above:

  1. The two ports of AsyncFIFO are never reset, therefore neither of them is reset while the other is not. Therefore stale data is never read as a result of incorrectly sequenced reset.
  2. When the leading clock domain resets the following clock domain to bring it into a well-defined state (in such case the FIFO could be full of garbage data), it is necessary to assert, simultaneously, all three of: following clock domain reset, clear signal of any FIFO read port in the leading clock domain (through an explicit connection), and clear signal of any FIFO read port in the following clock domain (the clear signal should be implicitly ORed with ResetSignal("read") in the FIFO). Therefore any garbage data is eliminated from FIFOs oriented in both directions before the reset is deasserted.

One possible logic bug with this scheme would be a case where the CDC FF chain inside AsyncFIFO is for some reason longer than the FF chain inside the reset synchronizer used to reset the following domain. Ideally, the CDC checker (#4) should flag any such cases.

@sbourdeauducq
Copy link
Member

sbourdeauducq commented Aug 22, 2019

Sounds good.
IIRC the only state that needs to be in the write domain is one pointer value modulo the size of the register, so we can never get that into an unrecoverable illegal state (e.g. because of clock glitches) and this should not need a reset.
The situation may become more complicated if we add support for watermarks ("almost empty/full" signals) - I'm not sure how those would work.

@whitequark
Copy link
Contributor Author

whitequark commented Aug 22, 2019

AsyncFIFO currently doesn't support non-power-of-2 memories anyway so it has no illegal states. And I'm not sure if it's possible to support such memories while still using a Gray counter.

@jordens
Copy link
Member

jordens commented Aug 22, 2019

Good.
How would the write half discard the data in the FIFO though (with minimal side effects)? CDC ping-pong with a "clear-request plus acknowledge" to the read side?

@whitequark
Copy link
Contributor Author

How would the write half discard the data in the FIFO though (with minimal side effects)? CDC ping-pong with a "clear-request plus acknowledge" to the read side?

I've thought a bit about this, and the exact same approach would work, since the FIFO is already symmetric: set the write pointer to the read pointer. In fact, it's possible to simplify this further: instead of an explicit clear signal, use the ResetSignal() of every port like a normal synchronous input to set the counter of that port to the value of the counter of the other port.

@programmerjake
Copy link
Contributor

one problem you may run into is that resetting changes multiple bits simultaneously in the grey-counter read and write pointers, so relying on just flip-flops to synchronize the signals won't work anymore, so there will need to be some sort of reset synchronization mechanism anyway

@whitequark
Copy link
Contributor Author

@programmerjake You are (unfortunately, for this approach) completely correct. I will need to find a different one.

@sbourdeauducq
Copy link
Member

sbourdeauducq commented Aug 22, 2019

For the clear signal on the read side, all this will cause is glitches on the writable signal that perhaps don't matter (I wonder if they could actually cause missing entries in FIFO data that need to be contiguous?). If we do want to suppress them, we can have a handshake mechanism:

  • read section sends request to block the write section (e.g. assert a signal through a FF synchronizer)
  • read section confirms that write section is blocked (by return of that signal through another FF synchronizer). Blocking forces writable to 0.
  • read section sets read pointer to write pointer
  • read section unblocks write section and waits for confirmation (to avoid a bug where you read the confirmation of a previous clear in step 2)

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

No branches or pull requests

4 participants