From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: arm64: dts: imx952: Describe Mali G310 GPU Date: Sun, 12 Apr 2026 14:13:24 +1000 Message-ID: In-Reply-To: <20260407-master-v3-1-5a05cea0c521@nxp.com> References: <20260407-master-v3-1-5a05cea0c521@nxp.com> <20260407-master-v3-1-5a05cea0c521@nxp.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 **Missing `coregroup` (APB) clock** The imx95 GPU node specifies two clocks: ``` clocks =3D <&scmi_clk IMX95_CLK_GPU_CGC>, <&scmi_clk IMX95_CLK_GPUAPB>; clock-names =3D "core", "coregroup"; ``` But this patch only specifies one: ```c + clocks =3D <&scmi_clk IMX952_CLK_GPU>; + clock-names =3D "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 c= lock-gating mechanism has been introduced, enabling GPUMIX to automatically= manage the GPU clock" =E2=80=94 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 au= to-managed =E2=80=94 the APB clock specifically? Saying "the GPU clock" is = ambiguous. 2. The `arm,mali-valhall-csf` binding allows 1=E2=80=933 clocks, so droppin= g one is valid from a binding perspective. But the Panthor driver may behav= e differently depending on whether the "coregroup" clock is provided. If th= e hardware truly gates it automatically, this should be fine, but it deserv= es 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 diffe= rent SCMI clock topology on i.MX952, but worth double-checking. **Missing `#cooling-cells`** The imx95 GPU node has: ``` #cooling-cells =3D <2>; ``` This patch omits it. If the i.MX952 doesn't connect the GPU to a thermal zo= ne as a cooling device, this is fine =E2=80=94 but it should be noted in th= e commit message or cover letter. If thermal management is planned for a fo= llow-up, adding `#cooling-cells =3D <2>` now would avoid a future patch tha= t only adds this one line. **`opp-hz-real` property** The OPP table entries include: ``` + opp-500000000 { + opp-hz =3D /bits/ 64 <500000000>; + opp-hz-real =3D /bits/ 64 <500000000>; + opp-microvolt =3D <920000>; + }; ``` `opp-hz-real` is not a standard property in the OPP v2 binding (`Documentat= ion/devicetree/bindings/opp/opp-v2-base.yaml`). However, this is consistent= with the existing `imx95.dtsi` which uses the same property, and the OPP t= able 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-va= lhall-csf.yaml` binding, which was a fix from v2=E2=86=92v3. - Using `"nxp,imx95-mali"` rather than `"nxp,imx952-mali"` as the compatibl= e string is the right call since it's the same GPU IP =E2=80=94 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 (0x= 4d9... > 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-manage= d by hardware (presumably the APB / `coregroup` clock), and why only one cl= ock is needed on imx952 vs. two on imx95. 2. Consider adding `#cooling-cells =3D <2>` for consistency with imx95, or = explain in the commit message why it's intentionally omitted. --- Generated by Claude Code Patch Reviewer