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

["Alexandre Courbot" <acourbot@nvidia.com>]

On Mon Nov 9, 2026 at 10:55 AM JST, Dave Airlie via B4 Relay wrote:
<...>
> diff --git a/drivers/gpu/nova-core/timer/hal/gh100.rs b/drivers/gpu/nova-core/timer/hal/gh100.rs
> new file mode 100644
> index 000000000000..154c7a301a0c
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer/hal/gh100.rs
> @@ -0,0 +1,63 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use super::TimerHal;
> +
> +use kernel::{
> +    io::Io,
> +    prelude::*,
> +    time::*, //
> +};
> +
> +use crate::{
> +    driver::Bar0,
> +    fsp::Fsp,
> +    timer::regs::gh100::*, //
> +};
> +
> +/// Timer HAL for Hopper and Blackwell.
> +struct Gh100;
> +
> +fn sec_timer_read(bar: Bar0<'_>) -> u64 {
> +    let mut hi2 = bar.read(NV_PGC6_SCI_SEC_TIMER_TIME_1);
> +    loop {
> +        let lo = bar.read(NV_PGC6_SCI_SEC_TIMER_TIME_0);
> +        let hi = bar.read(NV_PGC6_SCI_SEC_TIMER_TIME_1);
> +        if hi == hi2 {
> +            return u64::from(u32::from(hi)) << 32 | u64::from(u32::from(lo));
> +        }
> +        hi2 = bar.read(NV_PGC6_SCI_SEC_TIMER_TIME_1);
> +    }
> +}
> +
> +fn set_sys_timer_offset(bar: Bar0<'_>, fsp: &Fsp<'_>, time_ns: u64) -> Result<u64> {

This method cannot fail, so it should return just `u64`.

> +    let sec_timer_ns = sec_timer_read(bar);
> +
> +    let sys_timer_offset_ns = time_ns.wrapping_sub(sec_timer_ns);

As discussed in my other response, wrapping here is defensible, but I'd
like to have a comment explaining why this is so, i.e. that even if we
wrap after an underflow the SEC_TIMER + OFFSET value module 61 bits will
match the system clock.

> +
> +    // Don't set the timer offset registers on buggy firmware
> +    if !fsp.is_bug_3957833_affected(bar) {
> +        bar.write_reg(NV_PGC6_SCI_SYS_TIMER_OFFSET_1::from_nsec(
> +            sys_timer_offset_ns,
> +        ));
> +        bar.write_reg(
> +            NV_PGC6_SCI_SYS_TIMER_OFFSET_0::from_nsec(sys_timer_offset_ns).with_update(true),
> +        );
> +    }
> +    Ok(sys_timer_offset_ns)
> +}
> +
> +impl TimerHal for Gh100 {
> +    fn setup(&self, bar: Bar0<'_>, fsp: Option<&Fsp<'_>>) -> Result<u64> {
> +        // The OpenRM driver uses wall clock time here not monotonic
> +        let sys_time_ns = Instant::<RealTime>::now().as_nanos();
> +
> +        // fsp is required on gh100 - return error if not available.
> +        let fsp = fsp.ok_or(EINVAL)?;
> +
> +        // CAST: realtime system time can't be negative.
> +        set_sys_timer_offset(bar, fsp, sys_time_ns as u64)
> +    }
> +}
> +
> +const GH100: Gh100 = Gh100;
> +pub(super) const GH100_HAL: &dyn TimerHal = &GH100;
> diff --git a/drivers/gpu/nova-core/timer/hal/tu102.rs b/drivers/gpu/nova-core/timer/hal/tu102.rs
> new file mode 100644
> index 000000000000..064f9b92666e
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer/hal/tu102.rs
> @@ -0,0 +1,41 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use super::TimerHal;
> +
> +use kernel::{
> +    io::Io,
> +    prelude::*,
> +    time::*, //
> +};
> +
> +use crate::{
> +    driver::Bar0,
> +    fsp::Fsp,
> +    timer::regs::*, //
> +};
> +
> +/// Timer HAL for Turing, Ampere, Ada
> +struct Tu102;
> +
> +fn set_time(bar: Bar0<'_>, time_ns: u64) -> Result {
> +    if !bar.read(NV_PTIMER_TIME_PRIV_LEVEL_MASK).level0() {
> +        return Err(EINVAL);
> +    }

I don't think we want to make probe fail on this since the GPU should
still be usable (I don't know of a pre-GH100 platform that requires the
timer registers to be set in order to work). Let's log in the caller
(which should take a device::Device as argument for that) if this method
returns and error, and continue.

> +    bar.write_reg(NV_PTIMER_TIME_1::from_nsec(time_ns));
> +    bar.write_reg(NV_PTIMER_TIME_0::from_nsec(time_ns));
> +    Ok(())
> +}
> +
> +impl TimerHal for Tu102 {
> +    fn setup(&self, bar: Bar0<'_>, _fsp: Option<&Fsp<'_>>) -> Result<u64> {
> +        // The OpenRM driver uses wall clock time here not monotonic
> +        let sys_time_ns = Instant::<RealTime>::now().as_nanos();
> +
> +        // CAST: realtime system time can't be negative.
> +        set_time(bar, sys_time_ns as u64)?;
> +        Ok(0_u64)
> +    }
> +}
> +
> +const TU102: Tu102 = Tu102;
> +pub(super) const TU102_HAL: &dyn TimerHal = &TU102;
> diff --git a/drivers/gpu/nova-core/timer/regs.rs b/drivers/gpu/nova-core/timer/regs.rs
> new file mode 100644
> index 000000000000..229babc2c296
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer/regs.rs
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use kernel::io::register;
> +use kernel::num::Bounded;

