[PATCH v6 0/4] gpu: nova: add boot42 support for next-gen GPUs

[PATCH v6 0/4] gpu: nova: add boot42 support for next-gen GPUs

[John Hubbard]

Changes in v6:

1) Split out a separate patch for implementing Display for Spec.

2) Moved Spec implementation code to its proper location.

3) Significantly changed the use_boot42_instead() logic, and updated the
   comments accordingly.

4) Fixed the boot42 register and field values, which were wrong (and
   had not been exercised before).

5) Imported Revision, to save a few ::'s.

6) Rebased to the very latest drm-rust-next, which now includes the new
   "one 'use' item per line" updates.

Changes in v5:

Two fixes, both from Timur's review feedback (thanks!):

1) Updated both the cover letter, and patch 3 commit description, with
the correct description of the future contents of NV_PMC_BOOT_0.

2) Removed a trailing "boot42" typo from a comment in the code.

Changes in v4:

1) Simplified and improved the decision logic: reads both arch_0 and
arch_1 fields in boot0, and skips the unnecessary is_nv04() logic as
well. Thanks to Timur Tabi and Danilo for noticing these issues.

2) Added a patch to represent Architecture as a u8. This simplifies a
few things. (Thanks to Alex Courbot. I added your Suggested-by to that
patch.)

3) Enhanced the Revision type to do more, which simplifies the callers.
(Thanks to Danilo.)

Changes in v3:

1) Restored the Revision type as recommended by Danilo, but decoupled it
from boot0.

2) Applied Alex Courbot's suggestion to use TryFrom<NV_PMC_BOOT_0/42>
for Spec.

3) Reflowed the new comment documentation to 100 cols, to avoid wasting
a few vertical lines.

Changes in v2:

1) Restored the Spec type, and used that to encapsulate the subsequent
   boot42 enhancements. Thanks to Danilo Krummrich's feedback for that
   improvement.

v1 cover letter (with typos fixed)

NVIDIA GPUs are moving away from using NV_PMC_BOOT_0 to contain
architecture and revision details, and will instead use NV_PMC_BOOT_42
in the future. NV_PMC_BOOT_0 will contain a specific set of values that
will mean "go read NV_PMC_BOOT_42 instead".

Change the selection logic in Nova so that it will claim Turing and
later GPUs. This will work for the foreseeable future, without any
further code changes here, because all NVIDIA GPUs are considered, from
the oldest supported on Linux (NV04), through the future GPUs.

Add some comment documentation to explain, chronologically, how boot0
and boot42 change with the GPU eras, and how that affects the selection
logic.

Also, remove the Revision type, because Revision is no longer valuable
as a stand-alone type, because we only ever want the full information
that Spec provides.

This is based on today's drm-rust-next, which in turn is based on
Linux 6.18-rc2.

John Hubbard (4):
  gpu: nova-core: implement Display for Spec
  gpu: nova-core: prepare Spec and Revision types for boot0/boot42
  gpu: nova-core: make Architecture behave as a u8 type
  gpu: nova-core: add boot42 support for next-gen GPUs

 drivers/gpu/nova-core/gpu.rs  | 87 +++++++++++++++++++++++++++--------
 drivers/gpu/nova-core/regs.rs | 53 ++++++++++++++++++++-
 2 files changed, 119 insertions(+), 21 deletions(-)


base-commit: 80b3dc0a5a2e51fb2b8f3406f5ee20ad4a652316
-- 
2.51.2

[PATCH v6 1/4] gpu: nova-core: implement Display for Spec

[John Hubbard]

Implement Display for Spec. This simplifies the dev_info!() code for
printing banners such as:

    NVIDIA (Chipset: GA104, Architecture: Ampere, Revision: a.1)

Cc: Alexandre Courbot <acourbot@nvidia.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Timur Tabi <ttabi@nvidia.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 drivers/gpu/nova-core/gpu.rs | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 802e71e4f97d..7fd9e91771a6 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -180,6 +180,18 @@ fn new(bar: &Bar0) -> Result<Spec> {
     }
 }
 
