Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

riscv: Add macro to define CSR register types #218

Closed
rmsyn opened this issue May 25, 2024 · 5 comments
Closed

riscv: Add macro to define CSR register types #218

rmsyn opened this issue May 25, 2024 · 5 comments

Comments

@rmsyn
Copy link
Contributor

rmsyn commented May 25, 2024

As discussed in #217, it would be nice to have a macro to define CSR register types.

I worked on a couple solutions:

  • macros using the paste crate: master...rmsyn:riscv:riscv/csr-macro-paste
    • less verbose
    • more consistent docs across definitions
    • requires adding the paste crate as a dependency, and exposing the paste! macro as a public re-export
  • macros without using the paste crate: 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.
    • potentially less consistent docs across definitions
    • requires no additional dependencies

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

@rmsyn rmsyn changed the title riscv: Add macro to define CSR register types riscv: Add macro to define CSR register types May 25, 2024
@rmsyn
Copy link
Contributor Author

rmsyn commented May 25, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

rmsyn commented May 29, 2024

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

@romancardenas
Copy link
Contributor

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
Copy link
Contributor Author

rmsyn commented May 30, 2024

What do you think?

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants