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

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

[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 v5:
- Alexandre's review comments handled
- reworked registers/workaround logic into fsp
- fixed all the timer register handling to be non-falling.
- Link to v4: https://lore.kernel.org/r/20261105-nova-timer-v4-0-45a7c121d4de@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. (v5)

 drivers/gpu/nova-core/fsp.rs             | 19 ++++++-
 drivers/gpu/nova-core/fsp/hal.rs         |  5 ++
 drivers/gpu/nova-core/fsp/hal/gh100.rs   |  6 ++
 drivers/gpu/nova-core/fsp/regs.rs        | 28 +++++++++
 drivers/gpu/nova-core/gpu.rs             |  8 +++
 drivers/gpu/nova-core/nova_core.rs       |  1 +
 drivers/gpu/nova-core/timer.rs           | 38 +++++++++++++
 drivers/gpu/nova-core/timer/hal.rs       | 31 ++++++++++
 drivers/gpu/nova-core/timer/hal/gh100.rs | 62 ++++++++++++++++++++
 drivers/gpu/nova-core/timer/hal/tu102.rs | 53 +++++++++++++++++
 drivers/gpu/nova-core/timer/regs.rs      | 98 ++++++++++++++++++++++++++++++++
 rust/kernel/time.rs                      |  3 +-
 12 files changed, 348 insertions(+), 4 deletions(-)
---
base-commit: 7a6bc132f1b51437063c9203e7221067b40a791e
change-id: 20261102-nova-timer-f4b9de3274a1

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

[PATCH v5 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.

This is needed for following nova patch.

Reviewed-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Reviewed-by: Alexandre Courbot <acourbot@nvidia.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 b6a2aab12a50..4409291f3ffa 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -215,8 +215,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

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

[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 is for hopper+
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 to avoid sending it to registers
if the FSP firmware is too old.
Otherwise we write the offset to the offset registers.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/nova-core/fsp.rs             | 19 ++++++-
 drivers/gpu/nova-core/fsp/hal.rs         |  5 ++
 drivers/gpu/nova-core/fsp/hal/gh100.rs   |  6 ++
 drivers/gpu/nova-core/fsp/regs.rs        | 28 +++++++++
 drivers/gpu/nova-core/gpu.rs             |  8 +++
 drivers/gpu/nova-core/nova_core.rs       |  1 +
 drivers/gpu/nova-core/timer.rs           | 38 +++++++++++++
 drivers/gpu/nova-core/timer/hal.rs       | 31 ++++++++++
 drivers/gpu/nova-core/timer/hal/gh100.rs | 62 ++++++++++++++++++++
 drivers/gpu/nova-core/timer/hal/tu102.rs | 53 +++++++++++++++++
 drivers/gpu/nova-core/timer/regs.rs      | 98 ++++++++++++++++++++++++++++++++
 11 files changed, 346 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/nova-core/fsp.rs b/drivers/gpu/nova-core/fsp.rs
index 3a4a10487f33..a2ea50465a55 100644
--- a/drivers/gpu/nova-core/fsp.rs
+++ b/drivers/gpu/nova-core/fsp.rs
@@ -50,10 +50,11 @@
         NvdmHeader,
         NvdmType, //
     },
-    regs, //
+    regs as bootregs, //
 };
 
 mod hal;
+mod regs;
 
 /// PRC message sub-command.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
@@ -389,6 +390,7 @@ pub(crate) fn boot_params(&self) -> &Coherent<'_, GspFmcBootParams> {
 pub(crate) struct Fsp<'a> {
     falcon: Falcon<'a, FspEngine>,
     fmc_fw: GspFmcFirmware<'a>,
+    chipset: Chipset,
 }
 
 impl<'a> Fsp<'a> {
@@ -428,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),
         )
@@ -436,7 +438,11 @@ fn wait_secure_boot(
             dev_err!(dev, "FSP secure boot completion error: {:?}\n", e);
         })?;
 
-        Ok(Fsp { falcon, fmc_fw })
+        Ok(Fsp {
+            falcon,
+            fmc_fw,
+            chipset,
+        })
     }
 
     /// Sends a message to FSP and waits for the response.
@@ -549,4 +555,11 @@ pub(crate) fn boot_fmc(
         dev_dbg!(dev, "FSP Chain of Trust completed successfully\n");
         Ok(())
     }
+
+    pub(crate) fn is_bug_3957833_affected(&self, bar: Bar0<'_>) -> bool {
+        match hal::fsp_hal(self.chipset) {
+            Some(hal) => hal.is_bug_3957833_affected(bar),
+            None => false,
+        }
+    }
 }
