Claude review: drm/panfrost: Add bus_ace optional clock support for RZ/G2L

Claude review: drm/panfrost: Add bus_ace optional clock support for RZ/G2L

[Claude Code Review Bot]

Patch Review

This patch adds a `bus_ace_clock` optional clock, wiring it into init, fini, and runtime suspend/resume.

**Bug: Duplicate `err = PTR_ERR(...)` assignment.** In `panfrost_clk_init()`:

```c
+	if (IS_ERR(pfdev->bus_ace_clock)) {
+		err = PTR_ERR(pfdev->bus_ace_clock);
+		dev_err(pfdev->base.dev, "get bus_ace_clock failed %ld\n",
+			PTR_ERR(pfdev->bus_ace_clock));
+		err = PTR_ERR(pfdev->bus_ace_clock);
+		goto disable_bus_clock;
+	}
```

The `err = PTR_ERR(pfdev->bus_ace_clock)` is assigned twice (lines before and after the `dev_err`). This is harmless but clearly a copy-paste error. The first assignment should be removed (keeping it after `dev_err` for consistency with the existing `bus_clock` error block pattern), or the second one removed.

**Inconsistency with bus_clock init pattern.** The existing `bus_clock` init code has a `if (pfdev->bus_clock)` guard around `clk_prepare_enable()` (line 64-71 in the applied code), which also prints the clock rate. The new `bus_ace_clock` code calls `clk_prepare_enable()` unconditionally (which is fine since the clk API handles NULL), but this creates an inconsistency within the same function. Consider either: (a) also removing the NULL guard from the `bus_clock` block for consistency, or (b) matching the existing pattern. This is minor since the API handles NULL, but consistency aids readability.

**Runtime resume error path nesting.** The `clk_enable(pfdev->bus_ace_clock)` call and its error label `err_bus_ace_clk` are placed *inside* the `if (pfdev->comp->pm_features & BIT(GPU_PM_RT))` block, but the error label itself is *outside* the block:

```c
	if (pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
		...
		ret = clk_enable(pfdev->bus_ace_clock);
		if (ret)
			goto err_bus_ace_clk;
	}
	...
	return 0;

err_bus_ace_clk:
	if (pfdev->comp->pm_features & BIT(GPU_PM_RT))
		clk_disable(pfdev->bus_clock);
```

This works because the goto only fires when `GPU_PM_RT` is set, and the error handlers re-check the flag. This matches the existing pattern for `err_bus_clk` and `err_clk`, so it's fine.

---
Generated by Claude Code Patch Reviewer

[PATCH v2 0/4] Add RZ/G3L GFX support

[Biju]

From: Biju Das <biju.das.jz@bp.renesas.com>

The Renesas RZ/G3L SoC includes the Arm Mali-G31 GPU as a 3D Graphics
Engine (GE3D). The Arm Mali-G31 GPU is a graphics acceleration platform
that is based on open standards. It supports 2D graphics, 3D graphics, and
General Purpose computing on GPU (GPGPU). Add the binding and driver
support for suspend/resume functionality along with some improvements in
panfrost driver.

v1->v2:
 * Dropped duplicate err assignment
 * Changed the format string of dev_err to "%d" and simplify the dev_err()
   line by using err rather than the same PTR_ERR().
 * Collected tags

Biju Das (4):
  dt-bindings: gpu: mali-bifrost: Add compatible for RZ/G3L SoC
  drm/panfrost: Drop redundant optional clock checks in runtime PM
  drm/panfrost: Add bus_ace optional clock support for RZ/G2L
  drm/panfrost: Add GPU_PM_RT support for RZ/G3L SoC

 .../bindings/gpu/arm,mali-bifrost.yaml        |  2 ++
 drivers/gpu/drm/panfrost/panfrost_device.c    | 34 ++++++++++++++-----
 drivers/gpu/drm/panfrost/panfrost_device.h    |  1 +
 drivers/gpu/drm/panfrost/panfrost_drv.c       |  1 +
 4 files changed, 30 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH v2 1/4] dt-bindings: gpu: mali-bifrost: Add compatible for RZ/G3L SoC

[Biju]

From: Biju Das <biju.das.jz@bp.renesas.com>

Add a compatible string for the Renesas RZ/G3L SoC variants that include a
Mali-G31 GPU. These variants share the same restrictions on interrupts,
clocks, and power domains as the RZ/G2L SoC, so extend the existing schema
validation accordingly.

