riscv icon indicating copy to clipboard operation
riscv copied to clipboard

`riscv`: Add macro to define CSR register types

Open rmsyn opened this issue 1 year ago • 5 comments

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:

What are some thoughts and/or preferences about the two implementations?

rmsyn avatar May 25 '24 23:05 rmsyn

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.

rmsyn avatar May 25 '24 23:05 rmsyn

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.

romancardenas avatar May 28 '24 07:05 romancardenas

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 hpm fields

rmsyn avatar May 29 '24 02:05 rmsyn

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?

romancardenas avatar May 29 '24 08:05 romancardenas

What do you think?

Sounds good to me, I'll add some things we discussed here, and open a PR.

rmsyn avatar May 30 '24 00:05 rmsyn