jbush001 / NyuziProcessor

GPGPU microprocessor architecture
Apache License 2.0
1.96k stars 348 forks source link

Invalid address selection for 64bit datapath in SDRAM controller #166

Closed tampler closed 6 years ago

tampler commented 6 years ago

SDRAM controller fails to correctly select address for a 64-bit datapath due to hardcoded 31-bit data width.

This is true for both write_address and read_address. Currently lines 489 and 520 in file sdram_controller.sv

Pls fix this. Thanks for sharing a soooo much awesome project

jbush001 commented 6 years ago

That's a hard coded 32 bit address (is that what you meant?) I'm not at my computer to test it, but it's not clear to me why this code would break a 64 bit datapath. The lsb of the bit slice should account for differing data widths. I believe the hard coded 32 bit address width is only a problem if you have more than 4gb of memory.

However there is another limitation that I suspect may be causing an issue for you: the width of the AXI data bus must match the SDRAM data width. This is because I didn't implement support for widening.

Can you clarify how your setup is configured (e.g SDRAM data width) and the incorrect behavior you are seeing?

tampler commented 6 years ago

Hi Jeff

When I run verilator compilation, it fails with the following error for the 64-bit data path

// slv.awaddr is in terms of bytes. Convert to # of transfers. write_address <= INTERNAL_ADDR_WIDTH'(slv.awaddr[31:$clog2(DATA_WIDTH / 8)]);

// slv.araddr is in terms of bytes. Convert to # of transfers. read_address <= INTERNAL_ADDR_WIDTH'(slv.araddr[31:$clog2(DATA_WIDTH / 8)]);

When I change 31 to DATA_WIDTH, which is 64bit for me, I get the following: %Error: ../sdram/sdram_controller.sv:487: Extracting 62 bits from only 8 bit number %Warning-SELRANGE: ../sdram/sdram_controller.sv:487: Selection index out of range: 64:3 outside 7:0

I think internal addr generation logic should be carefully updated. Thanks, Boris

jbush001 commented 6 years ago

Thanks for the update.

One problem is that araddr is only 8 bits wide. That would only allow you to access 256 bytes of memory. Check the definition of slv.araddr.

You should not set the high bit to DATA_WIDTH, as araddr is an address bus, not data (also, in that sort of construct, the most significant bit should be WIDTH - 1, as, in Verilog, bit numbering starts at zero and bit slice ranges are inclusive).

tampler commented 6 years ago

Thanks Jeff for pointing out. Now works fine

jbush001 commented 6 years ago

I cleaned up the hard coded width in 9d205e30c147dbf1736082bb33ba97ca2a843c17.