John Hubbard
2025-Nov-06 01:27 UTC
[PATCH v5 1/3] gpu: nova-core: prepare Spec and Revision types for boot0/boot42
1) Implement Display for Spec. This simplifies the dev_info!() code for
printing banners such as:
NVIDIA (Chipset: GA104, Architecture: Ampere, Revision: a.1)
2) Decouple Revision from boot0.
3) Enhance Revision, which in turn simplifies Spec::new().
4) Also, slightly enhance the comment about Spec, to be more precise.
Cc: Alexandre Courbot <acourbot at nvidia.com>
Cc: Danilo Krummrich <dakr at kernel.org>
Cc: Timur Tabi <ttabi at nvidia.com>
Signed-off-by: John Hubbard <jhubbard at nvidia.com>
---
drivers/gpu/nova-core/gpu.rs | 45 +++++++++++++++++++----------------
drivers/gpu/nova-core/regs.rs | 8 +++++++
2 files changed, 33 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 9d182bffe8b4..8173cdcd8378 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -130,16 +130,18 @@ fn try_from(value: u8) -> Result<Self> {
}
pub(crate) struct Revision {
- major: u8,
- minor: u8,
+ pub(crate) major: u8,
+ pub(crate) minor: u8,
}
-impl Revision {
- fn from_boot0(boot0: regs::NV_PMC_BOOT_0) -> Self {
- Self {
- major: boot0.major_revision(),
- minor: boot0.minor_revision(),
- }
+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(),
+ })
}
}
@@ -149,7 +151,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.
@@ -160,10 +162,19 @@ impl Spec {
fn new(bar: &Bar0) -> Result<Spec> {
let boot0 = regs::NV_PMC_BOOT_0::read(bar);
- Ok(Self {
- chipset: boot0.chipset()?,
- revision: Revision::from_boot0(boot0),
- })
+ Spec::try_from(boot0)
+ }
+}
+
+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
+ )
}
}
@@ -193,13 +204,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.
diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
index 206dab2e1335..207b865335af 100644
--- a/drivers/gpu/nova-core/regs.rs
+++ b/drivers/gpu/nova-core/regs.rs
@@ -41,6 +41,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) -> crate::gpu::Revision {
+ crate::gpu::Revision {
+ major: self.major_revision(),
+ minor: self.minor_revision(),
+ }
+ }
}
// PBUS
--
2.51.2
Alexandre Courbot
2025-Nov-06 07:45 UTC
[PATCH v5 1/3] gpu: nova-core: prepare Spec and Revision types for boot0/boot42
On Thu Nov 6, 2025 at 10:27 AM JST, John Hubbard wrote:> 1) Implement Display for Spec. This simplifies the dev_info!() code for > printing banners such as: > > NVIDIA (Chipset: GA104, Architecture: Ampere, Revision: a.1) > > 2) Decouple Revision from boot0. > > 3) Enhance Revision, which in turn simplifies Spec::new(). > > 4) Also, slightly enhance the comment about Spec, to be more precise.A bullet-list in a patch description is a sure sign you will be asked to split things up. :) And in this case I think it makes all the more sense, since all these things taken separately ought to be simple, but having them in the same diff makes it confusing to review. Although it's mostly the `Display` implementation that at the very least should be its own patch, the rest can probably be kept together as it is related, and adding an intermediate patch would require temporary code to build Revision. The diff becomes much clearer once the impl blocks are moved where they should be (please see below). The comment update can be squashed together with the Revision/Spec patch.> > Cc: Alexandre Courbot <acourbot at nvidia.com> > Cc: Danilo Krummrich <dakr at kernel.org> > Cc: Timur Tabi <ttabi at nvidia.com> > Signed-off-by: John Hubbard <jhubbard at nvidia.com> > --- > drivers/gpu/nova-core/gpu.rs | 45 +++++++++++++++++++---------------- > drivers/gpu/nova-core/regs.rs | 8 +++++++ > 2 files changed, 33 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs > index 9d182bffe8b4..8173cdcd8378 100644 > --- a/drivers/gpu/nova-core/gpu.rs > +++ b/drivers/gpu/nova-core/gpu.rs > @@ -130,16 +130,18 @@ fn try_from(value: u8) -> Result<Self> { > } > > pub(crate) struct Revision { > - major: u8, > - minor: u8, > + pub(crate) major: u8, > + pub(crate) minor: u8, > } > > -impl Revision { > - fn from_boot0(boot0: regs::NV_PMC_BOOT_0) -> Self { > - Self { > - major: boot0.major_revision(), > - minor: boot0.minor_revision(), > - } > +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(), > + })This impl block for `Revision` gets replaced by a block for `Spec`, which is only declared later. Can you move it (and the one for BOOT_42 in the third patch) to the right place, next to the other impl blocks for `Spec`?> } > } > > @@ -149,7 +151,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. > @@ -160,10 +162,19 @@ impl Spec { > fn new(bar: &Bar0) -> Result<Spec> { > let boot0 = regs::NV_PMC_BOOT_0::read(bar); > > - Ok(Self { > - chipset: boot0.chipset()?, > - revision: Revision::from_boot0(boot0), > - }) > + Spec::try_from(boot0) > + } > +} > + > +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 > + ) > } > } > > @@ -193,13 +204,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. > diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs > index 206dab2e1335..207b865335af 100644 > --- a/drivers/gpu/nova-core/regs.rs > +++ b/drivers/gpu/nova-core/regs.rs > @@ -41,6 +41,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) -> crate::gpu::Revision { > + crate::gpu::Revision {nit: let's import `Revision`, or at least `gpu`, to align with what we do with the other structures.