Acked-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2:
 * Collected tag
---
 Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
index db49b8ff8c74..9db9f84ad964 100644
--- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
+++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
@@ -26,6 +26,7 @@ properties:
               - realtek,rtd1619-mali
               - renesas,r9a07g044-mali
               - renesas,r9a07g054-mali
+              - renesas,r9a08g046-mali
               - renesas,r9a09g047-mali
               - renesas,r9a09g056-mali
               - renesas,r9a09g057-mali
@@ -150,6 +151,7 @@ allOf:
             enum:
               - renesas,r9a07g044-mali
               - renesas,r9a07g054-mali
+              - renesas,r9a08g046-mali
               - renesas,r9a09g047-mali
               - renesas,r9a09g056-mali
               - renesas,r9a09g057-mali
-- 
2.43.0

[PATCH v2 2/4] drm/panfrost: Drop redundant optional clock checks in runtime PM

[Biju]

From: Biju Das <biju.das.jz@bp.renesas.com>

The clk_enable() and clk_disable() APIs already handle NULL clock pointers
gracefully — clk_enable() returns 0 and clk_disable() returns immediately
when passed a NULL or optional clock. The explicit if (pfdev->bus_clock)
guards around these calls in the runtime suspend/resume paths are
therefore unnecessary. Remove them to simplify the code.

Reviewed-by: Steven Price <steven.price@arm.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2:
 * Collected tag
---
 drivers/gpu/drm/panfrost/panfrost_device.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index dedc13e56631..01e702a0b2f0 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -429,11 +429,9 @@ static int panfrost_device_runtime_resume(struct device *dev)
 		if (ret)
 			goto err_clk;
 
-		if (pfdev->bus_clock) {
-			ret = clk_enable(pfdev->bus_clock);
-			if (ret)
-				goto err_bus_clk;
-		}
+		ret = clk_enable(pfdev->bus_clock);
+		if (ret)
+			goto err_bus_clk;
 	}
 
 	panfrost_device_reset(pfdev, true);
@@ -464,9 +462,7 @@ static int panfrost_device_runtime_suspend(struct device *dev)
 	panfrost_gpu_power_off(pfdev);
 
 	if (pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
-		if (pfdev->bus_clock)
-			clk_disable(pfdev->bus_clock);
-
+		clk_disable(pfdev->bus_clock);
 		clk_disable(pfdev->clock);
 		reset_control_assert(pfdev->rstc);
 	}
-- 
2.43.0

[PATCH v2 3/4] drm/panfrost: Add bus_ace optional clock support for RZ/G2L

[Biju]

From: Biju Das <biju.das.jz@bp.renesas.com>

On RZ/G2L SoCs, the GPU MMU requires a bus_ace clock to operate correctly.
Without it, unbind/bind cycles leave the GPU non-operational, manifesting
as an AS_ACTIVE bit stuck and a soft reset timeout falling back to hard
reset. Add bus_ace_clock as an optional clock, wiring it into init/fini,
and the runtime suspend/resume paths alongside the existing optional
bus_clock.

Reviewed-by: Steven Price <steven.price@arm.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2:
 * Dropped duplicate err assignment
 * Changed the format string of dev_err to "%d" and simplify the dev_err()
   line by using err rather than the same PTR_ERR().
 * Collected tag
---
 drivers/gpu/drm/panfrost/panfrost_device.c | 22 ++++++++++++++++++++++
 drivers/gpu/drm/panfrost/panfrost_device.h |  1 +
 2 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index 01e702a0b2f0..7fed22d555a5 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -70,8 +70,21 @@ static int panfrost_clk_init(struct panfrost_device *pfdev)
 			goto disable_clock;
 	}
 
+	pfdev->bus_ace_clock = devm_clk_get_optional(pfdev->base.dev, "bus_ace");
+	if (IS_ERR(pfdev->bus_ace_clock)) {
+		err = PTR_ERR(pfdev->bus_ace_clock);
+		dev_err(pfdev->base.dev, "get bus_ace_clock failed %d\n", err);
+		goto disable_bus_clock;
+	}
+
+	err = clk_prepare_enable(pfdev->bus_ace_clock);
+	if (err)
+		goto disable_bus_clock;
+
 	return 0;
 
