[PATCH v2 0/2] Add driver support for Eswin EIC7700 SoC PCIe controller

[PATCH v2 0/2] Add driver support for Eswin EIC7700 SoC PCIe controller

[zhangsenchuan]

From: Senchuan Zhang <zhangsenchuan@eswincomputing.com>

This series depends on the vendor prefix [1] and config option patch [2].

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20250825&id=ac29e4487aa20a21b7c3facbd1f14f5093835dc9
[2] https://lore.kernel.org/all/20250825132427.1618089-3-pinkesh.vaghela@einfochips.com/

Changes in v2:
- Updates: eswin,eic7700-pcie.yaml
  - Optimize the naming of "clock-names" and "reset-names".
  - Add a reference to "$ref: /schemas/pci/pci-host-bridge.yaml#".
    (The name of the reset attribute in the "snps,dw-pcie-common.yaml"
    file is different from our reset attribute and "snps,dw-pcie.yaml"
    file cannot be directly referenced)
  - Follow DTS coding style to optimize yaml attributes.
  - Remove status = "disabled" from yaml.

- Updates: pcie-eic7700.c
  - Remove unnecessary imported header files.
  - Use dev_err instead of pr_err and remove the WARN_ON function.
  - The eswin_evb_socket_power_on function is removed and not supported.
  - The eswin_pcie_remove function is placed after the probe function.
  - Optimize function alignment.
  - Manage the clock using the devm_clk_bulk_get_all_enabled function.
  - Handle the release of resources after the dw_pcie_host_init function
    call fails.
  - Remove the dev_dbg function and remove __exit_p.
  - Add support for the system pm function.
- Link to V1: https://lore.kernel.org/all/20250516094057.1300-1-zhangsenchuan@eswincomputing.com/

Senchuan Zhang (2):
  dt-bindings: PCI: eic7700: Add Eswin eic7700 PCIe host controller
  PCI: eic7700: Add Eswin eic7700 PCIe host controller driver

 .../bindings/pci/eswin,eic7700-pcie.yaml      | 142 +++++++
 drivers/pci/controller/dwc/Kconfig            |  12 +
 drivers/pci/controller/dwc/Makefile           |   1 +
 drivers/pci/controller/dwc/pcie-eic7700.c     | 350 ++++++++++++++++++
 4 files changed, 505 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/eswin,eic7700-pcie.yaml
 create mode 100644 drivers/pci/controller/dwc/pcie-eic7700.c

--
2.25.1

[PATCH v2 1/2] dt-bindings: PCI: eic7700: Add Eswin eic7700 PCIe host controller

[zhangsenchuan]

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

Re: [PATCH v2 1/2] dt-bindings: PCI: eic7700: Add Eswin eic7700 PCIe host controller

[Krzysztof Kozlowski]

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

Re: [PATCH v2 1/2] dt-bindings: PCI: eic7700: Add Eswin eic7700 PCIe host controller

[Manivannan Sadhasivam]

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

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

Re: Re: [PATCH v2 1/2] dt-bindings: PCI: eic7700: Add Eswin eic7700 PCIe host controller

[zhangsenchuan]

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

[PATCH v2 2/2] PCI: eic7700: Add Eswin eic7700 PCIe host controller driver

[zhangsenchuan]

From: Senchuan Zhang <zhangsenchuan@eswincomputing.com>

Add driver for the Eswin EIC7700 PCIe host controller.
The controller is based on the DesignWare PCIe core.

Signed-off-by: Yu Ning <ningyu@eswincomputing.com>
Signed-off-by: Senchuan Zhang<zhangsenchuan@eswincomputing.com>
---
 drivers/pci/controller/dwc/Kconfig        |  12 +
 drivers/pci/controller/dwc/Makefile       |   1 +
 drivers/pci/controller/dwc/pcie-eic7700.c | 350 ++++++++++++++++++++++
 3 files changed, 363 insertions(+)
 create mode 100644 drivers/pci/controller/dwc/pcie-eic7700.c

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index ff6b6d9e18ec..1c4063107a8a 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -492,4 +492,16 @@ config PCIE_VISCONTI_HOST
 	  Say Y here if you want PCIe controller support on Toshiba Visconti SoC.
 	  This driver supports TMPV7708 SoC.

+config PCIE_EIC7700
+	tristate "ESWIN PCIe host controller"
+	depends on PCI_MSI
+	depends on ARCH_ESWIN || COMPILE_TEST
+	select PCIE_DW_HOST
+	help
+	  Enables support for the PCIe controller in the Eswin SoC
+	  The PCI controller on Eswin is based on DesignWare hardware
+	  It is a high-speed hardware bus standard used to connect
+	  processors with external devices. Say Y here if you want
+	  PCIe controller support for the ESWIN.
+
 endmenu
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index 6919d27798d1..0717fe73a2a9 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -31,6 +31,7 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
 obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
 obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o
 obj-$(CONFIG_PCIE_RCAR_GEN4) += pcie-rcar-gen4.o