diff --git a/drivers/gpu/nova-core/fsp/hal.rs b/drivers/gpu/nova-core/fsp/hal.rs
index eaf5837ac5a8..8bcc8f71cf8f 100644
--- a/drivers/gpu/nova-core/fsp/hal.rs
+++ b/drivers/gpu/nova-core/fsp/hal.rs
@@ -23,6 +23,11 @@ pub(super) trait FspHal {
     // TODO: consider moving this into the TLV firmware metadata when ready
     /// Returns the size reserved at the end of the framebuffer, in bytes.
     fn fb_end_reserved_size(&self) -> u64;
+
+    /// Return if the bug 3957833 is affected
+    fn is_bug_3957833_affected(&self, _bar: Bar0<'_>) -> bool {
+        false
+    }
 }
 
 /// Returns the FSP HAL, or `None` if the architecture doesn't support FSP.
diff --git a/drivers/gpu/nova-core/fsp/hal/gh100.rs b/drivers/gpu/nova-core/fsp/hal/gh100.rs
index 9a8563799da8..538870566b8c 100644
--- a/drivers/gpu/nova-core/fsp/hal/gh100.rs
+++ b/drivers/gpu/nova-core/fsp/hal/gh100.rs
@@ -9,6 +9,7 @@
 use crate::{
     driver::Bar0,
     fsp::hal::FspHal,
+    fsp::regs as fspregs,
     regs, //
 };
 
@@ -33,6 +34,11 @@ fn cot_version(&self) -> u16 {
     fn fb_end_reserved_size(&self) -> u64 {
         u64::SZ_2M
     }
+
+    fn is_bug_3957833_affected(&self, bar: Bar0<'_>) -> bool {
+        bar.read(fspregs::NV_GFW_FSP_UCODE_VERSION)
+            .is_bug3957833_affected()
+    }
 }
 
 const GH100: Gh100 = Gh100;
diff --git a/drivers/gpu/nova-core/fsp/regs.rs b/drivers/gpu/nova-core/fsp/regs.rs
new file mode 100644
index 000000000000..de69d5bed427
--- /dev/null
+++ b/drivers/gpu/nova-core/fsp/regs.rs
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use kernel::io::register;
+
+use crate::driver::NovaRegisters;
+
+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(super) NV_GFW_FSP_UCODE_VERSION(u32) @ 0x008f0334 {
+        11:0 full;
+    }
+}
+
+impl NV_GFW_FSP_UCODE_VERSION {
+    /// OpenRM 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.
+    pub(super) 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() < FSP_BUG_3957833_FIX_VERSION
+    }
+}
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index ac5305d7ff5e..cd281460c509 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::setup(bar, spec.chipset, fsp.as_ref())?,
+
                 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..4c329221c674
