Re: [PATCH v2 08/10] gpu: nova-core: convert falcon registers to kernel register macro

["Gary Guo" <gary@garyguo.net>]

On Fri Mar 20, 2026 at 8:07 PM GMT, Danilo Krummrich wrote:
> On 3/20/26 8:52 PM, John Hubbard wrote:
>> On 3/20/26 10:38 AM, Joel Fernandes wrote:
>>> Hi Alex,
>>>
>>> On 3/20/2026 8:19 AM, Alexandre Courbot wrote:
>>>>      /// Reset the controller, select the falcon core, and wait for memory scrubbing to complete.
>>>> @@ -616,9 +462,10 @@ pub(crate) fn reset(&self, bar: &Bar0) -> Result {
>>>>          self.hal.select_core(self, bar)?;
>>>>          self.hal.reset_wait_mem_scrubbing(bar)?;
>>>>  
>>>> -        regs::NV_PFALCON_FALCON_RM::default()
>>>> -            .set_value(bar.read(regs::NV_PMC_BOOT_0).into())
>>>> -            .write(bar, &E::ID);
>>>> +        bar.write(
>>>> +            WithBase::of::<E>(),
>>>> +            regs::NV_PFALCON_FALCON_RM::from(bar.read(regs::NV_PMC_BOOT_0).into_raw()),
>>>> +        );
>>>>  
>>>
>>> Overall, I think the series is good improvement but I still feel this part is a
>>> step back in readability, and we should come up with something better. I don't
>>> think there's any chance anyone can memorize this syntax.
>> 
>> I must reluctantly (because I know this conversation has gone very
>> long, across so many versions) agree. That .write() statement is just
>> brutal, and we will be relying on AI in order to even figure it out,
>> I fear.
>> 
>> I'd hoped that there were other, simpler forms to express this,
>> is that not the case here?
>
> I mean, it is barely different from the above, except for the
> WithBase::of::<E>() argument instead of &E::ID.
>
> It becomes much more readable if you break it down (which is what we should
> probably do):
>
> 	let boot0 = bar.read(regs::NV_PMC_BOOT_0).into_raw();
>
> 	bar.write(WithBase::of::<E>(), regs::NV_PFALCON_FALCON_RM::from(boot0));
>
> For other cases where we read a register and write the same register we will
> have a way in the future that can omit WithBase::of::<E>() entirely, see also my
> reply in [1].
>
> I want to expand write_reg() to also be able to handle variable offset
> registers, e.g.:
>
> 	let reg = bar.read_reg(regs::NV_PFALCON_FALCON_RM::of::<E>());
>
> 	// modify reg
>
> 	bar.write_reg(reg);

If we add API that allows you to do projection based on a register, then you
could do:

    let reg: View<'_, _, _> = bar.project(regs::NV_PFALCON_FALCON_RM::of::<E>());
    let v = reg.read_val();
    reg.write_val(v.with_foo());

I'm planning to introduce `read_val`/`write_val` on `io::View<'_, IO, integer primitives>`
but with some traits it should be doable for register types, too.
(I think `read_val`, `write_val` makes sense here because they're views of a
simple value. I cannot use `read`/`write` here because they're used by register
mechanism already).

Best,
Gary