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

From: Krzysztof Kozlowski
Date: Thu Nov 24 2022 - 07:07:11 EST

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@xxxxxxxxxx>
> ---
> .../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@xxxxxxxxxx>
> + - Odelu Kukatla <okukatla@xxxxxxxxxxxxxx>

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