[PATCH v4 0/2] nova-core: add initial support for gpu timer (v4)

[PATCH v4 0/2] nova-core: add initial support for gpu timer (v4)

[Dave Airlie via B4 Relay]

The GPU has a timer, one of the pieces of information
we need to pass into GSP init later is the timer offset.
    
This series just adds support for setting up the timer
on Turing+, with hals for Turing and Hopper.
    
Right now it calculates the value to send to GSP but
the code to send it will be added later.
    
For some hardware variants sending a proper value
is required for GSP init to succeed.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
Changes in v4:
- reworked register handling
- used a timerstate.
- Link to v3: https://lore.kernel.org/r/20261103-nova-timer-v3-0-0cf9bbae8fd9@redhat.com

---
Dave Airlie (2):
      rust: time: expose as_nanos for clock sources.
      gpu: nova-core: add support for GPU timer block. (v4)

 drivers/gpu/nova-core/fsp.rs             |  5 +-
 drivers/gpu/nova-core/fsp/regs.rs        | 30 ++++++++++++
 drivers/gpu/nova-core/gpu.rs             |  8 ++++
 drivers/gpu/nova-core/nova_core.rs       |  1 +
 drivers/gpu/nova-core/timer.rs           | 33 +++++++++++++
 drivers/gpu/nova-core/timer/hal.rs       | 27 +++++++++++
 drivers/gpu/nova-core/timer/hal/gh100.rs | 79 ++++++++++++++++++++++++++++++
 drivers/gpu/nova-core/timer/hal/tu102.rs | 53 +++++++++++++++++++++
 drivers/gpu/nova-core/timer/regs.rs      | 82 ++++++++++++++++++++++++++++++++
 rust/kernel/time.rs                      |  3 +-
 10 files changed, 318 insertions(+), 3 deletions(-)
---
base-commit: 3497a403264b094ebd61b8b755165d56e7d9dc37
change-id: 20261102-nova-timer-f4b9de3274a1

Best regards,
-- 
Dave Airlie <airlied@redhat.com>

[PATCH v4 1/2] rust: time: expose as_nanos for clock sources.

[Dave Airlie via B4 Relay]

From: Dave Airlie <airlied@redhat.com>

The nouveau C driver does ktime_to_ns(ktime_get()) to program
the cpu ns into some hw/fw, so allow this method for rust drivers.

Reviewed-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 rust/kernel/time.rs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 6c0a5e8090d0..a91ddf04785b 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -211,8 +211,9 @@ pub fn elapsed(&self) -> Delta {
         Self::now() - *self
     }
 
+    /// Return the raw nanoseconds values since the epoch of clock `C`.
     #[inline]
-    pub(crate) fn as_nanos(&self) -> i64 {
+    pub fn as_nanos(&self) -> i64 {
         self.inner
     }
 

-- 
2.56.0

Re: [PATCH v4 1/2] rust: time: expose as_nanos for clock sources.

[Alexandre Courbot]

On Thu Nov 5, 2026 at 12:07 PM JST, Dave Airlie via B4 Relay wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> The nouveau C driver does ktime_to_ns(ktime_get()) to program
> the cpu ns into some hw/fw, so allow this method for rust drivers.

This sounds a bit like we are making this public for Nouveau. Maybe just
say it is needed for the next patch?

Regardless,

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

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

[Dave Airlie via B4 Relay]

From: Dave Airlie <airlied@redhat.com>

This is needed to calculate the system timer offset that is passed
to GSP init.

2 HALs exist:

a) tu102 is the gv100 path from open-gpu, the priv level is checked
and the cpu time is written to the gpu timer. no offset is used.

