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 MMU_CONTROL
Date: Sun, 12 Apr 2026 10:45:09 +1000
Message-ID: <review-patch4-20260409-b4-tyr-use-register-macro-v5-v5-4-8abfff8a0204@collabora.com>
In-Reply-To: <20260409-b4-tyr-use-register-macro-v5-v5-4-8abfff8a0204@collabora.com>
References: <20260409-b4-tyr-use-register-macro-v5-v5-0-8abfff8a0204@collabora.com> <20260409-b4-tyr-use-register-macro-v5-v5-4-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

The largest register definition patch. Defines the full MMU_CONTROL page including per-address space register arrays.

**Array register stride:**
```rust
const STRIDE: usize = 0x40;
// ...
pub(crate) TRANSTAB(u64)[MAX_AS, stride = STRIDE] @ 0x2400 {
```
The 0x40 (64 byte) stride per address space is correct per the panthor hardware layout.

**MMU_MEMATTR_STAGE1 pseudo-register:**
```rust
MMU_MEMATTR_STAGE1(u8) @ 0x0 {
    0:0     alloc_w => bool;
    1:1     alloc_r => bool;
    3:2     alloc_sel ?=> AllocPolicySelect;
    5:4     coherency ?=> Coherency;
    7:6     memory_type => MemoryType;
}
```
This is defined at offset 0x0 because it's not a real MMIO register -- it's a sub-byte bitfield type used within the 64-bit `MEMATTR` register. The comment above the definition explains this clearly. The `TryFrom<Bounded<u64, 8>>` and `From<MMU_MEMATTR_STAGE1>` for `Bounded<u64, 8>` impls bridge it into the MEMATTR field types. This is a creative and clean use of the macro.

**MmuExceptionType enum:**
Comprehensive list of fault codes. The `TryFrom` match covers all defined values and returns `EINVAL` for unknown ones, which is correct since undefined exception codes from hardware should be treated as errors.

**TRANSCFG register -- bit range gap:**
```rust
pub(crate) TRANSCFG(u64)[MAX_AS, stride = STRIDE] @ 0x2430 {
    3:0     mode ?=> AddressSpaceMode;
    10:6    ina_bits ?=> InaBits;
    18:14   outa_bits;
    22:22   sl_concat_en => bool;
    25:24   ptw_memattr ?=> PtwMemattr;
    29:28   ptw_sh ?=> PtwShareability;
    30:30   r_allocate => bool;
    33:33   disable_hier_ap => bool;
    34:34   disable_af_fault => bool;
    35:35   wxn => bool;
    36:36   xreadable => bool;
    63:60   ptw_pbha;
}
```
There are gaps (bits 4-5, 11-13, 19-21, 23, 26-27, 31-32, 37-59) which are reserved/unused per the spec. The `register!` macro should handle these gaps correctly by leaving them as zero. No issues.

**`PtwShareability` -- missing value 1:**
Values are 0 (NonShareable), 2 (OuterShareable), 3 (InnerShareable). Value 1 is not defined and returns `EINVAL`. This matches the ARM shareability encoding where value 1 is reserved.

**`#[allow(clippy::enum_variant_names)]` on `PtwShareability`:**
All variants end in "Shareable" -- the clippy suppression is appropriate here since the names are descriptive.

No issues with this patch.

---
Generated by Claude Code Patch Reviewer