Re: [RFC PATCH 1/2] dt-bindings: pcie: Document QCOM PCIE ECAM compatible root complex

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

On 04/04/2024 21:11, Mayank Rana wrote:
> On some of Qualcomm platform, firmware configures PCIe controller in RC

On which?

Your commit or binding must answer to all such questions.

> mode with static iATU window mappings of configuration space for entire
> supported bus range in ECAM compatible mode. Firmware also manages PCIe
> PHY as well required system resources. Here document properties and
> required configuration to power up QCOM PCIe ECAM compatible root complex
> and PHY for PCIe functionality.
> 
> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
> ---
>  .../devicetree/bindings/pci/qcom,pcie-ecam.yaml    | 94 ++++++++++++++++++++++
>  1 file changed, 94 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie-ecam.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ecam.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ecam.yaml
> new file mode 100644
> index 00000000..c209f12
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ecam.yaml
> @@ -0,0 +1,94 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/qcom,pcie-ecam.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm ECAM compliant PCI express root complex
> +
> +description: |
Do not need '|' unless you need to preserve formatting.


> +  Qualcomm SOC based ECAM compatible PCIe root complex supporting MSI controller.

Which SoC?

> +  Firmware configures PCIe controller in RC mode with static iATU window mappings
> +  of configuration space for entire supported bus range in ECAM compatible mode.
> +
> +maintainers:
> +  - Mayank Rana <quic_mrana@quicinc.com>
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-bus.yaml#
> +  - $ref: /schemas/power-domain/power-domain-consumer.yaml
> +
> +properties:
> +  compatible:
> +    const: qcom,pcie-ecam-rc

No, this must have SoC specific compatibles.

> +
> +  reg:
> +    minItems: 1

maxItems instead

> +    description: ECAM address space starting from root port till supported bus range
> +
> +  interrupts:
> +    minItems: 1
> +    maxItems: 8

This is way too unspecific.

> +
> +  ranges:
> +    minItems: 2
> +    maxItems: 3

Why variable?

> +
> +  iommu-map:
> +    minItems: 1
> +    maxItems: 16

Why variable?

Open existing bindings and look how it is done.


> +
> +  power-domains:
> +    maxItems: 1
> +    description: A phandle to node which is able support way to communicate with firmware
> +        for enabling PCIe controller and PHY as well managing all system resources needed to
> +        make both controller and PHY operational for PCIe functionality.

This description does not tell me much. Say something specific. And drop
redundant parts like phandle.


> +
> +  dma-coherent: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - ranges
> +  - power-domains
> +  - device_type
> +  - linux,pci-domain
> +  - bus-range
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +        pcie0: pci@1c00000 {
> +            compatible = "qcom,pcie-ecam-rc";
> +            reg = <0x4 0x00000000 0 0x10000000>;
> +            device_type = "pci";
> +            #address-cells = <3>;
> +            #size-cells = <2>;
> +            ranges = <0x01000000 0x0 0x40000000 0x0 0x40000000 0x0 0x100000>,
> +                <0x02000000 0x0 0x40100000 0x0 0x40100000 0x0 0x1ff00000>,
> +                <0x43000000 0x4 0x10100000 0x4 0x10100000 0x0 0x100000>;

Follow DTS coding style about placement and alignment.

Best regards,
Krzysztof