b) gh100 as two variants, gh100 and gb100.
This is the gh100 path from open-gpu, where the secure system
timer is read, the offset is calculated.
On gh100 which need a work around we avoid sending it to registers
other we write the offset to registers.
The gb100 path just avoids the war.
The value is also stored to send to GSP.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/nova-core/fsp.rs             |  5 +-
 drivers/gpu/nova-core/fsp/regs.rs        | 30 ++++++++++++
 drivers/gpu/nova-core/gpu.rs             |  8 ++++
 drivers/gpu/nova-core/nova_core.rs       |  1 +
 drivers/gpu/nova-core/timer.rs           | 33 +++++++++++++
 drivers/gpu/nova-core/timer/hal.rs       | 27 +++++++++++
 drivers/gpu/nova-core/timer/hal/gh100.rs | 79 ++++++++++++++++++++++++++++++
 drivers/gpu/nova-core/timer/hal/tu102.rs | 53 +++++++++++++++++++++
 drivers/gpu/nova-core/timer/regs.rs      | 82 ++++++++++++++++++++++++++++++++
 9 files changed, 316 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/nova-core/fsp.rs b/drivers/gpu/nova-core/fsp.rs
index b738dcabcdef..222b3dbaca47 100644
--- a/drivers/gpu/nova-core/fsp.rs
+++ b/drivers/gpu/nova-core/fsp.rs
@@ -51,10 +51,11 @@
         NvdmType, //
     },
     num,
-    regs, //
+    regs as bootregs, //
 };
 
 mod hal;
+pub(crate) mod regs;
 
 /// PRC message sub-command.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
@@ -429,7 +430,7 @@ fn wait_secure_boot(
 
         read_poll_timeout(
             || Ok(hal.fsp_boot_status(bar)),
-            |&status| status == regs::NV_THERM_I2CS_SCRATCH_FSP_BOOT_COMPLETE_STATUS_SUCCESS,
+            |&status| status == bootregs::NV_THERM_I2CS_SCRATCH_FSP_BOOT_COMPLETE_STATUS_SUCCESS,
             Delta::from_millis(10),
             Delta::from_millis(FSP_SECURE_BOOT_TIMEOUT_MS),
         )
diff --git a/drivers/gpu/nova-core/fsp/regs.rs b/drivers/gpu/nova-core/fsp/regs.rs
new file mode 100644
index 000000000000..d4b8482a6854
--- /dev/null
+++ b/drivers/gpu/nova-core/fsp/regs.rs
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0
+
+pub(crate) mod gh100 {
+    use kernel::io::register;
+
+    use crate::{
+        driver::NovaRegisters,
+    };
+
+    // 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;
+        }
+    }
+
+    impl NV_GFW_FSP_UCODE_VERSION {
+        /// Check if the hopper only workaround is required
+        pub(crate) fn is_bug3957833_affected(self) -> bool {
+            /// FSP ucode version from which on the fix for bug 3957833 is included.
+            const FSP_BUG_3957833_FIX_VERSION: u32 = 0x44c;
+
+            self.full().get() < FSP_BUG_3957833_FIX_VERSION
+        }
+    }
+}
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 4e59072cab50..99b6e33aa243 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -53,6 +53,10 @@
         GpuMm,
         VramAddress, //
     },
+    timer::{
+        Timer,
+        TimerState, //
+    },
     vgpu::VgpuManager, //
 };
 
@@ -331,6 +335,8 @@ struct GspResources<'gpu> {
     // TODO: use different resource types for each boot method, and make the relevant Gsp methods
     // generic against them.
     fsp: Option<Fsp<'gpu>>,
+    /// Timer state - contains system timer offset
+    timer: TimerState,
     /// vGPU state detected before GSP boot.
     vgpu: VgpuManager,
     /// GSP runtime data.
@@ -497,6 +503,8 @@ pub(crate) fn new<'a>(
 
                 fsp: Fsp::try_new(dev, bar, spec.chipset)?,
 
+                timer: Timer::init(bar, spec.chipset)?,
+
                 vgpu: VgpuManager::new(pdev, spec.chipset, fsp.as_mut()),
 
                 gsp <- Gsp::new(pdev, bar),
diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
index a8f6dc22057b..ee59d81e5105 100644
--- a/drivers/gpu/nova-core/nova_core.rs
+++ b/drivers/gpu/nova-core/nova_core.rs
@@ -27,6 +27,7 @@
 mod sbuffer;
 #[cfg(CONFIG_NOVA_CORE_SELFTESTS)]
 mod selftest;
