Harmonize ports and parameters

[andreaskurth]

Let us collect the changes required to harmonize ports and parameters and to minimize incompatibilities with EDA tools. Those changes will be breaking (as in "backwards-incompatible"), so let us make sure we get them right.

This is currently a draft and contributors are kindly asked to comment. Upon agreement, this information will be added to the Contribution Guidelines.

Preamble: The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in BCP 14 [RFC2119] [RFC8174] when, and only when, they appear in all capitals, as shown here.

Parameters

Legal Types

Every parameter of a synthesizable module MUST be either:
(a) a type, or
(b) a (vector of) one of the following SystemVerilog types:

• bit or logic, which MAY be signed (but are by default implicitly unsigned), or
• byte, shortint, int, or longint, which MUST be followed by unsigned or signed, or

(c) a typedefed type of one of the types in (b).

In particular, structs and strings MUST NOT be used as parameter of a synthesizable module.

Rationale: Many tools do not properly implement complex types for parameters.

For non-synthesizable modules and classes, the key words MUST and MUST NOT in this section are relaxed to SHOULD and SHOULD NOT, respectively. (In particular, testbench modules MAY use time and string parameters.)

Signedness

If an integer parameter (i.e., byte, shortint, int, or longint) is not supposed to take negative values, it MUST be declared unsigned instead of signed.

Default Value

Every parameter MUST have a default value.

If possible, the default value SHOULD be a null value that is outside the legal range of the parameter (e.g., a signal width of zero). In this case, the module SHOULD contain an assertion to ensure that the parameter is set to a value other than the null value at instantiation.

Rationale: Many tools require parameters to have a default value, but in many cases a parameter that is not set at instantiation indicates an error that should be detected.

Derived Parameters

The parameter list of a module MUST NOT contain localparams. Rationale: Unsupported by some tools.

Instead, if the value of a parameter is derived from another parameter and should not be overridden at instantiation, the line above the derived parameter SHOULD be as follows:

/// Dependent parameter, DO NOT OVERRIDE!

Names

• The name of a non-type parameter MUST be in UpperCamelCase.
  Rationale: style guide.
• The name of a type parameter MUST be in lower_snake_case and end with _t.
  Rationale: style guide.
• The name of a non-type parameter MUST NOT be prefixed with Axi.
  Example: A module with a parametrizable data width has a parameter named DataWidth, not AxiDataWidth.
  Rationale: Every module name starts with axi_ and prefixing parameters with Axi is redundant.
• If a parameter only applies to one port, its name MUST start with the prefix of the port (converted to the casing dictated above and to singular if the port is an array) or with Num (see below) followed by the prefix of the port.
  Example: For a crossbar, the ID width of each of its slave ports (part of an array prefixed slv_ports_) would be given by a parameter named SlvPortIdWidth, and the request type of each of its slave ports would be given by a parameter named slv_port_axi_req_t.
• Conversely, if a parameter applies to more than one port, its name MUST NOT start with the prefix of one of the ports.
• If the name of a type parameter does not have a port-specific prefix, it MUST be prefixed with axi_.
  Rationale: Some tools do not properly scope type definitions, and adding a topic-specific prefix reduces the chance of type name collisions.
• If a parameter defines the number of bits in a signal, its name SHOULD end with Width.
• If a parameter defines a quantity other than bits in a signal, its name SHOULD contain Num followed by a noun in plural. No MUST NOT be used to denote a quantity parameter. (Rationale: easily mistaken for negation, e.g., "no registers").
• If a parameter defines the maximum value of a quantity, its name SHOULD contain Max followed by a noun in plural.
• The name of every parameter of a testbench module MUST start with Tb or tb_. The name of any parameter of a non-testbench module MUST NOT start with Tb or tb_.
  Rationale: The name of each parameter of a top-level module that is to be assigned a value when the simulator is invoked must be unique among all simulated modules; see test: Prefix top-level TB parameters #152 (comment).

Examples

A crossbar with multiple slv_ports and mst_ports could have the following among its parameters:

