public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
From: Claude Code Review Bot <claude-review@example.com>
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	[thread overview]
Message-ID: <review-patch4-20260310033617.3108675-5-sibi.sankar@oss.qualcomm.com> (raw)
In-Reply-To: <20260310033617.3108675-5-sibi.sankar@oss.qualcomm.com>

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

  reply	other threads:[~2026-03-11  3:43 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-03-10  3:36 [PATCH V4 0/5] Enable ADSP and CDSP for Glymur SoC Sibi Sankar
2026-03-10  3:36 ` [PATCH V4 1/5] dt-bindings: remoteproc: qcom, sm8550-pas: Add Glymur ADSP Sibi Sankar
2026-03-10  7:50   ` [PATCH V4 1/5] dt-bindings: remoteproc: qcom,sm8550-pas: " Krzysztof Kozlowski
2026-03-10  9:17     ` Sibi Sankar
2026-03-11  3:43   ` Claude review: dt-bindings: remoteproc: qcom, sm8550-pas: " Claude Code Review Bot
2026-03-10  3:36 ` [PATCH V4 2/5] dt-bindings: remoteproc: qcom, sm8550-pas: Add Glymur CDSP Sibi Sankar
2026-03-11  3:43   ` Claude review: " Claude Code Review Bot
2026-03-10  3:36 ` [PATCH V4 3/5] dt-bindings: misc: qcom, fastrpc: Add compatible for Glymur Sibi Sankar
2026-03-10  7:52   ` [PATCH V4 3/5] dt-bindings: misc: qcom,fastrpc: " Krzysztof Kozlowski
2026-03-11  3:43   ` Claude review: dt-bindings: misc: qcom, fastrpc: " Claude Code Review Bot
2026-03-10  3:36 ` [PATCH V4 4/5] arm64: dts: qcom: glymur: Add ADSP and CDSP for Glymur SoC Sibi Sankar
2026-03-11  3:43   ` Claude Code Review Bot [this message]
2026-03-10  3:36 ` [PATCH V4 5/5] arm64: dts: qcom: glymur-crd: Enable ADSP and CDSP Sibi Sankar
2026-03-11  3:43   ` Claude review: " Claude Code Review Bot
2026-03-11  3:43 ` Claude review: Enable ADSP and CDSP for Glymur SoC Claude Code Review Bot
  -- strict thread matches above, loose matches on Subject: below --
2026-03-13 12:08 [PATCH V5 0/5] " Sibi Sankar
2026-03-13 12:08 ` [PATCH V5 4/5] arm64: dts: qcom: glymur: Add " Sibi Sankar
2026-03-13 21:07   ` Claude review: " Claude Code Review Bot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=review-patch4-20260310033617.3108675-5-sibi.sankar@oss.qualcomm.com \
    --to=claude-review@example.com \
    --cc=dri-devel-reviews@example.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox