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.
