Re: [PATCH 2/2] gpu: nova-core: add support for GPU timer block.

["Danilo Krummrich" <dakr@kernel.org>]

On Mon Oct 26, 2026 at 3:56 AM CET, Dave Airlie via B4 Relay wrote:
> diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
> index 88662da634dc..d5c67625b561 100644
> --- a/drivers/gpu/nova-core/regs.rs
> +++ b/drivers/gpu/nova-core/regs.rs
> @@ -419,6 +419,24 @@ pub(crate) fn mem_scrubbing_done(self) -> bool {
>      }
>  }
>  
> +// PTIMER
> +
> +register! {
> +    base: NovaRegisters;
> +
> +    pub(crate) NV_PTIMER_TIME_PRIV_LEVEL_MASK(u32) @ 0x00009430 {
> +        4:4 level0 => bool;
> +    }
> +
> +    pub(crate) NV_PTIMER_TIME_0(u32) @ 0x00009400 {
> +        31:5 nsec;
> +    }
> +
> +    pub(crate) NV_PTIMER_TIME_1(u32) @ 0x00009410 {
> +        28:0 nsec;
> +    }
> +}
> +
>  // The modules below provide registers that are not identical on all supported chips. They should
>  // only be used in HAL modules.
>  
> @@ -520,6 +538,28 @@ pub(crate) mod gh100 {
>              31:0    fsp_boot_complete;
>          }
>      }
> +
> +    // PGC6/TIMER
> +    register! {
> +        base: NovaRegisters;
> +
> +        pub(crate) NV_PGC6_SCI_SEC_TIMER_TIME_0(u32) @ 0x00118f54 {
> +            31:5 nsec;
> +        }
> +
> +        pub(crate) NV_PGC6_SCI_SEC_TIMER_TIME_1(u32) @ 0x00118f58 {
> +            28:0 nsec;
> +        }
> +
> +        pub(crate) NV_PGC6_SCI_SYS_TIMER_OFFSET_0(u32) @ 0x00118df4 {
> +            0:0 update;
> +            31:5 nsec;
> +        }
> +
> +        pub(crate) NV_PGC6_SCI_SYS_TIMER_OFFSET_1(u32) @ 0x00118df8 {
> +            28:0 nsec;
> +        }
> +    }
>  }

Timer specific register definitions should go into the timer module.

> +#[pin_data]

Why is this #[pin_data]?

> +pub(crate) struct Timer {
> +    chipset: Chipset,

Why does this carry the Chipset?

> +    sys_timer_offset_ns: i64,
> +}

More generally, what's the purpose of this struct? What does it represent?

Nothing seems to tie this object to a particular GPU or prevents constructing
multiple instances for the same GPU.

It also doesn't seem to provide any access to the underlying HAL's functions?

> +impl Timer {
> +    pub(crate) fn new(bar: Bar0<'_>, chipset: Chipset) -> Result<Self> {
> +        Ok(Self {
> +            chipset,
> +            sys_timer_offset_ns: hal::timer_hal(chipset).init(chipset, bar)?,
> +        })
> +    }
> +
> +    #[expect(dead_code)]
> +    pub(crate) fn sys_timer_offset_ns(&self) -> i64 {
> +        self.sys_timer_offset_ns
> +    }

What do we need this method for?

> +}
> +
> +pub(crate) fn get_cpu_time_ns() -> i64 {
> +    Instant::<Monotonic>::now().as_nanos()
> +}

I don't think this helper adds much value, let's just make users call
Instant::<Monotonic>::now().as_nanos() themselves.