>
> 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:
Thanks, looks cleaner, sent out in v4.
> > + // 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.
In theory the value doesn't represent anything, since it's hal
dependant, at this
point it should be an opaque value we give to GSP in SYS_TIMER_OFFSET_NS field.
> 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.
Just realised I missed this one in v4.
> > + < 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.
I don't think we can get rid of the hal function, since GH100 vs GB100
need different behaviour, which is
just setting war flag.
>
> Can't we just use unsigned for the argument and return type? None of those are
> ever expected to be negative, no?
No, I think I got lost in what rust needed to avoid crashing the
machine, so I just stayed on the side of least likely to overflow.
I've reworked it in v4 back to what I had in the first place with checked_sub.
>
> 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().
>
>
> 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)
> }
> }
Done in v4.
> > + 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.
Moved in v4.
Dave.
>
