feat: add native axi_to_apb converter - #431
Conversation
5f66dd6 to
9e222cd
Compare
nikgiu
left a comment
There was a problem hiding this comment.
While reviewing this I tried bumping OutFifoDepth to 2 in the testbench to see how the module behaves with more buffering, and the simulation blew up: Questa hits a zero-delay combinational loop (Autofindloop at 350 ns, X propagation, then vsimk SIGSEGV / exit 211). The loop it reports runs through mem_gnt/mem_rvalid in this module and back through the axi_to_detailed_mem response path (stream_join → stream_fork_dynamic).
Digging into it, I think the root cause is that bank_sel (and therefore apb_sel_idx) is purely combinational. It's the lzc output over mem_req, and it drives paddr/psel/pwdata/mem_gnt/… directly. So mem_gnt/mem_rvalid depend combinationally on mem_req, and once OutFifoDepth > 1 the front-end closes that path into a combinational cycle. With OutFifoDepth == 1 it happens to be safe only because a full depth-1 bank FIFO blocks the next beat's enqueue, so mem_req can't change mid-transfer.
I also think the same root cause can break the APB protocol even without hitting the loop: since the selection isn't frozen for the duration of a transfer, a lower-index bank asserting between SETUP and ACCESS shifts bank_sel/apb_sel_idx mid-transfer → SETUP is driven to one slave and ACCESS (penable) to another, and paddr/pwdata change while a transaction is in flight.
My suggestion would be to latch the selection at the SETUP→ACCESS transition and drive the bus from the latched values during ACCESS. That keeps the APB signals stable by construction and breaks the mem_req → bank_sel → mem_gnt combinational path, regardless of OutFifoDepth
|
Thanks for the detailed review @nikgiu! I will dig into the issue when I find some time again. I will draft it again, until I have fixed the issue. |
|
You were right with your analysis @nikgiu! And I think your proposed fix is indeed the simplest option. I pushed some changes and also rebased to |
This PR adds a native
axi_to_apbconverter, which supports both downsizing datawidth and truncating the address.Current situation
It is already possible now to convert from AXI to APB but multiple modules are needed like 1)
axi_to_axi_lite ─▶ axi_lite_to_apbor 2)axi_to_reg ─▶ reg_to_apb. Both add intermediate protocol signals and generally make the code more verbose. Further, 2) uses a non-standard protocol and pulls inregister_interfaceas a dependency.Also, 1) has the disadvantage that it pulls in a
axi_to_axi_litewhich has relatively heavy burst splitter and other components, which is overkill to convert to APB. Further, neitheraxi_to_axi_litenoraxi_lite_to_apballow to natively downsize the datawidth, hence aaxi_lite_dw_converteris also needed.Exploration
I prototyped three ways to bridge AXI4+ATOP to APB4 and compared area (post-synth, gate equivalents):
axi_to_axi_lite→axi_lite_dw_converter→axi_lite_to_apbchainaxi_to_detailed_mem+ inline APB master (this PR)The AXI4-Lite chain was the heaviest: it drags in burst splitting, and a Lite data-width converter.
A fully native FSM was the smallest (~10% below the chosen option), but it was vibecoded so not proven logic, and code complexity is higher.
The chosen
axi_to_detailed_mem+ inline APB master sits in between: it reusesaxi_to_detailed_memthe AXI conversion. It is ~20% smaller than the Lite chain.Verification
The adapter is verified with a
tb_axi_to_apbtestbench, that checks the APB interface for protocol compliancy, as well as an AXI scoreboard to check for correct data. I also added different types of slaves (ideal, random) to simulate backpressure.