Fix non-synchronized path in cdc_2phase_clearable #243
Conversation
This file contains hidden or 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
adapted from cdc_2phase_tb
Previously this used some old (development?) version of the DUT module. Now it uses the same implementation as is currently used. Todo: This should test the module, not a copy of the module.
The previous implementation creates a non-synchronized path across the CDC, making it significantly more vulnerable to metastability. This does not seem to be necessary for DECOUPLED to work in the cdc_2phase_clearable (cdc_reset_ctrlr). The important thing seems to be when on the source side the ready signal is asserted, not how the destination side synchronizes data.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
As it stands, the
cdc_2phase_clearablemodule has a path going from the source clock domain into the destination clock domain directly, without a 2-flop synchronizer.The path in question is in the reset/clear logic implemented in
cdc_reset_ctrlr.Specifically
receiver_next_phaseis clocked by the source domain (side A incdc_reset_ctrlr) and then goes into the other side (side B) where it goes intoi_state_transition_cdc_dst, acdc_4phase_dstwith theDECOUPLEDparameter set to zero.Setting this parameter to zero effectively bypasses the spill-register in there allowing the same asynchronous signal to leave and go into the destination clock domain, where it shows up as
receiver_next_phaseused to create the isolate and clear signals on the destination side.I strongly doubt this is intended behavior and would like to fix this issue.
Further, this shows to me that especially the clock domain crossings need working testbenches that run in a CI to make sure they work as intended.
As part of this PR, I also started to fix the testbenches which in the case of
cdc_2phase_clearabledid not even test the module but presumably some old development version instead. This happened because the module is essentially copied in the testbench instead of actually testing the module itself.