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: Fri, 13 Mar 2026 14:38:51 +1000
Message-ID: <review-patch1-20260311-b4-tyr-use-register-macro-v2-v2-1-b936d9eb8f51@collabora.com>
In-Reply-To: <20260311-b4-tyr-use-register-macro-v2-v2-1-b936d9eb8f51@collabora.com>
References: <20260311-b4-tyr-use-register-macro-v2-v2-0-b936d9eb8f51@collabora.com> <20260311-b4-tyr-use-register-macro-v2-v2-1-b936d9eb8f51@collabora.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

This is the largest and most important patch. It replaces all GPU_CONTROL r=
egister definitions with `register!` macro invocations, moves them into a `=
gpu_control` module, and updates all callers in `driver.rs` and `gpu.rs`.

**Critical: GPU_COMMAND_RESET missing command field**

The old code wrote:
```c
pub(crate) const GPU_CMD_SOFT_RESET: u32 =3D 1 | (1 << 8);
```

This sets both the command type (bits 7:0 =3D 1 =3D RESET) and the reset_mo=
de (bits 11:8 =3D 1 =3D SOFT_RESET), producing value `0x101`. The C panthor=
 driver confirms this: `GPU_CMD_DEF(type, payload) ((type) | ((payload) << =
8))`.

The new code in `driver.rs`:
```rust
GPU_COMMAND_RESET::zeroed().with_const_reset_mode::<{ GPU_COMMAND_RESET::SO=
FT_RESET }>()
```

This only sets `reset_mode` (bits 11:8) to 1, leaving the `command` field (=
bits 7:0) as 0 (which is `GPU_COMMAND::NOP`). This would write `0x100` inst=
ead of `0x101`, meaning the hardware would not interpret this as a reset co=
mmand. This will break GPU initialization. The fix should be:
```rust
GPU_COMMAND_RESET::zeroed()
    .with_const_command::<{ GPU_COMMAND::RESET }>()
    .with_const_reset_mode::<{ GPU_COMMAND_RESET::SOFT_RESET }>()
```

**Log output format change in `gpu_info_log`**

The old code printed:
```rust
self.gpu_id >> 16
```
which produces a 16-bit value like `0xb0a0` (matching the C panthor driver'=
s `ptdev->gpu_info.gpu_id >> 16`).

The new code constructs a canonical ID and prints the full 32-bit value:
```rust
let id =3D GPU_ID::zeroed()
    .with_arch_major(gpu_id.arch_major())
    .with_arch_minor(gpu_id.arch_minor())
    .with_arch_rev(gpu_id.arch_rev())
    .with_prod_major(gpu_id.prod_major())
    .into_raw();
```
This would print `0xb0a00000` instead of `0xb0a0`. This changes the user-vi=
sible log format compared to the C panthor driver, which could confuse deve=
lopers. Consider either keeping the `>> 16` shift or noting this intentiona=
l change in the commit message.

**`gpu_info_log` now reads registers directly instead of using the already-=
fetched GpuInfo**

The old `log()` method used `self.gpu_id`, `self.l2_features`, etc. from th=
e already-populated struct. The new `gpu_info_log()` function re-reads all =
registers from hardware via `iomem`. This is a minor inefficiency and a sem=
antic change =E2=80=94 the log now reflects the live register state rather =
than the cached `GpuInfo` values. It also means `gpu_info_log` can fail (re=
turns `Result`), whereas the old method was infallible.

**`va_bits()` and `pa_bits()` removed**

These methods on `GpuInfo` were removed. They were marked `#[expect(dead_co=
de)]` so this is fine, but it's a minor loss of API surface that could be u=
seful for future MMU setup. The new `MMU_FEATURES` register definition prov=
ides `va_bits` and `pa_bits` fields, so the information is still accessible.

**Documentation additions are good.** The glossary at the top of `regs.rs` =
(CEU, CS, CSF, CSG, etc.) is a nice addition.

**Incomplete COHERENCY_ENABLE doc comment:**
```rust
/// Possible values are in
/// This register only selects the protocol...
```
The sentence "Possible values are in" is incomplete =E2=80=94 it appears a =
reference (e.g., to the `impl` block constants) was accidentally omitted.

---
Generated by Claude Code Patch Reviewer