Message-ID: <2025092432-entrust-citizen-0232@gregkh>
Date: Wed, 24 Sep 2025 12:52:41 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Danilo Krummrich <dakr@...nel.org>
Cc: 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>,
	Yury Norov <yury.norov@...il.com>,
	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 Sun, Sep 21, 2025 at 03:47:55PM +0200, Danilo Krummrich wrote:
> On Sun Sep 21, 2025 at 2:45 PM CEST, Greg KH wrote:
> > Again, regmap handles this all just fine, why not just make bindings to
> > that api here instead?
> 
> The idea is to use this for the register!() macro, e.g.
> 
> 	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";
> 	});
> 
> (More examples in [1].)

Wonderful, but I fail to see where the endian-ness of this is set
anywhere.  Am I just missing that?  The regmap api enforces this idea,
and so the 

> 
> This generates a structure with the relevant accessors; we can also implement
> additional logic, such as:
> 
> 	impl NV_PMC_BOOT_0 {
> 	    /// Combines `architecture_0` and `architecture_1` to obtain the architecture of the chip.
> 	    pub(crate) fn architecture(self) -> Result<Architecture> {
> 	        Architecture::try_from(
> 	            self.architecture_0() | (self.architecture_1() << Self::ARCHITECTURE_0_RANGE.len()),
> 	        )
> 	    }
> 	
> 	    /// Combines `architecture` and `implementation` to obtain a code unique to the chipset.
> 	    pub(crate) fn chipset(self) -> Result<Chipset> {
> 	        self.architecture()
> 	            .map(|arch| {
> 	                ((arch as u32) << Self::IMPLEMENTATION_RANGE.len())
> 	                    | u32::from(self.implementation())
> 	            })
> 	            .and_then(Chipset::try_from)
> 	    }
> 	}
> 
> This conviniently allows us to read the register with
> 
> 	let boot0 = regs::NV_PMC_BOOT_0::read(bar);
> 
> and obtain an instance of the entire Chipset structure with
> 
> 	let chipset = boot0.chipset()?;
> 
> or pass it to a constructor that creates a Revision instance
> 
> 	let rev = Revision::from_boot0(boot0);
> 
> Analogously it allows us to modify and write registers without having to mess
> with error prone shifts, masks and casts, because that code is generated by the
> register!() macro. (Of course, unless we have more complicated cases where
> multiple fields have to be combined as illustrated above.)
> 
> Note that bar is of type pci::Bar<BAR0_SIZE> where BAR0_SIZE in our case is
> SZ_16M.
> 
> However, the type required by read() as generated by the register!() macro
> actually only requires something that implements an I/O backend, i.e
> kernel::io::Io<SIZE>.
> 
> pci::Bar is a specific implementation of kernel::io::Io.
> 
> With this we can let the actual I/O backend handle the endianness of the bus.

Ok, great, but right now it's not doing that from what I am seeing when
reading the code.  Shouldn't IoMem::new() take that as an argument?

But, that feels odd as our current iomem api in C doesn't care about
endian issues at all because it "assumes" that the caller has already
handle this properly and all that the caller "wants" is to write/read to
some memory chunk and not twiddle bits.

> (Actually, we could even implement an I/O backend that uses regmap.)

That would probably be best to do eventually as most platform drivers
use regmap today as it's the sanest api we have at the moment.

> So, I think the register!() stuff is rather orthogonal.

I think it's very relevant as people seem to just be "assuming" that all
the world (hardware and cpus) are little-endian, while in reality, they
are anything but.  As proof, the code that uses this register!() logic
today totally ignores endian issues and just assumes that it is both
running on a little-endian system, AND the hardware is little-endian.

As a crazy example, look at the USB host controllers that at runtime,
have to be queried to determine what endian they are running on and the
kernel drivers have to handle this "on the fly".  Yes, one can argue
that the hardware developers who came up with that should be forced to
write the drivers as penance for such sins, but in the end, it's us that
has to deal with it...

So ignoring it will get us quite a ways forward with controlling sane
hardware on sane systems, but when s390 finally realizes they can be
writing their drivers in rust, we are going to have to have these
conversations again :)

thanks,

greg k-h

Powered by blists - more mailing lists