+mod timer;
 mod vbios;
 mod vgpu;
 
diff --git a/drivers/gpu/nova-core/timer.rs b/drivers/gpu/nova-core/timer.rs
new file mode 100644
index 000000000000..e7ad37806e5b
--- /dev/null
+++ b/drivers/gpu/nova-core/timer.rs
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0
+
+mod hal;
+mod regs;
+
+use kernel::{
+    prelude::*, //
+};
+
+use crate::{
+    driver::Bar0,
+    gpu::Chipset, //
+};
+
+pub(crate) struct Timer;
+
+// struct that just holds the sys timer offset
+// after initialising the GPU timer block.
+pub(crate) struct TimerState {
+    /// This value is to be passed to GspInit SYS_TIMER_OFFSET_NS
+    /// It is considered an opaque value, passed from timer hal to GSP.
+    #[allow(dead_code)]
+    sys_timer_offset_ns: u64,
+}
+
+impl Timer {
+    /// Initialize the GPU timer.
+    pub(crate) fn init(bar: Bar0<'_>, chipset: Chipset) -> Result<TimerState> {
+        Ok(TimerState {
+            sys_timer_offset_ns: hal::timer_hal(chipset).init(bar)?,
+        })
+    }
+}
diff --git a/drivers/gpu/nova-core/timer/hal.rs b/drivers/gpu/nova-core/timer/hal.rs
new file mode 100644
index 000000000000..b78066bf9a0d
--- /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
+    fn init(&self, bar: Bar0<'_>) -> Result<u64>;
+}
+
+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..0486df588964
--- /dev/null
+++ b/drivers/gpu/nova-core/timer/hal/gh100.rs
@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use super::TimerHal;
+
+use kernel::{
+    io::Io,
+    prelude::*,
+    time::*, //
+};
+
+use crate::{
+    driver::Bar0,
+    fsp::regs as fspregs,
+    timer::regs, //
+};
+
+/// Timer HAL for Hopper and Blackwell.
+struct Gh100 {
+    /// `true` on Hopper, FSP ucode on newer GPUs always has the fix.
+    fsp_bug3957833_war: bool,
+}
+
+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. Can only be true
+    /// on GH100 with older FSP.
+    fn requires_bug3957833_war(&self, bar: Bar0<'_>) -> bool {
+        self.fsp_bug3957833_war
+            && bar.read(fspregs::gh100::NV_GFW_FSP_UCODE_VERSION).is_bug3957833_affected()
+    }
+
+    fn timer_time(&self, bar: Bar0<'_>, time_ns: u64) -> Result<u64> {
+        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.
+        let sys_timer_offset_ns = time_ns.checked_sub(sec_timer_ns).ok_or(EINVAL)?;
+
+        if !self.requires_bug3957833_war(bar) {
+            bar.write_reg(
+                regs::gh100::NV_PGC6_SCI_SYS_TIMER_OFFSET_1::try_from_nsec(sys_timer_offset_ns)?
+            );
+            bar.write_reg(
+                regs::gh100::NV_PGC6_SCI_SYS_TIMER_OFFSET_0::try_from_nsec(sys_timer_offset_ns)?
+                    .with_update(true),
+            );
+        }
+        Ok(sys_timer_offset_ns)
+    }
+}
+
+impl TimerHal for Gh100 {
+    fn init(&self, bar: Bar0<'_>) -> Result<u64> {
+        // The open drivers uses wall clock time here not monotonic
+        let sys_time_ns = Instant::<RealTime>::now().as_nanos();
+
+        self.timer_time(bar, sys_time_ns as u64)
+    }
+}
+
+const GH100: Gh100 = Gh100 {
+    fsp_bug3957833_war: true,
+};
+const GB100: Gh100 = Gh100 {
+    fsp_bug3957833_war: false,
+};
+
+pub(super) const GH100_HAL: &dyn TimerHal = &GH100;
+pub(super) const GB100_HAL: &dyn TimerHal = &GB100;
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..c0777b777362
--- /dev/null
+++ b/drivers/gpu/nova-core/timer/hal/tu102.rs
@@ -0,0 +1,53 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use super::TimerHal;
+
+use kernel::{
+    io::Io,
+    prelude::*,
+    time::*, //
+};
+
+use crate::{
+    driver::Bar0,
+    timer::regs, //
+};
+struct Tu102;
+
+// this code will be used in the gb10b hal eventually
+#[expect(dead_code)]
+fn timer_read(bar: Bar0<'_>) -> u64 {
+    let mut hi2: u32 = u32::from(bar.read(regs::NV_PTIMER_TIME_1));
+    loop {
+        let lo = u32::from(bar.read(regs::NV_PTIMER_TIME_0));
+        let hi = u32::from(bar.read(regs::NV_PTIMER_TIME_1));
+        if hi == hi2 {
+            return u64::from(hi) << 32 | u64::from(lo);
+        }
+        hi2 = u32::from(bar.read(regs::NV_PTIMER_TIME_1));
+    }
+}
+
+fn timer_time(bar: Bar0<'_>, time_ns: u64) -> Result {
+    if bar.read(regs::NV_PTIMER_TIME_PRIV_LEVEL_MASK).level0() {
+        bar.write_reg(regs::NV_PTIMER_TIME_1::try_from_nsec(time_ns)?);
+        bar.write_reg(regs::NV_PTIMER_TIME_0::try_from_nsec(time_ns)?);
+        Ok(())
+    } else {
+        Err(EINVAL)
+    }
+}
+
+impl TimerHal for Tu102 {
+    fn init(&self, bar: Bar0<'_>) -> Result<u64> {
+        // The RM uses wall clock time here (osGetSystemTime -> ktime_get_real), so the GPU time
+        // matches the host's real time, e.g. for timestamp correlation in userspace.
+        let sys_time_ns = Instant::<RealTime>::now().as_nanos();
+
+        timer_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..3de7174f20db
--- /dev/null
+++ b/drivers/gpu/nova-core/timer/regs.rs
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use kernel::io::register;
+use kernel::prelude::*;
+
+use crate::{
+    driver::NovaRegisters,
+};
+use pin_init::Zeroable;
+
+// 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;
+    }
+}
+
+impl NV_PTIMER_TIME_0 {
+    pub(crate) fn try_from_nsec(nsec: u64) -> Result<Self> {
+        Ok(Self::zeroed().try_with_nsec(((nsec >> 5) & 0x7FF_FFFF) as u32)?)
+    }
+}
+
+impl NV_PTIMER_TIME_1 {
+    pub(crate) fn try_from_nsec(nsec: u64) -> Result<Self> {
+        Ok(Self::zeroed().try_with_nsec(((nsec >> 32) & 0x1FFF_FFFF) as u32)?)
+    }
+}
+
+pub(crate) mod gh100 {
+    use kernel::io::register;
+    use kernel::prelude::*;
+
+    use crate::driver::NovaRegisters;
+    use pin_init::Zeroable;
+
+    // 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;
+        }
+
+    }
+
+    impl NV_PGC6_SCI_SYS_TIMER_OFFSET_0 {
+        pub(crate) fn try_from_nsec(nsec: u64) -> Result<Self> {
+            Ok(Self::zeroed().try_with_nsec(((nsec >> 5) & 0x7FF_FFFF) as u32)?)
+        }
+    }
+
+    impl NV_PGC6_SCI_SYS_TIMER_OFFSET_1 {
+        pub(crate) fn try_from_nsec(nsec: u64) -> Result<Self> {
+            Ok(Self::zeroed().try_with_nsec(((nsec >> 32) & 0x1FFF_FFFF) as u32)?)
+        }
+    }
+}