+obj-$(CONFIG_PCIE_EIC7700) += pcie-eic7700.o

 # The following drivers are for devices that use the generic ACPI
 # pci_root.c driver but don't support standard ECAM config access.
diff --git a/drivers/pci/controller/dwc/pcie-eic7700.c b/drivers/pci/controller/dwc/pcie-eic7700.c
new file mode 100644
index 000000000000..bf942154d971
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-eic7700.c
@@ -0,0 +1,350 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ESWIN PCIe root complex driver
+ *
+ * Copyright 2025, Beijing ESWIN Computing Technology Co., Ltd.
+ *
+ * Authors: Yu Ning <ningyu@eswincomputing.com>
+ *          Senchuan Zhang <zhangsenchuan@eswincomputing.com>
+ */
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/resource.h>
+#include <linux/types.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/reset.h>
+#include <linux/pm_runtime.h>
+
+#include "pcie-designware.h"
+
+struct eswin_pcie {
+	struct dw_pcie pci;
+	void __iomem *mgmt_base;
+	struct clk_bulk_data *clks;
+	struct reset_control *powerup_rst;
+	struct reset_control *cfg_rst;
+	struct reset_control *perst;
+
+	int num_clks;
+};
+
+#define PCIE_PM_SEL_AUX_CLK BIT(16)
+#define PCIEMGMT_APP_LTSSM_ENABLE BIT(5)
+
+#define PCIEMGMT_CTRL0_OFFSET 0x0
+#define PCIEMGMT_STATUS0_OFFSET 0x100
+
+#define PCIE_TYPE_DEV_VEND_ID 0x0
+#define PCIE_DSP_PF0_MSI_CAP 0x50
+#define PCIE_NEXT_CAP_PTR 0x70
+#define DEVICE_CONTROL_DEVICE_STATUS 0x78
+
+#define PCIE_MSI_MULTIPLE_MSG_32 (0x5 << 17)
+#define PCIE_MSI_MULTIPLE_MSG_MASK (0x7 << 17)
+
+#define PCIEMGMT_LINKUP_STATE_VALIDATE ((0x11 << 2) | 0x3)
+#define PCIEMGMT_LINKUP_STATE_MASK 0xff
+
+static int eswin_pcie_start_link(struct dw_pcie *pci)
+{
+	struct device *dev = pci->dev;
+	struct eswin_pcie *pcie = dev_get_drvdata(dev);
+	u32 val;
+
+	/* Enable LTSSM */
+	val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
+	val |= PCIEMGMT_APP_LTSSM_ENABLE;
+	writel_relaxed(val, pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
+
+	return 0;
+}
+
+static bool eswin_pcie_link_up(struct dw_pcie *pci)
+{
+	struct device *dev = pci->dev;
+	struct eswin_pcie *pcie = dev_get_drvdata(dev);
+	u32 val;
+
+	val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_STATUS0_OFFSET);
+
+	return ((val & PCIEMGMT_LINKUP_STATE_MASK) ==
+		     PCIEMGMT_LINKUP_STATE_VALIDATE);
+}
+
+static int eswin_pcie_power_on(struct eswin_pcie *pcie)
+{
+	int ret = 0;
+
+	/* pciet_cfg_rstn */
+	ret = reset_control_deassert(pcie->cfg_rst);
+	if (ret) {
+		dev_err(pcie->pci.dev, "cfg signal is invalid");
+		return ret;
+	}
+
+	/* pciet_powerup_rstn */
+	ret = reset_control_deassert(pcie->powerup_rst);
+	if (ret) {
+		dev_err(pcie->pci.dev, "powerup signal is invalid");
+		goto err_deassert_powerup;
+	}
+
+	return ret;
+
+err_deassert_powerup:
+	reset_control_assert(pcie->cfg_rst);
+
+	return ret;
+}
+
+static void eswin_pcie_power_off(struct eswin_pcie *eswin_pcie)
+{
+	reset_control_assert(eswin_pcie->powerup_rst);
+	reset_control_assert(eswin_pcie->cfg_rst);
+}
+
+static int eswin_pcie_host_init(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct eswin_pcie *pcie = dev_get_drvdata(pci->dev);
+	int ret;
+	u32 val;
+	u32 retries;
+
+	/* Fetch clocks */
+	pcie->num_clks = devm_clk_bulk_get_all_enabled(pci->dev, &pcie->clks);
+	if (pcie->num_clks < 0)
+		return dev_err_probe(pci->dev, pcie->num_clks,
+				     "failed to get pcie clocks\n");
+
+	ret = eswin_pcie_power_on(pcie);
+	if (ret)
+		return ret;
+
+	/* set device type : rc */
+	val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
+	val &= 0xfffffff0;
+	writel_relaxed(val | 0x4, pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
+
+	ret = reset_control_assert(pcie->perst);
+	if (ret) {
+		dev_err(pci->dev, "perst assert signal is invalid");
+		goto err_perst;
+	}
+	msleep(100);
+	ret = reset_control_deassert(pcie->perst);
+	if (ret) {
+		dev_err(pci->dev, "perst deassert signal is invalid");
+		goto err_perst;
+	}
+
+	/* app_hold_phy_rst */
+	val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
+	val &= ~(0x40);
+	writel_relaxed(val, pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
+
+	/*
+	 * It takes at least 20ms to wait for the pcie
+	 * status register to be 0.
+	 */
+	retries = 30;
+	do {
+		val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_STATUS0_OFFSET);
+		if (!(val & PCIE_PM_SEL_AUX_CLK))
+			break;
+		usleep_range(1000, 1100);
+		retries--;
+	} while (retries);
+
+	if (!retries) {
+		dev_err(pci->dev, "No clock exist.\n");
+		ret = -ENODEV;
+		goto err_clock;
+	}
+
+	/* config eswin vendor id and eic7700 device id */
+	dw_pcie_writel_dbi(pci, PCIE_TYPE_DEV_VEND_ID, 0x20301fe1);
+
+	/* lane fix config, real driver NOT need, default x4 */
+	val = dw_pcie_readl_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL);
+	val &= 0xffffff80;
+	val |= 0x44;
+	dw_pcie_writel_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL, val);
+
+	val = dw_pcie_readl_dbi(pci, DEVICE_CONTROL_DEVICE_STATUS);
+	val &= ~(0x7 << 5);
+	val |= (0x2 << 5);
+	dw_pcie_writel_dbi(pci, DEVICE_CONTROL_DEVICE_STATUS, val);
+
+	/*  config support 32 msi vectors */
+	val = dw_pcie_readl_dbi(pci, PCIE_DSP_PF0_MSI_CAP);
+	val &= ~PCIE_MSI_MULTIPLE_MSG_MASK;
+	val |= PCIE_MSI_MULTIPLE_MSG_32;
+	dw_pcie_writel_dbi(pci, PCIE_DSP_PF0_MSI_CAP, val);
+
+	/* disable msix cap */
+	val = dw_pcie_readl_dbi(pci, PCIE_NEXT_CAP_PTR);
+	val &= 0xffff00ff;
+	dw_pcie_writel_dbi(pci, PCIE_NEXT_CAP_PTR, val);
+
+	return 0;
+
+err_clock:
+	reset_control_assert(pcie->perst);
+err_perst:
+	eswin_pcie_power_off(pcie);
+	return ret;
+}
+
+static const struct dw_pcie_host_ops eswin_pcie_host_ops = {
+	.init = eswin_pcie_host_init,
+};
+
+static const struct dw_pcie_ops dw_pcie_ops = {
+	.start_link = eswin_pcie_start_link,
+	.link_up = eswin_pcie_link_up,
+};
+
+static int eswin_pcie_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct dw_pcie *pci;
+	struct eswin_pcie *pcie;
+	int ret;
+
+	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	pci = &pcie->pci;
+	pci->dev = dev;
+	pci->ops = &dw_pcie_ops;
+	pci->pp.ops = &eswin_pcie_host_ops;
+
+	/* SiFive specific region: mgmt */
+	pcie->mgmt_base = devm_platform_ioremap_resource_byname(pdev, "mgmt");
+	if (IS_ERR(pcie->mgmt_base))
+		return dev_err_probe(dev, PTR_ERR(pcie->mgmt_base),
+				     "failed to map mgmt memory\n");
+
+	/* Fetch reset */
+	pcie->powerup_rst = devm_reset_control_get_optional(&pdev->dev,
+							    "powerup");
+	if (IS_ERR_OR_NULL(pcie->powerup_rst))
+		return dev_err_probe(dev, PTR_ERR(pcie->powerup_rst),
+				     "unable to get powerup reset\n");
+
+	pcie->cfg_rst = devm_reset_control_get_optional(&pdev->dev, "cfg");
+	if (IS_ERR_OR_NULL(pcie->cfg_rst))
+		return dev_err_probe(dev, PTR_ERR(pcie->cfg_rst),
+				     "unable to get cfg reset\n");
+
+	pcie->perst = devm_reset_control_get_optional(&pdev->dev, "pwren");
+	if (IS_ERR_OR_NULL(pcie->perst))
+		return dev_err_probe(dev, PTR_ERR(pcie->perst),
+				     "unable to get perst reset\n");
+
+	platform_set_drvdata(pdev, pcie);
+
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		dev_err(dev, "pm_runtime_get_sync failed: %d\n", ret);
+		goto err_get_sync;
+	}
+
+	ret = dw_pcie_host_init(&pci->pp);
+	if (ret) {
+		dev_err(dev, "failed to initialize host: %d\n", ret);
+		goto err_host_init;
+	}
+
+	return ret;
+
+err_host_init:
+	pm_runtime_put_sync(dev);
+err_get_sync:
+	pm_runtime_disable(dev);
+	return ret;
+}
+
+static void eswin_pcie_remove(struct platform_device *pdev)
+{
+	struct eswin_pcie *pcie = platform_get_drvdata(pdev);
+
+	dw_pcie_host_deinit(&pcie->pci.pp);
+	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+
+	reset_control_assert(pcie->perst);
+	eswin_pcie_power_off(pcie);
+}
+
+static void eswin_pcie_shutdown(struct platform_device *pdev)
+{
+	struct eswin_pcie *pcie = platform_get_drvdata(pdev);
+
+	/* Bring down link, so bootloader gets clean state in case of reboot */
+	reset_control_assert(pcie->perst);
+}
+
+static int eswin_pcie_suspend(struct device *dev)
+{
+	struct eswin_pcie *pcie = dev_get_drvdata(dev);
+
+	reset_control_assert(pcie->perst);
+	eswin_pcie_power_off(pcie);
+	clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
+
+	return 0;
+}
+
+static int eswin_pcie_resume(struct device *dev)
+{
+	int ret;
+	struct eswin_pcie *pcie = dev_get_drvdata(dev);
+
+	ret = eswin_pcie_host_init(&pcie->pci.pp);
+	if (ret < 0) {
+		dev_err(dev, "Failed to init host: %d\n", ret);
+		return ret;
+	}
+
+	dw_pcie_setup_rc(&pcie->pci.pp);
+	eswin_pcie_start_link(&pcie->pci);
+	dw_pcie_wait_for_link(&pcie->pci);
+
+	return 0;
+}
+
+static const struct dev_pm_ops eswin_pcie_pm_ops = {
+	NOIRQ_SYSTEM_SLEEP_PM_OPS(eswin_pcie_suspend, eswin_pcie_resume)
+};
+
+static const struct of_device_id eswin_pcie_of_match[] = {
+	{ .compatible = "eswin,eic7700-pcie" },
+	{},
+};
+
+static struct platform_driver eswin_pcie_driver = {
+	.driver = {
+		.name = "eic7700-pcie",
+		.of_match_table = eswin_pcie_of_match,
+		.suppress_bind_attrs = true,
+		.pm = &eswin_pcie_pm_ops,
+	},
+	.probe = eswin_pcie_probe,
+	.remove = eswin_pcie_remove,
+	.shutdown = eswin_pcie_shutdown,
+};
+
+module_platform_driver(eswin_pcie_driver);
+
+MODULE_DEVICE_TABLE(of, eswin_pcie_of_match);
+MODULE_DESCRIPTION("PCIe host controller driver for eic7700 SoCs");
+MODULE_AUTHOR("Yu Ning <ningyu@eswincomputing.com>");
+MODULE_AUTHOR("Senchuan Zhang <zhangsenchuan@eswincomputing.com>");
+MODULE_LICENSE("GPL");
--
2.25.1