+disable_bus_clock:
+	clk_disable_unprepare(pfdev->bus_clock);
 disable_clock:
 	clk_disable_unprepare(pfdev->clock);
 
@@ -80,6 +93,7 @@ static int panfrost_clk_init(struct panfrost_device *pfdev)
 
 static void panfrost_clk_fini(struct panfrost_device *pfdev)
 {
+	clk_disable_unprepare(pfdev->bus_ace_clock);
 	clk_disable_unprepare(pfdev->bus_clock);
 	clk_disable_unprepare(pfdev->clock);
 }
@@ -432,6 +446,10 @@ static int panfrost_device_runtime_resume(struct device *dev)
 		ret = clk_enable(pfdev->bus_clock);
 		if (ret)
 			goto err_bus_clk;
+
+		ret = clk_enable(pfdev->bus_ace_clock);
+		if (ret)
+			goto err_bus_ace_clk;
 	}
 
 	panfrost_device_reset(pfdev, true);
@@ -439,6 +457,9 @@ static int panfrost_device_runtime_resume(struct device *dev)
 
 	return 0;
 
+err_bus_ace_clk:
+	if (pfdev->comp->pm_features & BIT(GPU_PM_RT))
+		clk_disable(pfdev->bus_clock);
 err_bus_clk:
 	if (pfdev->comp->pm_features & BIT(GPU_PM_RT))
 		clk_disable(pfdev->clock);
