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: glymur: Add ADSP and CDSP for Glymur SoC Date: Wed, 11 Mar 2026 13:43:35 +1000 Message-ID: In-Reply-To: <20260310033617.3108675-5-sibi.sankar@oss.qualcomm.com> References: <20260310033617.3108675-1-sibi.sankar@oss.qualcomm.com> <20260310033617.3108675-5-sibi.sankar@oss.qualcomm.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review **Generally looks good.** The ADSP and CDSP nodes follow the established patterns from sm8550/sm8750. A few observations: 1. **ADSP `iommus` property on remoteproc node:** The Glymur ADSP has `iommus = <&apps_smmu 0x1000 0x0>;` on the top-level remoteproc node. Neither sm8550 nor sm8750 have this property on their ADSP remoteproc nodes. The cover letter mentions that some Glymur variants run Linux at EL2 where firmware streams are managed in-kernel, which likely explains this difference. It should work since the `qcom,pas-common.yaml` schema allows `iommus` as an optional property, but this is worth noting. 2. **CDSP `iommus` property:** Similarly, `iommus = <&apps_smmu 0x2400 0x400>;` on the CDSP remoteproc node. Same reasoning as above. 3. **ADSP fastrpc compute-cb SIDs differ from sm8750:** The second SMMU SID for ADSP fastrpc compute-cb nodes uses `0x1063`/`0x1064`/etc. (Glymur) vs `0x1043`/`0x1044`/etc. (sm8750). However, they match sm8550's pattern (`0x1063`, `0x1064`, etc.). Since the v4 changelog says "Fix SID used in ADSP/CDSP for correctness [Konrad]", this appears to be an intentional correction. 4. **CDSP fastrpc compute-cb SIDs:** The CDSP compute-cb nodes use a different SID pattern from sm8550. For example, compute-cb@1: - sm8550: `<&apps_smmu 0x1961 0x0>, <&apps_smmu 0x0c01 0x20>, <&apps_smmu 0x19c1 0x10>` - Glymur: `<&apps_smmu 0x2401 0x440>, <&apps_smmu 0x1961 0x0>, <&apps_smmu 0x19c1 0x0>` The Glymur CDSP fastrpc uses `0x2401`-based primary SIDs with `0x440` mask, plus `0x1961`/`0x19c1` auxiliary SIDs with `0x0` mask. The `0x0c0x` SIDs from sm8550 are absent. This is plausible as different SoC generations remap SMMU SIDs differently, and has been reviewed by Konrad. 5. **CDSP compute-cb@10 SID mismatch with unit address:** compute-cb@10 has `reg = <10>` but uses SMMU SIDs `0x240c` (which is CB index 12, not 10). This is actually the pattern: `0x2400 + 0xc = 0x240c`, where `0xc` = 12 decimal. Comparing with cb@8 (`0x2408`, CB index 8) and cb@11 (`0x240d`, CB index 13) and cb@12 (`0x240e`, CB index 14), there's a consistent offset: the SMMU SID low nibble is `reg + 2` for reg >= 10. This seems intentional (CB9 is secure and skipped, so the SMMU SID mapping continues sequentially past it). This is fine. 6. **No `qcom,non-secure-domain` on fastrpc nodes:** Both sm8550 and sm8750 have `qcom,non-secure-domain` on their ADSP fastrpc nodes, but Glymur does not. This could be intentional given the EL2 context, but is worth confirming. 7. **No `memory-region` or `qcom,vmids` on fastrpc nodes:** sm8750 has `memory-region` and `qcom,vmids` on its ADSP fastrpc node. Glymur does not. Again, possibly intentional for the EL2 use case. --- Generated by Claude Code Patch Reviewer