Claude review: Re: [PATCH v2 1/4] accel: rocket: Add support for Rockchip RK3568

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

Patch Review

**1. Runtime PM gap — `noc_init` not called on resume (significant)**

The `noc_init` callback runs once during `rocket_core_init()`, after `pm_runtime_resume_and_get()`. But `rocket_device_runtime_resume()` only re-enables clocks — it does not re-run the NOC de-idle sequence. If runtime PM suspends the device (disabling clocks), and the NOC bus re-idles or the PVTPLL state is lost, the next `pm_runtime_resume_and_get()` will not re-initialize the bus.

The cover letter says `RK3568_PD_NPU` is `always_on` so genpd never powers it off, which might save you here for runtime PM. But for system suspend/resume (`pm_runtime_force_suspend` / `pm_runtime_force_resume`), the hardware state is very likely lost. This should be addressed — either by hooking `noc_init` into the runtime resume path, or by documenting why it's not needed.

**2. `clk_set_rate()` return values unchecked**

```c
clk_set_rate(core->clks[ROCKET_CLK_NPU_IDX].clk, 600000000UL);
clk_set_rate(core->clks[ROCKET_CLK_NPU_IDX].clk, 1000000000UL);
```

Both calls return `int` but the return is ignored. If the SCMI clock set fails (e.g., TF-A doesn't support the rate), the function silently proceeds to PMU writes on a bus that may not be ready. Check and propagate the error.

**3. `regmap_write()` return values unchecked**

```c
regmap_write(pmu, 0x70, BIT(2 + 16));
regmap_write(pmu, 0xa0, BIT(1 + 16));
regmap_write(pmu, 0x50, BIT(2 + 16));
```

`regmap_write()` can fail. While failures on MMIO-backed regmaps are rare, checking returns is good practice, especially for a sequence where ordering matters.

**4. Hardcoded PMU register offsets**

The magic numbers `0x70`, `0xa0`, `0x50`, `0x60` are explained in comments but should be `#define` constants:

```c
#define RK3568_PMU_BUS_IDLE_SFTCON0   0x50
#define RK3568_PMU_BUS_IDLE_ST        0x60
#define RK3568_PMU_NOC_AUTO_CON0      0x70
#define RK3568_PMU_PWR_GATE_SFTCON    0xa0
```

This makes the code self-documenting and greppable.

**5. Fragile PVTPLL two-step clock workaround**

```c
clk_set_rate(core->clks[ROCKET_CLK_NPU_IDX].clk, 600000000UL);
clk_set_rate(core->clks[ROCKET_CLK_NPU_IDX].clk, 1000000000UL);
```

This depends on the kernel clock framework treating 600 MHz → 1 GHz as a real rate change that triggers an SCMI call. If the clock framework or SCMI driver optimizes this differently in the future (e.g., cached rate tracking changes), this breaks silently. The comment explains the rationale well, but this is inherently fragile. Consider whether the SCMI clock driver exposes a way to force a firmware call, or whether `clk_set_rate(0)` + `clk_set_rate(target)` is more robust.

**6. Scoped block in `rocket_device_init` — non-idiomatic kernel style**

```c
	{
		struct device_node *n;
		unsigned int dma_bits = 40;

		for_each_compatible_node(n, NULL, "rockchip,rk3568-rknn-core")
			...
	}
```

Linux kernel style discourages bare scoped blocks for variable scoping. Extract this into a small helper function (e.g., `rocket_get_dma_bits()`) or declare variables at function scope.

**7. `ROCKET_CLK_NPU_IDX` relies on implicit array ordering**

```c
#define ROCKET_CLK_NPU_IDX	2
```

The `clks[]` array is populated by `devm_clk_bulk_get()` which fills entries by DT index. This works today because the DT binding mandates `clock-names = "aclk", "hclk", "npu", "pclk"` in that order. But the coupling between a `#define` in `rocket_drv.c` and the DT binding ordering is fragile and not obvious. A comment cross-referencing the binding, or better yet using `devm_clk_get(dev, "npu")` for just this one clock, would be safer.

**8. Cleanup path after `noc_init` failure**

```c
if (core->soc_data->noc_init) {
	err = core->soc_data->noc_init(core);
	if (err) {
		pm_runtime_put_sync(dev);
		rocket_job_fini(core);
		return err;
	}
}
```

This cleans up `rocket_job` and drops the runtime PM ref, but doesn't clean up IOMMU group (`iommu_group_put`) or disable runtime PM (`pm_runtime_disable`). Compare the existing error path after `pm_runtime_resume_and_get()` fails, which calls `rocket_core_fini()` — the `noc_init` failure path should be consistent.

---

---
Generated by Claude Code Patch Reviewer