public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH v3] arm64: dts: imx952: Describe Mali G310 GPU
@ 2026-04-07  3:15 Guangliu Ding
  2026-04-08 13:56 ` Liviu Dudau
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Guangliu Ding @ 2026-04-07  3:15 UTC (permalink / raw)
  To: Daniel Almeida, Alice Ryhl, Boris Brezillon, Steven Price,
	Liviu Dudau, David Airlie, Simona Vetter, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Frank Li, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam
  Cc: dri-devel, devicetree, linux-kernel, imx, linux-arm-kernel,
	Guangliu Ding

Support Mali G310 GPU on i.MX952 board. Describe this GPU in the DT.
Include dummy GPU voltage regulator and OPP tables.

A hardware GPU auto clock‑gating mechanism has been introduced,
enabling GPUMIX to automatically manage the GPU clock. This improves
overall response time.

Signed-off-by: Guangliu Ding <guangliu.ding@nxp.com>
---
This series enable Mali G310 GPU support on i.MX952 boards, the same GPU
IP as the instance on i.MX95 boards.
---
Changes in v3:
- Follow the order of interrupts/interrupt-names in arm,mali-valhall-csf.yaml.
- Drop dt-bindings change in arm,mali-valhall-csf.yaml.
- Replace "nxp,imx952-mali" with "nxp,imx95-mali" in compatible.
- Link to v2: https://patch.msgid.link/20260401-master-v2-0-20d3fbcd19d6@nxp.com

Changes in v2:
- Improve patch description, adding more GPU information.
- Remove Reviewed-by tag.
- Link to v1: https://patch.msgid.link/20260331-master-v1-0-65c8e318d462@nxp.com
---
 arch/arm64/boot/dts/freescale/imx952.dtsi | 36 +++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx952.dtsi b/arch/arm64/boot/dts/freescale/imx952.dtsi
index 91fe4916ac04..ced09e7a1dc5 100644
--- a/arch/arm64/boot/dts/freescale/imx952.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx952.dtsi
@@ -318,6 +318,28 @@ usbphynop2: usbphynop2 {
 		clock-names = "main_clk";
 	};
 
+	gpu_opp_table: opp-table {
+		compatible = "operating-points-v2";
+
+		opp-500000000 {
+			opp-hz = /bits/ 64 <500000000>;
+			opp-hz-real = /bits/ 64 <500000000>;
+			opp-microvolt = <920000>;
+		};
+
+		opp-800000000 {
+			opp-hz = /bits/ 64 <800000000>;
+			opp-hz-real = /bits/ 64 <800000000>;
+			opp-microvolt = <920000>;
+		};
+
+		opp-1000000000 {
+			opp-hz = /bits/ 64 <1000000000>;
+			opp-hz-real = /bits/ 64 <1000000000>;
+			opp-microvolt = <920000>;
+		};
+	};
+
 	soc {
 		compatible = "simple-bus";
 		#address-cells = <2>;
@@ -1262,5 +1284,19 @@ usbmisc2: usbmisc@4c200200 {
 			reg = <0x0 0x4c200200 0x0 0x200>,
 			      <0x0 0x4c010014 0x0 0x4>;
 		};
+
+		gpu: gpu@4d900000 {
+			compatible = "nxp,imx95-mali", "arm,mali-valhall-csf";
+			reg = <0 0x4d900000 0 0x480000>;
+			interrupts = <GIC_SPI 289 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 290 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 288 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "job", "mmu", "gpu";
+			clocks = <&scmi_clk IMX952_CLK_GPU>;
+			clock-names = "core";
+			power-domains = <&scmi_devpd IMX952_PD_GPU>;
+			operating-points-v2 = <&gpu_opp_table>;
+			dynamic-power-coefficient = <1013>;
+		};
 	};
 };

---
base-commit: 0138af2472dfdef0d56fc4697416eaa0ff2589bd
change-id: 20260331-master-7ec7ff0fe1b2

Best regards,
--  
Guangliu Ding <guangliu.ding@nxp.com>


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] arm64: dts: imx952: Describe Mali G310 GPU
  2026-04-07  3:15 [PATCH v3] arm64: dts: imx952: Describe Mali G310 GPU Guangliu Ding
@ 2026-04-08 13:56 ` Liviu Dudau
  2026-04-12  4:13 ` Claude review: " Claude Code Review Bot
  2026-04-12  4:13 ` Claude Code Review Bot
  2 siblings, 0 replies; 4+ messages in thread
From: Liviu Dudau @ 2026-04-08 13:56 UTC (permalink / raw)
  To: Guangliu Ding
  Cc: Daniel Almeida, Alice Ryhl, Boris Brezillon, Steven Price,
	David Airlie, Simona Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Frank Li, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	dri-devel, devicetree, linux-kernel, imx, linux-arm-kernel