/// Number of slave ports of the crossbar
parameter int unsigned NumSlvPorts = 0,
/// Number of master ports of the crossbar
parameter int unsigned NumMstPorts = 0,
/// AXI address width
parameter int unsigned AddrWidth = 0,
/// AXI data width
parameter int unsigned DataWidth = 0,
/// AXI ID width at the slave ports
parameter int unsigned SlvPortIdWidth = 0,
/// Maximum number of in-flight transactions at each slave port
parameter int unsigned SlvPortMaxTxns = 0,
/// Maximum number of in-flight transactions at each master port
parameter int unsigned MstPortMaxTxns = 0,
/// AXI4(+ATOPs) request struct of each slave port
parameter type slv_port_axi_req_t = logic,
/// AXI4 response struct of each slave port
parameter type slv_port_axi_rsp_t = logic,
/// AXI4(+ATOPs) request struct of each master port
parameter type mst_port_axi_req_t = logic,
/// AXI4 response struct of each master port
parameter type mst_port_axi_rsp_t = logic,

Ports

• Each input port MUST end with _i (or _ni if it is active-low).
• Each output port MUST end with _o (or _no if it is active-low).
• The name of each slave port MUST contain slv_port_ (or slv_ports_ if the port is an array).
• The name of each master port MUST contain mst_port_ (or mst_ports_ if the port is an array).
• The name of each request port MUST contain _req directly before the input/output suffix.
• The name of each response port MUST contain _rsp directly before the input/output suffix.

Examples

A module with a single AXI-Lite slave port could contain in its inputs and outputs:

input  axi_lite_req_t slv_port_req_i,
output axi_lite_rsp_t slv_port_rsp_o,

A CDC from a src_clk_i to a dst_clk_i would contain in its inputs and outputs:

// Slave Port in Source Clock Domain
input  axi_req_t src_slv_port_req_i,
output axi_rsp_t src_slv_port_rsp_o,
// Master Port in Destination Clock Domain
output axi_req_t dst_mst_port_req_o,
input  axi_rsp_t dst_mst_port_rsp_i,

A crossbar with multiple slave and master ports would contain in its inputs and outputs:

// Slave Ports
input  slv_port_axi_req_t [NumSlvPorts-1:0] slv_ports_req_i,
output slv_port_axi_rsp_t [NumSlvPorts-1:0] slv_ports_rsp_o,
// Master Ports
output mst_port_axi_req_t [NumMstPorts-1:0] mst_ports_req_o,
input  mst_port_axi_rsp_t [NumMstPorts-1:0] mst_ports_rsp_i,

A protocol converter from AXI to AXI-Lite would contain in its inputs and outputs:

// AXI Slave Port
input  slv_port_axi_req_t slv_port_req_i,
output slv_port_axi_rsp_t slv_port_rsp_o,
// AXI-Lite Master Port
output mst_port_axi_lite_req_t mst_port_req_o,
input  mst_port_axi_lite_rsp_t mst_port_rsp_i,

Channel and Request/Response Types

In this section, X MUST be either axi or axi_lite in accordance with whether the type is part of full AXI or AXI-Lite.

• A channel type MUST end with X_Y_t, where Y is one of aw, w, b, ar, or r and MUST correspond to the channel type.
• A request type MUST end with X_req_t.
• A response type MUST end with X_rsp_t.

Interfaces

• This repository defines four interfaces: axi_if, axi_dv_if, axi_lite_if, and axi_lite_dv_if.
  Rationale for naming: compliant with Google Verible's interface-name-style and consistent with the name of types in the style guide we follow (which does not have rules for naming interfaces).
• The modports are named slv_port (for a slave port modport), mst_port (for a master port modport), and mon_port (for an all-input monitor modport).
  Rationale: consistent with the naming of non-interface ports.
• The non-dv interfaces MUST be synthesizable. The dv interfaces are used for design verification and MAY contain non-synthesizable code.
• All parameters in an interface MUST obey the rules in the above Parameters section.
• The name of each slave port interface MUST contain slv_port (or slv_ports if the interface port is an array).
• The name of each master port interface MUST contain mst_port (or mst_ports if the interface port is an array).
• Arrays of interfaces MUST be unpacked (i.e., dimensions after identifier). The dimensions MUST be in big-endian notation (e.g., [0:N-1]). Dimensions SHOULD be zero-based unless there are strong reasons against it. Zero-based dimensions SHOULD use the short [N] notation. There MUST NOT be a space between identifier and dimensions.

Examples

A crossbar (or rather, its _intf variant) with multiple slave and master ports would contain in its port list:

axi_if.slv_port slv_ports[NumSlvPorts],
axi_if.mst_port mst_ports[NumMstPorts],

[andreaskurth]

RFC CC @zarubaf @fabianschuiki @WRoenninger

[zarubaf]