--- /dev/null
+++ b/drivers/gpu/nova-core/timer.rs
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0
+
+mod hal;
+mod regs;
+
+use kernel::{
+    prelude::*, //
+};
+
+use crate::{
+    driver::Bar0,
+    fsp::Fsp,
+    gpu::Chipset, //
+};
+
+pub(crate) struct Timer;
+
+/// Struct that holds the sys timer offset value
+/// 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.
+    #[expect(dead_code)]
+    pub(crate) sys_timer_offset_ns: u64,
+}
+
+impl Timer {
+    /// Setup the GPU timer hardware
+    pub(crate) fn setup(
+        bar: Bar0<'_>,
+        chipset: Chipset,
+        fsp: Option<&Fsp<'_>>,
+    ) -> Result<TimerState> {
+        Ok(TimerState {
+            sys_timer_offset_ns: hal::timer_hal(chipset).setup(bar, fsp)?,
+        })
+    }
+}
diff --git a/drivers/gpu/nova-core/timer/hal.rs b/drivers/gpu/nova-core/timer/hal.rs
new file mode 100644
index 000000000000..c2f2bbcad890
--- /dev/null
+++ b/drivers/gpu/nova-core/timer/hal.rs
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use kernel::prelude::*;
+
+use crate::{
+    driver::Bar0,
+    fsp::Fsp,
+    gpu::{
+        Architecture,
+        Chipset, //
+    },
+};
+
+mod gh100;
+mod tu102;
+
+pub(super) trait TimerHal {
+    /// Setup the GPU's system timer.
+    ///
+    /// Returns the value to program into GspInitRequest.
+    fn setup(&self, bar: Bar0<'_>, fsp: Option<&Fsp<'_>>) -> 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 | Architecture::BlackwellGB10x | Architecture::BlackwellGB20x => {
+            gh100::GH100_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..49e348c89e52
--- /dev/null
+++ b/drivers/gpu/nova-core/timer/hal/gh100.rs
@@ -0,0 +1,62 @@
+// 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 Hopper and Blackwell.
+struct Gh100;
+
+fn sec_timer_read(bar: Bar0<'_>) -> u64 {
+    let mut hi2 = bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_1);
+    loop {
+        let lo = bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_0);
+        let hi = bar.read(regs::gh100::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(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_1);
+    }
+}
+
+fn sys_timer_offset(bar: Bar0<'_>, fsp: &Fsp<'_>, time_ns: u64) -> Result<u64> {
+    let sec_timer_ns = sec_timer_read(bar);
+
+    let sys_timer_offset_ns = time_ns.wrapping_sub(sec_timer_ns);
+
+    if !fsp.is_bug_3957833_affected(bar) {
+        bar.write_reg(regs::gh100::NV_PGC6_SCI_SYS_TIMER_OFFSET_1::from_nsec(
+            sys_timer_offset_ns,
+        ));
+        bar.write_reg(
+            regs::gh100::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 open drivers 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)?;
+
+        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..629d744a4b12
--- /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,
+    fsp::Fsp,
+    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 = bar.read(regs::NV_PTIMER_TIME_1);
+    loop {
+        let lo = bar.read(regs::NV_PTIMER_TIME_0);
+        let hi = bar.read(regs::NV_PTIMER_TIME_1);
+        if hi == hi2 {
+            return u64::from(u32::from(hi)) << 32 | u64::from(u32::from(lo));
+        }
+        hi2 = bar.read(regs::NV_PTIMER_TIME_1);
+    }
+}
+
+fn set_time(bar: Bar0<'_>, time_ns: u64) -> Result {
+    if !bar.read(regs::NV_PTIMER_TIME_PRIV_LEVEL_MASK).level0() {
+        return Err(EINVAL);
+    }
+    bar.write_reg(regs::NV_PTIMER_TIME_1::from_nsec(time_ns));
+    bar.write_reg(regs::NV_PTIMER_TIME_0::from_nsec(time_ns));
+    Ok(())
+}
+
+impl TimerHal for Tu102 {
+    fn setup(&self, bar: Bar0<'_>, _fsp: Option<&Fsp<'_>>) -> 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();
+
+        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..89dd478a37be
--- /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;
+
+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, _>()
+}
+
+/// 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)
+}
+
+// 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,
+    };
+    use crate::driver::NovaRegisters;
+    use pin_init::Zeroable;
+
+    // PGC6/TIMER
+    register! {
+        base: NovaRegisters;
+
+        pub(in crate::timer) NV_PGC6_SCI_SEC_TIMER_TIME_0(u32) @ 0x00118f54 {
+            31:5 nsec;
+        }
+
+        pub(in crate::timer) NV_PGC6_SCI_SEC_TIMER_TIME_1(u32) @ 0x00118f58 {
+            28:0 nsec;
+        }
+
+        pub(in crate::timer) NV_PGC6_SCI_SYS_TIMER_OFFSET_0(u32) @ 0x00118df4 {
+            0:0 update;
+            31:5 nsec;
+        }
+
+        pub(in crate::timer) NV_PGC6_SCI_SYS_TIMER_OFFSET_1(u32) @ 0x00118df8 {
+            28:0 nsec;
+        }
+
+    }
+
+    impl NV_PGC6_SCI_SYS_TIMER_OFFSET_0 {
+        pub(in crate::timer) fn from_nsec(nsec: u64) -> Self {
+            Self::zeroed().with_nsec(nsec_lo(nsec))
+        }
+    }
+
+    impl NV_PGC6_SCI_SYS_TIMER_OFFSET_1 {
+        pub(in crate::timer) fn from_nsec(nsec: u64) -> Self {
+            Self::zeroed().with_nsec(nsec_hi(nsec))
+        }
+    }
+}

-- 
2.56.0

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

[Alexandre Courbot]

On Fri Nov 6, 2026 at 2:34 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 is for hopper+
> 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 to avoid sending it to registers

nit: "On gh100 a work around is needed to avoid..." maybe?

