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

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

On Tue Nov 3, 2026 at 1:33 AM CET, Dave Airlie via B4 Relay wrote:
> +// struct that just holds the sys timer offset
> +// after initialising the GPU timer block.
> +pub(crate) struct Timer {
> +    sys_timer_offset_ns: i64,
> +}

I feel like this should actually be TimerState, as it is not really a timer
(instance) representation in the sense of carrying the HAL and providing methods
to interact with the hardware (which we do not intend).

I'd suggest making Timer a unit struct and giving it an init() function, i.e.
something like this:

	struct Timer;

	struct TimerState {
	    sys_timer_offset_ns: i64,
	}

	impl Timer {
	    /// Initialize the GPU's system timer.
	    pub(crate) fn init(bar: Bar0<'_>, chipset: Chipset) -> Result<TimerState>;
	}

> +
> +impl Timer {
> +    pub(crate) fn new(bar: Bar0<'_>, chipset: Chipset) -> Result<Self> {
> +        Ok(Self {
> +            sys_timer_offset_ns: hal::timer_hal(chipset).init(bar)?,
> +        })
> +    }
> +
> +    // this value will be passed to GspInitRequest SYS_TIMER_OFFSET_NS

NIT: This should describe what the returned value represents, although in this
specific case it may also still make sense to give a hint how it is used.

Please use intra-doc links and markdown where it makes sense.

> +    #[expect(dead_code)]
> +    pub(crate) fn sys_timer_offset_ns(&self) -> i64 {
> +        self.sys_timer_offset_ns
> +    }
> +}
> diff --git a/drivers/gpu/nova-core/timer/hal.rs b/drivers/gpu/nova-core/timer/hal.rs
> new file mode 100644
> index 000000000000..fd702867b470
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer/hal.rs
> @@ -0,0 +1,27 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use kernel::prelude::*;
> +
> +use crate::{
> +    driver::Bar0,
> +    gpu::{
> +        Architecture,
> +        Chipset, //
> +    },
> +};
> +
> +mod gh100;
> +mod tu102;
> +
> +pub(super) trait TimerHal {
> +    /// return system timer offset ns

NIT:

	/// Initialize the GPU's system timer.
	///
	/// Returns the system timer offset in nanoseconds.

> +    fn init(&self, bar: Bar0<'_>) -> Result<i64>;
> +}
> +
> +pub(super) fn timer_hal(chipset: Chipset) -> &'static dyn TimerHal {
> +    match chipset.arch() {
> +        Architecture::Turing | Architecture::Ampere | Architecture::Ada => tu102::TU102_HAL,
> +        Architecture::Hopper => gh100::GH100_HAL,
> +        Architecture::BlackwellGB10x | Architecture::BlackwellGB20x => gh100::GB100_HAL,
> +    }
> +}
> 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..a9d46bdd2527
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer/hal/gh100.rs
> @@ -0,0 +1,94 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use super::TimerHal;
> +
> +use kernel::{
> +    io::Io,
> +    prelude::*,
> +    time::*, //
> +};
> +
> +use crate::{
> +    driver::Bar0,
> +    timer::regs, //
> +};
> +
> +/// Timer HAL for Hopper and Blackwell.
> +struct Gh100 {
> +    /// `true` on Hopper, the only architecture where the FSP ucode can predate the fix for
> +    /// bug 3957833 and hence the sys timer offset must not be programmed. Blackwell FSP
> +    /// always includes the fix.
> +    fsp_bug3957833_war: bool,
> +}
> +
> +/// FSP ucode version from which on the fix for bug 3957833 is included.
> +const FSP_BUG_3957833_FIX_VERSION: u32 = 0x44c;
> +
> +impl Gh100 {
> +    /// kfspRequiresBug3957833WAR_HAL: returns `true` if the FSP ucode predates the fix for
> +    /// bug 3957833 and hence the sys timer offset must not be programmed. The RM implements
> +    /// the version check for GH100 only; for all later chips it defaults to `false`.
> +    fn requires_bug3957833_war(&self, bar: Bar0<'_>) -> bool {
> +        self.fsp_bug3957833_war
> +            && bar.read(regs::gh100::NV_GFW_FSP_UCODE_VERSION).full().get()
> +                < FSP_BUG_3957833_FIX_VERSION

I'd move this into regs.rs and make it a method of NV_GFW_FSP_UCODE_VERSION,
e.g.

	impl NV_GFW_FSP_UCODE_VERSION {
	    pub(crate) fn is_bug_3957833_affected(self) -> bool {
	        const FSP_BUG_3957833_FIX_VERSION: u32 = 0x44c;
	
	        self.full().get() < FSP_BUG_3957833_FIX_VERSION
	    }
	}

so you can just call

	bar.read(regs::gh100::NV_GFW_FSP_UCODE_VERSION).is_bug_3957833_affected()
	
This also keeps FSP_BUG_3957833_FIX_VERSION local to regs.rs and gets us rid of
this HAL function.

> +    }
> +
> +    fn timer_time(&self, bar: Bar0<'_>, time_ns: i64) -> Result<i64> {

Can't we just use unsigned for the argument and return type? None of those are
ever expected to be negative, no?

The invariant for the argument to be non-negative comes from Instant, so I'd convert
before calling into timer_time(). This should avoid the corresponding as casts below.

> +        let mut sec_hi2 = u32::from(bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_1));
> +        let sec_timer_ns: u64;
> +        loop {
> +            let sec_lo = u32::from(bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_0));
> +            let sec_hi = u32::from(bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_1));
> +            if sec_hi == sec_hi2 {
> +                sec_timer_ns = u64::from(sec_hi) << 32 | u64::from(sec_lo);
> +                break;
> +            }
> +            sec_hi2 = u32::from(bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_1));
> +        }
> +
> +        // The system and the GPU timer are both free-running with unrelated epochs, so the
> +        // difference can be negative. Both operands are < 2^62, hence this cannot overflow.

