cv32e40p
cv32e40p copied to clipboard
Use of SV compile macros in RTL
The RTL code for cv32e40p contains a large number of compile-time macro checks. For example, in riscv_pmp.sv, lines 233..242 we see:
`ifdef EN_NAPOT_RULE_8B
`RULE_0:
begin: BYTE_ALIGN_8B
mask_addr[i] = 32'hFFFF_FFFE;
start_addr[i] = pmp_addr_i[i] & mask_addr[i];
stop_addr[i] = pmp_addr_i[i];
`ifdef DEBUG_RULE $display(" NAPOT[%d] --> BYTE_ALIGN_8B: %8h<-- addr --> %8h", i, start_addr[i]<<2, stop_addr[i]<<2 ); `endif
end
`endif
There are three issues highlighted here:
- Macros like
EN_NAPOT_RULE_8B
are defined in the file that uses them. These should be moved to a central location for all such macros. - Macros likes
RULE_0
are used as constants. However, since macros have global scope, this could affect the entire design, so it is recommended to define these as localparams. - Macros are global, so things like
DEBUG_RULE
could create very unexpected behavior if it is used in another way elsewhere in the RTL.
I would like to see the following:
- Prefix all macros with CV32E40 (e.g.
CV32E40P_EN_NAPOT
) to maximize the likelihood of having unique macros. - Move all macro definitions (`define) to a central file.
- Update the manifest to ensure that the macro definitions are read in first to ensure that all simulation, formal verification and synthesis are using the same model.
- Replace macros used as constants with localparams.
Attached are two files: rtl_ifdefs.uniq.txt
lists all the unique uses of macros in the RTL and rtl_ifdefs.uniq.txt
shows where these are defined.
rtl_ifdefs.uniq.txt
rtl-defines.txt
I agree with all the points you have brought up, especially the namespacing is useful. This is a common pitfall in the C language too.
One more point I'd like to add: The lowrisc style guide recommends macros that are only (file)locally used to be undefd after usage. A local macro could e.g. be some code generation thing that severly reduces repetitive code, say exploding dozens of AXI interfaces to signals.
The issue has been solved except for the PMP file. PMP is however not used in CV32E40P, but file itself is kept. Once we start on a core using the PMP the issue will be addressed there as well (e.g. as was done in the Ibex PMP). Therefore I am waiving the 'remainder of this issue' for CV32E40P.