Re: [PATCH v2 2/2] PCI: eic7700: Add Eswin eic7700 PCIe host controller driver

[Manivannan Sadhasivam]

On Fri, Aug 29, 2025 at 04:24:05PM GMT, zhangsenchuan@eswincomputing.com wrote:
> From: Senchuan Zhang <zhangsenchuan@eswincomputing.com>
> 
> Add driver for the Eswin EIC7700 PCIe host controller.
> The controller is based on the DesignWare PCIe core.
> 

Add more info about the controller: DWC IP revision, data rate, lanes etc...

> Signed-off-by: Yu Ning <ningyu@eswincomputing.com>
> Signed-off-by: Senchuan Zhang<zhangsenchuan@eswincomputing.com>
> ---
>  drivers/pci/controller/dwc/Kconfig        |  12 +
>  drivers/pci/controller/dwc/Makefile       |   1 +
>  drivers/pci/controller/dwc/pcie-eic7700.c | 350 ++++++++++++++++++++++
>  3 files changed, 363 insertions(+)
>  create mode 100644 drivers/pci/controller/dwc/pcie-eic7700.c
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index ff6b6d9e18ec..1c4063107a8a 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -492,4 +492,16 @@ config PCIE_VISCONTI_HOST
>  	  Say Y here if you want PCIe controller support on Toshiba Visconti SoC.
>  	  This driver supports TMPV7708 SoC.
> 
> +config PCIE_EIC7700
> +	tristate "ESWIN PCIe host controller"
> +	depends on PCI_MSI
> +	depends on ARCH_ESWIN || COMPILE_TEST
> +	select PCIE_DW_HOST
> +	help
> +	  Enables support for the PCIe controller in the Eswin SoC
> +	  The PCI controller on Eswin is based on DesignWare hardware
> +	  It is a high-speed hardware bus standard used to connect
> +	  processors with external devices.

