Message-Id: <D8GWNOPYRQN3.GO3XVERSQUXH@nvidia.com>
Date: Sat, 15 Mar 2025 23:21:48 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Danilo Krummrich" <dakr@...nel.org>
Cc: "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
 <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...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>,
 <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 2:14 AM JST, Danilo Krummrich wrote:
> Hi Alex,
>
> Thanks for working on a generic solution for this! Few comments below.
>
> 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";
>
> Should we call the macro just `register!`?

I was thinking this is maybe too generic, but yeah it probably makes
sense. ^_^;

>
>>         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"
>
> I think we probably need an argument indicating whether the register field is
> {RW, RO, WO}, such that we can generate the corresponding accessors / set the
> corresponding masks.
>
>>     );
>> 
>> 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.
>
> I like that, including the conversion seems pretty convenient.
>
>> 
>> 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.
>
> Feel free to add my Suggested-by. You can also refer to the corresponding task
> in our nova-core task list.

Sure!

>
>> 
>> 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.
>
> Can't we figure this out from the bit mask in the macro?

We probably can't do that reliably IIUC - a 32-bit register might only
have its 8 LSBs holding useful data, it would still need to be read
using a 32-bit access nonetheless... Or maybe I misunderstood your
suggestion?

>
>> - 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.
>
> Did you consider proc macros for more flexibility?

Another rabbit hole to dive into. ^_^;

That would probably allow a better syntax, while at the same time
solving the issue of the exported helper macros. Let me give it a try.

>
>> - We probably need an option to make some fields or whole registers
>>   read-only.
>
> Ah, I see, you thought of this already.
>
>> - The I/O offset and read/write methods should be optional, so the
>>   layout part can be used for things that are not registers.
>
> I guess you think of shared memory? For DMA we already have the dma_read! and
> dma_write! macros that may fit in.

Right, in this case you wouldn't need to access the value through Io,
and only the field helpers make sense - so I guess they are orthogonal
with the Io access impl in the end.

Powered by blists - more mailing lists