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: Re: [PATCH v2 1/4] accel: rocket: Add support for Rockchip RK3568
Date: Thu, 04 Jun 2026 16:18:11 +1000
Message-ID: <review-patch1-5132269.NnENhoQgcM@phil>
In-Reply-To: <5132269.NnENhoQgcM@phil>
References: <26069b3b-43c0-45fa-92ee-729d270a8b6b@kernel.org> <5132269.NnENhoQgcM@phil>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**1. Runtime PM gap =E2=80=94 `noc_init` not called on resume (significant)=
**

The `noc_init` callback runs once during `rocket_core_init()`, after `pm_ru=
ntime_resume_and_get()`. But `rocket_device_runtime_resume()` only re-enabl=
es clocks =E2=80=94 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-i=
nitialize 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/re=
sume (`pm_runtime_force_suspend` / `pm_runtime_force_resume`), the hardware=
 state is very likely lost. This should be addressed =E2=80=94 either by ho=
oking `noc_init` into the runtime resume path, or by documenting why it's n=
ot 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 fa=
ils (e.g., TF-A doesn't support the rate), the function silently proceeds t=
o 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 =E2=86=92 1 GHz=
 as a real rate change that triggers an SCMI call. If the clock framework o=
r SCMI driver optimizes this differently in the future (e.g., cached rate t=
racking changes), this breaks silently. The comment explains the rationale =
well, but this is inherently fragile. Consider whether the SCMI clock drive=
r exposes a way to force a firmware call, or whether `clk_set_rate(0)` + `c=
lk_set_rate(target)` is more robust.

**6. Scoped block in `rocket_device_init` =E2=80=94 non-idiomatic kernel st=
yle**

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

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

Linux kernel style discourages bare scoped blocks for variable scoping. Ext=
ract this into a small helper function (e.g., `rocket_get_dma_bits()`) or d=
eclare 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 entrie=
s by DT index. This works today because the DT binding mandates `clock-name=
s =3D "aclk", "hclk", "npu", "pclk"` in that order. But the coupling betwee=
n 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 =3D 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_disa=
ble`). Compare the existing error path after `pm_runtime_resume_and_get()` =
fails, which calls `rocket_core_fini()` =E2=80=94 the `noc_init` failure pa=
th should be consistent.

---

---
Generated by Claude Code Patch Reviewer