From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/tyr: Use register! macro for GPU_CONTROL
Date: Sun, 12 Apr 2026 10:45:08 +1000
Message-ID: <review-patch1-20260409-b4-tyr-use-register-macro-v5-v5-1-8abfff8a0204@collabora.com>
In-Reply-To: <20260409-b4-tyr-use-register-macro-v5-v5-1-8abfff8a0204@collabora.com>
References: <20260409-b4-tyr-use-register-macro-v5-v5-0-8abfff8a0204@collabora.com> <20260409-b4-tyr-use-register-macro-v5-v5-1-8abfff8a0204@collabora.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

This is the largest and most important patch. It defines the entire GPU_CONTROL register page with the `register!` macro, converts all users (`issue_soft_reset`, `l2_power_on`, `GpuInfo::new`, `GpuInfo::log`), adds helper types, and removes the old `GpuId` struct.

**Register offset fix (silent bug fix):**
The old code had:
```rust
pub(crate) const L2_PWRTRANS_HI: Register<0x204> = Register;
```
The new code correctly defines:
```rust
pub(crate) L2_PWRTRANS_HI(u32) @ 0x224 {
```
Offset 0x204 is actually `SHADER_PWRTRANS_HI` (confirmed by panthor's `SHADER_PWRTRANS` at 0x200). This was a copy-paste bug in the original tyr code. This fix should be explicitly mentioned in the commit message rather than being hidden in a large refactor.

**`read_u64_no_tearing` helper:**
```rust
pub(crate) fn read_u64_no_tearing(lo_read: impl Fn() -> u32, hi_read: impl Fn() -> u32) -> u64 {
    loop {
        let hi1 = hi_read();
        let lo = lo_read();
        let hi2 = hi_read();
        if hi1 == hi2 {
            return join_u64(lo, hi1);
        }
    }
}
```
This is a standard pattern for tear-free 64-bit reads from split registers. The potential infinite loop is fine in practice -- the high half of hardware counters doesn't change faster than software can read it. However, this function is not used anywhere in the series. It's harmless under `#![allow(dead_code)]`, but could be deferred to the patch that actually needs it.

**GPU_COMMAND aliasing pattern:**
```rust
GPU_COMMAND (u32) @ 0x30 {
    7:0     command ?=> GpuCommand;
}
GPU_COMMAND_RESET (u32) => GPU_COMMAND {
    7:0     command ?=> GpuCommand;
    11:8    reset_mode ?=> ResetMode;
}
GPU_COMMAND_FLUSH (u32) => GPU_COMMAND {
    7:0     command ?=> GpuCommand;
    11:8    l2_flush ?=> FlushMode;
    15:12   lsc_flush ?=> FlushMode;
    19:16   other_flush ?=> FlushMode;
}
```
This is a clever approach to handle the GPU_COMMAND register's multiple layouts. The aliases share the same MMIO offset via `=> GPU_COMMAND` and the constructor methods on `GPU_COMMAND` (`reset()`, `flush_caches()`, `clear_fault()`) provide a clean API. Well designed.

**Texture features -- fixing a TODO:**
```rust
texture_features: [
    io.read(TEXTURE_FEATURES::at(0)).supported_formats().get(),
    io.read(TEXTURE_FEATURES::at(1)).supported_formats().get(),
    io.read(TEXTURE_FEATURES::at(2)).supported_formats().get(),
    io.read(TEXTURE_FEATURES::at(3)).supported_formats().get(),
],
```
The old code only read `TEXTURE_FEATURES0` and hardcoded `[texture_features, 0, 0, 0]` with a `// TODO: Add texture_features_{1,2,3}` comment. Now all four are read from hardware. Good improvement.

**Minor nit -- comment formatting:**
```rust
//GPU_FEATURES register is not available; it was introduced in arch 11.x.
```
Missing space after `//`. Should be `// GPU_FEATURES register...`.

**Signature change of `GpuInfo::log`:**
```rust
-    pub(crate) fn log(&self, pdev: &platform::Device) {
+    pub(crate) fn log(&self, dev: &Device<Bound>) {
```
And the caller is updated:
```rust
-        gpu_info.log(pdev);
+        gpu_info.log(pdev.as_ref());
```
This is a reasonable change since `log()` only needs `dev_info!` which works with `Device<Bound>`.

**Removed `va_bits()` and `pa_bits()` methods:**
These were marked `#[expect(dead_code)]` and are now redundant since `MMU_FEATURES` has typed `va_bits` and `pa_bits` fields. Clean removal.

---
Generated by Claude Code Patch Reviewer