Message-Id: <DD15H63RK1KJ.1J584C1QC4L28@kernel.org>
Date: Wed, 24 Sep 2025 17:53:40 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Yury Norov" <yury.norov@...il.com>
Cc: "Greg KH" <gregkh@...uxfoundation.org>, "Benno Lossin"
 <lossin@...nel.org>, "Joel Fernandes" <joelagnelf@...dia.com>,
 <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>,
 <dri-devel@...ts.freedesktop.org>, <acourbot@...dia.com>, "Alistair Popple"
 <apopple@...dia.com>, "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
 <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
 <gary@...yguo.net>, <bjorn3_gh@...tonmail.com>, "Andreas Hindborg"
 <a.hindborg@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>, "Trevor
 Gross" <tmgross@...ch.edu>, "David Airlie" <airlied@...il.com>, "Simona
 Vetter" <simona@...ll.ch>, "Maarten Lankhorst"
 <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard" <mripard@...nel.org>,
 "Thomas Zimmermann" <tzimmermann@...e.de>, "John Hubbard"
 <jhubbard@...dia.com>, "Timur Tabi" <ttabi@...dia.com>,
 <joel@...lfernandes.org>, "Elle Rhumsaa" <elle@...thered-steel.dev>,
 "Daniel Almeida" <daniel.almeida@...labora.com>,
 <nouveau@...ts.freedesktop.org>
Subject: Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code
 from register! into new macro

On Wed Sep 24, 2025 at 4:38 PM CEST, Yury Norov wrote:
> I didn't ask explicitly, and maybe it's a good time to ask now: Joel,
> Danilo and everyone, have you considered adopting this project in
> kernel?
>
> The bitfield_struct builds everything into the structure:
>
>         use bitfield_struct::bitfield;
>         
>         #[bitfield(u8, order = Msb)]
>         struct MyMsbByte {
>             /// The first field occupies the *most* significant bits
>             #[bits(4)]
>             kind: usize,
>             system: bool,
>             #[bits(2)]
>             level: usize,
>             present: bool
>         }
>         let my_byte_msb = MyMsbByte::new()
>             .with_kind(10)
>             .with_system(false)
>             .with_level(2)
>             .with_present(true);
>         
>         //                          .- kind
>         //                          |    .- system
>         //                          |    | .- level
>         //                          |    | |  .- present
>         assert_eq!(my_byte_msb.0, 0b1010_0_10_1);
>
> Here MSB is not BE. For BE you'd specify:
>
>         #[bitfield(u16, repr = be16, from = be16::from_ne, into = be16::to_ne)]
>         struct MyBeBitfield {
>             #[bits(4)]
>             first_nibble: u8,
>             #[bits(12)]
>             other: u16,
>         }
>
> The "from = be16::from_ne",  is seemingly the same as cpu_to_be32() here.
>
> It looks like bitfield_struct tries to resolve hw access problems
> by outsourcing them to 'from' and 'to' callbacks, and that looks
> similar to what regmap API does (is that correct?).
>
> Greg, Is that what you're asking about?
>
> This is another bitfield crate with the similar approach 
>
> https://crates.io/crates/bitfield
>
> So we're not the first, and we need to discuss what is already done.
>
> As far as I understand, Joel decided to go in the other direction:
> bitfields are always native in terms of endianess and not designed to
> be mapped on registers directly. Which means they don't specify order
> of accesses, number of accesses, access timing, atomicity, alignment,
> cacheability and whatever else I/O related.
>
> I discussed with Joel about the hw register access and he confirmed
> that the idea of his bitfields is to be a simple wrapper around logical
> ops, while the I/O is a matter of 'backbone', which is entirely different
> thing:

When I was working on initial Rust driver support about a year ago, I also
thought about how Rust drivers can deal with registers and added the TODO in
[1]. This was picked up by Alex, who came up with a great implementation for the
register!() macro, which Joel splitted up into separate register!() and bitfield
parts in the context of moving it from a nova internal implementation into a
core kernel API.

As also described in [2], the idea is to have a macro, register!(), that creates
an abstract representation of a register, in order to remove the need for
drivers to manually construct values through shift operations, masks, etc.

At the same time the idea also is to get proper documentation of the hardware
registers in the kernel; the register!() macro encourages that, by its
definition trying to come close to how registers are typically documented in
datasheets, i.e. get rid of thousands of lines of auto-generated #defines for
base addresses, shift and masks with cryptic names and provide something like

	register!(NV_PMC_BOOT_0 @ 0x00000000, "Basic revision information about the GPU" {
	    28:24   architecture_0 as u8, "Lower bits of the architecture";
	    23:20   implementation as u8, "Implementation version of the architecture";
	    8:8     architecture_1 as u8, "MSB of the architecture";
	    7:4     major_revision as u8, "Major revision of the chip";
	    3:0     minor_revision as u8, "Minor revision of the chip";
	});

instead.

(It has quite some more features that also allow you to directly derive complex
types from primitives and define arrays of registers, such as in

	register!(NV_PFALCON_FBIF_TRANSCFG @ PFalconBase[0x00000600[8]] {
	    1:0     target as u8 ?=> FalconFbifTarget;
	    2:2     mem_type as bool => FalconFbifMemType;
	});

which makes dealing with such registers in drivers way less error prone.

Here's one example of how it looks like to alter a single field within a
register:

	// `bar` is the `pci::Bar` I/O backend.
	regs::NV_PFALCON_FALCON_ENGINE::alter(bar, |v| v.set_reset(true));

Of course you could also alter multiple fields at once by doing more changes
within the closure.)

It intentionally avoids encoding hardware bus specific endianness, because
otherwise we'd need to define this for every single register definition, which
also falls apart when the device can sit on top of multiple different busses.

Instead, the only thing that matters is that there is a contract between the
abstract register representation and the I/O backends, such that the data can be
correctly layed out by the I/O backend, which has to be aware of the actual
hardware bus instead.

As mentioned in another thread, one option for that is to use regmap within the
I/O backends, but that still needs to be addressed.

So, for the register!() macro, I think we should keep it an abstract
representation and deal with endianness in the I/O backend.

However, that's or course orthogonal to the actual feature set of the bitfield
macro itself.

- Danilo

[1] https://docs.kernel.org/gpu/nova/core/todo.html#generic-register-abstraction-rega
[2] https://lore.kernel.org/lkml/DD0ZTZM8S84H.1YDWSY7DF14LM@kernel.org/

Powered by blists - more mailing lists