Re: [PATCH v4 1/3] dt-bindings: remoteproc: Move iommus constraint to SoC-specific bindings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dt-bindings: remoteproc: qcom,sm8550-pas: missing iommus for Glymur PAS
- [Low] dt-bindings: remoteproc: duplicated iommus rules and unwieldy conditionals
--

commit 95697c560209b6d85ebaddcbbc163e85e3f0686b
Author: Komal Bajaj <komal.bajaj@oss.qualcomm.com>

dt-bindings: remoteproc: Move iommus constraint to SoC-specific bindings

This commit removes the common iommus maxItems: 1 constraint from
qcom,pas-common.yaml and adds per-SoC iommus constraints to
qcom,sa8775p-pas.yaml, qcom,sm8150-pas.yaml, and qcom,sm8550-pas.yaml.

> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sa8775p-pas.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sa8775p-pas.yaml
> index dda2d144b7206..711c2f723b179 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,sa8775p-pas.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sa8775p-pas.yaml
> @@ -106,6 +106,8 @@ allOf:
>            items:
>              - const: lcx
>              - const: lmx
> +        iommus:
> +          maxItems: 1
>  
>    - if:
>        properties:
> @@ -126,6 +128,8 @@ allOf:
>              - const: cx
>              - const: mxc
>              - const: nsp
> +        iommus:
> +          maxItems: 1
>  
>    - if:
>        properties:
> @@ -144,6 +148,8 @@ allOf:
>            items:
>              - const: cx
>              - const: mxc
> +        iommus:
> +          maxItems: 1

[Severity: Low]
This isn't a bug, but since every compatible in
Documentation/devicetree/bindings/remoteproc/qcom,sa8775p-pas.yaml matches one
of these three allOf branches (covering qcom,sa8775p-adsp-pas,
qcom,sa8775p-cdsp0-pas, qcom,sa8775p-cdsp1-pas, qcom,sa8775p-gpdsp0-pas, and
qcom,sa8775p-gpdsp1-pas), could iommus with maxItems: 1 be defined once under
the top-level properties block instead of duplicating it across all three
allOf branches?

Also, in Documentation/devicetree/bindings/remoteproc/qcom,sm8150-pas.yaml and
Documentation/devicetree/bindings/remoteproc/qcom,sm8550-pas.yaml, adding more
per-compatible if-then blocks into allOf makes those single binding schemas
increasingly unwieldy as variant differences grow. Would it make sense to
split divergent variants into separate schema documents rather than piling on
more conditionals?

[ ... ]

> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sm8550-pas.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sm8550-pas.yaml
> index 99e9aeeb4f62c..dd81ecdb8dc49 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,sm8550-pas.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sm8550-pas.yaml
> @@ -317,6 +317,18 @@ allOf:
>              - const: mx
>              - const: nsp
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,x1e80100-adsp-pas
> +              - qcom,x1e80100-cdsp-pas
> +    then:
> +      properties:
> +        iommus:
> +          maxItems: 1

[Severity: Medium]
Does removing iommus from
Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml while only
allowing iommus for qcom,x1e80100-adsp-pas and qcom,x1e80100-cdsp-pas here
cause dtbs_check regressions for Glymur?

Documentation/devicetree/bindings/remoteproc/qcom,sm8550-pas.yaml also covers
qcom,glymur-adsp-pas and qcom,glymur-cdsp-pas and sets unevaluatedProperties:
false. In arch/arm64/boot/dts/qcom/glymur.dtsi, both remoteproc_adsp and
remoteproc_cdsp specify iommus:

    remoteproc_adsp: remoteproc@6800000 {
        compatible = "qcom,glymur-adsp-pas", "qcom,sm8550-adsp-pas";
        reg = <0x0 0x06800000 0x0 0x10000>;

        iommus = <&apps_smmu 0x1000 0x0>;
        ...
    };

    remoteproc_cdsp: remoteproc@32300000 {
        compatible = "qcom,glymur-cdsp-pas", "qcom,sm8550-cdsp-pas";
        reg = <0x0 0x32300000 0x0 0x10000>;

        iommus = <&apps_smmu 0x2400 0x400>;
        ...
    };

Should qcom,glymur-adsp-pas and qcom,glymur-cdsp-pas also be included in this
conditional so iommus remains evaluated on those nodes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-shikra-el2-v4-0-97e7019debff@oss.qualcomm.com?part=1