-- 
2.56.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] gpu: nova-core: u64::checked_sub() fails on negative offset in timer_time()
--

commit 8e6e7110b2947fca24843940e52d712c197357df
Author: Dave Airlie <airlied@redhat.com>

gpu: nova-core: add support for GPU timer block. (v4)

This commit adds GPU timer block initialization support for nova-core across
Turing/Ampere/Ada (tu102 HAL) and Hopper/Blackwell (gh100/gb100 HALs) to
compute the system timer offset passed to GSP initialization.

> 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 0000000000000..0486df5889647
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer/hal/gh100.rs
> @@ -0,0 +1,79 @@

[ ... ]

> +    fn timer_time(&self, bar: Bar0<'_>, time_ns: u64) -> Result<u64> {
> +        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.
> +        let sys_timer_offset_ns = time_ns.checked_sub(sec_timer_ns).ok_or(EINVAL)?;

[Severity: High]
In Gh100::timer_time(), both time_ns and sec_timer_ns are u64 values. When
the difference is negative (time_ns < sec_timer_ns), u64::checked_sub()
returns None, causing Gh100::timer_time() to return Err(EINVAL) and abort
Gpu::new() -> Timer::init() -> Gh100::init() during probe.

Since the comment notes that the difference can be negative and both operands
are less than 2^62, should this use wrapping_sub() (or signed subtraction)
instead of checked_sub()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-nova-timer-v4-0-45a7c121d4de@redhat.com?part=2

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

[Alexandre Courbot]

On Thu Nov 5, 2026 at 12:07 PM JST, Dave Airlie via B4 Relay wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> This is needed to calculate the system timer offset that is passed
> to GSP init.
>
> 2 HALs exist:
>
> a) tu102 is the gv100 path from open-gpu, the priv level is checked
> and the cpu time is written to the gpu timer. no offset is used.
>
> b) gh100 as two variants, gh100 and gb100.
> This is the gh100 path from open-gpu, where the secure system
> timer is read, the offset is calculated.
> On gh100 which need a work around we avoid sending it to registers
> other we write the offset to registers.
> The gb100 path just avoids the war.
> The value is also stored to send to GSP.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>

