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 <ningyu@eswincomputing.com>
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
+
+ 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>;
+ };
+ };
--
2.25.1On Fri, Aug 29, 2025 at 04:22:37PM +0800, zhangsenchuan@eswincomputing.com wrote:
> 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 <ningyu@eswincomputing.com>
> 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
> +
> + 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>;
num-lanes and perst are per-Root Port items. Please put anything
related specifically to the Root Port in its own stanza to make it
easier to support multiple Root Ports in future versions of the
hardware.
See
https://lore.kernel.org/linux-pci/20250625221653.GA1590146@bhelgaas/
for examples of how to do this.
> + };
> + };
> --
> 2.25.1
>
> -----Original Messages-----
> From: "Bjorn Helgaas" <helgaas@kernel.org>
> Send time:Friday, 05/09/2025 00:06:00
> 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:
> > 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 <ningyu@eswincomputing.com>
> > 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
> > +
> > + 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>;
>
> num-lanes and perst are per-Root Port items. Please put anything
> related specifically to the Root Port in its own stanza to make it
> easier to support multiple Root Ports in future versions of the
> hardware.
>
> See
> https://lore.kernel.org/linux-pci/20250625221653.GA1590146@bhelgaas/
> for examples of how to do this.
Dear Bjorn
Thank you very much for your review.
I think the suggestions you put forward are very good，I placed perst in the
root port as per your suggestion.
I'm a bit confused about the "num-lanes" attribute.
The "num-lanes" attribute will be parsed in the "pcie-designware.c" file. In the
"pcie-designware-host.c" file, When our driver calls the dw_pcie_host_init function
for initialization, the attribute "num_lanes" will be judged. If the attribute is
available, use the value parsed from the device tree. If the attribute cannot be
obtained from the node, the lanes supported by the hardware default will be obtained
by reading the register.Can I avoid reparsing the num-lanes attribute?
I saw vendors based on Synopsys implementation. They separated the root port node
and did not place "num-lanes" in the root port node.
For examples:
hisilicon,kirin-pcie.yaml
qcom,pcie-sc7280.yaml
qcom,pcie-sa8255p.yaml
Is it allowed for me not to place "num-lanes" in the root port node?
Looking forward to your reply, thanks!
Best regards,
Senchuan zhang
On Thu, Sep 18, 2025 at 01:35:40PM +0800, zhangsenchuan wrote: > > -----Original Messages----- > > From: "Bjorn Helgaas" <helgaas@kernel.org> > > On Fri, Aug 29, 2025 at 04:22:37PM +0800, zhangsenchuan@eswincomputing.com wrote: > > > 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. > > > + 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>; > > > > num-lanes and perst are per-Root Port items. Please put anything > > related specifically to the Root Port in its own stanza to make it > > easier to support multiple Root Ports in future versions of the > > hardware. > > > > See > > https://lore.kernel.org/linux-pci/20250625221653.GA1590146@bhelgaas/ > > for examples of how to do this. > > Thank you very much for your review. > I think the suggestions you put forward are very good，I placed > perst in the root port as per your suggestion. > > I'm a bit confused about the "num-lanes" attribute. The "num-lanes" > attribute will be parsed in the "pcie-designware.c" file. In the > "pcie-designware-host.c" file, When our driver calls the > dw_pcie_host_init function for initialization, the attribute > "num_lanes" will be judged. If the attribute is available, use the > value parsed from the device tree. If the attribute cannot be > obtained from the node, the lanes supported by the hardware default > will be obtained by reading the register.Can I avoid reparsing the > num-lanes attribute? > > I saw vendors based on Synopsys implementation. They separated the > root port node and did not place "num-lanes" in the root port node. > For examples: > hisilicon,kirin-pcie.yaml > qcom,pcie-sc7280.yaml > qcom,pcie-sa8255p.yaml This is currently a problem because the DWC core doesn't know to look for "num-lanes" in a Root Port node. Similar situation in the NXP driver: https://lore.kernel.org/r/20250917212833.GA1873293@bhelgaas Would it work for you to add a Root Port parser in eic7700, similar to mvebu_pcie_parse_port() or qcom_pcie_parse_port() that would get "num-lanes"? It looks like that would keep the DWC core from setting num-lanes. Eventually the DWC core should look first for a Root Port node before falling back to the current behavior of looking in the host bridge node. If/when that happens, we should be able to remove the num-lanes parsing in eic7700 and similar drivers. I'd like to separate the per-Root Port things in the devicetree from the beginning because once devicetrees are out in the world, we basically have to support their structure forever. Bjorn
On Fri, Aug 29, 2025 at 04:22:37PM GMT, zhangsenchuan@eswincomputing.com wrote:
> 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 <ningyu@eswincomputing.com>
> 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}.
> +
> + 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.
> + <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?
> + clock-names = "mstr", "dbi", "pclk", "aux";
> + resets = <&reset 8 (1 << 0)>,
> + <&reset 8 (1 << 1)>,
> + <&reset 8 (1 << 2)>;
Same here.
> + 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>;
nit: Most of the bindings define num-lanes as decimal.
- Mani
--
மணிவண்ணன் சதாசிவம்
> -----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:
> > 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 <ningyu@eswincomputing.com>
> > 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.
> > +
> > + 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>;
>
> > + <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.
> > + clock-names = "mstr", "dbi", "pclk", "aux";
> > + resets = <&reset 8 (1 << 0)>,
> > + <&reset 8 (1 << 1)>,
> > + <&reset 8 (1 << 2)>;
>
> Same here.
>
> > + 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>;
>
> nit: Most of the bindings define num-lanes as decimal.
>
Best Regards,
Senchuan Zhang
On Thu, Sep 04, 2025 at 04:10:23PM GMT, zhangsenchuan wrote:
>
>
>
> > -----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:
> > > 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 <ningyu@eswincomputing.com>
> > > 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.
> > > +
> > > + 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.
> >
> > > + <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
--
மணிவண்ணன் சதாசிவம்
On Fri, Aug 29, 2025 at 04:22:37PM +0800, zhangsenchuan@eswincomputing.com wrote:
> +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?
> +
> + '#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 <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
> -----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:
> > +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?
>
> > +
> > + '#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 <krzysztof.kozlowski@linaro.org>
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 <krzysztof.kozlowski@linaro.org>" in next patch.
May I ask for your opinion?
Best regards,
Senchuan zhang
On 18/09/2025 12:15, zhangsenchuan wrote: >>> + 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 1, 2 or 4 lanes are correct, for this compatible, then yes. > If so, the num-lanes attribute should need to be described here. > What do you think? > ... >>> + 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 <krzysztof.kozlowski@linaro.org> > > 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 <krzysztof.kozlowski@linaro.org>" in next patch. If you are changing it significantly, then drop/ignore my tag and write in the changelog reasons why the tag was dropped. Usually adding new properties is a significant change. Changing some clock name from A to B is not a significant change. Other cases vary. More important is to explain the differences and reason of dropping tag. Best regards, Krzysztof
© 2016 - 2025 Red Hat, Inc.