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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxx>
---
  .../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@xxxxxxxxxxx>
+
+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




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux