Re: [PATCH v2 1/5] drm/tyr: Use register! macro for GPU_CONTROL

Re: [PATCH v2 1/5] drm/tyr: Use register! macro for GPU_CONTROL

[Daniel Almeida]

Hi Deb,

I went through the addresses and ranges again. I couldn’t spot any errors.

To avoid accidentally damaging devices, let’s not include these:

> +        /// Power state key. Write only.
> +        pub(crate) PWR_KEY(u32) @ 0x50 {
> +            /// Set to [`PWR_KEY::KEY_UNLOCK`] to unlock writes to other power state registers.
> +            31:0    key;
> +        }
> +    }
> +
> +    impl PWR_KEY {
> +        /// Key value to unlock writes to other power state registers.
> +        /// This value was generated at random.
> +        pub(crate) const KEY_UNLOCK: u32 = 0x2968A819;
> +    }
> +
> +    register! {
> +        /// Power manager override settings.
> +        pub(crate) PWR_OVERRIDE0(u32) @ 0x54 {
> +            /// Override the PWRUP signal.
> +            1:0     pwrup_override;
> +            /// Override the ISOLATE signal.
> +            3:2     isolate_override;
> +            /// Override the RESET signal.
> +            5:4     reset_override;
> +            /// Override the PWRUP_ACK signal.
> +            9:8     pwrup_ack_override;
> +            /// Override the ISOLATE_ACK signal.
> +            11:10   isolate_ack_override;
> +            /// Override the FUNC_ISOLATE signal.
> +            13:12   func_iso_override;
> +            /// Override the FUNC_ISOLATE_ACK signal.
> +            15:14   func_iso_ack_override;
> +            /// Maximum number of power transitions.
> +            21:16   pwrtrans_limit;
> +            /// Core startup throttling enabled.
> +            23:23   throttle_enable;
> +            /// Maximum number of simultaneous core startups.
> +            29:24   throttle_limit;
> +        }
> +    }
> +
> +    /// Power override mode constants (`pwr_override_t` in hardware spec).
> +    ///
> +    /// These constants can be used with any field in [`PWR_OVERRIDE0`] that ends with
> +    /// the `_override` suffix.
> +    impl PWR_OVERRIDE0 {
> +        /// The signal behaves normally.
> +        pub(crate) const NONE: u32 = 0;
> +        /// The signal is inverted (on when normally off, and off when normally on).
> +        pub(crate) const INVERT: u32 = 1;
> +        /// The signal is always kept on.
> +        pub(crate) const ON: u32 = 2;
> +        /// The signal is always kept off.
> +        pub(crate) const OFF: u32 = 3;
> +    }
> +
> +    register! {
> +        /// Power manager override settings for device manufacturer.
> +        pub(crate) PWR_OVERRIDE1(u32) @ 0x58 {
> +            31:0    pwrtrans_vendor;
> +        }


Alex might have more suggestions as he said, but to me at least, this is acceptable.

— Daniel

Re: [PATCH v2 1/5] drm/tyr: Use register! macro for GPU_CONTROL

[Daniel Almeida]

>> @@ -182,37 +105,67 @@ struct GpuModels {
>>     prod_major: 7,
>> }];
>> 
>> -#[allow(dead_code)]
>> -pub(crate) struct GpuId {
>> -    pub(crate) arch_major: u32,
>> -    pub(crate) arch_minor: u32,
>> -    pub(crate) arch_rev: u32,
>> -    pub(crate) prod_major: u32,
>> -    pub(crate) ver_major: u32,
>> -    pub(crate) ver_minor: u32,
>> -    pub(crate) ver_status: u32,
>> -}
>> -
>> -impl From<u32> for GpuId {
>> -    fn from(value: u32) -> Self {
>> -        GpuId {
>> -            arch_major: (value & genmask_u32(28..=31)) >> 28,
>> -            arch_minor: (value & genmask_u32(24..=27)) >> 24,
>> -            arch_rev: (value & genmask_u32(20..=23)) >> 20,
>> -            prod_major: (value & genmask_u32(16..=19)) >> 16,
>> -            ver_major: (value & genmask_u32(12..=15)) >> 12,
>> -            ver_minor: (value & genmask_u32(4..=11)) >> 4,
>> -            ver_status: value & genmask_u32(0..=3),
>> -        }
>> -    }
>> +pub(crate) fn gpu_info_log(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result {
>> +    let io = (*iomem).access(dev)?;
>> +    let gpu_id = io.read(GPU_ID);
>> +
>> +    let model_name = if let Some(model) = GPU_MODELS.iter().find(|&f| {
>> +        f.arch_major == gpu_id.arch_major().get() && f.prod_major == gpu_id.prod_major().get()
>> +    }) {
>> +        model.name
>> +    } else {
>> +        "unknown"
>> +    };
>> +
>> +    // Create canonical product ID with only arch/product fields, excluding version
>> +    // fields. This ensures the same product at different revisions has the same ID.
>> +    let id = GPU_ID::zeroed()
>> +        .with_arch_major(gpu_id.arch_major())
>> +        .with_arch_minor(gpu_id.arch_minor())
>> +        .with_arch_rev(gpu_id.arch_rev())
>> +        .with_prod_major(gpu_id.prod_major())
>> +        .into_raw();
>> +
>> +    dev_info!(
>> +        dev,
>> +        "mali-{} id 0x{:x} major 0x{:x} minor 0x{:x} status 0x{:x}",
>> +        model_name,
>> +        id,
> 
> This was previously right-shifted by 16. Now, I'm questioning this
> decision to filter out the version fields. I think it'd be better to
> print the raw ID directly. We're already extracting and printing the
> arch major.minor and version status, but if there's anything else
> we want to clearly extract from this raw ID, we could add more.
> 
> TLDR; that's probably one place where I think it's not such a bad idea
> to diverge from Panthor and print an unmodified GPU_ID. Maybe s/id/raw
> GPU ID/ to make the distincting clear. None of this should happen in
> this commit though. Either we do that in a preliminary commit that
> drops the `>> 16`, or we keep the `>> 16` here, and change that in a
> follow-up.

IIRC, Onur recently cleaned this up, hasn’t he?

> 
>> +        gpu_id.ver_major().get(),
>> +        gpu_id.ver_minor().get(),
>> +        gpu_id.ver_status().get()
>> +    );
> 
> [...]
> 
>> +
>> +    impl GPU_COMMAND {
>> +        /// No operation. This is the default value.
>> +        pub(crate) const NOP: u32 = 0;
>> +        /// Reset the GPU.
>> +        pub(crate) const RESET: u32 = 1;
>> +        /// Flush caches.
>> +        pub(crate) const FLUSH_CACHES: u32 = 4;
>> +        /// Clear GPU faults.
>> +        pub(crate) const CLEAR_FAULT: u32 = 7;
>> +    }
>> +
>> +    register! {
>> +        /// GPU command register in reset mode.
>> +        /// Set command to [`GPU_COMMAND::RESET`] to set reset_mode.
>> +        pub(crate) GPU_COMMAND_RESET (u32) => GPU_COMMAND {
>> +            7:0     command;
> 
> Alexandre, dunno how hard it would be to extend this alias syntax to
> provide auto-initialization/expected-value of certain fields, like:
> 
> pub(crate) GPU_COMMAND_RESET (u32) => GPU_COMMAND {
>            7:0     command <- GPU_COMMAND::RESET;
>            11:8    reset_mode;
>        }
> 

+1 to the syntax above. This looks quite ergonomic IMHO.

> so that when you instantiate a GPU_COMMAND_RESET, all you have to set is
> reset_mode and the command gets set to GPU_COMMAND::RESET for you.
> (that's for the write path, for the read path, you'll need some sort of
> match to do the re-interpretation anyway).
> 
> Just to be clear, I'm not asking for any of that in the current
> register!() patchset. It's more a suggestion for a potential future
> improvement.
> 
>> +            11:8    reset_mode;
>> +        }
>> +    }

— Daniel

Re: [PATCH v2 3/5] drm/tyr: Use register! macro for JOB_CONTROL

[Daniel Almeida]

> On 11 Mar 2026, at 20:04, Deborah Brouwer <deborah.brouwer@collabora.com> wrote:
> 
> Convert the JOB_CONTROL register definitions to use the `register!` macro.
> 
> Using the `register!` macro allows us to replace manual bit masks and
> shifts with typed register and field accessors, which makes the code
> easier to read and avoids errors from bit manipulation.
> 
> Co-developed-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: Deborah Brouwer <deborah.brouwer@collabora.com>
> ---
> drivers/gpu/drm/tyr/regs.rs | 58 ++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 50 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tyr/regs.rs b/drivers/gpu/drm/tyr/regs.rs
> index ba61a3dbe2a3e6fa1169b03d4f62e82769041057..686986536297ac2cc53ff14b162b19eaa759c192 100644
> --- a/drivers/gpu/drm/tyr/regs.rs
> +++ b/drivers/gpu/drm/tyr/regs.rs
> @@ -28,7 +28,6 @@
> #![allow(dead_code)]
> 
> use kernel::{
> -    bits::bit_u32,
>     device::{
>         Bound,
>         Device, //
> @@ -628,14 +627,57 @@ impl MCU_STATUS {
> 
> pub(super) use gpu_control::*;
> 
> -pub(crate) const JOB_IRQ_RAWSTAT: Register<0x1000> = Register;
> -pub(crate) const JOB_IRQ_CLEAR: Register<0x1004> = Register;
> -pub(crate) const JOB_IRQ_MASK: Register<0x1008> = Register;
> -pub(crate) const JOB_IRQ_STAT: Register<0x100c> = Register;
> -
> -pub(crate) const JOB_IRQ_GLOBAL_IF: u32 = bit_u32(31);
> -
> pub(crate) const MMU_IRQ_RAWSTAT: Register<0x2000> = Register;
> pub(crate) const MMU_IRQ_CLEAR: Register<0x2004> = Register;
> pub(crate) const MMU_IRQ_MASK: Register<0x2008> = Register;
> pub(crate) const MMU_IRQ_STAT: Register<0x200c> = Register;
> +
> +/// These registers correspond to the JOB_CONTROL register page.
> +/// They are involved in communication between the firmware running on the MCU and the host.
> +pub(super) mod job_control {
> +    use kernel::register;
> +
> +    register! {
> +        /// Raw status of job interrupts.
> +        ///
> +        /// Write to this register to trigger these interrupts.
> +        /// Writing a 1 to a bit forces that bit on.
> +        pub(crate) JOB_IRQ_RAWSTAT(u32) @ 0x1000 {
> +            /// CSG request. These bits indicate that CSGn requires attention from the host.
> +            30:0    csg;
> +            /// GLB request. Indicates that the GLB interface requires attention from the host.
> +            31:31   glb;
> +        }
> +
> +        /// Clear job interrupts. Write only.
> +        ///
> +        /// Write a 1 to a bit to clear the corresponding bit in [`JOB_IRQ_RAWSTAT`].
> +        pub(crate) JOB_IRQ_CLEAR(u32) @ 0x1004 {
> +            /// Clear CSG request interrupts.
> +            30:0    csg;
> +            /// Clear GLB request interrupt.
> +            31:31   glb;
> +        }
> +
> +        /// Mask for job interrupts.
> +        ///
> +        /// Set each bit to 1 to enable the corresponding interrupt source or to 0 to disable it.
> +        pub(crate) JOB_IRQ_MASK(u32) @ 0x1008 {
> +            /// Enable CSG request interrupts.
> +            30:0    csg;
> +            /// Enable GLB request interrupt.
> +            31:31   glb;
> +        }
> +
> +        /// Active job interrupts. Read only.
> +        ///
> +        /// This register contains the result of ANDing together [`JOB_IRQ_RAWSTAT`] and
> +        /// [`JOB_IRQ_MASK`].
> +        pub(crate) JOB_IRQ_STATUS(u32) @ 0x100c {
> +            /// CSG request interrupt status.
> +            30:0    csg;
> +            /// GLB request interrupt status.
> +            31:31   glb;
> +        }
> +    }
> +}
> 
> -- 
> 2.52.0
> 

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH v2 1/5] drm/tyr: Use register! macro for GPU_CONTROL

[Deborah Brouwer]

On Fri, Mar 13, 2026 at 03:29:03PM -0300, Daniel Almeida wrote:
> 
> >> @@ -182,37 +105,67 @@ struct GpuModels {
> >>     prod_major: 7,
> >> }];
> >> 
> >> -#[allow(dead_code)]
> >> -pub(crate) struct GpuId {
> >> -    pub(crate) arch_major: u32,
> >> -    pub(crate) arch_minor: u32,
> >> -    pub(crate) arch_rev: u32,
> >> -    pub(crate) prod_major: u32,
> >> -    pub(crate) ver_major: u32,
> >> -    pub(crate) ver_minor: u32,
> >> -    pub(crate) ver_status: u32,
> >> -}
> >> -
> >> -impl From<u32> for GpuId {
> >> -    fn from(value: u32) -> Self {
> >> -        GpuId {
> >> -            arch_major: (value & genmask_u32(28..=31)) >> 28,
> >> -            arch_minor: (value & genmask_u32(24..=27)) >> 24,
> >> -            arch_rev: (value & genmask_u32(20..=23)) >> 20,
> >> -            prod_major: (value & genmask_u32(16..=19)) >> 16,
> >> -            ver_major: (value & genmask_u32(12..=15)) >> 12,
> >> -            ver_minor: (value & genmask_u32(4..=11)) >> 4,
> >> -            ver_status: value & genmask_u32(0..=3),
> >> -        }
> >> -    }
> >> +pub(crate) fn gpu_info_log(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result {
> >> +    let io = (*iomem).access(dev)?;
> >> +    let gpu_id = io.read(GPU_ID);
> >> +
> >> +    let model_name = if let Some(model) = GPU_MODELS.iter().find(|&f| {
> >> +        f.arch_major == gpu_id.arch_major().get() && f.prod_major == gpu_id.prod_major().get()
> >> +    }) {
> >> +        model.name
> >> +    } else {
> >> +        "unknown"
> >> +    };
> >> +
> >> +    // Create canonical product ID with only arch/product fields, excluding version
> >> +    // fields. This ensures the same product at different revisions has the same ID.
> >> +    let id = GPU_ID::zeroed()
> >> +        .with_arch_major(gpu_id.arch_major())
> >> +        .with_arch_minor(gpu_id.arch_minor())
> >> +        .with_arch_rev(gpu_id.arch_rev())
> >> +        .with_prod_major(gpu_id.prod_major())
> >> +        .into_raw();
> >> +
> >> +    dev_info!(
> >> +        dev,
> >> +        "mali-{} id 0x{:x} major 0x{:x} minor 0x{:x} status 0x{:x}",
> >> +        model_name,
> >> +        id,
> > 
> > This was previously right-shifted by 16. Now, I'm questioning this
> > decision to filter out the version fields. I think it'd be better to
> > print the raw ID directly. We're already extracting and printing the
> > arch major.minor and version status, but if there's anything else
> > we want to clearly extract from this raw ID, we could add more.
> > 
> > TLDR; that's probably one place where I think it's not such a bad idea
> > to diverge from Panthor and print an unmodified GPU_ID. Maybe s/id/raw
> > GPU ID/ to make the distincting clear. None of this should happen in
> > this commit though. Either we do that in a preliminary commit that
> > drops the `>> 16`, or we keep the `>> 16` here, and change that in a
> > follow-up.
> 
> IIRC, Onur recently cleaned this up, hasn’t he?

Onur's patch fixes the model name detection and it's in drm-rust-next:
289cf6f91459 drm/tyr: gpu: fix GpuInfo::log model/version decoding

But it doesn't touch the generation of the id. So, in v3 I'll add a
separate commit to print the GPU_ID without filtering.

> 
> > 
> >> +        gpu_id.ver_major().get(),
> >> +        gpu_id.ver_minor().get(),
> >> +        gpu_id.ver_status().get()
> >> +    );
> > 
> > [...]
> > 
> >> +
> >> +    impl GPU_COMMAND {
> >> +        /// No operation. This is the default value.
> >> +        pub(crate) const NOP: u32 = 0;
> >> +        /// Reset the GPU.
> >> +        pub(crate) const RESET: u32 = 1;
> >> +        /// Flush caches.
> >> +        pub(crate) const FLUSH_CACHES: u32 = 4;
> >> +        /// Clear GPU faults.
> >> +        pub(crate) const CLEAR_FAULT: u32 = 7;
> >> +    }
> >> +
> >> +    register! {
> >> +        /// GPU command register in reset mode.
> >> +        /// Set command to [`GPU_COMMAND::RESET`] to set reset_mode.
> >> +        pub(crate) GPU_COMMAND_RESET (u32) => GPU_COMMAND {
> >> +            7:0     command;
> > 
> > Alexandre, dunno how hard it would be to extend this alias syntax to
> > provide auto-initialization/expected-value of certain fields, like:
> > 
> > pub(crate) GPU_COMMAND_RESET (u32) => GPU_COMMAND {
> >            7:0     command <- GPU_COMMAND::RESET;
> >            11:8    reset_mode;
> >        }
> > 
> 
> +1 to the syntax above. This looks quite ergonomic IMHO.
> 
> > so that when you instantiate a GPU_COMMAND_RESET, all you have to set is
> > reset_mode and the command gets set to GPU_COMMAND::RESET for you.
> > (that's for the write path, for the read path, you'll need some sort of
> > match to do the re-interpretation anyway).
> > 
> > Just to be clear, I'm not asking for any of that in the current
> > register!() patchset. It's more a suggestion for a potential future
> > improvement.
> > 
> >> +            11:8    reset_mode;
> >> +        }
> >> +    }
> 
> — Daniel

Re: [PATCH v2 4/5] drm/tyr: Use register! macro for MMU_CONTROL

[Daniel Almeida]

> On 11 Mar 2026, at 20:04, Deborah Brouwer <deborah.brouwer@collabora.com> wrote:
> 
> Convert the MMU_CONTROL register definitions to use the `register!` macro.
> 
> Using the `register!` macro allows us to replace manual bit masks and
> shifts with typed register and field accessors, which makes the code
> easier to read and avoids errors from bit manipulation.
> 
> Co-developed-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: Deborah Brouwer <deborah.brouwer@collabora.com>
> ---
> drivers/gpu/drm/tyr/regs.rs | 56 +++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 51 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tyr/regs.rs b/drivers/gpu/drm/tyr/regs.rs
> index 686986536297ac2cc53ff14b162b19eaa759c192..6c16a041ab3c36f8aaf785487ad61925be65a026 100644
> --- a/drivers/gpu/drm/tyr/regs.rs
> +++ b/drivers/gpu/drm/tyr/regs.rs
> @@ -627,11 +627,6 @@ impl MCU_STATUS {
> 
> pub(super) use gpu_control::*;
> 
> -pub(crate) const MMU_IRQ_RAWSTAT: Register<0x2000> = Register;
> -pub(crate) const MMU_IRQ_CLEAR: Register<0x2004> = Register;
> -pub(crate) const MMU_IRQ_MASK: Register<0x2008> = Register;
> -pub(crate) const MMU_IRQ_STAT: Register<0x200c> = Register;
> -
> /// These registers correspond to the JOB_CONTROL register page.
> /// They are involved in communication between the firmware running on the MCU and the host.
> pub(super) mod job_control {
> @@ -681,3 +676,54 @@ pub(super) mod job_control {
>         }
>     }
> }
> +
> +/// These registers correspond to the MMU_CONTROL register page.
> +/// They are involved in MMU configuration and control.
> +pub(super) mod mmu_control {

Like Boris, I see no reason for pub(super) instead of pub(crate).

> +    use kernel::register;
> +
> +    register! {
> +        /// IRQ sources raw status.
> +        ///
> +        /// This register contains the raw unmasked interrupt sources for MMU status and exception
> +        /// handling.
> +        ///
> +        /// Writing to this register forces bits on.
> +        /// Use [`IRQ_CLEAR`] to clear interrupts.
> +        pub(crate) IRQ_RAWSTAT(u32) @ 0x2000 {
> +            /// Page fault for address spaces.
> +            15:0    page_fault;
> +            /// Command completed in address spaces.
> +            31:16   command_completed;
> +        }
> +
> +        /// IRQ sources to clear.
> +        /// Write a 1 to a bit to clear the corresponding bit in [`IRQ_RAWSTAT`].
> +        pub(crate) IRQ_CLEAR(u32) @ 0x2004 {
> +            /// Clear the PAGE_FAULT interrupt.
> +            15:0    page_fault;
> +            /// Clear the COMMAND_COMPLETED interrupt.
> +            31:16   command_completed;
> +        }
> +
> +        /// IRQ sources enabled.
> +        ///
> +        /// Set each bit to 1 to enable the corresponding interrupt source, and to 0 to disable it.
> +        pub(crate) IRQ_MASK(u32) @ 0x2008 {
> +            /// Enable the PAGE_FAULT interrupt.
> +            15:0    page_fault;
> +            /// Enable the COMMAND_COMPLETED interrupt.
> +            31:16   command_completed;
> +        }
> +
> +        /// IRQ status for enabled sources. Read only.
> +        ///
> +        /// This register contains the result of ANDing together [`IRQ_RAWSTAT`] and [`IRQ_MASK`].
> +        pub(crate) IRQ_STATUS(u32) @ 0x200c {
> +            /// PAGE_FAULT interrupt status.
> +            15:0    page_fault;
> +            /// COMMAND_COMPLETED interrupt status.
> +            31:16   command_completed;
> +        }
> +    }
> +}
> 
> -- 
> 2.52.0
> 

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH v2 5/5] drm/tyr: Remove custom register struct

[Daniel Almeida]

> On 11 Mar 2026, at 20:04, Deborah Brouwer <deborah.brouwer@collabora.com> wrote:
> 
> Now that Tyr uses the register! macro, it no longer needs to define a
> custom register struct or read/write functions, so delete them.
> 
> Co-developed-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: Deborah Brouwer <deborah.brouwer@collabora.com>
> ---
> drivers/gpu/drm/tyr/regs.rs | 33 ---------------------------------
> 1 file changed, 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tyr/regs.rs b/drivers/gpu/drm/tyr/regs.rs
> index 6c16a041ab3c36f8aaf785487ad61925be65a026..3fc5101c2dcd5d892abc726ea07d75d8f22b5d23 100644
> --- a/drivers/gpu/drm/tyr/regs.rs
> +++ b/drivers/gpu/drm/tyr/regs.rs
> @@ -27,39 +27,6 @@
> // does.
> #![allow(dead_code)]
> 
> -use kernel::{
> -    device::{
> -        Bound,
> -        Device, //
> -    },
> -    devres::Devres,
> -    io::Io,
> -    prelude::*, //
> -};
> -
> -use crate::driver::IoMem;
> -
> -/// Represents a register in the Register Set
> -///
> -/// TODO: Replace this with the Nova `register!()` macro when it is available.
> -/// In particular, this will automatically give us 64bit register reads and
> -/// writes.
> -pub(crate) struct Register<const OFFSET: usize>;
> -
> -impl<const OFFSET: usize> Register<OFFSET> {
> -    #[inline]
> -    pub(crate) fn read(&self, dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result<u32> {
> -        let value = (*iomem).access(dev)?.read32(OFFSET);
> -        Ok(value)
> -    }
> -
> -    #[inline]
> -    pub(crate) fn write(&self, dev: &Device<Bound>, iomem: &Devres<IoMem>, value: u32) -> Result {
> -        (*iomem).access(dev)?.write32(value, OFFSET);
> -        Ok(())
> -    }
> -}
> -
> /// These registers correspond to the GPU_CONTROL register page.
> /// They are involved in GPU configuration and control.
> pub(super) mod gpu_control {
> 
> -- 
> 2.52.0
> 

Ah, much better now. Thanks!

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH v2 1/5] drm/tyr: Use register! macro for GPU_CONTROL

[Alexandre Courbot]

On Thu Mar 12, 2026 at 8:03 AM JST, Deborah Brouwer wrote:
> From: Daniel Almeida <daniel.almeida@collabora.com>
>
> Convert the GPU_CONTROL register definitions to use the `register!` macro.
>
> Using the `register!` macro allows us to replace manual bit masks and
> shifts with typed register and field accessors, which makes the code
> easier to read and avoids errors from bit manipulation.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> Co-developed-by: Deborah Brouwer <deborah.brouwer@collabora.com>
> Signed-off-by: Deborah Brouwer <deborah.brouwer@collabora.com>
> ---
>  drivers/gpu/drm/tyr/driver.rs |  26 +-
>  drivers/gpu/drm/tyr/gpu.rs    | 211 ++++++--------
>  drivers/gpu/drm/tyr/regs.rs   | 644 ++++++++++++++++++++++++++++++++++++++----
>  3 files changed, 687 insertions(+), 194 deletions(-)
>
> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> index 611434641580574ec6b5afa49a8fe79888bb7ace..10c212a3a01910858f02c6d637edff8a263f017b 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs
> @@ -13,7 +13,10 @@
>      devres::Devres,
>      drm,
>      drm::ioctl,
> -    io::poll,
> +    io::{
> +        poll,
> +        Io, //
> +    },
>      new_mutex,
>      of,
>      platform,
> @@ -33,8 +36,11 @@
>      file::TyrDrmFileData,
>      gem::TyrObject,
>      gpu,
> -    gpu::GpuInfo,
> -    regs, //
> +    gpu::{
> +        gpu_info_log, //
> +        GpuInfo,
> +    },
> +    regs::*, //
>  };
>  
>  pub(crate) type IoMem = kernel::io::mem::IoMem<SZ_2M>;
> @@ -78,11 +84,17 @@ unsafe impl Send for TyrDrmDeviceData {}
>  unsafe impl Sync for TyrDrmDeviceData {}
>  
>  fn issue_soft_reset(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result {
> -    regs::GPU_CMD.write(dev, iomem, regs::GPU_CMD_SOFT_RESET)?;
> +    let io = (*iomem).access(dev)?;
> +    io.write_val(
> +        GPU_COMMAND_RESET::zeroed().with_const_reset_mode::<{ GPU_COMMAND_RESET::SOFT_RESET }>(),
> +    );

My biggest piece of feedback for this patchset: replace the const values
with enums and use the `?=>` (or `=>` where possible) syntax to directly
convert your fields from and into them. It associates each field with
its possible set of values and guarantees you won't use a given constant
with the wrong field (and also that you cannot set invalid field values
at all).

It is a bit cumbersome at the moment because you will need to provide
`TryFrom` and `Into` implementations between the enum and the
corresponding bounded type, but it makes setting fields easier and is
the way the API was designed to be used. The `TryFrom/Into` derive macro
work will remove all the boilerplace once it is merged.

>  
>      poll::read_poll_timeout(
> -        || regs::GPU_IRQ_RAWSTAT.read(dev, iomem),
> -        |status| *status & regs::GPU_IRQ_RAWSTAT_RESET_COMPLETED != 0,
> +        || {
> +            let io = (*iomem).access(dev)?;
> +            Ok(io.read(GPU_IRQ_RAWSTAT))
> +        },
> +        |status| status.reset_completed().get() != 0,

Here you can do the following in the declaration of `GPU_IRQ_RAWSTAT`:

    /// Reset has completed.
    8:8     reset_completed => bool;

and change this line to just 

    |status| status.reset_completed(),

You will probably want to do that for most of the single-bit fields,
unless their semantic is different from a boolean value.

An alternative is to use `into_bool` instead of `get` to at least get a
boolean value from the single-bit field.

<snip>
> +pub(crate) fn gpu_info_log(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result {
> +    let io = (*iomem).access(dev)?;
> +    let gpu_id = io.read(GPU_ID);
> +
> +    let model_name = if let Some(model) = GPU_MODELS.iter().find(|&f| {
> +        f.arch_major == gpu_id.arch_major().get() && f.prod_major == gpu_id.prod_major().get()
> +    }) {
> +        model.name
> +    } else {
> +        "unknown"
> +    };
> +
> +    // Create canonical product ID with only arch/product fields, excluding version
> +    // fields. This ensures the same product at different revisions has the same ID.
> +    let id = GPU_ID::zeroed()
> +        .with_arch_major(gpu_id.arch_major())
> +        .with_arch_minor(gpu_id.arch_minor())
> +        .with_arch_rev(gpu_id.arch_rev())
> +        .with_prod_major(gpu_id.prod_major())
> +        .into_raw();

It might be simpler to just clear the values of the fields you don't want.

> +
> +    dev_info!(
> +        dev,
> +        "mali-{} id 0x{:x} major 0x{:x} minor 0x{:x} status 0x{:x}",
> +        model_name,
> +        id,
> +        gpu_id.ver_major().get(),
> +        gpu_id.ver_minor().get(),
> +        gpu_id.ver_status().get()
> +    );

Note that the `Debug` implementation of register types will display
their raw hex value and the individual values of all their fields - you
might want to leverage that.

> +
> +    dev_info!(
> +        dev,
> +        "Features: L2:{:#x} Tiler:{:#x} Mem:{:#x} MMU:{:#x} AS:{:#x}",
> +        io.read(L2_FEATURES).into_raw(),
> +        io.read(TILER_FEATURES).into_raw(),
> +        io.read(MEM_FEATURES).into_raw(),
> +        io.read(MMU_FEATURES).into_raw(),
> +        io.read(AS_PRESENT).into_raw(),
> +    );
> +
> +    dev_info!(
> +        dev,
> +        "shader_present=0x{:016x} l2_present=0x{:016x} tiler_present=0x{:016x}",
> +        io.read(SHADER_PRESENT).into_raw(),
> +        io.read(L2_PRESENT).into_raw(),
> +        io.read(TILER_PRESENT).into_raw(),
> +    );
> +    Ok(())
>  }
>  
>  /// Powers on the l2 block.
>  pub(crate) fn l2_power_on(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result {
> -    regs::L2_PWRON_LO.write(dev, iomem, 1)?;
> +    let io = (*iomem).access(dev)?;
> +    io.write_val(L2_PWRON::zeroed().with_const_request::<1>());
>  
>      poll::read_poll_timeout(
> -        || regs::L2_READY_LO.read(dev, iomem),
> +        || {
> +            let io = (*iomem).access(dev)?;
> +            Ok(io.read(L2_READY).into_raw())
> +        },
>          |status| *status == 1,

Why not

    poll::read_poll_timeout(
        || regs::L2_READY_LO.read(dev, iomem),
        || {
            let io = (*iomem).access(dev)?;
            Ok(io.read(L2_READY))
        },
        |status| status.ready() == 1,

?

<snip>
> +        /// Layout is interpreted differently depending on the command value.
> +        /// Default command is [`GPU_COMMAND::NOP`] with no payload.
> +        pub(crate) GPU_COMMAND (u32) @ 0x30 {
> +            7:0     command;
> +        }
> +    }
> +
> +    impl GPU_COMMAND {
> +        /// No operation. This is the default value.
> +        pub(crate) const NOP: u32 = 0;
> +        /// Reset the GPU.
> +        pub(crate) const RESET: u32 = 1;
> +        /// Flush caches.
> +        pub(crate) const FLUSH_CACHES: u32 = 4;
> +        /// Clear GPU faults.
> +        pub(crate) const CLEAR_FAULT: u32 = 7;
> +    }

So this should really be turned into an enum:

    #[derive(Copy, Clone, Debug)]
    #[repr(u32)]
    enum GpuCommand {
        Nop = 0,
        Reset = 1,
        FlushCaches = 4,
        ClearFault = 7,
    }

    impl TryFrom<Bounded<u32, 8>> for GpuCommand {
        ...
    }

    impl From<GpuCommand> for Bounded<u32, 8> {
        ...
    }

Then `GPU_COMMAND` becomes:

    pub(crate) GPU_COMMAND (u32) @ 0x30 {
        7:0     command ?=> GpuCommand;
    }

... and everything becomes easier, as you can only set valid commands.

I see you also define aliases that reassign the roles of bitfields
depending on the command. I think you can harden that by having an
`impl` block for `GPU_COMMAND` with constructor methods for each command
taking exactly the arguments it needs. These constructors could then
build the proper value using one of the aliases register, otherwise you
are at risk of setting e.g. the `Reset` command on the
`GPU_COMMAND_FLUSH` alias.

> +
> +    register! {
> +        /// GPU command register in reset mode.
> +        /// Set command to [`GPU_COMMAND::RESET`] to set reset_mode.
> +        pub(crate) GPU_COMMAND_RESET (u32) => GPU_COMMAND {
> +            7:0     command;
> +            11:8    reset_mode;
> +        }
> +    }
> +
> +    impl GPU_COMMAND_RESET {
> +        /// Stop all external bus interfaces, then reset the entire GPU.
> +        pub(crate) const SOFT_RESET: u32 = 1;
> +        /// Force a full GPU reset.
> +        pub(crate) const HARD_RESET: u32 = 2;
> +    }
> +
> +    register! {
> +        /// GPU command register in cache flush mode.
> +        /// Set command to [`GPU_COMMAND::FLUSH_CACHES`] to set flush modes.
> +        pub(crate) GPU_COMMAND_FLUSH (u32) => GPU_COMMAND {
> +            7:0     command;
> +            /// L2 cache flush mode.
> +            11:8    l2_flush;
> +            /// Shader core load/store cache flush mode.
> +            15:12   lsc_flush;
> +            /// Shader core other caches flush mode.
> +            19:16   other_flush;
> +        }
> +    }
> +
> +    impl GPU_COMMAND_FLUSH {
> +        /// No flush.
> +        pub(crate) const NONE: u32 = 0;
> +        /// Clean the caches.
> +        pub(crate) const CLEAN: u32 = 1;
> +        /// Invalidate the caches.
> +        pub(crate) const INVALIDATE: u32 = 2;
> +        /// Clean and invalidate the caches.
> +        pub(crate) const CLEAN_INVALIDATE: u32 = 3;
> +    }
> +
> +    register! {
> +        /// GPU status register. Read only.
> +        pub(crate) GPU_STATUS(u32) @ 0x34 {
> +            /// GPU active.
> +            0:0     gpu_active;
> +            /// Power manager active.
> +            1:1     pwr_active;
> +            /// Page fault active.
> +            4:4     page_fault;
> +            /// Protected mode active.
> +            7:7     protected_mode_active;
> +            /// Debug mode active.
> +            8:8     gpu_dbg_enabled;
> +        }
> +
> +        /// GPU fault status register. Read only.
> +        pub(crate) GPU_FAULTSTATUS(u32) @ 0x3c {
> +            /// Exception type.
> +            7:0     exception_type;
> +            /// Access type.
> +            9:8     access_type;
> +            /// The GPU_FAULTADDRESS is valid.
> +            10:10   address_valid;
> +            /// The JASID field is valid.
> +            11:11   jasid_valid;
> +            /// JASID of the fault, if known.
> +            15:12   jasid;
> +            /// ID of the source that triggered the fault.
> +            31:16   source_id;
> +        }
> +    }
> +
> +    impl GPU_FAULTSTATUS {
> +        /// Exception type: No error.
> +        pub(crate) const EXCEPTION_OK: u32 = 0x00;
> +        /// Exception type: GPU external bus error.
> +        pub(crate) const EXCEPTION_GPU_BUS_FAULT: u32 = 0x80;
> +        /// Exception type: GPU shareability error.
> +        pub(crate) const EXCEPTION_GPU_SHAREABILITY_FAULT: u32 = 0x88;
> +        /// Exception type: System shareability error.
> +        pub(crate) const EXCEPTION_SYSTEM_SHAREABILITY_FAULT: u32 = 0x89;
> +        /// Exception type: GPU cacheability error.
> +        pub(crate) const EXCEPTION_GPU_CACHEABILITY_FAULT: u32 = 0x8A;
> +
> +        /// Access type: An atomic (read/write) transaction.
> +        pub(crate) const ACCESS_ATOMIC: u32 = 0;
> +        /// Access type: An execute transaction.
> +        pub(crate) const ACCESS_EXECUTE: u32 = 1;
> +        /// Access type: A read transaction.
> +        pub(crate) const ACCESS_READ: u32 = 2;
> +        /// Access type: A write transaction.
> +        pub(crate) const ACCESS_WRITE: u32 = 3;

Since these consts cover all the possible values of `access_type`, you
can use the `=>` syntax for it and implement `From<Bounded<u32, 2>>`
instead of `TryFrom`. This will make the getter infallible as there are
no invalid values.

> +    }
> +
> +    register! {
> +        /// GPU fault address. Read only.
> +        /// Once a fault is reported, it must be manually cleared by issuing a
> +        /// [`GPU_COMMAND::CLEAR_FAULT`] command to the [`GPU_COMMAND`] register. No further GPU
> +        /// faults will be reported until the previous fault has been cleared.
> +        pub(crate) GPU_FAULTADDRESS(u64) @ 0x40 {
> +            63:0    pointer;
> +        }
> +
> +        /// Level 2 cache configuration.
> +        pub(crate) L2_CONFIG(u32) @ 0x48 {
> +            /// Requested cache size.
> +            23:16   cache_size;
> +            /// Requested hash function index.
> +            31:24   hash_function;
> +        }
> +
> +        /// Power state key. Write only.
> +        pub(crate) PWR_KEY(u32) @ 0x50 {
> +            /// Set to [`PWR_KEY::KEY_UNLOCK`] to unlock writes to other power state registers.
> +            31:0    key;
> +        }
> +    }
> +
> +    impl PWR_KEY {
> +        /// Key value to unlock writes to other power state registers.
> +        /// This value was generated at random.
> +        pub(crate) const KEY_UNLOCK: u32 = 0x2968A819;

Note that you can also create constants of the register type directly,
so you don't need to reconstruct one using this value.

> +    }
> +
> +    register! {
> +        /// Power manager override settings.
> +        pub(crate) PWR_OVERRIDE0(u32) @ 0x54 {
> +            /// Override the PWRUP signal.
> +            1:0     pwrup_override;
> +            /// Override the ISOLATE signal.
> +            3:2     isolate_override;
> +            /// Override the RESET signal.
> +            5:4     reset_override;
> +            /// Override the PWRUP_ACK signal.
> +            9:8     pwrup_ack_override;
> +            /// Override the ISOLATE_ACK signal.
> +            11:10   isolate_ack_override;
> +            /// Override the FUNC_ISOLATE signal.
> +            13:12   func_iso_override;
> +            /// Override the FUNC_ISOLATE_ACK signal.
> +            15:14   func_iso_ack_override;
> +            /// Maximum number of power transitions.
> +            21:16   pwrtrans_limit;
> +            /// Core startup throttling enabled.
> +            23:23   throttle_enable;
> +            /// Maximum number of simultaneous core startups.
> +            29:24   throttle_limit;
> +        }
> +    }
> +
> +    /// Power override mode constants (`pwr_override_t` in hardware spec).
> +    ///
> +    /// These constants can be used with any field in [`PWR_OVERRIDE0`] that ends with
> +    /// the `_override` suffix.
> +    impl PWR_OVERRIDE0 {
> +        /// The signal behaves normally.
> +        pub(crate) const NONE: u32 = 0;
> +        /// The signal is inverted (on when normally off, and off when normally on).
> +        pub(crate) const INVERT: u32 = 1;
> +        /// The signal is always kept on.
> +        pub(crate) const ON: u32 = 2;
> +        /// The signal is always kept off.
> +        pub(crate) const OFF: u32 = 3;
> +    }
> +
> +    register! {
> +        /// Power manager override settings for device manufacturer.
> +        pub(crate) PWR_OVERRIDE1(u32) @ 0x58 {
> +            31:0    pwrtrans_vendor;
> +        }
> +
> +        /// Global time stamp offset.
> +        pub(crate) TIMESTAMP_OFFSET(u64) @ 0x88 {
> +            63:0    offset;
> +        }
> +
> +        /// GPU cycle counter. Read only.
> +        pub(crate) CYCLE_COUNT(u64) @ 0x90 {
> +            63:0    count;
> +        }
> +
> +        /// Global time stamp. Read only.
> +        pub(crate) TIMESTAMP(u64) @ 0x98 {
> +            63:0    timestamp;
> +        }
> +
> +        /// Maximum number of threads per core. Read only constant.
> +        pub(crate) THREAD_MAX_THREADS(u32) @ 0xa0 {
> +            31:0    threads;
> +        }
> +
> +        /// Maximum number of threads per workgroup. Read only constant.
> +        pub(crate) THREAD_MAX_WORKGROUP_SIZE(u32) @ 0xa4 {
> +            31:0    threads;
> +        }
> +
> +        /// Maximum number of threads per barrier. Read only constant.
> +        pub(crate) THREAD_MAX_BARRIER_SIZE(u32) @ 0xa8 {
> +            31:0    threads;
> +        }
> +
> +        /// Thread features. Read only constant.
> +        pub(crate) THREAD_FEATURES(u32) @ 0xac {
> +            /// Total number of registers per core.
> +            21:0    max_registers;
> +            /// Implementation technology type.
> +            23:22   implementation_technology;

Here as well you will probably want an enum type for the values.

Re: [PATCH v2 1/5] drm/tyr: Use register! macro for GPU_CONTROL

[Deborah Brouwer]

On Wed, Mar 18, 2026 at 12:14:26PM +0900, Alexandre Courbot wrote:
> On Thu Mar 12, 2026 at 8:03 AM JST, Deborah Brouwer wrote:
> > From: Daniel Almeida <daniel.almeida@collabora.com>
> >
> > Convert the GPU_CONTROL register definitions to use the `register!` macro.
> >
> > Using the `register!` macro allows us to replace manual bit masks and
> > shifts with typed register and field accessors, which makes the code
> > easier to read and avoids errors from bit manipulation.
> >
> > Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> > Co-developed-by: Deborah Brouwer <deborah.brouwer@collabora.com>
> > Signed-off-by: Deborah Brouwer <deborah.brouwer@collabora.com>
> > ---
> >  drivers/gpu/drm/tyr/driver.rs |  26 +-
> >  drivers/gpu/drm/tyr/gpu.rs    | 211 ++++++--------
> >  drivers/gpu/drm/tyr/regs.rs   | 644 ++++++++++++++++++++++++++++++++++++++----
> >  3 files changed, 687 insertions(+), 194 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> > index 611434641580574ec6b5afa49a8fe79888bb7ace..10c212a3a01910858f02c6d637edff8a263f017b 100644
> > --- a/drivers/gpu/drm/tyr/driver.rs
> > +++ b/drivers/gpu/drm/tyr/driver.rs
> > @@ -13,7 +13,10 @@
> >      devres::Devres,
> >      drm,
> >      drm::ioctl,
> > -    io::poll,
> > +    io::{
> > +        poll,
> > +        Io, //
> > +    },
> >      new_mutex,
> >      of,
> >      platform,
> > @@ -33,8 +36,11 @@
> >      file::TyrDrmFileData,
> >      gem::TyrObject,
> >      gpu,
> > -    gpu::GpuInfo,
> > -    regs, //
> > +    gpu::{
> > +        gpu_info_log, //
> > +        GpuInfo,
> > +    },
> > +    regs::*, //
> >  };
> >  
> >  pub(crate) type IoMem = kernel::io::mem::IoMem<SZ_2M>;
> > @@ -78,11 +84,17 @@ unsafe impl Send for TyrDrmDeviceData {}
> >  unsafe impl Sync for TyrDrmDeviceData {}
> >  
> >  fn issue_soft_reset(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result {
> > -    regs::GPU_CMD.write(dev, iomem, regs::GPU_CMD_SOFT_RESET)?;
> > +    let io = (*iomem).access(dev)?;
> > +    io.write_val(
> > +        GPU_COMMAND_RESET::zeroed().with_const_reset_mode::<{ GPU_COMMAND_RESET::SOFT_RESET }>(),
> > +    );
> 
> My biggest piece of feedback for this patchset: replace the const values
> with enums and use the `?=>` (or `=>` where possible) syntax to directly
> convert your fields from and into them. It associates each field with
> its possible set of values and guarantees you won't use a given constant
> with the wrong field (and also that you cannot set invalid field values
> at all).
> 
> It is a bit cumbersome at the moment because you will need to provide
> `TryFrom` and `Into` implementations between the enum and the
> corresponding bounded type, but it makes setting fields easier and is
> the way the API was designed to be used. The `TryFrom/Into` derive macro
> work will remove all the boilerplace once it is merged.

Ok, thanks for this feedback, I definitely didn't understand this syntax
for the register macro and will use it now.

> 
> >  
> >      poll::read_poll_timeout(
> > -        || regs::GPU_IRQ_RAWSTAT.read(dev, iomem),
> > -        |status| *status & regs::GPU_IRQ_RAWSTAT_RESET_COMPLETED != 0,
> > +        || {
> > +            let io = (*iomem).access(dev)?;
> > +            Ok(io.read(GPU_IRQ_RAWSTAT))
> > +        },
> > +        |status| status.reset_completed().get() != 0,
> 
> Here you can do the following in the declaration of `GPU_IRQ_RAWSTAT`:
> 
>     /// Reset has completed.
>     8:8     reset_completed => bool;
> 
> and change this line to just 
> 
>     |status| status.reset_completed(),
> 
> You will probably want to do that for most of the single-bit fields,
> unless their semantic is different from a boolean value.

Yep, I will include that in v3. I have checked so far all of those fields
are just 1-bit boolean flags.

> 
> An alternative is to use `into_bool` instead of `get` to at least get a
> boolean value from the single-bit field.
> 
> <snip>
> > +pub(crate) fn gpu_info_log(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result {
> > +    let io = (*iomem).access(dev)?;
> > +    let gpu_id = io.read(GPU_ID);
> > +
> > +    let model_name = if let Some(model) = GPU_MODELS.iter().find(|&f| {
> > +        f.arch_major == gpu_id.arch_major().get() && f.prod_major == gpu_id.prod_major().get()
> > +    }) {
> > +        model.name
> > +    } else {
> > +        "unknown"
> > +    };
> > +
> > +    // Create canonical product ID with only arch/product fields, excluding version
> > +    // fields. This ensures the same product at different revisions has the same ID.
> > +    let id = GPU_ID::zeroed()
> > +        .with_arch_major(gpu_id.arch_major())
> > +        .with_arch_minor(gpu_id.arch_minor())
> > +        .with_arch_rev(gpu_id.arch_rev())
> > +        .with_prod_major(gpu_id.prod_major())
> > +        .into_raw();
> 
> It might be simpler to just clear the values of the fields you don't want.

I'm dropping this part in v3 and will just print the whole register.

> 
> > +
> > +    dev_info!(
> > +        dev,
> > +        "mali-{} id 0x{:x} major 0x{:x} minor 0x{:x} status 0x{:x}",
> > +        model_name,
> > +        id,
> > +        gpu_id.ver_major().get(),
> > +        gpu_id.ver_minor().get(),
> > +        gpu_id.ver_status().get()
> > +    );
> 
> Note that the `Debug` implementation of register types will display
> their raw hex value and the individual values of all their fields - you
> might want to leverage that.

I gave this a try but the "Bounded" around the values makes it a bit
hard to read i think. Compactly it would look like this:

gpu: mali-g610 GPU_ID { <raw>: 0xa8670005, ver_status: Bounded(5),
ver_minor: Bounded(0), ver_major: Bounded(0), prod_major: Bounded(7),
arch_rev: Bounded(6), arch_minor: Bounded(8), arch_major: Bounded(10) }

> 
> > +
> > +    dev_info!(
> > +        dev,
> > +        "Features: L2:{:#x} Tiler:{:#x} Mem:{:#x} MMU:{:#x} AS:{:#x}",
> > +        io.read(L2_FEATURES).into_raw(),
> > +        io.read(TILER_FEATURES).into_raw(),
> > +        io.read(MEM_FEATURES).into_raw(),
> > +        io.read(MMU_FEATURES).into_raw(),
> > +        io.read(AS_PRESENT).into_raw(),
> > +    );
> > +
> > +    dev_info!(
> > +        dev,
> > +        "shader_present=0x{:016x} l2_present=0x{:016x} tiler_present=0x{:016x}",
> > +        io.read(SHADER_PRESENT).into_raw(),
> > +        io.read(L2_PRESENT).into_raw(),
> > +        io.read(TILER_PRESENT).into_raw(),
> > +    );
> > +    Ok(())
> >  }
> >  
> >  /// Powers on the l2 block.
> >  pub(crate) fn l2_power_on(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result {
> > -    regs::L2_PWRON_LO.write(dev, iomem, 1)?;
> > +    let io = (*iomem).access(dev)?;
> > +    io.write_val(L2_PWRON::zeroed().with_const_request::<1>());
> >  
> >      poll::read_poll_timeout(
> > -        || regs::L2_READY_LO.read(dev, iomem),
> > +        || {
> > +            let io = (*iomem).access(dev)?;
> > +            Ok(io.read(L2_READY).into_raw())
> > +        },
> >          |status| *status == 1,
> 
> Why not
> 
>     poll::read_poll_timeout(
>         || regs::L2_READY_LO.read(dev, iomem),
>         || {
>             let io = (*iomem).access(dev)?;
>             Ok(io.read(L2_READY))
>         },
>         |status| status.ready() == 1,
> 
> ?
Thanks, will fix.

> 
> <snip>
> > +        /// Layout is interpreted differently depending on the command value.
> > +        /// Default command is [`GPU_COMMAND::NOP`] with no payload.
> > +        pub(crate) GPU_COMMAND (u32) @ 0x30 {
> > +            7:0     command;
> > +        }
> > +    }
> > +
> > +    impl GPU_COMMAND {
> > +        /// No operation. This is the default value.
> > +        pub(crate) const NOP: u32 = 0;
> > +        /// Reset the GPU.
> > +        pub(crate) const RESET: u32 = 1;
> > +        /// Flush caches.
> > +        pub(crate) const FLUSH_CACHES: u32 = 4;
> > +        /// Clear GPU faults.
> > +        pub(crate) const CLEAR_FAULT: u32 = 7;
> > +    }
> 
> So this should really be turned into an enum:
> 
>     #[derive(Copy, Clone, Debug)]
>     #[repr(u32)]
>     enum GpuCommand {
>         Nop = 0,
>         Reset = 1,
>         FlushCaches = 4,
>         ClearFault = 7,
>     }
> 
>     impl TryFrom<Bounded<u32, 8>> for GpuCommand {
>         ...
>     }
> 
>     impl From<GpuCommand> for Bounded<u32, 8> {
>         ...
>     }
> 
> Then `GPU_COMMAND` becomes:
> 
>     pub(crate) GPU_COMMAND (u32) @ 0x30 {
>         7:0     command ?=> GpuCommand;
>     }
> 
> ... and everything becomes easier, as you can only set valid commands.
> 
> I see you also define aliases that reassign the roles of bitfields
> depending on the command. I think you can harden that by having an
> `impl` block for `GPU_COMMAND` with constructor methods for each command
> taking exactly the arguments it needs. These constructors could then
> build the proper value using one of the aliases register, otherwise you
> are at risk of setting e.g. the `Reset` command on the
> `GPU_COMMAND_FLUSH` alias.

I'll add this to v3. So, instead of implementing a default() for each
alias register, I will make those alias registers private and force them
to be used through GPU_COMMAND methods like this:

impl GPU_COMMAND {
        pub(crate) fn reset(mode: ResetMode) -> Self {
            Self::from_raw(
                GPU_COMMAND_RESET::zeroed()
                    .with_command(GpuCommand::Reset)
                    .with_reset_mode(mode)
                    .into_raw(),
            )
        }

and use it like:
 io.write_reg(GPU_COMMAND::reset(ResetMode::SoftReset));

> 
> > +
> > +    register! {
> > +        /// GPU command register in reset mode.
> > +        /// Set command to [`GPU_COMMAND::RESET`] to set reset_mode.
> > +        pub(crate) GPU_COMMAND_RESET (u32) => GPU_COMMAND {
> > +            7:0     command;
> > +            11:8    reset_mode;
> > +        }
> > +    }
> > +
> > +    impl GPU_COMMAND_RESET {
> > +        /// Stop all external bus interfaces, then reset the entire GPU.
> > +        pub(crate) const SOFT_RESET: u32 = 1;
> > +        /// Force a full GPU reset.
> > +        pub(crate) const HARD_RESET: u32 = 2;
> > +    }
> > +
> > +    register! {
> > +        /// GPU command register in cache flush mode.
> > +        /// Set command to [`GPU_COMMAND::FLUSH_CACHES`] to set flush modes.
> > +        pub(crate) GPU_COMMAND_FLUSH (u32) => GPU_COMMAND {
> > +            7:0     command;
> > +            /// L2 cache flush mode.
> > +            11:8    l2_flush;
> > +            /// Shader core load/store cache flush mode.
> > +            15:12   lsc_flush;
> > +            /// Shader core other caches flush mode.
> > +            19:16   other_flush;
> > +        }
> > +    }
> > +
> > +    impl GPU_COMMAND_FLUSH {
> > +        /// No flush.
> > +        pub(crate) const NONE: u32 = 0;
> > +        /// Clean the caches.
> > +        pub(crate) const CLEAN: u32 = 1;
> > +        /// Invalidate the caches.
> > +        pub(crate) const INVALIDATE: u32 = 2;
> > +        /// Clean and invalidate the caches.
> > +        pub(crate) const CLEAN_INVALIDATE: u32 = 3;
> > +    }
> > +
> > +    register! {
> > +        /// GPU status register. Read only.
> > +        pub(crate) GPU_STATUS(u32) @ 0x34 {
> > +            /// GPU active.
> > +            0:0     gpu_active;
> > +            /// Power manager active.
> > +            1:1     pwr_active;
> > +            /// Page fault active.
> > +            4:4     page_fault;
> > +            /// Protected mode active.
> > +            7:7     protected_mode_active;
> > +            /// Debug mode active.
> > +            8:8     gpu_dbg_enabled;
> > +        }
> > +
> > +        /// GPU fault status register. Read only.
> > +        pub(crate) GPU_FAULTSTATUS(u32) @ 0x3c {
> > +            /// Exception type.
> > +            7:0     exception_type;
> > +            /// Access type.
> > +            9:8     access_type;
> > +            /// The GPU_FAULTADDRESS is valid.
> > +            10:10   address_valid;
> > +            /// The JASID field is valid.
> > +            11:11   jasid_valid;
> > +            /// JASID of the fault, if known.
> > +            15:12   jasid;
> > +            /// ID of the source that triggered the fault.
> > +            31:16   source_id;
> > +        }
> > +    }
> > +
> > +    impl GPU_FAULTSTATUS {
> > +        /// Exception type: No error.
> > +        pub(crate) const EXCEPTION_OK: u32 = 0x00;
> > +        /// Exception type: GPU external bus error.
> > +        pub(crate) const EXCEPTION_GPU_BUS_FAULT: u32 = 0x80;
> > +        /// Exception type: GPU shareability error.
> > +        pub(crate) const EXCEPTION_GPU_SHAREABILITY_FAULT: u32 = 0x88;
> > +        /// Exception type: System shareability error.
> > +        pub(crate) const EXCEPTION_SYSTEM_SHAREABILITY_FAULT: u32 = 0x89;
> > +        /// Exception type: GPU cacheability error.
> > +        pub(crate) const EXCEPTION_GPU_CACHEABILITY_FAULT: u32 = 0x8A;
> > +
> > +        /// Access type: An atomic (read/write) transaction.
> > +        pub(crate) const ACCESS_ATOMIC: u32 = 0;
> > +        /// Access type: An execute transaction.
> > +        pub(crate) const ACCESS_EXECUTE: u32 = 1;
> > +        /// Access type: A read transaction.
> > +        pub(crate) const ACCESS_READ: u32 = 2;
> > +        /// Access type: A write transaction.
> > +        pub(crate) const ACCESS_WRITE: u32 = 3;
> 
> Since these consts cover all the possible values of `access_type`, you
> can use the `=>` syntax for it and implement `From<Bounded<u32, 2>>`
> instead of `TryFrom`. This will make the getter infallible as there are
> no invalid values.

Thanks, that works.

> 
> > +    }
> > +
> > +    register! {
> > +        /// GPU fault address. Read only.
> > +        /// Once a fault is reported, it must be manually cleared by issuing a
> > +        /// [`GPU_COMMAND::CLEAR_FAULT`] command to the [`GPU_COMMAND`] register. No further GPU
> > +        /// faults will be reported until the previous fault has been cleared.
> > +        pub(crate) GPU_FAULTADDRESS(u64) @ 0x40 {
> > +            63:0    pointer;
> > +        }
> > +
> > +        /// Level 2 cache configuration.
> > +        pub(crate) L2_CONFIG(u32) @ 0x48 {
> > +            /// Requested cache size.
> > +            23:16   cache_size;
> > +            /// Requested hash function index.
> > +            31:24   hash_function;
> > +        }
> > +
> > +        /// Power state key. Write only.
> > +        pub(crate) PWR_KEY(u32) @ 0x50 {
> > +            /// Set to [`PWR_KEY::KEY_UNLOCK`] to unlock writes to other power state registers.
> > +            31:0    key;
> > +        }
> > +    }
> > +
> > +    impl PWR_KEY {
> > +        /// Key value to unlock writes to other power state registers.
> > +        /// This value was generated at random.
> > +        pub(crate) const KEY_UNLOCK: u32 = 0x2968A819;
> 
> Note that you can also create constants of the register type directly,
> so you don't need to reconstruct one using this value.
>

I'm going to take this constant out for v3 as Daniel requested, but
will keep this in mind for future registers.

> > +    }
> > +
> > +    register! {
> > +        /// Power manager override settings.
> > +        pub(crate) PWR_OVERRIDE0(u32) @ 0x54 {
> > +            /// Override the PWRUP signal.
> > +            1:0     pwrup_override;
> > +            /// Override the ISOLATE signal.
> > +            3:2     isolate_override;
> > +            /// Override the RESET signal.
> > +            5:4     reset_override;
> > +            /// Override the PWRUP_ACK signal.
> > +            9:8     pwrup_ack_override;
> > +            /// Override the ISOLATE_ACK signal.
> > +            11:10   isolate_ack_override;
> > +            /// Override the FUNC_ISOLATE signal.
> > +            13:12   func_iso_override;
> > +            /// Override the FUNC_ISOLATE_ACK signal.
> > +            15:14   func_iso_ack_override;
> > +            /// Maximum number of power transitions.
> > +            21:16   pwrtrans_limit;
> > +            /// Core startup throttling enabled.
> > +            23:23   throttle_enable;
> > +            /// Maximum number of simultaneous core startups.
> > +            29:24   throttle_limit;
> > +        }
> > +    }
> > +
> > +    /// Power override mode constants (`pwr_override_t` in hardware spec).
> > +    ///
> > +    /// These constants can be used with any field in [`PWR_OVERRIDE0`] that ends with
> > +    /// the `_override` suffix.
> > +    impl PWR_OVERRIDE0 {
> > +        /// The signal behaves normally.
> > +        pub(crate) const NONE: u32 = 0;
> > +        /// The signal is inverted (on when normally off, and off when normally on).
> > +        pub(crate) const INVERT: u32 = 1;
> > +        /// The signal is always kept on.
> > +        pub(crate) const ON: u32 = 2;
> > +        /// The signal is always kept off.
> > +        pub(crate) const OFF: u32 = 3;
> > +    }
> > +
> > +    register! {
> > +        /// Power manager override settings for device manufacturer.
> > +        pub(crate) PWR_OVERRIDE1(u32) @ 0x58 {
> > +            31:0    pwrtrans_vendor;
> > +        }
> > +
> > +        /// Global time stamp offset.
> > +        pub(crate) TIMESTAMP_OFFSET(u64) @ 0x88 {
> > +            63:0    offset;
> > +        }
> > +
> > +        /// GPU cycle counter. Read only.
> > +        pub(crate) CYCLE_COUNT(u64) @ 0x90 {
> > +            63:0    count;
> > +        }
> > +
> > +        /// Global time stamp. Read only.
> > +        pub(crate) TIMESTAMP(u64) @ 0x98 {
> > +            63:0    timestamp;
> > +        }
> > +
> > +        /// Maximum number of threads per core. Read only constant.
> > +        pub(crate) THREAD_MAX_THREADS(u32) @ 0xa0 {
> > +            31:0    threads;
> > +        }
> > +
> > +        /// Maximum number of threads per workgroup. Read only constant.
> > +        pub(crate) THREAD_MAX_WORKGROUP_SIZE(u32) @ 0xa4 {
> > +            31:0    threads;
> > +        }
> > +
> > +        /// Maximum number of threads per barrier. Read only constant.
> > +        pub(crate) THREAD_MAX_BARRIER_SIZE(u32) @ 0xa8 {
> > +            31:0    threads;
> > +        }
> > +
> > +        /// Thread features. Read only constant.
> > +        pub(crate) THREAD_FEATURES(u32) @ 0xac {
> > +            /// Total number of registers per core.
> > +            21:0    max_registers;
> > +            /// Implementation technology type.
> > +            23:22   implementation_technology;
> 
> Here as well you will probably want an enum type for the values.

I've changed all the constants to enums for v3 which I will send
probably next week. Thanks again for this macro and your review.