-----Original Messages-----
From: "Krzysztof Kozlowski" <krzk@kernel.org>
Send time:Monday, 01/09/2025 13:19:04
To: zhangsenchuan@eswincomputing.com
Cc: bhelgaas@google.com, lpieralisi@kernel.org, kwilczynski@kernel.org, mani@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 +0800, zhangsenchuan@eswincomputing.com wrote:

quoted

+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

If that's const, you do not need it. It's implied by the compatible.
I see some other bindings do similarly and I think that's not the
correct choice.

Well, maybe @Rob knows if PCI is different here anyhow?

Dear Krzysztof

Thank you very much for your review.
You're right，If that's const,  I don't think it's necessary either.
After investigation, the description of the "num-lanes" attribute here 
is incorrect. The correct one should be the following description:
    num-lanes:
      maximum: 4
If so, the num-lanes attribute should need to be described here.
What do you think?

quoted

+
+  '#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
+
+  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>,
+                     <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>;
+            clock-names = "mstr", "dbi", "pclk", "aux";
+            resets = <&reset 8 (1 << 0)>,
+                     <&reset 8 (1 << 1)>,
+                     <&reset 8 (1 << 2)>;
+            reset-names = "cfg", "powerup", "pwren";
+            interrupts = <220>, <179>, <180>, <181>, <182>, <183>, <184>, <185>, <186>;
+            interrupt-names = "msi", "inta", "intb", "intc", "intd",
+                              "inte", "intf", "intg", "inth";
+            interrupt-parent = <&plic>;
+            interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+            interrupt-map = <0x0 0x0 0x0 0x1 &plic 179>,
+                            <0x0 0x0 0x0 0x2 &plic 180>,
+                            <0x0 0x0 0x0 0x3 &plic 181>,
+                            <0x0 0x0 0x0 0x4 &plic 182>;
+            device_type = "pci";
+            num-lanes = <0x4>;

That's not a hex number, but decimal.

Reviewed-by: Krzysztof Kozlowski <redacted>

Manivannan and Bjorn have provided me with some excellent suggestions for my yaml. 
My yaml will be refactored in the next patch, and I might need you to review it 
again for me in the next patch. I'm a little wondering if I need to add
"Reviewed-by: Krzysztof Kozlowski [off-list ref]" in next patch.
May I ask for your opinion?


Best regards,
Senchuan zhang