Added tweaks to the FIFO to prevent assertion errors. #766
Conversation
…r fifo_depth_c of 1. Added reset condition around async_read to prevent X assertion until read pointer and fifo_mem is reset
|
Thanks for your modifications! I'll have a look at this. |
stnolting
added
HW
| @@ -157,7 +157,13 @@ begin | |||
| begin | |||
| if rising_edge(clk_i) then | |||
| if (we = '1') then | |||
| -- Added fifo_depth check to the read out path to preven a NULL assertion for | |||
| -- fifo_depth_c of 1. | |||
| if (fifo_depth_c > 1) then | |||
There was a problem hiding this comment.
Looks good! But shouldn't we add this to the FULL_RESET write process above as well?
There was a problem hiding this comment.
Doh. Good catch, yes we should. This would be a copy paste error on my side as I was forward writing from an older version of the code base.
Want me to fix it?
| rdata_o <= fifo_mem(to_integer(unsigned(r_pnt(r_pnt'left-1 downto 0)))); | ||
| -- Added a reset driven bypass to ensure that rdata_o is defined during reset | ||
| -- This prevents X generation while fifo_mem and r_pnt are still being initialized. | ||
| rdata_o <= fifo_mem(to_integer(unsigned(r_pnt(r_pnt'left-1 downto 0)))) when (rstn_i = '1') else (others => '0'); |
There was a problem hiding this comment.
I think we should fifo_depth_c as well:
rdata_o <= fifo_mem(to_integer(unsigned(r_pnt(r_pnt'left-1 downto 0)))) when (fifo_depth_c > 1) else fifo_mem(0);There was a problem hiding this comment.
I do not like using rstn_i as direct input here 😅 The reset should be a global net that is only driving the dedicated reset inputs of FFs. Furthermore, the reset is asynchronous, so we might get trouble with timing analysis here.
The when-else statement adds another layer of logic without "being relevant for the function" so I would like to avoid this to keep the hardware footprint (and the critical path) at a minimum. As far as I understand the X-issue here comes from fifo_mem being uninitialized at the beginning, right?
There was a problem hiding this comment.
I agree on the rstn_i and the whole logic thing. The challenge that remains is the r_pnt ending up undefined until the simulation event handler catches the rstn_i event that sets r_pnt.
Sadly no, the issue arises from r_pnt being undefined - I believe - but I could be wrong. Hence why my "hack" was basically to just drive rdata_o to '0' while rstn_o is active. A mux basically on the output of the register array. Its not pretty but also not unheard of.
The change will work for fifo_depth_c <= 1. But for fifo_depth > 1, like in the pre-fetch buffer, we are back to the original problem. Assuming I'm right, which should be tested.
Let me take action here and debug this one some more.
There was a problem hiding this comment.
But how can we fix the X-issue at this point?
On Intel FPGAs the entire "asynchronous" FIFO read logic gets absorbed into the primitive memory bits. I am not sure how this is done (need to investigate on that). Maybe the logic to set the output to zero on reset could also be absorbed as most BRAM have output registers with a dedicated reset signal. I need to dig into the technology data sheets...
Anyway, with FULL_RESET => true, this X-issue should be gone, right?
|
I really like the proposals you made in this PR. Should I add those to one of my next "minor edits" PRs or are you working on a revised PR? 😉 |
|
Just added a new PR, couldn't reopen this one as to much time had passed :( |
But not in this repository, right? Maybe in a fork?! 🤔 |
NULL assertion fix in FIFO, was PR stnolting#766
|
Indeed in my fork. PR #778 submitted. |
The FIFO generates two types of assertion errors:
The subtle fixes will prevent X generation for async read by forcing a mux on the read output during reset low periods.
The NULL fix is simple detecting fifo_depth_c as being 1 and setting the fifo_mem index accordingly. The read/write point math ends up as a 0 downto 0 index which results in a NULL assertion warning in xcelium.