There are a couple of Clippy issues with this patch, please make sure
to build with `CLIPPY=1`. `make rustfmt` also reformatted a few lines.

<...>
> diff --git a/drivers/gpu/nova-core/fsp/regs.rs b/drivers/gpu/nova-core/fsp/regs.rs
> new file mode 100644
> index 000000000000..d4b8482a6854
> --- /dev/null
> +++ b/drivers/gpu/nova-core/fsp/regs.rs
> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +pub(crate) mod gh100 {

FSP only makes sense for GH100 and more recent, so I guess we can do
without this sub-module?

> +    use kernel::io::register;
> +
> +    use crate::{
> +        driver::NovaRegisters,
> +    };
> +
> +    // FSP PRI space; the timer is its only consumer so far.

I guess we don't need to mention who the consumer is here; especially
since the list might increase in the future.

> +    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 {

We are trying to keep registers (at least those defined within
sub-modules) local to their owning module, so ideally the visibility
should be `pub(super)`. And instead of accessing the register directly
in `timer`, a `Fsp::is_bug3957833_affected()` method would expose the
functionality we need.

> +            11:0 full;
> +        }
> +    }
> +
> +    impl NV_GFW_FSP_UCODE_VERSION {
> +        /// Check if the hopper only workaround is required
> +        pub(crate) fn is_bug3957833_affected(self) -> bool {

So as mentioned above, this method would be moved to a method of `Fsp`
that `timer` would call, keeping registers local to their module.

We will need to store the hal and `Chipset` into the `Fsp` struct for
that, but that's standard practice.

> +            /// FSP ucode version from which on the fix for bug 3957833 is included.
> +            const FSP_BUG_3957833_FIX_VERSION: u32 = 0x44c;
> +
> +            self.full().get() < FSP_BUG_3957833_FIX_VERSION

You can omit the `.get()` here, there are comparison operators
implementations between `Bounded` and regular integer types.

<...>
> diff --git a/drivers/gpu/nova-core/timer.rs b/drivers/gpu/nova-core/timer.rs
> new file mode 100644
> index 000000000000..e7ad37806e5b
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer.rs
> @@ -0,0 +1,33 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +mod hal;
> +mod regs;
> +
> +use kernel::{
> +    prelude::*, //
> +};
> +
> +use crate::{
> +    driver::Bar0,
> +    gpu::Chipset, //
> +};
> +
> +pub(crate) struct Timer;
> +
> +// struct that just holds the sys timer offset
> +// after initialising the GPU timer block.

Can this be a regular Rust doccomment? If possible describing its
purpose, not its contents.

> +pub(crate) struct TimerState {
> +    /// This value is to be passed to GspInit SYS_TIMER_OFFSET_NS
> +    /// It is considered an opaque value, passed from timer hal to GSP.
> +    #[allow(dead_code)]
> +    sys_timer_offset_ns: u64,

I assume a future patch will need to read this value, so we can
preemptively make it `pub(crate)`.

> +}
> +
> +impl Timer {
> +    /// Initialize the GPU timer.
> +    pub(crate) fn init(bar: Bar0<'_>, chipset: Chipset) -> Result<TimerState> {
> +        Ok(TimerState {
> +            sys_timer_offset_ns: hal::timer_hal(chipset).init(bar)?,
> +        })
> +    }
> +}
> diff --git a/drivers/gpu/nova-core/timer/hal.rs b/drivers/gpu/nova-core/timer/hal.rs
> new file mode 100644
> index 000000000000..b78066bf9a0d
> --- /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: capitalization.

Maybe also explain that this can program some timer-related registers,
otherwise the name `init` isn't justified for someone who believes that
this just returns the time without any side-effect.

> +    fn init(&self, bar: Bar0<'_>) -> Result<u64>;
> +}
> +
> +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..0486df588964
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer/hal/gh100.rs
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use super::TimerHal;
> +
> +use kernel::{
> +    io::Io,
> +    prelude::*,
> +    time::*, //
> +};
> +
> +use crate::{
> +    driver::Bar0,
> +    fsp::regs as fspregs,
> +    timer::regs, //
> +};
> +
> +/// Timer HAL for Hopper and Blackwell.
> +struct Gh100 {
> +    /// `true` on Hopper, FSP ucode on newer GPUs always has the fix.

This comment doesn't parse for me - can we simplify it?

> +    fsp_bug3957833_war: bool,
> +}
> +
> +impl Gh100 {
> +    /// kfspRequiresBug3957833WAR_HAL: returns `true` if the FSP ucode predates the fix for

If we name OpenRM methods, let's state that they come from OpenRM as that
name might be confusing to an unfamiliar reader.

> +    /// bug 3957833 and hence the sys timer offset must not be programmed. Can only be true
> +    /// on GH100 with older FSP.
> +    fn requires_bug3957833_war(&self, bar: Bar0<'_>) -> bool {
> +        self.fsp_bug3957833_war
> +            && bar.read(fspregs::gh100::NV_GFW_FSP_UCODE_VERSION).is_bug3957833_affected()

With the change suggested above we would call something like
`Fsp::is_bug3957833_affected()`.

> +    }
> +
> +    fn timer_time(&self, bar: Bar0<'_>, time_ns: u64) -> Result<u64> {
> +        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));
> +        }

Looks like this mirrors the `timer_read` function of `tu102`, can we
move it into its own method for symmetry?

Note that you don't need to wrap everything in `u32::from` - register
values can be compared directly. You'll just need to access `.nsec()`
when building `sec_timer_ns`, and shift `sec_lo.nsec()` by `5` (or
better, `NV_PGC6_SCI_SEC_TIMER_TIME_0::NSEC_SHIFT`) to get
the actual value. Usually when we have a value that needs to be shifted
we add a convenience method to the register type directly, that probably
applies here as well.

Then if you bulk-import `regs::gh100::*`, you can also shorten every
line that accesses a register.

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

This comment doesn't look right, both operands are `u64` and IIUC this
*will* underflow if `sec_timer_ns > time_ns`.

I'd ignore Sashiko's suggestion as it looks like it is trying to
reconcile the code against the comment instead of the other way around.

> +        let sys_timer_offset_ns = time_ns.checked_sub(sec_timer_ns).ok_or(EINVAL)?;
> +
> +        if !self.requires_bug3957833_war(bar) {
> +            bar.write_reg(
> +                regs::gh100::NV_PGC6_SCI_SYS_TIMER_OFFSET_1::try_from_nsec(sys_timer_offset_ns)?
> +            );
> +            bar.write_reg(
> +                regs::gh100::NV_PGC6_SCI_SYS_TIMER_OFFSET_0::try_from_nsec(sys_timer_offset_ns)?
> +                    .with_update(true),
> +            );
> +        }
> +        Ok(sys_timer_offset_ns)

Once we have moved the timer read loop into its own method, I guess the
remainder can be nicely folded into `init` directly, and we don't need
`timer_time` anymore?

> +    }
> +}
> +
> +impl TimerHal for Gh100 {
> +    fn init(&self, bar: Bar0<'_>) -> Result<u64> {
> +        // The open drivers uses wall clock time here not monotonic
> +        let sys_time_ns = Instant::<RealTime>::now().as_nanos();
> +
> +        self.timer_time(bar, sys_time_ns as u64)
> +    }
> +}
> +
> +const GH100: Gh100 = Gh100 {
> +    fsp_bug3957833_war: true,
> +};
> +const GB100: Gh100 = Gh100 {
> +    fsp_bug3957833_war: false,
> +};
> +
> +pub(super) const GH100_HAL: &dyn TimerHal = &GH100;
> +pub(super) const GB100_HAL: &dyn TimerHal = &GB100;

