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

[Mayank Rana <quic_mrana@quicinc.com>]

Hi Krzysztof

On 4/4/2024 12:30 PM, Krzysztof Kozlowski wrote:
> 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.
ACK
> 
>> +  Qualcomm SOC based ECAM compatible PCIe root complex supporting MSI controller.
> 
> Which SoC?
ACK
>> +  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.
This driver is proposed to work with any PCIe controller supported ECAM 
functionality on Qualcomm platform
where firmware running on other VM/processor is controlling PCIe PHY and 
controller for PCIe link up functionality.
Do you still suggest to have SoC specific compatibles here ?
>> +
>> +  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.
will review and update.
>> +
>> +  ranges:
>> +    minItems: 2
>> +    maxItems: 3
> 
> Why variable?
It depends on how ECAM configured to support 32-bit and 64-bit based 
prefetch address space.
So there are different combination of prefetch (32-bit or 64-bit or 
both) and non-prefetch (32-bit), and IO address space available. hence 
kept it as variable with based on required use case and address space 
availability.
>> +
>> +  iommu-map:
>> +    minItems: 1
>> +    maxItems: 16
> 
> Why variable?
> 
> Open existing bindings and look how it is done.
ok. will review and update as needed.
> 
>> +
>> +  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.
ok. will rephrase it.
> 
>> +
>> +  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
> 
Regards,
Mayank