On Tue, Apr 07, 2026 at 11:15:03AM +0800, Guangliu Ding wrote:
> Support Mali G310 GPU on i.MX952 board. Describe this GPU in the DT.
> Include dummy GPU voltage regulator and OPP tables.
> 
> A hardware GPU auto clock‑gating mechanism has been introduced,
> enabling GPUMIX to automatically manage the GPU clock. This improves
> overall response time.
> 
> Signed-off-by: Guangliu Ding <guangliu.ding@nxp.com>

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> ---
> This series enable Mali G310 GPU support on i.MX952 boards, the same GPU
> IP as the instance on i.MX95 boards.
> ---
> Changes in v3:
> - Follow the order of interrupts/interrupt-names in arm,mali-valhall-csf.yaml.
> - Drop dt-bindings change in arm,mali-valhall-csf.yaml.
> - Replace "nxp,imx952-mali" with "nxp,imx95-mali" in compatible.
> - Link to v2: https://patch.msgid.link/20260401-master-v2-0-20d3fbcd19d6@nxp.com
> 
> Changes in v2:
> - Improve patch description, adding more GPU information.
> - Remove Reviewed-by tag.
> - Link to v1: https://patch.msgid.link/20260331-master-v1-0-65c8e318d462@nxp.com
> ---
>  arch/arm64/boot/dts/freescale/imx952.dtsi | 36 +++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx952.dtsi b/arch/arm64/boot/dts/freescale/imx952.dtsi
> index 91fe4916ac04..ced09e7a1dc5 100644
> --- a/arch/arm64/boot/dts/freescale/imx952.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx952.dtsi
> @@ -318,6 +318,28 @@ usbphynop2: usbphynop2 {
>  		clock-names = "main_clk";
>  	};
>  
> +	gpu_opp_table: opp-table {
> +		compatible = "operating-points-v2";
> +
> +		opp-500000000 {
> +			opp-hz = /bits/ 64 <500000000>;
> +			opp-hz-real = /bits/ 64 <500000000>;
> +			opp-microvolt = <920000>;
> +		};
> +
> +		opp-800000000 {
> +			opp-hz = /bits/ 64 <800000000>;
> +			opp-hz-real = /bits/ 64 <800000000>;
> +			opp-microvolt = <920000>;
> +		};
> +
> +		opp-1000000000 {
> +			opp-hz = /bits/ 64 <1000000000>;
> +			opp-hz-real = /bits/ 64 <1000000000>;
> +			opp-microvolt = <920000>;
> +		};
> +	};
> +
>  	soc {
>  		compatible = "simple-bus";
>  		#address-cells = <2>;
> @@ -1262,5 +1284,19 @@ usbmisc2: usbmisc@4c200200 {
>  			reg = <0x0 0x4c200200 0x0 0x200>,
>  			      <0x0 0x4c010014 0x0 0x4>;
>  		};
> +
> +		gpu: gpu@4d900000 {
> +			compatible = "nxp,imx95-mali", "arm,mali-valhall-csf";
> +			reg = <0 0x4d900000 0 0x480000>;
> +			interrupts = <GIC_SPI 289 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 290 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 288 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "job", "mmu", "gpu";
> +			clocks = <&scmi_clk IMX952_CLK_GPU>;
> +			clock-names = "core";
> +			power-domains = <&scmi_devpd IMX952_PD_GPU>;
> +			operating-points-v2 = <&gpu_opp_table>;
> +			dynamic-power-coefficient = <1013>;
> +		};
>  	};
>  };
> 
> ---
> base-commit: 0138af2472dfdef0d56fc4697416eaa0ff2589bd
> change-id: 20260331-master-7ec7ff0fe1b2
> 
> Best regards,
> --  
> Guangliu Ding <guangliu.ding@nxp.com>
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Claude review: arm64: dts: imx952: Describe Mali G310 GPU
  2026-04-07  3:15 [PATCH v3] arm64: dts: imx952: Describe Mali G310 GPU Guangliu Ding
  2026-04-08 13:56 ` Liviu Dudau
@ 2026-04-12  4:13 ` Claude Code Review Bot
  2026-04-12  4:13 ` Claude Code Review Bot
  2 siblings, 0 replies; 4+ messages in thread
From: Claude Code Review Bot @ 2026-04-12  4:13 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: arm64: dts: imx952: Describe Mali G310 GPU
Author: Guangliu Ding <guangliu.ding@nxp.com>
Patches: 2
Reviewed: 2026-04-12T14:13:24.488598

---

This is a single patch (v3) adding a device tree description for the Mali G310 GPU on the i.MX952 SoC. The patch adds an OPP table and a GPU device node to `imx952.dtsi`, mirroring the existing GPU definition in `imx95.dtsi` since both SoCs use the same Mali G310 GPU IP block.

The patch is mostly straightforward and follows the existing imx95 GPU node as a template. There are two items worth discussing — one potentially significant (missing clock) and one minor (missing cooling-cells).

**Verdict**: Patch needs clarification on the missing `coregroup` clock, and the missing `#cooling-cells` should be explained or added.

