Claude review: accel/amdxdna: Add AIE4 firmware loading

Claude Code Review Bot
2026-03-31T07:05:09Z

Patch Review

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

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

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

**Issue 3 — 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 to the same physical address `MPASP_C2PMSG_123_ALT_1`. Similarly `PSP_ARG0_REG` 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 — `virt_to_phys` usage:**
```c
*paddr = 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 it's worth noting that this won't work on systems with IOMMU remapping if the PSP can't bypass it. This appears to be an existing design choice.

**Issue 5 — 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 to use them after `aiem_psp_create`, but storing dangling pointers is a latent bug.

---
Generated by Claude Code Patch Reviewer