open-logic / open-logic

Open Logic HDL Standard Library
Other
330 stars 23 forks source link

Range issue in olo_axi_master_simple for narrow AxiAddrWidth_g #87

Open danielkraak opened 5 days ago

danielkraak commented 5 days ago

https://github.com/open-logic/open-logic/blob/8f50f5477dff628ac690eb5e8ec9915bdd8a1bec/src/axi/vhdl/olo_axi_master_simple.vhd#L312

I am trying to use the simple AXI4 master. Unfortunately, on line 312 of olo_axi_master_simple.vhd, I am encountering a Fatal error when simulating in Questasim:

Fatal: (vsim-3420) Array lengths do not match. Left is 12 (11 downto 0). Right is 18 (17 downto 0).

The line giving this error looks as follows:

v.WrAddr := r.WrAddr + 2*UnusedAddrBits_c r.WrTfBeats;

I am connecting it to a simple AXI4 slave with a 12 bits address space and 8 bits wide data. r.WrTfBeats is 9 bits wide in my case. As far as I can tell, 2**UnusedAddrBits_c is also converted to a 9 bits wide unsigned before multiplication resulting in an 18 bits wide unsigned.

Shouldn't this line of code use a resize()?

obruendl commented 5 days ago

I'll check it out. Can you provide me the complete configuration (all generics)?

You said you encounter the error in Questasim. I assume you then have a testbench you try to run when encountering the issue. Could you provide this one?

As workaround until I could fix it, would it be an option for you to use the master with AxiDataWidth_g=16? Normally interconnects will add width conversions where required anyways.

What version of Open Logic are you using?

I tried reproducing the issue by adding a DataWidth_g=8 configuration to the Open Logic regression tests but this worked perfectly fine (in GHDL as well as in Modelsim). See below screenshot.

image

You can find the state where I ran the 8-bit simulation with on the branch debug/olo_axi_master_simple_8bit. Feel free to either provide your exact test-case that fails compilation or to edit mine. As long as I can reproduce the problem I will be able to follow-up on it.

danielkraak commented 4 days ago

I think the issue is not in the data width, but in the address width. These are my used generics for the simple axi master:

AxiAddrWidth_g => 12,
AxiDataWidth_g => 8,
AxiMaxBeats_g => 256,
AxiMaxOpenTransactions_g  => 1,
UserTransactionSizeBits_g => 10,
DataFifoDepth_g => 1024,
ImplRead_g => true,
ImplWrite_g => true,
RamBehavior_g => "RBW"

I wrote a simple AXI4 RAM (a slave that utilizes block ram). In my testbench I hook up open-logic's axi master to it to check if I can write and read to/from it. Unfortunately, I cannot share this testbench easily as it includes some copyrighted files. I am looking to temporarily use the AXI4 RAM before switching to external memory (and using a memory controller with an AXI4 interface).

I think the issue is as follows: my AXI4 RAM is rather small (only 12 bits address space, so 4096 bytes). This is pretty uncommon for AXI4 (but I suppose it should still work). Only thing I can imagine is that your test cases have not checked such small address spaces. What seems to happen is 2*UnusedAddrBits_c r.WrTfBeats resolves to a 18 bit wide vector, but my AXI4 address space is only 12 bits, resulting in the FATAL error. If my AXI4 address space would be bigger (which would be a more common scenario), this would not give an error.

I think line 312 should become something like this:

v.WrAddr := r.WrAddr + resize(2*UnusedAddrBits_c r.WrTfBeats, CmdWr_Addr'length);

Note that line 499 would also require a similar fix for reads. Applying above change fixed my testbench. :)

obruendl commented 4 days ago

Confirmed. I could reproducde it.

It's interesting that 2*0=1 (when constructed from all constants) gets upsized to 9 bits. I would expect `2 some_9_bit_vector` to consume 10 bits but not 18.

However, your workaround does the job.

I fixed the bug on the develop branch. Please use this one - the fix will be included in the next release but because it is a rather special case (very narrow address width) I don't push out an emergency release.

OK?

obruendl commented 4 days ago

Summary

olo_axi_master_simple failed for cases with very low AxiAddrWidth_g (lower than 2 x log2(AxiMaxBeats_g+1)). Other configurations are not affected.

Last affected version: 3.0.0

Suggested workaround: Use develop branch until next release.

State: The bug is fixed on develop and the fix will be contained in the next release.

danielkraak commented 3 days ago

I can confirm the fix works for me.

It's interesting that 2*0=1 (when constructed from all constants) gets upsized to 9 bits. I would expect 2 some_9_bit_vector to consume 10 bits but not 18.

When multiplying an integer with a vector, the integer is afaik first casted to a vector of the same length (9 bits in this case). Multiplying two vectors of 9 bits gives an 18 bit result. Multiplication is something to be cautious with in VHDL. :)

Thanks for the quick fix! Eager to slowly start trying some more components and replace my Verilog implementations. :)

obruendl commented 2 days ago

@danielkraak , I keep it open until the release is out so the issue is visible in in the doc. Open issues for any components are automatically added to the doc - and in the sense of open communication they should show up there until the fix is released.