@@ -462,6 +483,7 @@ static int panfrost_device_runtime_suspend(struct device *dev)
 	panfrost_gpu_power_off(pfdev);
 
 	if (pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
+		clk_disable(pfdev->bus_ace_clock);
 		clk_disable(pfdev->bus_clock);
 		clk_disable(pfdev->clock);
 		reset_control_assert(pfdev->rstc);
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index 0f3992412205..ec55c136b1b6 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -136,6 +136,7 @@ struct panfrost_device {
 	void __iomem *iomem;
 	struct clk *clock;
 	struct clk *bus_clock;
+	struct clk *bus_ace_clock;
 	struct regulator_bulk_data *regulators;
 	struct reset_control *rstc;
 	/* pm_domains for devices with more than one. */
-- 
2.43.0

[PATCH v2 4/4] drm/panfrost: Add GPU_PM_RT support for RZ/G3L SoC

[Biju]

From: Biju Das <biju.das.jz@bp.renesas.com>

RZ/G3L SoC is embedded with Mali-G31 GPU system. Add GPU_PM_RT support as
it needs to be assert/deassert the reset during suspend/resume.

Reviewed-by: Steven Price <steven.price@arm.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2:
 * Collected tag
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 711f5101aa04..3d0bdba2a474 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -1156,6 +1156,7 @@ static const struct of_device_id dt_match[] = {
 	  .data = &amlogic_data, },
 	{ .compatible = "amlogic,meson-g12a-mali",
 	  .data = &amlogic_data, },
+	{ .compatible = "renesas,r9a08g046-mali", .data = &default_pm_rt_data },
 	{ .compatible = "renesas,r9a09g047-mali", .data = &default_pm_rt_data },
 	{ .compatible = "arm,mali-t604", .data = &default_data, },
 	{ .compatible = "arm,mali-t624", .data = &default_data, },
-- 
2.43.0

Re: [PATCH v2 4/4] drm/panfrost: Add GPU_PM_RT support for RZ/G3L SoC

[Adrián Larumbe]

Reviewed-by: Adrián Larumbe <adrian.larumbe@collabora.com>

On 20.03.2026 16:41, Biju wrote:
From: Biju Das <biju.das.jz@bp.renesas.com>

RZ/G3L SoC is embedded with Mali-G31 GPU system. Add GPU_PM_RT support as
it needs to be assert/deassert the reset during suspend/resume.

Reviewed-by: Steven Price <steven.price@arm.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2:
 * Collected tag
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 711f5101aa04..3d0bdba2a474 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -1156,6 +1156,7 @@ static const struct of_device_id dt_match[] = {
 	  .data = &amlogic_data, },
 	{ .compatible = "amlogic,meson-g12a-mali",
 	  .data = &amlogic_data, },
+	{ .compatible = "renesas,r9a08g046-mali", .data = &default_pm_rt_data },
 	{ .compatible = "renesas,r9a09g047-mali", .data = &default_pm_rt_data },
 	{ .compatible = "arm,mali-t604", .data = &default_data, },
 	{ .compatible = "arm,mali-t624", .data = &default_data, },
--
2.43.0


Adrian Larumbe

Re: [PATCH v2 3/4] drm/panfrost: Add bus_ace optional clock support for RZ/G2L

[Adrián Larumbe]

Reviewed-by: Adrián Larumbe <adrian.larumbe@collabora.com>

On 20.03.2026 16:41, Biju wrote:
> From: Biju Das <biju.das.jz@bp.renesas.com>
>
> On RZ/G2L SoCs, the GPU MMU requires a bus_ace clock to operate correctly.
> Without it, unbind/bind cycles leave the GPU non-operational, manifesting
> as an AS_ACTIVE bit stuck and a soft reset timeout falling back to hard
> reset. Add bus_ace_clock as an optional clock, wiring it into init/fini,
> and the runtime suspend/resume paths alongside the existing optional
> bus_clock.
>
> Reviewed-by: Steven Price <steven.price@arm.com>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v1->v2:
>  * Dropped duplicate err assignment
>  * Changed the format string of dev_err to "%d" and simplify the dev_err()
>    line by using err rather than the same PTR_ERR().
>  * Collected tag
> ---
>  drivers/gpu/drm/panfrost/panfrost_device.c | 22 ++++++++++++++++++++++
>  drivers/gpu/drm/panfrost/panfrost_device.h |  1 +
>  2 files changed, 23 insertions(+)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 01e702a0b2f0..7fed22d555a5 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -70,8 +70,21 @@ static int panfrost_clk_init(struct panfrost_device *pfdev)
>  			goto disable_clock;
>  	}
>
> +	pfdev->bus_ace_clock = devm_clk_get_optional(pfdev->base.dev, "bus_ace");
> +	if (IS_ERR(pfdev->bus_ace_clock)) {
> +		err = PTR_ERR(pfdev->bus_ace_clock);
> +		dev_err(pfdev->base.dev, "get bus_ace_clock failed %d\n", err);
> +		goto disable_bus_clock;
> +	}
> +
> +	err = clk_prepare_enable(pfdev->bus_ace_clock);
> +	if (err)
> +		goto disable_bus_clock;
> +
>  	return 0;
>
> +disable_bus_clock:
> +	clk_disable_unprepare(pfdev->bus_clock);
>  disable_clock:
>  	clk_disable_unprepare(pfdev->clock);
>
> @@ -80,6 +93,7 @@ static int panfrost_clk_init(struct panfrost_device *pfdev)
>
>  static void panfrost_clk_fini(struct panfrost_device *pfdev)
>  {
> +	clk_disable_unprepare(pfdev->bus_ace_clock);
>  	clk_disable_unprepare(pfdev->bus_clock);
>  	clk_disable_unprepare(pfdev->clock);
>  }
> @@ -432,6 +446,10 @@ static int panfrost_device_runtime_resume(struct device *dev)
>  		ret = clk_enable(pfdev->bus_clock);
>  		if (ret)
>  			goto err_bus_clk;
> +
> +		ret = clk_enable(pfdev->bus_ace_clock);
> +		if (ret)
> +			goto err_bus_ace_clk;
>  	}
>
>  	panfrost_device_reset(pfdev, true);
> @@ -439,6 +457,9 @@ static int panfrost_device_runtime_resume(struct device *dev)
>
>  	return 0;
>
> +err_bus_ace_clk:
> +	if (pfdev->comp->pm_features & BIT(GPU_PM_RT))
> +		clk_disable(pfdev->bus_clock);
>  err_bus_clk:
>  	if (pfdev->comp->pm_features & BIT(GPU_PM_RT))
>  		clk_disable(pfdev->clock);
> @@ -462,6 +483,7 @@ static int panfrost_device_runtime_suspend(struct device *dev)
>  	panfrost_gpu_power_off(pfdev);
>
>  	if (pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
> +		clk_disable(pfdev->bus_ace_clock);
>  		clk_disable(pfdev->bus_clock);
>  		clk_disable(pfdev->clock);
>  		reset_control_assert(pfdev->rstc);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index 0f3992412205..ec55c136b1b6 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -136,6 +136,7 @@ struct panfrost_device {
>  	void __iomem *iomem;
>  	struct clk *clock;
>  	struct clk *bus_clock;
> +	struct clk *bus_ace_clock;
>  	struct regulator_bulk_data *regulators;
>  	struct reset_control *rstc;
>  	/* pm_domains for devices with more than one. */
> --
> 2.43.0


Adrian Larumbe

Re: [PATCH v2 2/4] drm/panfrost: Drop redundant optional clock checks in runtime PM

[Adrián Larumbe]

Hi Biju,

On 20.03.2026 16:41, Biju wrote:
> From: Biju Das <biju.das.jz@bp.renesas.com>
>
> The clk_enable() and clk_disable() APIs already handle NULL clock pointers
> gracefully — clk_enable() returns 0 and clk_disable() returns immediately
> when passed a NULL or optional clock. The explicit if (pfdev->bus_clock)
> guards around these calls in the runtime suspend/resume paths are
> therefore unnecessary. Remove them to simplify the code.
>
> Reviewed-by: Steven Price <steven.price@arm.com>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v1->v2:
>  * Collected tag
> ---
>  drivers/gpu/drm/panfrost/panfrost_device.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index dedc13e56631..01e702a0b2f0 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -429,11 +429,9 @@ static int panfrost_device_runtime_resume(struct device *dev)
>  		if (ret)
>  			goto err_clk;
>
> -		if (pfdev->bus_clock) {
> -			ret = clk_enable(pfdev->bus_clock);
> -			if (ret)
> -				goto err_bus_clk;
> -		}
> +		ret = clk_enable(pfdev->bus_clock);
> +		if (ret)
> +			goto err_bus_clk;
>  	}

It seems clk_prepare_enable() can also deal with NULL clock device pointers gracefully,
so maybe you could also do away with pointer checks in panfrost_clk_init?

Other than that,

Reviewed-by: Adrián Larumbe <adrian.larumbe@collabora.com>

>  	panfrost_device_reset(pfdev, true);
> @@ -464,9 +462,7 @@ static int panfrost_device_runtime_suspend(struct device *dev)
>  	panfrost_gpu_power_off(pfdev);
>
>  	if (pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
> -		if (pfdev->bus_clock)
> -			clk_disable(pfdev->bus_clock);
> -
> +		clk_disable(pfdev->bus_clock);
>  		clk_disable(pfdev->clock);
>  		reset_control_assert(pfdev->rstc);
>  	}
> --
> 2.43.0


Adrian Larumbe

RE: [PATCH v2 2/4] drm/panfrost: Drop redundant optional clock checks in runtime PM

[Biju Das]

> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Adrián Larumbe
> Sent: 20 March 2026 21:20
> To: biju.das.au <biju.das.au@gmail.com>
> Cc: Boris Brezillon <boris.brezillon@collabora.com>; Rob Herring <robh@kernel.org>; Steven Price
> <steven.price@arm.com>; Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Maxime Ripard
> <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>; David Airlie <airlied@gmail.com>;
> Simona Vetter <simona@ffwll.ch>; Biju Das <biju.das.jz@bp.renesas.com>; dri-
> devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; Geert Uytterhoeven
> <geert+renesas@glider.be>; Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>; linux-
> renesas-soc@vger.kernel.org
> Subject: Re: [PATCH v2 2/4] drm/panfrost: Drop redundant optional clock checks in runtime PM
> 
> Hi Biju,
> 
> On 20.03.2026 16:41, Biju wrote:
> > From: Biju Das <biju.das.jz@bp.renesas.com>
> >
> > The clk_enable() and clk_disable() APIs already handle NULL clock
> > pointers gracefully — clk_enable() returns 0 and clk_disable() returns
> > immediately when passed a NULL or optional clock. The explicit if
> > (pfdev->bus_clock) guards around these calls in the runtime
> > suspend/resume paths are therefore unnecessary. Remove them to simplify the code.
> >
> > Reviewed-by: Steven Price <steven.price@arm.com>
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v1->v2:
> >  * Collected tag
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_device.c | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c
> > b/drivers/gpu/drm/panfrost/panfrost_device.c
> > index dedc13e56631..01e702a0b2f0 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> > @@ -429,11 +429,9 @@ static int panfrost_device_runtime_resume(struct device *dev)
> >  		if (ret)
> >  			goto err_clk;
> >
> > -		if (pfdev->bus_clock) {
> > -			ret = clk_enable(pfdev->bus_clock);
> > -			if (ret)
> > -				goto err_bus_clk;
> > -		}
> > +		ret = clk_enable(pfdev->bus_clock);
> > +		if (ret)
> > +			goto err_bus_clk;
> >  	}
> 
> It seems clk_prepare_enable() can also deal with NULL clock device pointers gracefully, so maybe you
> could also do away with pointer checks in panfrost_clk_init?