> if the FSP firmware is too old.
> Otherwise we write the offset to the offset registers.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/nova-core/fsp.rs             | 19 ++++++-
>  drivers/gpu/nova-core/fsp/hal.rs         |  5 ++
>  drivers/gpu/nova-core/fsp/hal/gh100.rs   |  6 ++
>  drivers/gpu/nova-core/fsp/regs.rs        | 28 +++++++++
>  drivers/gpu/nova-core/gpu.rs             |  8 +++
>  drivers/gpu/nova-core/nova_core.rs       |  1 +
>  drivers/gpu/nova-core/timer.rs           | 38 +++++++++++++
>  drivers/gpu/nova-core/timer/hal.rs       | 31 ++++++++++
>  drivers/gpu/nova-core/timer/hal/gh100.rs | 62 ++++++++++++++++++++
>  drivers/gpu/nova-core/timer/hal/tu102.rs | 53 +++++++++++++++++
>  drivers/gpu/nova-core/timer/regs.rs      | 98 ++++++++++++++++++++++++++++++++
>  11 files changed, 346 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/fsp.rs b/drivers/gpu/nova-core/fsp.rs
> index 3a4a10487f33..a2ea50465a55 100644
> --- a/drivers/gpu/nova-core/fsp.rs
> +++ b/drivers/gpu/nova-core/fsp.rs
> @@ -50,10 +50,11 @@
>          NvdmHeader,
>          NvdmType, //
>      },
> -    regs, //
> +    regs as bootregs, //
>  };
>  
>  mod hal;
> +mod regs;
>  
>  /// PRC message sub-command.
>  #[derive(Debug, Clone, Copy, PartialEq, Eq)]
> @@ -389,6 +390,7 @@ pub(crate) fn boot_params(&self) -> &Coherent<'_, GspFmcBootParams> {
>  pub(crate) struct Fsp<'a> {
>      falcon: Falcon<'a, FspEngine>,
>      fmc_fw: GspFmcFirmware<'a>,
> +    chipset: Chipset,
>  }
>  
>  impl<'a> Fsp<'a> {
> @@ -428,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),
>          )
> @@ -436,7 +438,11 @@ fn wait_secure_boot(
>              dev_err!(dev, "FSP secure boot completion error: {:?}\n", e);
>          })?;
>  
> -        Ok(Fsp { falcon, fmc_fw })
> +        Ok(Fsp {
> +            falcon,
> +            fmc_fw,
> +            chipset,
> +        })
>      }
>  
>      /// Sends a message to FSP and waits for the response.
> @@ -549,4 +555,11 @@ pub(crate) fn boot_fmc(
>          dev_dbg!(dev, "FSP Chain of Trust completed successfully\n");
>          Ok(())
>      }
> +
> +    pub(crate) fn is_bug_3957833_affected(&self, bar: Bar0<'_>) -> bool {
> +        match hal::fsp_hal(self.chipset) {
> +            Some(hal) => hal.is_bug_3957833_affected(bar),
> +            None => false,

As I mentioned in my other comment, storing the HAL instead of the
chipset is technically better, but I'm ok if this is not done just for
this single-use method.

> +        }
> +    }
>  }
> diff --git a/drivers/gpu/nova-core/fsp/hal.rs b/drivers/gpu/nova-core/fsp/hal.rs
> index eaf5837ac5a8..8bcc8f71cf8f 100644
> --- a/drivers/gpu/nova-core/fsp/hal.rs
> +++ b/drivers/gpu/nova-core/fsp/hal.rs
> @@ -23,6 +23,11 @@ pub(super) trait FspHal {
>      // TODO: consider moving this into the TLV firmware metadata when ready
>      /// Returns the size reserved at the end of the framebuffer, in bytes.
>      fn fb_end_reserved_size(&self) -> u64;
> +
> +    /// Return if the bug 3957833 is affected
> +    fn is_bug_3957833_affected(&self, _bar: Bar0<'_>) -> bool {
> +        false
> +    }
>  }
>  
>  /// Returns the FSP HAL, or `None` if the architecture doesn't support FSP.
> diff --git a/drivers/gpu/nova-core/fsp/hal/gh100.rs b/drivers/gpu/nova-core/fsp/hal/gh100.rs
> index 9a8563799da8..538870566b8c 100644
> --- a/drivers/gpu/nova-core/fsp/hal/gh100.rs
> +++ b/drivers/gpu/nova-core/fsp/hal/gh100.rs
> @@ -9,6 +9,7 @@
>  use crate::{
>      driver::Bar0,
>      fsp::hal::FspHal,
> +    fsp::regs as fspregs,

Since this is the fsp-local registers module, let's just import
`fsp::regs::*` and use the registers without any module prefix.

>      regs, //
>  };
>  
> @@ -33,6 +34,11 @@ fn cot_version(&self) -> u16 {
>      fn fb_end_reserved_size(&self) -> u64 {
>          u64::SZ_2M
>      }
> +
> +    fn is_bug_3957833_affected(&self, bar: Bar0<'_>) -> bool {
> +        bar.read(fspregs::NV_GFW_FSP_UCODE_VERSION)
> +            .is_bug3957833_affected()

s/is_bug39/is_bug_39

> +    }
>  }
>  
>  const GH100: Gh100 = Gh100;
> diff --git a/drivers/gpu/nova-core/fsp/regs.rs b/drivers/gpu/nova-core/fsp/regs.rs
> new file mode 100644
> index 000000000000..de69d5bed427
> --- /dev/null
> +++ b/drivers/gpu/nova-core/fsp/regs.rs
> @@ -0,0 +1,28 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use kernel::io::register;
> +
> +use crate::driver::NovaRegisters;
> +
> +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(super) NV_GFW_FSP_UCODE_VERSION(u32) @ 0x008f0334 {
> +        11:0 full;
> +    }

