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

Add r_level and w_level to AsyncFIFO #328

Closed
awygle opened this issue Mar 5, 2020 · 7 comments
Closed

Add r_level and w_level to AsyncFIFO #328

awygle opened this issue Mar 5, 2020 · 7 comments
Labels
Milestone

Comments

@awygle
Copy link
Contributor

awygle commented Mar 5, 2020

It's often useful to have a "high watermark/low watermark" signal from a FIFO, particularly when processing any kind of fixed-length frames. This could be implemented as a wrapper around SyncFIFO pretty easily but probably wants to be more deeply integrated with AsyncFIFO since there's already a clock-domain-crossing counter to use (which is not currently exposed to consumers).

@whitequark
Copy link
Member

I'm not sure how an async FIFO with water marks works, CDC-wise. Do you have any references?

@awygle
Copy link
Contributor Author

awygle commented Mar 5, 2020

As section 5.6 of http://www.sunburst-design.com/papers/CummingsSNUG2002SJ_FIFO1.pdf says, you compare against (ptr+N) instead of (ptr) as you would for empty/full. That section appears to be discussing FIFO style 1, but the same technique should be adaptable to FIFO style 2 with minor modifications.

@whitequark
Copy link
Member

That means we have to Gray decode the resynchronized counters, right? Then it seems like we should rather have r_level and w_level for consistency with SyncFIFO, and the consumer of the FIFO interface can then write a comparator with HWM/LWM. We can even make it a part of the general FIFO interface, with the SyncFIFO case aliasing level as both.

@awygle
Copy link
Contributor Author

awygle commented Mar 5, 2020

That makes sense, then we can always create a HWM/LWM wrapper as a utility module if we want to and it can work with any FIFOInterface instance.

@awygle awygle changed the title Add (optional) support for watermarks in FIFOs Add r_level and w_level to AsyncFIFO Mar 5, 2020
@whitequark
Copy link
Member

In particular I think having a level is strictly more useful than watermarks (even configurable watermarks), if only for debugging. It's true that comparators are cheaper on architectures which have lookahead carry primitives, but I would say that it's the toolchain's job to simplify subtract-then-compare, since that's a fairly simple local transformation.

@awygle
Copy link
Contributor Author

awygle commented Mar 5, 2020

I'll be honest, I missed level on SyncFIFO when I was looking at the existing FIFO code 😅

awygle added a commit to awygle/nmigen that referenced this issue 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.
awygle added a commit to awygle/nmigen that referenced this issue Mar 6, 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.
@whitequark whitequark added this to the 0.3 milestone Aug 27, 2020
@whitequark
Copy link
Member

Fixed in #472, #512, #513, #524.

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

No branches or pull requests

2 participants