nit: `kernel` imports should be grouped.

> +
> +use crate::driver::NovaRegisters;
> +use pin_init::Zeroable;
> +
> +/// Field value for the NSEC field (31:5) of the `_0` registers: `nsec[31:5]`.
> +///
> +/// The GPU timer counts nanoseconds with 32 ns granularity, so `nsec[4:0]` is dropped.
> +fn nsec_lo(nsec: u64) -> Bounded<u32, 27> {
> +    Bounded::<u32, 32>::from(nsec as u32).shr::<5, _>()

Since this uses `as`, this needs a `CAST:` comment. Something like:

    // CAST: we want to take the lowest 32 bits of `nsec`.

(we may turn a linter on in the future that warns if such comments are
missing, so I'd like to make sure we set them ahead-of-time even if they
are trivial)

> +}
> +
> +/// Field value for the NSEC field (28:0) of the `_1` registers: `nsec[60:32]`.
> +///
> +/// The GPU timer is 61 bits wide, so `nsec[63:61]` is not representable and is
> +/// truncated - the same bits the hardware discards on a raw register write.
> +fn nsec_hi(nsec: u64) -> Bounded<u32, 29> {
> +    Bounded::<u32, 29>::from_expr((nsec >> 32) as u32 & 0x1fff_ffff)

And for this one it can be:

    // CAST: we are shifting a 64-bit number by 32 bits, leaving 32 bits of value.

Also, instead of the `0x1fff_ffff` mask, can you use
`bits::genmask_u32(0..=28)` (after importing `kernel::bits`)? It makes
it easier to understand what we are masking.

> +}
> +
> +// PTIMER
> +
> +register! {
> +    base: NovaRegisters;
> +
> +    pub(super) NV_PTIMER_TIME_PRIV_LEVEL_MASK(u32) @ 0x00009430 {
> +        4:4 level0 => bool;
> +    }
> +
> +    pub(super) NV_PTIMER_TIME_0(u32) @ 0x00009400 {
> +        31:5 nsec;
> +    }
> +
> +    pub(super) NV_PTIMER_TIME_1(u32) @ 0x00009410 {
> +        28:0 nsec;
> +    }
> +}
> +
> +impl NV_PTIMER_TIME_0 {
> +    pub(super) fn from_nsec(nsec: u64) -> Self {
> +        Self::zeroed().with_nsec(nsec_lo(nsec))
> +    }
> +}
> +
> +impl NV_PTIMER_TIME_1 {
> +    pub(super) fn from_nsec(nsec: u64) -> Self {
> +        Self::zeroed().with_nsec(nsec_hi(nsec))
> +    }
> +}
> +
> +pub(super) mod gh100 {
> +    use kernel::io::register;
> +
> +    use super::{
> +        nsec_hi, //
> +        nsec_lo,

nit: `//` should be on the last import.