You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This includes a number of fixes for {r,w}_level in AsyncFIFOBuffered.
The accounting for the output register used AsyncFFSynchronizer instead of FFSynchronizer. This is wrong as we want to synchronize both edges
Use the proper clock domains in the tests (not really a bug in the fifo logic obviously)
The FFSynchronizer should have a latency of 4 to match the latency of consume_w_bin / w_level. Two cycles for the FFSynchronizer from the read to the write domain of consume_w_gry, one for the GrayDecoder and one for the w_level calculation.
Fix the output register accounting. If the user asserts r_en even if r_rdy is low, we get -1 in the intermediate calculation which overflows to 1 as we have a unsigned Signal
Let me know if I should split this into multiple PRs or elaborate more on the fixes. The last two include additional testcases that fail without the fix. I did not include a testcase for the AsyncFFSynchronizer to FFSynchronizer change, as this is obviously wrong and would be hard to properly test without proper multi clock support in formal.
Let me know if I should split this into multiple PRs or elaborate more on the fixes.
Everything looks good. However, does anyone remember how we ended using AsyncFFSynchronizer there in the first place? I vaguely remember that the ability to use it there motivated a refactoring elsewhere, so it was probably not a simple typo, and I'd like to understand why it's there before we change it. It is also possible that I misremember.
To me it happens quite often that I end up using AsyncFFSyncronizer by accident because of the naming. My thought process is something like "I need to synchronize a signal that is async to $domain into $domain. Must be AsyncFFSyncronizer". I think it is very likely that it slipped in there similarly?
I misread this because of what @whitequark said about it motivating a refactoring. This is a different case than what I was thinking of (the reset AsyncFFSynchronizer, which is what that module was invented for). This seems correct to me, I am OK with merging it.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This includes a number of fixes for
{r,w}_levelin AsyncFIFOBuffered.AsyncFFSynchronizerinstead ofFFSynchronizer. This is wrong as we want to synchronize both edgesconsume_w_bin/w_level. Two cycles for theFFSynchronizerfrom the read to the write domain ofconsume_w_gry, one for theGrayDecoderand one for thew_levelcalculation.r_eneven ifr_rdyis low, we get-1in the intermediate calculation which overflows to1as we have a unsignedSignalLet me know if I should split this into multiple PRs or elaborate more on the fixes. The last two include additional testcases that fail without the fix. I did not include a testcase for the
AsyncFFSynchronizertoFFSynchronizerchange, as this is obviously wrong and would be hard to properly test without proper multi clock support in formal.