`riscv`: Add macro to define CSR register types
As discussed in https://github.com/rust-embedded/riscv/pull/217, it would be nice to have a macro to define CSR register types.
I worked on a couple solutions:
- macros using the
pastecrate: https://github.com/rust-embedded/riscv/compare/master...rmsyn:riscv:riscv/csr-macro-paste- less verbose
- more consistent docs across definitions
- requires adding the
pastecrate as a dependency, and exposing thepaste!macro as a public re-export
- macros without using the
pastecrate: https://github.com/rust-embedded/riscv/compare/master...rmsyn:riscv:riscv/csr-macro-no-paste- more verbose
- requires caller-supplied docstrings and
set_<field>values - can potentially omit the docstrings at the expense of having generic documentation, e.g.
/// Gets the field value.
- requires caller-supplied docstrings and
- potentially less consistent docs across definitions
- requires no additional dependencies
- more verbose
What are some thoughts and/or preferences about the two implementations?
We could also potentially add a mask parameter to describe the bitmask of readable/writable fields (or two masks if those sets are not identical).
Adding a mask parameter(s) would allow us to add CsrType::bits and CsrType::set methods to read and write legal values safely. We could also then safely provide a impl From<usize> for CsrType implementation that calls CsrType::set.
I was thinking more about csr_field_read, csr_field_write, and csr_field_read_write macros that would be called independently for every field. Maybe this is not that worthy for having macros, though...
Regarding option 1: I would prefer to leave as less dependencies as possible in riscv
Regarding the mask parameter: I am not sure what you mean. Is it a parameter of the macro? We also need to think about WARL registers, in which you may read something different from what you wrote.
I would prefer to leave as less dependencies as possible in riscv
Agreed, I just included it because it reduces the verbosity of the macro call.
Regarding the mask parameter: I am not sure what you mean. Is it a parameter of the macro?
Yes, it would be a macro parameter to define the valid bitmask for the register.
For example, a register with valid fields bitranges [7:7], [1:1], [0:0], the mask parameter would be 0b1000_0011 or 0x83.
Which would then allow us to provide a safe interfaces for creating/setting all fileds at once:
pub const fn from_bits(val: usize) -> Self {
Self { bits: val & $mask }
}
pub const fn bits(&self) -> usize {
self.bits & $mask
}
We also need to think about WARL registers, in which you may read something different from what you wrote.
I don't think this is something we really need to worry about in the macro. Since we are effectively implementing a WLRL (write-legal, read-legal), by only providing interfaces to set legal fields.
For additional constraints like: "Field-A is only writable when Field-B is 1", I also don't think we should try to address with these helper macros.
What are the situations for WARL registers you think need to be covered that aren't currently addressed by the macro?
After thinking some more about the macro, and more generally the interfaces we want to provide, should we also provide another variant that takes in a multi-bit mask that covers a single field?
For example, say we have a field with the bitrange [3:0] that represents a single 4-bit value.
Currently, there are only two variants:
- single-bit field
- multi-bit range of single-bit field, e.g. the
hpmfields
I think you could open a PR as an RFC where we could work on an implementation with reviews etc. and we can see how it would look like. What do you think?
What do you think?
Sounds good to me, I'll add some things we discussed here, and open a PR.