Add asserts to br_cdc_bit_pulse and scrub some obsolete TODOs#653
Add asserts to br_cdc_bit_pulse and scrub some obsolete TODOs#653
Conversation
mgottscho
changed the title
| //------------------------------------------ | ||
| // Relying on checks in br_cdc_bit_toggle | ||
|
|
||
| `BR_ASSERT_CR_INTG(src_pulse_high_then_low_a, src_pulse |=> !src_pulse, src_clk, src_rst) |
There was a problem hiding this comment.
I didn't add these in my PR because I think this is too stringent. If the source clock is much slower than the destination clock, it would be perfectly fine to have two back to back pulses.
There was a problem hiding this comment.
I disagree: the purpose of the module is to transfer pulses. If you're saying you don't have to pulse on the sender side then it seems ill-defined?
Also "two back to back pulses" means there must be a falling edge between, no?
| // Relying on checks in br_cdc_bit_toggle | ||
|
|
||
| `BR_ASSERT_CR_INTG(src_pulse_high_then_low_a, src_pulse |=> !src_pulse, src_clk, src_rst) | ||
| `BR_COVER_CR_INTG(back_to_back_src_pulse_c, src_pulse ##2 src_pulse, src_clk, src_rst) |
There was a problem hiding this comment.
Conversely, if the source clock is much faster than the destination clock, it may not be safe to have pulses two cycles apart on the source clock.
There was a problem hiding this comment.
I suppose if this is true then you should probably use a 1-bit CDC FIFO?
There was a problem hiding this comment.
If you can guarantee that the pulses are sufficiently spaced on the source side, you don't need to use a FIFO. I think if you want to add this cover, you should have a parameter like MinPulseSeparation.
There was a problem hiding this comment.
Right so I only see two possibilities. Unless I'm missing something.
- If you want to guarantee no data loss - use a CDC FIFO.
- If you can guarantee the source pulses are far enough apart that they can be synchronized to destination - use this.
If 2, then shouldn't we have the source side assert MinPulseSeparation unconditionally? If it fails, the user should switch to CDC FIFO for their use case?
There was a problem hiding this comment.
Yes, I think have an assert that the pulses are minimum number of cycles apart and have the minimum be configurable.
2 participants
No description provided.