I'd like our register definitions to reflect the aliasing of the comment
(which is also represented by OpenRM). So we would start by defining the
actual `NV_PFSP_FALCON_COMMON_SCRATCH_GROUP_3` (which is an array of 4
registers per OpenRM):

    pub(super) NV_PFSP_FALCON_COMMON_SCRATCH_GROUP_3(u32)[4] @ 0x008f0330 {
        31:0 val;
    }

And then define `NV_GFW_FSP_UCODE_VERSION` as an alias to the register
at index 1.

    /// FSP ucode version, written by the FSP during boot.
    pub(super) NV_GFW_FSP_UCODE_VERSION(u32) => NV_PFSP_FALCON_COMMON_SCRATCH_GROUP_3[1] {
        11:0 full;
    }

That way we can reduce the comment to the information that is not
obvious from the declaration. I'd also drop the references to the
non-defined fields, unless you want to add them - it is possible to
define overlapping fields, but since the code doesn't use them we can do
without for now.

<...>
> 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..49e348c89e52
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer/hal/gh100.rs
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use super::TimerHal;
> +
> +use kernel::{
> +    io::Io,
> +    prelude::*,
> +    time::*, //
> +};
> +
> +use crate::{
> +    driver::Bar0,
> +    fsp::Fsp,
> +    timer::regs, //

Here as well, let's bring the whole `timer::regs::gh100::*` since we are
in the GH100 HAL, this will make the code more concise and easier to read.

> +};
> +
> +/// Timer HAL for Hopper and Blackwell.
> +struct Gh100;
> +
> +fn sec_timer_read(bar: Bar0<'_>) -> u64 {
> +    let mut hi2 = bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_1);
> +    loop {
> +        let lo = bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_0);
> +        let hi = bar.read(regs::gh100::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(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_1);
> +    }
> +}
> +
> +fn sys_timer_offset(bar: Bar0<'_>, fsp: &Fsp<'_>, time_ns: u64) -> Result<u64> {
> +    let sec_timer_ns = sec_timer_read(bar);
> +
> +    let sys_timer_offset_ns = time_ns.wrapping_sub(sec_timer_ns);
> +
> +    if !fsp.is_bug_3957833_affected(bar) {
> +        bar.write_reg(regs::gh100::NV_PGC6_SCI_SYS_TIMER_OFFSET_1::from_nsec(
> +            sys_timer_offset_ns,
> +        ));
> +        bar.write_reg(
> +            regs::gh100::NV_PGC6_SCI_SYS_TIMER_OFFSET_0::from_nsec(sys_timer_offset_ns)
> +                .with_update(true),
> +        );
> +    }
> +    Ok(sys_timer_offset_ns)
> +}

So I know we discussed that on v4, but this function is named like it
just returns a value, whereas it also might program some registers. Can
we mirror what `tu102` does and extract the `if fsp.is_bug...` block
into another function that `setup` calls?

Your argument was that resume might need to do the same thing, but
it seems to me that resume would just call `setup` again anyway.

> +
> +impl TimerHal for Gh100 {
> +    fn setup(&self, bar: Bar0<'_>, fsp: Option<&Fsp<'_>>) -> Result<u64> {
> +        // The open drivers uses wall clock time here not monotonic

This module calls it "open drivers", tu102 says "The RM"... Let's call
it "OpenRM" in both. :)

> +        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)?;
> +
> +        sys_timer_offset(bar, fsp, sys_time_ns as u64)

`as` statements require a `CAST:` comment that explains why they are safe.

In this particular case I am not sure what we are supposed to expect, as
an `Instant<RealTime>` is not supposed to be negative anyway. So I
suppose that could be the `CAST:` comment.

> +    }
> +}
> +
> +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..629d744a4b12
> --- /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,
> +    fsp::Fsp,
> +    timer::regs, //
> +};
> +struct Tu102;

For symmetry, let's grant this a comment like its gh100 counterpart.

> +
> +// this code will be used in the gb10b hal eventually
> +#[expect(dead_code)]
> +fn timer_read(bar: Bar0<'_>) -> u64 {
> +    let mut hi2 = bar.read(regs::NV_PTIMER_TIME_1);
> +    loop {
> +        let lo = bar.read(regs::NV_PTIMER_TIME_0);
> +        let hi = bar.read(regs::NV_PTIMER_TIME_1);
> +        if hi == hi2 {
> +            return u64::from(u32::from(hi)) << 32 | u64::from(u32::from(lo));
> +        }
> +        hi2 = bar.read(regs::NV_PTIMER_TIME_1);
> +    }
> +}

Usually we try to not introduce dead code proactively, especially if the
user is not in sight. Can we introduce that alongside the gb10b code
that makes use of it?