No need to explain what PCIe bus is.

> Say Y here if you want
> +	  PCIe controller support for the ESWIN.
> +
>  endmenu
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index 6919d27798d1..0717fe73a2a9 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -31,6 +31,7 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
>  obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
>  obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o
>  obj-$(CONFIG_PCIE_RCAR_GEN4) += pcie-rcar-gen4.o
> +obj-$(CONFIG_PCIE_EIC7700) += pcie-eic7700.o
> 
>  # The following drivers are for devices that use the generic ACPI
>  # pci_root.c driver but don't support standard ECAM config access.
> diff --git a/drivers/pci/controller/dwc/pcie-eic7700.c b/drivers/pci/controller/dwc/pcie-eic7700.c
> new file mode 100644
> index 000000000000..bf942154d971
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-eic7700.c
> @@ -0,0 +1,350 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ESWIN PCIe root complex driver
> + *
> + * Copyright 2025, Beijing ESWIN Computing Technology Co., Ltd.
> + *
> + * Authors: Yu Ning <ningyu@eswincomputing.com>
> + *          Senchuan Zhang <zhangsenchuan@eswincomputing.com>
> + */
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/resource.h>
> +#include <linux/types.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/reset.h>
> +#include <linux/pm_runtime.h>
> +
> +#include "pcie-designware.h"
> +
> +struct eswin_pcie {
> +	struct dw_pcie pci;
> +	void __iomem *mgmt_base;
> +	struct clk_bulk_data *clks;
> +	struct reset_control *powerup_rst;
> +	struct reset_control *cfg_rst;
> +	struct reset_control *perst;
> +
> +	int num_clks;
> +};
> +
> +#define PCIE_PM_SEL_AUX_CLK BIT(16)
> +#define PCIEMGMT_APP_LTSSM_ENABLE BIT(5)
> +
> +#define PCIEMGMT_CTRL0_OFFSET 0x0
> +#define PCIEMGMT_STATUS0_OFFSET 0x100
> +
> +#define PCIE_TYPE_DEV_VEND_ID 0x0
> +#define PCIE_DSP_PF0_MSI_CAP 0x50
> +#define PCIE_NEXT_CAP_PTR 0x70
> +#define DEVICE_CONTROL_DEVICE_STATUS 0x78
> +
> +#define PCIE_MSI_MULTIPLE_MSG_32 (0x5 << 17)
> +#define PCIE_MSI_MULTIPLE_MSG_MASK (0x7 << 17)
> +
> +#define PCIEMGMT_LINKUP_STATE_VALIDATE ((0x11 << 2) | 0x3)
> +#define PCIEMGMT_LINKUP_STATE_MASK 0xff
> +
> +static int eswin_pcie_start_link(struct dw_pcie *pci)
> +{
> +	struct device *dev = pci->dev;
> +	struct eswin_pcie *pcie = dev_get_drvdata(dev);
> +	u32 val;
> +
> +	/* Enable LTSSM */
> +	val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
> +	val |= PCIEMGMT_APP_LTSSM_ENABLE;
> +	writel_relaxed(val, pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
> +
> +	return 0;
> +}
> +
> +static bool eswin_pcie_link_up(struct dw_pcie *pci)
> +{
> +	struct device *dev = pci->dev;
> +	struct eswin_pcie *pcie = dev_get_drvdata(dev);
> +	u32 val;
> +
> +	val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_STATUS0_OFFSET);
> +
> +	return ((val & PCIEMGMT_LINKUP_STATE_MASK) ==
> +		     PCIEMGMT_LINKUP_STATE_VALIDATE);
> +}
> +
> +static int eswin_pcie_power_on(struct eswin_pcie *pcie)
> +{
> +	int ret = 0;

Don't initialize ret.

> +
> +	/* pciet_cfg_rstn */
> +	ret = reset_control_deassert(pcie->cfg_rst);
> +	if (ret) {
> +		dev_err(pcie->pci.dev, "cfg signal is invalid");
> +		return ret;
> +	}
> +
> +	/* pciet_powerup_rstn */
> +	ret = reset_control_deassert(pcie->powerup_rst);
> +	if (ret) {
> +		dev_err(pcie->pci.dev, "powerup signal is invalid");
> +		goto err_deassert_powerup;
> +	}
> +
> +	return ret;

	return 0;

> +
> +err_deassert_powerup:
> +	reset_control_assert(pcie->cfg_rst);
> +
> +	return ret;
> +}
> +
> +static void eswin_pcie_power_off(struct eswin_pcie *eswin_pcie)
> +{
> +	reset_control_assert(eswin_pcie->powerup_rst);
> +	reset_control_assert(eswin_pcie->cfg_rst);
> +}
> +
> +static int eswin_pcie_host_init(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct eswin_pcie *pcie = dev_get_drvdata(pci->dev);
> +	int ret;
> +	u32 val;
> +	u32 retries;
> +
> +	/* Fetch clocks */

Drop the comment.

> +	pcie->num_clks = devm_clk_bulk_get_all_enabled(pci->dev, &pcie->clks);
> +	if (pcie->num_clks < 0)
> +		return dev_err_probe(pci->dev, pcie->num_clks,
> +				     "failed to get pcie clocks\n");
> +
> +	ret = eswin_pcie_power_on(pcie);
> +	if (ret)
> +		return ret;
> +
> +	/* set device type : rc */
> +	val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
> +	val &= 0xfffffff0;

Can you add bitfield definition for the mask?

> +	writel_relaxed(val | 0x4, pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);

Also for 0x4.

> +
> +	ret = reset_control_assert(pcie->perst);
> +	if (ret) {
> +		dev_err(pci->dev, "perst assert signal is invalid");

'Failed to assert PERST#'

> +		goto err_perst;
> +	}
> +	msleep(100);
> +	ret = reset_control_deassert(pcie->perst);
> +	if (ret) {
> +		dev_err(pci->dev, "perst deassert signal is invalid");

'Failed to deassert PERST#'

> +		goto err_perst;
> +	}
> +
> +	/* app_hold_phy_rst */
> +	val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
> +	val &= ~(0x40);

Add definition here and everywhere.

> +	writel_relaxed(val, pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
> +
> +	/*
> +	 * It takes at least 20ms to wait for the pcie
> +	 * status register to be 0.

Make use of 80 columns for comments.

> +	 */
> +	retries = 30;
> +	do {
> +		val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_STATUS0_OFFSET);
> +		if (!(val & PCIE_PM_SEL_AUX_CLK))
> +			break;
> +		usleep_range(1000, 1100);
> +		retries--;
> +	} while (retries);
> +
> +	if (!retries) {
> +		dev_err(pci->dev, "No clock exist.\n");

What does this error mean exactly? "No clock exist" is not a valid error. Error
has something to do with PCIE_PM_SEL_AUX_CLK, no?

> +		ret = -ENODEV;

-ETIMEDOUT?

> +		goto err_clock;
> +	}
> +
> +	/* config eswin vendor id and eic7700 device id */
> +	dw_pcie_writel_dbi(pci, PCIE_TYPE_DEV_VEND_ID, 0x20301fe1);

Does it need to be configured all the time?

> +
> +	/* lane fix config, real driver NOT need, default x4 */

What do you mean by 'readl driver NOT need'?

> +	val = dw_pcie_readl_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL);
> +	val &= 0xffffff80;
> +	val |= 0x44;
> +	dw_pcie_writel_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL, val);
> +
> +	val = dw_pcie_readl_dbi(pci, DEVICE_CONTROL_DEVICE_STATUS);
> +	val &= ~(0x7 << 5);
> +	val |= (0x2 << 5);
> +	dw_pcie_writel_dbi(pci, DEVICE_CONTROL_DEVICE_STATUS, val);
> +
> +	/*  config support 32 msi vectors */
> +	val = dw_pcie_readl_dbi(pci, PCIE_DSP_PF0_MSI_CAP);
> +	val &= ~PCIE_MSI_MULTIPLE_MSG_MASK;
> +	val |= PCIE_MSI_MULTIPLE_MSG_32;
> +	dw_pcie_writel_dbi(pci, PCIE_DSP_PF0_MSI_CAP, val);
> +
> +	/* disable msix cap */

