drivers/gpu/drm/tyr/driver.rs | 15 ++- drivers/gpu/drm/tyr/gpu.rs | 55 ++++---- drivers/gpu/drm/tyr/regs.rs | 302 ++++++++++++++++++++++++++++++++---------- 3 files changed, 267 insertions(+), 105 deletions(-)
Replace regs::Register with kernel::register. This allow us to more
succinctly express the register set by introducing the ability to describe
fields and their documentation and to auto-generate the accessors. In
particular, this is very helpful as it does away with a lot of manual masks
and shifts.
A future commit will eliminate HI/LO pairs once there is support for 64bit
reads and writes in kernel::register.
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
Note that this patch depends on a rebased version of Joel's patch at [0].
That version is stale, so I ended up rebasing it locally myself for the
purpose of developing this patch and gathering some reviews on the list. In
other words, the current patch does not apply for the time being, but will
once a v7 for Joel's series is out.
[0]: https://lore.kernel.org/rust-for-linux/20251003154748.1687160-1-joelagnelf@nvidia.com/
---
drivers/gpu/drm/tyr/driver.rs | 15 ++-
drivers/gpu/drm/tyr/gpu.rs | 55 ++++----
drivers/gpu/drm/tyr/regs.rs | 302 ++++++++++++++++++++++++++++++++----------
3 files changed, 267 insertions(+), 105 deletions(-)
diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
index 0389c558c036..8e06db5320bf 100644
--- a/drivers/gpu/drm/tyr/driver.rs
+++ b/drivers/gpu/drm/tyr/driver.rs
@@ -66,19 +66,20 @@ unsafe impl Send for TyrData {}
unsafe impl Sync for TyrData {}
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)?;
+
+ regs::GpuCommand::default()
+ .set_command(regs::GPU_CMD_SOFT_RESET)
+ .write(io);
// TODO: We cannot poll, as there is no support in Rust currently, so we
// sleep. Change this when read_poll_timeout() is implemented in Rust.
kernel::time::delay::fsleep(time::Delta::from_millis(100));
- if regs::GPU_IRQ_RAWSTAT.read(dev, iomem)? & regs::GPU_IRQ_RAWSTAT_RESET_COMPLETED == 0 {
+ let rawstat = regs::GpuIrqRawstat::read(io);
+ if !rawstat.reset_completed() {
dev_err!(dev, "GPU reset failed with errno\n");
- dev_err!(
- dev,
- "GPU_INT_RAWSTAT is {}\n",
- regs::GPU_IRQ_RAWSTAT.read(dev, iomem)?
- );
+ dev_err!(dev, "GPU_INT_RAWSTAT is {}\n", u32::from(rawstat));
return Err(EIO);
}
diff --git a/drivers/gpu/drm/tyr/gpu.rs b/drivers/gpu/drm/tyr/gpu.rs
index 6c582910dd5d..7c698fb1e36a 100644
--- a/drivers/gpu/drm/tyr/gpu.rs
+++ b/drivers/gpu/drm/tyr/gpu.rs
@@ -44,34 +44,36 @@ pub(crate) struct GpuInfo {
impl GpuInfo {
pub(crate) fn new(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result<Self> {
- let gpu_id = regs::GPU_ID.read(dev, iomem)?;
- let csf_id = regs::GPU_CSF_ID.read(dev, iomem)?;
- let gpu_rev = regs::GPU_REVID.read(dev, iomem)?;
- let core_features = regs::GPU_CORE_FEATURES.read(dev, iomem)?;
- let l2_features = regs::GPU_L2_FEATURES.read(dev, iomem)?;
- let tiler_features = regs::GPU_TILER_FEATURES.read(dev, iomem)?;
- let mem_features = regs::GPU_MEM_FEATURES.read(dev, iomem)?;
- let mmu_features = regs::GPU_MMU_FEATURES.read(dev, iomem)?;
- let thread_features = regs::GPU_THREAD_FEATURES.read(dev, iomem)?;
- let max_threads = regs::GPU_THREAD_MAX_THREADS.read(dev, iomem)?;
- let thread_max_workgroup_size = regs::GPU_THREAD_MAX_WORKGROUP_SIZE.read(dev, iomem)?;
- let thread_max_barrier_size = regs::GPU_THREAD_MAX_BARRIER_SIZE.read(dev, iomem)?;
- let coherency_features = regs::GPU_COHERENCY_FEATURES.read(dev, iomem)?;
-
- let texture_features = regs::GPU_TEXTURE_FEATURES0.read(dev, iomem)?;
-
- let as_present = regs::GPU_AS_PRESENT.read(dev, iomem)?;
-
- let shader_present = u64::from(regs::GPU_SHADER_PRESENT_LO.read(dev, iomem)?);
+ let io = (*iomem).access(dev)?;
+
+ let gpu_id = regs::GpuId::read(io).into();
+ let csf_id = regs::CsfId::read(io).into();
+ let gpu_rev = regs::RevIdr::read(io).into();
+ let core_features = regs::CoreFeatures::read(io).into();
+ let l2_features = regs::L2Features::read(io).into();
+ let tiler_features = regs::TilerFeatures::read(io).into();
+ let mem_features = regs::MemFeatures::read(io).into();
+ let mmu_features = regs::MmuFeatures::read(io).into();
+ let thread_features = regs::ThreadFeatures::read(io).into();
+ let max_threads = regs::ThreadMaxThreads::read(io).into();
+ let thread_max_workgroup_size = regs::ThreadMaxWorkgroupSize::read(io).into();
+ let thread_max_barrier_size = regs::ThreadMaxBarrierSize::read(io).into();
+ let coherency_features = regs::CoherencyFeatures::read(io).into();
+
+ let texture_features = regs::TextureFeatures::read(io, 0).into();
+
+ let as_present = regs::AsPresent::read(io).into();
+
+ let shader_present = u64::from(u32::from(regs::ShaderPresentLo::read(io)));
let shader_present =
- shader_present | u64::from(regs::GPU_SHADER_PRESENT_HI.read(dev, iomem)?) << 32;
+ shader_present | u64::from(u32::from(regs::ShaderPresentHi::read(io))) << 32;
- let tiler_present = u64::from(regs::GPU_TILER_PRESENT_LO.read(dev, iomem)?);
+ let tiler_present = u64::from(u32::from(regs::TilerPresentLo::read(io)));
let tiler_present =
- tiler_present | u64::from(regs::GPU_TILER_PRESENT_HI.read(dev, iomem)?) << 32;
+ tiler_present | u64::from(u32::from(regs::TilerPresentHi::read(io))) << 32;
- let l2_present = u64::from(regs::GPU_L2_PRESENT_LO.read(dev, iomem)?);
- let l2_present = l2_present | u64::from(regs::GPU_L2_PRESENT_HI.read(dev, iomem)?) << 32;
+ let l2_present = u64::from(u32::from(regs::L2PresentLo::read(io)));
+ let l2_present = l2_present | u64::from(u32::from(regs::L2PresentHi::read(io))) << 32;
Ok(Self {
gpu_id,
@@ -204,13 +206,14 @@ fn from(value: u32) -> Self {
/// 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)?;
+ regs::L2PwrOnLo::default().set_l2_pwron_lo(1).write(io);
// TODO: We cannot poll, as there is no support in Rust currently, so we
// sleep. Change this when read_poll_timeout() is implemented in Rust.
kernel::time::delay::fsleep(time::Delta::from_millis(100));
- if regs::L2_READY_LO.read(dev, iomem)? != 1 {
+ if regs::L2ReadyLo::read(io).l2_ready_lo() != 1 {
dev_err!(dev, "Failed to power on the GPU\n");
return Err(EIO);
}
diff --git a/drivers/gpu/drm/tyr/regs.rs b/drivers/gpu/drm/tyr/regs.rs
index f46933aaa221..a4e05ff463c0 100644
--- a/drivers/gpu/drm/tyr/regs.rs
+++ b/drivers/gpu/drm/tyr/regs.rs
@@ -8,44 +8,62 @@
#![allow(dead_code)]
use kernel::bits::bit_u32;
-use kernel::device::Bound;
-use kernel::device::Device;
-use kernel::devres::Devres;
use kernel::prelude::*;
+use kernel::register;
-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(())
- }
-}
-
-pub(crate) const GPU_ID: Register<0x0> = Register;
-pub(crate) const GPU_L2_FEATURES: Register<0x4> = Register;
-pub(crate) const GPU_CORE_FEATURES: Register<0x8> = Register;
-pub(crate) const GPU_CSF_ID: Register<0x1c> = Register;
-pub(crate) const GPU_REVID: Register<0x280> = Register;
-pub(crate) const GPU_TILER_FEATURES: Register<0xc> = Register;
-pub(crate) const GPU_MEM_FEATURES: Register<0x10> = Register;
-pub(crate) const GPU_MMU_FEATURES: Register<0x14> = Register;
-pub(crate) const GPU_AS_PRESENT: Register<0x18> = Register;
-pub(crate) const GPU_IRQ_RAWSTAT: Register<0x20> = Register;
+register!(GpuId @ 0x0, "Information about the GPU architecture and release version" {
+ 3:0 version_status as u32, "Status of the GPU release";
+ 11:4 version_minor as u32, "Minor release version number";
+ 15:12 version_major as u32, "Major release version number";
+ 19:16 product_major as u32, "Product identifier";
+ 23:20 arch_rev as u32, "Architecture patch revision";
+ 27:24 arch_minor as u32, "Architecture minor revision";
+ 31:28 arch_major as u32, "Architecture major revision";
+});
+
+register!(L2Features @ 0x4, "Level 2 cache features" {
+ 7:0 line_size as u32, "L2 cache line size";
+ 15:8 associativity as u32, "L2 cache associativity";
+ 23:16 cache_size as u32, "L2 cache slice size";
+ 31:24 bus_width as u32, "L2 cache bus width";
+});
+
+register!(CoreFeatures @ 0x8, "Information about the features of a shader core" {
+ 7:0 core_variant as u32, "Shader core variant";
+});
+
+register!(CsfId @ 0x1c, "Version of the CSF hardware and MMU subsystem" {
+ 3:0 mcu_rev as u32, "MCU revision ID";
+ 9:4 mcu_minor as u32, "MCU minor revision number";
+ 15:10 mcu_major as u32, "MCU major revision number";
+ 19:16 cshw_rev as u32, "CSHW revision ID";
+ 25:20 cshw_minor as u32, "CSHW minor revision number";
+ 31:26 cshw_major as u32, "CSHW major revision number";
+});
+
+register!(RevIdr @ 0x280, "Extra revision information" {
+ 31:0 revision as u32, "Revision information";
+});
+
+register!(TilerFeatures @ 0xc, "Tiler features" {
+ 5:0 bin_size as u32, "Log of the tiler's bin size";
+ 11:8 max_levels as u32, "Maximum number of available levels";
+});
+
+register!(MemFeatures @ 0x10, "Memory features" {
+ 0:0 coherent_core_group as bool, "Core group is coherent";
+ 1:1 coherent_super_group as bool, "Core supergroup is coherent";
+ 11:8 l2_slices as u32, "L2 slice count";
+});
+
+register!(MmuFeatures @ 0x14, "MMU features" {
+ 7:0 va_bits as u32, "Number of bits supported in virtual addresses";
+ 15:8 pa_bits as u32, "Number of bits supported in physical addresses";
+});
+
+register!(AsPresent @ 0x18, "Address spaces present" {
+ 31:0 as_present as u32, "Bitmask of present address spaces";
+});
pub(crate) const GPU_IRQ_RAWSTAT_FAULT: u32 = bit_u32(0);
pub(crate) const GPU_IRQ_RAWSTAT_PROTECTED_FAULT: u32 = bit_u32(1);
@@ -56,53 +74,193 @@ pub(crate) fn write(&self, dev: &Device<Bound>, iomem: &Devres<IoMem>, value: u3
pub(crate) const GPU_IRQ_RAWSTAT_DOORBELL_STATUS: u32 = bit_u32(18);
pub(crate) const GPU_IRQ_RAWSTAT_MCU_STATUS: u32 = bit_u32(19);
-pub(crate) const GPU_IRQ_CLEAR: Register<0x24> = Register;
-pub(crate) const GPU_IRQ_MASK: Register<0x28> = Register;
-pub(crate) const GPU_IRQ_STAT: Register<0x2c> = Register;
-pub(crate) const GPU_CMD: Register<0x30> = Register;
+register!(GpuIrqRawstat @ 0x20, "Raw unmasked interrupt status for the GPU" {
+ 0:0 fault as bool, "A GPU fault has occourred";
+ 1:1 protected_fault as bool, "Indicates a protected memory fault has occurred";
+ 8:8 reset_completed as bool, "Indicates that a GPU reset has completed";
+ 9:9 power_changed_single as bool, "Set when a single power domain has powered up or down";
+ 10:10 power_changed_all as bool, "Set when all pending power domain changes are completed ";
+ 17:17 clean_caches_completed as bool, "Indicates that a cache clean operation has completed";
+ 18:18 doorbell_status as bool, "Mirrors the doorbell interrupt line to the CPU";
+ 19:19 mcu_status as bool, "The MCU requires attention";
+});
+
+register!(GpuIrqClear @ 0x24, "Clears pending GPU interrupts" {
+ 0:0 fault as bool, "Clear the fault interrupt";
+ 1:1 protected_fault as bool, "Clear the protected_fault interrupt";
+ 8:8 reset_completed as bool, "Clear the reset_completed interrupt";
+ 9:9 power_changed_single as bool, "Clear the power_changed_single interrupt";
+ 10:10 power_changed_all as bool, "Clear the power_changed_all interrupt";
+ 17:17 clean_caches_completed as bool, "Clear the clean_caches_completed interrupt";
+ 18:18 doorbell_status as bool, "Clear the doorbell_status interrupt";
+ 19:19 mcu_status as bool, "Clear the mcu_status interrupt";
+});
+
+register!(GpuIrqMask @ 0x28, "Enabled GPU interrupts" {
+ 0:0 fault as bool, "Enable the fault interrupt";
+ 1:1 protected_fault as bool, "Enable the protected_fault interrupt";
+ 8:8 reset_completed as bool, "Enable the reset_completed interrupt";
+ 9:9 power_changed_single as bool, "Enable the power_changed_single interrupt";
+ 10:10 power_changed_all as bool, "Enable the power_changed_all interrupt";
+ 17:17 clean_caches_completed as bool, "Enable the clean_caches_completed interrupt";
+ 18:18 doorbell_status as bool, "Enable the doorbell_status interrupt";
+ 19:19 mcu_status as bool, "Enable the mcu_status interrupt";
+});
+
+register!(GpuIrqStatus @ 0x2c, "Masked GPU interrupt status" {
+ 0:0 fault as bool, "The fault interrupt is pending";
+ 1:1 protected_fault as bool, "The protected_fault interrupt is pending";
+ 8:8 reset_completed as bool, "The reset_completed interrupt is pending";
+ 9:9 power_changed_single as bool, "The power_changed_single interrupt is pending";
+ 10:10 power_changed_all as bool, "The power_changed_all interrupt is pending";
+ 17:17 clean_caches_completed as bool, "The clean_caches_completed interrupt is pending";
+ 18:18 doorbell_status as bool, "The doorbell_status interrupt is pending";
+ 19:19 mcu_status as bool, "The mcu_status interrupt is pending";
+});
+
pub(crate) const GPU_CMD_SOFT_RESET: u32 = 1 | (1 << 8);
pub(crate) const GPU_CMD_HARD_RESET: u32 = 1 | (2 << 8);
-pub(crate) const GPU_THREAD_FEATURES: Register<0xac> = Register;
-pub(crate) const GPU_THREAD_MAX_THREADS: Register<0xa0> = Register;
-pub(crate) const GPU_THREAD_MAX_WORKGROUP_SIZE: Register<0xa4> = Register;
-pub(crate) const GPU_THREAD_MAX_BARRIER_SIZE: Register<0xa8> = Register;
-pub(crate) const GPU_TEXTURE_FEATURES0: Register<0xb0> = Register;
-pub(crate) const GPU_SHADER_PRESENT_LO: Register<0x100> = Register;
-pub(crate) const GPU_SHADER_PRESENT_HI: Register<0x104> = Register;
-pub(crate) const GPU_TILER_PRESENT_LO: Register<0x110> = Register;
-pub(crate) const GPU_TILER_PRESENT_HI: Register<0x114> = Register;
-pub(crate) const GPU_L2_PRESENT_LO: Register<0x120> = Register;
-pub(crate) const GPU_L2_PRESENT_HI: Register<0x124> = Register;
-pub(crate) const L2_READY_LO: Register<0x160> = Register;
-pub(crate) const L2_READY_HI: Register<0x164> = Register;
-pub(crate) const L2_PWRON_LO: Register<0x1a0> = Register;
-pub(crate) const L2_PWRON_HI: Register<0x1a4> = Register;
-pub(crate) const L2_PWRTRANS_LO: Register<0x220> = Register;
-pub(crate) const L2_PWRTRANS_HI: Register<0x204> = Register;
-pub(crate) const L2_PWRACTIVE_LO: Register<0x260> = Register;
-pub(crate) const L2_PWRACTIVE_HI: Register<0x264> = Register;
-
-pub(crate) const MCU_CONTROL: Register<0x700> = Register;
+
+register!(GpuCommand @ 0x30, "GPU command register" {
+ 7:0 command as u32, "GPU-specific command to execute";
+ 31:8 payload as u32, "Payload for the command";
+});
+
+register!(ThreadFeatures @ 0xac, "Thread features of the GPU's threading system" {
+ 21:0 max_registers as u32, "Total number of registers per core";
+ 23:22 implementation_technology as u32;
+ 31:24 max_task_queue as u32, "Maximum number of compute tasks waiting";
+
+});
+
+register!(ThreadMaxThreads @ 0xa0, "Maximum number of threads per core" {
+ 31:0 max_threads as u32, "Maximum number of threads per core";
+});
+
+register!(ThreadMaxWorkgroupSize @ 0xa4, "Maximum number of threads per workgroup" {
+ 31:0 max_workgroup_size as u32, "Maximum number of threads per workgroup";
+});
+
+register!(ThreadMaxBarrierSize @ 0xa8, "Maximum number of threads per barrier" {
+ 31:0 max_barrier_size as u32, "Maximum number of threads per barrier";
+});
+
+register!(TextureFeatures @ 0xb0 [4], "Bitmap of supported texture formats" {});
+
+register!(ShaderPresentLo @ 0x100, "Bitmap of shader cores present in the hardware (lower 32 bits)" {
+ 31:0 shader_present_lo as u32, "Bitmap of shader cores present in the hardware (lower 32 bits)";
+});
+
+register!(ShaderPresentHi @ 0x104, "Bitmap of shader cores present in the hardware (higher 32 bits)" {
+ 31:0 shader_present_hi as u32, "Bitmap of shader cores present in the hardware (higher 32 bits)";
+});
+
+register!(TilerPresentLo @ 0x110, "Bitmap of tiler cores present in the hardware (lower 32 bits)" {
+ 31:0 tiler_present_lo as u32, "Bitmap of tiler cores present in the hardware (lower 32 bits)";
+});
+
+register!(TilerPresentHi @ 0x114, "Bitmap of tiler cores present in the hardware (higher 32 bits)" {
+ 31:0 tiler_present_hi as u32, "Bitmap of tiler cores present in the hardware (higher 32 bits)";
+});
+
+register!(L2PresentLo @ 0x120, "Bitmap of L2 caches present in the hardware (lower 32 bits)" {
+ 31:0 l2_present_lo as u32, "Bitmap of L2 caches present in the hardware (lower 32 bits)";
+});
+
+register!(L2PresentHi @ 0x124, "Bitmap of L2 caches present in the hardware (higher 32 bits)" {
+ 31:0 l2_present_hi as u32, "Bitmap of L2 caches present in the hardware (higher 32 bits)";
+});
+
+register!(L2ReadyLo @ 0x160, "Bitmap of L2 caches ready (lower 32 bits)" {
+ 31:0 l2_ready_lo as u32, "Bitmap of L2 caches ready (lower 32 bits)";
+});
+
+register!(L2ReadyHi @ 0x164, "Bitmap of L2 caches ready (higher 32 bits)" {
+ 31:0 l2_ready_hi as u32, "Bitmap of L2 caches ready (higher 32 bits)";
+});
+
+register!(L2PwrOnLo @ 0x1a0, "Bitmap of L2 caches power on requests (lower 32 bits)" {
+ 31:0 l2_pwron_lo as u32, "Bitmap of L2 caches power on requests (lower 32 bits)";
+});
+
+register!(L2PwrOnHi @ 0x1a4, "Bitmap of L2 caches power on requests (higher 32 bits)" {
+ 31:0 l2_pwron_hi as u32, "Bitmap of L2 caches power on requests (higher 32 bits)";
+});
+
+register!(L2PwrTransLo @ 0x200, "Bitmap of L2 caches in power transition (lower 32 bits)" {
+ 31:0 l2_pwrtrans_lo as u32, "Bitmap of L2 caches in power transition (lower 32 bits)";
+});
+
+register!(L2PwrTransHi @ 0x204, "Bitmap of L2 caches in power transition (higher 32 bits)" {
+ 31:0 l2_pwrtrans_hi as u32, "Bitmap of L2 caches in power transition (higher 32 bits)";
+});
+
+register!(L2PowerActiveLo @ 0x260, "Bitmap of L2 caches active (lower 32 bits)" {
+ 31:0 l2_pwractive_lo as u32, "Bitmap of L2 caches active (lower 32 bits)";
+});
+
+register!(L2PowerActiveHi @ 0x264, "Bitmap of L2 caches active (higher 32 bits)" {
+ 31:0 l2_pwractive_hi as u32, "Bitmap of L2 caches active (higher 32 bits)";
+});
+
pub(crate) const MCU_CONTROL_ENABLE: u32 = 1;
pub(crate) const MCU_CONTROL_AUTO: u32 = 2;
pub(crate) const MCU_CONTROL_DISABLE: u32 = 0;
-pub(crate) const MCU_STATUS: Register<0x704> = Register;
+register!(McuControl @ 0x700, "Controls the execution state of the MCU subsystem" {
+ 1:0 req as u32, "Request state change";
+});
+
pub(crate) const MCU_STATUS_DISABLED: u32 = 0;
pub(crate) const MCU_STATUS_ENABLED: u32 = 1;
pub(crate) const MCU_STATUS_HALT: u32 = 2;
pub(crate) const MCU_STATUS_FATAL: u32 = 3;
-pub(crate) const GPU_COHERENCY_FEATURES: Register<0x300> = Register;
+register!(McuStatus @ 0x704, "Reports the current execution state of the MCU subsystem" {
+ 1:0 status as u32, "Current MCU status";
+});
+
+register!(CoherencyFeatures @ 0x300, "GPU coherency features" {
+ 0:0 ace_lite as bool, "ACE-Lite protocol supported";
+ 1:1 ace as bool, "ACE protocol supported";
+});
+
+register!(JobIrqRawstat @ 0x1000, "Raw unmasked interrupt status for firmware interrupts" {
+ 30:0 csg as u32, "CSG request";
+ 31:31 glb as bool, "GLB request";
+});
+
+register!(JobIrqClear @ 0x1004, "Clears pending firmware interrupts" {
+ 30:0 csg as u32, "Clear CSG requests";
+ 31:31 glb as bool, "Clear GLB request";
+});
+
+register!(JobIrqMask @ 0x1008, "Enabled firmware interrupts" {
+ 30:0 csg as u32, "Enable CSG requests";
+ 31:31 glb as bool, "Enable GLB request";
+});
+
+register!(JobIrqStatus @ 0x100c, "Masked firmware interrupt status" {
+ 30:0 csg as u32, "Pending CSG requests";
+ 31:31 glb as bool, "Pending GLB request";
+});
+
+register!(MmuIrqRawstat @ 0x2000, "Raw unmasked interrupt status for MMU interrupts" {
+ 15:0 page_fault as u32, "Bitmask indicating which address spaces page-faulted";
+ 31:31 command_completed as bool, "Bitmask indicating whether a command completed in a given AS";
+});
-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;
+register!(MmuIrqClear @ 0x2004, "Clears pending MMU interrupts" {
+ 15:0 page_fault as u32, "Clear page-fault interrupts for the given address spaces";
+ 31:31 command_completed as bool, "Clear command-completed interrupt for the given address spaces";
+});
-pub(crate) const JOB_IRQ_GLOBAL_IF: u32 = bit_u32(31);
+register!(MmuIrqMask @ 0x2008, "Enabled MMU interrupts" {
+ 15:0 page_fault as u32, "Enable page-fault interrupts for the given address spaces";
+ 31:31 command_completed as bool, "Enable command-completed interrupt for the given address spaces";
+});
-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;
+register!(MmuIrqStatus @ 0x200c, "Masked MMU interrupt status" {
+ 15:0 page_fault as u32, "Pending page-fault interrupts for the given address spaces";
+ 31:31 command_completed as bool, "Pending command-completed interrupt for the given address spaces";
+});
---
base-commit: f10c325a345fef0a688a2bcdfab1540d1c924148
change-id: 20260108-tyr-register-ea913f8e2330
prerequisite-message-id: <20251003154748.1687160-1-joelagnelf@nvidia.com>
prerequisite-patch-id: 027ea340650912c31c3b3e2e2ba60f390b449218
Best regards,
--
Daniel Almeida <daniel.almeida@collabora.com>Hi Daniel,
On 14.01.26 23:53, Daniel Almeida wrote:
> Replace regs::Register with kernel::register. This allow us to more
> succinctly express the register set by introducing the ability to describe
> fields and their documentation and to auto-generate the accessors. In
> particular, this is very helpful as it does away with a lot of manual masks
> and shifts.
As mentioned somewhere else already I really like switching to
register!(). Thanks!
Some coments below:
> A future commit will eliminate HI/LO pairs once there is support for 64bit
> reads and writes in kernel::register.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
> Note that this patch depends on a rebased version of Joel's patch at [0].
>
> That version is stale, so I ended up rebasing it locally myself for the
> purpose of developing this patch and gathering some reviews on the list. In
> other words, the current patch does not apply for the time being, but will
> once a v7 for Joel's series is out.
>
> [0]: https://lore.kernel.org/rust-for-linux/20251003154748.1687160-1-joelagnelf@nvidia.com/
> ---
> drivers/gpu/drm/tyr/driver.rs | 15 ++-
> drivers/gpu/drm/tyr/gpu.rs | 55 ++++----
> drivers/gpu/drm/tyr/regs.rs | 302 ++++++++++++++++++++++++++++++++----------
> 3 files changed, 267 insertions(+), 105 deletions(-)
>
> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> index 0389c558c036..8e06db5320bf 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs
> @@ -66,19 +66,20 @@ unsafe impl Send for TyrData {}
> unsafe impl Sync for TyrData {}
>
> 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)?;
> +
> + regs::GpuCommand::default()
> + .set_command(regs::GPU_CMD_SOFT_RESET)
> + .write(io);
>
> // TODO: We cannot poll, as there is no support in Rust currently, so we
> // sleep. Change this when read_poll_timeout() is implemented in Rust.
> kernel::time::delay::fsleep(time::Delta::from_millis(100));
>
> - if regs::GPU_IRQ_RAWSTAT.read(dev, iomem)? & regs::GPU_IRQ_RAWSTAT_RESET_COMPLETED == 0 {
> + let rawstat = regs::GpuIrqRawstat::read(io);
> + if !rawstat.reset_completed() {
> dev_err!(dev, "GPU reset failed with errno\n");
> - dev_err!(
> - dev,
> - "GPU_INT_RAWSTAT is {}\n",
> - regs::GPU_IRQ_RAWSTAT.read(dev, iomem)?
> - );
> + dev_err!(dev, "GPU_INT_RAWSTAT is {}\n", u32::from(rawstat));
This is pre-existing, but printing `... INT ...` for `...IRQ...`
register looks confusing (wrong?).
> return Err(EIO);
> }
> diff --git a/drivers/gpu/drm/tyr/gpu.rs b/drivers/gpu/drm/tyr/gpu.rs
> index 6c582910dd5d..7c698fb1e36a 100644
> --- a/drivers/gpu/drm/tyr/gpu.rs
> +++ b/drivers/gpu/drm/tyr/gpu.rs
> @@ -44,34 +44,36 @@ pub(crate) struct GpuInfo {
>
> impl GpuInfo {
> pub(crate) fn new(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result<Self> {
> - let gpu_id = regs::GPU_ID.read(dev, iomem)?;
> - let csf_id = regs::GPU_CSF_ID.read(dev, iomem)?;
> - let gpu_rev = regs::GPU_REVID.read(dev, iomem)?;
> - let core_features = regs::GPU_CORE_FEATURES.read(dev, iomem)?;
> - let l2_features = regs::GPU_L2_FEATURES.read(dev, iomem)?;
> - let tiler_features = regs::GPU_TILER_FEATURES.read(dev, iomem)?;
> - let mem_features = regs::GPU_MEM_FEATURES.read(dev, iomem)?;
> - let mmu_features = regs::GPU_MMU_FEATURES.read(dev, iomem)?;
> - let thread_features = regs::GPU_THREAD_FEATURES.read(dev, iomem)?;
> - let max_threads = regs::GPU_THREAD_MAX_THREADS.read(dev, iomem)?;
> - let thread_max_workgroup_size = regs::GPU_THREAD_MAX_WORKGROUP_SIZE.read(dev, iomem)?;
> - let thread_max_barrier_size = regs::GPU_THREAD_MAX_BARRIER_SIZE.read(dev, iomem)?;
> - let coherency_features = regs::GPU_COHERENCY_FEATURES.read(dev, iomem)?;
> -
> - let texture_features = regs::GPU_TEXTURE_FEATURES0.read(dev, iomem)?;
> -
> - let as_present = regs::GPU_AS_PRESENT.read(dev, iomem)?;
> -
> - let shader_present = u64::from(regs::GPU_SHADER_PRESENT_LO.read(dev, iomem)?);
> + let io = (*iomem).access(dev)?;
> +
> + let gpu_id = regs::GpuId::read(io).into();
> + let csf_id = regs::CsfId::read(io).into();
> + let gpu_rev = regs::RevIdr::read(io).into();
> + let core_features = regs::CoreFeatures::read(io).into();
> + let l2_features = regs::L2Features::read(io).into();
> + let tiler_features = regs::TilerFeatures::read(io).into();
> + let mem_features = regs::MemFeatures::read(io).into();
> + let mmu_features = regs::MmuFeatures::read(io).into();
> + let thread_features = regs::ThreadFeatures::read(io).into();
> + let max_threads = regs::ThreadMaxThreads::read(io).into();
> + let thread_max_workgroup_size = regs::ThreadMaxWorkgroupSize::read(io).into();
> + let thread_max_barrier_size = regs::ThreadMaxBarrierSize::read(io).into();
> + let coherency_features = regs::CoherencyFeatures::read(io).into();
Is there any reason why you replace the UPPERCASE register names with
CamelCase ones?
I was under the impression that we want to use UPPERCASE for register
names. Like in nova
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/nova-core/regs.rs
> + let texture_features = regs::TextureFeatures::read(io, 0).into();
> +
> + let as_present = regs::AsPresent::read(io).into();
> +
> + let shader_present = u64::from(u32::from(regs::ShaderPresentLo::read(io)));
> let shader_present =
> - shader_present | u64::from(regs::GPU_SHADER_PRESENT_HI.read(dev, iomem)?) << 32;
> + shader_present | u64::from(u32::from(regs::ShaderPresentHi::read(io))) << 32;
>
> - let tiler_present = u64::from(regs::GPU_TILER_PRESENT_LO.read(dev, iomem)?);
> + let tiler_present = u64::from(u32::from(regs::TilerPresentLo::read(io)));
> let tiler_present =
> - tiler_present | u64::from(regs::GPU_TILER_PRESENT_HI.read(dev, iomem)?) << 32;
> + tiler_present | u64::from(u32::from(regs::TilerPresentHi::read(io))) << 32;
>
> - let l2_present = u64::from(regs::GPU_L2_PRESENT_LO.read(dev, iomem)?);
> - let l2_present = l2_present | u64::from(regs::GPU_L2_PRESENT_HI.read(dev, iomem)?) << 32;
> + let l2_present = u64::from(u32::from(regs::L2PresentLo::read(io)));
> + let l2_present = l2_present | u64::from(u32::from(regs::L2PresentHi::read(io))) << 32;
>
> Ok(Self {
> gpu_id,
> @@ -204,13 +206,14 @@ fn from(value: u32) -> Self {
>
> /// 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)?;
> + regs::L2PwrOnLo::default().set_l2_pwron_lo(1).write(io);
>
> // TODO: We cannot poll, as there is no support in Rust currently, so we
> // sleep. Change this when read_poll_timeout() is implemented in Rust.
> kernel::time::delay::fsleep(time::Delta::from_millis(100));
>
> - if regs::L2_READY_LO.read(dev, iomem)? != 1 {
> + if regs::L2ReadyLo::read(io).l2_ready_lo() != 1 {
> dev_err!(dev, "Failed to power on the GPU\n");
> return Err(EIO);
> }
> diff --git a/drivers/gpu/drm/tyr/regs.rs b/drivers/gpu/drm/tyr/regs.rs
> index f46933aaa221..a4e05ff463c0 100644
> --- a/drivers/gpu/drm/tyr/regs.rs
> +++ b/drivers/gpu/drm/tyr/regs.rs
> @@ -8,44 +8,62 @@
> #![allow(dead_code)]
>
> use kernel::bits::bit_u32;
> -use kernel::device::Bound;
> -use kernel::device::Device;
> -use kernel::devres::Devres;
> use kernel::prelude::*;
> +use kernel::register;
>
> -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(())
> - }
> -}
> -
> -pub(crate) const GPU_ID: Register<0x0> = Register;
> -pub(crate) const GPU_L2_FEATURES: Register<0x4> = Register;
> -pub(crate) const GPU_CORE_FEATURES: Register<0x8> = Register;
> -pub(crate) const GPU_CSF_ID: Register<0x1c> = Register;
> -pub(crate) const GPU_REVID: Register<0x280> = Register;
> -pub(crate) const GPU_TILER_FEATURES: Register<0xc> = Register;
> -pub(crate) const GPU_MEM_FEATURES: Register<0x10> = Register;
> -pub(crate) const GPU_MMU_FEATURES: Register<0x14> = Register;
> -pub(crate) const GPU_AS_PRESENT: Register<0x18> = Register;
> -pub(crate) const GPU_IRQ_RAWSTAT: Register<0x20> = Register;
> +register!(GpuId @ 0x0, "Information about the GPU architecture and release version" {
> + 3:0 version_status as u32, "Status of the GPU release";
> + 11:4 version_minor as u32, "Minor release version number";
> + 15:12 version_major as u32, "Major release version number";
> + 19:16 product_major as u32, "Product identifier";
> + 23:20 arch_rev as u32, "Architecture patch revision";
> + 27:24 arch_minor as u32, "Architecture minor revision";
> + 31:28 arch_major as u32, "Architecture major revision";
> +});
> +
> +register!(L2Features @ 0x4, "Level 2 cache features" {
> + 7:0 line_size as u32, "L2 cache line size";
> + 15:8 associativity as u32, "L2 cache associativity";
> + 23:16 cache_size as u32, "L2 cache slice size";
> + 31:24 bus_width as u32, "L2 cache bus width";
> +});
> +
> +register!(CoreFeatures @ 0x8, "Information about the features of a shader core" {
> + 7:0 core_variant as u32, "Shader core variant";
> +});
> +
> +register!(CsfId @ 0x1c, "Version of the CSF hardware and MMU subsystem" {
> + 3:0 mcu_rev as u32, "MCU revision ID";
> + 9:4 mcu_minor as u32, "MCU minor revision number";
> + 15:10 mcu_major as u32, "MCU major revision number";
> + 19:16 cshw_rev as u32, "CSHW revision ID";
> + 25:20 cshw_minor as u32, "CSHW minor revision number";
> + 31:26 cshw_major as u32, "CSHW major revision number";
> +});
> +
> +register!(RevIdr @ 0x280, "Extra revision information" {
> + 31:0 revision as u32, "Revision information";
> +});
> +
> +register!(TilerFeatures @ 0xc, "Tiler features" {
> + 5:0 bin_size as u32, "Log of the tiler's bin size";
> + 11:8 max_levels as u32, "Maximum number of available levels";
> +});
> +
> +register!(MemFeatures @ 0x10, "Memory features" {
> + 0:0 coherent_core_group as bool, "Core group is coherent";
> + 1:1 coherent_super_group as bool, "Core supergroup is coherent";
> + 11:8 l2_slices as u32, "L2 slice count";
> +});
> +
> +register!(MmuFeatures @ 0x14, "MMU features" {
> + 7:0 va_bits as u32, "Number of bits supported in virtual addresses";
> + 15:8 pa_bits as u32, "Number of bits supported in physical addresses";
> +});
> +
> +register!(AsPresent @ 0x18, "Address spaces present" {
> + 31:0 as_present as u32, "Bitmask of present address spaces";
> +});
>
> pub(crate) const GPU_IRQ_RAWSTAT_FAULT: u32 = bit_u32(0);
> pub(crate) const GPU_IRQ_RAWSTAT_PROTECTED_FAULT: u32 = bit_u32(1);
> @@ -56,53 +74,193 @@ pub(crate) fn write(&self, dev: &Device<Bound>, iomem: &Devres<IoMem>, value: u3
> pub(crate) const GPU_IRQ_RAWSTAT_DOORBELL_STATUS: u32 = bit_u32(18);
> pub(crate) const GPU_IRQ_RAWSTAT_MCU_STATUS: u32 = bit_u32(19);
>
> -pub(crate) const GPU_IRQ_CLEAR: Register<0x24> = Register;
> -pub(crate) const GPU_IRQ_MASK: Register<0x28> = Register;
> -pub(crate) const GPU_IRQ_STAT: Register<0x2c> = Register;
> -pub(crate) const GPU_CMD: Register<0x30> = Register;
> +register!(GpuIrqRawstat @ 0x20, "Raw unmasked interrupt status for the GPU" {
> + 0:0 fault as bool, "A GPU fault has occourred";
> + 1:1 protected_fault as bool, "Indicates a protected memory fault has occurred";
> + 8:8 reset_completed as bool, "Indicates that a GPU reset has completed";
> + 9:9 power_changed_single as bool, "Set when a single power domain has powered up or down";
> + 10:10 power_changed_all as bool, "Set when all pending power domain changes are completed ";
> + 17:17 clean_caches_completed as bool, "Indicates that a cache clean operation has completed";
> + 18:18 doorbell_status as bool, "Mirrors the doorbell interrupt line to the CPU";
> + 19:19 mcu_status as bool, "The MCU requires attention";
> +});
> +
> +register!(GpuIrqClear @ 0x24, "Clears pending GPU interrupts" {
> + 0:0 fault as bool, "Clear the fault interrupt";
> + 1:1 protected_fault as bool, "Clear the protected_fault interrupt";
> + 8:8 reset_completed as bool, "Clear the reset_completed interrupt";
> + 9:9 power_changed_single as bool, "Clear the power_changed_single interrupt";
> + 10:10 power_changed_all as bool, "Clear the power_changed_all interrupt";
> + 17:17 clean_caches_completed as bool, "Clear the clean_caches_completed interrupt";
> + 18:18 doorbell_status as bool, "Clear the doorbell_status interrupt";
> + 19:19 mcu_status as bool, "Clear the mcu_status interrupt";
> +});
> +
> +register!(GpuIrqMask @ 0x28, "Enabled GPU interrupts" {
> + 0:0 fault as bool, "Enable the fault interrupt";
> + 1:1 protected_fault as bool, "Enable the protected_fault interrupt";
> + 8:8 reset_completed as bool, "Enable the reset_completed interrupt";
> + 9:9 power_changed_single as bool, "Enable the power_changed_single interrupt";
> + 10:10 power_changed_all as bool, "Enable the power_changed_all interrupt";
> + 17:17 clean_caches_completed as bool, "Enable the clean_caches_completed interrupt";
> + 18:18 doorbell_status as bool, "Enable the doorbell_status interrupt";
> + 19:19 mcu_status as bool, "Enable the mcu_status interrupt";
> +});
> +
> +register!(GpuIrqStatus @ 0x2c, "Masked GPU interrupt status" {
> + 0:0 fault as bool, "The fault interrupt is pending";
> + 1:1 protected_fault as bool, "The protected_fault interrupt is pending";
> + 8:8 reset_completed as bool, "The reset_completed interrupt is pending";
> + 9:9 power_changed_single as bool, "The power_changed_single interrupt is pending";
> + 10:10 power_changed_all as bool, "The power_changed_all interrupt is pending";
> + 17:17 clean_caches_completed as bool, "The clean_caches_completed interrupt is pending";
> + 18:18 doorbell_status as bool, "The doorbell_status interrupt is pending";
> + 19:19 mcu_status as bool, "The mcu_status interrupt is pending";
> +});
> +
> pub(crate) const GPU_CMD_SOFT_RESET: u32 = 1 | (1 << 8);
> pub(crate) const GPU_CMD_HARD_RESET: u32 = 1 | (2 << 8);
> -pub(crate) const GPU_THREAD_FEATURES: Register<0xac> = Register;
> -pub(crate) const GPU_THREAD_MAX_THREADS: Register<0xa0> = Register;
> -pub(crate) const GPU_THREAD_MAX_WORKGROUP_SIZE: Register<0xa4> = Register;
> -pub(crate) const GPU_THREAD_MAX_BARRIER_SIZE: Register<0xa8> = Register;
> -pub(crate) const GPU_TEXTURE_FEATURES0: Register<0xb0> = Register;
> -pub(crate) const GPU_SHADER_PRESENT_LO: Register<0x100> = Register;
> -pub(crate) const GPU_SHADER_PRESENT_HI: Register<0x104> = Register;
> -pub(crate) const GPU_TILER_PRESENT_LO: Register<0x110> = Register;
> -pub(crate) const GPU_TILER_PRESENT_HI: Register<0x114> = Register;
> -pub(crate) const GPU_L2_PRESENT_LO: Register<0x120> = Register;
> -pub(crate) const GPU_L2_PRESENT_HI: Register<0x124> = Register;
> -pub(crate) const L2_READY_LO: Register<0x160> = Register;
> -pub(crate) const L2_READY_HI: Register<0x164> = Register;
> -pub(crate) const L2_PWRON_LO: Register<0x1a0> = Register;
> -pub(crate) const L2_PWRON_HI: Register<0x1a4> = Register;
> -pub(crate) const L2_PWRTRANS_LO: Register<0x220> = Register;
> -pub(crate) const L2_PWRTRANS_HI: Register<0x204> = Register;
> -pub(crate) const L2_PWRACTIVE_LO: Register<0x260> = Register;
> -pub(crate) const L2_PWRACTIVE_HI: Register<0x264> = Register;
> -
> -pub(crate) const MCU_CONTROL: Register<0x700> = Register;
> +
> +register!(GpuCommand @ 0x30, "GPU command register" {
> + 7:0 command as u32, "GPU-specific command to execute";
> + 31:8 payload as u32, "Payload for the command";
> +});
> +
> +register!(ThreadFeatures @ 0xac, "Thread features of the GPU's threading system" {
> + 21:0 max_registers as u32, "Total number of registers per core";
> + 23:22 implementation_technology as u32;
> + 31:24 max_task_queue as u32, "Maximum number of compute tasks waiting";
> +
> +});
> +
> +register!(ThreadMaxThreads @ 0xa0, "Maximum number of threads per core" {
> + 31:0 max_threads as u32, "Maximum number of threads per core";
> +});
> +
> +register!(ThreadMaxWorkgroupSize @ 0xa4, "Maximum number of threads per workgroup" {
> + 31:0 max_workgroup_size as u32, "Maximum number of threads per workgroup";
> +});
> +
> +register!(ThreadMaxBarrierSize @ 0xa8, "Maximum number of threads per barrier" {
> + 31:0 max_barrier_size as u32, "Maximum number of threads per barrier";
> +});
> +
> +register!(TextureFeatures @ 0xb0 [4], "Bitmap of supported texture formats" {});
> +
> +register!(ShaderPresentLo @ 0x100, "Bitmap of shader cores present in the hardware (lower 32 bits)" {
> + 31:0 shader_present_lo as u32, "Bitmap of shader cores present in the hardware (lower 32 bits)";
> +});
> +
> +register!(ShaderPresentHi @ 0x104, "Bitmap of shader cores present in the hardware (higher 32 bits)" {
> + 31:0 shader_present_hi as u32, "Bitmap of shader cores present in the hardware (higher 32 bits)";
> +});
> +
> +register!(TilerPresentLo @ 0x110, "Bitmap of tiler cores present in the hardware (lower 32 bits)" {
> + 31:0 tiler_present_lo as u32, "Bitmap of tiler cores present in the hardware (lower 32 bits)";
> +});
> +
> +register!(TilerPresentHi @ 0x114, "Bitmap of tiler cores present in the hardware (higher 32 bits)" {
> + 31:0 tiler_present_hi as u32, "Bitmap of tiler cores present in the hardware (higher 32 bits)";
> +});
> +
> +register!(L2PresentLo @ 0x120, "Bitmap of L2 caches present in the hardware (lower 32 bits)" {
> + 31:0 l2_present_lo as u32, "Bitmap of L2 caches present in the hardware (lower 32 bits)";
> +});
> +
> +register!(L2PresentHi @ 0x124, "Bitmap of L2 caches present in the hardware (higher 32 bits)" {
> + 31:0 l2_present_hi as u32, "Bitmap of L2 caches present in the hardware (higher 32 bits)";
> +});
> +
> +register!(L2ReadyLo @ 0x160, "Bitmap of L2 caches ready (lower 32 bits)" {
> + 31:0 l2_ready_lo as u32, "Bitmap of L2 caches ready (lower 32 bits)";
> +});
> +
> +register!(L2ReadyHi @ 0x164, "Bitmap of L2 caches ready (higher 32 bits)" {
> + 31:0 l2_ready_hi as u32, "Bitmap of L2 caches ready (higher 32 bits)";
> +});
> +
> +register!(L2PwrOnLo @ 0x1a0, "Bitmap of L2 caches power on requests (lower 32 bits)" {
> + 31:0 l2_pwron_lo as u32, "Bitmap of L2 caches power on requests (lower 32 bits)";
> +});
> +
> +register!(L2PwrOnHi @ 0x1a4, "Bitmap of L2 caches power on requests (higher 32 bits)" {
> + 31:0 l2_pwron_hi as u32, "Bitmap of L2 caches power on requests (higher 32 bits)";
> +});
> +
> +register!(L2PwrTransLo @ 0x200, "Bitmap of L2 caches in power transition (lower 32 bits)" {
> + 31:0 l2_pwrtrans_lo as u32, "Bitmap of L2 caches in power transition (lower 32 bits)";
> +});
> +
> +register!(L2PwrTransHi @ 0x204, "Bitmap of L2 caches in power transition (higher 32 bits)" {
> + 31:0 l2_pwrtrans_hi as u32, "Bitmap of L2 caches in power transition (higher 32 bits)";
> +});
> +
> +register!(L2PowerActiveLo @ 0x260, "Bitmap of L2 caches active (lower 32 bits)" {
> + 31:0 l2_pwractive_lo as u32, "Bitmap of L2 caches active (lower 32 bits)";
> +});
> +
> +register!(L2PowerActiveHi @ 0x264, "Bitmap of L2 caches active (higher 32 bits)" {
> + 31:0 l2_pwractive_hi as u32, "Bitmap of L2 caches active (higher 32 bits)";
> +});
> +
> pub(crate) const MCU_CONTROL_ENABLE: u32 = 1;
> pub(crate) const MCU_CONTROL_AUTO: u32 = 2;
> pub(crate) const MCU_CONTROL_DISABLE: u32 = 0;
>
> -pub(crate) const MCU_STATUS: Register<0x704> = Register;
> +register!(McuControl @ 0x700, "Controls the execution state of the MCU subsystem" {
> + 1:0 req as u32, "Request state change";
> +});
Any reason why req is a u32 and not a u8? Same for some other places.
And would it be an option to move the const MCU_CONTROL* to an ìmpl
McuControl Same for STATUS below.
> +
> pub(crate) const MCU_STATUS_DISABLED: u32 = 0;
> pub(crate) const MCU_STATUS_ENABLED: u32 = 1;
> pub(crate) const MCU_STATUS_HALT: u32 = 2;
> pub(crate) const MCU_STATUS_FATAL: u32 = 3;
>
> -pub(crate) const GPU_COHERENCY_FEATURES: Register<0x300> = Register;
> +register!(McuStatus @ 0x704, "Reports the current execution state of the MCU subsystem" {
> + 1:0 status as u32, "Current MCU status";
> +});
> +
> +register!(CoherencyFeatures @ 0x300, "GPU coherency features" {
> + 0:0 ace_lite as bool, "ACE-Lite protocol supported";
> + 1:1 ace as bool, "ACE protocol supported";
> +});
> +
> +register!(JobIrqRawstat @ 0x1000, "Raw unmasked interrupt status for firmware interrupts" {
> + 30:0 csg as u32, "CSG request";
> + 31:31 glb as bool, "GLB request";
> +});
> +
> +register!(JobIrqClear @ 0x1004, "Clears pending firmware interrupts" {
> + 30:0 csg as u32, "Clear CSG requests";
> + 31:31 glb as bool, "Clear GLB request";
> +});
> +
> +register!(JobIrqMask @ 0x1008, "Enabled firmware interrupts" {
> + 30:0 csg as u32, "Enable CSG requests";
> + 31:31 glb as bool, "Enable GLB request";
> +});
> +
> +register!(JobIrqStatus @ 0x100c, "Masked firmware interrupt status" {
> + 30:0 csg as u32, "Pending CSG requests";
> + 31:31 glb as bool, "Pending GLB request";
> +});
> +
> +register!(MmuIrqRawstat @ 0x2000, "Raw unmasked interrupt status for MMU interrupts" {
> + 15:0 page_fault as u32, "Bitmask indicating which address spaces page-faulted";
> + 31:31 command_completed as bool, "Bitmask indicating whether a command completed in a given AS";
> +});
>
> -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;
> +register!(MmuIrqClear @ 0x2004, "Clears pending MMU interrupts" {
> + 15:0 page_fault as u32, "Clear page-fault interrupts for the given address spaces";
> + 31:31 command_completed as bool, "Clear command-completed interrupt for the given address spaces";
> +});
>
> -pub(crate) const JOB_IRQ_GLOBAL_IF: u32 = bit_u32(31);
> +register!(MmuIrqMask @ 0x2008, "Enabled MMU interrupts" {
> + 15:0 page_fault as u32, "Enable page-fault interrupts for the given address spaces";
> + 31:31 command_completed as bool, "Enable command-completed interrupt for the given address spaces";
> +});
>
> -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;
> +register!(MmuIrqStatus @ 0x200c, "Masked MMU interrupt status" {
> + 15:0 page_fault as u32, "Pending page-fault interrupts for the given address spaces";
> + 31:31 command_completed as bool, "Pending command-completed interrupt for the given address spaces";
> +});
Thanks again
Dirk
Hi Dirk, thanks for the review!
> On 15 Jan 2026, at 14:05, Dirk Behme <dirk.behme@gmail.com> wrote:
>
> Hi Daniel,
>
> On 14.01.26 23:53, Daniel Almeida wrote:
>> Replace regs::Register with kernel::register. This allow us to more
>> succinctly express the register set by introducing the ability to describe
>> fields and their documentation and to auto-generate the accessors. In
>> particular, this is very helpful as it does away with a lot of manual masks
>> and shifts.
>
>
> As mentioned somewhere else already I really like switching to
> register!(). Thanks!
>
> Some coments below:
>
>
>> A future commit will eliminate HI/LO pairs once there is support for 64bit
>> reads and writes in kernel::register.
>>
>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>> ---
>> Note that this patch depends on a rebased version of Joel's patch at [0].
>>
>> That version is stale, so I ended up rebasing it locally myself for the
>> purpose of developing this patch and gathering some reviews on the list. In
>> other words, the current patch does not apply for the time being, but will
>> once a v7 for Joel's series is out.
>>
>> [0]: https://lore.kernel.org/rust-for-linux/20251003154748.1687160-1-joelagnelf@nvidia.com/
>> ---
>> drivers/gpu/drm/tyr/driver.rs | 15 ++-
>> drivers/gpu/drm/tyr/gpu.rs | 55 ++++----
>> drivers/gpu/drm/tyr/regs.rs | 302 ++++++++++++++++++++++++++++++++----------
>> 3 files changed, 267 insertions(+), 105 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
>> index 0389c558c036..8e06db5320bf 100644
>> --- a/drivers/gpu/drm/tyr/driver.rs
>> +++ b/drivers/gpu/drm/tyr/driver.rs
>> @@ -66,19 +66,20 @@ unsafe impl Send for TyrData {}
>> unsafe impl Sync for TyrData {}
>>
>> 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)?;
>> +
>> + regs::GpuCommand::default()
>> + .set_command(regs::GPU_CMD_SOFT_RESET)
>> + .write(io);
>>
>> // TODO: We cannot poll, as there is no support in Rust currently, so we
>> // sleep. Change this when read_poll_timeout() is implemented in Rust.
>> kernel::time::delay::fsleep(time::Delta::from_millis(100));
>>
>> - if regs::GPU_IRQ_RAWSTAT.read(dev, iomem)? & regs::GPU_IRQ_RAWSTAT_RESET_COMPLETED == 0 {
>> + let rawstat = regs::GpuIrqRawstat::read(io);
>> + if !rawstat.reset_completed() {
>> dev_err!(dev, "GPU reset failed with errno\n");
>> - dev_err!(
>> - dev,
>> - "GPU_INT_RAWSTAT is {}\n",
>> - regs::GPU_IRQ_RAWSTAT.read(dev, iomem)?
>> - );
>> + dev_err!(dev, "GPU_INT_RAWSTAT is {}\n", u32::from(rawstat));
>
>
> This is pre-existing, but printing `... INT ...` for `...IRQ...`
> register looks confusing (wrong?).
Yeah, this needs to change indeed.
>
>
>> return Err(EIO);
>> }
>> diff --git a/drivers/gpu/drm/tyr/gpu.rs b/drivers/gpu/drm/tyr/gpu.rs
>> index 6c582910dd5d..7c698fb1e36a 100644
>> --- a/drivers/gpu/drm/tyr/gpu.rs
>> +++ b/drivers/gpu/drm/tyr/gpu.rs
>> @@ -44,34 +44,36 @@ pub(crate) struct GpuInfo {
>>
>> impl GpuInfo {
>> pub(crate) fn new(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result<Self> {
>> - let gpu_id = regs::GPU_ID.read(dev, iomem)?;
>> - let csf_id = regs::GPU_CSF_ID.read(dev, iomem)?;
>> - let gpu_rev = regs::GPU_REVID.read(dev, iomem)?;
>> - let core_features = regs::GPU_CORE_FEATURES.read(dev, iomem)?;
>> - let l2_features = regs::GPU_L2_FEATURES.read(dev, iomem)?;
>> - let tiler_features = regs::GPU_TILER_FEATURES.read(dev, iomem)?;
>> - let mem_features = regs::GPU_MEM_FEATURES.read(dev, iomem)?;
>> - let mmu_features = regs::GPU_MMU_FEATURES.read(dev, iomem)?;
>> - let thread_features = regs::GPU_THREAD_FEATURES.read(dev, iomem)?;
>> - let max_threads = regs::GPU_THREAD_MAX_THREADS.read(dev, iomem)?;
>> - let thread_max_workgroup_size = regs::GPU_THREAD_MAX_WORKGROUP_SIZE.read(dev, iomem)?;
>> - let thread_max_barrier_size = regs::GPU_THREAD_MAX_BARRIER_SIZE.read(dev, iomem)?;
>> - let coherency_features = regs::GPU_COHERENCY_FEATURES.read(dev, iomem)?;
>> -
>> - let texture_features = regs::GPU_TEXTURE_FEATURES0.read(dev, iomem)?;
>> -
>> - let as_present = regs::GPU_AS_PRESENT.read(dev, iomem)?;
>> -
>> - let shader_present = u64::from(regs::GPU_SHADER_PRESENT_LO.read(dev, iomem)?);
>> + let io = (*iomem).access(dev)?;
>> +
>> + let gpu_id = regs::GpuId::read(io).into();
>> + let csf_id = regs::CsfId::read(io).into();
>> + let gpu_rev = regs::RevIdr::read(io).into();
>> + let core_features = regs::CoreFeatures::read(io).into();
>> + let l2_features = regs::L2Features::read(io).into();
>> + let tiler_features = regs::TilerFeatures::read(io).into();
>> + let mem_features = regs::MemFeatures::read(io).into();
>> + let mmu_features = regs::MmuFeatures::read(io).into();
>> + let thread_features = regs::ThreadFeatures::read(io).into();
>> + let max_threads = regs::ThreadMaxThreads::read(io).into();
>> + let thread_max_workgroup_size = regs::ThreadMaxWorkgroupSize::read(io).into();
>> + let thread_max_barrier_size = regs::ThreadMaxBarrierSize::read(io).into();
>> + let coherency_features = regs::CoherencyFeatures::read(io).into();
>
>
> Is there any reason why you replace the UPPERCASE register names with
> CamelCase ones?
>
> I was under the impression that we want to use UPPERCASE for register
> names. Like in nova
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/nova-core/regs.rs
Not really. UPPERCASE for non-const items will trigger the linter. The Nova
people chose to #[allow] this to align with OpenRM and, IIRC from the LPC
discussions, their registers are automatically generated from some internal
docs.
We have only a few, we can simply convert them to CamelCase.
>
>
>
>> + let texture_features = regs::TextureFeatures::read(io, 0).into();
>> +
>> + let as_present = regs::AsPresent::read(io).into();
>> +
>> + let shader_present = u64::from(u32::from(regs::ShaderPresentLo::read(io)));
>> let shader_present =
>> - shader_present | u64::from(regs::GPU_SHADER_PRESENT_HI.read(dev, iomem)?) << 32;
>> + shader_present | u64::from(u32::from(regs::ShaderPresentHi::read(io))) << 32;
>>
>> - let tiler_present = u64::from(regs::GPU_TILER_PRESENT_LO.read(dev, iomem)?);
>> + let tiler_present = u64::from(u32::from(regs::TilerPresentLo::read(io)));
>> let tiler_present =
>> - tiler_present | u64::from(regs::GPU_TILER_PRESENT_HI.read(dev, iomem)?) << 32;
>> + tiler_present | u64::from(u32::from(regs::TilerPresentHi::read(io))) << 32;
>>
>> - let l2_present = u64::from(regs::GPU_L2_PRESENT_LO.read(dev, iomem)?);
>> - let l2_present = l2_present | u64::from(regs::GPU_L2_PRESENT_HI.read(dev, iomem)?) << 32;
>> + let l2_present = u64::from(u32::from(regs::L2PresentLo::read(io)));
>> + let l2_present = l2_present | u64::from(u32::from(regs::L2PresentHi::read(io))) << 32;
>>
>> Ok(Self {
>> gpu_id,
>> @@ -204,13 +206,14 @@ fn from(value: u32) -> Self {
>>
>> /// 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)?;
>> + regs::L2PwrOnLo::default().set_l2_pwron_lo(1).write(io);
>>
>> // TODO: We cannot poll, as there is no support in Rust currently, so we
>> // sleep. Change this when read_poll_timeout() is implemented in Rust.
>> kernel::time::delay::fsleep(time::Delta::from_millis(100));
>>
>> - if regs::L2_READY_LO.read(dev, iomem)? != 1 {
>> + if regs::L2ReadyLo::read(io).l2_ready_lo() != 1 {
>> dev_err!(dev, "Failed to power on the GPU\n");
>> return Err(EIO);
>> }
>> diff --git a/drivers/gpu/drm/tyr/regs.rs b/drivers/gpu/drm/tyr/regs.rs
>> index f46933aaa221..a4e05ff463c0 100644
>> --- a/drivers/gpu/drm/tyr/regs.rs
>> +++ b/drivers/gpu/drm/tyr/regs.rs
>> @@ -8,44 +8,62 @@
>> #![allow(dead_code)]
>>
>> use kernel::bits::bit_u32;
>> -use kernel::device::Bound;
>> -use kernel::device::Device;
>> -use kernel::devres::Devres;
>> use kernel::prelude::*;
>> +use kernel::register;
>>
>> -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(())
>> - }
>> -}
>> -
>> -pub(crate) const GPU_ID: Register<0x0> = Register;
>> -pub(crate) const GPU_L2_FEATURES: Register<0x4> = Register;
>> -pub(crate) const GPU_CORE_FEATURES: Register<0x8> = Register;
>> -pub(crate) const GPU_CSF_ID: Register<0x1c> = Register;
>> -pub(crate) const GPU_REVID: Register<0x280> = Register;
>> -pub(crate) const GPU_TILER_FEATURES: Register<0xc> = Register;
>> -pub(crate) const GPU_MEM_FEATURES: Register<0x10> = Register;
>> -pub(crate) const GPU_MMU_FEATURES: Register<0x14> = Register;
>> -pub(crate) const GPU_AS_PRESENT: Register<0x18> = Register;
>> -pub(crate) const GPU_IRQ_RAWSTAT: Register<0x20> = Register;
>> +register!(GpuId @ 0x0, "Information about the GPU architecture and release version" {
>> + 3:0 version_status as u32, "Status of the GPU release";
>> + 11:4 version_minor as u32, "Minor release version number";
>> + 15:12 version_major as u32, "Major release version number";
>> + 19:16 product_major as u32, "Product identifier";
>> + 23:20 arch_rev as u32, "Architecture patch revision";
>> + 27:24 arch_minor as u32, "Architecture minor revision";
>> + 31:28 arch_major as u32, "Architecture major revision";
>> +});
>> +
>> +register!(L2Features @ 0x4, "Level 2 cache features" {
>> + 7:0 line_size as u32, "L2 cache line size";
>> + 15:8 associativity as u32, "L2 cache associativity";
>> + 23:16 cache_size as u32, "L2 cache slice size";
>> + 31:24 bus_width as u32, "L2 cache bus width";
>> +});
>> +
>> +register!(CoreFeatures @ 0x8, "Information about the features of a shader core" {
>> + 7:0 core_variant as u32, "Shader core variant";
>> +});
>> +
>> +register!(CsfId @ 0x1c, "Version of the CSF hardware and MMU subsystem" {
>> + 3:0 mcu_rev as u32, "MCU revision ID";
>> + 9:4 mcu_minor as u32, "MCU minor revision number";
>> + 15:10 mcu_major as u32, "MCU major revision number";
>> + 19:16 cshw_rev as u32, "CSHW revision ID";
>> + 25:20 cshw_minor as u32, "CSHW minor revision number";
>> + 31:26 cshw_major as u32, "CSHW major revision number";
>> +});
>> +
>> +register!(RevIdr @ 0x280, "Extra revision information" {
>> + 31:0 revision as u32, "Revision information";
>> +});
>> +
>> +register!(TilerFeatures @ 0xc, "Tiler features" {
>> + 5:0 bin_size as u32, "Log of the tiler's bin size";
>> + 11:8 max_levels as u32, "Maximum number of available levels";
>> +});
>> +
>> +register!(MemFeatures @ 0x10, "Memory features" {
>> + 0:0 coherent_core_group as bool, "Core group is coherent";
>> + 1:1 coherent_super_group as bool, "Core supergroup is coherent";
>> + 11:8 l2_slices as u32, "L2 slice count";
>> +});
>> +
>> +register!(MmuFeatures @ 0x14, "MMU features" {
>> + 7:0 va_bits as u32, "Number of bits supported in virtual addresses";
>> + 15:8 pa_bits as u32, "Number of bits supported in physical addresses";
>> +});
>> +
>> +register!(AsPresent @ 0x18, "Address spaces present" {
>> + 31:0 as_present as u32, "Bitmask of present address spaces";
>> +});
>>
>> pub(crate) const GPU_IRQ_RAWSTAT_FAULT: u32 = bit_u32(0);
>> pub(crate) const GPU_IRQ_RAWSTAT_PROTECTED_FAULT: u32 = bit_u32(1);
>> @@ -56,53 +74,193 @@ pub(crate) fn write(&self, dev: &Device<Bound>, iomem: &Devres<IoMem>, value: u3
>> pub(crate) const GPU_IRQ_RAWSTAT_DOORBELL_STATUS: u32 = bit_u32(18);
>> pub(crate) const GPU_IRQ_RAWSTAT_MCU_STATUS: u32 = bit_u32(19);
>>
>> -pub(crate) const GPU_IRQ_CLEAR: Register<0x24> = Register;
>> -pub(crate) const GPU_IRQ_MASK: Register<0x28> = Register;
>> -pub(crate) const GPU_IRQ_STAT: Register<0x2c> = Register;
>> -pub(crate) const GPU_CMD: Register<0x30> = Register;
>> +register!(GpuIrqRawstat @ 0x20, "Raw unmasked interrupt status for the GPU" {
>> + 0:0 fault as bool, "A GPU fault has occourred";
>> + 1:1 protected_fault as bool, "Indicates a protected memory fault has occurred";
>> + 8:8 reset_completed as bool, "Indicates that a GPU reset has completed";
>> + 9:9 power_changed_single as bool, "Set when a single power domain has powered up or down";
>> + 10:10 power_changed_all as bool, "Set when all pending power domain changes are completed ";
>> + 17:17 clean_caches_completed as bool, "Indicates that a cache clean operation has completed";
>> + 18:18 doorbell_status as bool, "Mirrors the doorbell interrupt line to the CPU";
>> + 19:19 mcu_status as bool, "The MCU requires attention";
>> +});
>> +
>> +register!(GpuIrqClear @ 0x24, "Clears pending GPU interrupts" {
>> + 0:0 fault as bool, "Clear the fault interrupt";
>> + 1:1 protected_fault as bool, "Clear the protected_fault interrupt";
>> + 8:8 reset_completed as bool, "Clear the reset_completed interrupt";
>> + 9:9 power_changed_single as bool, "Clear the power_changed_single interrupt";
>> + 10:10 power_changed_all as bool, "Clear the power_changed_all interrupt";
>> + 17:17 clean_caches_completed as bool, "Clear the clean_caches_completed interrupt";
>> + 18:18 doorbell_status as bool, "Clear the doorbell_status interrupt";
>> + 19:19 mcu_status as bool, "Clear the mcu_status interrupt";
>> +});
>> +
>> +register!(GpuIrqMask @ 0x28, "Enabled GPU interrupts" {
>> + 0:0 fault as bool, "Enable the fault interrupt";
>> + 1:1 protected_fault as bool, "Enable the protected_fault interrupt";
>> + 8:8 reset_completed as bool, "Enable the reset_completed interrupt";
>> + 9:9 power_changed_single as bool, "Enable the power_changed_single interrupt";
>> + 10:10 power_changed_all as bool, "Enable the power_changed_all interrupt";
>> + 17:17 clean_caches_completed as bool, "Enable the clean_caches_completed interrupt";
>> + 18:18 doorbell_status as bool, "Enable the doorbell_status interrupt";
>> + 19:19 mcu_status as bool, "Enable the mcu_status interrupt";
>> +});
>> +
>> +register!(GpuIrqStatus @ 0x2c, "Masked GPU interrupt status" {
>> + 0:0 fault as bool, "The fault interrupt is pending";
>> + 1:1 protected_fault as bool, "The protected_fault interrupt is pending";
>> + 8:8 reset_completed as bool, "The reset_completed interrupt is pending";
>> + 9:9 power_changed_single as bool, "The power_changed_single interrupt is pending";
>> + 10:10 power_changed_all as bool, "The power_changed_all interrupt is pending";
>> + 17:17 clean_caches_completed as bool, "The clean_caches_completed interrupt is pending";
>> + 18:18 doorbell_status as bool, "The doorbell_status interrupt is pending";
>> + 19:19 mcu_status as bool, "The mcu_status interrupt is pending";
>> +});
>> +
>> pub(crate) const GPU_CMD_SOFT_RESET: u32 = 1 | (1 << 8);
>> pub(crate) const GPU_CMD_HARD_RESET: u32 = 1 | (2 << 8);
>> -pub(crate) const GPU_THREAD_FEATURES: Register<0xac> = Register;
>> -pub(crate) const GPU_THREAD_MAX_THREADS: Register<0xa0> = Register;
>> -pub(crate) const GPU_THREAD_MAX_WORKGROUP_SIZE: Register<0xa4> = Register;
>> -pub(crate) const GPU_THREAD_MAX_BARRIER_SIZE: Register<0xa8> = Register;
>> -pub(crate) const GPU_TEXTURE_FEATURES0: Register<0xb0> = Register;
>> -pub(crate) const GPU_SHADER_PRESENT_LO: Register<0x100> = Register;
>> -pub(crate) const GPU_SHADER_PRESENT_HI: Register<0x104> = Register;
>> -pub(crate) const GPU_TILER_PRESENT_LO: Register<0x110> = Register;
>> -pub(crate) const GPU_TILER_PRESENT_HI: Register<0x114> = Register;
>> -pub(crate) const GPU_L2_PRESENT_LO: Register<0x120> = Register;
>> -pub(crate) const GPU_L2_PRESENT_HI: Register<0x124> = Register;
>> -pub(crate) const L2_READY_LO: Register<0x160> = Register;
>> -pub(crate) const L2_READY_HI: Register<0x164> = Register;
>> -pub(crate) const L2_PWRON_LO: Register<0x1a0> = Register;
>> -pub(crate) const L2_PWRON_HI: Register<0x1a4> = Register;
>> -pub(crate) const L2_PWRTRANS_LO: Register<0x220> = Register;
>> -pub(crate) const L2_PWRTRANS_HI: Register<0x204> = Register;
>> -pub(crate) const L2_PWRACTIVE_LO: Register<0x260> = Register;
>> -pub(crate) const L2_PWRACTIVE_HI: Register<0x264> = Register;
>> -
>> -pub(crate) const MCU_CONTROL: Register<0x700> = Register;
>> +
>> +register!(GpuCommand @ 0x30, "GPU command register" {
>> + 7:0 command as u32, "GPU-specific command to execute";
>> + 31:8 payload as u32, "Payload for the command";
>> +});
>> +
>> +register!(ThreadFeatures @ 0xac, "Thread features of the GPU's threading system" {
>> + 21:0 max_registers as u32, "Total number of registers per core";
>> + 23:22 implementation_technology as u32;
>> + 31:24 max_task_queue as u32, "Maximum number of compute tasks waiting";
>> +
>> +});
>> +
>> +register!(ThreadMaxThreads @ 0xa0, "Maximum number of threads per core" {
>> + 31:0 max_threads as u32, "Maximum number of threads per core";
>> +});
>> +
>> +register!(ThreadMaxWorkgroupSize @ 0xa4, "Maximum number of threads per workgroup" {
>> + 31:0 max_workgroup_size as u32, "Maximum number of threads per workgroup";
>> +});
>> +
>> +register!(ThreadMaxBarrierSize @ 0xa8, "Maximum number of threads per barrier" {
>> + 31:0 max_barrier_size as u32, "Maximum number of threads per barrier";
>> +});
>> +
>> +register!(TextureFeatures @ 0xb0 [4], "Bitmap of supported texture formats" {});
>> +
>> +register!(ShaderPresentLo @ 0x100, "Bitmap of shader cores present in the hardware (lower 32 bits)" {
>> + 31:0 shader_present_lo as u32, "Bitmap of shader cores present in the hardware (lower 32 bits)";
>> +});
>> +
>> +register!(ShaderPresentHi @ 0x104, "Bitmap of shader cores present in the hardware (higher 32 bits)" {
>> + 31:0 shader_present_hi as u32, "Bitmap of shader cores present in the hardware (higher 32 bits)";
>> +});
>> +
>> +register!(TilerPresentLo @ 0x110, "Bitmap of tiler cores present in the hardware (lower 32 bits)" {
>> + 31:0 tiler_present_lo as u32, "Bitmap of tiler cores present in the hardware (lower 32 bits)";
>> +});
>> +
>> +register!(TilerPresentHi @ 0x114, "Bitmap of tiler cores present in the hardware (higher 32 bits)" {
>> + 31:0 tiler_present_hi as u32, "Bitmap of tiler cores present in the hardware (higher 32 bits)";
>> +});
>> +
>> +register!(L2PresentLo @ 0x120, "Bitmap of L2 caches present in the hardware (lower 32 bits)" {
>> + 31:0 l2_present_lo as u32, "Bitmap of L2 caches present in the hardware (lower 32 bits)";
>> +});
>> +
>> +register!(L2PresentHi @ 0x124, "Bitmap of L2 caches present in the hardware (higher 32 bits)" {
>> + 31:0 l2_present_hi as u32, "Bitmap of L2 caches present in the hardware (higher 32 bits)";
>> +});
>> +
>> +register!(L2ReadyLo @ 0x160, "Bitmap of L2 caches ready (lower 32 bits)" {
>> + 31:0 l2_ready_lo as u32, "Bitmap of L2 caches ready (lower 32 bits)";
>> +});
>> +
>> +register!(L2ReadyHi @ 0x164, "Bitmap of L2 caches ready (higher 32 bits)" {
>> + 31:0 l2_ready_hi as u32, "Bitmap of L2 caches ready (higher 32 bits)";
>> +});
>> +
>> +register!(L2PwrOnLo @ 0x1a0, "Bitmap of L2 caches power on requests (lower 32 bits)" {
>> + 31:0 l2_pwron_lo as u32, "Bitmap of L2 caches power on requests (lower 32 bits)";
>> +});
>> +
>> +register!(L2PwrOnHi @ 0x1a4, "Bitmap of L2 caches power on requests (higher 32 bits)" {
>> + 31:0 l2_pwron_hi as u32, "Bitmap of L2 caches power on requests (higher 32 bits)";
>> +});
>> +
>> +register!(L2PwrTransLo @ 0x200, "Bitmap of L2 caches in power transition (lower 32 bits)" {
>> + 31:0 l2_pwrtrans_lo as u32, "Bitmap of L2 caches in power transition (lower 32 bits)";
>> +});
>> +
>> +register!(L2PwrTransHi @ 0x204, "Bitmap of L2 caches in power transition (higher 32 bits)" {
>> + 31:0 l2_pwrtrans_hi as u32, "Bitmap of L2 caches in power transition (higher 32 bits)";
>> +});
>> +
>> +register!(L2PowerActiveLo @ 0x260, "Bitmap of L2 caches active (lower 32 bits)" {
>> + 31:0 l2_pwractive_lo as u32, "Bitmap of L2 caches active (lower 32 bits)";
>> +});
>> +
>> +register!(L2PowerActiveHi @ 0x264, "Bitmap of L2 caches active (higher 32 bits)" {
>> + 31:0 l2_pwractive_hi as u32, "Bitmap of L2 caches active (higher 32 bits)";
>> +});
>> +
>> pub(crate) const MCU_CONTROL_ENABLE: u32 = 1;
>> pub(crate) const MCU_CONTROL_AUTO: u32 = 2;
>> pub(crate) const MCU_CONTROL_DISABLE: u32 = 0;
>>
>> -pub(crate) const MCU_STATUS: Register<0x704> = Register;
>> +register!(McuControl @ 0x700, "Controls the execution state of the MCU subsystem" {
>> + 1:0 req as u32, "Request state change";
>> +});
>
>
> Any reason why req is a u32 and not a u8? Same for some other places.
I tend to default to u32/i32 in general, as that’s usually the native machine integer type.
All we get from smaller types is a spam of `into()`, `from()` and their `try_`
equivalents. When stored in a struct, they usually do not save space due to
padding that is usually inserted to fix the alignment for the type. IMHO not
worth it unless it really matters. Correct me if I'm wrong, but it doesn't seem
to be the case here.
>
> And would it be an option to move the const MCU_CONTROL* to an ìmpl
> McuControl Same for STATUS below.
Ah true! I forgot about this. Thanks.
— Daniel
On Fri Jan 16, 2026 at 12:23 PM GMT, Daniel Almeida wrote:
> Hi Dirk, thanks for the review!
>
>> On 15 Jan 2026, at 14:05, Dirk Behme <dirk.behme@gmail.com> wrote:
>>
>> Hi Daniel,
>>
>> On 14.01.26 23:53, Daniel Almeida wrote:
>>> Replace regs::Register with kernel::register. This allow us to more
>>> succinctly express the register set by introducing the ability to describe
>>> fields and their documentation and to auto-generate the accessors. In
>>> particular, this is very helpful as it does away with a lot of manual masks
>>> and shifts.
>>
>>
>> As mentioned somewhere else already I really like switching to
>> register!(). Thanks!
>>
>> Some coments below:
>>
>>
>>> A future commit will eliminate HI/LO pairs once there is support for 64bit
>>> reads and writes in kernel::register.
>>>
>>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>>> ---
>>> Note that this patch depends on a rebased version of Joel's patch at [0].
>>>
>>> That version is stale, so I ended up rebasing it locally myself for the
>>> purpose of developing this patch and gathering some reviews on the list. In
>>> other words, the current patch does not apply for the time being, but will
>>> once a v7 for Joel's series is out.
>>>
>>> [0]: https://lore.kernel.org/rust-for-linux/20251003154748.1687160-1-joelagnelf@nvidia.com/
>>> ---
>>> drivers/gpu/drm/tyr/driver.rs | 15 ++-
>>> drivers/gpu/drm/tyr/gpu.rs | 55 ++++----
>>> drivers/gpu/drm/tyr/regs.rs | 302 ++++++++++++++++++++++++++++++++----------
>>> 3 files changed, 267 insertions(+), 105 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
>>> index 0389c558c036..8e06db5320bf 100644
>>> --- a/drivers/gpu/drm/tyr/driver.rs
>>> +++ b/drivers/gpu/drm/tyr/driver.rs
>>> @@ -66,19 +66,20 @@ unsafe impl Send for TyrData {}
>>> unsafe impl Sync for TyrData {}
>>>
>>> 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)?;
>>> +
>>> + regs::GpuCommand::default()
>>> + .set_command(regs::GPU_CMD_SOFT_RESET)
>>> + .write(io);
>>>
>>> // TODO: We cannot poll, as there is no support in Rust currently, so we
>>> // sleep. Change this when read_poll_timeout() is implemented in Rust.
>>> kernel::time::delay::fsleep(time::Delta::from_millis(100));
>>>
>>> - if regs::GPU_IRQ_RAWSTAT.read(dev, iomem)? & regs::GPU_IRQ_RAWSTAT_RESET_COMPLETED == 0 {
>>> + let rawstat = regs::GpuIrqRawstat::read(io);
>>> + if !rawstat.reset_completed() {
>>> dev_err!(dev, "GPU reset failed with errno\n");
>>> - dev_err!(
>>> - dev,
>>> - "GPU_INT_RAWSTAT is {}\n",
>>> - regs::GPU_IRQ_RAWSTAT.read(dev, iomem)?
>>> - );
>>> + dev_err!(dev, "GPU_INT_RAWSTAT is {}\n", u32::from(rawstat));
>>
>>
>> This is pre-existing, but printing `... INT ...` for `...IRQ...`
>> register looks confusing (wrong?).
>
> Yeah, this needs to change indeed.
>
>>
>>
>>> return Err(EIO);
>>> }
>>> diff --git a/drivers/gpu/drm/tyr/gpu.rs b/drivers/gpu/drm/tyr/gpu.rs
>>> index 6c582910dd5d..7c698fb1e36a 100644
>>> --- a/drivers/gpu/drm/tyr/gpu.rs
>>> +++ b/drivers/gpu/drm/tyr/gpu.rs
>>> @@ -44,34 +44,36 @@ pub(crate) struct GpuInfo {
>>>
>>> impl GpuInfo {
>>> pub(crate) fn new(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result<Self> {
>>> - let gpu_id = regs::GPU_ID.read(dev, iomem)?;
>>> - let csf_id = regs::GPU_CSF_ID.read(dev, iomem)?;
>>> - let gpu_rev = regs::GPU_REVID.read(dev, iomem)?;
>>> - let core_features = regs::GPU_CORE_FEATURES.read(dev, iomem)?;
>>> - let l2_features = regs::GPU_L2_FEATURES.read(dev, iomem)?;
>>> - let tiler_features = regs::GPU_TILER_FEATURES.read(dev, iomem)?;
>>> - let mem_features = regs::GPU_MEM_FEATURES.read(dev, iomem)?;
>>> - let mmu_features = regs::GPU_MMU_FEATURES.read(dev, iomem)?;
>>> - let thread_features = regs::GPU_THREAD_FEATURES.read(dev, iomem)?;
>>> - let max_threads = regs::GPU_THREAD_MAX_THREADS.read(dev, iomem)?;
>>> - let thread_max_workgroup_size = regs::GPU_THREAD_MAX_WORKGROUP_SIZE.read(dev, iomem)?;
>>> - let thread_max_barrier_size = regs::GPU_THREAD_MAX_BARRIER_SIZE.read(dev, iomem)?;
>>> - let coherency_features = regs::GPU_COHERENCY_FEATURES.read(dev, iomem)?;
>>> -
>>> - let texture_features = regs::GPU_TEXTURE_FEATURES0.read(dev, iomem)?;
>>> -
>>> - let as_present = regs::GPU_AS_PRESENT.read(dev, iomem)?;
>>> -
>>> - let shader_present = u64::from(regs::GPU_SHADER_PRESENT_LO.read(dev, iomem)?);
>>> + let io = (*iomem).access(dev)?;
>>> +
>>> + let gpu_id = regs::GpuId::read(io).into();
>>> + let csf_id = regs::CsfId::read(io).into();
>>> + let gpu_rev = regs::RevIdr::read(io).into();
>>> + let core_features = regs::CoreFeatures::read(io).into();
>>> + let l2_features = regs::L2Features::read(io).into();
>>> + let tiler_features = regs::TilerFeatures::read(io).into();
>>> + let mem_features = regs::MemFeatures::read(io).into();
>>> + let mmu_features = regs::MmuFeatures::read(io).into();
>>> + let thread_features = regs::ThreadFeatures::read(io).into();
>>> + let max_threads = regs::ThreadMaxThreads::read(io).into();
>>> + let thread_max_workgroup_size = regs::ThreadMaxWorkgroupSize::read(io).into();
>>> + let thread_max_barrier_size = regs::ThreadMaxBarrierSize::read(io).into();
>>> + let coherency_features = regs::CoherencyFeatures::read(io).into();
>>
>>
>> Is there any reason why you replace the UPPERCASE register names with
>> CamelCase ones?
>>
>> I was under the impression that we want to use UPPERCASE for register
>> names. Like in nova
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/nova-core/regs.rs
>
> Not really. UPPERCASE for non-const items will trigger the linter. The Nova
> people chose to #[allow] this to align with OpenRM and, IIRC from the LPC
> discussions, their registers are automatically generated from some internal
> docs.
>
> We have only a few, we can simply convert them to CamelCase.
Frankly, register names do look nicer in UPPER_CASE, especially that they're in
many cases, packed with acronyms.
Best,
Gary
>>
>>
>>
>>> + let texture_features = regs::TextureFeatures::read(io, 0).into();
>>> +
>>> + let as_present = regs::AsPresent::read(io).into();
>>> +
>>> + let shader_present = u64::from(u32::from(regs::ShaderPresentLo::read(io)));
>>> let shader_present =
>>> - shader_present | u64::from(regs::GPU_SHADER_PRESENT_HI.read(dev, iomem)?) << 32;
>>> + shader_present | u64::from(u32::from(regs::ShaderPresentHi::read(io))) << 32;
>>> >>> Is there any reason why you replace the UPPERCASE register names with >>> CamelCase ones? >>> >>> I was under the impression that we want to use UPPERCASE for register >>> names. Like in nova >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/nova-core/regs.rs >> >> Not really. UPPERCASE for non-const items will trigger the linter. The Nova >> people chose to #[allow] this to align with OpenRM and, IIRC from the LPC >> discussions, their registers are automatically generated from some internal >> docs. >> >> We have only a few, we can simply convert them to CamelCase. > > Frankly, register names do look nicer in UPPER_CASE, especially that they're in > many cases, packed with acronyms. > > Best, > Gary > I don’t have an opinion here, to be honest. I think CamelCase does make it easier on the eyes since our register names look quite simple, specially when compared to Nova. However, I can switch to UPPER_CASE and add an #![allow(non_camel_case_types)] if more people chime in.
On Fri Jan 16, 2026 at 1:38 PM GMT, Daniel Almeida wrote: > >>>> >>>> Is there any reason why you replace the UPPERCASE register names with >>>> CamelCase ones? >>>> >>>> I was under the impression that we want to use UPPERCASE for register >>>> names. Like in nova >>>> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/nova-core/regs.rs >>> >>> Not really. UPPERCASE for non-const items will trigger the linter. The Nova >>> people chose to #[allow] this to align with OpenRM and, IIRC from the LPC >>> discussions, their registers are automatically generated from some internal >>> docs. >>> >>> We have only a few, we can simply convert them to CamelCase. >> >> Frankly, register names do look nicer in UPPER_CASE, especially that they're in >> many cases, packed with acronyms. >> >> Best, >> Gary >> > > I don’t have an opinion here, to be honest. I think CamelCase does make it > easier on the eyes since our register names look quite simple, You're on the lucky side! Most hardware don't enjoy that, especially if you want to match register names with the ones documented on the datasheet. > specially when > compared to Nova. However, I can switch to UPPER_CASE and add an > #![allow(non_camel_case_types)] if more people chime in. I wonder if we should just such allow `non_camel_case_types` to the register macro? I don't have an opinion on whether we want to enforce using UPPER_CASE, but at least I think we should allow it. Best, Gary
On Fri Jan 16, 2026 at 2:53 PM CET, Gary Guo wrote: > On Fri Jan 16, 2026 at 1:38 PM GMT, Daniel Almeida wrote: >> >>>>> >>>>> Is there any reason why you replace the UPPERCASE register names with >>>>> CamelCase ones? >>>>> >>>>> I was under the impression that we want to use UPPERCASE for register >>>>> names. Like in nova >>>>> >>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/nova-core/regs.rs >>>> >>>> Not really. UPPERCASE for non-const items will trigger the linter. The Nova >>>> people chose to #[allow] this to align with OpenRM and, IIRC from the LPC >>>> discussions, their registers are automatically generated from some internal >>>> docs. >>>> >>>> We have only a few, we can simply convert them to CamelCase. >>> >>> Frankly, register names do look nicer in UPPER_CASE, especially that they're in >>> many cases, packed with acronyms. >>> >>> Best, >>> Gary >>> >> >> I don’t have an opinion here, to be honest. I think CamelCase does make it >> easier on the eyes since our register names look quite simple, I think you want to go with what your datasheets do, it is much easier for people if names are consistent. > > You're on the lucky side! Most hardware don't enjoy that, especially if > you want to match register names with the ones documented on the datasheet. > >> specially when >> compared to Nova. However, I can switch to UPPER_CASE and add an >> #![allow(non_camel_case_types)] if more people chime in. > > I wonder if we should just such allow `non_camel_case_types` to the register > macro? I don't have an opinion on whether we want to enforce using UPPER_CASE, > but at least I think we should allow it. I fully agree here. I would not enforce it either, but given that the absolute majority of datasheets uses UPPER_CASE for register names, we should allow it in the register!() macro.
Hi Daniel,
On 16/01/2026 13:23, Daniel Almeida wrote:
> Hi Dirk, thanks for the review!
>
>> On 15 Jan 2026, at 14:05, Dirk Behme <dirk.behme@gmail.com> wrote:
>>
>> Hi Daniel,
>>
>> On 14.01.26 23:53, Daniel Almeida wrote:
>>> Replace regs::Register with kernel::register. This allow us to more
>>> succinctly express the register set by introducing the ability to describe
>>> fields and their documentation and to auto-generate the accessors. In
>>> particular, this is very helpful as it does away with a lot of manual masks
>>> and shifts.
>>
>>
>> As mentioned somewhere else already I really like switching to
>> register!(). Thanks!
>>
>> Some coments below:
>>
>>
>>> A future commit will eliminate HI/LO pairs once there is support for 64bit
>>> reads and writes in kernel::register.
>>>
>>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>>> ---
>>> Note that this patch depends on a rebased version of Joel's patch at [0].
>>>
>>> That version is stale, so I ended up rebasing it locally myself for the
>>> purpose of developing this patch and gathering some reviews on the list. In
>>> other words, the current patch does not apply for the time being, but will
>>> once a v7 for Joel's series is out.
>>>
>>> [0]: https://lore.kernel.org/rust-for-linux/20251003154748.1687160-1-joelagnelf@nvidia.com/
>>> ---
>>> drivers/gpu/drm/tyr/driver.rs | 15 ++-
>>> drivers/gpu/drm/tyr/gpu.rs | 55 ++++----
>>> drivers/gpu/drm/tyr/regs.rs | 302 ++++++++++++++++++++++++++++++++----------
>>> 3 files changed, 267 insertions(+), 105 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
>>> index 0389c558c036..8e06db5320bf 100644
>>> --- a/drivers/gpu/drm/tyr/driver.rs
>>> +++ b/drivers/gpu/drm/tyr/driver.rs
>>> @@ -66,19 +66,20 @@ unsafe impl Send for TyrData {}
>>> unsafe impl Sync for TyrData {}
>>>
>>> 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)?;
>>> +
>>> + regs::GpuCommand::default()
>>> + .set_command(regs::GPU_CMD_SOFT_RESET)
>>> + .write(io);
>>>
>>> // TODO: We cannot poll, as there is no support in Rust currently, so we
>>> // sleep. Change this when read_poll_timeout() is implemented in Rust.
>>> kernel::time::delay::fsleep(time::Delta::from_millis(100));
>>>
>>> - if regs::GPU_IRQ_RAWSTAT.read(dev, iomem)? & regs::GPU_IRQ_RAWSTAT_RESET_COMPLETED == 0 {
>>> + let rawstat = regs::GpuIrqRawstat::read(io);
>>> + if !rawstat.reset_completed() {
>>> dev_err!(dev, "GPU reset failed with errno\n");
>>> - dev_err!(
>>> - dev,
>>> - "GPU_INT_RAWSTAT is {}\n",
>>> - regs::GPU_IRQ_RAWSTAT.read(dev, iomem)?
>>> - );
>>> + dev_err!(dev, "GPU_INT_RAWSTAT is {}\n", u32::from(rawstat));
>>
>>
>> This is pre-existing, but printing `... INT ...` for `...IRQ...`
>> register looks confusing (wrong?).
>
> Yeah, this needs to change indeed.
>
>>
>>
>>> return Err(EIO);
>>> }
>>> diff --git a/drivers/gpu/drm/tyr/gpu.rs b/drivers/gpu/drm/tyr/gpu.rs
>>> index 6c582910dd5d..7c698fb1e36a 100644
>>> --- a/drivers/gpu/drm/tyr/gpu.rs
>>> +++ b/drivers/gpu/drm/tyr/gpu.rs
>>> @@ -44,34 +44,36 @@ pub(crate) struct GpuInfo {
>>>
>>> impl GpuInfo {
>>> pub(crate) fn new(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result<Self> {
>>> - let gpu_id = regs::GPU_ID.read(dev, iomem)?;
>>> - let csf_id = regs::GPU_CSF_ID.read(dev, iomem)?;
>>> - let gpu_rev = regs::GPU_REVID.read(dev, iomem)?;
>>> - let core_features = regs::GPU_CORE_FEATURES.read(dev, iomem)?;
>>> - let l2_features = regs::GPU_L2_FEATURES.read(dev, iomem)?;
>>> - let tiler_features = regs::GPU_TILER_FEATURES.read(dev, iomem)?;
>>> - let mem_features = regs::GPU_MEM_FEATURES.read(dev, iomem)?;
>>> - let mmu_features = regs::GPU_MMU_FEATURES.read(dev, iomem)?;
>>> - let thread_features = regs::GPU_THREAD_FEATURES.read(dev, iomem)?;
>>> - let max_threads = regs::GPU_THREAD_MAX_THREADS.read(dev, iomem)?;
>>> - let thread_max_workgroup_size = regs::GPU_THREAD_MAX_WORKGROUP_SIZE.read(dev, iomem)?;
>>> - let thread_max_barrier_size = regs::GPU_THREAD_MAX_BARRIER_SIZE.read(dev, iomem)?;
>>> - let coherency_features = regs::GPU_COHERENCY_FEATURES.read(dev, iomem)?;
>>> -
>>> - let texture_features = regs::GPU_TEXTURE_FEATURES0.read(dev, iomem)?;
>>> -
>>> - let as_present = regs::GPU_AS_PRESENT.read(dev, iomem)?;
>>> -
>>> - let shader_present = u64::from(regs::GPU_SHADER_PRESENT_LO.read(dev, iomem)?);
>>> + let io = (*iomem).access(dev)?;
>>> +
>>> + let gpu_id = regs::GpuId::read(io).into();
>>> + let csf_id = regs::CsfId::read(io).into();
>>> + let gpu_rev = regs::RevIdr::read(io).into();
>>> + let core_features = regs::CoreFeatures::read(io).into();
>>> + let l2_features = regs::L2Features::read(io).into();
>>> + let tiler_features = regs::TilerFeatures::read(io).into();
>>> + let mem_features = regs::MemFeatures::read(io).into();
>>> + let mmu_features = regs::MmuFeatures::read(io).into();
>>> + let thread_features = regs::ThreadFeatures::read(io).into();
>>> + let max_threads = regs::ThreadMaxThreads::read(io).into();
>>> + let thread_max_workgroup_size = regs::ThreadMaxWorkgroupSize::read(io).into();
>>> + let thread_max_barrier_size = regs::ThreadMaxBarrierSize::read(io).into();
>>> + let coherency_features = regs::CoherencyFeatures::read(io).into();
>>
>>
>> Is there any reason why you replace the UPPERCASE register names with
>> CamelCase ones?
>>
>> I was under the impression that we want to use UPPERCASE for register
>> names. Like in nova
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/nova-core/regs.rs
>
> Not really. UPPERCASE for non-const items will trigger the linter. The Nova
> people chose to #[allow] this to align with OpenRM and, IIRC from the LPC
> discussions, their registers are automatically generated from some internal
> docs.
>
> We have only a few, we can simply convert them to CamelCase.
I'm under the impression that we define the "future RFL register!()
style standard" here.
So we want to make the CamelCase the default? And nova is the exception?
I'm fine with that. Just want to make sure we talked about it :)
....
>>> pub(crate) const MCU_CONTROL_ENABLE: u32 = 1;
>>> pub(crate) const MCU_CONTROL_AUTO: u32 = 2;
>>> pub(crate) const MCU_CONTROL_DISABLE: u32 = 0;
>>>
>>> -pub(crate) const MCU_STATUS: Register<0x704> = Register;
>>> +register!(McuControl @ 0x700, "Controls the execution state of the MCU subsystem" {
>>> + 1:0 req as u32, "Request state change";
>>> +});
>>
>>
>> Any reason why req is a u32 and not a u8? Same for some other places.
>
>
> I tend to default to u32/i32 in general, as that’s usually the native machine integer type.
>
> All we get from smaller types is a spam of `into()`, `from()` and their `try_`
> equivalents. When stored in a struct, they usually do not save space due to
> padding that is usually inserted to fix the alignment for the type. IMHO not
> worth it unless it really matters. Correct me if I'm wrong, but it doesn't seem
> to be the case here.
Wouldn't using u8 prevent any accidental access to 31:8 ?
Best regards
Dirk
>>>
>>> Is there any reason why you replace the UPPERCASE register names with
>>> CamelCase ones?
>>>
>>> I was under the impression that we want to use UPPERCASE for register
>>> names. Like in nova
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/nova-core/regs.rs
>> Not really. UPPERCASE for non-const items will trigger the linter. The Nova
>> people chose to #[allow] this to align with OpenRM and, IIRC from the LPC
>> discussions, their registers are automatically generated from some internal
>> docs.
>> We have only a few, we can simply convert them to CamelCase.
>
>
> I'm under the impression that we define the "future RFL register!() style standard" here.
>
> So we want to make the CamelCase the default? And nova is the exception?
I’m not sure I would say this. It’s just that you would hit this lint
[0]. If UPPER_CASE was the “default", we would have to have the #[allow] on
every driver.
>
> I'm fine with that. Just want to make sure we talked about it :)
>
>
> ....
>>>> pub(crate) const MCU_CONTROL_ENABLE: u32 = 1;
>>>> pub(crate) const MCU_CONTROL_AUTO: u32 = 2;
>>>> pub(crate) const MCU_CONTROL_DISABLE: u32 = 0;
>>>>
>>>> -pub(crate) const MCU_STATUS: Register<0x704> = Register;
>>>> +register!(McuControl @ 0x700, "Controls the execution state of the MCU subsystem" {
>>>> + 1:0 req as u32, "Request state change";
>>>> +});
>>>
>>>
>>> Any reason why req is a u32 and not a u8? Same for some other places.
>> I tend to default to u32/i32 in general, as that’s usually the native machine integer type.
>> All we get from smaller types is a spam of `into()`, `from()` and their `try_`
>> equivalents. When stored in a struct, they usually do not save space due to
>> padding that is usually inserted to fix the alignment for the type. IMHO not
>> worth it unless it really matters. Correct me if I'm wrong, but it doesn't seem
>> to be the case here.
>
>
> Wouldn't using u8 prevent any accidental access to 31:8 ?
The macro is doing the appropriate masking according to the bit ranges you pass
in (i.e.: 31:8), not according to the type (u8 or u32), i.e.:
const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi; <------
const [<$field:upper _MASK>]: $storage = {
// Generate mask for shifting
match ::core::mem::size_of::<$storage>() {
1 => ::kernel::bits::genmask_u8($lo..=$hi) as $storage,
2 => ::kernel::bits::genmask_u16($lo..=$hi) as $storage,
4 => ::kernel::bits::genmask_u32($lo..=$hi) as $storage,
8 => ::kernel::bits::genmask_u64($lo..=$hi) as $storage,
_ => ::kernel::build_error!("Unsupported storage type size")
}
};
const [<$field:upper _SHIFT>]: u32 = $lo;
);
$(
#[doc="Returns the value of this field:"]
#[doc=$comment]
)?
#[inline(always)]
$vis fn $field(self) -> $res_type {
::kernel::macros::paste!(
const MASK: $storage = $name::[<$field:upper _MASK>];
const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
);
let field = ((self.0 & MASK) >> SHIFT);
$process(field)
}
>
>
> Best regards
>
> Dirk
[0]: https://doc.rust-lang.org/stable/nightly-rustc/rustc_lint/nonstandard_style/static.NON_CAMEL_CASE_TYPES.html
On 15/01/2026 18:05, Dirk Behme wrote:
> Hi Daniel,
>
> On 14.01.26 23:53, Daniel Almeida wrote:
>> Replace regs::Register with kernel::register. This allow us to more
>> succinctly express the register set by introducing the ability to describe
>> fields and their documentation and to auto-generate the accessors. In
>> particular, this is very helpful as it does away with a lot of manual masks
>> and shifts.
>
>
> As mentioned somewhere else already I really like switching to
> register!(). Thanks!
>
> Some coments below:
>
>
>> A future commit will eliminate HI/LO pairs once there is support for 64bit
>> reads and writes in kernel::register.
>>
>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>> ---
>> Note that this patch depends on a rebased version of Joel's patch at [0].
>>
>> That version is stale, so I ended up rebasing it locally myself for the
>> purpose of developing this patch and gathering some reviews on the list. In
>> other words, the current patch does not apply for the time being, but will
>> once a v7 for Joel's series is out.
>>
>> [0]: https://lore.kernel.org/rust-for-linux/20251003154748.1687160-1-joelagnelf@nvidia.com/
>> ---
>> drivers/gpu/drm/tyr/driver.rs | 15 ++-
>> drivers/gpu/drm/tyr/gpu.rs | 55 ++++----
>> drivers/gpu/drm/tyr/regs.rs | 302 ++++++++++++++++++++++++++++++++----------
>> 3 files changed, 267 insertions(+), 105 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
>> index 0389c558c036..8e06db5320bf 100644
>> --- a/drivers/gpu/drm/tyr/driver.rs
>> +++ b/drivers/gpu/drm/tyr/driver.rs
>> @@ -66,19 +66,20 @@ unsafe impl Send for TyrData {}
>> unsafe impl Sync for TyrData {}
>>
>> 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)?;
>> +
>> + regs::GpuCommand::default()
>> + .set_command(regs::GPU_CMD_SOFT_RESET)
>> + .write(io);
>>
>> // TODO: We cannot poll, as there is no support in Rust currently, so we
>> // sleep. Change this when read_poll_timeout() is implemented in Rust.
>> kernel::time::delay::fsleep(time::Delta::from_millis(100));
>>
>> - if regs::GPU_IRQ_RAWSTAT.read(dev, iomem)? & regs::GPU_IRQ_RAWSTAT_RESET_COMPLETED == 0 {
>> + let rawstat = regs::GpuIrqRawstat::read(io);
>> + if !rawstat.reset_completed() {
>> dev_err!(dev, "GPU reset failed with errno\n");
>> - dev_err!(
>> - dev,
>> - "GPU_INT_RAWSTAT is {}\n",
>> - regs::GPU_IRQ_RAWSTAT.read(dev, iomem)?
>> - );
>> + dev_err!(dev, "GPU_INT_RAWSTAT is {}\n", u32::from(rawstat));
>
>
> This is pre-existing, but printing `... INT ...` for `...IRQ...`
> register looks confusing (wrong?).
>
>
>> return Err(EIO);
>> }
>> diff --git a/drivers/gpu/drm/tyr/gpu.rs b/drivers/gpu/drm/tyr/gpu.rs
>> index 6c582910dd5d..7c698fb1e36a 100644
>> --- a/drivers/gpu/drm/tyr/gpu.rs
>> +++ b/drivers/gpu/drm/tyr/gpu.rs
>> @@ -44,34 +44,36 @@ pub(crate) struct GpuInfo {
>>
>> impl GpuInfo {
>> pub(crate) fn new(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result<Self> {
>> - let gpu_id = regs::GPU_ID.read(dev, iomem)?;
>> - let csf_id = regs::GPU_CSF_ID.read(dev, iomem)?;
>> - let gpu_rev = regs::GPU_REVID.read(dev, iomem)?;
>> - let core_features = regs::GPU_CORE_FEATURES.read(dev, iomem)?;
>> - let l2_features = regs::GPU_L2_FEATURES.read(dev, iomem)?;
>> - let tiler_features = regs::GPU_TILER_FEATURES.read(dev, iomem)?;
>> - let mem_features = regs::GPU_MEM_FEATURES.read(dev, iomem)?;
>> - let mmu_features = regs::GPU_MMU_FEATURES.read(dev, iomem)?;
>> - let thread_features = regs::GPU_THREAD_FEATURES.read(dev, iomem)?;
>> - let max_threads = regs::GPU_THREAD_MAX_THREADS.read(dev, iomem)?;
>> - let thread_max_workgroup_size = regs::GPU_THREAD_MAX_WORKGROUP_SIZE.read(dev, iomem)?;
>> - let thread_max_barrier_size = regs::GPU_THREAD_MAX_BARRIER_SIZE.read(dev, iomem)?;
>> - let coherency_features = regs::GPU_COHERENCY_FEATURES.read(dev, iomem)?;
>> -
>> - let texture_features = regs::GPU_TEXTURE_FEATURES0.read(dev, iomem)?;
>> -
>> - let as_present = regs::GPU_AS_PRESENT.read(dev, iomem)?;
>> -
>> - let shader_present = u64::from(regs::GPU_SHADER_PRESENT_LO.read(dev, iomem)?);
>> + let io = (*iomem).access(dev)?;
>> +
>> + let gpu_id = regs::GpuId::read(io).into();
>> + let csf_id = regs::CsfId::read(io).into();
>> + let gpu_rev = regs::RevIdr::read(io).into();
>> + let core_features = regs::CoreFeatures::read(io).into();
>> + let l2_features = regs::L2Features::read(io).into();
>> + let tiler_features = regs::TilerFeatures::read(io).into();
>> + let mem_features = regs::MemFeatures::read(io).into();
>> + let mmu_features = regs::MmuFeatures::read(io).into();
>> + let thread_features = regs::ThreadFeatures::read(io).into();
>> + let max_threads = regs::ThreadMaxThreads::read(io).into();
>> + let thread_max_workgroup_size = regs::ThreadMaxWorkgroupSize::read(io).into();
>> + let thread_max_barrier_size = regs::ThreadMaxBarrierSize::read(io).into();
>> + let coherency_features = regs::CoherencyFeatures::read(io).into();
>
>
> Is there any reason why you replace the UPPERCASE register names with
> CamelCase ones?
>
> I was under the impression that we want to use UPPERCASE for register
> names. Like in nova
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/nova-core/regs.rs
>
>
>
>> + let texture_features = regs::TextureFeatures::read(io, 0).into();
>> +
>> + let as_present = regs::AsPresent::read(io).into();
>> +
>> + let shader_present = u64::from(u32::from(regs::ShaderPresentLo::read(io)));
>> let shader_present =
>> - shader_present | u64::from(regs::GPU_SHADER_PRESENT_HI.read(dev, iomem)?) << 32;
>> + shader_present | u64::from(u32::from(regs::ShaderPresentHi::read(io))) << 32;
>>
>> - let tiler_present = u64::from(regs::GPU_TILER_PRESENT_LO.read(dev, iomem)?);
>> + let tiler_present = u64::from(u32::from(regs::TilerPresentLo::read(io)));
>> let tiler_present =
>> - tiler_present | u64::from(regs::GPU_TILER_PRESENT_HI.read(dev, iomem)?) << 32;
>> + tiler_present | u64::from(u32::from(regs::TilerPresentHi::read(io))) << 32;
>>
>> - let l2_present = u64::from(regs::GPU_L2_PRESENT_LO.read(dev, iomem)?);
>> - let l2_present = l2_present | u64::from(regs::GPU_L2_PRESENT_HI.read(dev, iomem)?) << 32;
>> + let l2_present = u64::from(u32::from(regs::L2PresentLo::read(io)));
>> + let l2_present = l2_present | u64::from(u32::from(regs::L2PresentHi::read(io))) << 32;
>>
>> Ok(Self {
>> gpu_id,
>> @@ -204,13 +206,14 @@ fn from(value: u32) -> Self {
>>
>> /// 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)?;
>> + regs::L2PwrOnLo::default().set_l2_pwron_lo(1).write(io);
>>
>> // TODO: We cannot poll, as there is no support in Rust currently, so we
>> // sleep. Change this when read_poll_timeout() is implemented in Rust.
>> kernel::time::delay::fsleep(time::Delta::from_millis(100));
>>
>> - if regs::L2_READY_LO.read(dev, iomem)? != 1 {
>> + if regs::L2ReadyLo::read(io).l2_ready_lo() != 1 {
>> dev_err!(dev, "Failed to power on the GPU\n");
>> return Err(EIO);
>> }
>> diff --git a/drivers/gpu/drm/tyr/regs.rs b/drivers/gpu/drm/tyr/regs.rs
>> index f46933aaa221..a4e05ff463c0 100644
>> --- a/drivers/gpu/drm/tyr/regs.rs
>> +++ b/drivers/gpu/drm/tyr/regs.rs
>> @@ -8,44 +8,62 @@
>> #![allow(dead_code)]
>>
>> use kernel::bits::bit_u32;
>> -use kernel::device::Bound;
>> -use kernel::device::Device;
>> -use kernel::devres::Devres;
>> use kernel::prelude::*;
>> +use kernel::register;
>>
>> -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(())
>> - }
>> -}
>> -
>> -pub(crate) const GPU_ID: Register<0x0> = Register;
>> -pub(crate) const GPU_L2_FEATURES: Register<0x4> = Register;
>> -pub(crate) const GPU_CORE_FEATURES: Register<0x8> = Register;
>> -pub(crate) const GPU_CSF_ID: Register<0x1c> = Register;
>> -pub(crate) const GPU_REVID: Register<0x280> = Register;
>> -pub(crate) const GPU_TILER_FEATURES: Register<0xc> = Register;
>> -pub(crate) const GPU_MEM_FEATURES: Register<0x10> = Register;
>> -pub(crate) const GPU_MMU_FEATURES: Register<0x14> = Register;
>> -pub(crate) const GPU_AS_PRESENT: Register<0x18> = Register;
>> -pub(crate) const GPU_IRQ_RAWSTAT: Register<0x20> = Register;
>> +register!(GpuId @ 0x0, "Information about the GPU architecture and release version" {
>> + 3:0 version_status as u32, "Status of the GPU release";
>> + 11:4 version_minor as u32, "Minor release version number";
>> + 15:12 version_major as u32, "Major release version number";
>> + 19:16 product_major as u32, "Product identifier";
>> + 23:20 arch_rev as u32, "Architecture patch revision";
>> + 27:24 arch_minor as u32, "Architecture minor revision";
>> + 31:28 arch_major as u32, "Architecture major revision";
>> +});
>> +
>> +register!(L2Features @ 0x4, "Level 2 cache features" {
>> + 7:0 line_size as u32, "L2 cache line size";
>> + 15:8 associativity as u32, "L2 cache associativity";
>> + 23:16 cache_size as u32, "L2 cache slice size";
>> + 31:24 bus_width as u32, "L2 cache bus width";
>> +});
>> +
>> +register!(CoreFeatures @ 0x8, "Information about the features of a shader core" {
>> + 7:0 core_variant as u32, "Shader core variant";
>> +});
>> +
>> +register!(CsfId @ 0x1c, "Version of the CSF hardware and MMU subsystem" {
>> + 3:0 mcu_rev as u32, "MCU revision ID";
>> + 9:4 mcu_minor as u32, "MCU minor revision number";
>> + 15:10 mcu_major as u32, "MCU major revision number";
>> + 19:16 cshw_rev as u32, "CSHW revision ID";
>> + 25:20 cshw_minor as u32, "CSHW minor revision number";
>> + 31:26 cshw_major as u32, "CSHW major revision number";
>> +});
>> +
>> +register!(RevIdr @ 0x280, "Extra revision information" {
>> + 31:0 revision as u32, "Revision information";
>> +});
>> +
>> +register!(TilerFeatures @ 0xc, "Tiler features" {
>> + 5:0 bin_size as u32, "Log of the tiler's bin size";
>> + 11:8 max_levels as u32, "Maximum number of available levels";
>> +});
>> +
>> +register!(MemFeatures @ 0x10, "Memory features" {
>> + 0:0 coherent_core_group as bool, "Core group is coherent";
>> + 1:1 coherent_super_group as bool, "Core supergroup is coherent";
>> + 11:8 l2_slices as u32, "L2 slice count";
>> +});
>> +
>> +register!(MmuFeatures @ 0x14, "MMU features" {
>> + 7:0 va_bits as u32, "Number of bits supported in virtual addresses";
>> + 15:8 pa_bits as u32, "Number of bits supported in physical addresses";
>> +});
>> +
>> +register!(AsPresent @ 0x18, "Address spaces present" {
>> + 31:0 as_present as u32, "Bitmask of present address spaces";
>> +});
>>
>> pub(crate) const GPU_IRQ_RAWSTAT_FAULT: u32 = bit_u32(0);
>> pub(crate) const GPU_IRQ_RAWSTAT_PROTECTED_FAULT: u32 = bit_u32(1);
>> @@ -56,53 +74,193 @@ pub(crate) fn write(&self, dev: &Device<Bound>, iomem: &Devres<IoMem>, value: u3
>> pub(crate) const GPU_IRQ_RAWSTAT_DOORBELL_STATUS: u32 = bit_u32(18);
>> pub(crate) const GPU_IRQ_RAWSTAT_MCU_STATUS: u32 = bit_u32(19);
>>
>> -pub(crate) const GPU_IRQ_CLEAR: Register<0x24> = Register;
>> -pub(crate) const GPU_IRQ_MASK: Register<0x28> = Register;
>> -pub(crate) const GPU_IRQ_STAT: Register<0x2c> = Register;
>> -pub(crate) const GPU_CMD: Register<0x30> = Register;
>> +register!(GpuIrqRawstat @ 0x20, "Raw unmasked interrupt status for the GPU" {
>> + 0:0 fault as bool, "A GPU fault has occourred";
>> + 1:1 protected_fault as bool, "Indicates a protected memory fault has occurred";
>> + 8:8 reset_completed as bool, "Indicates that a GPU reset has completed";
>> + 9:9 power_changed_single as bool, "Set when a single power domain has powered up or down";
>> + 10:10 power_changed_all as bool, "Set when all pending power domain changes are completed ";
>> + 17:17 clean_caches_completed as bool, "Indicates that a cache clean operation has completed";
>> + 18:18 doorbell_status as bool, "Mirrors the doorbell interrupt line to the CPU";
>> + 19:19 mcu_status as bool, "The MCU requires attention";
>> +});
>> +
>> +register!(GpuIrqClear @ 0x24, "Clears pending GPU interrupts" {
>> + 0:0 fault as bool, "Clear the fault interrupt";
>> + 1:1 protected_fault as bool, "Clear the protected_fault interrupt";
>> + 8:8 reset_completed as bool, "Clear the reset_completed interrupt";
>> + 9:9 power_changed_single as bool, "Clear the power_changed_single interrupt";
>> + 10:10 power_changed_all as bool, "Clear the power_changed_all interrupt";
>> + 17:17 clean_caches_completed as bool, "Clear the clean_caches_completed interrupt";
>> + 18:18 doorbell_status as bool, "Clear the doorbell_status interrupt";
>> + 19:19 mcu_status as bool, "Clear the mcu_status interrupt";
>> +});
>> +
>> +register!(GpuIrqMask @ 0x28, "Enabled GPU interrupts" {
>> + 0:0 fault as bool, "Enable the fault interrupt";
>> + 1:1 protected_fault as bool, "Enable the protected_fault interrupt";
>> + 8:8 reset_completed as bool, "Enable the reset_completed interrupt";
>> + 9:9 power_changed_single as bool, "Enable the power_changed_single interrupt";
>> + 10:10 power_changed_all as bool, "Enable the power_changed_all interrupt";
>> + 17:17 clean_caches_completed as bool, "Enable the clean_caches_completed interrupt";
>> + 18:18 doorbell_status as bool, "Enable the doorbell_status interrupt";
>> + 19:19 mcu_status as bool, "Enable the mcu_status interrupt";
>> +});
>> +
>> +register!(GpuIrqStatus @ 0x2c, "Masked GPU interrupt status" {
>> + 0:0 fault as bool, "The fault interrupt is pending";
>> + 1:1 protected_fault as bool, "The protected_fault interrupt is pending";
>> + 8:8 reset_completed as bool, "The reset_completed interrupt is pending";
>> + 9:9 power_changed_single as bool, "The power_changed_single interrupt is pending";
>> + 10:10 power_changed_all as bool, "The power_changed_all interrupt is pending";
>> + 17:17 clean_caches_completed as bool, "The clean_caches_completed interrupt is pending";
>> + 18:18 doorbell_status as bool, "The doorbell_status interrupt is pending";
>> + 19:19 mcu_status as bool, "The mcu_status interrupt is pending";
>> +});
>> +
>> pub(crate) const GPU_CMD_SOFT_RESET: u32 = 1 | (1 << 8);
>> pub(crate) const GPU_CMD_HARD_RESET: u32 = 1 | (2 << 8);
>> -pub(crate) const GPU_THREAD_FEATURES: Register<0xac> = Register;
>> -pub(crate) const GPU_THREAD_MAX_THREADS: Register<0xa0> = Register;
>> -pub(crate) const GPU_THREAD_MAX_WORKGROUP_SIZE: Register<0xa4> = Register;
>> -pub(crate) const GPU_THREAD_MAX_BARRIER_SIZE: Register<0xa8> = Register;
>> -pub(crate) const GPU_TEXTURE_FEATURES0: Register<0xb0> = Register;
>> -pub(crate) const GPU_SHADER_PRESENT_LO: Register<0x100> = Register;
>> -pub(crate) const GPU_SHADER_PRESENT_HI: Register<0x104> = Register;
>> -pub(crate) const GPU_TILER_PRESENT_LO: Register<0x110> = Register;
>> -pub(crate) const GPU_TILER_PRESENT_HI: Register<0x114> = Register;
>> -pub(crate) const GPU_L2_PRESENT_LO: Register<0x120> = Register;
>> -pub(crate) const GPU_L2_PRESENT_HI: Register<0x124> = Register;
>> -pub(crate) const L2_READY_LO: Register<0x160> = Register;
>> -pub(crate) const L2_READY_HI: Register<0x164> = Register;
>> -pub(crate) const L2_PWRON_LO: Register<0x1a0> = Register;
>> -pub(crate) const L2_PWRON_HI: Register<0x1a4> = Register;
>> -pub(crate) const L2_PWRTRANS_LO: Register<0x220> = Register;
>> -pub(crate) const L2_PWRTRANS_HI: Register<0x204> = Register;
>> -pub(crate) const L2_PWRACTIVE_LO: Register<0x260> = Register;
>> -pub(crate) const L2_PWRACTIVE_HI: Register<0x264> = Register;
>> -
>> -pub(crate) const MCU_CONTROL: Register<0x700> = Register;
>> +
>> +register!(GpuCommand @ 0x30, "GPU command register" {
>> + 7:0 command as u32, "GPU-specific command to execute";
>> + 31:8 payload as u32, "Payload for the command";
>> +});
>> +
>> +register!(ThreadFeatures @ 0xac, "Thread features of the GPU's threading system" {
>> + 21:0 max_registers as u32, "Total number of registers per core";
>> + 23:22 implementation_technology as u32;
>> + 31:24 max_task_queue as u32, "Maximum number of compute tasks waiting";
>> +
>> +});
>> +
>> +register!(ThreadMaxThreads @ 0xa0, "Maximum number of threads per core" {
>> + 31:0 max_threads as u32, "Maximum number of threads per core";
>> +});
>> +
>> +register!(ThreadMaxWorkgroupSize @ 0xa4, "Maximum number of threads per workgroup" {
>> + 31:0 max_workgroup_size as u32, "Maximum number of threads per workgroup";
>> +});
>> +
>> +register!(ThreadMaxBarrierSize @ 0xa8, "Maximum number of threads per barrier" {
>> + 31:0 max_barrier_size as u32, "Maximum number of threads per barrier";
>> +});
>> +
>> +register!(TextureFeatures @ 0xb0 [4], "Bitmap of supported texture formats" {});
>> +
>> +register!(ShaderPresentLo @ 0x100, "Bitmap of shader cores present in the hardware (lower 32 bits)" {
>> + 31:0 shader_present_lo as u32, "Bitmap of shader cores present in the hardware (lower 32 bits)";
>> +});
>> +
>> +register!(ShaderPresentHi @ 0x104, "Bitmap of shader cores present in the hardware (higher 32 bits)" {
>> + 31:0 shader_present_hi as u32, "Bitmap of shader cores present in the hardware (higher 32 bits)";
>> +});
>> +
>> +register!(TilerPresentLo @ 0x110, "Bitmap of tiler cores present in the hardware (lower 32 bits)" {
>> + 31:0 tiler_present_lo as u32, "Bitmap of tiler cores present in the hardware (lower 32 bits)";
>> +});
>> +
>> +register!(TilerPresentHi @ 0x114, "Bitmap of tiler cores present in the hardware (higher 32 bits)" {
>> + 31:0 tiler_present_hi as u32, "Bitmap of tiler cores present in the hardware (higher 32 bits)";
>> +});
>> +
>> +register!(L2PresentLo @ 0x120, "Bitmap of L2 caches present in the hardware (lower 32 bits)" {
>> + 31:0 l2_present_lo as u32, "Bitmap of L2 caches present in the hardware (lower 32 bits)";
>> +});
>> +
>> +register!(L2PresentHi @ 0x124, "Bitmap of L2 caches present in the hardware (higher 32 bits)" {
>> + 31:0 l2_present_hi as u32, "Bitmap of L2 caches present in the hardware (higher 32 bits)";
>> +});
>> +
>> +register!(L2ReadyLo @ 0x160, "Bitmap of L2 caches ready (lower 32 bits)" {
>> + 31:0 l2_ready_lo as u32, "Bitmap of L2 caches ready (lower 32 bits)";
>> +});
>> +
>> +register!(L2ReadyHi @ 0x164, "Bitmap of L2 caches ready (higher 32 bits)" {
>> + 31:0 l2_ready_hi as u32, "Bitmap of L2 caches ready (higher 32 bits)";
>> +});
>> +
>> +register!(L2PwrOnLo @ 0x1a0, "Bitmap of L2 caches power on requests (lower 32 bits)" {
>> + 31:0 l2_pwron_lo as u32, "Bitmap of L2 caches power on requests (lower 32 bits)";
>> +});
>> +
>> +register!(L2PwrOnHi @ 0x1a4, "Bitmap of L2 caches power on requests (higher 32 bits)" {
>> + 31:0 l2_pwron_hi as u32, "Bitmap of L2 caches power on requests (higher 32 bits)";
>> +});
>> +
>> +register!(L2PwrTransLo @ 0x200, "Bitmap of L2 caches in power transition (lower 32 bits)" {
>> + 31:0 l2_pwrtrans_lo as u32, "Bitmap of L2 caches in power transition (lower 32 bits)";
>> +});
>> +
>> +register!(L2PwrTransHi @ 0x204, "Bitmap of L2 caches in power transition (higher 32 bits)" {
>> + 31:0 l2_pwrtrans_hi as u32, "Bitmap of L2 caches in power transition (higher 32 bits)";
>> +});
>> +
>> +register!(L2PowerActiveLo @ 0x260, "Bitmap of L2 caches active (lower 32 bits)" {
>> + 31:0 l2_pwractive_lo as u32, "Bitmap of L2 caches active (lower 32 bits)";
>> +});
>> +
>> +register!(L2PowerActiveHi @ 0x264, "Bitmap of L2 caches active (higher 32 bits)" {
>> + 31:0 l2_pwractive_hi as u32, "Bitmap of L2 caches active (higher 32 bits)";
>> +});
>> +
>> pub(crate) const MCU_CONTROL_ENABLE: u32 = 1;
>> pub(crate) const MCU_CONTROL_AUTO: u32 = 2;
>> pub(crate) const MCU_CONTROL_DISABLE: u32 = 0;
>>
>> -pub(crate) const MCU_STATUS: Register<0x704> = Register;
>> +register!(McuControl @ 0x700, "Controls the execution state of the MCU subsystem" {
>> + 1:0 req as u32, "Request state change";
>> +});
>
>
> Any reason why req is a u32 and not a u8? Same for some other places.
>
> And would it be an option to move the const MCU_CONTROL* to an ìmpl
> McuControl Same for STATUS below.
Just fyi something like [1] builds for me.
This is inspired by
https://lore.kernel.org/rust-for-linux/20251003154748.1687160-6-joelagnelf@nvidia.com/
Best regards
Dirk
[1]
#[repr(u32)]
#[derive(Debug, Default, Clone, Copy, PartialEq)]
enum McuControl {
#[default]
Disable = 0,
Enable = 1,
Auto = 2,
}
impl From<McuControl> for u8 {
fn from(ctrl: McuControl) -> Self {
ctrl as u8
}
}
impl From<u8> for McuControl {
fn from(req: u8) -> Self {
match req & 0x3 {
0 => McuControl::Disable,
1 => McuControl::Enable,
2 => McuControl::Auto,
_ => McuControl::Disable,
}
}
}
register!(MCUCONTROL @ 0x700, "Controls the execution state of the MCU
subsystem" {
1:0 req as u8 => McuControl, "Request state change";
});
+cc Alex,
>>> + 31:0 l2_pwractive_hi as u32, "Bitmap of L2 caches active (higher 32 bits)";
>>> +});
>>> +
>>> pub(crate) const MCU_CONTROL_ENABLE: u32 = 1;
>>> pub(crate) const MCU_CONTROL_AUTO: u32 = 2;
>>> pub(crate) const MCU_CONTROL_DISABLE: u32 = 0;
>>> -pub(crate) const MCU_STATUS: Register<0x704> = Register;
>>> +register!(McuControl @ 0x700, "Controls the execution state of the MCU subsystem" {
>>> + 1:0 req as u32, "Request state change";
>>> +});
>> Any reason why req is a u32 and not a u8? Same for some other places.
>> And would it be an option to move the const MCU_CONTROL* to an ìmpl
>> McuControl Same for STATUS below.
>
> Just fyi something like [1] builds for me.
>
> This is inspired by
>
> https://lore.kernel.org/rust-for-linux/20251003154748.1687160-6-joelagnelf@nvidia.com/
>
> Best regards
>
> Dirk
>
> [1]
>
> #[repr(u32)]
> #[derive(Debug, Default, Clone, Copy, PartialEq)]
> enum McuControl {
> #[default]
> Disable = 0,
> Enable = 1,
> Auto = 2,
> }
>
> impl From<McuControl> for u8 {
> fn from(ctrl: McuControl) -> Self {
> ctrl as u8
> }
> }
>
> impl From<u8> for McuControl {
> fn from(req: u8) -> Self {
> match req & 0x3 {
> 0 => McuControl::Disable,
> 1 => McuControl::Enable,
> 2 => McuControl::Auto,
> _ => McuControl::Disable,
> }
> }
> }
>
> register!(MCUCONTROL @ 0x700, "Controls the execution state of the MCU subsystem" {
> 1:0 req as u8 => McuControl, "Request state change";
> });
Alex, looking at the above, I wonder if a “as Foo” would be a good
addition to the macro? That would then invoke a TryFrom implementation, i.e.:
register!(MCUCONTROL @ 0x700, "Controls the execution state of the MCU subsystem" {
1:0 req as McuControl => McuControl, "Request state change";
});
On Fri Jan 16, 2026 at 9:26 PM JST, Daniel Almeida wrote:
> +cc Alex,
>
>
>>>> + 31:0 l2_pwractive_hi as u32, "Bitmap of L2 caches active (higher 32 bits)";
>>>> +});
>>>> +
>>>> pub(crate) const MCU_CONTROL_ENABLE: u32 = 1;
>>>> pub(crate) const MCU_CONTROL_AUTO: u32 = 2;
>>>> pub(crate) const MCU_CONTROL_DISABLE: u32 = 0;
>>>> -pub(crate) const MCU_STATUS: Register<0x704> = Register;
>>>> +register!(McuControl @ 0x700, "Controls the execution state of the MCU subsystem" {
>>>> + 1:0 req as u32, "Request state change";
>>>> +});
>>> Any reason why req is a u32 and not a u8? Same for some other places.
>>> And would it be an option to move the const MCU_CONTROL* to an ìmpl
>>> McuControl Same for STATUS below.
>>
>> Just fyi something like [1] builds for me.
>>
>> This is inspired by
>>
>> https://lore.kernel.org/rust-for-linux/20251003154748.1687160-6-joelagnelf@nvidia.com/
>>
>> Best regards
>>
>> Dirk
>>
>> [1]
>>
>> #[repr(u32)]
>> #[derive(Debug, Default, Clone, Copy, PartialEq)]
>> enum McuControl {
>> #[default]
>> Disable = 0,
>> Enable = 1,
>> Auto = 2,
>> }
>>
>> impl From<McuControl> for u8 {
>> fn from(ctrl: McuControl) -> Self {
>> ctrl as u8
>> }
>> }
>>
>> impl From<u8> for McuControl {
>> fn from(req: u8) -> Self {
>> match req & 0x3 {
>> 0 => McuControl::Disable,
>> 1 => McuControl::Enable,
>> 2 => McuControl::Auto,
>> _ => McuControl::Disable,
>> }
>> }
>> }
>>
>> register!(MCUCONTROL @ 0x700, "Controls the execution state of the MCU subsystem" {
>> 1:0 req as u8 => McuControl, "Request state change";
>> });
>
> Alex, looking at the above, I wonder if a “as Foo” would be a good
> addition to the macro? That would then invoke a TryFrom implementation, i.e.:
>
>
> register!(MCUCONTROL @ 0x700, "Controls the execution state of the MCU subsystem" {
> 1:0 req as McuControl => McuControl, "Request state change";
> });
So the register macro that I will submit for the kernel crate will be
significantly different from the one in Nova. Expect something closer to
[1].
This means that fields won't need to have an explicit integer type
anymore (they will automatically use the corresponding `BoundedInt`
instead).
IIUC this will them behave as you expect.
[1] https://lore.kernel.org/rust-for-linux/20251108-bounded_ints-v4-4-c9342ac7ebd1@nvidia.com/
On Fri Jan 16, 2026 at 4:19 PM CET, Alexandre Courbot wrote: > This means that fields won't need to have an explicit integer type > anymore (they will automatically use the corresponding `BoundedInt` > instead). Oh right, I forgot about this for a second.
On Fri Jan 16, 2026 at 1:26 PM CET, Daniel Almeida wrote:
> +cc Alex,
>
>
>>>> + 31:0 l2_pwractive_hi as u32, "Bitmap of L2 caches active (higher 32 bits)";
>>>> +});
>>>> +
>>>> pub(crate) const MCU_CONTROL_ENABLE: u32 = 1;
>>>> pub(crate) const MCU_CONTROL_AUTO: u32 = 2;
>>>> pub(crate) const MCU_CONTROL_DISABLE: u32 = 0;
>>>> -pub(crate) const MCU_STATUS: Register<0x704> = Register;
>>>> +register!(McuControl @ 0x700, "Controls the execution state of the MCU subsystem" {
>>>> + 1:0 req as u32, "Request state change";
>>>> +});
>>> Any reason why req is a u32 and not a u8? Same for some other places.
>>> And would it be an option to move the const MCU_CONTROL* to an ìmpl
>>> McuControl Same for STATUS below.
>>
>> Just fyi something like [1] builds for me.
>>
>> This is inspired by
>>
>> https://lore.kernel.org/rust-for-linux/20251003154748.1687160-6-joelagnelf@nvidia.com/
>>
>> Best regards
>>
>> Dirk
>>
>> [1]
>>
>> #[repr(u32)]
>> #[derive(Debug, Default, Clone, Copy, PartialEq)]
>> enum McuControl {
>> #[default]
>> Disable = 0,
>> Enable = 1,
>> Auto = 2,
>> }
>>
>> impl From<McuControl> for u8 {
>> fn from(ctrl: McuControl) -> Self {
>> ctrl as u8
>> }
>> }
>>
>> impl From<u8> for McuControl {
>> fn from(req: u8) -> Self {
>> match req & 0x3 {
>> 0 => McuControl::Disable,
>> 1 => McuControl::Enable,
>> 2 => McuControl::Auto,
>> _ => McuControl::Disable,
>> }
>> }
>> }
>>
>> register!(MCUCONTROL @ 0x700, "Controls the execution state of the MCU subsystem" {
>> 1:0 req as u8 => McuControl, "Request state change";
>> });
>
> Alex, looking at the above, I wonder if a “as Foo” would be a good
> addition to the macro? That would then invoke a TryFrom implementation, i.e.:
>
>
> register!(MCUCONTROL @ 0x700, "Controls the execution state of the MCU subsystem" {
> 1:0 req as McuControl => McuControl, "Request state change";
> });
This would imply the assumption that req is treated as u8 by register!()
automatically before calling the TryFrom impl.
One could argue that this is reasonable, since the value is only two bits wide,
but it might not always be desired. I think keeping this explict is better.
Hi Daniel,
As always I'm a bit lost on the Rust, but some comments below.
On 14/01/2026 22:53, Daniel Almeida wrote:
> Replace regs::Register with kernel::register. This allow us to more
> succinctly express the register set by introducing the ability to describe
> fields and their documentation and to auto-generate the accessors. In
> particular, this is very helpful as it does away with a lot of manual masks
> and shifts.
>
> A future commit will eliminate HI/LO pairs once there is support for 64bit
> reads and writes in kernel::register.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
> Note that this patch depends on a rebased version of Joel's patch at [0].
>
> That version is stale, so I ended up rebasing it locally myself for the
> purpose of developing this patch and gathering some reviews on the list. In
> other words, the current patch does not apply for the time being, but will
> once a v7 for Joel's series is out.
>
> [0]: https://lore.kernel.org/rust-for-linux/20251003154748.1687160-1-joelagnelf@nvidia.com/
> ---
> drivers/gpu/drm/tyr/driver.rs | 15 ++-
> drivers/gpu/drm/tyr/gpu.rs | 55 ++++----
> drivers/gpu/drm/tyr/regs.rs | 302 ++++++++++++++++++++++++++++++++----------
> 3 files changed, 267 insertions(+), 105 deletions(-)
>
> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> index 0389c558c036..8e06db5320bf 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs
> @@ -66,19 +66,20 @@ unsafe impl Send for TyrData {}
> unsafe impl Sync for TyrData {}
>
> 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)?;
> +
> + regs::GpuCommand::default()
> + .set_command(regs::GPU_CMD_SOFT_RESET)
> + .write(io);
This sets the command but not the payload (although also see below).
>
> // TODO: We cannot poll, as there is no support in Rust currently, so we
> // sleep. Change this when read_poll_timeout() is implemented in Rust.
> kernel::time::delay::fsleep(time::Delta::from_millis(100));
>
> - if regs::GPU_IRQ_RAWSTAT.read(dev, iomem)? & regs::GPU_IRQ_RAWSTAT_RESET_COMPLETED == 0 {
> + let rawstat = regs::GpuIrqRawstat::read(io);
> + if !rawstat.reset_completed() {
> dev_err!(dev, "GPU reset failed with errno\n");
> - dev_err!(
> - dev,
> - "GPU_INT_RAWSTAT is {}\n",
> - regs::GPU_IRQ_RAWSTAT.read(dev, iomem)?
> - );
> + dev_err!(dev, "GPU_INT_RAWSTAT is {}\n", u32::from(rawstat));
>
> return Err(EIO);
> }
> diff --git a/drivers/gpu/drm/tyr/gpu.rs b/drivers/gpu/drm/tyr/gpu.rs
> index 6c582910dd5d..7c698fb1e36a 100644
> --- a/drivers/gpu/drm/tyr/gpu.rs
> +++ b/drivers/gpu/drm/tyr/gpu.rs
> @@ -44,34 +44,36 @@ pub(crate) struct GpuInfo {
>
> impl GpuInfo {
> pub(crate) fn new(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result<Self> {
> - let gpu_id = regs::GPU_ID.read(dev, iomem)?;
> - let csf_id = regs::GPU_CSF_ID.read(dev, iomem)?;
> - let gpu_rev = regs::GPU_REVID.read(dev, iomem)?;
> - let core_features = regs::GPU_CORE_FEATURES.read(dev, iomem)?;
> - let l2_features = regs::GPU_L2_FEATURES.read(dev, iomem)?;
> - let tiler_features = regs::GPU_TILER_FEATURES.read(dev, iomem)?;
> - let mem_features = regs::GPU_MEM_FEATURES.read(dev, iomem)?;
> - let mmu_features = regs::GPU_MMU_FEATURES.read(dev, iomem)?;
> - let thread_features = regs::GPU_THREAD_FEATURES.read(dev, iomem)?;
> - let max_threads = regs::GPU_THREAD_MAX_THREADS.read(dev, iomem)?;
> - let thread_max_workgroup_size = regs::GPU_THREAD_MAX_WORKGROUP_SIZE.read(dev, iomem)?;
> - let thread_max_barrier_size = regs::GPU_THREAD_MAX_BARRIER_SIZE.read(dev, iomem)?;
> - let coherency_features = regs::GPU_COHERENCY_FEATURES.read(dev, iomem)?;
> -
> - let texture_features = regs::GPU_TEXTURE_FEATURES0.read(dev, iomem)?;
> -
> - let as_present = regs::GPU_AS_PRESENT.read(dev, iomem)?;
> -
> - let shader_present = u64::from(regs::GPU_SHADER_PRESENT_LO.read(dev, iomem)?);
> + let io = (*iomem).access(dev)?;
> +
> + let gpu_id = regs::GpuId::read(io).into();
> + let csf_id = regs::CsfId::read(io).into();
> + let gpu_rev = regs::RevIdr::read(io).into();
> + let core_features = regs::CoreFeatures::read(io).into();
> + let l2_features = regs::L2Features::read(io).into();
> + let tiler_features = regs::TilerFeatures::read(io).into();
> + let mem_features = regs::MemFeatures::read(io).into();
> + let mmu_features = regs::MmuFeatures::read(io).into();
> + let thread_features = regs::ThreadFeatures::read(io).into();
> + let max_threads = regs::ThreadMaxThreads::read(io).into();
> + let thread_max_workgroup_size = regs::ThreadMaxWorkgroupSize::read(io).into();
> + let thread_max_barrier_size = regs::ThreadMaxBarrierSize::read(io).into();
> + let coherency_features = regs::CoherencyFeatures::read(io).into();
> +
> + let texture_features = regs::TextureFeatures::read(io, 0).into();
> +
> + let as_present = regs::AsPresent::read(io).into();
> +
> + let shader_present = u64::from(u32::from(regs::ShaderPresentLo::read(io)));
> let shader_present =
> - shader_present | u64::from(regs::GPU_SHADER_PRESENT_HI.read(dev, iomem)?) << 32;
> + shader_present | u64::from(u32::from(regs::ShaderPresentHi::read(io))) << 32;
>
> - let tiler_present = u64::from(regs::GPU_TILER_PRESENT_LO.read(dev, iomem)?);
> + let tiler_present = u64::from(u32::from(regs::TilerPresentLo::read(io)));
> let tiler_present =
> - tiler_present | u64::from(regs::GPU_TILER_PRESENT_HI.read(dev, iomem)?) << 32;
> + tiler_present | u64::from(u32::from(regs::TilerPresentHi::read(io))) << 32;
>
> - let l2_present = u64::from(regs::GPU_L2_PRESENT_LO.read(dev, iomem)?);
> - let l2_present = l2_present | u64::from(regs::GPU_L2_PRESENT_HI.read(dev, iomem)?) << 32;
> + let l2_present = u64::from(u32::from(regs::L2PresentLo::read(io)));
> + let l2_present = l2_present | u64::from(u32::from(regs::L2PresentHi::read(io))) << 32;
>
> Ok(Self {
> gpu_id,
> @@ -204,13 +206,14 @@ fn from(value: u32) -> Self {
>
> /// 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)?;
> + regs::L2PwrOnLo::default().set_l2_pwron_lo(1).write(io);
>
> // TODO: We cannot poll, as there is no support in Rust currently, so we
> // sleep. Change this when read_poll_timeout() is implemented in Rust.
> kernel::time::delay::fsleep(time::Delta::from_millis(100));
>
> - if regs::L2_READY_LO.read(dev, iomem)? != 1 {
> + if regs::L2ReadyLo::read(io).l2_ready_lo() != 1 {
> dev_err!(dev, "Failed to power on the GPU\n");
> return Err(EIO);
> }
> diff --git a/drivers/gpu/drm/tyr/regs.rs b/drivers/gpu/drm/tyr/regs.rs
> index f46933aaa221..a4e05ff463c0 100644
> --- a/drivers/gpu/drm/tyr/regs.rs
> +++ b/drivers/gpu/drm/tyr/regs.rs
> @@ -8,44 +8,62 @@
> #![allow(dead_code)]
>
> use kernel::bits::bit_u32;
> -use kernel::device::Bound;
> -use kernel::device::Device;
> -use kernel::devres::Devres;
> use kernel::prelude::*;
> +use kernel::register;
>
> -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(())
> - }
> -}
> -
> -pub(crate) const GPU_ID: Register<0x0> = Register;
> -pub(crate) const GPU_L2_FEATURES: Register<0x4> = Register;
> -pub(crate) const GPU_CORE_FEATURES: Register<0x8> = Register;
> -pub(crate) const GPU_CSF_ID: Register<0x1c> = Register;
> -pub(crate) const GPU_REVID: Register<0x280> = Register;
> -pub(crate) const GPU_TILER_FEATURES: Register<0xc> = Register;
> -pub(crate) const GPU_MEM_FEATURES: Register<0x10> = Register;
> -pub(crate) const GPU_MMU_FEATURES: Register<0x14> = Register;
> -pub(crate) const GPU_AS_PRESENT: Register<0x18> = Register;
> -pub(crate) const GPU_IRQ_RAWSTAT: Register<0x20> = Register;
> +register!(GpuId @ 0x0, "Information about the GPU architecture and release version" {
> + 3:0 version_status as u32, "Status of the GPU release";
> + 11:4 version_minor as u32, "Minor release version number";
> + 15:12 version_major as u32, "Major release version number";
> + 19:16 product_major as u32, "Product identifier";
> + 23:20 arch_rev as u32, "Architecture patch revision";
> + 27:24 arch_minor as u32, "Architecture minor revision";
> + 31:28 arch_major as u32, "Architecture major revision";
> +});
> +
> +register!(L2Features @ 0x4, "Level 2 cache features" {
> + 7:0 line_size as u32, "L2 cache line size";
> + 15:8 associativity as u32, "L2 cache associativity";
> + 23:16 cache_size as u32, "L2 cache slice size";
> + 31:24 bus_width as u32, "L2 cache bus width";
> +});
> +
> +register!(CoreFeatures @ 0x8, "Information about the features of a shader core" {
> + 7:0 core_variant as u32, "Shader core variant";
> +});
> +
> +register!(CsfId @ 0x1c, "Version of the CSF hardware and MMU subsystem" {
> + 3:0 mcu_rev as u32, "MCU revision ID";
> + 9:4 mcu_minor as u32, "MCU minor revision number";
> + 15:10 mcu_major as u32, "MCU major revision number";
> + 19:16 cshw_rev as u32, "CSHW revision ID";
> + 25:20 cshw_minor as u32, "CSHW minor revision number";
> + 31:26 cshw_major as u32, "CSHW major revision number";
> +});
> +
> +register!(RevIdr @ 0x280, "Extra revision information" {
> + 31:0 revision as u32, "Revision information";
> +});
Not a new thing - but it would be good if these were kept in order of
register address - the above two are out of place.
> +
> +register!(TilerFeatures @ 0xc, "Tiler features" {
> + 5:0 bin_size as u32, "Log of the tiler's bin size";
> + 11:8 max_levels as u32, "Maximum number of available levels";
> +});
> +
> +register!(MemFeatures @ 0x10, "Memory features" {
> + 0:0 coherent_core_group as bool, "Core group is coherent";
> + 1:1 coherent_super_group as bool, "Core supergroup is coherent";
> + 11:8 l2_slices as u32, "L2 slice count";
> +});
> +
> +register!(MmuFeatures @ 0x14, "MMU features" {
> + 7:0 va_bits as u32, "Number of bits supported in virtual addresses";
> + 15:8 pa_bits as u32, "Number of bits supported in physical addresses";
> +});
> +
> +register!(AsPresent @ 0x18, "Address spaces present" {
> + 31:0 as_present as u32, "Bitmask of present address spaces";
> +});
>
> pub(crate) const GPU_IRQ_RAWSTAT_FAULT: u32 = bit_u32(0);
> pub(crate) const GPU_IRQ_RAWSTAT_PROTECTED_FAULT: u32 = bit_u32(1);
> @@ -56,53 +74,193 @@ pub(crate) fn write(&self, dev: &Device<Bound>, iomem: &Devres<IoMem>, value: u3
> pub(crate) const GPU_IRQ_RAWSTAT_DOORBELL_STATUS: u32 = bit_u32(18);
> pub(crate) const GPU_IRQ_RAWSTAT_MCU_STATUS: u32 = bit_u32(19);
>
> -pub(crate) const GPU_IRQ_CLEAR: Register<0x24> = Register;
> -pub(crate) const GPU_IRQ_MASK: Register<0x28> = Register;
> -pub(crate) const GPU_IRQ_STAT: Register<0x2c> = Register;
> -pub(crate) const GPU_CMD: Register<0x30> = Register;
> +register!(GpuIrqRawstat @ 0x20, "Raw unmasked interrupt status for the GPU" {
> + 0:0 fault as bool, "A GPU fault has occourred";
> + 1:1 protected_fault as bool, "Indicates a protected memory fault has occurred";
> + 8:8 reset_completed as bool, "Indicates that a GPU reset has completed";
> + 9:9 power_changed_single as bool, "Set when a single power domain has powered up or down";
> + 10:10 power_changed_all as bool, "Set when all pending power domain changes are completed ";
NIT: Stray space at the end of the description.
> + 17:17 clean_caches_completed as bool, "Indicates that a cache clean operation has completed";
> + 18:18 doorbell_status as bool, "Mirrors the doorbell interrupt line to the CPU";
> + 19:19 mcu_status as bool, "The MCU requires attention";
> +});
> +
> +register!(GpuIrqClear @ 0x24, "Clears pending GPU interrupts" {
> + 0:0 fault as bool, "Clear the fault interrupt";
> + 1:1 protected_fault as bool, "Clear the protected_fault interrupt";
> + 8:8 reset_completed as bool, "Clear the reset_completed interrupt";
> + 9:9 power_changed_single as bool, "Clear the power_changed_single interrupt";
> + 10:10 power_changed_all as bool, "Clear the power_changed_all interrupt";
> + 17:17 clean_caches_completed as bool, "Clear the clean_caches_completed interrupt";
> + 18:18 doorbell_status as bool, "Clear the doorbell_status interrupt";
NIT: doorbell_status (or "DOORBELL_MIRROR" in my copy of the spec)
cannot be cleared through this register.
> + 19:19 mcu_status as bool, "Clear the mcu_status interrupt";
> +});
> +
> +register!(GpuIrqMask @ 0x28, "Enabled GPU interrupts" {
> + 0:0 fault as bool, "Enable the fault interrupt";
> + 1:1 protected_fault as bool, "Enable the protected_fault interrupt";
> + 8:8 reset_completed as bool, "Enable the reset_completed interrupt";
> + 9:9 power_changed_single as bool, "Enable the power_changed_single interrupt";
> + 10:10 power_changed_all as bool, "Enable the power_changed_all interrupt";
> + 17:17 clean_caches_completed as bool, "Enable the clean_caches_completed interrupt";
> + 18:18 doorbell_status as bool, "Enable the doorbell_status interrupt";
> + 19:19 mcu_status as bool, "Enable the mcu_status interrupt";
> +});
> +
> +register!(GpuIrqStatus @ 0x2c, "Masked GPU interrupt status" {
> + 0:0 fault as bool, "The fault interrupt is pending";
> + 1:1 protected_fault as bool, "The protected_fault interrupt is pending";
> + 8:8 reset_completed as bool, "The reset_completed interrupt is pending";
> + 9:9 power_changed_single as bool, "The power_changed_single interrupt is pending";
> + 10:10 power_changed_all as bool, "The power_changed_all interrupt is pending";
> + 17:17 clean_caches_completed as bool, "The clean_caches_completed interrupt is pending";
> + 18:18 doorbell_status as bool, "The doorbell_status interrupt is pending";
> + 19:19 mcu_status as bool, "The mcu_status interrupt is pending";
> +});
> +
> pub(crate) const GPU_CMD_SOFT_RESET: u32 = 1 | (1 << 8);
> pub(crate) const GPU_CMD_HARD_RESET: u32 = 1 | (2 << 8);
This is a combination of the GPU command and the payload. Since you've
(below) described these fields separately I don't think this works. I
presume in the call above the high part of the value is getting lost
(although I don't actually understand how Rust handles this).
> -pub(crate) const GPU_THREAD_FEATURES: Register<0xac> = Register;
> -pub(crate) const GPU_THREAD_MAX_THREADS: Register<0xa0> = Register;
> -pub(crate) const GPU_THREAD_MAX_WORKGROUP_SIZE: Register<0xa4> = Register;
> -pub(crate) const GPU_THREAD_MAX_BARRIER_SIZE: Register<0xa8> = Register;
> -pub(crate) const GPU_TEXTURE_FEATURES0: Register<0xb0> = Register;
> -pub(crate) const GPU_SHADER_PRESENT_LO: Register<0x100> = Register;
> -pub(crate) const GPU_SHADER_PRESENT_HI: Register<0x104> = Register;
> -pub(crate) const GPU_TILER_PRESENT_LO: Register<0x110> = Register;
> -pub(crate) const GPU_TILER_PRESENT_HI: Register<0x114> = Register;
> -pub(crate) const GPU_L2_PRESENT_LO: Register<0x120> = Register;
> -pub(crate) const GPU_L2_PRESENT_HI: Register<0x124> = Register;
> -pub(crate) const L2_READY_LO: Register<0x160> = Register;
> -pub(crate) const L2_READY_HI: Register<0x164> = Register;
> -pub(crate) const L2_PWRON_LO: Register<0x1a0> = Register;
> -pub(crate) const L2_PWRON_HI: Register<0x1a4> = Register;
> -pub(crate) const L2_PWRTRANS_LO: Register<0x220> = Register;
> -pub(crate) const L2_PWRTRANS_HI: Register<0x204> = Register;
> -pub(crate) const L2_PWRACTIVE_LO: Register<0x260> = Register;
> -pub(crate) const L2_PWRACTIVE_HI: Register<0x264> = Register;
> -
> -pub(crate) const MCU_CONTROL: Register<0x700> = Register;
> +
> +register!(GpuCommand @ 0x30, "GPU command register" {
> + 7:0 command as u32, "GPU-specific command to execute";
> + 31:8 payload as u32, "Payload for the command";
> +});
> +
> +register!(ThreadFeatures @ 0xac, "Thread features of the GPU's threading system" {
> + 21:0 max_registers as u32, "Total number of registers per core";
> + 23:22 implementation_technology as u32;
> + 31:24 max_task_queue as u32, "Maximum number of compute tasks waiting";
> +
> +});
> +
> +register!(ThreadMaxThreads @ 0xa0, "Maximum number of threads per core" {
> + 31:0 max_threads as u32, "Maximum number of threads per core";
> +});
> +
> +register!(ThreadMaxWorkgroupSize @ 0xa4, "Maximum number of threads per workgroup" {
> + 31:0 max_workgroup_size as u32, "Maximum number of threads per workgroup";
> +});
> +
> +register!(ThreadMaxBarrierSize @ 0xa8, "Maximum number of threads per barrier" {
> + 31:0 max_barrier_size as u32, "Maximum number of threads per barrier";
> +});
> +
> +register!(TextureFeatures @ 0xb0 [4], "Bitmap of supported texture formats" {});
> +
> +register!(ShaderPresentLo @ 0x100, "Bitmap of shader cores present in the hardware (lower 32 bits)" {
> + 31:0 shader_present_lo as u32, "Bitmap of shader cores present in the hardware (lower 32 bits)";
> +});
> +
> +register!(ShaderPresentHi @ 0x104, "Bitmap of shader cores present in the hardware (higher 32 bits)" {
> + 31:0 shader_present_hi as u32, "Bitmap of shader cores present in the hardware (higher 32 bits)";
> +});
> +
> +register!(TilerPresentLo @ 0x110, "Bitmap of tiler cores present in the hardware (lower 32 bits)" {
> + 31:0 tiler_present_lo as u32, "Bitmap of tiler cores present in the hardware (lower 32 bits)";
> +});
> +
> +register!(TilerPresentHi @ 0x114, "Bitmap of tiler cores present in the hardware (higher 32 bits)" {
> + 31:0 tiler_present_hi as u32, "Bitmap of tiler cores present in the hardware (higher 32 bits)";
> +});
> +
> +register!(L2PresentLo @ 0x120, "Bitmap of L2 caches present in the hardware (lower 32 bits)" {
> + 31:0 l2_present_lo as u32, "Bitmap of L2 caches present in the hardware (lower 32 bits)";
> +});
> +
> +register!(L2PresentHi @ 0x124, "Bitmap of L2 caches present in the hardware (higher 32 bits)" {
> + 31:0 l2_present_hi as u32, "Bitmap of L2 caches present in the hardware (higher 32 bits)";
> +});
> +
> +register!(L2ReadyLo @ 0x160, "Bitmap of L2 caches ready (lower 32 bits)" {
> + 31:0 l2_ready_lo as u32, "Bitmap of L2 caches ready (lower 32 bits)";
> +});
> +
> +register!(L2ReadyHi @ 0x164, "Bitmap of L2 caches ready (higher 32 bits)" {
> + 31:0 l2_ready_hi as u32, "Bitmap of L2 caches ready (higher 32 bits)";
> +});
> +
> +register!(L2PwrOnLo @ 0x1a0, "Bitmap of L2 caches power on requests (lower 32 bits)" {
> + 31:0 l2_pwron_lo as u32, "Bitmap of L2 caches power on requests (lower 32 bits)";
> +});
> +
> +register!(L2PwrOnHi @ 0x1a4, "Bitmap of L2 caches power on requests (higher 32 bits)" {
> + 31:0 l2_pwron_hi as u32, "Bitmap of L2 caches power on requests (higher 32 bits)";
> +});
> +
> +register!(L2PwrTransLo @ 0x200, "Bitmap of L2 caches in power transition (lower 32 bits)" {
> + 31:0 l2_pwrtrans_lo as u32, "Bitmap of L2 caches in power transition (lower 32 bits)";
> +});
> +
> +register!(L2PwrTransHi @ 0x204, "Bitmap of L2 caches in power transition (higher 32 bits)" {
> + 31:0 l2_pwrtrans_hi as u32, "Bitmap of L2 caches in power transition (higher 32 bits)";
> +});
> +
> +register!(L2PowerActiveLo @ 0x260, "Bitmap of L2 caches active (lower 32 bits)" {
> + 31:0 l2_pwractive_lo as u32, "Bitmap of L2 caches active (lower 32 bits)";
> +});
> +
> +register!(L2PowerActiveHi @ 0x264, "Bitmap of L2 caches active (higher 32 bits)" {
> + 31:0 l2_pwractive_hi as u32, "Bitmap of L2 caches active (higher 32 bits)";
> +});
> +
> pub(crate) const MCU_CONTROL_ENABLE: u32 = 1;
> pub(crate) const MCU_CONTROL_AUTO: u32 = 2;
> pub(crate) const MCU_CONTROL_DISABLE: u32 = 0;
>
> -pub(crate) const MCU_STATUS: Register<0x704> = Register;
> +register!(McuControl @ 0x700, "Controls the execution state of the MCU subsystem" {
> + 1:0 req as u32, "Request state change";
> +});
> +
> pub(crate) const MCU_STATUS_DISABLED: u32 = 0;
> pub(crate) const MCU_STATUS_ENABLED: u32 = 1;
> pub(crate) const MCU_STATUS_HALT: u32 = 2;
> pub(crate) const MCU_STATUS_FATAL: u32 = 3;
>
> -pub(crate) const GPU_COHERENCY_FEATURES: Register<0x300> = Register;
> +register!(McuStatus @ 0x704, "Reports the current execution state of the MCU subsystem" {
> + 1:0 status as u32, "Current MCU status";
> +});
> +
> +register!(CoherencyFeatures @ 0x300, "GPU coherency features" {
> + 0:0 ace_lite as bool, "ACE-Lite protocol supported";
> + 1:1 ace as bool, "ACE protocol supported";
> +});
> +
> +register!(JobIrqRawstat @ 0x1000, "Raw unmasked interrupt status for firmware interrupts" {
> + 30:0 csg as u32, "CSG request";
> + 31:31 glb as bool, "GLB request";
> +});
> +
> +register!(JobIrqClear @ 0x1004, "Clears pending firmware interrupts" {
> + 30:0 csg as u32, "Clear CSG requests";
> + 31:31 glb as bool, "Clear GLB request";
> +});
> +
> +register!(JobIrqMask @ 0x1008, "Enabled firmware interrupts" {
> + 30:0 csg as u32, "Enable CSG requests";
> + 31:31 glb as bool, "Enable GLB request";
> +});
> +
> +register!(JobIrqStatus @ 0x100c, "Masked firmware interrupt status" {
> + 30:0 csg as u32, "Pending CSG requests";
> + 31:31 glb as bool, "Pending GLB request";
> +});
> +
> +register!(MmuIrqRawstat @ 0x2000, "Raw unmasked interrupt status for MMU interrupts" {
> + 15:0 page_fault as u32, "Bitmask indicating which address spaces page-faulted";
> + 31:31 command_completed as bool, "Bitmask indicating whether a command completed in a given AS";
This should be 16:31 and be a u32 - the description is correct this is a
bitmask.
> +});
>
> -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;
> +register!(MmuIrqClear @ 0x2004, "Clears pending MMU interrupts" {
> + 15:0 page_fault as u32, "Clear page-fault interrupts for the given address spaces";
> + 31:31 command_completed as bool, "Clear command-completed interrupt for the given address spaces";
Also 16:31.
> +});
>
> -pub(crate) const JOB_IRQ_GLOBAL_IF: u32 = bit_u32(31);
> +register!(MmuIrqMask @ 0x2008, "Enabled MMU interrupts" {
> + 15:0 page_fault as u32, "Enable page-fault interrupts for the given address spaces";
> + 31:31 command_completed as bool, "Enable command-completed interrupt for the given address spaces";
Also 16: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;
> +register!(MmuIrqStatus @ 0x200c, "Masked MMU interrupt status" {
> + 15:0 page_fault as u32, "Pending page-fault interrupts for the given address spaces";
> + 31:31 command_completed as bool, "Pending command-completed interrupt for the given address spaces";
Also 16:31.
Thanks,
Steve
> +});
>
> ---
> base-commit: f10c325a345fef0a688a2bcdfab1540d1c924148
> change-id: 20260108-tyr-register-ea913f8e2330
> prerequisite-message-id: <20251003154748.1687160-1-joelagnelf@nvidia.com>
> prerequisite-patch-id: 027ea340650912c31c3b3e2e2ba60f390b449218
>
> Best regards,
© 2016 - 2026 Red Hat, Inc.