Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6e91cf76-bb2d-5f4c-6648-59e666a71e9d@linaro.org>
Date:   Sat, 23 Apr 2022 21:55:22 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH 2/4] dt-bindings: interconnect: Add Qualcomm SM6350 NoC
 support
Content-Language: en-US
To:     Luca Weiss <luca.weiss@fairphone.com>,
        linux-arm-msm@vger.kernel.org
Cc:     ~postmarketos/upstreaming@lists.sr.ht, phone-devel@vger.kernel.org,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Georgi Djakov <djakov@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Odelu Kukatla <okukatla@codeaurora.org>,
        linux-pm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220422144021.232993-1-luca.weiss@fairphone.com>
 <20220422144021.232993-3-luca.weiss@fairphone.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20220422144021.232993-3-luca.weiss@fairphone.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 22/04/2022 16:40, Luca Weiss wrote:
> Add bindings for Qualcomm SM6350 Network-On-Chip interconnect devices.
> 
> As SM6350 has two pairs of NoCs sharing the same reg, allow this in the
> binding documentation, as was done for qcm2290.
> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>  .../bindings/interconnect/qcom,rpmh.yaml      |  44 ++++++
>  .../dt-bindings/interconnect/qcom,sm6350.h    | 148 ++++++++++++++++++
>  2 files changed, 192 insertions(+)
>  create mode 100644 include/dt-bindings/interconnect/qcom,sm6350.h
> 
> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml b/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml
> index 5a911be0c2ea..797107212a42 100644
> --- a/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml
> +++ b/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml
> @@ -72,6 +72,14 @@ properties:
>        - qcom,sdx55-mc-virt
>        - qcom,sdx55-mem-noc
>        - qcom,sdx55-system-noc
> +      - qcom,sm6350-aggre1-noc
> +      - qcom,sm6350-aggre2-noc
> +      - qcom,sm6350-config-noc
> +      - qcom,sm6350-dc-noc
> +      - qcom,sm6350-gem-noc
> +      - qcom,sm6350-mmss-noc
> +      - qcom,sm6350-npu-noc
> +      - qcom,sm6350-system-noc
>        - qcom,sm8150-aggre1-noc
>        - qcom,sm8150-aggre2-noc
>        - qcom,sm8150-camnoc-noc
> @@ -131,6 +139,42 @@ properties:
>      description: |
>        Names for each of the qcom,bcm-voters specified.
>  
> +# Child node's properties
> +patternProperties:
> +  '^interconnect-[a-z0-9\-]+$':
> +    type: object
> +    description:
> +      The interconnect providers do not have a separate QoS register space,
> +      but share parent's space.
> +
> +    properties:
> +      compatible:
> +        enum:
> +          - qcom,sm6350-clk-virt
> +          - qcom,sm6350-compute-noc
> +
> +      '#interconnect-cells':
> +        const: 1
> +
> +      qcom,bcm-voters:
> +        $ref: /schemas/types.yaml#/definitions/phandle-array

You need maxItems for the entire list, so how many phandles could be
here. You only defined how the each phandle looks like.

> +        items:
> +          maxItems: 1
> +        description: |
> +          List of phandles to qcom,bcm-voter nodes that are required by
> +          this interconnect to send RPMh commands.
> +
> +      qcom,bcm-voter-names:
> +        description: |
> +          Names for each of the qcom,bcm-voters specified.
> +
> +    required:
> +      - compatible
> +      - '#interconnect-cells'
> +      - qcom,bcm-voters
> +
> +    additionalProperties: false

You need to add allOf:if:then constraints where these children can
appear. If we want to be strict with bindings, this could be a new
binding, only for SM6350, using common qcom-interconnect binding.

The problem is that you allow now any other variant to have
qcom,sm6350-clk-virt|qcom,sm6350-compute-noc children which obviously is
wrong, but someone might want to re-use your compatibles.

Probably we hit the limitation of this Qcom bindings here - they are
just too big, too many compatibles.

Instead maybe let's define:
1. Common Qcom interconnect rpmh part.
2. Reference it from qcom,rpmh.yaml.
3. Reference it from SM6350 rpmh and customize the children.

> +
>  required:
>    - compatible
>    - reg
> diff --git a/include/dt-bindings/interconnect/qcom,sm6350.h b/include/dt-bindings/interconnect/qcom,sm6350.h
> new file mode 100644
> index 000000000000..c6eab5b42858
> --- /dev/null
> +++ b/include/dt-bindings/interconnect/qcom,sm6350.h
> @@ -0,0 +1,148 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

Same license as bindings, so GPL-2.0 or BSD-2-clause


Best regards,
Krzysztof