I think we accumulated a bunch of those comments in nova-core, I think it would
be nice to consider whether we want a prefix for this, similar to TIMEOUT or
CALC. Maybe OVERFLOW? But this can be a follow-up of course.

On the other hand, this is a fallible method already, so I'd just go with
checked_sub().

> +        let sys_timer_offset_ns = time_ns - sec_timer_ns as i64;
> +
> +        // The offset is passed to the GSP as an unsigned value and, like in the RM's
> +        // tmrSetCurrentTime_GH100, is required to be non-negative. With wall clock time this
> +        // can only fail if the system clock is set behind the GPU power-on time.
> +        if sys_timer_offset_ns < 0 {
> +            return Err(EINVAL);
> +        }
> +
> +        if !self.requires_bug3957833_war(bar) {
> +            let offset_ns = sys_timer_offset_ns as u64;
> +            bar.write_reg(
> +                regs::gh100::NV_PGC6_SCI_SYS_TIMER_OFFSET_1::zeroed()
> +                    .try_with_nsec(((offset_ns >> 32) & 0x1fff_ffff) as u32)?,
> +            );
> +            bar.write_reg(
> +                regs::gh100::NV_PGC6_SCI_SYS_TIMER_OFFSET_0::zeroed()
> +                    .try_with_nsec(((offset_ns >> 5) & 0x7ff_ffff) as u32)?
> +                    .with_update(true),
> +            );

This looks similar to NV_TLB_FLUSH_PDB_LO::from_pdb_addr(). I think we should do
this in the register impl:

	impl NV_PGC6_SCI_SYS_TIMER_OFFSET_1 {
	    /// Creates a register value from bits 60:32 of the offset.
	    pub(crate) fn from_offset_ns(offset_ns: u64) -> Self {
	        let nsec = Bounded::<u32, 29>::from_expr(
	            ((offset_ns >> 32) & 0x1fff_ffff) as u32,
	        );
	
	        Self::zeroed().with_nsec(nsec)
	    }
	}
	
> +    // FSP PRI space; the timer is its only consumer so far.
> +    register! {
> +        base: NovaRegisters;
> +
> +        /// FSP ucode version, encoded as `major.minor` in bits 11:8 and 7:0, written by the FSP
> +        /// during boot. (`NV_PFSP_FALCON_COMMON_SCRATCH_GROUP_3(1)` in the hardware headers.)
> +        pub(crate) NV_GFW_FSP_UCODE_VERSION(u32) @ 0x008f0334 {
> +            11:0 full;
> +        }
> +    }
> +}

Hm...I feel like it should go into fsp/regs.rs regardless.