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 <[email protected]> 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 <[email protected]>
---
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/[email protected]/
---
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
