Re: [PATCH v6 4/5] rust: Move register and bitfield macros out of Nova

[Dirk Behme <dirk.behme@de.bosch.com>]

On 09/10/2025 13:16, 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.
> 
>> * 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();


Many thanks! I think such verbose explanation helps a lot!


>> 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.

Intuitively for this example I was thinking about &io ;)

Thanks again!

Dirk