Message-Id: <DDDQZ8LM2OGP.VSEG03ZE0K04@kernel.org>
Date: Thu, 09 Oct 2025 13:16:39 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Dirk Behme" <dirk.behme@...bosch.com>
Cc: "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>, "Benno Lossin"
 <lossin@...nel.org>, "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>, "Andrea Righi" <arighi@...dia.com>,
 <nouveau@...ts.freedesktop.org>
Subject: Re: [PATCH v6 4/5] rust: Move register and bitfield macros out of
 Nova

On Thu Oct 9, 2025 at 8:59 AM CEST, Dirk Behme wrote:
> Assuming that register.rs is supposed to become the "generic" way to 
> access hardware registers I started to have a look to it. Some weeks 
> back testing interrupts I used some quite simple timer with 4 registers 
> [1]. Now, thinking about converting it to register!() I have three 
> understanding / usage questions:
>
> * At the moment register!() is for 32-bit registers, only? So it can't 
> be used for my example having 8-bit and 16-bit registers as well?

Yes, currently the register!() macro always generates a 32-bit register type
(mainly because nova-core did not need anything else). However, this will of
course be generalized (which should be pretty straight forward).

Having a brief look at the TMU datasheet it looks like you should be able to
treat TSTR and TCR as 32-bit registers without any issues for testing the
register!() macro today. I.e. you can just define it as:

	register!(TSTR @ 0x04, "Timer Start Register" {
	    2:2    str2 as bool, "Specifies whether TCNT2 is operated or stopped.";
	    1:1    str1 as bool, "Specifies whether TCNT1 is operated or stopped.";
	    0:0    str0 as bool, "Specifies whether TCNT0 is operated or stopped.";
	});

Same for TCR.

> * In my example I used io.try_write*() and io.try_read*() for the 
> register access. What is the relationship between these and the 
> register!() accessors (e.g. from the examples BOOT_0::read(&bar);)? Will 
> both stay? When to use which?

The try_*() variants should only be used of the offset of the register is not
known at compile time.

If it is known at compile time (such as in your case) you should use the
infallible variants that perform a range check at compile time.

For this to work you need to specify the minimum MMIO range your driver expects,
i.e. instead of

	let iomem = Arc::pin_init(request.iomap()?, GFP_KERNEL)?;

you call

	let iomem = Arc::pin_init(request.iomap_sized::<TMU_MMIO_SIZE>()?, GFP_KERNEL)?;

where TMU_MMIO_SIZE is the minimum MMIO region size your driver is able to
operate on. In your case this would be 0x12, given that TCR has a width of
16-bit. However, if you treat it as 32-bit register it would be 0x14.

Even without the register!() macro this would be a huge improvement. For
instance, your IRQ handler from [1] can do

	let tcr = io.read16_relaxed(TCR);
	if tcr & (0x1 << 8) != 0 {
	    io.write16_relaxed(tcr & !(0x1 << 8), TCR);
	}

instead of

	let tcr = io.try_read16_relaxed(TCR).unwrap_or(0);
	if tcr & (0x1 << 8) != 0 {
	    io.try_write16_relaxed(tcr & !(0x1 << 8), TCR).unwrap_or(());
	}

And with the register macro it becomes

	let tcr = TCR::read(io);
	if tcr.underflow() {
		tcr.set_underflow(false);
		tcr.write(io);
	}

Note that you can also extend the generated TCR type accordingly, for instance:

	impl TCR {
	    fn handle_underflow<const SIZE: usize, T>(io: &T)
	    where
	        T: Deref<Target = Io<SIZE>>,
	    {
	        let this = Self::read(io);
	        if this.underflow() {
	            this.set_underflow(false);
	            this.write(io);
	        }
	    }
	}

and then from your IRQ handler you can just call

	TCR::handle_underflow();

> Note: Due to the file move obviously not the full content of the "new" 
> file register.rs is shown in this patch. Therefore, let me try it this 
> way, citing from register.rs:
>
> -- cut --
> ...
> /// This defines a `BOOT_0` type which can be read or written from 
> offset `0x100` of an `Io`
> /// region
> ....
> /// ```ignore
> /// // Read from the register's defined offset (0x100).
> /// let boot0 = BOOT_0::read(&bar);
> -- cut --
>
> * What is "&bar" in this example? Is it the `Io` region the explanation 
> talks about?

Yes, it's the I/O backend (a pci::Bar in this case). However, we should probably
use a more generic name in the examples.

> [1] 
> https://lore.kernel.org/rust-for-linux/dd34e5f4-5027-4096-9f32-129c8a067d0a@de.bosch.com/
>
> The registers:
>
> const TSTR: usize =  0x4; //  8 Bit register
> const TCOR: usize =  0x8; // 32 Bit register
> const TCNT: usize =  0xC; // 32 Bit register
> const TCR:  usize = 0x10; // 16 Bit register

Powered by blists - more mailing lists