We prefer to keep each HAL in a module bearing the same name, so `GB100`
should be in `gb100.rs`.

But if we move `is_bug3957833_affected` into Fsp, then both the
`fsp_bug3957833_war` flag and `GB100` HAL can go away. `Timer::init`
will just need to be passed an `Option<&Fsp>`, which it passes down to
the HAL method so it can query it (similarly to what `VgpuManager::new`
does - only with `as_ref()` instead of `as_mut()`).

> 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..c0777b777362
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer/hal/tu102.rs
> @@ -0,0 +1,53 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use super::TimerHal;
> +
> +use kernel::{
> +    io::Io,
> +    prelude::*,
> +    time::*, //
> +};
> +
> +use crate::{
> +    driver::Bar0,
> +    timer::regs, //
> +};
> +struct Tu102;
> +
> +// this code will be used in the gb10b hal eventually
> +#[expect(dead_code)]
> +fn timer_read(bar: Bar0<'_>) -> u64 {
> +    let mut hi2: u32 = u32::from(bar.read(regs::NV_PTIMER_TIME_1));
> +    loop {
> +        let lo = u32::from(bar.read(regs::NV_PTIMER_TIME_0));
> +        let hi = u32::from(bar.read(regs::NV_PTIMER_TIME_1));
> +        if hi == hi2 {
> +            return u64::from(hi) << 32 | u64::from(lo);
> +        }
> +        hi2 = u32::from(bar.read(regs::NV_PTIMER_TIME_1));
> +    }
> +}

