On Thu, Sep 04, 2025 at 04:10:23PM GMT, zhangsenchuan wrote:

quoted

-----Original Messages-----
From: "Manivannan Sadhasivam" <mani@kernel.org>
Send time:Monday, 01/09/2025 14:04:50
To: zhangsenchuan@eswincomputing.com
Cc: bhelgaas@google.com, lpieralisi@kernel.org, kwilczynski@kernel.org, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, linux-pci@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, p.zabel@pengutronix.de, johan+linaro@kernel.org, quic_schintav@quicinc.com, shradha.t@samsung.com, cassel@kernel.org, thippeswamy.havalige@amd.com, mayank.rana@oss.qualcomm.com, inochiama@gmail.com, ningyu@eswincomputing.com, linmin@eswincomputing.com, pinkesh.vaghela@einfochips.com
Subject: Re: [PATCH v2 1/2] dt-bindings: PCI: eic7700: Add Eswin eic7700 PCIe host controller

On Fri, Aug 29, 2025 at 04:22:37PM GMT, zhangsenchuan@eswincomputing.com wrote:

quoted

From: Senchuan Zhang <zhangsenchuan@eswincomputing.com>

Add Device Tree binding documentation for the ESWIN EIC7700
PCIe controller module,the PCIe controller enables the core
to correctly initialize and manage the PCIe bus and connected
devices.

Signed-off-by: Yu Ning <redacted>
Signed-off-by: Senchuan Zhang <zhangsenchuan@eswincomputing.com>
---
 .../bindings/pci/eswin,eic7700-pcie.yaml      | 142 ++++++++++++++++++
 1 file changed, 142 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/eswin,eic7700-pcie.yaml

diff --git a/Documentation/devicetree/bindings/pci/eswin,eic7700-pcie.yaml b/Documentation/devicetree/bindings/pci/eswin,eic7700-pcie.yaml
new file mode 100644
index 000000000000..65f640902b11
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/eswin,eic7700-pcie.yaml

@@ -0,0 +1,142 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/eswin,eic7700-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Eswin EIC7700 PCIe host controller
+
+maintainers:
+  - Yu Ning <ningyu@eswincomputing.com>
+  - Senchuan Zhang <zhangsenchuan@eswincomputing.com>
+
+description:
+  The PCIe controller on EIC7700 SoC.
+
+allOf:
+  - $ref: /schemas/pci/pci-host-bridge.yaml#
+
+properties:
+  compatible:
+    const: eswin,eic7700-pcie
+
+  reg:
+    maxItems: 3
+
+  reg-names:
+    items:
+      - const: dbi
+      - const: config
+      - const: mgmt
+
+  ranges:
+    maxItems: 3
+
+  num-lanes:
+    const: 4
+
+  '#interrupt-cells':
+    const: 1
+
+  interrupts:
+    maxItems: 9
+
+  interrupt-names:
+    items:
+      - const: msi
+      - const: inta
+      - const: intb
+      - const: intc
+      - const: intd
+      - const: inte
+      - const: intf
+      - const: intg
+      - const: inth

What? Are these standard INTx or something elese? PCI(e) spec defines only
INT{A-D}.

Dear Manivannan

Thank you for your thorough review .
You are right, the PCI(e) spec defines only four legacy INTx interrupts (INTA#, INTB#, INTC#,  INTD#). 
PCI(e) spec defines also mentions that INTX interrupts have two control states (Assert_INTx/Deassert_INTx Message).
In our yaml, inta~intd corresponds to Assert_INTA~Assert_INTD, and inte~inth corresponds to Deassert_INTA~Deassert_INTD. 
May I ask if inte~inth needs to be removed or if the naming needs to be standardized? 
I saw that in "sifive,fu740-pcie.yaml", interrupt-names only retain inta to intd.

If these are actual interrupts and these names align with the interrupt names in
the hardware IP, then you can keep it as it is and should add a comment for
inte-inth:
		- const: inte /* INTA_Deassert */
		...

If not, then you should remove inte-inth.

quoted

quoted

+
+  interrupt-map:
+    maxItems: 4
+
+  interrupt-map-mask:
+    items:
+      - const: 0
+      - const: 0
+      - const: 0
+      - const: 7
+
+  clocks:
+    maxItems: 4
+
+  clock-names:
+    items:
+      - const: mstr
+      - const: dbi
+      - const: pclk
+      - const: aux
+
+  resets:
+    maxItems: 3
+
+  reset-names:
+    items:
+      - const: cfg
+      - const: powerup
+      - const: pwren
+
+required:
+  - compatible
+  - reg
+  - ranges
+  - num-lanes
+  - interrupts
+  - interrupt-names
+  - interrupt-map-mask
+  - interrupt-map
+  - '#interrupt-cells'
+  - clocks
+  - clock-names
+  - resets
+  - reset-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        pcie@54000000 {
+            compatible = "eswin,eic7700-pcie";
+            reg = <0x0 0x54000000 0x0 0x4000000>,
+                  <0x0 0x40000000 0x0 0x800000>,
+                  <0x0 0x50000000 0x0 0x100000>;
+            reg-names = "dbi", "config", "mgmt";
+            #address-cells = <3>;
+            #size-cells = <2>;
+            #interrupt-cells = <1>;
+            ranges = <0x81000000 0x0 0x40800000 0x0 0x40800000 0x0 0x800000>,

I/O CPU range starts from 0x0

Also, I don't think you need to set the relocatable flag (bit 31) for any
regions.

if cannot set the relocatable flag (bit 31) for any regions.Is it appropriate to write it this way：
ranges = <0x01000000 0x0 0x40800000 0x0 0x40800000 0x0 0x800000>,
         <0x02000000 0x0 0x41000000 0x0 0x41000000 0x0 0xf000000>,
         <0x43000000 0x80 0x00000000 0x80 0x00000000 0x2 0x00000000>;

Yes.

quoted

quoted

+                     <0x82000000 0x0 0x41000000 0x0 0x41000000 0x0 0xf000000>,
+                     <0xc3000000 0x80 0x00000000 0x80 0x00000000 0x2 0x00000000>;
+            bus-range = <0x0 0xff>;
+            clocks = <&clock 562>,
+                     <&clock 563>,
+                     <&clock 564>,
+                     <&clock 565>;

Don't you have clock definitions for these values?

Our clock and reset drivers have the definitions of these values, but the clock and reset drivers are under review. 
Currently, these values can only be replaced by constants.

Ok, you should also mention this in description or cover letter.

- Mani

-- 
மணிவண்ணன் சதாசிவம்