search

enjoy-digital / litedram

Small footprint and configurable DRAM core
Other
365 stars 115 forks source link

frontend/fifo.py: The fifo_depth was being set incorrectly. #282

Closed jersey99 closed 2 years ago

jersey99 commented 2 years ago

@enjoy-digital I believe the delayed write to FIFO test passes, because the write function waits for the FIFO sink to be valid (?) I didn't look into the test very deeply, but I can if you need me to.

enjoy-digital commented 2 years ago

Hi @jersey99,

sorry, I'm not sure to understand the fix. fifo_depth is the depth of the DRAM FIFO expressed in DRAM's words and with fifo_depth = int(depth/(port_data_width/8)) we are just converting the depth expressed by the User in bytes to DRAM words.

jersey99 commented 2 years ago

@enjoy-digital my understanding was that "depth" is the size of the FIFO at user's "data_width". And this depth needs to get translated to dram's write port width.

Say I want a FIFO with depth of 1024 @ 64 bit dw, and the DRAM port width is 512 bits

Then fifo_depth = 1024 / (512//64) .. right?

Sorry for the confusion :-/

jersey99 commented 2 years ago

Ok, after re-reading your comment, I may have just understood something: Is the dram fifo depth that is expected from the user in bytes (and not DRAM words)? If this is the case, please ignore this issue and close it.

However, this still would remain confusing, maybe add a comment somewhere in fifo.py that clarifies this?

enjoy-digital commented 2 years ago

@jersey99: The fifo depth is now indeed expected in bytes. I just added a description of the parameters with https://github.com/enjoy-digital/litedram/commit/bf9b3609d9904fb73664038bf88f0879a41b013c. Thanks for the feedback!