Why? Hw doesn't support MSI-X but it advertises MSI-X capability?

> +	val = dw_pcie_readl_dbi(pci, PCIE_NEXT_CAP_PTR);
> +	val &= 0xffff00ff;
> +	dw_pcie_writel_dbi(pci, PCIE_NEXT_CAP_PTR, val);
> +
> +	return 0;
> +
> +err_clock:
> +	reset_control_assert(pcie->perst);
> +err_perst:
> +	eswin_pcie_power_off(pcie);
> +	return ret;
> +}
> +
> +static const struct dw_pcie_host_ops eswin_pcie_host_ops = {
> +	.init = eswin_pcie_host_init,
> +};
> +
> +static const struct dw_pcie_ops dw_pcie_ops = {
> +	.start_link = eswin_pcie_start_link,
> +	.link_up = eswin_pcie_link_up,
> +};
> +
> +static int eswin_pcie_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct dw_pcie *pci;
> +	struct eswin_pcie *pcie;

Use reverse Xmas order for all local variables in this driver.

> +	int ret;
> +
> +	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> +	if (!pcie)
> +		return -ENOMEM;
> +
> +	pci = &pcie->pci;
> +	pci->dev = dev;
> +	pci->ops = &dw_pcie_ops;
> +	pci->pp.ops = &eswin_pcie_host_ops;
> +
> +	/* SiFive specific region: mgmt */