> +
> +fn set_time(bar: Bar0<'_>, time_ns: u64) -> Result {
> +    if !bar.read(regs::NV_PTIMER_TIME_PRIV_LEVEL_MASK).level0() {
> +        return Err(EINVAL);
> +    }
> +    bar.write_reg(regs::NV_PTIMER_TIME_1::from_nsec(time_ns));
> +    bar.write_reg(regs::NV_PTIMER_TIME_0::from_nsec(time_ns));
> +    Ok(())
> +}
> +
> +impl TimerHal for Tu102 {
> +    fn setup(&self, bar: Bar0<'_>, _fsp: Option<&Fsp<'_>>) -> 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.

nit: s/The RM/OpenRM.

> +        let sys_time_ns = Instant::<RealTime>::now().as_nanos();
> +
> +        set_time(bar, sys_time_ns as u64)?;

Same here, this needs a `CAST:` comment.

> +        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..89dd478a37be
> --- /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;
> +
> +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, _>()
> +}
> +
> +/// 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.

Non-ascii character (em dash) in the comment.

Regarding the truncated MSBs in `nsec_hi`, I *think* it is ok to
truncate them indeed, as the value written would still be a valid anchor
value for the GPU. It also shouldn't manifest itself until 2043 or so
for chips < GH100, unless someone decides to mess with their system
clock.

I guess this issue is the reason why GH100+ are storing a delta instead
of an absolute value.

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

[David Airlie]

