Claude review: accel/amdxdna: Read real-time clock frequencies

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**1. Potential field-name swap in `npu4_update_counters` (npu4_regs.c) — needs clarification**

```c
ndev->npuclk_freq = npu_metrics.mpnpuclk_freq;
ndev->hclk_freq = npu_metrics.npuclk_freq;
```

From `include/linux/amd-pmf-io.h`, the PMF struct documents:
- `npuclk_freq` — "NPU clock frequency [MHz]"
- `mpnpuclk_freq` — "MPNPU [MHz]"

In the driver (`npu4_set_dpm`), the existing mapping is:
```c
ndev->npuclk_freq = ndev->priv->dpm_clk_tbl[dpm_level].npuclk;  // MP-NPU clock
ndev->hclk_freq = ndev->priv->dpm_clk_tbl[dpm_level].hclk;      // H clock
```

So this maps PMF's `mpnpuclk_freq` → driver's `npuclk_freq` (MP-NPU), and PMF's `npuclk_freq` → driver's `hclk_freq` (H clock). The cross-wiring of names is extremely confusing. Even if this mapping is hardware-correct, a comment explaining why `npu_metrics.npuclk_freq` maps to `ndev->hclk_freq` (and not `npuclk_freq`) would prevent future maintenance mistakes.

**2. `aie2_update_counters` macro silently discards errors (aie2_pci.h)**

```c
#define aie2_update_counters(ndev)                  \
({                                                  \
    typeof(ndev) _ndev = ndev;                      \
    if (_ndev->priv->hw_ops->update_counters)       \
        _ndev->priv->hw_ops->update_counters(_ndev);\
})
```

This macro has two problems:

- The return value of `update_counters()` is silently discarded. If PMF is unavailable (`CONFIG_AMD_PMF=n`), `AIE2_GET_PMF_NPU_METRICS` returns `-EOPNOTSUPP`, and the caller proceeds to report stale/previous frequency data to userspace with no indication it's stale.
- The macro itself is a statement expression but its value is the result of the `if` statement, which is `void`. Callers cannot check for errors even if they wanted to.

At minimum, consider making this return `int` and propagating the error, or at least adding `dev_dbg` on failure so the silent fallback to stale data is discoverable during debugging:

```c
#define aie2_update_counters(ndev)                          \
({                                                          \
    typeof(ndev) _ndev = ndev;                              \
    int _ret = 0;                                           \
    if (_ndev->priv->hw_ops->update_counters)               \
        _ret = _ndev->priv->hw_ops->update_counters(_ndev); \
    _ret;                                                   \
})
```

**3. `npu4_update_counters` doesn't update `max_tops` (npu4_regs.c)**

Compare with `npu4_set_dpm`:
```c
ndev->max_tops = NPU4_DPM_TOPS(ndev, ndev->priv->dpm_clk_tbl[ndev->max_dpm_level].hclk);
ndev->curr_tops = NPU4_DPM_TOPS(ndev, ndev->hclk_freq);
```

But `npu4_update_counters` only updates `curr_tops`:
```c
ndev->curr_tops = NPU4_DPM_TOPS(ndev, ndev->hclk_freq);
```

This is probably intentional (max TOPS shouldn't change at runtime), but `aie2_query_resource_info` reads `ndev->max_tops` right after calling `aie2_update_counters`. If this is deliberate, a brief comment explaining that `max_tops` is set once in `set_dpm` and doesn't need runtime refresh would help reviewers.

**4. No locking around field updates in `npu4_update_counters`**

`npu4_update_counters` modifies `ndev->npuclk_freq`, `ndev->hclk_freq`, and `ndev->curr_tops` without any visible locking. These same fields are also written by `npu4_set_dpm` (called via `aie2_pm_set_dpm` from ioctl paths). If a DPM-level change races with a clock metadata query, the reported values could be inconsistent (e.g., `npuclk_freq` from PMF but `curr_tops` computed from a DPM-table hclk). This may be an existing issue, but the new call sites in `aie2_get_clock_metadata` and `aie2_query_resource_info` widen the race window.

**5. Compound literal vs named constant inconsistency (npu1_regs.c)**

```c
.hw_ops = &(const struct aie2_hw_ops) {
    .set_dpm = npu1_set_dpm,
},
```

NPU1 uses an anonymous compound literal while NPU4/5/6 use the named `npu4_hw_ops`. For consistency and debuggability, consider defining a `npu1_hw_ops` named constant as well.

**6. Missing trailing commas (npu4_regs.c, npu5_regs.c, npu6_regs.c)**

```c
.hw_ops = &npu4_hw_ops
```

The kernel coding style convention is to include trailing commas after the last struct member initializer, to reduce future diff noise. The original code had them.

**7. NPU5 and NPU6 inherit `update_counters` (npu5_regs.c, npu6_regs.c)**

By pointing `.hw_ops = &npu4_hw_ops`, npu5 and npu6 now also get `npu4_update_counters`. This means PMF metrics queries will be made for these NPU generations too. Is the PMF interface guaranteed to work the same way across NPU4/5/6? The commit message doesn't address this.

**8. Patch should be split**

This patch bundles a mechanical refactor (struct → pointer, making `npu4_set_dpm` static, sharing `npu4_hw_ops`) with a functional change (new `update_counters` callback and its call sites). Kernel convention is to separate these:
- Patch 1: Refactor `hw_ops` from embedded struct to const pointer
- Patch 2: Add `update_counters` callback and PMF integration

This aids bisection if the PMF integration causes issues.

**9. `NPU4_DPM_TOPS` macro simplification is correct (npu4_regs.c)**

The old macro:
```c
#define NPU4_DPM_TOPS(ndev, dpm_level) \
({ \
    typeof(ndev) _ndev = ndev; \
    (4096 * (_ndev)->total_col * \
     (_ndev)->priv->dpm_clk_tbl[dpm_level].hclk / 1000000); \
})
```

The new macro:
```c
#define NPU4_DPM_TOPS(ndev, hclk) (4096 * (ndev)->total_col * (hclk) / 1000000)
```

This is a good simplification. Taking the raw hclk value as input rather than a DPM level index makes the macro more reusable (needed for `update_counters` which gets hclk from PMF, not from the DPM table). The removal of `typeof` guard is safe since `ndev` is only dereferenced once and all callers pass a simple pointer.

---
Generated by Claude Code Patch Reviewer