> On Oct 9, 2025, at 5:37 AM, Alexandre Courbot <[email protected]> wrote: > > The getter method of a field works with the field type, but its setter > expects the type of the register. This leads to an asymmetry in the > From/Into implementations required for a field with a dedicated type. > For instance, a field declared as > > pub struct ControlReg(u32) { > 3:0 mode as u8 ?=> Mode; > ... > } > > currently requires the following implementations: > > impl TryFrom<u8> for Mode { > ... > } > > impl From<Mode> for u32 { > ... > } > > Change this so the `From<Mode>` now needs to be implemented for `u8`, > i.e. the primitive type of the field. This is more consistent, and will > become a requirement once we start using the TryFrom/Into derive macros > to implement these automatically. > > Reported-by: Edwin Peer <[email protected]> > Signed-off-by: Alexandre Courbot <[email protected]> > --- > drivers/gpu/nova-core/falcon.rs | 38 +++++++++++++++++++++++++----------- > drivers/gpu/nova-core/regs/macros.rs | 10 +++++----- > 2 files changed, 32 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs > index 37e6298195e4..3f505b870601 100644 > --- a/drivers/gpu/nova-core/falcon.rs > +++ b/drivers/gpu/nova-core/falcon.rs > @@ -22,11 +22,11 @@ > pub(crate) mod sec2; > > // TODO[FPRI]: Replace with `ToPrimitive`. > -macro_rules! impl_from_enum_to_u32 { > +macro_rules! impl_from_enum_to_u8 { > ($enum_type:ty) => { > - impl From<$enum_type> for u32 { > + impl From<$enum_type> for u8 { > fn from(value: $enum_type) -> Self { > - value as u32 > + value as u8 > } > } > }; > @@ -46,7 +46,7 @@ pub(crate) enum FalconCoreRev { > Rev6 = 6, > Rev7 = 7, > } > -impl_from_enum_to_u32!(FalconCoreRev); > +impl_from_enum_to_u8!(FalconCoreRev); > > // TODO[FPRI]: replace with `FromPrimitive`. > impl TryFrom<u8> for FalconCoreRev { > @@ -81,7 +81,7 @@ pub(crate) enum FalconCoreRevSubversion { > Subversion2 = 2, > Subversion3 = 3, > } > -impl_from_enum_to_u32!(FalconCoreRevSubversion); > +impl_from_enum_to_u8!(FalconCoreRevSubversion); > > // TODO[FPRI]: replace with `FromPrimitive`. > impl TryFrom<u8> for FalconCoreRevSubversion { > @@ -125,7 +125,7 @@ pub(crate) enum FalconSecurityModel { > /// Also known as High-Secure, Privilege Level 3 or PL3. > Heavy = 3, > } > -impl_from_enum_to_u32!(FalconSecurityModel); > +impl_from_enum_to_u8!(FalconSecurityModel); > > // TODO[FPRI]: replace with `FromPrimitive`. > impl TryFrom<u8> for FalconSecurityModel { > @@ -157,7 +157,7 @@ pub(crate) enum FalconModSelAlgo { > #[default] > Rsa3k = 1, > } > -impl_from_enum_to_u32!(FalconModSelAlgo); > +impl_from_enum_to_u8!(FalconModSelAlgo); > > // TODO[FPRI]: replace with `FromPrimitive`. > impl TryFrom<u8> for FalconModSelAlgo { > @@ -179,7 +179,7 @@ pub(crate) enum DmaTrfCmdSize { > #[default] > Size256B = 0x6, > } > -impl_from_enum_to_u32!(DmaTrfCmdSize); > +impl_from_enum_to_u8!(DmaTrfCmdSize); > > // TODO[FPRI]: replace with `FromPrimitive`. > impl TryFrom<u8> for DmaTrfCmdSize { > @@ -202,7 +202,6 @@ pub(crate) enum PeregrineCoreSelect { > /// RISC-V core is active. > Riscv = 1, > } > -impl_from_enum_to_u32!(PeregrineCoreSelect); > > impl From<bool> for PeregrineCoreSelect { > fn from(value: bool) -> Self { > @@ -213,6 +212,15 @@ fn from(value: bool) -> Self { > } > } > > +impl From<PeregrineCoreSelect> for bool { > + fn from(value: PeregrineCoreSelect) -> Self { > + match value { > + PeregrineCoreSelect::Falcon => false, > + PeregrineCoreSelect::Riscv => true, > + } > + } > +} > + > /// Different types of memory present in a falcon core. > #[derive(Debug, Clone, Copy, PartialEq, Eq)] > pub(crate) enum FalconMem { > @@ -236,7 +244,7 @@ pub(crate) enum FalconFbifTarget { > /// Non-coherent system memory (System DRAM). > NoncoherentSysmem = 2, > } > -impl_from_enum_to_u32!(FalconFbifTarget); > +impl_from_enum_to_u8!(FalconFbifTarget); > > // TODO[FPRI]: replace with `FromPrimitive`. > impl TryFrom<u8> for FalconFbifTarget { > @@ -263,7 +271,6 @@ pub(crate) enum FalconFbifMemType { > /// Physical memory addresses. > Physical = 1, > } > -impl_from_enum_to_u32!(FalconFbifMemType); > > /// Conversion from a single-bit register field. > impl From<bool> for FalconFbifMemType { > @@ -275,6 +282,15 @@ fn from(value: bool) -> Self { > } > } > > +impl From<FalconFbifMemType> for bool { > + fn from(value: FalconFbifMemType) -> Self { > + match value { > + FalconFbifMemType::Virtual => false, > + FalconFbifMemType::Physical => true, > + } > + } > +} > + > /// Type used to represent the `PFALCON` registers address base for a given > falcon engine. > pub(crate) struct PFalconBase(()); > > diff --git a/drivers/gpu/nova-core/regs/macros.rs > b/drivers/gpu/nova-core/regs/macros.rs
The examples in the doc comments should also be updated. Otherwise, LGTM. Reviewed-by: Edwin Peer <[email protected]> Regards, Edwin Peer > index 754c14ee7f40..73811a115762 100644 > --- a/drivers/gpu/nova-core/regs/macros.rs > +++ b/drivers/gpu/nova-core/regs/macros.rs > @@ -482,7 +482,7 @@ impl $name { > register!( > @leaf_accessor $name $hi:$lo $field > { |f| <$into_type>::from(if f != 0 { true } else { false }) } > - $into_type => $into_type $(, $comment)?; > + bool $into_type => $into_type $(, $comment)?; > ); > }; > > @@ -499,7 +499,7 @@ impl $name { > $(, $comment:literal)?; > ) => { > register!(@leaf_accessor $name $hi:$lo $field > - { |f| <$try_into_type>::try_from(f as $type) } $try_into_type => > + { |f| <$try_into_type>::try_from(f as $type) } $type > $try_into_type => > ::core::result::Result< > $try_into_type, > <$try_into_type as ::core::convert::TryFrom<$type>>::Error > @@ -513,7 +513,7 @@ impl $name { > $(, $comment:literal)?; > ) => { > register!(@leaf_accessor $name $hi:$lo $field > - { |f| <$into_type>::from(f as $type) } $into_type => $into_type > $(, $comment)?;); > + { |f| <$into_type>::from(f as $type) } $type $into_type => > $into_type $(, $comment)?;); > }; > > // Shortcut for non-boolean fields defined without the `=>` or `?=>` > syntax. > @@ -527,7 +527,7 @@ impl $name { > // Generates the accessor methods for a single field. > ( > @leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident > - { $process:expr } $to_type:ty => $res_type:ty $(, > $comment:literal)?; > + { $process:expr } $prim_type:tt $to_type:ty => $res_type:ty $(, > $comment:literal)?; > ) => { > ::kernel::macros::paste!( > const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = > $lo..=$hi; > @@ -559,7 +559,7 @@ pub(crate) fn $field(self) -> $res_type { > pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self { > const MASK: u32 = $name::[<$field:upper _MASK>]; > const SHIFT: u32 = $name::[<$field:upper _SHIFT>]; > - let value = (u32::from(value) << SHIFT) & MASK; > + let value = (u32::from($prim_type::from(value)) << SHIFT) & MASK; > self.0 = (self.0 & !MASK) | value; > > self > > -- > 2.51.0 >