On Sun, Nov 8, 2026 at 11:52 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> On Fri Nov 6, 2026 at 2:34 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 is for hopper+
> > 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 to avoid sending it to registers
>
> nit: "On gh100 a work around is needed to avoid..." maybe?
>
> > if the FSP firmware is too old.
> > Otherwise we write the offset to the offset registers.
> >
> > Signed-off-by: Dave Airlie <airlied@redhat.com>
> > ---
> >  drivers/gpu/nova-core/fsp.rs             | 19 ++++++-
> >  drivers/gpu/nova-core/fsp/hal.rs         |  5 ++
> >  drivers/gpu/nova-core/fsp/hal/gh100.rs   |  6 ++
> >  drivers/gpu/nova-core/fsp/regs.rs        | 28 +++++++++
> >  drivers/gpu/nova-core/gpu.rs             |  8 +++
> >  drivers/gpu/nova-core/nova_core.rs       |  1 +
> >  drivers/gpu/nova-core/timer.rs           | 38 +++++++++++++
> >  drivers/gpu/nova-core/timer/hal.rs       | 31 ++++++++++
> >  drivers/gpu/nova-core/timer/hal/gh100.rs | 62 ++++++++++++++++++++
> >  drivers/gpu/nova-core/timer/hal/tu102.rs | 53 +++++++++++++++++
> >  drivers/gpu/nova-core/timer/regs.rs      | 98 ++++++++++++++++++++++++++++++++
> >  11 files changed, 346 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/nova-core/fsp.rs b/drivers/gpu/nova-core/fsp.rs
> > index 3a4a10487f33..a2ea50465a55 100644
> > --- a/drivers/gpu/nova-core/fsp.rs
> > +++ b/drivers/gpu/nova-core/fsp.rs
> > @@ -50,10 +50,11 @@
> >          NvdmHeader,
> >          NvdmType, //
> >      },
> > -    regs, //
> > +    regs as bootregs, //
> >  };
> >
> >  mod hal;
> > +mod regs;
> >
> >  /// PRC message sub-command.
> >  #[derive(Debug, Clone, Copy, PartialEq, Eq)]
> > @@ -389,6 +390,7 @@ pub(crate) fn boot_params(&self) -> &Coherent<'_, GspFmcBootParams> {
> >  pub(crate) struct Fsp<'a> {
> >      falcon: Falcon<'a, FspEngine>,
> >      fmc_fw: GspFmcFirmware<'a>,
> > +    chipset: Chipset,
> >  }
> >
> >  impl<'a> Fsp<'a> {
> > @@ -428,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),
> >          )
> > @@ -436,7 +438,11 @@ fn wait_secure_boot(
> >              dev_err!(dev, "FSP secure boot completion error: {:?}\n", e);
> >          })?;
> >
> > -        Ok(Fsp { falcon, fmc_fw })
> > +        Ok(Fsp {
> > +            falcon,
> > +            fmc_fw,
> > +            chipset,
> > +        })
> >      }
> >
> >      /// Sends a message to FSP and waits for the response.
> > @@ -549,4 +555,11 @@ pub(crate) fn boot_fmc(
> >          dev_dbg!(dev, "FSP Chain of Trust completed successfully\n");
> >          Ok(())
> >      }
> > +
> > +    pub(crate) fn is_bug_3957833_affected(&self, bar: Bar0<'_>) -> bool {
> > +        match hal::fsp_hal(self.chipset) {
> > +            Some(hal) => hal.is_bug_3957833_affected(bar),
> > +            None => false,
>
> As I mentioned in my other comment, storing the HAL instead of the
> chipset is technically better, but I'm ok if this is not done just for
> this single-use method.

Okay I've done it for v6, there are places we don't do this already,
but I'll do it for fsp.

> > +fn sys_timer_offset(bar: Bar0<'_>, fsp: &Fsp<'_>, time_ns: u64) -> Result<u64> {
> > +    let sec_timer_ns = sec_timer_read(bar);
> > +
> > +    let sys_timer_offset_ns = time_ns.wrapping_sub(sec_timer_ns);
> > +
> > +    if !fsp.is_bug_3957833_affected(bar) {
> > +        bar.write_reg(regs::gh100::NV_PGC6_SCI_SYS_TIMER_OFFSET_1::from_nsec(
> > +            sys_timer_offset_ns,
> > +        ));
> > +        bar.write_reg(
> > +            regs::gh100::NV_PGC6_SCI_SYS_TIMER_OFFSET_0::from_nsec(sys_timer_offset_ns)
> > +                .with_update(true),
> > +        );
> > +    }
> > +    Ok(sys_timer_offset_ns)
> > +}
>
> So I know we discussed that on v4, but this function is named like it
> just returns a value, whereas it also might program some registers. Can
> we mirror what `tu102` does and extract the `if fsp.is_bug...` block
> into another function that `setup` calls?

No, this gives the workaround too much recognition. I've renamed this
function to set_sys_timer_offset,
That is the point of this function, to calculate and set the system
timer offset in hardware. Broken hardware
should not direct the function's focus, working hardware calculates
and programs the registers.

> Regarding the truncated MSBs in `nsec_hi`, I *think* it is ok to
> truncate them indeed, as the value written would still be a valid anchor
> value for the GPU. It also shouldn't manifest itself until 2043 or so
> for chips < GH100, unless someone decides to mess with their system
> clock.
>
> I guess this issue is the reason why GH100+ are storing a delta instead
> of an absolute value.

For v6, I've done everything else asked,

For how the timer hw works, I unfortunately can't know this, because
it isn't documented in public docs, if you guys want to pursue this
internally, I can only do what OpenRM does.

Also are we okay using OpenRM? like I know there was some issues in
the past, maybe we need to have a proper way to reference the NVIDIA
open-gpu-kernel-drivers.

Dave.

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

[Alexandre Courbot]

On Mon Nov 9, 2026 at 10:52 AM JST, David Airlie wrote:
> On Sun, Nov 8, 2026 at 11:52 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>>
>> On Fri Nov 6, 2026 at 2:34 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 is for hopper+
>> > 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 to avoid sending it to registers
>>
>> nit: "On gh100 a work around is needed to avoid..." maybe?
>>
>> > if the FSP firmware is too old.
>> > Otherwise we write the offset to the offset registers.
>> >
>> > Signed-off-by: Dave Airlie <airlied@redhat.com>
>> > ---
>> >  drivers/gpu/nova-core/fsp.rs             | 19 ++++++-
>> >  drivers/gpu/nova-core/fsp/hal.rs         |  5 ++
>> >  drivers/gpu/nova-core/fsp/hal/gh100.rs   |  6 ++
>> >  drivers/gpu/nova-core/fsp/regs.rs        | 28 +++++++++
>> >  drivers/gpu/nova-core/gpu.rs             |  8 +++
>> >  drivers/gpu/nova-core/nova_core.rs       |  1 +
>> >  drivers/gpu/nova-core/timer.rs           | 38 +++++++++++++
>> >  drivers/gpu/nova-core/timer/hal.rs       | 31 ++++++++++
>> >  drivers/gpu/nova-core/timer/hal/gh100.rs | 62 ++++++++++++++++++++
>> >  drivers/gpu/nova-core/timer/hal/tu102.rs | 53 +++++++++++++++++
>> >  drivers/gpu/nova-core/timer/regs.rs      | 98 ++++++++++++++++++++++++++++++++
>> >  11 files changed, 346 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/nova-core/fsp.rs b/drivers/gpu/nova-core/fsp.rs
>> > index 3a4a10487f33..a2ea50465a55 100644
>> > --- a/drivers/gpu/nova-core/fsp.rs
>> > +++ b/drivers/gpu/nova-core/fsp.rs
>> > @@ -50,10 +50,11 @@
>> >          NvdmHeader,
>> >          NvdmType, //
>> >      },
>> > -    regs, //
>> > +    regs as bootregs, //
>> >  };
>> >
>> >  mod hal;
>> > +mod regs;
>> >
>> >  /// PRC message sub-command.
>> >  #[derive(Debug, Clone, Copy, PartialEq, Eq)]
>> > @@ -389,6 +390,7 @@ pub(crate) fn boot_params(&self) -> &Coherent<'_, GspFmcBootParams> {
>> >  pub(crate) struct Fsp<'a> {
>> >      falcon: Falcon<'a, FspEngine>,
>> >      fmc_fw: GspFmcFirmware<'a>,
>> > +    chipset: Chipset,
>> >  }
>> >
>> >  impl<'a> Fsp<'a> {
>> > @@ -428,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),
>> >          )
>> > @@ -436,7 +438,11 @@ fn wait_secure_boot(
>> >              dev_err!(dev, "FSP secure boot completion error: {:?}\n", e);
>> >          })?;
>> >
>> > -        Ok(Fsp { falcon, fmc_fw })
>> > +        Ok(Fsp {
>> > +            falcon,
>> > +            fmc_fw,
>> > +            chipset,
>> > +        })
>> >      }
>> >
>> >      /// Sends a message to FSP and waits for the response.
>> > @@ -549,4 +555,11 @@ pub(crate) fn boot_fmc(
>> >          dev_dbg!(dev, "FSP Chain of Trust completed successfully\n");
>> >          Ok(())
>> >      }
>> > +
>> > +    pub(crate) fn is_bug_3957833_affected(&self, bar: Bar0<'_>) -> bool {
>> > +        match hal::fsp_hal(self.chipset) {
>> > +            Some(hal) => hal.is_bug_3957833_affected(bar),
>> > +            None => false,
>>
>> As I mentioned in my other comment, storing the HAL instead of the
>> chipset is technically better, but I'm ok if this is not done just for
>> this single-use method.
>
> Okay I've done it for v6, there are places we don't do this already,
> but I'll do it for fsp.
>
>> > +fn sys_timer_offset(bar: Bar0<'_>, fsp: &Fsp<'_>, time_ns: u64) -> Result<u64> {
>> > +    let sec_timer_ns = sec_timer_read(bar);
>> > +
>> > +    let sys_timer_offset_ns = time_ns.wrapping_sub(sec_timer_ns);
>> > +
>> > +    if !fsp.is_bug_3957833_affected(bar) {
>> > +        bar.write_reg(regs::gh100::NV_PGC6_SCI_SYS_TIMER_OFFSET_1::from_nsec(
>> > +            sys_timer_offset_ns,
>> > +        ));
>> > +        bar.write_reg(
>> > +            regs::gh100::NV_PGC6_SCI_SYS_TIMER_OFFSET_0::from_nsec(sys_timer_offset_ns)
>> > +                .with_update(true),
>> > +        );
>> > +    }
>> > +    Ok(sys_timer_offset_ns)
>> > +}
>>
>> So I know we discussed that on v4, but this function is named like it
>> just returns a value, whereas it also might program some registers. Can
>> we mirror what `tu102` does and extract the `if fsp.is_bug...` block
>> into another function that `setup` calls?
>
> No, this gives the workaround too much recognition. I've renamed this
> function to set_sys_timer_offset,
> That is the point of this function, to calculate and set the system
> timer offset in hardware. Broken hardware
> should not direct the function's focus, working hardware calculates
> and programs the registers.
>
>> Regarding the truncated MSBs in `nsec_hi`, I *think* it is ok to
>> truncate them indeed, as the value written would still be a valid anchor
>> value for the GPU. It also shouldn't manifest itself until 2043 or so
>> for chips < GH100, unless someone decides to mess with their system
>> clock.
>>
>> I guess this issue is the reason why GH100+ are storing a delta instead
>> of an absolute value.
>
> For v6, I've done everything else asked,
>
> For how the timer hw works, I unfortunately can't know this, because
> it isn't documented in public docs, if you guys want to pursue this
> internally, I can only do what OpenRM does.
>
> Also are we okay using OpenRM? like I know there was some issues in
> the past, maybe we need to have a proper way to reference the NVIDIA
> open-gpu-kernel-drivers.

I do recognize I have sometimes ambivalent feelings on the topic of
whether we should follow OpenRM or not. :)

When in doubt as we currently are, I think the case for following OpenRM
is that doing so at least guarantees we won't do worse than it does, and
it has the benefit of being battle-tested.

I've tried to find out more about how the timer registers work. It
appears that the GH100 SEC_TIMER starts at 0 from power-on, so unless
people mess with their system clock it should always be smaller than it,
hence subtracting it should not wrap.

Now if it *does* wrap for some unfortunate reason, we would still get a
valid timer value by adding `SEC_TIMER + OFFSET` modulo 61 bits, which
supports the case for wrapping - it just needs to be documented as
otherwise it can be pretty confusing.

Then there is the year 2043 issue because the timer values are 61 bits.
It affects both the TU102 and GH100 HALs. I guess software relying on
timestamps will have to adjust for that, but from the perspective of
programming the registers I don't think we can do much better than
masking.