diff mbox series

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

Message ID 1712257884-23841-2-git-send-email-quic_mrana@quicinc.com (mailing list archive)
State RFC
Delegated to: Manivannan Sadhasivam
Headers show
Series Add Qualcomm PCIe ECAM root complex driver | expand

Commit Message

Mayank Rana April 4, 2024, 7:11 p.m. UTC
On some of Qualcomm platform, firmware configures PCIe controller in RC
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

Comments

Krzysztof Kozlowski April 4, 2024, 7:30 p.m. UTC | #1
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
Mayank Rana April 8, 2024, 7:09 p.m. UTC | #2
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
Krzysztof Kozlowski April 9, 2024, 6:21 a.m. UTC | #3
On 08/04/2024 21:09, Mayank Rana wrote:
>>> +  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 ?

What does the writing-bindings document say? Why this is different than
all other bindings?

>>> +
>>> +  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.

Really? So same device has it configured once for 32 once for 64-bit
address space? Randomly?

Best regards,
Krzysztof
Mayank Rana April 18, 2024, 6:56 p.m. UTC | #4
On 4/8/2024 11:21 PM, Krzysztof Kozlowski wrote:
> On 08/04/2024 21:09, Mayank Rana wrote:
>>>> +  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 ?
> 
> What does the writing-bindings document say? Why this is different than
> all other bindings?
Thank you for all your review comment and suggestions.

If it is must to have SOC name, then I am not sure how 
pci-host-generic.c driver having non SOC prefix for standard ECAM 
driver. I am here saying this is QCOM vendor specific generic ECAM 
driver. saying that it seems that I would be updating now 
pci-host-generic.c driver to add generic functionality as Rob suggested 
part of review comment. With
that I am seeing possible options as i.e. continue using default generic 
compatible as "pcie-host-ecam-generic" OR use new as 
"qcom,pcie-host-ecam-generic". will this work ?>>>> +
>>>> +  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.
> 
> Really? So same device has it configured once for 32 once for 64-bit
> address space? Randomly?
no. as binding is not saying for any specific SOC. Depends on memory map
on particular SOC, how PCIe address space available based on that this 
would change for particular SOC variant.
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski April 18, 2024, 8:53 p.m. UTC | #5
On 18/04/2024 20:56, Mayank Rana wrote:
> 
> 
> On 4/8/2024 11:21 PM, Krzysztof Kozlowski wrote:
>> On 08/04/2024 21:09, Mayank Rana wrote:
>>>>> +  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 ?
>>
>> What does the writing-bindings document say? Why this is different than
>> all other bindings?
> Thank you for all your review comment and suggestions.
> 
> If it is must to have SOC name, then I am not sure how 
> pci-host-generic.c driver having non SOC prefix for standard ECAM 
> driver. I am here saying this is QCOM vendor specific generic ECAM 
> driver. saying that it seems that I would be updating now 
> pci-host-generic.c driver to add generic functionality as Rob suggested 

I don't see any problem here. I talk about bindings, not driver. You can
have also fallback, so how is it different than from existing code?

> part of review comment. With
> that I am seeing possible options as i.e. continue using default generic 
> compatible as "pcie-host-ecam-generic" OR use new as 
> "qcom,pcie-host-ecam-generic". will this work ?>>>> +

Compatible and bindings focus on the hardware, so just write them
describing the hardware. You keep asking it in context of driver, but I
would say it does not matter. Is this generic hardware/firmware
implementation or not?

>>>>> +  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.
>>
>> Really? So same device has it configured once for 32 once for 64-bit
>> address space? Randomly?
> no. as binding is not saying for any specific SOC. Depends on memory map
> on particular SOC, how PCIe address space available based on that this 

So specific to the SoC, so this is not variable.

> would change for particular SOC variant.

So this is not variable and you did not provide sufficient
argumentation. You basically did not provide any argument, just
disagreed with me. Bindings must be specific and all fields should be
constrained, when reasonable.

Best regards,
Krzysztof
diff mbox series

Patch

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: |
+  Qualcomm SOC based ECAM compatible PCIe root complex supporting MSI controller.
+  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
+
+  reg:
+    minItems: 1
+    description: ECAM address space starting from root port till supported bus range
+
+  interrupts:
+    minItems: 1
+    maxItems: 8
+
+  ranges:
+    minItems: 2
+    maxItems: 3
+
+  iommu-map:
+    minItems: 1
+    maxItems: 16
+
+  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.
+
+  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>;
+            bus-range = <0x00 0xff>;
+            dma-coherent;
+            linux,pci-domain = <0>;
+            power-domains = <&scmi5_pd 0>;
+            power-domain-names = "power";
+            iommu-map = <0x0 &pcie_smmu 0x0000 0x1>,
+                  <0x100 &pcie_smmu 0x0001 0x1>;
+            interrupt-parent = <&intc>;
+            interrupts = <GIC_SPI 307 IRQ_TYPE_LEVEL_HIGH>,
+                <GIC_SPI 308 IRQ_TYPE_LEVEL_HIGH>,
+                <GIC_SPI 309 IRQ_TYPE_LEVEL_HIGH>,
+                <GIC_SPI 312 IRQ_TYPE_LEVEL_HIGH>,
+                <GIC_SPI 313 IRQ_TYPE_LEVEL_HIGH>,
+                <GIC_SPI 314 IRQ_TYPE_LEVEL_HIGH>,
+                <GIC_SPI 374 IRQ_TYPE_LEVEL_HIGH>,
+                <GIC_SPI 375 IRQ_TYPE_LEVEL_HIGH>;
+        };
+    };
+...