Thanks for putting this together. This looks very, very promising. There is currently only one thing which came to my mind:

Regarding the interface wrapper for multiple slave or master ports this is essentially an unpacked type where the style guide recommends big-endianess. So I would recommend:

axi_if.slv_port slv_ports [NumSlvPorts],
axi_if.mst_port mst_ports [NumMstPorts],

or

axi_if.slv_port slv_ports [0:NumSlvPorts-1],
axi_if.mst_port mst_ports [0:NumMstPorts-1],

[andreaskurth]

Regarding the interface wrapper for multiple slave or master ports this is essentially an unpacked type where the style guide recommends big-endianess. So I would recommend:

axi_if.slv_port slv_ports [NumSlvPorts],
axi_if.mst_port mst_ports [NumMstPorts],

or

axi_if.slv_port slv_ports [0:NumSlvPorts-1],
axi_if.mst_port mst_ports [0:NumMstPorts-1],

Good point, thanks! I am adding it. To precisely follow the style guide, let us omit the space between the signal name and the array.

[WRoenninger]

Thanks for defining these. They look very well thought out. I will update my PRs #116 #115 and #33 accordingly when these harmonization guidelines are finalized.

I could do the flattening of the axi_pkg::xbar_cfg_t struct for axi_xbar as part of #116 as I have added there doc descriptions for the parameters there already.

One question regarding #33. There the LLC has a master and slave port with AXI4+ATOP and an AXI4 Lite cfg slave port. So the names should be therefore:

// Slave Port facing CPU
input  slv_port_axi_req_t slv_port_req_i,
output slv_port_axi_rsp_t slv_port_rsp_o,
// Master Port facing Memory
output mst_port_axi_req_t mst_port_req_o,
input  mst_port_axi_rsp_t mst_port_rsp_i,
// Configuration Lite Slave port
input  axi_lite_req_t cfg_slv_port_req_i,
output axi_lite_rsp_t cfg_slv_port_rsp_o, 

Also #33 would require some additional work from my part as the LLC submodules use a cfg struct to propagate the LLC configuration.

[andreaskurth]

Thanks for defining these. They look very well thought out. I will update my PRs #116 #115 and #33 accordingly when these harmonization guidelines are finalized.

Great, thanks!

I could do the flattening of the axi_pkg::xbar_cfg_t struct for axi_xbar as part of #116 as I have added there doc descriptions for the parameters there already.

I am fine with flattening the parameters of the XBAR in the same PR as adding the docstrings, but please do not mix this up with the crossbar-internal pipeline change. The crossbar pipeline change is complex enough in itself, and I would like to keep it separate for now. (The changes for pipelining the crossbar need careful reviewing to make sure we still guarantee deadlock freedom. Reviewing that is not high priority for me at the moment, because when a crossbar complex enough that it needs to be pipelined inside, it has many master and slave ports, and internal pipelining would add a lot of area -- which again has implications on timing. So I do not think there is urgent need for making the crossbar internally pipelineable.) You do not have to keep the xbar_pipeline branch rebased on master with high priority; instead please refactor all changes that are not related to pipelining and the axi_demux FIFO->counter into a different PR.

One question regarding #33. There the LLC has a master and slave port with AXI4+ATOP and an AXI4 Lite cfg slave port. So the names should be therefore:

// Slave Port facing CPU
input  slv_port_axi_req_t slv_port_req_i,
output slv_port_axi_rsp_t slv_port_rsp_o,
// Master Port facing Memory
output mst_port_axi_req_t mst_port_req_o,
input  mst_port_axi_rsp_t mst_port_rsp_i,
// Configuration Lite Slave port
input  axi_lite_req_t cfg_slv_port_req_i,
output axi_lite_rsp_t cfg_slv_port_rsp_o, 

I agree except for the configuration port, which would be

input  cfg_slv_port_axi_lite_req_t cfg_slv_port_req_i,
output cfg_slv_port_axi_lite_rsp_t cfg_slv_port_rsp_o,

because "If a parameter only applies to one port, it MUST start with the prefix of the port [...]". I realize this is a bit bulky to write, but it helps preventing type collisions.

Also #33 would require some additional work from my part as the LLC submodules use a cfg struct to propagate the LLC configuration.

Yes.

[WRoenninger]

Thank you for the clarification.