So the DWC glue wrapper belongs to SiFive?

> +	pcie->mgmt_base = devm_platform_ioremap_resource_byname(pdev, "mgmt");
> +	if (IS_ERR(pcie->mgmt_base))
> +		return dev_err_probe(dev, PTR_ERR(pcie->mgmt_base),
> +				     "failed to map mgmt memory\n");
> +
> +	/* Fetch reset */

Drop the comment.

> +	pcie->powerup_rst = devm_reset_control_get_optional(&pdev->dev,
> +							    "powerup");
> +	if (IS_ERR_OR_NULL(pcie->powerup_rst))
> +		return dev_err_probe(dev, PTR_ERR(pcie->powerup_rst),
> +				     "unable to get powerup reset\n");
> +
> +	pcie->cfg_rst = devm_reset_control_get_optional(&pdev->dev, "cfg");
> +	if (IS_ERR_OR_NULL(pcie->cfg_rst))
> +		return dev_err_probe(dev, PTR_ERR(pcie->cfg_rst),
> +				     "unable to get cfg reset\n");
> +
> +	pcie->perst = devm_reset_control_get_optional(&pdev->dev, "pwren");
> +	if (IS_ERR_OR_NULL(pcie->perst))
> +		return dev_err_probe(dev, PTR_ERR(pcie->perst),
> +				     "unable to get perst reset\n");