This is the only check and no need to print rate for optional clk. That is the reason I have not
touched this.

	if (pfdev->bus_clock) {
		rate = clk_get_rate(pfdev->bus_clock);
		dev_info(pfdev->base.dev, "bus_clock rate = %lu\n", rate);

		err = clk_prepare_enable(pfdev->bus_clock);
		if (err)
			goto disable_clock;
	}

Cheers,
Biju
> 
> Other than that,
> 
> Reviewed-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> 
> >  	panfrost_device_reset(pfdev, true);
> > @@ -464,9 +462,7 @@ static int panfrost_device_runtime_suspend(struct device *dev)
> >  	panfrost_gpu_power_off(pfdev);
> >
> >  	if (pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
> > -		if (pfdev->bus_clock)
> > -			clk_disable(pfdev->bus_clock);
> > -
> > +		clk_disable(pfdev->bus_clock);
> >  		clk_disable(pfdev->clock);
> >  		reset_control_assert(pfdev->rstc);
> >  	}
> > --
> > 2.43.0
> 
> 
> Adrian Larumbe

RE: [PATCH v2 2/4] drm/panfrost: Drop redundant optional clock checks in runtime PM

[Biju Das]

Hi Adrián Larumbe,

Thanks for the feedback.

> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Biju Das
> Sent: 20 March 2026 21:32
> Subject: RE: [PATCH v2 2/4] drm/panfrost: Drop redundant optional clock checks in runtime PM
> 
> 
> 
> > -----Original Message-----
> > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
> > Adrián Larumbe
> > Sent: 20 March 2026 21:20
> > Subject: Re: [PATCH v2 2/4] drm/panfrost: Drop redundant optional
> > clock checks in runtime PM
> >
> > Hi Biju,
> >
> > On 20.03.2026 16:41, Biju wrote:
> > > From: Biju Das <biju.das.jz@bp.renesas.com>
> > >
> > > The clk_enable() and clk_disable() APIs already handle NULL clock
> > > pointers gracefully — clk_enable() returns 0 and clk_disable()
> > > returns immediately when passed a NULL or optional clock. The
> > > explicit if
> > > (pfdev->bus_clock) guards around these calls in the runtime
> > > suspend/resume paths are therefore unnecessary. Remove them to simplify the code.
> > >
> > > Reviewed-by: Steven Price <steven.price@arm.com>
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > > v1->v2:
> > >  * Collected tag
> > > ---
> > >  drivers/gpu/drm/panfrost/panfrost_device.c | 12 ++++--------
> > >  1 file changed, 4 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c
> > > b/drivers/gpu/drm/panfrost/panfrost_device.c
> > > index dedc13e56631..01e702a0b2f0 100644
> > > --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> > > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> > > @@ -429,11 +429,9 @@ static int panfrost_device_runtime_resume(struct device *dev)
> > >  		if (ret)
> > >  			goto err_clk;
> > >
> > > -		if (pfdev->bus_clock) {
> > > -			ret = clk_enable(pfdev->bus_clock);
> > > -			if (ret)
> > > -				goto err_bus_clk;
> > > -		}
> > > +		ret = clk_enable(pfdev->bus_clock);
> > > +		if (ret)
> > > +			goto err_bus_clk;
> > >  	}
> >
> > It seems clk_prepare_enable() can also deal with NULL clock device
> > pointers gracefully, so maybe you could also do away with pointer checks in panfrost_clk_init?
> 
> This is the only check and no need to print rate for optional clk. That is the reason I have not
> touched this.
> 
> 	if (pfdev->bus_clock) {
> 		rate = clk_get_rate(pfdev->bus_clock);
> 		dev_info(pfdev->base.dev, "bus_clock rate = %lu\n", rate);
> 
> 		err = clk_prepare_enable(pfdev->bus_clock);
> 		if (err)
> 			goto disable_clock;
> 	}