Here as well you can remove the `u32::from` easily.

> +
> +fn timer_time(bar: Bar0<'_>, time_ns: u64) -> Result {
> +    if bar.read(regs::NV_PTIMER_TIME_PRIV_LEVEL_MASK).level0() {
> +        bar.write_reg(regs::NV_PTIMER_TIME_1::try_from_nsec(time_ns)?);
> +        bar.write_reg(regs::NV_PTIMER_TIME_0::try_from_nsec(time_ns)?);
> +        Ok(())
> +    } else {
> +        Err(EINVAL)
> +    }
> +}
> +
> +impl TimerHal for Tu102 {
> +    fn init(&self, bar: Bar0<'_>) -> Result<u64> {
> +        // The RM uses wall clock time here (osGetSystemTime -> ktime_get_real), so the GPU time
> +        // matches the host's real time, e.g. for timestamp correlation in userspace.
> +        let sys_time_ns = Instant::<RealTime>::now().as_nanos();
> +
> +        timer_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..3de7174f20db
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer/regs.rs
> @@ -0,0 +1,82 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use kernel::io::register;
> +use kernel::prelude::*;
> +
> +use crate::{
> +    driver::NovaRegisters,
> +};
> +use pin_init::Zeroable;
> +
> +// PTIMER
> +
> +register! {
> +    base: NovaRegisters;
> +
> +    pub(crate) NV_PTIMER_TIME_PRIV_LEVEL_MASK(u32) @ 0x00009430 {

All the definitions of this module can be `pub(super)`

> +        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;
> +    }
> +}
> +
> +impl NV_PTIMER_TIME_0 {
> +    pub(crate) fn try_from_nsec(nsec: u64) -> Result<Self> {
> +        Ok(Self::zeroed().try_with_nsec(((nsec >> 5) & 0x7FF_FFFF) as u32)?)
> +    }

Clippy pointed out that you can get rid of the `Ok()` because
`try_with_nsec` already returns the expected type.

There are 2 operations that silently truncate the passed value - the
right shift (which we cannot avoid), and the mask. We should document
these, or maybe take a `u32` to leave that responsibility to the caller
so we only need to mention the truncated 5 LSBs.

With that this method can actually become infallible, because `nsec`
covers the full u32 range. We just need to construct the expected
`Bounded` value, which looks something like this:

    pub(super) fn from_nsec(nsec: u32) -> Self {
        let nsec = Bounded::<u32, 32>::from(nsec).shr::<{ Self::NSEC_SHIFT }, _>();

        Self::zeroed().with_nsec(nsec)
    }

The `shr` method on `Bounded` reduces its number of valid bits, turning
it into exactly the type expected for `nsec`, so we don't need to use
the fallible `try_with_nsec` method. Also note that I have used the
constant shift that is automatically defined for each bitfield instead
of the immediate value `5`.

It's a bit of a mouthful but it is demonstrably correct and doesn't rely
on any truncating operation (notably `as` that we try to avoid whenever
possible).

> +}
> +
> +impl NV_PTIMER_TIME_1 {
> +    pub(crate) fn try_from_nsec(nsec: u64) -> Result<Self> {

Here as well we should document that this works with only the 32 MSBs
(or, again, take a `u32` and leave that to the caller).

> +        Ok(Self::zeroed().try_with_nsec(((nsec >> 32) & 0x1FFF_FFFF) as u32)?)

You can just do

    Self::zeroed().try_with_nsec(nsec >> 32)

`try_with_nsec` is happy to take a u64, and here I expect that we want
to fail if the value is larger than what the registers pair can
represent? Otherwise this method should also be infallible, as the call
to `try_with_nsec` cannot technically fail with the mask you applied
anyway.

> +    }
> +}
> +
> +pub(crate) mod gh100 {
> +    use kernel::io::register;
> +    use kernel::prelude::*;
> +
> +    use crate::driver::NovaRegisters;
> +    use pin_init::Zeroable;
> +
> +    // PGC6/TIMER
> +    register! {
> +        base: NovaRegisters;
> +
> +        pub(crate) NV_PGC6_SCI_SEC_TIMER_TIME_0(u32) @ 0x00118f54 {

Since we are two modules down, instead of `pub(super)` this will need to
be `pub(in crate::timer)`.

> +            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;
> +        }
> +
> +    }
> +
> +    impl NV_PGC6_SCI_SYS_TIMER_OFFSET_0 {
> +        pub(crate) fn try_from_nsec(nsec: u64) -> Result<Self> {

As for `NV_PTIMER_TIME_0` this method can be infallible. Actually it's
an exact mirror...

> +            Ok(Self::zeroed().try_with_nsec(((nsec >> 5) & 0x7FF_FFFF) as u32)?)

... so we can leverage its sibling like this:

    Self::zeroed().with_nsec(NV_PTIMER_TIME_0::from_nsec(nsec).nsec())

(this just leverages the NV_PTIMER_TIME_0 method to build a value,
extract the field, and write it as-is.)

> +        }
> +    }
> +
> +    impl NV_PGC6_SCI_SYS_TIMER_OFFSET_1 {
> +        pub(crate) fn try_from_nsec(nsec: u64) -> Result<Self> {
> +            Ok(Self::zeroed().try_with_nsec(((nsec >> 32) & 0x1FFF_FFFF) as u32)?)

and here as well:

    NV_PTIMER_TIME_1::try_from_nsec(nsec).map(|v| Self::zeroed().with_nsec(v.nsec()))