---

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Claude review: arm64: dts: imx952: Describe Mali G310 GPU
  2026-04-07  3:15 [PATCH v3] arm64: dts: imx952: Describe Mali G310 GPU Guangliu Ding
  2026-04-08 13:56 ` Liviu Dudau
  2026-04-12  4:13 ` Claude review: " Claude Code Review Bot
@ 2026-04-12  4:13 ` Claude Code Review Bot
  2 siblings, 0 replies; 4+ messages in thread
From: Claude Code Review Bot @ 2026-04-12  4:13 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Missing `coregroup` (APB) clock**

The imx95 GPU node specifies two clocks:
```
clocks = <&scmi_clk IMX95_CLK_GPU_CGC>, <&scmi_clk IMX95_CLK_GPUAPB>;
clock-names = "core", "coregroup";
```

But this patch only specifies one:
```c
+			clocks = <&scmi_clk IMX952_CLK_GPU>;
+			clock-names = "core";
```

The imx952-clock.h header *does* define `IMX952_CLK_GPUAPB` (value 83), so the clock exists in the SoC. The commit message says "A hardware GPU auto clock-gating mechanism has been introduced, enabling GPUMIX to automatically manage the GPU clock" — this presumably explains why the APB clock is omitted (it's auto-managed now). However, a couple of concerns:

1. It would be helpful to clarify in the commit message *which* clock is auto-managed — the APB clock specifically? Saying "the GPU clock" is ambiguous.
2. The `arm,mali-valhall-csf` binding allows 1–3 clocks, so dropping one is valid from a binding perspective. But the Panthor driver may behave differently depending on whether the "coregroup" clock is provided. If the hardware truly gates it automatically, this should be fine, but it deserves explicit mention.

Also note that `imx95` uses `IMX95_CLK_GPU_CGC` for the "core" clock, while this patch uses `IMX952_CLK_GPU`. This is presumably correct for the different SCMI clock topology on i.MX952, but worth double-checking.

**Missing `#cooling-cells`**

The imx95 GPU node has:
```
#cooling-cells = <2>;
```

This patch omits it. If the i.MX952 doesn't connect the GPU to a thermal zone as a cooling device, this is fine — but it should be noted in the commit message or cover letter. If thermal management is planned for a follow-up, adding `#cooling-cells = <2>` now would avoid a future patch that only adds this one line.

**`opp-hz-real` property**

The OPP table entries include:
```
+		opp-500000000 {
+			opp-hz = /bits/ 64 <500000000>;
+			opp-hz-real = /bits/ 64 <500000000>;
+			opp-microvolt = <920000>;
+		};
```

`opp-hz-real` is not a standard property in the OPP v2 binding (`Documentation/devicetree/bindings/opp/opp-v2-base.yaml`). However, this is consistent with the existing `imx95.dtsi` which uses the same property, and the OPP table is actually identical to the one in imx95. So this is not a new issue introduced by this patch, but it's worth noting for the record.

**Positive observations**:
- The interrupt ordering (job, mmu, gpu) correctly matches the `arm,mali-valhall-csf.yaml` binding, which was a fix from v2→v3.
- Using `"nxp,imx95-mali"` rather than `"nxp,imx952-mali"` as the compatible string is the right call since it's the same GPU IP — no need for a new compatible and binding update.
- The register address (`0x4d900000`) and size (`0x480000`) match the imx95 GPU, which is consistent with "same GPU IP."
- Node placement at the end of the `soc` bus maintains address ordering (0x4d9... > 0x4c2...).
- The OPP table is correctly placed outside the `soc` node, consistent with imx95.dtsi.

**Summary of requested changes**:
1. Please clarify in the commit message which specific clock is auto-managed by hardware (presumably the APB / `coregroup` clock), and why only one clock is needed on imx952 vs. two on imx95.
2. Consider adding `#cooling-cells = <2>` for consistency with imx95, or explain in the commit message why it's intentionally omitted.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-04-12  4:13 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-07  3:15 [PATCH v3] arm64: dts: imx952: Describe Mali G310 GPU Guangliu Ding
2026-04-08 13:56 ` Liviu Dudau
2026-04-12  4:13 ` Claude review: " Claude Code Review Bot
2026-04-12  4:13 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox