alexforencich / verilog-axis

Verilog AXI stream components for FPGA implementation
MIT License
710 stars 220 forks source link

FIFO reset #11

Closed LeChuck42 closed 3 years ago

LeChuck42 commented 4 years ago

I ran into an issue where the FIFO was reset and data became available the cycle after. The FIFO axis-slave hast tready asserted, but the internal write address is not correct in that case. One solution might be to deassert tready for the cycle after the reset. Alternatively the address calculation can provide the correct address immediately after reset. This implements the latter. Also I think the tready signal should be deasserted during reset. The core is definitely not going to be ready to receive data and hence should signal accordingly. I prepared test cases for these fixes and added them to the test bench.

LeChuck42 commented 4 years ago

Did you have time to look at this?

alexforencich commented 4 years ago

I'm not understanding where the bug is. Splitting resets like that is not supported; both ends of the AXI stream connection must be driven by the same reset signal. Adding a combinatorial connection between reset and tready is not necessary.

LeChuck42 commented 4 years ago

Hi Alex, Thanks for having a look. The problem is not different reset domains. At the moment, the FIFO signals tready in the first cycle after reset. If the connected master has tvalid asserted during this cycle, data should be transferred. However, due to the wrong address counters during this cycle, the data is lost. This can happen even if using the same reset signal. For the test I created a separate domain, so the reset behaviour can actually be tested. Hope this makes it clearer.

alexforencich commented 4 years ago

I think I can see what the issue is now. However, I explicitly did not reset that register so that there would be no issues with merging it into block RAM instances. Adding a reset could prevent proper inference of the block RAM. I would have to do quite a bit of reading and/or synthesis testing for various targets to figure out if adding that reset is acceptable. What I can look in to, however, is disabling the tready input for one cycle after reset is released.

alexforencich commented 4 years ago

Well, after doing some investigating, it's unclear why I added that reg in the first place. At the time I was working with the VCU108 board with a XCVU095 FPGA. However, the block RAM on that FPGA doesn't absorb those address registers, and removing them results in the same level of timing performance. For UltraRAMs, I think all inputs need to be registered, both address and data. I'm thinking I will remove that register completely and consider adding parametrizable input registers to aid in timing when using URAMs.

alexforencich commented 3 years ago

The registers in question have now been removed, so I'm considering this resolved.