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: accel/amdxdna: Add AIE4 firmware loading
Date: Tue, 31 Mar 2026 17:05:09 +1000
Message-ID: <review-patch4-20260330163705.3153647-5-lizhi.hou@amd.com>
In-Reply-To: <20260330163705.3153647-5-lizhi.hou@amd.com>
References: <20260330163705.3153647-1-lizhi.hou@amd.com> <20260330163705.3153647-5-lizhi.hou@amd.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

Adds CERT firmware support to PSP, AIE4 firmware loading paths, and NPU3 PS=
P register definitions.

**Issue 1 =E2=80=94 Bisection fix for patch 3:**
```c
+	psp_conf.arg2_mask =3D GENMASK(23, 0);
+	psp_conf.notify_val =3D 1;
```
This is added in `aie2_pci.c`, fixing the bisection issue from patch 3. The=
se two lines should have been in patch 3.

**Issue 2 =E2=80=94 `PSP_SET_CMD` macro readability:**
```c
#define PSP_SET_CMD(psp, reg_vals, cmd, arg0, arg1, arg2)       \
({                                                              \
    ...
    _regs[3] =3D ((arg2) | ((_cmd) << 24)) & (psp)->conf.arg2_mask;  \
})
```
The `arg2` register now encodes both `arg2` data AND the command shifted le=
ft by 24 bits, masked by a per-device mask. For AIE2, `arg2_mask =3D GENMAS=
K(23, 0)` strips the command bits (so `arg2` register =3D just the size, ba=
ckwards compatible). For AIE4, `arg2_mask =3D ~0` keeps command bits. This =
is clever but should be documented in a comment explaining the protocol dif=
ference.

**Issue 3 =E2=80=94 PSP register aliasing for NPU3:**
```c
DEFINE_BAR_OFFSET(PSP_CMD_REG,    NPU3_PSP, MPASP_C2PMSG_123_ALT_1),
...
DEFINE_BAR_OFFSET(PSP_ARG2_REG,   NPU3_PSP, MPASP_C2PMSG_123_ALT_1),
...
DEFINE_BAR_OFFSET(PSP_STATUS_REG, NPU3_PSP, MPASP_C2PMSG_123_ALT_1),
```
Three registers (`PSP_CMD_REG`, `PSP_ARG2_REG`, `PSP_STATUS_REG`) all map t=
o the same physical address `MPASP_C2PMSG_123_ALT_1`. Similarly `PSP_ARG0_R=
EG` and `PSP_RESP_REG` share `MPASP_C2PMSG_156_ALT_1`. This means writes to=
 CMD, ARG2, and reads from STATUS all hit the same MMIO register. This is a=
 fundamental protocol difference from AIE2 and should be documented, as it =
means the register serves different purposes depending on direction (write =
for CMD/ARG2, read for STATUS).

**Issue 4 =E2=80=94 `virt_to_phys` usage:**
```c
*paddr =3D virt_to_phys(buffer);
```
`virt_to_phys` on `kmalloc`/`drmm_kmalloc` memory is technically valid but =
fragile. The existing AIE2 code already does this, so this isn't new, but i=
t's worth noting that this won't work on systems with IOMMU remapping if th=
e PSP can't bypass it. This appears to be an existing design choice.

**Issue 5 =E2=80=94 psp_conf stored by value:**
```c
memcpy(&psp->conf, conf, sizeof(psp->conf));
```
`psp->conf` stores `fw_buf` and `certfw_buf` pointers. After `aie4_release_=
firmware()` is called, these pointers become dangling. This is fine if the =
firmware data was already copied into the aligned buffers, but the pointers=
 in `conf` should not be accessed after this point. The code doesn't seem t=
o use them after `aiem_psp_create`, but storing dangling pointers is a late=
nt bug.

---
Generated by Claude Code Patch Reviewer