+impl fmt::Display for Spec {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(
+            f,
+            "Chipset: {}, Architecture: {:?}, Revision: {}",
+            self.chipset,
+            self.chipset.arch(),
+            self.revision
+        )
+    }
+}
+
 /// Structure holding the resources required to operate the GPU.
 #[pin_data]
 pub(crate) struct Gpu {
@@ -206,13 +218,7 @@ pub(crate) fn new<'a>(
     ) -> impl PinInit<Self, Error> + 'a {
         try_pin_init!(Self {
             spec: Spec::new(bar).inspect(|spec| {
-                dev_info!(
-                    pdev.as_ref(),
-                    "NVIDIA (Chipset: {}, Architecture: {:?}, Revision: {})\n",
-                    spec.chipset,
-                    spec.chipset.arch(),
-                    spec.revision
-                );
+                dev_info!(pdev.as_ref(),"NVIDIA ({})\n", spec);
             })?,
 
             // We must wait for GFW_BOOT completion before doing any significant setup on the GPU.
-- 
2.51.2

[PATCH v6 2/4] gpu: nova-core: prepare Spec and Revision types for boot0/boot42

[John Hubbard]

1) Decouple Revision from boot0.

2) Enhance Revision, which in turn simplifies Spec::new().

3) Also, slightly enhance the comment about Spec, to be more precise.

Cc: Alexandre Courbot <acourbot@nvidia.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Timur Tabi <ttabi@nvidia.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 drivers/gpu/nova-core/gpu.rs  | 25 ++++++++++++-------------
 drivers/gpu/nova-core/regs.rs | 11 ++++++++++-
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 7fd9e91771a6..acf564fee9c8 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -143,17 +143,8 @@ fn try_from(value: u8) -> Result<Self> {
 }
 
 pub(crate) struct Revision {
-    major: u8,
-    minor: u8,
-}
-
-impl Revision {
-    fn from_boot0(boot0: regs::NV_PMC_BOOT_0) -> Self {
-        Self {
-            major: boot0.major_revision(),
-            minor: boot0.minor_revision(),
-        }
-    }
+    pub(crate) major: u8,
+    pub(crate) minor: u8,
 }
 
 impl fmt::Display for Revision {
@@ -162,7 +153,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
     }
 }
 
-/// Structure holding the metadata of the GPU.
+/// Structure holding a basic description of the GPU: Architecture, Chipset and Revision.
 pub(crate) struct Spec {
     chipset: Chipset,
     /// The revision of the chipset.
@@ -173,9 +164,17 @@ impl Spec {
     fn new(bar: &Bar0) -> Result<Spec> {
         let boot0 = regs::NV_PMC_BOOT_0::read(bar);
 
+        Spec::try_from(boot0)
+    }
+}
+
+impl TryFrom<regs::NV_PMC_BOOT_0> for Spec {
+    type Error = Error;
+
+    fn try_from(boot0: regs::NV_PMC_BOOT_0) -> Result<Self> {
         Ok(Self {
             chipset: boot0.chipset()?,
-            revision: Revision::from_boot0(boot0),
+            revision: boot0.revision(),
         })
     }
 }
diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
index 934003cab8a8..8c9af3c59708 100644
--- a/drivers/gpu/nova-core/regs.rs
+++ b/drivers/gpu/nova-core/regs.rs
@@ -24,7 +24,8 @@
     },
     gpu::{
         Architecture,
-        Chipset, //
+        Chipset,
+        Revision, //
     },
     num::FromSafeCast,
 };
@@ -56,6 +57,14 @@ pub(crate) fn chipset(self) -> Result<Chipset> {
             })
             .and_then(Chipset::try_from)
     }
+
+    /// Returns the revision information of the chip.
+    pub(crate) fn revision(self) -> Revision {
+        Revision {
+            major: self.major_revision(),
+            minor: self.minor_revision(),
+        }
+    }
 }
 
 // PBUS
-- 
2.51.2

[PATCH v6 3/4] gpu: nova-core: make Architecture behave as a u8 type