All 3 resets are optional? Even the ones you were using to power on the
controller?

> +
> +	platform_set_drvdata(pdev, pcie);
> +
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);

So by this time controller is powered on? I don't think so, as
eswin_pcie_power_on() is called from host_init() callback. If I'm right, then
these should be moved below dw_pcie_host_init().

> +	ret = pm_runtime_get_sync(dev);

Why do you need get_sync? Are you depending on any parent to power on any
resource?

> +	if (ret < 0) {
> +		dev_err(dev, "pm_runtime_get_sync failed: %d\n", ret);
> +		goto err_get_sync;
> +	}
> +
> +	ret = dw_pcie_host_init(&pci->pp);
> +	if (ret) {
> +		dev_err(dev, "failed to initialize host: %d\n", ret);
> +		goto err_host_init;
> +	}
> +
> +	return ret;

	return 0;

> +
> +err_host_init:
> +	pm_runtime_put_sync(dev);
> +err_get_sync:
> +	pm_runtime_disable(dev);
> +	return ret;
> +}
> +
> +static void eswin_pcie_remove(struct platform_device *pdev)
> +{
> +	struct eswin_pcie *pcie = platform_get_drvdata(pdev);
> +
> +	dw_pcie_host_deinit(&pcie->pci.pp);
> +	pm_runtime_put_sync(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +
> +	reset_control_assert(pcie->perst);
> +	eswin_pcie_power_off(pcie);
> +}
> +
> +static void eswin_pcie_shutdown(struct platform_device *pdev)
> +{
> +	struct eswin_pcie *pcie = platform_get_drvdata(pdev);
> +
> +	/* Bring down link, so bootloader gets clean state in case of reboot */

Asserting PERST# won't bring down the device. It just provides indication that
the device might get powered off and it needs to be prepared for that.

Also, I don't get what you mean by 'bootloder'. Is it the host system bootloader
or device? Usually, shutdown callback is needed to power off the device when the
host powers down/reboots so that the device will be reset to a clean state.

> +	reset_control_assert(pcie->perst);
> +}
> +
> +static int eswin_pcie_suspend(struct device *dev)
> +{
> +	struct eswin_pcie *pcie = dev_get_drvdata(dev);
> +
> +	reset_control_assert(pcie->perst);
> +	eswin_pcie_power_off(pcie);

So you want to power off the device even if it intends to be in D0? Like NVMe.

> +	clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
> +
> +	return 0;
> +}
> +
> +static int eswin_pcie_resume(struct device *dev)
> +{
> +	int ret;
> +	struct eswin_pcie *pcie = dev_get_drvdata(dev);
> +
> +	ret = eswin_pcie_host_init(&pcie->pci.pp);
> +	if (ret < 0) {

if (ret)

> +		dev_err(dev, "Failed to init host: %d\n", ret);
> +		return ret;
> +	}
> +
> +	dw_pcie_setup_rc(&pcie->pci.pp);
> +	eswin_pcie_start_link(&pcie->pci);
> +	dw_pcie_wait_for_link(&pcie->pci);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops eswin_pcie_pm_ops = {
> +	NOIRQ_SYSTEM_SLEEP_PM_OPS(eswin_pcie_suspend, eswin_pcie_resume)
> +};
> +
> +static const struct of_device_id eswin_pcie_of_match[] = {
> +	{ .compatible = "eswin,eic7700-pcie" },
> +	{},
> +};
> +
> +static struct platform_driver eswin_pcie_driver = {
> +	.driver = {
> +		.name = "eic7700-pcie",
> +		.of_match_table = eswin_pcie_of_match,
> +		.suppress_bind_attrs = true,
> +		.pm = &eswin_pcie_pm_ops,
> +	},
> +	.probe = eswin_pcie_probe,
> +	.remove = eswin_pcie_remove,

Since this controller implements irqchip using the DWC core driver, it is not
safe to remove it during runtime.

> +	.shutdown = eswin_pcie_shutdown,
> +};
> +
> +module_platform_driver(eswin_pcie_driver);

builtin_platform_driver()

> +
> +MODULE_DEVICE_TABLE(of, eswin_pcie_of_match);

Move it below eswin_pcie_of_match[].

> +MODULE_DESCRIPTION("PCIe host controller driver for eic7700 SoCs");

EIC7700?

- Mani

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

Re: Re: [PATCH v2 2/2] PCI: eic7700: Add Eswin eic7700 PCIe host controller driver

[zhangsenchuan]

Dear Manivannan

Thank you for your thorough review.Here are some of my clarifications and questions.
Looking forward to your answer, Thank you very much.

> -----Original Messages-----
> From: "Manivannan Sadhasivam" <mani@kernel.org>
> Send time:Monday, 01/09/2025 14:40:41
> 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 2/2] PCI: eic7700: Add Eswin eic7700 PCIe host controller driver


> > +	/* config eswin vendor id and eic7700 device id */
> > +	dw_pcie_writel_dbi(pci, PCIE_TYPE_DEV_VEND_ID, 0x20301fe1);
> 
> Does it need to be configured all the time?

Clarification：
Our hardware initialization did not configure the device Id and vendor Id.
Now, we can only rewrite the device Id and vendor Id in the code.

> 
> > +
> > +	/* lane fix config, real driver NOT need, default x4 */
> 
> What do you mean by 'readl driver NOT need'?
> 

Clarification：
Sorry, this was added during the compatibility platform test. It is not needed for real devices. 
I will remove it later.

> > +	val = dw_pcie_readl_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL);
> > +	val &= 0xffffff80;
> > +	val |= 0x44;
> > +	dw_pcie_writel_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL, val);
> > +
> > +	val = dw_pcie_readl_dbi(pci, DEVICE_CONTROL_DEVICE_STATUS);
> > +	val &= ~(0x7 << 5);
> > +	val |= (0x2 << 5);
> > +	dw_pcie_writel_dbi(pci, DEVICE_CONTROL_DEVICE_STATUS, val);
> > +
> > +	/*  config support 32 msi vectors */
> > +	val = dw_pcie_readl_dbi(pci, PCIE_DSP_PF0_MSI_CAP);
> > +	val &= ~PCIE_MSI_MULTIPLE_MSG_MASK;
> > +	val |= PCIE_MSI_MULTIPLE_MSG_32;
> > +	dw_pcie_writel_dbi(pci, PCIE_DSP_PF0_MSI_CAP, val);
> > +
> > +	/* disable msix cap */
> 
> Why? Hw doesn't support MSI-X but it advertises MSI-X capability?
> 

I'm not quite sure what this comment means? Indeed, our hardware doesn't support MSI-X.
We can't disable the MSI-X capability using the PCIE_NEXT_CAP_PTR register? Then which 
register is needed to disable the MSI-X capability?

> > +	val = dw_pcie_readl_dbi(pci, PCIE_NEXT_CAP_PTR);
> > +	val &= 0xffff00ff;
> > +	dw_pcie_writel_dbi(pci, PCIE_NEXT_CAP_PTR, val);
> > +
> > +	return 0;
> > +
> > +err_clock:
> > +	reset_control_assert(pcie->perst);
> > +err_perst:
> > +	eswin_pcie_power_off(pcie);
> > +	return ret;
> > +}
> > +




Best Regards,
Senchuan Zhang