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: drm/panfrost: Add bus_ace optional clock support for RZ/G2L
Date: Sun, 22 Mar 2026 03:29:03 +1000
Message-ID: <review-patch3-20260320164158.487406-4-biju.das.jz@bp.renesas.com>
In-Reply-To: <20260320164158.487406-4-biju.das.jz@bp.renesas.com>
References: <20260320164158.487406-1-biju.das.jz@bp.renesas.com> <20260320164158.487406-4-biju.das.jz@bp.renesas.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

This adds an optional `bus_ace` clock alongside the existing `bus` clock. T=
he init/fini paths look correct. However, there is an issue in the runtime =
resume error handling and a question about the runtime resume enable path.

**Issue: bus_ace_clock enable is gated by GPU_PM_RT but only conditionally =
needed**

In `panfrost_device_runtime_resume()`, the new `clk_enable(pfdev->bus_ace_c=
lock)` call is placed *inside* the `if (pfdev->comp->pm_features & BIT(GPU_=
PM_RT))` block:

```c
	if (pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
		...
		ret =3D clk_enable(pfdev->bus_clock);
		if (ret)
			goto err_bus_clk;

		ret =3D clk_enable(pfdev->bus_ace_clock);
		if (ret)
			goto err_bus_ace_clk;
	}
```

This is fine because `clk_enable(NULL)` returns 0, and the clock is optiona=
l, so it will be NULL on platforms that don't need it. However, the commit =
message says the clock is needed for RZ/G2L, yet looking at the existing tr=
ee, RZ/G2L (`r9a07g044`) uses `default_pm_rt_data` =E2=80=94 so the `GPU_PM=
_RT` gate is the correct place. This seems consistent.

**Issue: `panfrost_device_resume`/`panfrost_device_suspend` (`GPU_PM_CLK_DI=
S` path) not updated**

The `bus_ace_clock` is not handled in `panfrost_device_resume()` and `panfr=
ost_device_suspend()` which manage clocks for the `GPU_PM_CLK_DIS` feature.=
 If any future SoC with `bus_ace_clock` also uses `GPU_PM_CLK_DIS`, the clo=
ck would not be properly managed during system suspend/resume. For current =
hardware this is not a problem since RZ/G2L only uses `GPU_PM_RT`, but it c=
ould be worth a note or a follow-up.

**The runtime resume/suspend paths look correct** for the current use case.=
 The error cleanup properly disables `bus_clock` before falling through to =
`err_bus_clk`.

---
Generated by Claude Code Patch Reviewer