The above block is good for optional clock.

Otherwise, there will be 2 checks for optional clk.

One here:

if (pfdev->bus_clock) {
	rate = clk_get_rate(pfdev->bus_clock);
	dev_info(pfdev->base.dev, "bus_clock rate = %lu\n", rate);
}

and one inside the clk_prepare_enable(): 

err = clk_prepare_enable(pfdev->bus_clock);

Please let me know your thoughts.

Cheers,
Biju

> 
> Cheers,
> Biju
> >
> > Other than that,
> >
> > Reviewed-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> >
> > >  	panfrost_device_reset(pfdev, true); @@ -464,9 +462,7 @@ static int
> > > panfrost_device_runtime_suspend(struct device *dev)
> > >  	panfrost_gpu_power_off(pfdev);
> > >
> > >  	if (pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
> > > -		if (pfdev->bus_clock)
> > > -			clk_disable(pfdev->bus_clock);
> > > -
> > > +		clk_disable(pfdev->bus_clock);
> > >  		clk_disable(pfdev->clock);
> > >  		reset_control_assert(pfdev->rstc);
> > >  	}
> > > --
> > > 2.43.0
> >
> >
> > Adrian Larumbe

Claude review: Add RZ/G3L GFX support

[Claude Code Review Bot]

Overall Series Review

