Re: [PATCH v2 1/3] dt-bindings: interconnect: Add schema for SM8550

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 24/11/2022 12:22, Abel Vesa wrote:
> Add dedicated schema file for SM8500. This allows better constraining
> of reg property, depending on the type of the NOC node. Also allows
> better constraining of the clocks property. All of the above while
> keeping the file itself comprehensible.
> 
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>  .../interconnect/qcom,sm8550-rpmh.yaml        | 141 ++++++++++++++++++
>  1 file changed, 141 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,sm8550-rpmh.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,sm8550-rpmh.yaml b/Documentation/devicetree/bindings/interconnect/qcom,sm8550-rpmh.yaml
> new file mode 100644
> index 000000000000..9627b629d4ce
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interconnect/qcom,sm8550-rpmh.yaml
> @@ -0,0 +1,141 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interconnect/qcom,sm8550-rpmh.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm RPMh Network-On-Chip Interconnect on SM8550
> +
> +maintainers:
> +  - Georgi Djakov <djakov@kernel.org>
> +  - Odelu Kukatla <okukatla@codeaurora.org>

I think this is not accurate email. Georgi also might not be interested
in SM8550 itself, so I propose add here yourself and Neil.

> +
> +description: |
> +   RPMh interconnect providers support system bandwidth requirements through
> +   RPMh hardware accelerators known as Bus Clock Manager (BCM). The provider is
> +   able to communicate with the BCM through the Resource State Coordinator (RSC)
> +   associated with each execution environment. Provider nodes must point to at
> +   least one RPMh device child node pertaining to their RSC and each provider
> +   can map to multiple RPMh resources.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,sm8550-aggre1-noc
> +      - qcom,sm8550-aggre2-noc
> +      - qcom,sm8550-clk-virt
> +      - qcom,sm8550-cnoc-main
> +      - qcom,sm8550-config-noc
> +      - qcom,sm8550-gem-noc
> +      - qcom,sm8550-lpass-ag-noc
> +      - qcom,sm8550-lpass-lpiaon-noc
> +      - qcom,sm8550-lpass-lpicx-noc
> +      - qcom,sm8550-mc-virt
> +      - qcom,sm8550-mmss-noc
> +      - qcom,sm8550-nsp-noc
> +      - qcom,sm8550-pcie-anoc
> +      - qcom,sm8550-system-noc
> +

reg:
  maxItems: 1

> +allOf:
> +  - $ref: qcom,rpmh-common.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,sm8550-aggre1-noc
> +              - qcom,sm8550-aggre2-noc
> +              - qcom,sm8550-cnoc-main
> +              - qcom,sm8550-config-noc
> +              - qcom,sm8550-gem-noc
> +              - qcom,sm8550-lpass-ag-noc
> +              - qcom,sm8550-lpass-lpiaon-noc
> +              - qcom,sm8550-lpass-lpicx-noc
> +              - qcom,sm8550-mmss-noc
> +              - qcom,sm8550-nsp-noc
> +              - qcom,sm8550-pcie-anoc
> +              - qcom,sm8550-system-noc
> +    then:
> +      properties:
> +        reg:
> +          minItems: 1
> +          maxItems: 1

Instead:

  if:
     ....

     enum:
       - virt interconnects
  then:
    properties:
      reg: false
  else:
    required:
      - reg


> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,sm8550-pcie-anoc
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: aggre-NOC PCIe AXI clock
> +            - description: cfg-NOC PCIe a-NOC AHB clock
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,sm8550-aggre1-noc
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: aggre UFS PHY AXI clock
> +            - description: aggre USB3 PRIM AXI clock
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,sm8550-aggre2-noc
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: RPMH CC IPA clock

This part is in general fine, but I propose to do it differently -
mentioning clocks in top-level properties, because it's defining
properties in allOf:if:then is error prone and easy to miss. Better to
have one definition and customization in if:then:.

Therefore:
1. in top-level properties:
  clocks:
    minItems: 1
    maxItems: 2

2. All your ifs stay the same.

3. One more if: compatible: enum: pcie/aggre1/aggre2

  then:
    required:
      - clocks
  else:
    properties:
      clocks: false


> +
> +required:
> +  - compatible
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +      #include <dt-bindings/clock/qcom,gcc-sm8550.h>
> +      #include <dt-bindings/interconnect/qcom,sm8550.h>
> +      #include <dt-bindings/clock/qcom,rpmh.h>
> +
> +      clk_virt: interconnect-0 {
> +             compatible = "qcom,sm8550-clk-virt";
> +             #interconnect-cells = <2>;
> +             qcom,bcm-voters = <&apps_bcm_voter>;
> +      };
> +
> +      cnoc_main: interconnect@1500000 {
> +             compatible = "qcom,sm8550-cnoc-main";
> +             reg = <0x01500000 0x13080>;
> +             #interconnect-cells = <2>;
> +             qcom,bcm-voters = <&apps_bcm_voter>;
> +      };
> +
> +      aggre1_noc: interconnect@16e0000 {
> +             compatible = "qcom,sm8550-aggre1-noc";
> +             reg = <0x016e0000 0x14400>;
> +             #interconnect-cells = <2>;
> +             clocks = <&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>,
> +                      <&gcc GCC_AGGRE_USB3_PRIM_AXI_CLK>;
> +             qcom,bcm-voters = <&apps_bcm_voter>;
> +      };
> +
> +      aggre2_noc: interconnect@1700000 {
> +             compatible = "qcom,sm8550-aggre2-noc";
> +             reg = <0x01700000 0x1E400>;
> +             #interconnect-cells = <2>;
> +             clocks = <&rpmhcc RPMH_IPA_CLK>;
> +             qcom,bcm-voters = <&apps_bcm_voter>;
> +      };

and keep just two examples (e.g. virt and aggre1) - they differ only by
one or two properties, so it's a bit too much...

Best regards,
Krzysztof