-
Notifications
You must be signed in to change notification settings - Fork 332
Add multicast support to XBAR #398
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
To be improved: - 50% multicast AW throughput - Potential for combinational loops in mux
Multiple outstanding multicast transactions are allowed only if the slaves selected by these transactions are the same.
--------- Co-authored-by: Luca Colagrande <[email protected]> Co-authored-by: Raphael <[email protected]>
micprog
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is quite cumbersome to review, as most of the 5k line changes are code movement changes, and the code changes are not directly visible in the interface here. For some files, I added the corresponding git commands I used to check the changes. It might make sense to move some of these changes not related to multicast to dedicated PRs, simplifying the review and traceability of changes.
A further complication in reviewing this PR is the non-standard feature, as multicast is not supported in any available AXI specification. Please add documentation in the corresponding README files for what multicast is supported, how it works, and how it is intended to be used.
Also, given that this is not part of the spec, it might make sense to put this in a separate project, keeping this axi repository to what is available in the spec, but we can have that discussion offline.
Primarily, I was under the impression that this PR does not change the existing functionality when multicast is disabled or the existing modules are used, but this does not seem to be the case!
Along with comments placed to help understanding, there are a few other items that need to be addressed. Please see below.
| axi_xbar) | ||
| for NumMst in 1 6; do | ||
| for NumSlv in 1 8; do | ||
| for Atop in 0 1; do | ||
| for Exclusive in 0 1; do | ||
| for UniqueIds in 0 1; do | ||
| call_vsim tb_axi_xbar -gTbNumMasters=$NumMst -gTbNumSlaves=$NumSlv \ | ||
| -gTbEnAtop=$Atop -gTbEnExcl=$Exclusive \ | ||
| -gTbUniqueIds=$UniqueIds | ||
| done | ||
| done | ||
| done | ||
| done | ||
| done | ||
| ;; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the AXI xbar test no longer valid?
|
|
||
| dependencies: | ||
| common_cells: { git: "https://github.com/pulp-platform/common_cells.git", version: 1.37.0 } | ||
| common_cells: { git: "https://github.com/pulp-platform/common_cells.git", rev: "multicast-xbar" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update this to use a proper version, not a branch name.
| - src/axi_cdc_src.sv | ||
| - src/axi_cut.sv | ||
| - src/axi_delayer.sv | ||
| - src/axi_demux_simple.sv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
axi_demux_simple is no longer considered Level 2, as it now depends on the mcast module. Please update this ordering (also for other updated files).
| // outstanding transactions | ||
| output logic any_outstanding_trx_o |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just so I understand correctly, the only adjustment you made to this module is adding this port, otherwise it is copied from axi_demux_simple.sv.
git diff master:src/axi_demux_simple.sv multicast:src/axi_demux_id_counters.sv
| logic [NoMstPorts-1:0] aw_select_mask; | ||
|
|
||
| assign aw_select_mask = 1'b1 << slv_aw_select_i; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main changes here seem to be the deletion of all code to reference the new demux module, and the translation from select to onehot masking.
git diff master:src/axi_demux_simple.sv multicast:src/axi_demux_simple.sv
| ); | ||
|
|
||
| // Account for additional error slave | ||
| localparam int unsigned NoMstPortsExt = NoMstPorts + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is an error slave needed when a default port is enabled?
| spill_register #( | ||
| .T ( aw_chan_t ), | ||
| .Bypass ( ~SpillAw ) | ||
| ) i_aw_spill_reg ( | ||
| .clk_i, | ||
| .rst_ni, | ||
| .valid_i ( slv_req_i.aw_valid ), | ||
| .ready_o ( slv_resp_o.aw_ready ), | ||
| .data_i ( slv_req_i.aw ), | ||
| .valid_o ( slv_req_cut.aw_valid ), | ||
| .ready_i ( slv_resp_cut.aw_ready ), | ||
| .data_o ( slv_req_cut.aw ) | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module has a different timing behavior than the existing implementation, as outlined in the PR description. While this simplifies some things and reduces the number of registers, the address decoding can add quite a bit of timing overhead and can now no longer be pipelined out of the demux path when using any xbar configuration. Do you have an analysis of this timing impact before we integrate this change?
| dec_aw_addr = '0; | ||
| dec_aw_mask = '0; | ||
|
|
||
| if (slv_req_cut.aw.user.collective_mask == '0) begin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this will work for all tools if the custom user field is not present in the passed struct...
| @@ -0,0 +1,739 @@ | |||
| // Copyright (c) 2019 ETH Zurich and University of Bologna. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
git diff master:src/axi_demux_simple.sv multicast:src/axi_mcast_demux_simple.sv
| else $fatal(1, "slv_ar_select_i illegal while ar_valid."); | ||
| w_underflow: assert property( @(posedge clk_i) | ||
| ((w_open == '0) && (w_cnt_up ^ w_cnt_down) |-> !w_cnt_down)) else | ||
| $fatal(1, "W counter underflowed!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the following assertions that you removed fully no longer applicable, or is there a version we could keep?
- aw_select: assume property( @(posedge clk_i) (slv_req_i.aw_valid |->
- (slv_aw_select_i < NoMstPorts))) else
- $fatal(1, "slv_aw_select_i is %d: AW has selected a slave that is not defined.\
- NoMstPorts: %d", slv_aw_select_i, NoMstPorts);
- internal_aw_select: assert property( @(posedge clk_i)
- (aw_valid |-> slv_aw_select_i < NoMstPorts))
- else $fatal(1, "slv_aw_select_i illegal while aw_valid.");
This PR adds multicast support to the AXI XBAR, such that write requests from a master can be multicast to multiple slaves.
To support multicast with low area overhead, we adopt the mask-based encoding proposed in https://arxiv.org/pdf/2502.19215.
In detail, this PR implements the following changes:
axi_mcast_demux_simpleimplements a superset of the functionality of theaxi_demux_simple, including all multicast capabilities, which require a few more signals on the interface, namelymst_is_mcast_oandmst_aw_commit_o. Soaxi_demux_simpleis rewritten as a wrapper aroundaxi_mcast_demux_simple, tying off unused ports and parameters.axi_mcast_muxandaxi_muxmodules, theaxi_mcast_xbar_unmuxedandaxi_xbar_unmuxedmodules, and theaxi_mcast_xbarandaxi_xbarmodules.axi_demux_id_countersmodule is moved to its own file, which is anyways good practice, and it would otherwise not be clear whether it should now belong toaxi_demux_simple.svoraxi_mcast_demux_simple.sv.ax_selectsignals from the address decoders. To eliminate this overhead (in the case of multicast it would even increase due to additional output signals from the decoders), we move the address decoders downstream of the spill registers in theaxi_mcast_demux_mappedmodule. The_mappedprefix is to identify a demux whose slaves are memory-mapped, i.e. that uses address decoders to generate theax_selectsignals. Since several modules use the demux with a non-address-basedax_selectsignal generation, theaxi_demuxmodule preserves the same interface and functionality as before, and does not wrap theaxi_mcast_demux_mappedmodule, unlike the other unicast and multicast IP pairs.