Subject: Add RZ/G3L GFX support
Author: Biju <biju.das.au@gmail.com>
Patches: 10
Reviewed: 2026-03-22T03:29:02.632662

---

This is a clean, well-structured 4-patch series adding Mali-G31 GPU support for the Renesas RZ/G3L SoC via the panfrost driver. The series has already collected Reviewed-by/Acked-by tags from the relevant maintainers (Rob Herring, Steven Price). The changes are straightforward: DT binding addition, a small cleanup, a new optional clock, and a new compatible entry. Overall the series looks good with one notable issue in patch 3.

---
Generated by Claude Code Patch Reviewer

Claude review: dt-bindings: gpu: mali-bifrost: Add compatible for RZ/G3L SoC

[Claude Code Review Bot]

Patch Review

Straightforward addition of `renesas,r9a08g046-mali` to the mali-bifrost binding in two places: the compatible enum and the allOf conditional that applies RZ/G2L-family constraints (interrupt, clock, power-domain restrictions). The new entry is inserted in the correct sorted position by SoC part number.

No issues. Looks good.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/panfrost: Drop redundant optional clock checks in runtime PM

[Claude Code Review Bot]

Patch Review

The commit message correctly explains that `clk_enable(NULL)` returns 0 and `clk_disable(NULL)` is a no-op, so the `if (pfdev->bus_clock)` guards are unnecessary.

**Observation:** This patch only removes the NULL checks in `panfrost_device_runtime_resume()` and `panfrost_device_runtime_suspend()` (the `GPU_PM_RT` path), but leaves identical redundant checks in `panfrost_device_resume()` and `panfrost_device_suspend()` (the `GPU_PM_CLK_DIS` path):

```c
// panfrost_device_resume() still has:
if (pfdev->bus_clock) {
    ret = clk_enable(pfdev->bus_clock);

// panfrost_device_suspend() still has:
if (pfdev->bus_clock)
    clk_disable(pfdev->bus_clock);
```

For consistency, it would be cleaner to also drop those guards in the same patch. Not a blocking issue since the `GPU_PM_CLK_DIS` path is a separate code path, but it would be more complete.

The change itself is correct.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/panfrost: Add bus_ace optional clock support for RZ/G2L

[Claude Code Review Bot]

Patch Review

This adds an optional `bus_ace` clock alongside the existing `bus` clock. The 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_clock)` 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 = clk_enable(pfdev->bus_clock);
		if (ret)
			goto err_bus_clk;

		ret = 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 optional, 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 tree, RZ/G2L (`r9a07g044`) uses `default_pm_rt_data` — so the `GPU_PM_RT` gate is the correct place. This seems consistent.

**Issue: `panfrost_device_resume`/`panfrost_device_suspend` (`GPU_PM_CLK_DIS` path) not updated**

The `bus_ace_clock` is not handled in `panfrost_device_resume()` and `panfrost_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 clock 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 could 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

Claude review: drm/panfrost: Add GPU_PM_RT support for RZ/G3L SoC

[Claude Code Review Bot]

Patch Review

Simple one-liner adding the `r9a08g046-mali` compatible with `default_pm_rt_data`, placed in the correct sorted position next to the existing `r9a09g047-mali` entry.

No issues. Looks good.

---
Generated by Claude Code Patch Reviewer