Message-ID: <1da3adab-f52c-461d-bd68-8623d6d7e223@de.bosch.com>
Date: Thu, 13 Nov 2025 07:26:46 +0100
From: Dirk Behme <dirk.behme@...bosch.com>
To: Alexandre Courbot <acourbot@...dia.com>, Dirk Behme
	<dirk.behme@...il.com>, Danilo Krummrich <dakr@...nel.org>, Joel Fernandes
	<joelagnelf@...dia.com>
CC: <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>,
	<dri-devel@...ts.freedesktop.org>, 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>,
	dri-devel <dri-devel-bounces@...ts.freedesktop.org>
Subject: Re: [PATCH v6 4/5] rust: Move register and bitfield macros out of
 Nova

On 02/11/2025 04:00, Alexandre Courbot wrote:
> On Sun Nov 2, 2025 at 3:51 AM JST, Dirk Behme wrote:
>> On 09.10.25 13:28, Alexandre Courbot wrote:
>>> On Thu Oct 9, 2025 at 8:16 PM JST, Danilo Krummrich wrote:
>>>> 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.
>>>
>>> Patch 2 of this series actually adds support for 16 and 8 bit register
>>> storage.
>>
>> Hmm, how to use that with the register!() macro? I mean patch 2 adds
>> support for different storage widths for *bitfields*. But looking at
>> patch 4 [2] it looks like *register!()* still uses $name(u32)? With
>> that it looks like that the register!() macro still just supports 32
>> bit registers? Or what have I missed?
>>
>> What I'm looking for is a way to specify if a register is 8, 16 or 32
>> bit. Using the example from above something like
>>
>> register!(TSTR<u8> @ ....
> 
> Errr indeed, you are correct. The `register` macro's syntax has not been
> updated to take advantage of `bitfield`'s storage types, and `u32` is
> still hardcoded as of this series.
> 
> This looks like an oversight - a register is basically a bitfield with
> some I/O, so making it support storage types should be trivial. I guess
> this hasn't been done yet because Nova is the only user so far, and we
> don't need/want to explicitly specify a type for each register since
> they are invariably `u32`.
> 
> But it wouldn't look good to change the syntax of `register` after
> moving it out, so I agree this should take place before the move. The
> same applies to the visiblity feature.
> 
> One way to avoid a update all the declarations so far would be to give
> Nova its own `register` macro that invokes the one in `kernel` with
> the relevant parameters hardcoded.


Just fyi, hacking something like [1] below does work for my (very 
limited) use case. And it defaults `register!` without type to <u32>.

Thanks!

Dirk

[1]

--- a/rust/kernel/io/register.rs
+++ b/rust/kernel/io/register.rs
@@ -276,33 +276,38 @@ pub trait RegisterBase<T> {
  /// ```
  #[macro_export]
  macro_rules! register {
-    // Creates a register at a fixed offset of the MMIO space.
+    // Creates a register at a fixed offset of the MMIO space, defaults 
to u32 if no type is specified.
      ($name:ident @ $offset:literal $(, $comment:literal)? { 
$($fields:tt)* } ) => {
-        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? 
{ $($fields)* } );
-        register!(@io_fixed $name @ $offset);
+        register!($name<u32> @ $offset $(, $comment)? { $($fields)* });
+    };
+
+    // Creates a register at a fixed offset of the MMIO space, explicit 
type required.
+    ($name:ident<$ty:ty> @ $offset:literal $(, $comment:literal)? { 
$($fields:tt)* } ) => {
+        ::kernel::bitfield!(pub(crate) struct $name($ty) $(, $comment)? 
{ $($fields)* } );
+        register!(@io_fixed<$ty> $name @ $offset);
      };

      // Creates an alias register of fixed offset register `alias` with 
its own fields.
-    ($name:ident => $alias:ident $(, $comment:literal)? { 
$($fields:tt)* } ) => {
-        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? 
{ $($fields)* } );
-        register!(@io_fixed $name @ $alias::OFFSET);
+    ($name:ident<$ty:ty> => $alias:ident $(, $comment:literal)? { 
$($fields:tt)* } ) => {
+        ::kernel::bitfield!(pub(crate) struct $name($ty) $(, $comment)? 
{ $($fields)* } );
+        register!(@io_fixed<$ty> $name @ $alias::OFFSET);
      };

      // Creates a register at a relative offset from a base address 
provider.
-    ($name:ident @ $base:ty [ $offset:literal ] $(, $comment:literal)? 
{ $($fields:tt)* } ) => {
-        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? 
{ $($fields)* } );
-        register!(@io_relative $name @ $base [ $offset ]);
+    ($name:ident<$ty:ty> @ $base:ty [ $offset:literal ] $(, 
$comment:literal)? { $($fields:tt)* } ) => {
+        ::kernel::bitfield!(pub(crate) struct $name($ty) $(, $comment)? 
{ $($fields)* } );
+        register!(@io_relative<$ty> $name @ $base [ $offset ]);
      };

      // Creates an alias register of relative offset register `alias` 
with its own fields.
-    ($name:ident => $base:ty [ $alias:ident ] $(, $comment:literal)? { 
$($fields:tt)* }) => {
-        ::kernel::bitfield!(pub(crate) struct $name(u32) $(, $comment)? 
{ $($fields)* } );
-        register!(@io_relative $name @ $base [ $alias::OFFSET ]);
+    ($name:ident<$ty:ty> => $base:ty [ $alias:ident ] $(, 
$comment:literal)? { $($fields:tt)* }) => {
+        ::kernel::bitfield!(pub(crate) struct $name($ty) $(, $comment)? 
{ $($fields)* } );
+        register!(@io_relative<$ty> $name @ $base [ $alias::OFFSET ]);
      };

-    // Creates an array of registers at a fixed offset of the MMIO space.
+    // Creates an array of registers at a fixed offset of the MMIO 
space. (u32 only for now)
      (
-        $name:ident @ $offset:literal [ $size:expr ; $stride:expr ] $(, 
$comment:literal)? {
+        $name:ident<u32> @ $offset:literal [ $size:expr ; $stride:expr 
] $(, $comment:literal)? {
              $($fields:tt)*
          }
      ) => {
@@ -311,20 +316,20 @@ macro_rules! register {
          register!(@io_array $name @ $offset [ $size ; $stride ]);
      };

-    // Shortcut for contiguous array of registers (stride == size of 
element).
+    // Shortcut for contiguous array of registers (stride == size of 
element). (u32 only for now)
      (
-        $name:ident @ $offset:literal [ $size:expr ] $(, 
$comment:literal)? {
+        $name:ident<u32> @ $offset:literal [ $size:expr ] $(, 
$comment:literal)? {
              $($fields:tt)*
          }
      ) => {
-        register!($name @ $offset [ $size ; 
::core::mem::size_of::<u32>() ] $(, $comment)? {
+        register!($name<u32> @ $offset [ $size ; 
::core::mem::size_of::<u32>() ] $(, $comment)? {
              $($fields)*
          } );
      };

-    // Creates an array of registers at a relative offset from a base 
address provider.
+    // Creates an array of registers at a relative offset from a base 
address provider. (u32 only for now)
      (
-        $name:ident @ $base:ty [ $offset:literal [ $size:expr ; 
$stride:expr ] ]
+        $name:ident<u32> @ $base:ty [ $offset:literal [ $size:expr ; 
$stride:expr ] ]
              $(, $comment:literal)? { $($fields:tt)* }
      ) => {
          static_assert!(::core::mem::size_of::<u32>() <= $stride);
@@ -332,20 +337,19 @@ macro_rules! register {
          register!(@io_relative_array $name @ $base [ $offset [ $size ; 
$stride ] ]);
      };

-    // Shortcut for contiguous array of relative registers (stride == 
size of element).
+    // Shortcut for contiguous array of relative registers (stride == 
size of element). (u32 only for now)
      (
-        $name:ident @ $base:ty [ $offset:literal [ $size:expr ] ] $(, 
$comment:literal)? {
+        $name:ident<u32> @ $base:ty [ $offset:literal [ $size:expr ] ] 
$(, $comment:literal)? {
              $($fields:tt)*
          }
      ) => {
-        register!($name @ $base [ $offset [ $size ; 
::core::mem::size_of::<u32>() ] ]
+        register!($name<u32> @ $base:ty [ $offset:literal [ $size:expr 
; ::core::mem::size_of::<u32>() ] ]
              $(, $comment)? { $($fields)* } );
      };

-    // Creates an alias of register `idx` of relative array of 
registers `alias` with its own
-    // fields.
+    // Creates an alias of register `idx` of relative array of 
registers `alias` with its own fields. (u32 only for now)
      (
-        $name:ident => $base:ty [ $alias:ident [ $idx:expr ] ] $(, 
$comment:literal)? {
+        $name:ident<u32> => $base:ty [ $alias:ident [ $idx:expr ] ] $(, 
$comment:literal)? {
              $($fields:tt)*
          }
      ) => {
@@ -354,17 +358,15 @@ macro_rules! register {
          register!(@io_relative $name @ $base [ $alias::OFFSET + $idx * 
$alias::STRIDE ] );
      };

-    // Creates an alias of register `idx` of array of registers `alias` 
with its own fields.
-    // This rule belongs to the (non-relative) register arrays set, but 
needs to be put last
-    // to avoid it being interpreted in place of the relative register 
array alias rule.
-    ($name:ident => $alias:ident [ $idx:expr ] $(, $comment:literal)? { 
$($fields:tt)* }) => {
+    // Creates an alias of register `idx` of array of registers `alias` 
with its own fields. (u32 only for now)
+    ($name:ident<u32> => $alias:ident [ $idx:expr ] $(, 
$comment:literal)? { $($fields:tt)* }) => {
          static_assert!($idx < $alias::SIZE);
          ::kernel::bitfield!(pub(crate) struct $name(u32) $(, 
$comment)? { $($fields)* } );
          register!(@io_fixed $name @ $alias::OFFSET + $idx * 
$alias::STRIDE );
      };

-    // Generates the IO accessors for a fixed offset register.
-    (@io_fixed $name:ident @ $offset:expr) => {
+    // Generates the IO accessors for a fixed offset register, using 
the type parameter.
+    (@io_fixed<$ty:ty> $name:ident @ $offset:expr) => {
          #[allow(dead_code)]
          impl $name {
              pub(crate) const OFFSET: usize = $offset;
@@ -374,7 +376,15 @@ impl $name {
              pub(crate) fn read<const SIZE: usize, T>(io: &T) -> Self where
                  T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
              {
-                Self(io.read32($offset))
+                Self(
+                    if core::any::TypeId::of::<$ty>() == 
core::any::TypeId::of::<u8>() {
+                        io.read8($offset) as $ty
+                    } else if core::any::TypeId::of::<$ty>() == 
core::any::TypeId::of::<u16>() {
+                        io.read16($offset) as $ty
+                    } else {
+                        io.read32($offset) as $ty
+                    }
+                )
              }

              /// Write the value contained in `self` to the register 
address in `io`.
@@ -382,7 +392,13 @@ pub(crate) fn read<const SIZE: usize, T>(io: &T) -> 
Self where
              pub(crate) fn write<const SIZE: usize, T>(self, io: &T) where
                  T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
              {
-                io.write32(self.0, $offset)
+                if core::any::TypeId::of::<$ty>() == 
core::any::TypeId::of::<u8>() {
+                    io.write8(self.0 as u8, $offset)
+                } else if core::any::TypeId::of::<$ty>() == 
core::any::TypeId::of::<u16>() {
+                    io.write16(self.0 as u16, $offset)
+                } else {
+                    io.write32(self.0 as u32, $offset)
+                }
              }

              /// Read the register from its address in `io` and run `f` 
on its value to obtain a new
@@ -401,8 +417,8 @@ pub(crate) fn alter<const SIZE: usize, T, F>(
          }
      };

-    // Generates the IO accessors for a relative offset register.
-    (@io_relative $name:ident @ $base:ty [ $offset:expr ]) => {
+    // Generates the IO accessors for a relative offset register, using 
the type parameter.
+    (@io_relative<$ty:ty> $name:ident @ $base:ty [ $offset:expr ]) => {
          #[allow(dead_code)]
          impl $name {
              pub(crate) const OFFSET: usize = $offset;
@@ -420,9 +436,7 @@ pub(crate) fn read<const SIZE: usize, T, B>(
              {
                  const OFFSET: usize = $name::OFFSET;

-                let value = io.read32(
-                    <B as 
::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET
-                );
+                let value = io.read::<$ty>(<B as 
::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET);

                  Self(value)
              }
@@ -441,10 +455,7 @@ pub(crate) fn write<const SIZE: usize, T, B>(
              {
                  const OFFSET: usize = $name::OFFSET;

-                io.write32(
-                    self.0,
-                    <B as 
::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET
-                );
+                io.write::<$ty>(self.0, <B as 
::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET);
              }

              /// Read the register from `io`, using the base address 
provided by `base` and adding
-- 
2.48.0

Powered by blists - more mailing lists