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: qcom: Add GPU support for Glymur Date: Tue, 05 May 2026 09:19:37 +1000 Message-ID: In-Reply-To: <20260501-glymur-gpu-dt-v2-4-2f128b5596bb@oss.qualcomm.com> References: <20260501-glymur-gpu-dt-v2-0-2f128b5596bb@oss.qualcomm.com> <20260501-glymur-gpu-dt-v2-4-2f128b5596bb@oss.qualcomm.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 **Status: Mostly good, a few items to consider.** This is the main patch, adding the GPU and GMU nodes with a 12-entry OPP ta= ble. **GPU node:** ``` + gpu: gpu@3d00000 { + compatible =3D "qcom,adreno-44070001", "qcom,adreno"; + reg =3D <0x0 0x03d00000 0x0 0x6c000>, + <0x0 0x03d9e000 0x0 0x2000>; + reg-names =3D "kgsl_3d0_reg_memory", + "cx_mem"; ``` - The `kgsl_3d0_reg_memory` region is `0x6c000` bytes, larger than sm8550's= `0x40000`. This is consistent with a newer, larger GPU IP block. The cover= letter mentions it includes the `cx_dbgc` range that was previously separa= te. **OPP table:** ``` + gpu_opp_table: opp-table { + compatible =3D "operating-points-v2-adreno", + "operating-points-v2"; ``` - Uses `operating-points-v2-adreno` dual-compatible, consistent with the pa= ttern in sm8650 and hamoa (newer SoCs). This is correct for supporting the = `opp-supported-hw` and `qcom,opp-acd-level` properties used by the adreno d= river. - The `opp-supported-hw` values form a clear speed-bin mask pattern: `0xf` = (all bins) for lower frequencies, `0x7` (bins 0-2) for 1550-1700 MHz, and `= 0x3` (bins 0-1) for the top 1850 MHz. This is a reasonable binning scheme. - The two lowest OPPs (310 MHz, 410 MHz) have `/* ACD is disabled */` comme= nts instead of `qcom,opp-acd-level`. This is a reasonable pattern =E2=80=94= ACD (Adaptive Clock Distribution) is typically not needed at low voltages/= frequencies. **GMU node:** ``` + gmu: gmu@3d6c000 { + compatible =3D "qcom,adreno-gmu-x285.1", "qcom,adreno-gmu"; ``` - The GMU compatible `qcom,adreno-gmu-x285.1` follows the newer naming patt= ern (matching the marketing "X2-85" name more closely than the GPU chip ID). - **6 clocks** vs 7 on sm8550/sm8650. Missing `axi` and `demet`, but adds `= rscc`. This is a different clock topology =E2=80=94 presumably the RSCC (Re= source State Coordinator Controller) hub clock replaces or subsumes the dem= et clock on this platform, and the AXI path is handled differently. This sh= ould be fine as long as the GMU driver supports this configuration. - The GMU `reg` is at `0x03d6c000` with size `0x32000`, which ends at `0x03= d9e000` =E2=80=94 exactly where `cx_mem` begins. Good, no overlap. **Node ordering:** The GPU node is placed before `gxclkctl` (at `0x3d64000`), and the GMU is p= laced after `gxclkctl` but before `gpucc` (at `0x3d90000`). The GPU node's = address `0x3d00000` comes before `gxclkctl`'s `0x3d64000`, and the GMU at `= 0x3d6c000` comes after `gxclkctl`. This ordering follows the DT convention = of sorting by unit address, which is correct. **Minor observations:** 1. The GPU `reg` range (`0x03d00000` to `0x03d6c000`) overlaps with `gxclkc= tl` at `0x03d64000` size `0x6000` (ending at `0x03d6a000`). The GPU `kgsl_3= d0_reg_memory` covers `0x03d00000 + 0x6c000 =3D 0x03d6c000`, so it includes= the range `0x03d64000-0x03d6a000` which is also claimed by `gxclkctl`. Thi= s is likely intentional =E2=80=94 the GPU register space is a large contigu= ous block and the clock controller within it is described as a separate dev= ice for the clock framework. This is a common pattern in Qualcomm SoCs (the= gpucc at `0x3d90000` is similarly within the broader GPU register range on= some platforms). But reviewers may want to double-check this is intended. 2. No `status =3D "disabled"` on the GPU/GMU/SMMU nodes. The cover letter e= xplicitly says "Keep GPU/GMU enabled by default and drop the enablement pat= ch (Konrad)" =E2=80=94 this was a v2 change based on reviewer feedback, so = this is intentional. **No blocking issues found.** --- Generated by Claude Code Patch Reviewer