Message-Id: <D8F89QYB689L.UWU0WFGZ04XU@nvidia.com>
Date: Fri, 14 Mar 2025 00:02:29 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Boqun Feng" <boqun.feng@...il.com>
Cc: "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
 <alex.gaynor@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <benno.lossin@...ton.me>, "Andreas Hindborg" <a.hindborg@...nel.org>,
 "Alice Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>,
 "Danilo Krummrich" <dakr@...nel.org>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH RFC] rust: add macros to define registers layout

On Fri Mar 14, 2025 at 12:00 AM JST, Boqun Feng wrote:
> On Thu, Mar 13, 2025 at 11:48:25PM +0900, Alexandre Courbot wrote:
>> Add two macros, reg_def!() and reg_def_rel!(), that define a given
>> register's layout and provide accessors for absolute or relative
>> offsets, respectively.
>> 
>> The following example (taken from the rustdoc) helps understanding how
>> they are used:
>> 
>>     reg_def!(Boot0@...0000100, "Basic revision information about the chip";
>>         3:0     minor_rev => as u8, "minor revision of the chip";
>>         7:4     major_rev => as u8, "major revision of the chip";
>>         28:20   chipset => try_into Chipset, "chipset model"
>>     );
>> 
>> This defines a `Boot0` type which can be read or written from offset
>> `0x100` of an `Io` region. It is composed of 3 fields, for instance
>> `minor_rev` is made of the 4 less significant bits of the register. Each
>> field can be accessed and modified using helper methods:
>> 
>>     // Read from offset `0x100`.
>>     let boot0 = Boot0.read(&bar);
>>     pr_info!("chip revision: {}.{}", boot0.major_rev(), boot0.minor_rev());
>> 
>>     // `Chipset::try_from` will be called with the value of the field and
>>     // returns an error if the value is invalid.
>>     let chipset = boot0.chipset()?;
>> 
>>     // Update some fields and write the value back.
>>     boot0.set_major_rev(3).set_minor_rev(10).write(&bar);
>> 
>> Fields are made accessible using one of the following strategies:
>> 
>> - `as <type>` simply casts the field value to the requested type.
>> - `as_bit <type>` turns the field into a boolean and calls
>>   <type>::from()` with the obtained value. To be used with single-bit
>>   fields.
>> - `into <type>` calls `<type>::from()` on the value of the field. It is
>>   expected to handle all the possible values for the bit range selected.
>> - `try_into <type>` calls `<type>::try_from()` on the value of the field
>>   and returns its result.
>> 
>> The documentation strings are optional. If present, they will be added
>> to the type or the field getter and setter methods they are attached to.
>> 
>> Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
>> ---
>> I have written these initially for the nova-core driver, then it has
>> been suggested that they might be useful outside of it as well, so here
>> goes.
>> 
>> This is my first serious attempt at writing Rust macros and I am sure
>> there is a lot that is wrong with them, but I'd like to get early
>> feedback and see whether this is actually something we want for the
>> kernel in general.
>> 
>> The following in particular needs to be improved, suggestions are
>> welcome:
>> 
>> - Inner types other than `u32` need to be supported - this can probably
>>   just be an extra parameter of the macro.
>> - The syntax can certainly be improved. I've tried to some with
>>   something that makes the register layout obvious, while fitting within
>>   the expectations of the Rust macro parser, but my lack of experience
>>   certainly shows here.
>> - We probably need an option to make some fields or whole registers
>>   read-only.
>> - The I/O offset and read/write methods should be optional, so the
>>   layout part can be used for things that are not registers.
>> - The visibility of the helper macros is a bit of a headache - I haven't
>>   found a way to completely hide them to the outside, so I have prefixed
>>   them with `__` for now.
>> - Formatting - there are some pretty long lines, not sure how to break
>>   them in an idiomatic way.
>> 
>> Sorry if this is still a bit rough around the edges, but hopefully the
>> potential benefit is properly conveyed.
>> ---
>>  rust/kernel/lib.rs |   1 +
>>  rust/kernel/reg.rs | 284 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 285 insertions(+)
>> 
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index 398242f92a961c3a445d681c65449047a847968a..d610199f6675d22fa01d4db524d9989581f7b646 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -69,6 +69,7 @@
>>  pub mod prelude;
>>  pub mod print;
>>  pub mod rbtree;
>> +mod reg;
>
> This is for io registers? Could you please move it into kernel::io
> instead of defining it as a top level mod?

It is (although one could argue that the bitfield accessors can probably
be useful for non-register types as well), and agreed that this would
fit better in kernel::io. Thanks for the suggestion.

Powered by blists - more mailing lists