alexforencich / verilog-axis

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

axis_fifo.v KEEP_ENABLE parameter changes ADDR_WIDTH #26

Closed erikliljey closed 10 months ago

erikliljey commented 10 months ago

I used the axis_fifo module in a design and it wasn't working due to some issues with the depth of the memory. After some digging I found that the KEEP_ENABLE parameter causes the FIFO depth to shrink based on how many tkeep bits there are.

parameter ADDR_WIDTH = (KEEP_ENABLE && KEEP_WIDTH > 1) ? $clog2(DEPTH/KEEP_WIDTH) : $clog2(DEPTH);

Is there a reason the tkeep is impacting the depth of the FIFO? If I force the code to always use $clogs(DEPTH) then it works great. If there's a reason it was done this way I'd like to understand it in order to use the IP as-is instead of modifying it in my project.

The version I'm using isn't the latest, but I see the same line in the current version.

alexforencich commented 10 months ago

The idea is that DEPTH is the number of bytes in the FIFO, where the data bus is divided into len(tkeep) bytes. If tkeep is disabled, then the byte size is assumed to be equal to the data width. I used to have ADDR_WIDTH, but this doesn't work nicely for the FIFO + width converter wrapper since the data widths can be different. I guess maybe what I could do is add an additional parameter where you could force a certain byte size even though tkeep is not used.

erikliljey commented 10 months ago

I see. So I just need to adjust my depth to take into account the width. No need to the change the IP. I just didn't understand how to use it properly. I made the change and it works. Thanks for the feedback!