Just a heads up, the changes for the xbar_pipeline are required for the LLC. There is a bypass for uncached memory accesses using both an axi_demux and axi_mux. The issue is the same deadlocking that happens if you put pipeline stages into the cross of the crossbar currently. The LLC also acts as sort of a 'pipeline' and needs the fix from #116 to allow simultaneous LLC and bypass write accesses.

[andreaskurth]

Just a heads up, the changes for the xbar_pipeline are required for the LLC. There is a bypass for uncached memory accesses using both an axi_demux and axi_mux. The issue is the same deadlocking that happens if you put pipeline stages into the cross of the crossbar currently. The LLC also acts as sort of a 'pipeline' and needs the fix from #116 to allow simultaneous LLC and bypass write accesses.

I see, thanks for clarifying this. This gives the changes to axi_demux higher priority again.

[WRoenninger]

A crossbar with multiple slave and master ports would contain in its inputs and outputs:

// Slave Ports
input  slv_ports_axi_req_t [NumSlvPorts-1:0] slv_ports_req_i,
output slv_ports_axi_rsp_t [NumSlvPorts-1:0] slv_ports_rsp_o,
// Master Ports
output mst_ports_axi_req_t [NumMstPorts-1:0] mst_ports_req_o,
input  mst_ports_axi_rsp_t [NumMstPorts-1:0] mst_ports_rsp_i,

Should the req/rsp type in this case not be singular on the port part?
Rationale: The types themselves describe a single request/response struct of an individual port. The construct that makes the port plural is the array definition directly in the module port definition. So the proposition is to change it to:

// Slave Ports
input  slv_port_axi_req_t [NumSlvPorts-1:0] slv_ports_req_i,
output slv_port_axi_rsp_t [NumSlvPorts-1:0] slv_ports_rsp_o,
// Master Ports
output mst_port_axi_req_t [NumMstPorts-1:0] mst_ports_req_o,
input  mst_port_axi_rsp_t [NumMstPorts-1:0] mst_ports_rsp_i,

Dropping the s from the parameterized type, however keep it in the port name.

Consequentially: *_ports_axi_req_t would be the type with the array included:

typedef slv_port_axi_req_t [NumSlvPorts-1:0] slv_ports_req_t;

[andreaskurth]

A crossbar with multiple slave and master ports would contain in its inputs and outputs:

// Slave Ports
input  slv_ports_axi_req_t [NumSlvPorts-1:0] slv_ports_req_i,
output slv_ports_axi_rsp_t [NumSlvPorts-1:0] slv_ports_rsp_o,
// Master Ports
output mst_ports_axi_req_t [NumMstPorts-1:0] mst_ports_req_o,
input  mst_ports_axi_rsp_t [NumMstPorts-1:0] mst_ports_rsp_i,

Should the req/rsp type in this case not be singular on the port part?
Rationale: The types themselves describe a single request/response struct of an individual port. The construct that makes the port plural is the array definition directly in the module port definition. So the proposition is to change it to:

// Slave Ports
input  slv_port_axi_req_t [NumSlvPorts-1:0] slv_ports_req_i,
output slv_port_axi_rsp_t [NumSlvPorts-1:0] slv_ports_rsp_o,
// Master Ports
output mst_port_axi_req_t [NumMstPorts-1:0] mst_ports_req_o,
input  mst_port_axi_rsp_t [NumMstPorts-1:0] mst_ports_rsp_i,

Dropping the s from the parameterized type, however keep it in the port name.

I agree that it is more intuitive to use a singular name for the type for an array port. However, I am not convinced that the additional complexity of the type naming rule is worth the gained intuitiveness.

Currently, the rule is

If a parameter only applies to one port, its name MUST start with the prefix of the port (converted to the casing dictated above) or with Num (see below) followed by the prefix of the port.

Should we change that to "If a parameter only applies to one port, its name MUST start with the prefix of the port (converted to the casing dictated above and to singular if the port is an array) or with Num (see below) followed by the prefix of the port." ?

Consequentially: *_ports_axi_req_t would be the type with the array included:

typedef slv_port_axi_req_t [NumSlvPorts-1:0] slv_ports_req_t;

slv_ports_axi_req_t, because an AXI request struct type MUST end with axi_req_t.

[andreaskurth]

Should we change that to "If a parameter only applies to one port, its name MUST start with the prefix of the port (converted to the casing dictated above and to singular if the port is an array) or with Num (see below) followed by the prefix of the port." ?

Updated! 👍

[andreaskurth]

Any other change requests or objections? If not, I would add this to our Contribution Guidelines and start accepting PRs that implement these changes.