[John Hubbard]

This allows Architecture to be passed into register!() and bitfield!()
macro calls. That in turn requires a default implementation for
Architecture.

This simplifies transforming BOOT0 (and later, BOOT42) register values
into GPU architectures.

Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Timur Tabi <ttabi@nvidia.com>
Suggested-by: Alexandre Courbot <acourbot@nvidia.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 drivers/gpu/nova-core/gpu.rs | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index acf564fee9c8..94a6054bab95 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -122,8 +122,10 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
 }
 
 /// Enum representation of the GPU generation.
-#[derive(fmt::Debug)]
+#[derive(fmt::Debug, Default, Copy, Clone)]
+#[repr(u8)]
 pub(crate) enum Architecture {
+    #[default]
     Turing = 0x16,
     Ampere = 0x17,
     Ada = 0x19,
@@ -142,6 +144,13 @@ fn try_from(value: u8) -> Result<Self> {
     }
 }
 
+impl From<Architecture> for u8 {
+    fn from(value: Architecture) -> Self {
+        // CAST: `Architecture` is `repr(u8)`, so this cast is always lossless.
+        value as u8
+    }
+}
+
 pub(crate) struct Revision {
     pub(crate) major: u8,
     pub(crate) minor: u8,
-- 
2.51.2

[PATCH v6 4/4] gpu: nova-core: add boot42 support for next-gen GPUs

[John Hubbard]

NVIDIA GPUs are moving away from using NV_PMC_BOOT_0 to contain
architecture and revision details, and will instead use NV_PMC_BOOT_42
in the future. NV_PMC_BOOT_0 will contain a specific set of values
that will mean "go read NV_PMC_BOOT_42 instead".

Change the selection logic in Nova so that it will claim Turing and
later GPUs. This will work for the foreseeable future, without any
further code changes here, because all NVIDIA GPUs are considered, from
the oldest supported on Linux (NV04), through the future GPUs.

Add some comment documentation to explain, chronologically, how boot0
and boot42 change with the GPU eras, and how that affects the selection
logic.

Cc: Alexandre Courbot <acourbot@nvidia.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Timur Tabi <ttabi@nvidia.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 drivers/gpu/nova-core/gpu.rs  | 35 ++++++++++++++++++++++++++++-
 drivers/gpu/nova-core/regs.rs | 42 +++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 94a6054bab95..5650c115c613 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -171,9 +171,31 @@ pub(crate) struct Spec {
 
 impl Spec {
     fn new(bar: &Bar0) -> Result<Spec> {
+        // Some brief notes about boot0 and boot42, in chronological order:
+        //
+        // NV04 through NV50:
+        //
+        //    Not supported by Nova. boot0 is necessary and sufficient to identify these GPUs.
+        //    boot42 may not even exist on some of these GPUs.
+        //
+        // Fermi through Volta:
+        //
+        //     Not supported by Nova. boot0 is still sufficient to identify these GPUs, but boot42
+        //     is also guaranteed to be both present and accurate.
+        //
+        // Turing and later:
+        //
+        //     Supported by Nova. Identified by first checking boot0 to ensure that the GPU is not
+        //     from an earlier (pre-Fermi) era, and then using boot42 to precisely identify the GPU.
+        //     Somewhere in the Rubin timeframe, boot0 will no longer have space to add new GPU IDs.
+
         let boot0 = regs::NV_PMC_BOOT_0::read(bar);
 
-        Spec::try_from(boot0)
+        if boot0.use_boot42_instead() {
+            Spec::try_from(regs::NV_PMC_BOOT_42::read(bar))
+        } else {
+            Spec::try_from(boot0)
+        }
     }
 }
 
@@ -188,6 +210,17 @@ fn try_from(boot0: regs::NV_PMC_BOOT_0) -> Result<Self> {
     }
 }
 
+impl TryFrom<regs::NV_PMC_BOOT_42> for Spec {
+    type Error = Error;
+
+    fn try_from(boot42: regs::NV_PMC_BOOT_42) -> Result<Self> {
+        Ok(Self {
+            chipset: boot42.chipset()?,
+            revision: boot42.revision(),
+        })
+    }
+}
+
 impl fmt::Display for Spec {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         write!(
diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
index 8c9af3c59708..5d6397f6450a 100644
--- a/drivers/gpu/nova-core/regs.rs
+++ b/drivers/gpu/nova-core/regs.rs
@@ -41,6 +41,22 @@
 });
 
 impl NV_PMC_BOOT_0 {
+    fn older_than_fermi(self) -> bool {
+        // From https://github.com/NVIDIA/open-gpu-doc/tree/master/manuals :
+        const NV_PMC_BOOT_0_ARCHITECTURE_GF100: u8 = 0xc;
+
+        // Older chips left arch1 zeroed out. That, combined with an arch0 value that is less than
+        // GF00, means "older than Fermi".
+        self.architecture_1() == 0 && self.architecture_0() < NV_PMC_BOOT_0_ARCHITECTURE_GF100
+    }
+
+    pub(crate) fn use_boot42_instead(self) -> bool {
+        // For Fermi+ GPUs, boot42 is guaranteed to be both present and accurate, so that's the
+        // point at which we switch over to relying on boot42 for precise identification.
+
+        !self.older_than_fermi()
+    }
+
     /// Combines `architecture_0` and `architecture_1` to obtain the architecture of the chip.
     pub(crate) fn architecture(self) -> Result<Architecture> {
         Architecture::try_from(
@@ -67,6 +83,32 @@ pub(crate) fn revision(self) -> Revision {
     }
 }
 
+register!(NV_PMC_BOOT_42 @ 0x00000a00, "Extended architecture information" {
+    15:12   minor_revision as u8, "Minor revision of the chip";
+    19:16   major_revision as u8, "Major revision of the chip";
+    23:20   implementation as u8, "Implementation version of the architecture";
+    29:24   architecture as u8 ?=> Architecture, "Architecture value";
+});
+
+impl NV_PMC_BOOT_42 {
+    pub(crate) fn chipset(self) -> Result<Chipset> {
+        self.architecture()
+            .map(|arch| {
+                ((arch as u32) << Self::IMPLEMENTATION_RANGE.len())
+                    | u32::from(self.implementation())
+            })
+            .and_then(Chipset::try_from)
+    }
+
+    /// Returns the revision information of the chip.
+    pub(crate) fn revision(self) -> Revision {
+        Revision {
+            major: self.major_revision(),
+            minor: self.minor_revision(),
+        }
+    }
+}
+
 // PBUS
 
 register!(NV_PBUS_SW_SCRATCH @ 0x00001400[64]  {});
-- 
2.51.2

Re: [PATCH v6 1/4] gpu: nova-core: implement Display for Spec

[Timur Tabi]

On Fri, 2025-11-07 at 20:39 -0800, John Hubbard wrote:
> Implement Display for Spec. This simplifies the dev_info!() code for
> printing banners such as:
> 
>     NVIDIA (Chipset: GA104, Architecture: Ampere, Revision: a.1)
> 
> Cc: Alexandre Courbot <acourbot@nvidia.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Timur Tabi <ttabi@nvidia.com>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>

I'm okay with the entire patch set, but I do have a few questions.

> +impl fmt::Display for Spec {
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        write!(
> +            f,
> +            "Chipset: {}, Architecture: {:?}, Revision: {}",
> +            self.chipset,
> +            self.chipset.arch(),
> +            self.revision
> +        )
> +    }
> +}
> +
>  /// Structure holding the resources required to operate the GPU.
>  #[pin_data]
>  pub(crate) struct Gpu {
> @@ -206,13 +218,7 @@ pub(crate) fn new<'a>(
>      ) -> impl PinInit<Self, Error> + 'a {
>          try_pin_init!(Self {
>              spec: Spec::new(bar).inspect(|spec| {
> -                dev_info!(
> -                    pdev.as_ref(),
> -                    "NVIDIA (Chipset: {}, Architecture: {:?}, Revision: {})\n",
> -                    spec.chipset,
> -                    spec.chipset.arch(),
> -                    spec.revision
> -                );
> +                dev_info!(pdev.as_ref(),"NVIDIA ({})\n", spec);

I believe that this is the only place where a `Spec` is actually printed.  Does it really make
sense to implement Display for a single usage?  Do we generally want to implement Display for
new types?

Re: [PATCH v6 3/4] gpu: nova-core: make Architecture behave as a u8 type

[Timur Tabi]

On Fri, 2025-11-07 at 20:39 -0800, John Hubbard wrote:
>  /// Enum representation of the GPU generation.
> -#[derive(fmt::Debug)]
> +#[derive(fmt::Debug, Default, Copy, Clone)]
> +#[repr(u8)]
>  pub(crate) enum Architecture {
> +    #[default]
>      Turing = 0x16,
>      Ampere = 0x17,
>      Ada = 0x19,
> @@ -142,6 +144,13 @@ fn try_from(value: u8) -> Result<Self> {
>      }
>  }

Does it make sense to designate a default Architecture?  Turing is not a fallback for Ampere --
you can't boot an Ampere with Turing's HAL.  Also, we don't even make Turing cards any more, so
over time, Turing will be less and less common.

Re: [PATCH v6 3/4] gpu: nova-core: make Architecture behave as a u8 type

[John Hubbard]

On 11/7/25 9:03 PM, Timur Tabi wrote:
> On Fri, 2025-11-07 at 20:39 -0800, John Hubbard wrote:
>>  /// Enum representation of the GPU generation.
>> -#[derive(fmt::Debug)]
>> +#[derive(fmt::Debug, Default, Copy, Clone)]
>> +#[repr(u8)]
>>  pub(crate) enum Architecture {
>> +    #[default]
>>      Turing = 0x16,
>>      Ampere = 0x17,
>>      Ada = 0x19,
>> @@ -142,6 +144,13 @@ fn try_from(value: u8) -> Result<Self> {
>>      }
>>  }
> 
> Does it make sense to designate a default Architecture?  Turing is not a fallback for Ampere --

Definitely not! However, we do want to use Architecture in places
(register! and bitfield! macros) that expect u8 or u32, and that also
expect to use integer defaults.

So that's why we have to supply it. 


thanks,
-- 
John Hubbard

Re: [PATCH v6 4/4] gpu: nova-core: add boot42 support for next-gen GPUs

[Timur Tabi]

On Fri, 2025-11-07 at 20:39 -0800, John Hubbard wrote:
> +        // Turing and later:
> +        //
> +        //     Supported by Nova. Identified by first checking boot0 to ensure that the GPU
> is not
> +        //     from an earlier (pre-Fermi) era, and then using boot42 to precisely identify
> the GPU.
> +        //     Somewhere in the Rubin timeframe, boot0 will no longer have space to add new
> GPU IDs.
> +
>          let boot0 = regs::NV_PMC_BOOT_0::read(bar);
>  
> -        Spec::try_from(boot0)
> +        if boot0.use_boot42_instead() {
> +            Spec::try_from(regs::NV_PMC_BOOT_42::read(bar))
> +        } else {
> +            Spec::try_from(boot0)
> +        }
>      }

Spec::try_from(boot0) will always fail, because we can't generate a Spec from a pre-Turing GPU,
so it seems weird that we have it as an else condition.

I don't think the comment and the code aligns.  The code implies that sometimes we'll be using
boot0 to generate the Spec, but that isn't true.  However, the comment makes it clear that we'll
be using boot42 only.

Re: [PATCH v6 1/4] gpu: nova-core: implement Display for Spec

[John Hubbard]

On 11/7/25 9:02 PM, Timur Tabi wrote:
> On Fri, 2025-11-07 at 20:39 -0800, John Hubbard wrote:
>> Implement Display for Spec. This simplifies the dev_info!() code for
>> printing banners such as:
>>
>>     NVIDIA (Chipset: GA104, Architecture: Ampere, Revision: a.1)
>>
>> Cc: Alexandre Courbot <acourbot@nvidia.com>
>> Cc: Danilo Krummrich <dakr@kernel.org>
>> Cc: Timur Tabi <ttabi@nvidia.com>
>> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> 
> I'm okay with the entire patch set, but I do have a few questions.

Great!

> 
>> +impl fmt::Display for Spec {
>> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>> +        write!(
>> +            f,
>> +            "Chipset: {}, Architecture: {:?}, Revision: {}",
>> +            self.chipset,
>> +            self.chipset.arch(),
>> +            self.revision
>> +        )
>> +    }
>> +}
>> +
>>  /// Structure holding the resources required to operate the GPU.
>>  #[pin_data]
>>  pub(crate) struct Gpu {
>> @@ -206,13 +218,7 @@ pub(crate) fn new<'a>(
>>      ) -> impl PinInit<Self, Error> + 'a {
>>          try_pin_init!(Self {
>>              spec: Spec::new(bar).inspect(|spec| {
>> -                dev_info!(
>> -                    pdev.as_ref(),
>> -                    "NVIDIA (Chipset: {}, Architecture: {:?}, Revision: {})\n",
>> -                    spec.chipset,
>> -                    spec.chipset.arch(),
>> -                    spec.revision
>> -                );
>> +                dev_info!(pdev.as_ref(),"NVIDIA ({})\n", spec);
> 
> I believe that this is the only place where a `Spec` is actually printed.  Does it really make
> sense to implement Display for a single usage?  Do we generally want to implement Display for
> new types?
> 

I agree that it looks a little excessive, but I defer to reviewers
who have a lot more Rust experience, and they requested this during
a review of an earlier version.


thanks,
-- 
John Hubbard

Re: [PATCH v6 4/4] gpu: nova-core: add boot42 support for next-gen GPUs

[John Hubbard]

On 11/7/25 9:09 PM, Timur Tabi wrote:
> On Fri, 2025-11-07 at 20:39 -0800, John Hubbard wrote:
...
>>          let boot0 = regs::NV_PMC_BOOT_0::read(bar);
>>  
>> -        Spec::try_from(boot0)
>> +        if boot0.use_boot42_instead() {
>> +            Spec::try_from(regs::NV_PMC_BOOT_42::read(bar))
>> +        } else {
>> +            Spec::try_from(boot0)
>> +        }
>>      }
> 
> Spec::try_from(boot0) will always fail, because we can't generate a Spec from a pre-Turing GPU,
> so it seems weird that we have it as an else condition.
> 
> I don't think the comment and the code aligns.  The code implies that sometimes we'll be using
> boot0 to generate the Spec, but that isn't true.  However, the comment makes it clear that we'll
> be using boot42 only.

Hmmm, yes, the new use_boot42_instead() logic means that most of the
boot0 logic should actually be deleted now. OK, so I can apply this
diff on top, and everything still works:

diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 5650c115c613..6d17ad3cec40 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -194,22 +194,11 @@ fn new(bar: &Bar0) -> Result<Spec> {
         if boot0.use_boot42_instead() {
             Spec::try_from(regs::NV_PMC_BOOT_42::read(bar))
         } else {
-            Spec::try_from(boot0)
+            Err(ENOTSUPP)
         }
     }
 }
 
-impl TryFrom<regs::NV_PMC_BOOT_0> for Spec {
-    type Error = Error;
-
-    fn try_from(boot0: regs::NV_PMC_BOOT_0) -> Result<Self> {
-        Ok(Self {
-            chipset: boot0.chipset()?,
-            revision: boot0.revision(),
-        })
-    }
-}
-
 impl TryFrom<regs::NV_PMC_BOOT_42> for Spec {
     type Error = Error;
 
diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
index 5d6397f6450a..018bee114a3f 100644
--- a/drivers/gpu/nova-core/regs.rs
+++ b/drivers/gpu/nova-core/regs.rs
@@ -56,31 +56,6 @@ pub(crate) fn use_boot42_instead(self) -> bool {
 
         !self.older_than_fermi()
     }
-
-    /// Combines `architecture_0` and `architecture_1` to obtain the architecture of the chip.
-    pub(crate) fn architecture(self) -> Result<Architecture> {
-        Architecture::try_from(
-            self.architecture_0() | (self.architecture_1() << Self::ARCHITECTURE_0_RANGE.len()),
-        )
-    }
-
-    /// Combines `architecture` and `implementation` to obtain a code unique to the chipset.
-    pub(crate) fn chipset(self) -> Result<Chipset> {
-        self.architecture()
-            .map(|arch| {
-                ((arch as u32) << Self::IMPLEMENTATION_RANGE.len())
-                    | u32::from(self.implementation())
-            })
-            .and_then(Chipset::try_from)
-    }
-
-    /// Returns the revision information of the chip.
-    pub(crate) fn revision(self) -> Revision {
-        Revision {
-            major: self.major_revision(),
-            minor: self.minor_revision(),
-        }
-    }
 }
 
 register!(NV_PMC_BOOT_42 @ 0x00000a00, "Extended architecture information" {



thanks,
-- 
John Hubbard

Re: [PATCH v6 1/4] gpu: nova-core: implement Display for Spec

[Alexandre Courbot]

On Sat Nov 8, 2025 at 2:10 PM JST, John Hubbard wrote:
> On 11/7/25 9:02 PM, Timur Tabi wrote:
>> On Fri, 2025-11-07 at 20:39 -0800, John Hubbard wrote:
>>> Implement Display for Spec. This simplifies the dev_info!() code for
>>> printing banners such as:
>>>
>>>     NVIDIA (Chipset: GA104, Architecture: Ampere, Revision: a.1)
>>>
>>> Cc: Alexandre Courbot <acourbot@nvidia.com>
>>> Cc: Danilo Krummrich <dakr@kernel.org>
>>> Cc: Timur Tabi <ttabi@nvidia.com>
>>> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
>> 
>> I'm okay with the entire patch set, but I do have a few questions.
>
> Great!
>
>> 
>>> +impl fmt::Display for Spec {
>>> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>>> +        write!(
>>> +            f,
>>> +            "Chipset: {}, Architecture: {:?}, Revision: {}",
>>> +            self.chipset,
>>> +            self.chipset.arch(),
>>> +            self.revision
>>> +        )
>>> +    }
>>> +}
>>> +
>>>  /// Structure holding the resources required to operate the GPU.
>>>  #[pin_data]
>>>  pub(crate) struct Gpu {
>>> @@ -206,13 +218,7 @@ pub(crate) fn new<'a>(
>>>      ) -> impl PinInit<Self, Error> + 'a {
>>>          try_pin_init!(Self {
>>>              spec: Spec::new(bar).inspect(|spec| {
>>> -                dev_info!(
>>> -                    pdev.as_ref(),
>>> -                    "NVIDIA (Chipset: {}, Architecture: {:?}, Revision: {})\n",
>>> -                    spec.chipset,
>>> -                    spec.chipset.arch(),
>>> -                    spec.revision
>>> -                );
>>> +                dev_info!(pdev.as_ref(),"NVIDIA ({})\n", spec);
>> 
>> I believe that this is the only place where a `Spec` is actually printed.  Does it really make
>> sense to implement Display for a single usage?  Do we generally want to implement Display for
>> new types?
>> 
>
> I agree that it looks a little excessive, but I defer to reviewers
> who have a lot more Rust experience, and they requested this during
> a review of an earlier version.

I think this is the correct way to do; `Spec` should be the one to
decide how it is displayed, and from a maintainability perspective this
ensures that other sites that will want to print a `Spec` in the future
will reuse this implementation, instead of either rewriting one
themselves or having to figure out that there was already an existing
site and factor it out.

Iow, this code is proactively doing the right thing.

Re: [PATCH v6 3/4] gpu: nova-core: make Architecture behave as a u8 type

[Alexandre Courbot]

On Sat Nov 8, 2025 at 2:08 PM JST, John Hubbard wrote:
> On 11/7/25 9:03 PM, Timur Tabi wrote:
>> On Fri, 2025-11-07 at 20:39 -0800, John Hubbard wrote:
>>>  /// Enum representation of the GPU generation.
>>> -#[derive(fmt::Debug)]
>>> +#[derive(fmt::Debug, Default, Copy, Clone)]
>>> +#[repr(u8)]
>>>  pub(crate) enum Architecture {
>>> +    #[default]
>>>      Turing = 0x16,
>>>      Ampere = 0x17,
>>>      Ada = 0x19,
>>> @@ -142,6 +144,13 @@ fn try_from(value: u8) -> Result<Self> {
>>>      }
>>>  }
>> 
>> Does it make sense to designate a default Architecture?  Turing is not a fallback for Ampere --
>
> Definitely not! However, we do want to use Architecture in places
> (register! and bitfield! macros) that expect u8 or u32, and that also
> expect to use integer defaults.
>
> So that's why we have to supply it. 

To be precise, we need to supply this because of a shortcoming in the
`register`` macro: it doesn't support read-only registers yet, and write
support requires a `Default` implementation for its fields. This is
subject to be fixed in the future but for now we need this little
workaround.

Re: [PATCH v6 3/4] gpu: nova-core: make Architecture behave as a u8 type

[Timur Tabi]

On Sat, 2025-11-08 at 20:45 +0900, Alexandre Courbot wrote:
> To be precise, we need to supply this because of a shortcoming in the
> `register`` macro: it doesn't support read-only registers yet, and write
> support requires a `Default` implementation for its fields. This is
> subject to be fixed in the future but for now we need this little
> workaround.

This definitely feels like something that needs a TODO comment.

Re: [PATCH v6 4/4] gpu: nova-core: add boot42 support for next-gen GPUs

[Timur Tabi]

On Fri, 2025-11-07 at 21:19 -0800, John Hubbard wrote:
> > 
> > Spec::try_from(boot0) will always fail, because we can't generate a Spec from a pre-Turing
> > GPU,
> > so it seems weird that we have it as an else condition.
> > 
> > I don't think the comment and the code aligns.  The code implies that sometimes we'll be
> > using
> > boot0 to generate the Spec, but that isn't true.  However, the comment makes it clear that
> > we'll
> > be using boot42 only.
> 
> Hmmm, yes, the new use_boot42_instead() logic means that most of the
> boot0 logic should actually be deleted now. OK, so I can apply this
> diff on top, and everything still works:

...

This is much better, thanks.

Re: [PATCH v6 1/4] gpu: nova-core: implement Display for Spec

[Miguel Ojeda]

On Sat, Nov 8, 2025 at 12:42 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> I think this is the correct way to do; `Spec` should be the one to
> decide how it is displayed, and from a maintainability perspective this
> ensures that other sites that will want to print a `Spec` in the future
> will reuse this implementation, instead of either rewriting one
> themselves or having to figure out that there was already an existing
> site and factor it out.
>
> Iow, this code is proactively doing the right thing.

Yes, please, avoid hardcoding inlined display code.

Cheers,
Miguel

Re: [PATCH v6 3/4] gpu: nova-core: make Architecture behave as a u8 type

[John Hubbard]

On 11/8/25 9:27 AM, Timur Tabi wrote:
> On Sat, 2025-11-08 at 20:45 +0900, Alexandre Courbot wrote:
>> To be precise, we need to supply this because of a shortcoming in the
>> `register`` macro: it doesn't support read-only registers yet, and write
>> support requires a `Default` implementation for its fields. This is
>> subject to be fixed in the future but for now we need this little
>> workaround.
> 
> This definitely feels like something that needs a TODO comment.

I've drafted it this way, for the next patchset revision:

/// Enum representation of the GPU generation.
///
/// TODO: remove the `Default` trait implementation, and the `#[default]`
/// attribute, once the register!() macro (which creates Architecture items) no
/// longer requires it for read-only fields.


thanks,
-- 
John Hubbard