Expose ports #239
Expose ports #239
Conversation
belanoa
requested review from
colluca,
fischeti,
viv-eth and
paulsc96
as code owners
There was a problem hiding this comment.
I left a few questions below.
For the rest it looks good to me. Thanks for the contribution :)
We need to do two final things to get the PR ready:
- rebase on the latest
main, once we have finished merging the multicast PR (process started, currently the commit is staged in thedevelopbranch) - merge the
riscv-opcodesbranch you have into thesnitchbranch.
| @@ -127,7 +127,7 @@ module snitch import snitch_pkg::*; import riscv_instr::*; #( | |||
| logic interrupt, ecall, ebreak; | |||
| logic zero_lsb; | |||
|
|
|||
| logic meip, mtip, msip, mcip; | |||
| logic meip, mtip, msip, mcip, mxip; | |||
There was a problem hiding this comment.
What exactly is the mxip? What does it stand for? Accelerator, external...?
Why is it needed? Can the existing meip not be used instead?
| @@ -93,6 +96,13 @@ package ${cfg['cluster']['name']}_pkg; | |||
| `AXI_TYPEDEF_ALL(narrow_out, addr_t, narrow_out_id_t, data_t, strb_t, user_t) | |||
| `AXI_TYPEDEF_ALL(wide_in, addr_t, wide_in_id_t, data_dma_t, strb_dma_t, user_dma_t) | |||
| `AXI_TYPEDEF_ALL(wide_out, addr_t, wide_out_id_t, data_dma_t, strb_dma_t, user_dma_t) | |||
| `AXI_TYPEDEF_ALL(narrow_ext, addr_t, narrow_out_id_t, data_t, strb_t, user_t) | |||
There was a problem hiding this comment.
Is this needed? As the narrow_out types are identical.
| @@ -203,6 +203,21 @@ | |||
| "description": "Whether to expose memory cut configuration inputs for implementation", | |||
| "default": false | |||
| }, | |||
| "wide_tcdm_port_expose": { | |||
| "type": "boolean", | |||
| "description": "Whether to expose a wide memory port", | |||
There was a problem hiding this comment.
We can add more details, e.g.:
"Whether to expose a wide port into the TCDM at the cluster interface. Used to provide external masters, such as accelerators, with wide access to the TCDM."
| }, | ||
| "narrow_axi_port_expose": { | ||
| "type": "boolean", | ||
| "description": "Whether to expose a narrow AXI master port", |
There was a problem hiding this comment.
We can add more details, e.g.:
"Whether to expose a narrow AXI master port at the cluster interface. Used to provide a narrow interface to external slaves, e.g. to configure external accelerators."
| typedef logic [WideDataWidth-1:0] data_ext_t; | ||
| typedef logic [WideDataWidth/8-1:0] strb_ext_t; | ||
| typedef logic [NarrowIdWidthIn-1:0] id_mst_t; |
There was a problem hiding this comment.
Do we need dedicated types here? We could probably just reuse the *_dma_t types, which are the same. Ideally, we could call this e.g. *_wide_t if we wanted it to be DMA agnostic, but not really necessary for the time being.
There was a problem hiding this comment.
The mem_wide_narrow_mux_intf module at the end of the file should also be adapted. In my view, we could also just get rid of it entirely otherwise.
| // Decouple the narrow AXI master ports of the external port | ||
| axi_cut #( | ||
| .Bypass ( 1'b0 ), | ||
| .aw_chan_t ( axi_slv_aw_chan_t ), | ||
| .w_chan_t ( axi_slv_w_chan_t ), | ||
| .b_chan_t ( axi_slv_b_chan_t ), | ||
| .ar_chan_t ( axi_slv_ar_chan_t ), | ||
| .r_chan_t ( axi_slv_r_chan_t ), | ||
| .axi_req_t ( axi_slv_req_t ), | ||
| .axi_resp_t ( axi_slv_resp_t ) | ||
| ) i_axi_cut_hwpe_mst ( | ||
| .clk_i ( clk_i ), | ||
| .rst_ni ( rst_ni ), | ||
| .slv_req_i ( narrow_axi_slv_req[Ext] ), | ||
| .slv_resp_o ( narrow_axi_slv_rsp[Ext] ), | ||
| .mst_req_o ( narrow_ext_req_o ), | ||
| .mst_resp_i ( narrow_ext_resp_i ) | ||
| ); | ||
|
|
There was a problem hiding this comment.
I think it's better to just let people do this outside if there is a need to. Otherwise we can expose a parameter to optionally enable this. See cut immediately below this one.
| @@ -480,6 +496,10 @@ module snitch_cluster | |||
| assign zero_mem_start_address = cluster_periph_end_address; | |||
| assign zero_mem_end_address = cluster_periph_end_address + ZeroMemorySize * 1024; | |||
|
|
|||
| addr_t ext_mem_start_address, ext_mem_end_address; | |||
| assign ext_mem_start_address = zero_mem_end_address; | |||
| assign ext_mem_end_address = ext_mem_start_address + 'h100; | |||
There was a problem hiding this comment.
It would be good to also replace this 'h100 with a parameter instead of hardcoding it here. Compare e.g. the ZeroMemorySize parameter.
| @@ -492,6 +512,9 @@ module snitch_cluster | |||
| localparam addr_t ZeroMemAliasStart = PeriphAliasEnd; | |||
| localparam addr_t ZeroMemAliasEnd = PeriphAliasEnd + ZeroMemorySize * 1024; | |||
|
|
|||
| localparam addr_t ExtAliasStart = ZeroMemAliasEnd; | |||
| localparam addr_t ExtAliasEnd = ExtAliasStart + 'h100; | |||
| typedef logic [TcdmAddrWidth-1:0] tcdm_addr_t; | ||
| typedef logic [WideDataWidth-1:0] data_ext_t; | ||
| typedef logic [WideDataWidth/8-1:0] strb_ext_t; | ||
|
|
||
| `TCDM_TYPEDEF_ALL(tcdm_ext, tcdm_addr_t, data_ext_t, strb_ext_t, logic) | ||
|
|
There was a problem hiding this comment.
Similar types are also defined in snitch_cluster.sv. I see the need to provide them to the external user, but I think we can avoid duplicating them. Perhaps we can just move the existing ones to the package.
This PR allows the user to expose a narrow AXI master port and a wide TCDM port. This is useful if one wants to improve the cluster's capabilities with accelerators.