rust-embedded / riscv

Low level access to RISC-V processors
789 stars 157 forks source link

`riscv`: Add macro to define CSR register types #218

Open rmsyn opened 1 month ago

rmsyn commented 1 month ago

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 commented 1 month ago

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.

romancardenas commented 1 month ago

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.

rmsyn commented 1 month ago

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:

romancardenas commented 1 month ago

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?

rmsyn commented 1 month ago

What do you think?

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