[PATCH v2 00/10] Add ASPEED PCIe Root Complex support

[PATCH v2 00/10] Add ASPEED PCIe Root Complex support

[Jacky Chou]

This patch series adds support for the ASPEED PCIe Root Complex,
including device tree bindings, pinctrl support, and the PCIe host controller
driver. The patches introduce the necessary device tree nodes, pinmux groups,
and driver implementation to enable PCIe functionality on ASPEED platforms.
Currently, the ASPEED PCIe Root Complex only supports a single port.

Summary of changes:
- Add device tree binding documents for ASPEED PCIe PHY, PCIe Config, and PCIe RC
- Update MAINTAINERS for new bindings and driver
- Add PCIe RC node and PERST control pin to aspeed-g6 device tree
- Add PCIe RC PERST pin group to aspeed-g6 pinctrl
- Implement ASPEED PCIe Root Complex host controller driver

This series has been tested on AST2600/AST2700 platforms and enables PCIe device
enumeration and operation.

Jacky Chou (10):
  dt-bindings: soc: aspeed: Add ASPEED PCIe Config support
  dt-bindings: soc: aspeed: Add ASPEED PCIe PHY support
  dt-bindings: PCI: Add ASPEED PCIe RC support
  dt-bindings: pinctrl: aspeed,ast2600-pinctrl: Add PCIe RC PERST# group
  ARM: dts: aspeed-g6: Add AST2600 PCIe RC PERST#
  ARM: dts: aspeed-g6: Add PCIe RC node
  pinctrl: aspeed-g6: Add PCIe RC PERST pin group
  PCI: Add FMT and TYPE definition for TLP header
  PCI: aspeed: Add ASPEED PCIe RC driver
  MAINTAINERS: Add ASPEED PCIe RC driver

 .../bindings/pci/aspeed,ast2600-pcie.yaml     |  198 +++
 .../pinctrl/aspeed,ast2600-pinctrl.yaml       |    2 +
 .../bindings/soc/aspeed/aspeed,pcie-cfg.yaml  |   41 +
 .../bindings/soc/aspeed/aspeed,pcie-phy.yaml  |   44 +
 MAINTAINERS                                   |   10 +
 .../boot/dts/aspeed/aspeed-g6-pinctrl.dtsi    |    5 +
 arch/arm/boot/dts/aspeed/aspeed-g6.dtsi       |   61 +
 drivers/pci/controller/Kconfig                |   13 +
 drivers/pci/controller/Makefile               |    1 +
 drivers/pci/controller/pcie-aspeed.c          | 1137 +++++++++++++++++
 drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c    |   12 +-
 include/uapi/linux/pci_regs.h                 |   32 +
 12 files changed, 1555 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/pci/aspeed,ast2600-pcie.yaml
 create mode 100644 Documentation/devicetree/bindings/soc/aspeed/aspeed,pcie-cfg.yaml
 create mode 100644 Documentation/devicetree/bindings/soc/aspeed/aspeed,pcie-phy.yaml
 create mode 100644 drivers/pci/controller/pcie-aspeed.c

---
v2:
 - Moved ASPEED PCIe PHY yaml binding to `soc/aspeed` directory and
   changed it as syscon
 - Added `MAINTAINERS` entry for the new PCIe RC driver
 - Updated device tree bindings to reflect the new structure
 - Refactored configuration read and write functions to main bus and
   child bus ops
 - Refactored initialization to implement multiple ports support
 - Added PCIe FMT and TYPE definitions for TLP header in
   `include/uapi/linux/pci_regs.h`
 - Updated from reviewer comments
---

-- 
2.43.0

[PATCH v2 01/10] dt-bindings: soc: aspeed: Add ASPEED PCIe Config support

[Jacky Chou]

Add the ASPEED PCIe configuration syscon block. This shared register
space is used by multiple PCIe-related devices to coordinate and manage
common PCIe settings. The binding describes the required compatible
strings and register space for the configuration node.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 .../bindings/soc/aspeed/aspeed,pcie-cfg.yaml  | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/aspeed/aspeed,pcie-cfg.yaml

diff --git a/Documentation/devicetree/bindings/soc/aspeed/aspeed,pcie-cfg.yaml b/Documentation/devicetree/bindings/soc/aspeed/aspeed,pcie-cfg.yaml
new file mode 100644
index 000000000000..6b282f06b914
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/aspeed/aspeed,pcie-cfg.yaml
@@ -0,0 +1,41 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/aspeed/aspeed,pcie-cfg.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ASPEED PCIe Configuration
+
+maintainers:
+  - Jacky Chou <jacky_chou@aspeedtech.com>
+
+description: |
+  The ASPEED PCIe configuration syscon block provides a set of registers shared
+  by multiple PCIe-related devices within the SoC. This node represents the
+  common configuration space that allows these devices to coordinate and manage
+  shared PCIe settings, including address mapping, control, and status
+  registers. The syscon interface enables for various PCIe devices to access
+  and modify these shared registers in a consistent and centralized manner.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - aspeed,pcie-cfg
+      - const: syscon
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    syscon@1e770000 {
+      compatible = "aspeed,pcie-cfg", "syscon";
+      reg = <0x1e770000 0x80>;
+    };
-- 
2.43.0

Re: [PATCH v2 01/10] dt-bindings: soc: aspeed: Add ASPEED PCIe Config support

[Krzysztof Kozlowski]

On Tue, Jul 15, 2025 at 11:43:11AM +0800, Jacky Chou wrote:
> +maintainers:
> +  - Jacky Chou <jacky_chou@aspeedtech.com>
> +
> +description: |

Drop |

> +  The ASPEED PCIe configuration syscon block provides a set of registers shared
> +  by multiple PCIe-related devices within the SoC. This node represents the
> +  common configuration space that allows these devices to coordinate and manage
> +  shared PCIe settings, including address mapping, control, and status
> +  registers. The syscon interface enables for various PCIe devices to access
> +  and modify these shared registers in a consistent and centralized manner.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - aspeed,pcie-cfg

NAK, see writing bindings. You already received comments about generic
compatible in the past.

Best regards,
Krzysztof

回覆: [PATCH v2 01/10] dt-bindings: soc: aspeed: Add ASPEED PCIe Config support

[Jacky Chou]

Hi Krzysztof,

Thank you for your reply.

> > +maintainers:
> > +  - Jacky Chou <jacky_chou@aspeedtech.com>
> > +
> > +description: |
> 
> Drop |
> 

Agreed.

> > +  The ASPEED PCIe configuration syscon block provides a set of
> > + registers shared  by multiple PCIe-related devices within the SoC.
> > + This node represents the  common configuration space that allows
> > + these devices to coordinate and manage  shared PCIe settings,
> > + including address mapping, control, and status  registers. The
> > + syscon interface enables for various PCIe devices to access  and modify
> these shared registers in a consistent and centralized manner.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - aspeed,pcie-cfg
> 
> NAK, see writing bindings. You already received comments about generic
> compatible in the past.
> 

I understand the generic aspeed,pcie-cfg is not acceptable per the binding guidelines.
I will update it in the next version to use a more specific name like aspeed,ast2600-pciecfg.
Thanks again for your guidance.

Thanks,
Jacky

[PATCH v2 02/10] dt-bindings: soc: aspeed: Add ASPEED PCIe PHY support

[Jacky Chou]

This PHY is used by many modules. In our design, our
PCIe has RC and EP funcitons. On the different function,
each driver will use configure and get some information
from the PHY interface to do somting that it wants to.
Getting link status, setting syscon credits and so on.
Therefore, define it as syscon for all modules.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 .../bindings/soc/aspeed/aspeed,pcie-phy.yaml  | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/aspeed/aspeed,pcie-phy.yaml

diff --git a/Documentation/devicetree/bindings/soc/aspeed/aspeed,pcie-phy.yaml b/Documentation/devicetree/bindings/soc/aspeed/aspeed,pcie-phy.yaml
new file mode 100644
index 000000000000..5fa585d63ca6
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/aspeed/aspeed,pcie-phy.yaml
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/aspeed/aspeed,pcie-phy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ASPEED PCIe PHY
+
+maintainers:
+  - Jacky Chou <jacky_chou@aspeedtech.com>
+
+description:
+  The ASPEED PCIe PHY provides the physical layer interface for PCIe
+  controllers in the SoC. This node represents the register block for the PCIe
+  PHY, which is typically accessed by PCIe Root Complex or Endpoint drivers
+  via syscon. It is used to configure and get the status of the PCIe PHY
+  hardware, including power management, link training, and other PHY-specific
+  operations.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - aspeed,pcie-phy
+      - const: syscon
+
+  reg:
+    maxItems: 1
+
+  "#phy-cells":
+    const: 0
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    syscon@1e6ed200 {
+      compatible = "aspeed,pcie-phy", "syscon";
+      reg = <0x1e6ed200 0x100>;
+    };
-- 
2.43.0

Re: [PATCH v2 02/10] dt-bindings: soc: aspeed: Add ASPEED PCIe PHY support

[Krzysztof Kozlowski]

On Tue, Jul 15, 2025 at 11:43:12AM +0800, Jacky Chou wrote:
> This PHY is used by many modules. In our design, our
> PCIe has RC and EP funcitons. On the different function,
> each driver will use configure and get some information
> from the PHY interface to do somting that it wants to.
> Getting link status, setting syscon credits and so on.
> Therefore, define it as syscon for all modules.
> 
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> ---
>  .../bindings/soc/aspeed/aspeed,pcie-phy.yaml  | 44 +++++++++++++++++++
>  1 file changed, 44 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/aspeed/aspeed,pcie-phy.yaml

Phys go to phy, not soc directory. Soc is not a dumping ground.

> 
> diff --git a/Documentation/devicetree/bindings/soc/aspeed/aspeed,pcie-phy.yaml b/Documentation/devicetree/bindings/soc/aspeed/aspeed,pcie-phy.yaml
> new file mode 100644
> index 000000000000..5fa585d63ca6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/aspeed/aspeed,pcie-phy.yaml
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/aspeed/aspeed,pcie-phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ASPEED PCIe PHY
> +
> +maintainers:
> +  - Jacky Chou <jacky_chou@aspeedtech.com>
> +
> +description:
> +  The ASPEED PCIe PHY provides the physical layer interface for PCIe
> +  controllers in the SoC. This node represents the register block for the PCIe
> +  PHY, which is typically accessed by PCIe Root Complex or Endpoint drivers
> +  via syscon. It is used to configure and get the status of the PCIe PHY
> +  hardware, including power management, link training, and other PHY-specific
> +  operations.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - aspeed,pcie-phy

No, see writing bindings.

> +      - const: syscon

It's not a syscon, but phy. I don't think you understood previous
feedback. Go back to v1. You just send something to pass the review
instead of reworking to make it correct.

> +
> +  reg:
> +    maxItems: 1
> +
> +  "#phy-cells":
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    syscon@1e6ed200 {

wrong name, that's a phy

> +      compatible = "aspeed,pcie-phy", "syscon";
> +      reg = <0x1e6ed200 0x100>;

Incomplete

> +    };
> -- 
> 2.43.0
> 

[PATCH v2 03/10] dt-bindings: PCI: Add ASPEED PCIe RC support

[Jacky Chou]

This binding describes the required and optional properties for
configuring the PCIe RC node, including support for syscon phandles,
MSI, clocks, resets, and interrupt mapping. The schema enforces strict
property validation and provides a comprehensive example for reference.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 .../bindings/pci/aspeed,ast2600-pcie.yaml     | 198 ++++++++++++++++++
 1 file changed, 198 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/aspeed,ast2600-pcie.yaml

diff --git a/Documentation/devicetree/bindings/pci/aspeed,ast2600-pcie.yaml b/Documentation/devicetree/bindings/pci/aspeed,ast2600-pcie.yaml
new file mode 100644
index 000000000000..6fb6cf59c230
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/aspeed,ast2600-pcie.yaml
@@ -0,0 +1,198 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/aspeed,ast2600-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ASPEED PCIe Root Complex Controller
+
+maintainers:
+  - Jacky Chou <jacky_chou@aspeedtech.com>
+
+description:
+  The ASPEED PCIe Root Complex controller provides PCI Express Root Complex
+  functionality for ASPEED SoCs, such as the AST2600 and AST2700.
+  This controller enables connectivity to PCIe endpoint devices, supporting
+  memory and I/O windows, MSI and legacy interrupts, and integration with
+  the SoC's clock, reset, and pinctrl subsystems.
+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2600-pcie
+      - aspeed,ast2700-pcie
+
+  reg:
+    maxItems: 1
+
+  ranges:
+    minItems: 2
+    maxItems: 2
+
+  interrupts:
+    maxItems: 1
+    description: IntX and MSI interrupt
+
+  resets:
+    items:
+      - description: PCIe controller reset
+
+  reset-names:
+    items:
+      - const: h2x
+
+  msi-parent: true
+
+  aspeed,ahbc:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Phandle to the ASPEED AHB Controller (AHBC) syscon node.
+      This reference is used by the PCIe controller to access
+      system-level configuration registers related to the AHB bus.
+      To enable AHB access for the PCIe controller.
+
+  aspeed,pciecfg:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Phandle to the ASPEED PCIe configuration syscon node.
+      This reference allows the PCIe controller to access
+      SoC-specific PCIe configuration registers. There are the others
+      functions such PCIe RC and PCIe EP will use this common register
+      to configure the SoC interfaces.
+
+  aspeed,pciephy:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Phandle to the ASPEED PCIe PHY syscon node.
+      This property provides access to the PCIe PHY control
+      registers required for link initialization and management.
+      The other functions such PCIe RC and PCIe EP will use this
+      common register to configure the PHY interfaces and get some
+      information from the PHY.
+
+  interrupt-controller:
+    description: Interrupt controller node for handling legacy PCI interrupts.
+    type: object
+    properties:
+      '#address-cells':
+        const: 0
+      '#interrupt-cells':
+        const: 1
+      interrupt-controller: true
+
+    required:
+      - '#address-cells'
+      - '#interrupt-cells'
+      - interrupt-controller
+
+    additionalProperties: false
+
+allOf:
+  - $ref: /schemas/pci/pci-bus-common.yaml#
+  - $ref: /schemas/pci/pci-host-bridge.yaml#
+  - $ref: /schemas/interrupt-controller/msi-controller.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: aspeed,ast2600-pcie
+    then:
+      required:
+        - aspeed,ahbc
+    else:
+      properties:
+        aspeed,ahbc: false
+
+required:
+  - reg
+  - interrupts
+  - bus-range
+  - ranges
+  - resets
+  - reset-names
+  - msi-parent
+  - msi-controller
+  - aspeed,pciecfg
+  - interrupt-map-mask
+  - interrupt-map
+  - interrupt-controller
+
+unevaluatedProperties: false
+
+patternProperties:
+  "^pcie@[0-9a-f,]+$":
+    type: object
+    properties:
+      resets:
+        items:
+          - description: PCIe PERST
+      reset-names:
+        items:
+          - const: perst
+      clocks:
+        maxItems: 1
+        description: PCIe BUS clock
+    required:
+      - resets
+      - reset-names
+      - clocks
+      - aspeed,pciephy
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/ast2600-clock.h>
+
+    apb {
+      #address-cells = <1>;
+      #size-cells = <1>;
+
+      pcie0: pcie@1e7700c0 {
+        compatible = "aspeed,ast2600-pcie";
+        device_type = "pci";
+        reg = <0x1e7700c0 0x40>;
+        linux,pci-domain = <0>;
+        #address-cells = <3>;
+        #size-cells = <2>;
+        interrupts = <GIC_SPI 168 IRQ_TYPE_LEVEL_HIGH>;
+        bus-range = <0x80 0xff>;
+
+        ranges = <0x01000000 0x0 0x00018000 0x00018000 0x0 0x00008000
+                  0x02000000 0x0 0x70000000 0x70000000 0x0 0x10000000>;
+
+        resets = <&syscon ASPEED_RESET_H2X>;
+        reset-names = "h2x";
+
+        #interrupt-cells = <1>;
+        msi-parent = <&pcie0>;
+        msi-controller;
+
+        aspeed,ahbc = <&ahbc>;
+        aspeed,pciecfg = <&pcie_cfg>;
+
+        interrupt-map-mask = <0 0 0 7>;
+        interrupt-map = <0 0 0 1 &pcie_intc0 0>,
+                        <0 0 0 2 &pcie_intc0 1>,
+                        <0 0 0 3 &pcie_intc0 2>,
+                        <0 0 0 4 &pcie_intc0 3>;
+        pcie_intc0: interrupt-controller {
+          interrupt-controller;
+          #address-cells = <0>;
+          #interrupt-cells = <1>;
+        };
+
+        pcie@8,0 {
+          reg = <0x804000 0 0 0 0>;
+          #address-cells = <3>;
+          #size-cells = <2>;
+          device_type = "pci";
+          resets = <&syscon ASPEED_RESET_PCIE_RC_O>;
+          reset-names = "perst";
+          clocks = <&syscon ASPEED_CLK_GATE_BCLK>;
+          pinctrl-names = "default";
+          pinctrl-0 = <&pinctrl_pcierc1_default>;
+          aspeed,pciephy = <&pcie_phy1>;
+          ranges;
+        };
+      };
+    };
-- 
2.43.0

Re: [PATCH v2 03/10] dt-bindings: PCI: Add ASPEED PCIe RC support

[Krzysztof Kozlowski]

On Tue, Jul 15, 2025 at 11:43:13AM +0800, Jacky Chou wrote:
> This binding describes the required and optional properties for

No, describe the hardware, not "this binding".

> configuring the PCIe RC node, including support for syscon phandles,
> MSI, clocks, resets, and interrupt mapping. The schema enforces strict
> property validation and provides a comprehensive example for reference.

Don't say what schema does or does not. It's completely redundant.
Describe the hardware.

Your entire commit is redundantn and not helpful at all.

> 

...

> +
> +  aspeed,ahbc:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to the ASPEED AHB Controller (AHBC) syscon node.
> +      This reference is used by the PCIe controller to access
> +      system-level configuration registers related to the AHB bus.
> +      To enable AHB access for the PCIe controller.
> +
> +  aspeed,pciecfg:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to the ASPEED PCIe configuration syscon node.
> +      This reference allows the PCIe controller to access
> +      SoC-specific PCIe configuration registers. There are the others
> +      functions such PCIe RC and PCIe EP will use this common register
> +      to configure the SoC interfaces.
> +
> +  aspeed,pciephy:

No, phys are not syscons. I already told you that in v1.

> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to the ASPEED PCIe PHY syscon node.
> +      This property provides access to the PCIe PHY control
> +      registers required for link initialization and management.
> +      The other functions such PCIe RC and PCIe EP will use this
> +      common register to configure the PHY interfaces and get some
> +      information from the PHY.
> +
> +  interrupt-controller:
> +    description: Interrupt controller node for handling legacy PCI interrupts.
> +    type: object
> +    properties:
> +      '#address-cells':
> +        const: 0
> +      '#interrupt-cells':
> +        const: 1
> +      interrupt-controller: true
> +
> +    required:
> +      - '#address-cells'
> +      - '#interrupt-cells'
> +      - interrupt-controller
> +
> +    additionalProperties: false
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-bus-common.yaml#

No other binding references this. Don't write completely different code
than all other SoCs. This entire binding is written such way.

> +  - $ref: /schemas/pci/pci-host-bridge.yaml#
> +  - $ref: /schemas/interrupt-controller/msi-controller.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: aspeed,ast2600-pcie
> +    then:
> +      required:
> +        - aspeed,ahbc
> +    else:
> +      properties:
> +        aspeed,ahbc: false
> +
> +required:
> +  - reg
> +  - interrupts
> +  - bus-range
> +  - ranges
> +  - resets
> +  - reset-names
> +  - msi-parent
> +  - msi-controller
> +  - aspeed,pciecfg
> +  - interrupt-map-mask
> +  - interrupt-map
> +  - interrupt-controller
> +
> +unevaluatedProperties: false
> +
> +patternProperties:
> +  "^pcie@[0-9a-f,]+$":

Why do you need it? Also, order things according to example schema.

Best regards,
Krzysztof

回覆: [PATCH v2 03/10] dt-bindings: PCI: Add ASPEED PCIe RC support

[Jacky Chou]

Hi Krzysztof,

Thank you for your reply.

> No, describe the hardware, not "this binding".
> 
> > configuring the PCIe RC node, including support for syscon phandles,
> > MSI, clocks, resets, and interrupt mapping. The schema enforces strict
> > property validation and provides a comprehensive example for reference.
> 
> Don't say what schema does or does not. It's completely redundant.
> Describe the hardware.
> 
> Your entire commit is redundantn and not helpful at all.
> 

I will revise the commit message in the next version.
Thanks for your guidance.

> >
> 
> ...
>
> > +
> > +  aspeed,ahbc:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      Phandle to the ASPEED AHB Controller (AHBC) syscon node.
> > +      This reference is used by the PCIe controller to access
> > +      system-level configuration registers related to the AHB bus.
> > +      To enable AHB access for the PCIe controller.
> > +
> > +  aspeed,pciecfg:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      Phandle to the ASPEED PCIe configuration syscon node.
> > +      This reference allows the PCIe controller to access
> > +      SoC-specific PCIe configuration registers. There are the others
> > +      functions such PCIe RC and PCIe EP will use this common register
> > +      to configure the SoC interfaces.
> > +
> > +  aspeed,pciephy:
> 
> No, phys are not syscons. I already told you that in v1.
> 

I will remove the aspeed,pciephy syscon reference and rework this part to use the standard phys binding properly.
Sorry for overlooking your previous feedback in v1.
Thanks again for your patience.

> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      Phandle to the ASPEED PCIe PHY syscon node.
> > +      This property provides access to the PCIe PHY control
> > +      registers required for link initialization and management.
> > +      The other functions such PCIe RC and PCIe EP will use this
> > +      common register to configure the PHY interfaces and get some
> > +      information from the PHY.
> > +
> > +  interrupt-controller:
> > +    description: Interrupt controller node for handling legacy PCI
> interrupts.
> > +    type: object
> > +    properties:
> > +      '#address-cells':
> > +        const: 0
> > +      '#interrupt-cells':
> > +        const: 1
> > +      interrupt-controller: true
> > +
> > +    required:
> > +      - '#address-cells'
> > +      - '#interrupt-cells'
> > +      - interrupt-controller
> > +
> > +    additionalProperties: false
> > +
> > +allOf:
> > +  - $ref: /schemas/pci/pci-bus-common.yaml#
> 
> No other binding references this. Don't write completely different code than all
> other SoCs. This entire binding is written such way.
> 

Agreed. I will remove it in next version.

> > +  - $ref: /schemas/pci/pci-host-bridge.yaml#
> > +  - $ref: /schemas/interrupt-controller/msi-controller.yaml#
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: aspeed,ast2600-pcie
> > +    then:
> > +      required:
> > +        - aspeed,ahbc
> > +    else:
> > +      properties:
> > +        aspeed,ahbc: false
> > +
> > +required:
> > +  - reg
> > +  - interrupts
> > +  - bus-range
> > +  - ranges
> > +  - resets
> > +  - reset-names
> > +  - msi-parent
> > +  - msi-controller
> > +  - aspeed,pciecfg
> > +  - interrupt-map-mask
> > +  - interrupt-map
> > +  - interrupt-controller
> > +
> > +unevaluatedProperties: false
> > +
> > +patternProperties:
> > +  "^pcie@[0-9a-f,]+$":
> 
> Why do you need it? Also, order things according to example schema.
> 

Thanks for your question.

In the v1 discussion, another reviewer suggested that we should support a
multi-port structure for the PCIe root complex, 
where each port is represented as a child node (e.g., pcie@...).
That's why patternProperties was added here — to explicitly allow such
subnodes and validate them properly.

Thanks,
Jacky

Re: 回覆: [PATCH v2 03/10] dt-bindings: PCI: Add ASPEED PCIe RC support

[Krzysztof Kozlowski]

On 21/07/2025 05:44, Jacky Chou wrote:
>>> +patternProperties:
>>> +  "^pcie@[0-9a-f,]+$":
>>
>> Why do you need it? Also, order things according to example schema.
>>
> 
> Thanks for your question.
> 
> In the v1 discussion, another reviewer suggested that we should support a
> multi-port structure for the PCIe root complex, 
> where each port is represented as a child node (e.g., pcie@...).
> That's why patternProperties was added here — to explicitly allow such
> subnodes and validate them properly.

And schema does not allow it already?

Best regards,
Krzysztof

[PATCH v2 03/10] dt-bindings: PCI: Add ASPEED PCIe RC support

[Jacky Chou]

> >>> +patternProperties:
> >>> +  "^pcie@[0-9a-f,]+$":
> >>
> >> Why do you need it? Also, order things according to example schema.
> >>
> >
> > Thanks for your question.
> >
> > In the v1 discussion, another reviewer suggested that we should
> > support a multi-port structure for the PCIe root complex, where each
> > port is represented as a child node (e.g., pcie@...).
> > That's why patternProperties was added here — to explicitly allow such
> > subnodes and validate them properly.
> 
> And schema does not allow it already?
> 

Agreed,
I will remove it in next version.

Thanks,
Jacky

[PATCH v2 04/10] dt-bindings: pinctrl: aspeed,ast2600-pinctrl: Add PCIe RC PERST# group

[Jacky Chou]

Add PCIe PERST# group to support for PCIe RC.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 .../devicetree/bindings/pinctrl/aspeed,ast2600-pinctrl.yaml     | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2600-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2600-pinctrl.yaml
index 80974c46f3ef..5d7fbb1c72b7 100644
--- a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2600-pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2600-pinctrl.yaml
@@ -254,6 +254,7 @@ additionalProperties:
         - WDTRST2
         - WDTRST3
         - WDTRST4
+        - PCIERC1
 
     groups:
       enum:
@@ -497,6 +498,7 @@ additionalProperties:
         - WDTRST2
         - WDTRST3
         - WDTRST4
+        - PCIERC1
 
     pins: true
     bias-disable: true
-- 
2.43.0

Re: [PATCH v2 04/10] dt-bindings: pinctrl: aspeed,ast2600-pinctrl: Add PCIe RC PERST# group

[Krzysztof Kozlowski]

On Tue, Jul 15, 2025 at 11:43:14AM +0800, Jacky Chou wrote:
> Add PCIe PERST# group to support for PCIe RC.
> 
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> ---
>  .../devicetree/bindings/pinctrl/aspeed,ast2600-pinctrl.yaml     | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2600-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2600-pinctrl.yaml
> index 80974c46f3ef..5d7fbb1c72b7 100644
> --- a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2600-pinctrl.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2600-pinctrl.yaml
> @@ -254,6 +254,7 @@ additionalProperties:
>          - WDTRST2
>          - WDTRST3
>          - WDTRST4
> +        - PCIERC1

What feedback Aspeed received about ordering lists? More than once?

Best regards,
Krzysztof

回覆: [PATCH v2 04/10] dt-bindings: pinctrl: aspeed,ast2600-pinctrl: Add PCIe RC PERST# group

[Jacky Chou]

Hi Krzysztof,

Thank you for your reply.

> > Add PCIe PERST# group to support for PCIe RC.
> >
> > Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> > ---
> >  .../devicetree/bindings/pinctrl/aspeed,ast2600-pinctrl.yaml     | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2600-pinctrl.yam
> > l
> > b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2600-pinctrl.yam
> > l index 80974c46f3ef..5d7fbb1c72b7 100644
> > ---
> > a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2600-pinctrl.yam
> > l
> > +++ b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2600-pinctrl
> > +++ .yaml
> > @@ -254,6 +254,7 @@ additionalProperties:
> >          - WDTRST2
> >          - WDTRST3
> >          - WDTRST4
> > +        - PCIERC1
> 
> What feedback Aspeed received about ordering lists? More than once?
> 

Thank you for pointing this out.
We will update the ordering accordingly in the next revision to keep the list sorted.

Thanks,
Jacky

[PATCH v2 05/10] ARM: dts: aspeed-g6: Add AST2600 PCIe RC PERST#

[Jacky Chou]

Add pinctrl support for PCIe RC PERST#.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 arch/arm/boot/dts/aspeed/aspeed-g6-pinctrl.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6-pinctrl.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6-pinctrl.dtsi
index 289668f051eb..ea879f086c25 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-g6-pinctrl.dtsi
+++ b/arch/arm/boot/dts/aspeed/aspeed-g6-pinctrl.dtsi
@@ -2,6 +2,11 @@
 // Copyright 2019 IBM Corp.
 
 &pinctrl {
+	pinctrl_pcierc1_default: pcierc1-default {
+		function = "PCIERC1";
+		groups = "PCIERC1";
+	};
+
 	pinctrl_adc0_default: adc0_default {
 		function = "ADC0";
 		groups = "ADC0";
-- 
2.43.0

[PATCH v2 06/10] ARM: dts: aspeed-g6: Add PCIe RC node

[Jacky Chou]

The AST2600 has one PCIe RC, and add the relative configure regmap.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 arch/arm/boot/dts/aspeed/aspeed-g6.dtsi | 61 +++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
index 8ed715bd53aa..ed99780b6860 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
@@ -379,6 +379,67 @@ rng: hwrng@1e6e2524 {
 				quality = <100>;
 			};
 
+			pcie_phy1: syscon@1e6ed200 {
+				compatible = "aspeed,pcie-phy", "syscon";
+				reg = <0x1e6ed200 0x100>;
+			};
+
+			pcie_cfg: syscon@1e770000 {
+				compatible = "aspeed,pcie-cfg", "syscon";
+				reg = <0x1e770000 0x80>;
+			};
+
+			pcie0: pcie@1e7700c0 {
+				compatible = "aspeed,ast2600-pcie";
+				device_type = "pci";
+				reg = <0x1e7700c0 0x40>;
+				linux,pci-domain = <0>;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				interrupts = <GIC_SPI 168 IRQ_TYPE_LEVEL_HIGH>;
+				bus-range = <0x80 0xff>;
+
+				ranges = <0x01000000 0x0 0x00018000 0x00018000 0x0 0x00008000
+					  0x02000000 0x0 0x70000000 0x70000000 0x0 0x10000000>;
+
+				status = "disabled";
+
+				resets = <&syscon ASPEED_RESET_H2X>;
+				reset-names = "h2x";
+
+				#interrupt-cells = <1>;
+				msi-parent = <&pcie0>;
+				msi-controller;
+
+				aspeed,ahbc = <&ahbc>;
+				aspeed,pciecfg = <&pcie_cfg>;
+
+				interrupt-map-mask = <0 0 0 7>;
+				interrupt-map = <0 0 0 1 &pcie_intc0 0>,
+						<0 0 0 2 &pcie_intc0 1>,
+						<0 0 0 3 &pcie_intc0 2>,
+						<0 0 0 4 &pcie_intc0 3>;
+				pcie_intc0: interrupt-controller {
+					interrupt-controller;
+					#address-cells = <0>;
+					#interrupt-cells = <1>;
+				};
+
+				pcie@8,0 {
+					reg = <0x804000 0 0 0 0>;
+					#address-cells = <3>;
+					#size-cells = <2>;
+					device_type = "pci";
+					resets = <&syscon ASPEED_RESET_PCIE_RC_O>;
+					reset-names = "perst";
+					clocks = <&syscon ASPEED_CLK_GATE_BCLK>;
+					pinctrl-names = "default";
+					pinctrl-0 = <&pinctrl_pcierc1_default>;
+					aspeed,pciephy = <&pcie_phy1>;
+					ranges;
+				};
+			};
+
 			gfx: display@1e6e6000 {
 				compatible = "aspeed,ast2600-gfx", "syscon";
 				reg = <0x1e6e6000 0x1000>;
-- 
2.43.0

Re: [PATCH v2 06/10] ARM: dts: aspeed-g6: Add PCIe RC node

[Rob Herring]

On Mon, Jul 14, 2025 at 10:43 PM Jacky Chou <jacky_chou@aspeedtech.com> wrote:
>
> The AST2600 has one PCIe RC, and add the relative configure regmap.
>
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> ---
>  arch/arm/boot/dts/aspeed/aspeed-g6.dtsi | 61 +++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> index 8ed715bd53aa..ed99780b6860 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> +++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> @@ -379,6 +379,67 @@ rng: hwrng@1e6e2524 {
>                                 quality = <100>;
>                         };
>
> +                       pcie_phy1: syscon@1e6ed200 {
> +                               compatible = "aspeed,pcie-phy", "syscon";
> +                               reg = <0x1e6ed200 0x100>;

This looks like part of something else? It should be a child of that.

If this is the controls for the PCIe PHY, then use the PHY binding
instead of your own custom phandle property.

> +                       };
> +
> +                       pcie_cfg: syscon@1e770000 {
> +                               compatible = "aspeed,pcie-cfg", "syscon";
> +                               reg = <0x1e770000 0x80>;

Looks like this is really part of the PCIe block as a h/w block isn't
going to start at offset 0xc0.


> +                       };
> +
> +                       pcie0: pcie@1e7700c0 {
> +                               compatible = "aspeed,ast2600-pcie";
> +                               device_type = "pci";
> +                               reg = <0x1e7700c0 0x40>;
> +                               linux,pci-domain = <0>;

No need for this. You only have 1 PCI host.

> +                               #address-cells = <3>;
> +                               #size-cells = <2>;
> +                               interrupts = <GIC_SPI 168 IRQ_TYPE_LEVEL_HIGH>;
> +                               bus-range = <0x80 0xff>;

Does this h/w not support bus 0-0x7f for some reason?

> +
> +                               ranges = <0x01000000 0x0 0x00018000 0x00018000 0x0 0x00008000
> +                                         0x02000000 0x0 0x70000000 0x70000000 0x0 0x10000000>;
> +
> +                               status = "disabled";
> +
> +                               resets = <&syscon ASPEED_RESET_H2X>;
> +                               reset-names = "h2x";
> +
> +                               #interrupt-cells = <1>;
> +                               msi-parent = <&pcie0>;
> +                               msi-controller;
> +
> +                               aspeed,ahbc = <&ahbc>;
> +                               aspeed,pciecfg = <&pcie_cfg>;
> +
> +                               interrupt-map-mask = <0 0 0 7>;
> +                               interrupt-map = <0 0 0 1 &pcie_intc0 0>,
> +                                               <0 0 0 2 &pcie_intc0 1>,
> +                                               <0 0 0 3 &pcie_intc0 2>,
> +                                               <0 0 0 4 &pcie_intc0 3>;
> +                               pcie_intc0: interrupt-controller {
> +                                       interrupt-controller;
> +                                       #address-cells = <0>;
> +                                       #interrupt-cells = <1>;
> +                               };
> +
> +                               pcie@8,0 {
> +                                       reg = <0x804000 0 0 0 0>;
> +                                       #address-cells = <3>;
> +                                       #size-cells = <2>;
> +                                       device_type = "pci";
> +                                       resets = <&syscon ASPEED_RESET_PCIE_RC_O>;
> +                                       reset-names = "perst";
> +                                       clocks = <&syscon ASPEED_CLK_GATE_BCLK>;
> +                                       pinctrl-names = "default";
> +                                       pinctrl-0 = <&pinctrl_pcierc1_default>;
> +                                       aspeed,pciephy = <&pcie_phy1>;
> +                                       ranges;
> +                               };
> +                       };
> +
>                         gfx: display@1e6e6000 {
>                                 compatible = "aspeed,ast2600-gfx", "syscon";
>                                 reg = <0x1e6e6000 0x1000>;
> --
> 2.43.0
>

[PATCH v2 06/10] ARM: dts: aspeed-g6: Add PCIe RC node

[Jacky Chou]

Hi Rob,

Thank you for your reply.

> >                                 quality = <100>;
> >                         };
> >
> > +                       pcie_phy1: syscon@1e6ed200 {
> > +                               compatible = "aspeed,pcie-phy",
> "syscon";
> > +                               reg = <0x1e6ed200 0x100>;
> 
> This looks like part of something else? It should be a child of that.
> 
> If this is the controls for the PCIe PHY, then use the PHY binding instead of your
> own custom phandle property.
> 

Our PCIe design has multiple functions. And the series of patches are submitted for
PCIe RC. The other PCIe functions also use this phy node.
I traced the PHY driver interface, it cannot meet our usage.
Therefore, the RC driver uses the phandle property to configure.
And this syscon also is used by the other PCIe functions.

> > +                       };
> > +
> > +                       pcie_cfg: syscon@1e770000 {
> > +                               compatible = "aspeed,pcie-cfg",
> "syscon";
> > +                               reg = <0x1e770000 0x80>;
> 
> Looks like this is really part of the PCIe block as a h/w block isn't going to start
> at offset 0xc0.
> 
> 

Actually.
There are two PCIe bus in AST2600
We use the other one PCIe to EP mode, here I call PCIe A.
I call the pcie0 node as PCIe B.
We do not provide PCIe A to RC mode for usage, just EP mode.
But, when PCIe A is used as RC, it reg mapping is starting from 0x1e770080.
I list there mapping.

0x1e77_0000 ~ 0x1e77_007f : common usage
0x1e77_0080 ~ 0x1e77_00bf : PCIE A
0x1e77_00C0 ~ 0x1e77_00ff : PCIE B

So, it is why we create one node to describe common usage for PCIe A and B.
And, why the pcie0 reg mapping is starting from 0x1e77_00c0.

> > +                       };
> > +
> > +                       pcie0: pcie@1e7700c0 {
> > +                               compatible = "aspeed,ast2600-pcie";
> > +                               device_type = "pci";
> > +                               reg = <0x1e7700c0 0x40>;
> > +                               linux,pci-domain = <0>;
> 
> No need for this. You only have 1 PCI host.
> 

Agreed.
We only provide one RC.

> > +                               #address-cells = <3>;
> > +                               #size-cells = <2>;
> > +                               interrupts = <GIC_SPI 168
> IRQ_TYPE_LEVEL_HIGH>;
> > +                               bus-range = <0x80 0xff>;
> 
> Does this h/w not support bus 0-0x7f for some reason?
> 

List:
PCIE A: 0-0x7f
PCIE B: 0x80-0xff

It is our design on PCIe B to use bus-range 0x80-0xff.

> > +
> > +                               ranges = <0x01000000 0x0
> 0x00018000 0x00018000 0x0 0x00008000
> > +                                         0x02000000 0x0
> 0x70000000
> > + 0x70000000 0x0 0x10000000>;
> > +
> > +                               status = "disabled";
> > +
> > +                               resets = <&syscon
> ASPEED_RESET_H2X>;
> > +                               reset-names = "h2x";
> > +
> > +                               #interrupt-cells = <1>;
> > +                               msi-parent = <&pcie0>;
> > +                               msi-controller;
> > +
> > +                               aspeed,ahbc = <&ahbc>;
> > +                               aspeed,pciecfg = <&pcie_cfg>;
> > +
> > +                               interrupt-map-mask = <0 0 0 7>;
> > +                               interrupt-map = <0 0 0 1 &pcie_intc0
> 0>,
> > +                                               <0 0 0 2
> &pcie_intc0 1>,
> > +                                               <0 0 0 3
> &pcie_intc0 2>,
> > +                                               <0 0 0 4
> &pcie_intc0 3>;
> > +                               pcie_intc0: interrupt-controller {
> > +                                       interrupt-controller;
> > +                                       #address-cells = <0>;
> > +                                       #interrupt-cells = <1>;
> > +                               };
> > +
> > +                               pcie@8,0 {
> > +                                       reg = <0x804000 0 0 0 0>;
> > +                                       #address-cells = <3>;
> > +                                       #size-cells = <2>;
> > +                                       device_type = "pci";
> > +                                       resets = <&syscon
> ASPEED_RESET_PCIE_RC_O>;
> > +                                       reset-names = "perst";
> > +                                       clocks = <&syscon
> ASPEED_CLK_GATE_BCLK>;
> > +                                       pinctrl-names = "default";
> > +                                       pinctrl-0 =
> <&pinctrl_pcierc1_default>;
> > +                                       aspeed,pciephy =
> <&pcie_phy1>;
> > +                                       ranges;
> > +                               };
> > +                       };
> > +
> >                         gfx: display@1e6e6000 {
> >                                 compatible = "aspeed,ast2600-gfx",
> "syscon";
> >                                 reg = <0x1e6e6000 0x1000>;
> > --
> > 2.43.0
> >

Re: [PATCH v2 06/10] ARM: dts: aspeed-g6: Add PCIe RC node

[Rob Herring]

On Wed, Jul 16, 2025 at 03:51:11AM +0000, Jacky Chou wrote:
> Hi Rob,
> 
> Thank you for your reply.
> 
> > >                                 quality = <100>;
> > >                         };
> > >
> > > +                       pcie_phy1: syscon@1e6ed200 {
> > > +                               compatible = "aspeed,pcie-phy",
> > "syscon";
> > > +                               reg = <0x1e6ed200 0x100>;
> > 
> > This looks like part of something else? It should be a child of that.
> > 
> > If this is the controls for the PCIe PHY, then use the PHY binding instead of your
> > own custom phandle property.
> > 
> 
> Our PCIe design has multiple functions. And the series of patches are submitted for
> PCIe RC. The other PCIe functions also use this phy node.
> I traced the PHY driver interface, it cannot meet our usage.

Why not?

There is also no requirement that using the DT PHY binding means you 
have to use the Linux PHY subsystem.

> Therefore, the RC driver uses the phandle property to configure.
> And this syscon also is used by the other PCIe functions.

Like what?

> > > +                       };
> > > +
> > > +                       pcie_cfg: syscon@1e770000 {
> > > +                               compatible = "aspeed,pcie-cfg",
> > "syscon";
> > > +                               reg = <0x1e770000 0x80>;
> > 
> > Looks like this is really part of the PCIe block as a h/w block isn't going to start
> > at offset 0xc0.
> > 
> > 
> 
> Actually.
> There are two PCIe bus in AST2600
> We use the other one PCIe to EP mode, here I call PCIe A.
> I call the pcie0 node as PCIe B.
> We do not provide PCIe A to RC mode for usage, just EP mode.
> But, when PCIe A is used as RC, it reg mapping is starting from 0x1e770080.
> I list there mapping.
> 
> 0x1e77_0000 ~ 0x1e77_007f : common usage
> 0x1e77_0080 ~ 0x1e77_00bf : PCIE A
> 0x1e77_00C0 ~ 0x1e77_00ff : PCIE B
> 
> So, it is why we create one node to describe common usage for PCIe A and B.
> And, why the pcie0 reg mapping is starting from 0x1e77_00c0.

In that case, maybe you need a common parent node with 2 child nodes for 
each bus.

Rob

> 
> > > +                       };
> > > +
> > > +                       pcie0: pcie@1e7700c0 {
> > > +                               compatible = "aspeed,ast2600-pcie";
> > > +                               device_type = "pci";
> > > +                               reg = <0x1e7700c0 0x40>;
> > > +                               linux,pci-domain = <0>;
> > 
> > No need for this. You only have 1 PCI host.
> > 
> 
> Agreed.
> We only provide one RC.
> 
> > > +                               #address-cells = <3>;
> > > +                               #size-cells = <2>;
> > > +                               interrupts = <GIC_SPI 168
> > IRQ_TYPE_LEVEL_HIGH>;
> > > +                               bus-range = <0x80 0xff>;
> > 
> > Does this h/w not support bus 0-0x7f for some reason?
> > 
> 
> List:
> PCIE A: 0-0x7f
> PCIE B: 0x80-0xff
> 
> It is our design on PCIe B to use bus-range 0x80-0xff.

That's a policy or h/w limitation?

Rob

回覆: [PATCH v2 06/10] ARM: dts: aspeed-g6: Add PCIe RC node

[Jacky Chou]

> > > >                                 quality = <100>;
> > > >                         };
> > > >
> > > > +                       pcie_phy1: syscon@1e6ed200 {
> > > > +                               compatible = "aspeed,pcie-phy",
> > > "syscon";
> > > > +                               reg = <0x1e6ed200 0x100>;
> > >
> > > This looks like part of something else? It should be a child of that.
> > >
> > > If this is the controls for the PCIe PHY, then use the PHY binding
> > > instead of your own custom phandle property.
> > >
> >
> > Our PCIe design has multiple functions. And the series of patches are
> > submitted for PCIe RC. The other PCIe functions also use this phy node.
> > I traced the PHY driver interface, it cannot meet our usage.
> 
> Why not?
> 
> There is also no requirement that using the DT PHY binding means you have to
> use the Linux PHY subsystem.
> 

Got it. I always focused on when using the "phys" property, I must use the Linux PHY
subsystem. I will change this part to use the "phys" property instead of our definition
property.
Thank you for your comments.

> > Therefore, the RC driver uses the phandle property to configure.
> > And this syscon also is used by the other PCIe functions.
> 
> Like what?
> 

Other PCIe functions such as MCTP also use the PHY interface.

> > > > +                       };
> > > > +
> > > > +                       pcie_cfg: syscon@1e770000 {
> > > > +                               compatible = "aspeed,pcie-cfg",
> > > "syscon";
> > > > +                               reg = <0x1e770000 0x80>;
> > >
> > > Looks like this is really part of the PCIe block as a h/w block
> > > isn't going to start at offset 0xc0.
> > >
> > >
> >
> > Actually.
> > There are two PCIe bus in AST2600
> > We use the other one PCIe to EP mode, here I call PCIe A.
> > I call the pcie0 node as PCIe B.
> > We do not provide PCIe A to RC mode for usage, just EP mode.
> > But, when PCIe A is used as RC, it reg mapping is starting from 0x1e770080.
> > I list there mapping.
> >
> > 0x1e77_0000 ~ 0x1e77_007f : common usage
> > 0x1e77_0080 ~ 0x1e77_00bf : PCIE A
> > 0x1e77_00C0 ~ 0x1e77_00ff : PCIE B
> >
> > So, it is why we create one node to describe common usage for PCIe A and B.
> > And, why the pcie0 reg mapping is starting from 0x1e77_00c0.
> 
> In that case, maybe you need a common parent node with 2 child nodes for
> each bus.

Got it. But we may remove the pcie_cfg node and merge these register regions.

> >
> > > > +                       };
> > > > +
> > > > +                       pcie0: pcie@1e7700c0 {
> > > > +                               compatible =
> "aspeed,ast2600-pcie";
> > > > +                               device_type = "pci";
> > > > +                               reg = <0x1e7700c0 0x40>;
> > > > +                               linux,pci-domain = <0>;
> > >
> > > No need for this. You only have 1 PCI host.
> > >
> >
> > Agreed.
> > We only provide one RC.
> >
> > > > +                               #address-cells = <3>;
> > > > +                               #size-cells = <2>;
> > > > +                               interrupts = <GIC_SPI 168
> > > IRQ_TYPE_LEVEL_HIGH>;
> > > > +                               bus-range = <0x80 0xff>;
> > >
> > > Does this h/w not support bus 0-0x7f for some reason?
> > >
> >
> > List:
> > PCIE A: 0-0x7f
> > PCIE B: 0x80-0xff
> >
> > It is our design on PCIe B to use bus-range 0x80-0xff.
> 
> That's a policy or h/w limitation?
> 

It is a hardware limitation of this PCIe RC.

Thanks,
Jacky

[PATCH v2 07/10] pinctrl: aspeed-g6: Add PCIe RC PERST pin group

[Jacky Chou]

The PCIe RC PERST uses SSPRST# as PERST#  and enable this pin
to output.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
index 5a7cd0a88687..c751703acdb9 100644
--- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
+++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
@@ -17,6 +17,7 @@
 #include "../pinctrl-utils.h"
 #include "pinctrl-aspeed.h"
 
+#define SCU040		0x040 /* Reset Control Set 1  */
 #define SCU400		0x400 /* Multi-function Pin Control #1  */
 #define SCU404		0x404 /* Multi-function Pin Control #2  */
 #define SCU40C		0x40C /* Multi-function Pin Control #3  */
@@ -52,7 +53,7 @@
 #define SCU6D0		0x6D0 /* Multi-function Pin Control #29 */
 #define SCUC20		0xC20 /* PCIE configuration Setting Control */
 
-#define ASPEED_G6_NR_PINS 256
+#define ASPEED_G6_NR_PINS 258
 
 #define M24 0
 SIG_EXPR_LIST_DECL_SESG(M24, MDC3, MDIO3, SIG_DESC_SET(SCU410, 0));
@@ -1636,6 +1637,12 @@ FUNC_DECL_1(USB11BHID, USBB);
 FUNC_DECL_1(USB2BD, USBB);
 FUNC_DECL_1(USB2BH, USBB);
 
+#define D7 257
+SIG_EXPR_LIST_DECL_SESG(D7, RCRST, PCIERC1, SIG_DESC_SET(SCU040, 19),
+			SIG_DESC_SET(SCU500, 24));
+PIN_DECL_(D7, SIG_EXPR_LIST_PTR(D7, RCRST));
+FUNC_GROUP_DECL(PCIERC1, D7);
+
 /* Pins, groups and functions are sort(1):ed alphabetically for sanity */
 
 static struct pinctrl_pin_desc aspeed_g6_pins[ASPEED_G6_NR_PINS] = {
@@ -1806,6 +1813,7 @@ static struct pinctrl_pin_desc aspeed_g6_pins[ASPEED_G6_NR_PINS] = {
 	ASPEED_PINCTRL_PIN(D4),
 	ASPEED_PINCTRL_PIN(D5),
 	ASPEED_PINCTRL_PIN(D6),
+	ASPEED_PINCTRL_PIN(D7),
 	ASPEED_PINCTRL_PIN(E1),
 	ASPEED_PINCTRL_PIN(E11),
 	ASPEED_PINCTRL_PIN(E12),
@@ -2073,6 +2081,7 @@ static const struct aspeed_pin_group aspeed_g6_groups[] = {
 	ASPEED_PINCTRL_GROUP(SALT9G1),
 	ASPEED_PINCTRL_GROUP(SD1),
 	ASPEED_PINCTRL_GROUP(SD2),
+	ASPEED_PINCTRL_GROUP(PCIERC1),
 	ASPEED_PINCTRL_GROUP(EMMCG1),
 	ASPEED_PINCTRL_GROUP(EMMCG4),
 	ASPEED_PINCTRL_GROUP(EMMCG8),
@@ -2314,6 +2323,7 @@ static const struct aspeed_pin_function aspeed_g6_functions[] = {
 	ASPEED_PINCTRL_FUNC(SPI2),
 	ASPEED_PINCTRL_FUNC(SPI2CS1),
 	ASPEED_PINCTRL_FUNC(SPI2CS2),
+	ASPEED_PINCTRL_FUNC(PCIERC1),
 	ASPEED_PINCTRL_FUNC(TACH0),
 	ASPEED_PINCTRL_FUNC(TACH1),
 	ASPEED_PINCTRL_FUNC(TACH10),
-- 
2.43.0

Re: [PATCH v2 07/10] pinctrl: aspeed-g6: Add PCIe RC PERST pin group

[Linus Walleij]

On Tue, Jul 15, 2025 at 5:43 AM Jacky Chou <jacky_chou@aspeedtech.com> wrote:

> The PCIe RC PERST uses SSPRST# as PERST#  and enable this pin
> to output.
>
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>

This patch 7/10 applied to the pinctrl tree, why not.

Yours,
Linus Walleij

[PATCH v2 07/10] pinctrl: aspeed-g6: Add PCIe RC PERST pin group

[Jacky Chou]

Hi Linus,

Thank you for your reply.

> > The PCIe RC PERST uses SSPRST# as PERST#  and enable this pin to
> > output.
> >
> > Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> 
> This patch 7/10 applied to the pinctrl tree, why not.
> 

Sorry. I'm late in replying to you.
I miss the patch for replying to you on version 1.
Thanks, this patch can apply to the pinctrl tree.
May I remove this patch in the next version of this series?

Thanks,
Jacky

Re: [PATCH v2 07/10] pinctrl: aspeed-g6: Add PCIe RC PERST pin group

[Linus Walleij]

On Wed, Aug 27, 2025 at 5:08 AM Jacky Chou <jacky_chou@aspeedtech.com> wrote:

> May I remove this patch in the next version of this series?

Yes, and in fact it could have been sent separately from the
rest as well, no need to keep things in a big bundle, it's
easier to merge in small pieces.

The only upside with the big bundles is to help developers
develop all in one place and have a "big branch" to test.
But it doesn't really help the Linux subsystem maintainers.

Yours,
Linus Walleij

[PATCH v2 07/10] pinctrl: aspeed-g6: Add PCIe RC PERST pin group

[Jacky Chou]

Hi Linus

> > May I remove this patch in the next version of this series?
> 
> Yes, and in fact it could have been sent separately from the rest as well, no
> need to keep things in a big bundle, it's easier to merge in small pieces.
> 
> The only upside with the big bundles is to help developers develop all in one
> place and have a "big branch" to test.
> But it doesn't really help the Linux subsystem maintainers.
> 

Thank you.
Next time I send a series of patches, I will consider whether it can be split into smaller topic.

Thanks,
Jacky

[PATCH v2 08/10] PCI: Add FMT and TYPE definition for TLP header

[Jacky Chou]

According to PCIe specification, add FMT and TYPE definition
for TLP header. And also add macro to combine FMT and TYPE to
1 byte.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 include/uapi/linux/pci_regs.h | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index a3a3e942dedf..700b915e00f5 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1230,4 +1230,36 @@
 #define PCI_DVSEC_CXL_PORT_CTL				0x0c
 #define PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR		0x00000001
 
+/* Fmt[2:0] encoding for TLP Header */
+#define PCI_TLP_FMT_3DW_NO_DATA		0x0  // 3DW header, no data
+#define PCI_TLP_FMT_4DW_NO_DATA		0x1  // 4DW header, no data
+#define PCI_TLP_FMT_3DW_DATA		0x2  // 3DW header, with data
+#define PCI_TLP_FMT_4DW_DATA		0x3  // 4DW header, with data
+#define PCI_TLP_FMT_PREFIX		0x4  // Prefix header
+
+/* Type[4:0] encoding for TLP Header */
+#define PCI_TLP_TYPE_MEM_RD		0x00 // Memory Read Request
+#define PCI_TLP_TYPE_MEM_RDLK		0x01 // Memory Read Lock Request
+#define PCI_TLP_TYPE_MEM_WR		0x00 // Memory Write Request (Fmt must be with data)
+#define PCI_TLP_TYPE_IO_RD		0x02 // IO Read Request
+#define PCI_TLP_TYPE_IO_WR		0x02 // IO Write Request (Fmt must be with data)
+#define PCI_TLP_TYPE_CFG0_RD		0x04 // Config Type 0 Read Request
+#define PCI_TLP_TYPE_CFG0_WR		0x04 // Config Type 0 Write Request (Fmt must be with data)
+#define PCI_TLP_TYPE_CFG1_RD		0x05 // Config Type 1 Read Request
+#define PCI_TLP_TYPE_CFG1_WR		0x05 // Config Type 1 Write Request (Fmt must be with data)
+#define PCI_TLP_TYPE_MSG		0x10 // Message Request (see routing field)
+#define PCI_TLP_TYPE_MSGD		0x11 // Message Request with Data (see routing field)
+#define PCI_TLP_TYPE_CPL		0x0A // Completion without Data
+#define PCI_TLP_TYPE_CPLD		0x0A // Completion with Data (Fmt must be with data)
+#define PCI_TLP_TYPE_CPLLCK		0x0B // Completion Locked
+#define PCI_TLP_TYPE_CPLDLCK		0x0B // Completion with Data Locked (Fmt must be with data)
+#define PCI_TLP_TYPE_FETCH_ADD		0x0C // Fetch and Add AtomicOp Request
+#define PCI_TLP_TYPE_SWAP		0x0D // Unconditional Swap AtomicOp Request
+#define PCI_TLP_TYPE_CMP_SWAP		0x0E // Compare and Swap AtomicOp Request
+#define PCI_TLP_TYPE_LOCAL_PREFIX	0x00 // Local TLP Prefix (Fmt = 0x4)
+#define PCI_TLP_TYPE_E2E_PREFIX		0x10 // End-to-End TLP Prefix (Fmt = 0x4)
+
+/* Macro to combine Fmt and Type into the 8-bit field */
+#define PCIE_TLP_FMT_TYPE(fmt, type)   (((fmt) << 5) | ((type) & 0x1F))
+
 #endif /* LINUX_PCI_REGS_H */
-- 
2.43.0

Re: [PATCH v2 08/10] PCI: Add FMT and TYPE definition for TLP header

[Bjorn Helgaas]

On Tue, Jul 15, 2025 at 11:43:18AM +0800, Jacky Chou wrote:
> According to PCIe specification, add FMT and TYPE definition
> for TLP header. And also add macro to combine FMT and TYPE to
> 1 byte.
> 
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> ---
>  include/uapi/linux/pci_regs.h | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)

I don't think these definitions are relevant to uapi users, so they
could go in drivers/pci/pci.h, similar to the existing PCIE_MSG_*
definitions.

Please follow the style of PCIE_MSG_*, including the brief spec
citations and /* */ comments.

Not sure we need *all* of these; it looks like you only use:

  PCI_TLP_TYPE_CFG0_RD
  PCI_TLP_TYPE_CFG0_WR
  PCI_TLP_TYPE_CFG1_RD
  PCI_TLP_TYPE_CFG1_WR
  PCI_TLP_FMT_3DW_NO_DATA
  PCI_TLP_FMT_3DW_DATA

> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index a3a3e942dedf..700b915e00f5 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1230,4 +1230,36 @@
>  #define PCI_DVSEC_CXL_PORT_CTL				0x0c
>  #define PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR		0x00000001
>  
> +/* Fmt[2:0] encoding for TLP Header */
> +#define PCI_TLP_FMT_3DW_NO_DATA		0x0  // 3DW header, no data
> +#define PCI_TLP_FMT_4DW_NO_DATA		0x1  // 4DW header, no data
> +#define PCI_TLP_FMT_3DW_DATA		0x2  // 3DW header, with data
> +#define PCI_TLP_FMT_4DW_DATA		0x3  // 4DW header, with data
> +#define PCI_TLP_FMT_PREFIX		0x4  // Prefix header
> +
> +/* Type[4:0] encoding for TLP Header */
> +#define PCI_TLP_TYPE_MEM_RD		0x00 // Memory Read Request
> +#define PCI_TLP_TYPE_MEM_RDLK		0x01 // Memory Read Lock Request
> +#define PCI_TLP_TYPE_MEM_WR		0x00 // Memory Write Request (Fmt must be with data)
> +#define PCI_TLP_TYPE_IO_RD		0x02 // IO Read Request
> +#define PCI_TLP_TYPE_IO_WR		0x02 // IO Write Request (Fmt must be with data)
> +#define PCI_TLP_TYPE_CFG0_RD		0x04 // Config Type 0 Read Request
> +#define PCI_TLP_TYPE_CFG0_WR		0x04 // Config Type 0 Write Request (Fmt must be with data)
> +#define PCI_TLP_TYPE_CFG1_RD		0x05 // Config Type 1 Read Request
> +#define PCI_TLP_TYPE_CFG1_WR		0x05 // Config Type 1 Write Request (Fmt must be with data)
> +#define PCI_TLP_TYPE_MSG		0x10 // Message Request (see routing field)
> +#define PCI_TLP_TYPE_MSGD		0x11 // Message Request with Data (see routing field)
> +#define PCI_TLP_TYPE_CPL		0x0A // Completion without Data
> +#define PCI_TLP_TYPE_CPLD		0x0A // Completion with Data (Fmt must be with data)
> +#define PCI_TLP_TYPE_CPLLCK		0x0B // Completion Locked
> +#define PCI_TLP_TYPE_CPLDLCK		0x0B // Completion with Data Locked (Fmt must be with data)
> +#define PCI_TLP_TYPE_FETCH_ADD		0x0C // Fetch and Add AtomicOp Request
> +#define PCI_TLP_TYPE_SWAP		0x0D // Unconditional Swap AtomicOp Request
> +#define PCI_TLP_TYPE_CMP_SWAP		0x0E // Compare and Swap AtomicOp Request
> +#define PCI_TLP_TYPE_LOCAL_PREFIX	0x00 // Local TLP Prefix (Fmt = 0x4)
> +#define PCI_TLP_TYPE_E2E_PREFIX		0x10 // End-to-End TLP Prefix (Fmt = 0x4)
> +
> +/* Macro to combine Fmt and Type into the 8-bit field */
> +#define PCIE_TLP_FMT_TYPE(fmt, type)   (((fmt) << 5) | ((type) & 0x1F))

This looks like it might be controller-specific and could go in
pcie-aspeed.c.

[PATCH v2 08/10] PCI: Add FMT and TYPE definition for TLP header

[Jacky Chou]

Hi Bjorn,

Thank you for your reply.

> >  include/uapi/linux/pci_regs.h | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> 
> I don't think these definitions are relevant to uapi users, so they could go in
> drivers/pci/pci.h, similar to the existing PCIE_MSG_* definitions.

Agreed.
I will move them to drivers/pci/pci.h and rename them in next version.

> 
> Please follow the style of PCIE_MSG_*, including the brief spec citations and
> /* */ comments.
> 
> Not sure we need *all* of these; it looks like you only use:
> 
>   PCI_TLP_TYPE_CFG0_RD
>   PCI_TLP_TYPE_CFG0_WR
>   PCI_TLP_TYPE_CFG1_RD
>   PCI_TLP_TYPE_CFG1_WR
>   PCI_TLP_FMT_3DW_NO_DATA
>   PCI_TLP_FMT_3DW_DATA
>

Yes, I just use these in our PCIe RC driver.
I will delete the others that are not used in next version.

> > diff --git a/include/uapi/linux/pci_regs.h
> > b/include/uapi/linux/pci_regs.h index a3a3e942dedf..700b915e00f5
> > 100644
> > --- a/include/uapi/linux/pci_regs.h
> > +++ b/include/uapi/linux/pci_regs.h
> > @@ -1230,4 +1230,36 @@
> >  #define PCI_DVSEC_CXL_PORT_CTL				0x0c
> >  #define PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR		0x00000001
> >
> > +/* Fmt[2:0] encoding for TLP Header */
> > +#define PCI_TLP_FMT_3DW_NO_DATA		0x0  // 3DW header, no data
> > +#define PCI_TLP_FMT_4DW_NO_DATA		0x1  // 4DW header, no data
> > +#define PCI_TLP_FMT_3DW_DATA		0x2  // 3DW header, with data
> > +#define PCI_TLP_FMT_4DW_DATA		0x3  // 4DW header, with data
> > +#define PCI_TLP_FMT_PREFIX		0x4  // Prefix header
> > +
> > +/* Type[4:0] encoding for TLP Header */
> > +#define PCI_TLP_TYPE_MEM_RD		0x00 // Memory Read Request
> > +#define PCI_TLP_TYPE_MEM_RDLK		0x01 // Memory Read Lock
> Request
> > +#define PCI_TLP_TYPE_MEM_WR		0x00 // Memory Write Request
> (Fmt must be with data)
> > +#define PCI_TLP_TYPE_IO_RD		0x02 // IO Read Request
> > +#define PCI_TLP_TYPE_IO_WR		0x02 // IO Write Request (Fmt must be
> with data)
> > +#define PCI_TLP_TYPE_CFG0_RD		0x04 // Config Type 0 Read
> Request
> > +#define PCI_TLP_TYPE_CFG0_WR		0x04 // Config Type 0 Write
> Request (Fmt must be with data)
> > +#define PCI_TLP_TYPE_CFG1_RD		0x05 // Config Type 1 Read
> Request
> > +#define PCI_TLP_TYPE_CFG1_WR		0x05 // Config Type 1 Write
> Request (Fmt must be with data)
> > +#define PCI_TLP_TYPE_MSG		0x10 // Message Request (see routing
> field)
> > +#define PCI_TLP_TYPE_MSGD		0x11 // Message Request with Data
> (see routing field)
> > +#define PCI_TLP_TYPE_CPL		0x0A // Completion without Data
> > +#define PCI_TLP_TYPE_CPLD		0x0A // Completion with Data (Fmt
> must be with data)
> > +#define PCI_TLP_TYPE_CPLLCK		0x0B // Completion Locked
> > +#define PCI_TLP_TYPE_CPLDLCK		0x0B // Completion with Data
> Locked (Fmt must be with data)
> > +#define PCI_TLP_TYPE_FETCH_ADD		0x0C // Fetch and Add AtomicOp
> Request
> > +#define PCI_TLP_TYPE_SWAP		0x0D // Unconditional Swap AtomicOp
> Request
> > +#define PCI_TLP_TYPE_CMP_SWAP		0x0E // Compare and Swap
> AtomicOp Request
> > +#define PCI_TLP_TYPE_LOCAL_PREFIX	0x00 // Local TLP Prefix (Fmt =
> 0x4)
> > +#define PCI_TLP_TYPE_E2E_PREFIX		0x10 // End-to-End TLP Prefix (Fmt
> = 0x4)
> > +
> > +/* Macro to combine Fmt and Type into the 8-bit field */
> > +#define PCIE_TLP_FMT_TYPE(fmt, type)   (((fmt) << 5) | ((type) & 0x1F))
> 
> This looks like it might be controller-specific and could go in pcie-aspeed.c.

Agreed.

Thanks,
Jacky

Re: [PATCH v2 08/10] PCI: Add FMT and TYPE definition for TLP header

[kernel test robot]

Hi Jacky,

kernel test robot noticed the following build warnings:

[auto build test WARNING on pci/next]
[also build test WARNING on pci/for-linus robh/for-next linusw-pinctrl/devel linusw-pinctrl/for-next linus/master v6.16-rc6 next-20250715]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jacky-Chou/dt-bindings-soc-aspeed-Add-ASPEED-PCIe-Config-support/20250715-114814
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20250715034320.2553837-9-jacky_chou%40aspeedtech.com
patch subject: [PATCH v2 08/10] PCI: Add FMT and TYPE definition for TLP header
config: i386-buildonly-randconfig-004-20250715 (https://download.01.org/0day-ci/archive/20250716/202507160314.e3odwyX7-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250716/202507160314.e3odwyX7-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507160314.e3odwyX7-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from <built-in>:1:
   In file included from ./usr/include/linux/pci.h:21:
>> usr/include/linux/pci_regs.h:1234:39: warning: // comments are not allowed in this language [-Wcomment]
    1234 | #define PCI_TLP_FMT_3DW_NO_DATA         0x0  // 3DW header, no data
         |                                              ^
   1 warning generated.
--
   In file included from <built-in>:1:
>> ./usr/include/linux/pci_regs.h:1234:39: warning: // comments are not allowed in this language [-Wcomment]
    1234 | #define PCI_TLP_FMT_3DW_NO_DATA         0x0  // 3DW header, no data
         |                                              ^
   1 warning generated.

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v2 09/10] PCI: aspeed: Add ASPEED PCIe RC driver

[Jacky Chou]

Introduce PCIe Root Complex driver for ASPEED SoCs. Support RC
initialization, reset, clock, IRQ domain, and MSI domain setup.
Implement platform-specific setup and register configuration for
ASPEED. And provide PCI config space read/write and INTx/MSI
interrupt handling.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/pci/controller/Kconfig       |   13 +
 drivers/pci/controller/Makefile      |    1 +
 drivers/pci/controller/pcie-aspeed.c | 1137 ++++++++++++++++++++++++++
 3 files changed, 1151 insertions(+)
 create mode 100644 drivers/pci/controller/pcie-aspeed.c

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 886f6f43a895..acab0dd48e8d 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -52,6 +52,19 @@ config PCIE_APPLE
 
 	  If unsure, say Y if you have an Apple Silicon system.
 
+config PCIE_ASPEED
+	bool "ASPEED PCIe controller"
+	depends on ARCH_ASPEED || COMPILE_TEST
+	depends on OF
+	select PCI_MSI_ARCH_FALLBACKS
+	help
+	  Enable this option to add support for the PCIe controller
+	  found on ASPEED SoCs.
+	  This driver provides initialization and management for PCIe
+	  Root Complex functionality, including interrupt and MSI support.
+	  Select Y if your platform uses an ASPEED SoC and requires PCIe
+	  connectivity.
+
 config PCI_VERSATILE
 	bool "ARM Versatile PB PCI controller"
 	depends on ARCH_VERSATILE || COMPILE_TEST
diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
index 038ccbd9e3ba..1339f88e153d 100644
--- a/drivers/pci/controller/Makefile
+++ b/drivers/pci/controller/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_PCI_LOONGSON) += pci-loongson.o
 obj-$(CONFIG_PCIE_HISI_ERR) += pcie-hisi-error.o
 obj-$(CONFIG_PCIE_APPLE) += pcie-apple.o
 obj-$(CONFIG_PCIE_MT7621) += pcie-mt7621.o
+obj-$(CONFIG_PCIE_ASPEED) += pcie-aspeed.o
 
 # pcie-hisi.o quirks are needed even without CONFIG_PCIE_DW
 obj-y				+= dwc/
diff --git a/drivers/pci/controller/pcie-aspeed.c b/drivers/pci/controller/pcie-aspeed.c
new file mode 100644
index 000000000000..a7e679d5fb42
--- /dev/null
+++ b/drivers/pci/controller/pcie-aspeed.c
@@ -0,0 +1,1137 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2025 Aspeed Technology Inc.
+ */
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/msi.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_pci.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+#define MAX_MSI_HOST_IRQS	64
+#define PCIE_RESET_CONFIG_DEVICE_WAIT_MS	500
+#define PCIE_RESET_CONFIG_RC_WAIT_MS		10
+
+/* AST2600 AHBC Registers */
+#define AHBC_KEY		0x00
+#define  AHBC_UNLOCK_KEY		0xAEED1A03
+#define  AHBC_UNLOCK			0x01
+#define AHBC_ADDR_MAPPING	0x8C
+#define  PCIE_RC_MEMORY_EN		BIT(5)
+
+/* AST2600 PCIe Host Controller Registers */
+#define PEHR_GLOBAL		0x30
+#define  AST2600_PORT_TYPE_MASK		GENMASK(5, 4)
+#define  AST2600_PORT_TYPE(x)		FIELD_PREP(AST2600_PORT_TYPE_MASK, x)
+#define PEHR_LOCK		0x7C
+#define  PCIE_UNLOCK			0xa8
+#define PEHR_LINK		0xC0
+#define  PCIE_LINK_UP			BIT(5)
+
+/* AST2600 H2X Controller Registers */
+/* Common Registers*/
+#define H2X_INT_STS		0x08
+#define  PCIE_TX_IDLE_CLEAR		BIT(0)
+#define  PCIE_INTX_STS			GENMASK(3, 0)
+#define H2X_TX_DESC0		0x10
+#define H2X_TX_DESC1		0x14
+#define H2X_TX_DESC2		0x18
+#define H2X_TX_DESC3		0x1C
+#define H2X_TX_DESC_DATA	0x20
+#define H2X_STS			0x24
+#define  PCIE_TX_IDLE			BIT(31)
+#define  PCIE_STATUS_OF_TX		GENMASK(25, 24)
+#define	 PCIE_RC_L_TX_COMPLETE		BIT(24)
+#define	 PCIE_RC_H_TX_COMPLETE		BIT(25)
+#define  PCIE_TRIGGER_TX		BIT(0)
+#define H2X_AHB_ADDR_CONFIG0	0x60
+#define  AHB_REMAP_LO_ADDR(x)		FIELD_PREP(GENMASK(15, 4), x)
+#define  AHB_MASK_LO_ADDR(x)		FIELD_PREP(GENMASK(31, 20), x)
+#define H2X_AHB_ADDR_CONFIG1	0x64
+#define  AHB_REMAP_HI_ADDR(x)		(x)
+#define H2X_AHB_ADDR_CONFIG2	0x68
+#define  AHB_MASK_HI_ADDR(x)		(x)
+
+/* Device Registers */
+#define H2X_DEV_CTRL		0x00
+#define  PCIE_RX_DMA_EN			BIT(9)
+#define  PCIE_RX_LINEAR			BIT(8)
+#define  PCIE_RX_MSI_SEL		BIT(7)
+#define  PCIE_RX_MSI_EN			BIT(6)
+#define  PCIE_UNLOCK_RX_BUFF		BIT(4)
+#define  PCIE_Wait_RX_TLP_CLR		BIT(2)
+#define  PCIE_RC_RX_ENABLE		BIT(1)
+#define  PCIE_RC_ENABLE			BIT(0)
+#define H2X_DEV_STS		0x08
+#define  PCIE_RC_RX_DONE_ISR		BIT(4)
+#define H2X_DEV_RX_DESC_DATA	0x0C
+#define H2X_DEV_RX_DESC1	0x14
+#define H2X_DEV_TX_TAG		0x3C
+
+/* AST2700 H2X */
+#define H2X_CTRL		0x00
+#define  H2X_BRIDGE_EN			BIT(0)
+#define  H2X_BRIDGE_DIRECT_EN		BIT(1)
+#define H2X_CFGE_INT_STS	0x08
+#define  CFGE_TX_IDLE			BIT(0)
+#define  CFGE_RX_BUSY			BIT(1)
+#define H2X_CFGI_TLP		0x20
+#define  CFGI_BYTE_EN_MASK		GENMASK(19, 16)
+#define  CFGI_BYTE_EN(x)		FIELD_PREP(CFGI_BYTE_EN_MASK, (x))
+#define H2X_CFGI_WR_DATA	0x24
+#define  CFGI_WRITE			BIT(20)
+#define H2X_CFGI_CTRL		0x28
+#define  CFGI_TLP_FIRE			BIT(0)
+#define H2X_CFGI_RET_DATA	0x2C
+#define H2X_CFGE_TLP_1ST	0x30
+#define H2X_CFGE_TLP_NEXT	0x34
+#define H2X_CFGE_CTRL		0x38
+#define  CFGE_TLP_FIRE			BIT(0)
+#define H2X_CFGE_RET_DATA	0x3C
+#define H2X_REMAP_PREF_ADDR	0x70
+#define  REMAP_PREF_ADDR_63_32(x)	(x)
+#define H2X_REMAP_DIRECT_ADDR	0x78
+#define  REMAP_BAR_BASE(x)		(x)
+
+/* AST2700 PEHR */
+#define PEHR_MISC_58		0x58
+#define  LOCAL_SCALE_SUP		BIT(0)
+#define PEHR_MISC_5C		0x5C
+#define  CONFIG_RC_DEVICE		BIT(30)
+#define PEHR_MISC_60		0x60
+#define  AST2700_PORT_TYPE_MASK		GENMASK(7, 4)
+#define  PORT_TYPE_ROOT			BIT(2)
+#define PEHR_MISC_70		0x70
+#define  POSTED_DATA_CREDITS(x)		FIELD_PREP(GENMASK(15, 0), x)
+#define  POSTED_HEADER_CREDITS(x)	FIELD_PREP(GENMASK(27, 16), x)
+#define PEHR_MISC_78		0x78
+#define  COMPLETION_DATA_CREDITS(x)	FIELD_PREP(GENMASK(15, 0), x)
+#define  COMPLETION_HEADER_CREDITS(x)	FIELD_PREP(GENMASK(27, 16), x)
+#define PEHR_MISC_300		0x300
+#define  RC_GEN2		BIT(0)
+#define PEHR_MISC_344		0x344
+#define  LINK_UP_GEN2			BIT(18)
+#define PEHR_MISC_358		0x358
+#define  LINK_UP_GEN4			BIT(8)
+
+/* AST2700 SCU */
+#define SCU_60			0x60
+#define  RC_E2M_PATH_EN			BIT(0)
+#define  RC_H2XS_PATH_EN		BIT(16)
+#define  RC_H2XD_PATH_EN		BIT(17)
+#define  RC_H2XX_PATH_EN		BIT(18)
+#define  RC_UPSTREAM_MEM_EN		BIT(19)
+#define SCU_64			0x64
+#define  RC0_DECODE_DMA_BASE(x)		FIELD_PREP(GENMASK(7, 0), x)
+#define  RC0_DECODE_DMA_LIMIT(x)	FIELD_PREP(GENMASK(15, 8), x)
+#define  RC1_DECODE_DMA_BASE(x)		FIELD_PREP(GENMASK(23, 16), x)
+#define  RC1_DECODE_DMA_LIMIT(x)	FIELD_PREP(GENMASK(31, 24), x)
+#define SCU_70			0x70
+#define  DISABLE_EP_FUNC		0
+
+/* TLP configuration type 0 and type 1 */
+#define CRG0_READ_FMTTYPE                                                      \
+	FIELD_PREP(GENMASK(31, 24), PCIE_TLP_FMT_TYPE(PCI_TLP_FMT_3DW_NO_DATA, \
+						      PCI_TLP_TYPE_CFG0_RD))
+#define CRG0_WRITE_FMTTYPE                                                  \
+	FIELD_PREP(GENMASK(31, 24), PCIE_TLP_FMT_TYPE(PCI_TLP_FMT_3DW_DATA, \
+						      PCI_TLP_TYPE_CFG0_WR))
+#define CRG1_READ_FMTTYPE                                                      \
+	FIELD_PREP(GENMASK(31, 24), PCIE_TLP_FMT_TYPE(PCI_TLP_FMT_3DW_NO_DATA, \
+						      PCI_TLP_TYPE_CFG1_RD))
+#define CRG1_WRITE_FMTTYPE                                                  \
+	FIELD_PREP(GENMASK(31, 24), PCIE_TLP_FMT_TYPE(PCI_TLP_FMT_3DW_DATA, \
+						      PCI_TLP_TYPE_CFG1_WR))
+#define CRG_PAYLOAD_SIZE		0x01 /* 1 DWORD */
+#define TLP_HEADER_BYTE_EN(x, y)	((GENMASK((x) - 1, 0) << ((y) % 4)))
+#define TLP_GET_VALUE(x, y, z)		(((x) >> ((((z) % 4)) * 8)) & GENMASK((8 * (y)) - 1, 0))
+#define TLP_SET_VALUE(x, y, z)		((((x) & GENMASK((8 * (y)) - 1, 0)) << ((((z) % 4)) * 8)))
+#define AST2600_TX_DESC1_VALUE		0x00002000
+#define AST2700_TX_DESC1_VALUE		0x00401000
+
+/**
+ * struct aspeed_pcie_port - PCIe port information
+ * @list: port list
+ * @pcie: pointer to PCIe host info
+ * @clk: pointer to the port clock gate
+ * @phy: pointer to PHY control block
+ * @perst: pointer to port reset control
+ * @slot: port slot
+ */
+struct aspeed_pcie_port {
+	struct list_head list;
+	struct aspeed_pcie *pcie;
+	struct clk *clk;
+	struct regmap *pciephy;
+	struct reset_control *perst;
+	u32 slot;
+};
+
+/**
+ * struct aspeed_pcie - PCIe port information
+ * @host: pointer to pcie host bridge
+ * @dev: pointer to device structure
+ * @reg: PCIe Host register base address
+ * @ahbc: pointer to AHHC register map
+ * @cfg: pointer to Aspeed PCIe configuration register map
+ * @platform: platform specific information
+ * @ports: list of PCIe ports
+ * @domain: PCI domain number
+ * @tx_tag: current TX tag for the port
+ * @h2xrst: pointer to H2X reset control
+ * @irq_domain: IRQ domain for INTx interrupts
+ * @dev_domain: IRQ domain for device interrupts
+ * @msi_domain: IRQ domain for MSI interrupts
+ * @lock: mutex to protect MSI bitmap variable
+ * @msi_irq_in_use: bitmap to track used MSI host IRQs
+ */
+struct aspeed_pcie {
+	struct pci_host_bridge *host;
+	struct device *dev;
+	void __iomem *reg;
+	struct regmap *ahbc;
+	struct regmap *cfg;
+	const struct aspeed_pcie_rc_platform *platform;
+	struct list_head ports;
+
+	int domain;
+	u8 tx_tag;
+
+	struct reset_control *h2xrst;
+
+	struct irq_domain *irq_domain;
+	struct irq_domain *dev_domain;
+	struct irq_domain *msi_domain;
+	struct mutex lock;		/* Protect MSI bitmap variable */
+	DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_HOST_IRQS);
+};
+
+/**
+ * struct aspeed_pcie_rc_platform - Platform information
+ * @setup: initialization function
+ * @get_link: port link status function
+ * @port_init: port initialization function
+ * @reg_intx_en: INTx enable register offset
+ * @reg_intx_sts: INTx status register offset
+ * @reg_msi_en: MSI enable register offset
+ * @reg_msi_sts: MSI enable register offset
+ * @msi_address: HW fixed MSI address
+ */
+struct aspeed_pcie_rc_platform {
+	int (*setup)(struct platform_device *pdev);
+	bool (*get_link)(struct aspeed_pcie_port *port);
+	void (*port_init)(struct aspeed_pcie_port *port);
+	int reg_intx_en;
+	int reg_intx_sts;
+	int reg_msi_en;
+	int reg_msi_sts;
+	int msi_address;
+};
+
+static void aspeed_pcie_intx_irq_ack(struct irq_data *d)
+{
+	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(d);
+	int intx_en = pcie->platform->reg_intx_en;
+	u32 en;
+
+	en = readl(pcie->reg + intx_en);
+	en |= BIT(d->hwirq);
+	writel(en, pcie->reg + intx_en);
+}
+
+static void aspeed_pcie_intx_irq_mask(struct irq_data *d)
+{
+	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(d);
+	int intx_en = pcie->platform->reg_intx_en;
+	u32 en;
+
+	en = readl(pcie->reg + intx_en);
+	en |= BIT(d->hwirq);
+	writel(en, pcie->reg + intx_en);
+}
+
+static void aspeed_pcie_intx_irq_unmask(struct irq_data *d)
+{
+	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(d);
+	int intx_en = pcie->platform->reg_intx_en;
+	u32 en;
+
+	en = readl(pcie->reg + intx_en);
+	en |= BIT(d->hwirq);
+	writel(en, pcie->reg + intx_en);
+}
+
+static struct irq_chip aspeed_intx_irq_chip = {
+	.name = "IntX",
+	.irq_ack = aspeed_pcie_intx_irq_ack,
+	.irq_mask = aspeed_pcie_intx_irq_mask,
+	.irq_unmask = aspeed_pcie_intx_irq_unmask,
+};
+
+static int aspeed_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
+				irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(irq, &aspeed_intx_irq_chip, handle_level_irq);
+	irq_set_chip_data(irq, domain->host_data);
+	irq_set_status_flags(irq, IRQ_LEVEL);
+
+	return 0;
+}
+
+static const struct irq_domain_ops aspeed_intx_domain_ops = {
+	.map = aspeed_pcie_intx_map,
+};
+
+static irqreturn_t aspeed_pcie_intr_handler(int irq, void *dev_id)
+{
+	struct aspeed_pcie *pcie = dev_id;
+	const struct aspeed_pcie_rc_platform *platform = pcie->platform;
+	unsigned long status;
+	unsigned long intx;
+	u32 bit;
+	int i;
+
+	intx = readl(pcie->reg + platform->reg_intx_sts) & PCIE_INTX_STS;
+	for_each_set_bit(bit, &intx, PCI_NUM_INTX)
+		generic_handle_domain_irq(pcie->irq_domain, bit);
+
+	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+		for (i = 0; i < 2; i++) {
+			status = readl(pcie->reg + platform->reg_msi_sts + (i * 4));
+			writel(status, pcie->reg + platform->reg_msi_sts + (i * 4));
+
+			for_each_set_bit(bit, &status, 32) {
+				bit += (i * 32);
+				generic_handle_domain_irq(pcie->dev_domain, bit);
+			}
+		}
+	}
+
+	return IRQ_HANDLED;
+}
+
+static u32 aspeed_pcie_get_bdf_offset(struct pci_bus *bus, unsigned int devfn,
+				      int where)
+{
+	return ((bus->number) << 24) | (PCI_SLOT(devfn) << 19) |
+		(PCI_FUNC(devfn) << 16) | (where & ~3);
+}
+
+static bool aspeed_ast2600_get_link(struct aspeed_pcie_port *port)
+{
+	u32 link_sts;
+
+	regmap_read(port->pciephy, PEHR_LINK, &link_sts);
+
+	return !!(link_sts & PCIE_LINK_UP);
+}
+
+static int aspeed_ast2600_conf(struct pci_bus *bus, unsigned int devfn,
+			       int where, int size, u32 *val, u32 fmt_type,
+			       bool write)
+{
+	struct aspeed_pcie *pcie = bus->sysdata;
+	u32 bdf_offset, cfg_val, isr;
+	int ret;
+
+	bdf_offset = aspeed_pcie_get_bdf_offset(bus, devfn, where);
+
+	/* Driver may set unlock RX buffer before triggering next TX config */
+	writel(PCIE_UNLOCK_RX_BUFF | readl(pcie->reg + H2X_DEV_CTRL),
+	       pcie->reg + H2X_DEV_CTRL);
+
+	cfg_val = fmt_type | CRG_PAYLOAD_SIZE;
+	regmap_write(pcie->cfg, H2X_TX_DESC0, cfg_val);
+
+	cfg_val = AST2600_TX_DESC1_VALUE | FIELD_PREP(GENMASK(11, 8), pcie->tx_tag) |
+		  TLP_HEADER_BYTE_EN(size, where);
+	regmap_write(pcie->cfg, H2X_TX_DESC1, cfg_val);
+
+	regmap_write(pcie->cfg, H2X_TX_DESC2, bdf_offset);
+	regmap_write(pcie->cfg, H2X_TX_DESC3, 0);
+	if (write)
+		regmap_write(pcie->cfg, H2X_TX_DESC_DATA, TLP_SET_VALUE(*val, size, where));
+
+	regmap_write_bits(pcie->cfg, H2X_STS, PCIE_TRIGGER_TX, PCIE_TRIGGER_TX);
+
+	ret = regmap_read_poll_timeout(pcie->cfg, H2X_STS, cfg_val,
+				       (cfg_val & PCIE_TX_IDLE), 0, 50);
+	if (ret) {
+		dev_err(pcie->dev,
+			"%04x:%02x:%02x.%d CR tx timeout sts: 0x%08x\n",
+			pcie->domain, bus->number, PCI_SLOT(devfn),
+			PCI_FUNC(devfn), cfg_val);
+		ret = PCIBIOS_SET_FAILED;
+		PCI_SET_ERROR_RESPONSE(val);
+		goto out;
+	}
+
+	regmap_write_bits(pcie->cfg, H2X_INT_STS, PCIE_TX_IDLE_CLEAR,
+			  PCIE_TX_IDLE_CLEAR);
+
+	regmap_read(pcie->cfg, H2X_STS, &cfg_val);
+	switch (cfg_val & PCIE_STATUS_OF_TX) {
+	case PCIE_RC_L_TX_COMPLETE:
+	case PCIE_RC_H_TX_COMPLETE:
+		ret = readl_poll_timeout(pcie->reg + H2X_DEV_STS, isr,
+					 (isr & PCIE_RC_RX_DONE_ISR), 0, 50);
+		if (ret) {
+			dev_err(pcie->dev,
+				"%04x:%02x:%02x.%d CR rx timeout sts: 0x%08x\n",
+				pcie->domain, bus->number, PCI_SLOT(devfn),
+				PCI_FUNC(devfn), isr);
+			ret = PCIBIOS_SET_FAILED;
+			PCI_SET_ERROR_RESPONSE(val);
+			goto out;
+		}
+		if (!write) {
+			if (readl(pcie->reg + H2X_DEV_RX_DESC1) & BIT(13)) {
+				ret = PCIBIOS_SET_FAILED;
+				PCI_SET_ERROR_RESPONSE(val);
+				goto out;
+			} else {
+				*val = readl(pcie->reg + H2X_DEV_RX_DESC_DATA);
+			}
+		}
+		break;
+	case PCIE_STATUS_OF_TX:
+		ret = PCIBIOS_SET_FAILED;
+		PCI_SET_ERROR_RESPONSE(val);
+		goto out;
+	default:
+		regmap_read(pcie->cfg, H2X_DEV_RX_DESC_DATA, &cfg_val);
+		*val = cfg_val;
+		break;
+	}
+
+	writel(PCIE_UNLOCK_RX_BUFF | readl(pcie->reg + H2X_DEV_CTRL),
+	       pcie->reg + H2X_DEV_CTRL);
+
+	*val = TLP_GET_VALUE(*val, size, where);
+
+	ret = PCIBIOS_SUCCESSFUL;
+out:
+	writel(readl(pcie->reg + H2X_DEV_STS), pcie->reg + H2X_DEV_STS);
+	pcie->tx_tag = (pcie->tx_tag + 1) % 0x8;
+	return ret;
+}
+
+static int aspeed_ast2600_rd_conf(struct pci_bus *bus, unsigned int devfn,
+				  int where, int size, u32 *val)
+{
+	int slot = PCI_SLOT(devfn);
+
+	if (slot != 0 && slot != 8)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	return aspeed_ast2600_conf(bus, devfn, where, size, val, CRG0_READ_FMTTYPE, false);
+}
+
+static int aspeed_ast2600_child_rd_conf(struct pci_bus *bus, unsigned int devfn,
+					int where, int size, u32 *val)
+{
+	return aspeed_ast2600_conf(bus, devfn, where, size, val, CRG1_READ_FMTTYPE, false);
+}
+
+static int aspeed_ast2600_wr_conf(struct pci_bus *bus, unsigned int devfn,
+				  int where, int size, u32 val)
+{
+	int slot = PCI_SLOT(devfn);
+
+	if (slot != 0 && slot != 8)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	return aspeed_ast2600_conf(bus, devfn, where, size, &val, CRG0_WRITE_FMTTYPE, true);
+}
+
+static int aspeed_ast2600_child_wr_conf(struct pci_bus *bus, unsigned int devfn,
+					int where, int size, u32 val)
+{
+	return aspeed_ast2600_conf(bus, devfn, where, size, &val, CRG1_WRITE_FMTTYPE, true);
+}
+
+static bool aspeed_ast2700_get_link(struct aspeed_pcie_port *port)
+{
+	u32 reg;
+
+	/* AST2700 has Gen2 and Gen4 RCs.
+	 * Read register to distinguish between Gen2 or Gen4.
+	 * Then read the corresonding register that is from Aspeed
+	 * design to get whether it linked up or not.
+	 */
+	regmap_read(port->pciephy, PEHR_MISC_300, &reg);
+	if (reg & RC_GEN2) {
+		regmap_read(port->pciephy, PEHR_MISC_344, &reg);
+		return !!(reg & LINK_UP_GEN2);
+	}
+
+	regmap_read(port->pciephy, PEHR_MISC_358, &reg);
+	return !!(reg & LINK_UP_GEN4);
+}
+
+static int aspeed_ast2700_config(struct pci_bus *bus, unsigned int devfn,
+				 int where, int size, u32 *val, bool write)
+{
+	struct aspeed_pcie *pcie = bus->sysdata;
+	u32 cfg_val;
+
+	cfg_val = CFGI_BYTE_EN(TLP_HEADER_BYTE_EN(size, where)) | (where & ~3);
+	if (write)
+		cfg_val |= CFGI_WRITE;
+	writel(cfg_val, pcie->reg + H2X_CFGI_TLP);
+
+	writel(TLP_SET_VALUE(*val, size, where), pcie->reg + H2X_CFGI_WR_DATA);
+	writel(CFGI_TLP_FIRE, pcie->reg + H2X_CFGI_CTRL);
+	*val = readl(pcie->reg + H2X_CFGI_RET_DATA);
+	*val = TLP_GET_VALUE(*val, size, where);
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int aspeed_ast2700_child_config(struct pci_bus *bus, unsigned int devfn,
+				       int where, int size, u32 *val,
+				       bool write)
+{
+	struct aspeed_pcie *pcie = bus->sysdata;
+	u32 bdf_offset, status, cfg_val;
+	int ret;
+
+	bdf_offset = aspeed_pcie_get_bdf_offset(bus, devfn, where);
+
+	cfg_val = CRG_PAYLOAD_SIZE;
+	if (write)
+		cfg_val |= (bus->number == 1) ? CRG0_WRITE_FMTTYPE : CRG1_WRITE_FMTTYPE;
+	else
+		cfg_val |= (bus->number == 1) ? CRG0_READ_FMTTYPE : CRG1_READ_FMTTYPE;
+	writel(cfg_val, pcie->reg + H2X_CFGE_TLP_1ST);
+
+	cfg_val = AST2700_TX_DESC1_VALUE |
+		  FIELD_PREP(GENMASK(11, 8), pcie->tx_tag) |
+		  TLP_HEADER_BYTE_EN(size, where);
+	writel(cfg_val, pcie->reg + H2X_CFGE_TLP_NEXT);
+
+	writel(bdf_offset, pcie->reg + H2X_CFGE_TLP_NEXT);
+	if (write)
+		writel(TLP_SET_VALUE(*val, size, where), pcie->reg + H2X_CFGE_TLP_NEXT);
+	writel(CFGE_TX_IDLE | CFGE_RX_BUSY, pcie->reg + H2X_CFGE_INT_STS);
+	writel(CFGE_TLP_FIRE, pcie->reg + H2X_CFGE_CTRL);
+
+	ret = readl_poll_timeout(pcie->reg + H2X_CFGE_INT_STS, status,
+				 (status & CFGE_TX_IDLE), 0, 50);
+	if (ret) {
+		dev_err(pcie->dev,
+			"%04x:%02x:%02x.%d CR tx timeout sts: 0x%08x\n",
+			pcie->domain, bus->number, PCI_SLOT(devfn),
+			PCI_FUNC(devfn), status);
+		ret = PCIBIOS_SET_FAILED;
+		PCI_SET_ERROR_RESPONSE(val);
+		goto out;
+	}
+
+	ret = readl_poll_timeout(pcie->reg + H2X_CFGE_INT_STS, status,
+				 (status & CFGE_RX_BUSY), 0, 50);
+	if (ret) {
+		dev_err(pcie->dev,
+			"%04x:%02x:%02x.%d CR rx timeoutsts: 0x%08x\n",
+			pcie->domain, bus->number, PCI_SLOT(devfn),
+			PCI_FUNC(devfn), status);
+		ret = PCIBIOS_SET_FAILED;
+		PCI_SET_ERROR_RESPONSE(val);
+		goto out;
+	}
+	*val = readl(pcie->reg + H2X_CFGE_RET_DATA);
+	*val = TLP_GET_VALUE(*val, size, where);
+
+	ret = PCIBIOS_SUCCESSFUL;
+out:
+	writel(status, pcie->reg + H2X_CFGE_INT_STS);
+	pcie->tx_tag = (pcie->tx_tag + 1) % 0xF;
+	return ret;
+}
+
+static int aspeed_ast2700_rd_conf(struct pci_bus *bus, unsigned int devfn,
+				  int where, int size, u32 *val)
+{
+	if (devfn != 0)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	return aspeed_ast2700_config(bus, devfn, where, size, val, false);
+}
+
+static int aspeed_ast2700_child_rd_conf(struct pci_bus *bus, unsigned int devfn,
+					int where, int size, u32 *val)
+{
+	return aspeed_ast2700_child_config(bus, devfn, where, size, val, false);
+}
+
+static int aspeed_ast2700_wr_conf(struct pci_bus *bus, unsigned int devfn,
+				  int where, int size, u32 val)
+{
+	if (devfn != 0)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	return aspeed_ast2700_config(bus, devfn, where, size, &val, true);
+}
+
+static int aspeed_ast2700_child_wr_conf(struct pci_bus *bus, unsigned int devfn,
+					int where, int size, u32 val)
+{
+	return aspeed_ast2700_child_config(bus, devfn, where, size, &val, true);
+}
+
+static struct pci_ops aspeed_ast2600_pcie_ops = {
+	.read = aspeed_ast2600_rd_conf,
+	.write = aspeed_ast2600_wr_conf,
+};
+
+static struct pci_ops aspeed_ast2600_pcie_child_ops = {
+	.read = aspeed_ast2600_child_rd_conf,
+	.write = aspeed_ast2600_child_wr_conf,
+};
+
+static struct pci_ops aspeed_ast2700_pcie_ops = {
+	.read = aspeed_ast2700_rd_conf,
+	.write = aspeed_ast2700_wr_conf,
+};
+
+static struct pci_ops aspeed_ast2700_pcie_child_ops = {
+	.read = aspeed_ast2700_child_rd_conf,
+	.write = aspeed_ast2700_child_wr_conf,
+};
+
+#ifdef CONFIG_PCI_MSI
+static void aspeed_msi_compose_msi_msg(struct irq_data *data,
+				       struct msi_msg *msg)
+{
+	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(data);
+
+	msg->address_hi = 0;
+	msg->address_lo = pcie->platform->msi_address;
+	msg->data = data->hwirq;
+}
+
+static int aspeed_msi_set_affinity(struct irq_data *irq_data,
+				   const struct cpumask *mask, bool force)
+{
+	return -EINVAL;
+}
+
+static struct irq_chip aspeed_msi_bottom_irq_chip = {
+	.name = "ASPEED MSI",
+	.irq_compose_msi_msg = aspeed_msi_compose_msi_msg,
+	.irq_set_affinity = aspeed_msi_set_affinity,
+};
+
+static int aspeed_irq_msi_domain_alloc(struct irq_domain *domain,
+				       unsigned int virq, unsigned int nr_irqs,
+				       void *args)
+{
+	struct aspeed_pcie *pcie = domain->host_data;
+	int bit;
+	int i;
+
+	mutex_lock(&pcie->lock);
+
+	bit = bitmap_find_free_region(pcie->msi_irq_in_use, MAX_MSI_HOST_IRQS,
+				      get_count_order(nr_irqs));
+
+	mutex_unlock(&pcie->lock);
+
+	if (bit < 0)
+		return -ENOSPC;
+
+	for (i = 0; i < nr_irqs; i++) {
+		irq_domain_set_info(domain, virq + i, bit + i,
+				    &aspeed_msi_bottom_irq_chip,
+				    domain->host_data, handle_simple_irq, NULL,
+				    NULL);
+	}
+
+	return 0;
+}
+
+static void aspeed_irq_msi_domain_free(struct irq_domain *domain,
+				       unsigned int virq, unsigned int nr_irqs)
+{
+	struct irq_data *data = irq_domain_get_irq_data(domain, virq);
+	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(data);
+
+	mutex_lock(&pcie->lock);
+
+	bitmap_release_region(pcie->msi_irq_in_use, data->hwirq,
+			      get_count_order(nr_irqs));
+
+	mutex_unlock(&pcie->lock);
+}
+
+static const struct irq_domain_ops aspeed_msi_domain_ops = {
+	.alloc = aspeed_irq_msi_domain_alloc,
+	.free = aspeed_irq_msi_domain_free,
+};
+
+static struct irq_chip aspeed_msi_irq_chip = {
+	.name = "PCIe MSI",
+	.irq_enable = pci_msi_unmask_irq,
+	.irq_disable = pci_msi_mask_irq,
+	.irq_mask = pci_msi_mask_irq,
+	.irq_unmask = pci_msi_unmask_irq,
+};
+
+static struct msi_domain_info aspeed_msi_domain_info = {
+	.flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
+		  MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX),
+	.chip = &aspeed_msi_irq_chip,
+};
+
+static int aspeed_pcie_msi_init(struct aspeed_pcie *pcie)
+{
+	int ret = 0;
+
+	writel(~0, pcie->reg + pcie->platform->reg_msi_en);
+	writel(~0, pcie->reg + pcie->platform->reg_msi_en + 0x04);
+	writel(~0, pcie->reg + pcie->platform->reg_msi_sts);
+	writel(~0, pcie->reg + pcie->platform->reg_msi_sts + 0x04);
+
+	pcie->dev_domain =
+		irq_domain_add_linear(NULL, MAX_MSI_HOST_IRQS, &aspeed_msi_domain_ops, pcie);
+	if (!pcie->dev_domain)
+		return dev_err_probe(pcie->dev, -ENOMEM, "failed to create IRQ domain\n");
+
+	pcie->msi_domain = pci_msi_create_irq_domain(dev_fwnode(pcie->dev), &aspeed_msi_domain_info,
+						     pcie->dev_domain);
+	if (!pcie->msi_domain)
+		return dev_err_probe(pcie->dev, -ENOMEM, "failed to create MSI domain\n");
+
+	return ret;
+}
+
+static void aspeed_pcie_msi_free(struct aspeed_pcie *pcie)
+{
+	if (pcie->msi_domain) {
+		irq_domain_remove(pcie->msi_domain);
+		pcie->msi_domain = NULL;
+	}
+
+	if (pcie->dev_domain) {
+		irq_domain_remove(pcie->dev_domain);
+		pcie->dev_domain = NULL;
+	}
+}
+#else
+static int aspeed_pcie_msi_init(struct aspeed_pcie *pcie)
+{
+	return 0;
+}
+
+static void aspeed_pcie_msi_free(struct aspeed_pcie *pcie) {}
+#endif
+
+static void aspeed_pcie_irq_domain_free(struct aspeed_pcie *pcie)
+{
+	if (pcie->irq_domain) {
+		irq_domain_remove(pcie->irq_domain);
+		pcie->irq_domain = NULL;
+	}
+	aspeed_pcie_msi_free(pcie);
+}
+
+static int aspeed_pcie_init_irq_domain(struct aspeed_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	struct device_node *node = dev->of_node;
+	struct device_node *pcie_intc_node;
+	int ret;
+
+	pcie_intc_node = of_get_next_child(node, NULL);
+	if (!pcie_intc_node)
+		return dev_err_probe(dev, -ENODEV, "No PCIe Intc node found\n");
+
+	pcie->irq_domain =
+		irq_domain_add_linear(pcie_intc_node, PCI_NUM_INTX, &aspeed_intx_domain_ops, pcie);
+	of_node_put(pcie_intc_node);
+	if (!pcie->irq_domain) {
+		ret = dev_err_probe(dev, -ENOMEM, "failed to get an INTx IRQ domain\n");
+		goto err;
+	}
+
+	writel(0, pcie->reg + pcie->platform->reg_intx_en);
+	writel(~0, pcie->reg + pcie->platform->reg_intx_sts);
+
+	ret = aspeed_pcie_msi_init(pcie);
+	if (ret)
+		goto err;
+
+	return 0;
+err:
+	aspeed_pcie_irq_domain_free(pcie);
+	return ret;
+}
+
+static void aspeed_ast2600_port_init(struct aspeed_pcie_port *port)
+{
+	regmap_write(port->pciephy, PEHR_LOCK, PCIE_UNLOCK);
+	regmap_write(port->pciephy, PEHR_GLOBAL, AST2600_PORT_TYPE(0x3));
+}
+
+static void aspeed_ast2700_port_init(struct aspeed_pcie_port *port)
+{
+	u32 cfg_val;
+
+	regmap_write(port->pciephy, PEHR_MISC_70,
+		     POSTED_DATA_CREDITS(0xc0) | POSTED_HEADER_CREDITS(0xa));
+	regmap_write(port->pciephy, PEHR_MISC_78,
+		     COMPLETION_DATA_CREDITS(0x30) | COMPLETION_HEADER_CREDITS(0x8));
+	regmap_write(port->pciephy, PEHR_MISC_58, LOCAL_SCALE_SUP);
+
+	regmap_write(port->pciephy, PEHR_MISC_5C, CONFIG_RC_DEVICE);
+	regmap_read(port->pciephy, PEHR_MISC_60, &cfg_val);
+	cfg_val &= ~AST2700_PORT_TYPE_MASK;
+	cfg_val |= FIELD_PREP(AST2700_PORT_TYPE_MASK, PORT_TYPE_ROOT);
+	regmap_write(port->pciephy, PEHR_MISC_60, cfg_val);
+}
+
+static int aspeed_pcie_port_init(struct aspeed_pcie_port *port)
+{
+	struct aspeed_pcie *pcie = port->pcie;
+	struct device *dev = pcie->dev;
+	int ret;
+
+	ret = clk_prepare_enable(port->clk);
+	if (ret)
+		return dev_err_probe(dev, ret, "enabling clk pcie%d\n", port->slot);
+
+	pcie->platform->port_init(port);
+
+	reset_control_deassert(port->perst);
+	mdelay(PCIE_RESET_CONFIG_DEVICE_WAIT_MS);
+
+	if (pcie->platform->get_link(port))
+		dev_dbg(dev, "PCIe%d port %d link up\n", pcie->domain, port->slot);
+	else
+		dev_dbg(dev, "PCIe%d port %d link down\n", pcie->domain, port->slot);
+
+	return 0;
+}
+
+static int aspeed_pcie_init_ports(struct aspeed_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	struct aspeed_pcie_port *port, *tmp;
+	u8 num_enabled = 0;
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+		u32 slot = port->slot;
+		int ret;
+
+		ret = aspeed_pcie_port_init(port);
+		if (ret) {
+			dev_err(dev, "initializing port %d failed\n", slot);
+			list_del(&port->list);
+		} else {
+			num_enabled++;
+		}
+	}
+
+	return (num_enabled > 0) ? 0 : -ENODEV;
+}
+
+static void aspeed_host_reset(struct aspeed_pcie *pcie)
+{
+	reset_control_assert(pcie->h2xrst);
+	mdelay(PCIE_RESET_CONFIG_RC_WAIT_MS);
+	reset_control_deassert(pcie->h2xrst);
+}
+
+static int aspeed_ast2600_setup(struct platform_device *pdev)
+{
+	struct aspeed_pcie *pcie = platform_get_drvdata(pdev);
+	struct device *dev = pcie->dev;
+
+	pcie->ahbc = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,ahbc");
+	if (IS_ERR(pcie->ahbc))
+		return dev_err_probe(dev, PTR_ERR(pcie->ahbc), "failed to map ahbc base\n");
+
+	aspeed_host_reset(pcie);
+
+	regmap_write(pcie->ahbc, AHBC_KEY, AHBC_UNLOCK_KEY);
+	regmap_update_bits(pcie->ahbc, AHBC_ADDR_MAPPING, PCIE_RC_MEMORY_EN, PCIE_RC_MEMORY_EN);
+	regmap_write(pcie->ahbc, AHBC_KEY, AHBC_UNLOCK);
+
+	regmap_write(pcie->cfg, H2X_AHB_ADDR_CONFIG0,
+		     AHB_REMAP_LO_ADDR(0x600) | AHB_MASK_LO_ADDR(0xE00));
+	regmap_write(pcie->cfg, H2X_AHB_ADDR_CONFIG1, AHB_REMAP_HI_ADDR(0));
+	regmap_write(pcie->cfg, H2X_AHB_ADDR_CONFIG2, AHB_MASK_HI_ADDR(~0));
+
+	regmap_write(pcie->cfg, H2X_CTRL, H2X_BRIDGE_EN);
+
+	writel(PCIE_RX_DMA_EN | PCIE_RX_LINEAR | PCIE_RX_MSI_SEL | PCIE_RX_MSI_EN |
+	       PCIE_Wait_RX_TLP_CLR | PCIE_RC_RX_ENABLE | PCIE_RC_ENABLE,
+	       pcie->reg + H2X_DEV_CTRL);
+
+	writel(0x28, pcie->reg + H2X_DEV_TX_TAG);
+
+	pcie->host->ops = &aspeed_ast2600_pcie_ops;
+	pcie->host->child_ops = &aspeed_ast2600_pcie_child_ops;
+
+	return 0;
+}
+
+static int aspeed_ast2700_bar_assign(struct aspeed_pcie *pcie)
+{
+	struct resource_entry *win, *tmp;
+	struct pci_host_bridge *bridge = pcie->host;
+
+	resource_list_for_each_entry_safe(win, tmp, &bridge->windows) {
+		struct resource *res = win->res;
+
+		if (resource_type(res) == IORESOURCE_MEM &&
+		    !(res->flags & IORESOURCE_MEM_64)) {
+			writel(REMAP_BAR_BASE(res->start), pcie->reg + H2X_REMAP_DIRECT_ADDR);
+			return 0;
+		}
+	}
+
+	return -ENODEV;
+}
+
+static int aspeed_ast2700_setup(struct platform_device *pdev)
+{
+	struct aspeed_pcie *pcie = platform_get_drvdata(pdev);
+	int ret;
+
+	regmap_update_bits(pcie->cfg, SCU_60,
+			   RC_E2M_PATH_EN | RC_H2XS_PATH_EN | RC_H2XD_PATH_EN | RC_H2XX_PATH_EN |
+			   RC_UPSTREAM_MEM_EN,
+			   RC_E2M_PATH_EN | RC_H2XS_PATH_EN | RC_H2XD_PATH_EN | RC_H2XX_PATH_EN |
+			   RC_UPSTREAM_MEM_EN);
+	regmap_write(pcie->cfg, SCU_64,
+		     RC0_DECODE_DMA_BASE(0) | RC0_DECODE_DMA_LIMIT(0xFF) | RC1_DECODE_DMA_BASE(0) |
+		     RC1_DECODE_DMA_LIMIT(0xFF));
+	regmap_write(pcie->cfg, SCU_70, DISABLE_EP_FUNC);
+
+	aspeed_host_reset(pcie);
+
+	writel(0, pcie->reg + H2X_CTRL);
+	writel(H2X_BRIDGE_EN | H2X_BRIDGE_DIRECT_EN, pcie->reg + H2X_CTRL);
+
+	ret = aspeed_ast2700_bar_assign(pcie);
+	if (ret)
+		return dev_err_probe(pcie->dev, ret, "Failed to assign bar\n");
+
+	/* Prepare for 64-bit BAR pref */
+	writel(REMAP_PREF_ADDR_63_32(0x3), pcie->reg + H2X_REMAP_PREF_ADDR);
+
+	pcie->host->ops = &aspeed_ast2700_pcie_ops;
+	pcie->host->child_ops = &aspeed_ast2700_pcie_child_ops;
+
+	return 0;
+}
+
+static int aspeed_pcie_parse_port(struct aspeed_pcie *pcie,
+				  struct device_node *node,
+				  int slot)
+{
+	struct aspeed_pcie_port *port;
+	struct device *dev = pcie->dev;
+
+	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+
+	port->pciephy = syscon_regmap_lookup_by_phandle(node, "aspeed,pciephy");
+	if (IS_ERR(port->pciephy))
+		return dev_err_probe(dev, PTR_ERR(port->pciephy),
+				     "Failed to map pcie%d pciephy base\n", slot);
+
+	port->clk = devm_get_clk_from_child(dev, node, NULL);
+	if (IS_ERR(port->clk))
+		return dev_err_probe(dev, PTR_ERR(port->clk),
+				     "Failed to get pcie%d clock\n", slot);
+
+	port->perst = of_reset_control_get_exclusive(node, "perst");
+	if (IS_ERR(port->perst))
+		return dev_err_probe(dev, PTR_ERR(port->perst),
+				     "Failed to get pcie%d reset control\n", slot);
+	reset_control_assert(port->perst);
+
+	port->slot = slot;
+	port->pcie = pcie;
+
+	INIT_LIST_HEAD(&port->list);
+	list_add_tail(&port->list, &pcie->ports);
+
+	return 0;
+}
+
+static int aspeed_pcie_parse_dt(struct aspeed_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	struct device_node *node = dev->of_node;
+	int ret;
+
+	for_each_available_child_of_node_scoped(node, child) {
+		int slot;
+		const char *type;
+
+		ret = of_property_read_string(child, "device_type", &type);
+		if (ret || strcmp(type, "pci"))
+			continue;
+
+		ret = of_pci_get_devfn(child);
+		if (ret < 0)
+			return dev_err_probe(dev, ret, "failed to parse devfn\n");
+
+		slot = PCI_SLOT(ret);
+
+		ret = aspeed_pcie_parse_port(pcie, child, slot);
+		if (ret)
+			return ret;
+	}
+
+	if (list_empty(&pcie->ports))
+		return dev_err_probe(dev, -ENODEV, "No PCIe port found in DT\n");
+
+	return 0;
+}
+
+static int aspeed_pcie_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct pci_host_bridge *host;
+	struct aspeed_pcie *pcie;
+	struct aspeed_pcie_port *port;
+	struct device_node *node = dev->of_node;
+	const struct aspeed_pcie_rc_platform *md = of_device_get_match_data(dev);
+	int irq, ret;
+
+	if (!md)
+		return -ENODEV;
+
+	host = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
+	if (!host)
+		return -ENOMEM;
+
+	pcie = pci_host_bridge_priv(host);
+	pcie->dev = dev;
+	pcie->tx_tag = 0;
+	platform_set_drvdata(pdev, pcie);
+
+	pcie->platform = md;
+	pcie->host = host;
+	INIT_LIST_HEAD(&pcie->ports);
+
+	pcie->reg = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(pcie->reg))
+		return PTR_ERR(pcie->reg);
+
+	of_property_read_u32(node, "linux,pci-domain", &pcie->domain);
+
+	pcie->cfg = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,pciecfg");
+	if (IS_ERR(pcie->cfg))
+		return dev_err_probe(dev, PTR_ERR(pcie->cfg), "Failed to map pciecfg base\n");
+
+	pcie->h2xrst = devm_reset_control_get_exclusive(dev, "h2x");
+	if (IS_ERR(pcie->h2xrst))
+		return dev_err_probe(dev, PTR_ERR(pcie->h2xrst), "Failed to get h2x reset\n");
+
+	ret = devm_mutex_init(dev, &pcie->lock);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to init mutex\n");
+
+	ret = pcie->platform->setup(pdev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to setup PCIe RC\n");
+
+	ret = aspeed_pcie_parse_dt(pcie);
+	if (ret)
+		return ret;
+
+	ret = aspeed_pcie_init_ports(pcie);
+	if (ret)
+		goto err_remove_resets;
+
+	host->sysdata = pcie;
+
+	ret = aspeed_pcie_init_irq_domain(pcie);
+	if (ret)
+		goto err_irq_init;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		ret = irq;
+		goto err_irq;
+	}
+
+	ret = devm_request_irq(dev, irq, aspeed_pcie_intr_handler, IRQF_SHARED, dev_name(dev),
+			       pcie);
+	if (ret)
+		goto err_irq;
+
+	ret = pci_host_probe(host);
+	if (ret)
+		goto err_irq;
+
+	return 0;
+err_irq:
+	aspeed_pcie_irq_domain_free(pcie);
+err_irq_init:
+err_remove_resets:
+	list_for_each_entry(port, &pcie->ports, list)
+		reset_control_put(port->perst);
+	dev_err_probe(dev, ret, "Failed to initial RC\n");
+	return ret;
+}
+
+const struct aspeed_pcie_rc_platform pcie_rc_ast2600 = {
+	.setup = aspeed_ast2600_setup,
+	.get_link = aspeed_ast2600_get_link,
+	.port_init = aspeed_ast2600_port_init,
+	.reg_intx_en = 0x04,
+	.reg_intx_sts = 0x08,
+	.reg_msi_en = 0x20,
+	.reg_msi_sts = 0x28,
+	.msi_address = 0x1e77005c,
+};
+
+const struct aspeed_pcie_rc_platform pcie_rc_ast2700 = {
+	.setup = aspeed_ast2700_setup,
+	.get_link = aspeed_ast2700_get_link,
+	.port_init = aspeed_ast2700_port_init,
+	.reg_intx_en = 0x40,
+	.reg_intx_sts = 0x48,
+	.reg_msi_en = 0x50,
+	.reg_msi_sts = 0x58,
+	.msi_address = 0x000000F0,
+};
+
+static const struct of_device_id aspeed_pcie_of_match[] = {
+	{ .compatible = "aspeed,ast2600-pcie", .data = &pcie_rc_ast2600 },
+	{ .compatible = "aspeed,ast2700-pcie", .data = &pcie_rc_ast2700 },
+	{}
+};
+
+static struct platform_driver aspeed_pcie_driver = {
+	.driver = {
+		.name = "aspeed-pcie",
+		.of_match_table = aspeed_pcie_of_match,
+	},
+	.probe = aspeed_pcie_probe,
+};
+
+module_platform_driver(aspeed_pcie_driver);
+
+MODULE_AUTHOR("Jacky Chou <jacky_chou@aspeedtech.com>");
+MODULE_DESCRIPTION("ASPEED PCIe Root Complex");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH v2 09/10] PCI: aspeed: Add ASPEED PCIe RC driver

[Philipp Zabel]

On Di, 2025-07-15 at 11:43 +0800, Jacky Chou wrote:
> Introduce PCIe Root Complex driver for ASPEED SoCs. Support RC
> initialization, reset, clock, IRQ domain, and MSI domain setup.
> Implement platform-specific setup and register configuration for
> ASPEED. And provide PCI config space read/write and INTx/MSI
> interrupt handling.
> 
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> ---
>  drivers/pci/controller/Kconfig       |   13 +
>  drivers/pci/controller/Makefile      |    1 +
>  drivers/pci/controller/pcie-aspeed.c | 1137 ++++++++++++++++++++++++++
>  3 files changed, 1151 insertions(+)
>  create mode 100644 drivers/pci/controller/pcie-aspeed.c
> 
[...]
> diff --git a/drivers/pci/controller/pcie-aspeed.c b/drivers/pci/controller/pcie-aspeed.c
> new file mode 100644
> index 000000000000..a7e679d5fb42
> --- /dev/null
> +++ b/drivers/pci/controller/pcie-aspeed.c
> @@ -0,0 +1,1137 @@
[...]
> +static int aspeed_pcie_parse_port(struct aspeed_pcie *pcie,
> +				  struct device_node *node,
> +				  int slot)
> +{
> +	struct aspeed_pcie_port *port;
> +	struct device *dev = pcie->dev;
> +
> +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return -ENOMEM;
> +
> +	port->pciephy = syscon_regmap_lookup_by_phandle(node, "aspeed,pciephy");
> +	if (IS_ERR(port->pciephy))
> +		return dev_err_probe(dev, PTR_ERR(port->pciephy),
> +				     "Failed to map pcie%d pciephy base\n", slot);
> +
> +	port->clk = devm_get_clk_from_child(dev, node, NULL);
> +	if (IS_ERR(port->clk))
> +		return dev_err_probe(dev, PTR_ERR(port->clk),
> +				     "Failed to get pcie%d clock\n", slot);
> +
> +	port->perst = of_reset_control_get_exclusive(node, "perst");
> +	if (IS_ERR(port->perst))
> +		return dev_err_probe(dev, PTR_ERR(port->perst),
> +				     "Failed to get pcie%d reset control\n", slot);

How about registering a reset_control_put() via
devm_add_action_or_reset()?
Otherwise these reset controls are not released on .remove.

[...]
> +static int aspeed_pcie_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct pci_host_bridge *host;
> +	struct aspeed_pcie *pcie;
> +	struct aspeed_pcie_port *port;
> +	struct device_node *node = dev->of_node;
> +	const struct aspeed_pcie_rc_platform *md = of_device_get_match_data(dev);
> +	int irq, ret;
> +
> +	if (!md)
> +		return -ENODEV;
> +
> +	host = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
> +	if (!host)
> +		return -ENOMEM;
> +
> +	pcie = pci_host_bridge_priv(host);
> +	pcie->dev = dev;
> +	pcie->tx_tag = 0;
> +	platform_set_drvdata(pdev, pcie);
> +
> +	pcie->platform = md;
> +	pcie->host = host;
> +	INIT_LIST_HEAD(&pcie->ports);
> +
> +	pcie->reg = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(pcie->reg))
> +		return PTR_ERR(pcie->reg);
> +
> +	of_property_read_u32(node, "linux,pci-domain", &pcie->domain);
> +
> +	pcie->cfg = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,pciecfg");
> +	if (IS_ERR(pcie->cfg))
> +		return dev_err_probe(dev, PTR_ERR(pcie->cfg), "Failed to map pciecfg base\n");
> +
> +	pcie->h2xrst = devm_reset_control_get_exclusive(dev, "h2x");
> +	if (IS_ERR(pcie->h2xrst))
> +		return dev_err_probe(dev, PTR_ERR(pcie->h2xrst), "Failed to get h2x reset\n");
> +
> +	ret = devm_mutex_init(dev, &pcie->lock);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to init mutex\n");
> +
> +	ret = pcie->platform->setup(pdev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to setup PCIe RC\n");
> +
> +	ret = aspeed_pcie_parse_dt(pcie);
> +	if (ret)
> +		return ret;
> +
> +	ret = aspeed_pcie_init_ports(pcie);
> +	if (ret)
> +		goto err_remove_resets;
> +
> +	host->sysdata = pcie;
> +
> +	ret = aspeed_pcie_init_irq_domain(pcie);
> +	if (ret)
> +		goto err_irq_init;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		ret = irq;
> +		goto err_irq;
> +	}
> +
> +	ret = devm_request_irq(dev, irq, aspeed_pcie_intr_handler, IRQF_SHARED, dev_name(dev),
> +			       pcie);
> +	if (ret)
> +		goto err_irq;
> +
> +	ret = pci_host_probe(host);
> +	if (ret)
> +		goto err_irq;
> +
> +	return 0;
> +err_irq:
> +	aspeed_pcie_irq_domain_free(pcie);

If pci_host_probe() fails, aspeed_pcie_irq_domain_free() will be called
before the IRQ requested with devm_request_irq() above is released.
Also, this is never called on .remove. You can fix both with
devm_add_action_or_reset().

> +err_irq_init:
> +err_remove_resets:
> +	list_for_each_entry(port, &pcie->ports, list)
> +		reset_control_put(port->perst);

I suggest to let devres handle this (see above).

regards
Philipp

[PATCH v2 09/10] PCI: aspeed: Add ASPEED PCIe RC driver

[Jacky Chou]

Hi Philipp

Thank you for your reply.

> > Introduce PCIe Root Complex driver for ASPEED SoCs. Support RC
> > initialization, reset, clock, IRQ domain, and MSI domain setup.
> > Implement platform-specific setup and register configuration for
> > ASPEED. And provide PCI config space read/write and INTx/MSI interrupt
> > handling.
> >
> > Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> > ---
> >  drivers/pci/controller/Kconfig       |   13 +
> >  drivers/pci/controller/Makefile      |    1 +
> >  drivers/pci/controller/pcie-aspeed.c | 1137
> > ++++++++++++++++++++++++++
> >  3 files changed, 1151 insertions(+)
> >  create mode 100644 drivers/pci/controller/pcie-aspeed.c
> >
> [...]
> > diff --git a/drivers/pci/controller/pcie-aspeed.c
> > b/drivers/pci/controller/pcie-aspeed.c
> > new file mode 100644
> > index 000000000000..a7e679d5fb42
> > --- /dev/null
> > +++ b/drivers/pci/controller/pcie-aspeed.c
> > @@ -0,0 +1,1137 @@
> [...]
> > +static int aspeed_pcie_parse_port(struct aspeed_pcie *pcie,
> > +				  struct device_node *node,
> > +				  int slot)
> > +{
> > +	struct aspeed_pcie_port *port;
> > +	struct device *dev = pcie->dev;
> > +
> > +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> > +	if (!port)
> > +		return -ENOMEM;
> > +
> > +	port->pciephy = syscon_regmap_lookup_by_phandle(node,
> "aspeed,pciephy");
> > +	if (IS_ERR(port->pciephy))
> > +		return dev_err_probe(dev, PTR_ERR(port->pciephy),
> > +				     "Failed to map pcie%d pciephy base\n", slot);
> > +
> > +	port->clk = devm_get_clk_from_child(dev, node, NULL);
> > +	if (IS_ERR(port->clk))
> > +		return dev_err_probe(dev, PTR_ERR(port->clk),
> > +				     "Failed to get pcie%d clock\n", slot);
> > +
> > +	port->perst = of_reset_control_get_exclusive(node, "perst");
> > +	if (IS_ERR(port->perst))
> > +		return dev_err_probe(dev, PTR_ERR(port->perst),
> > +				     "Failed to get pcie%d reset control\n", slot);
> 
> How about registering a reset_control_put() via devm_add_action_or_reset()?
> Otherwise these reset controls are not released on .remove.
> 

Agreed. It is good method.
Thank you for your suggestion.

> [...]
> > +static int aspeed_pcie_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct pci_host_bridge *host;
> > +	struct aspeed_pcie *pcie;
> > +	struct aspeed_pcie_port *port;
> > +	struct device_node *node = dev->of_node;
> > +	const struct aspeed_pcie_rc_platform *md =
> of_device_get_match_data(dev);
> > +	int irq, ret;
> > +
> > +	if (!md)
> > +		return -ENODEV;
> > +
> > +	host = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
> > +	if (!host)
> > +		return -ENOMEM;
> > +
> > +	pcie = pci_host_bridge_priv(host);
> > +	pcie->dev = dev;
> > +	pcie->tx_tag = 0;
> > +	platform_set_drvdata(pdev, pcie);
> > +
> > +	pcie->platform = md;
> > +	pcie->host = host;
> > +	INIT_LIST_HEAD(&pcie->ports);
> > +
> > +	pcie->reg = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(pcie->reg))
> > +		return PTR_ERR(pcie->reg);
> > +
> > +	of_property_read_u32(node, "linux,pci-domain", &pcie->domain);
> > +
> > +	pcie->cfg = syscon_regmap_lookup_by_phandle(dev->of_node,
> "aspeed,pciecfg");
> > +	if (IS_ERR(pcie->cfg))
> > +		return dev_err_probe(dev, PTR_ERR(pcie->cfg), "Failed to map
> > +pciecfg base\n");
> > +
> > +	pcie->h2xrst = devm_reset_control_get_exclusive(dev, "h2x");
> > +	if (IS_ERR(pcie->h2xrst))
> > +		return dev_err_probe(dev, PTR_ERR(pcie->h2xrst), "Failed to get h2x
> > +reset\n");
> > +
> > +	ret = devm_mutex_init(dev, &pcie->lock);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to init mutex\n");
> > +
> > +	ret = pcie->platform->setup(pdev);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to setup PCIe RC\n");
> > +
> > +	ret = aspeed_pcie_parse_dt(pcie);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = aspeed_pcie_init_ports(pcie);
> > +	if (ret)
> > +		goto err_remove_resets;
> > +
> > +	host->sysdata = pcie;
> > +
> > +	ret = aspeed_pcie_init_irq_domain(pcie);
> > +	if (ret)
> > +		goto err_irq_init;
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0) {
> > +		ret = irq;
> > +		goto err_irq;
> > +	}
> > +
> > +	ret = devm_request_irq(dev, irq, aspeed_pcie_intr_handler, IRQF_SHARED,
> dev_name(dev),
> > +			       pcie);
> > +	if (ret)
> > +		goto err_irq;
> > +
> > +	ret = pci_host_probe(host);
> > +	if (ret)
> > +		goto err_irq;
> > +
> > +	return 0;
> > +err_irq:
> > +	aspeed_pcie_irq_domain_free(pcie);
> 
> If pci_host_probe() fails, aspeed_pcie_irq_domain_free() will be called before
> the IRQ requested with devm_request_irq() above is released.
> Also, this is never called on .remove. You can fix both with
> devm_add_action_or_reset().
> 
> > +err_irq_init:
> > +err_remove_resets:
> > +	list_for_each_entry(port, &pcie->ports, list)
> > +		reset_control_put(port->perst);
> 
> I suggest to let devres handle this (see above).
> 

I will apply these parts with your suggestion in this driver.

Thanks,
Jacky

Re: [PATCH v2 09/10] PCI: aspeed: Add ASPEED PCIe RC driver

[Bjorn Helgaas]

v1 posting was
https://lore.kernel.org/r/20250613033001.3153637-1-jacky_chou@aspeedtech.com
Links to previous postings are helpful in the cover letter.

On Tue, Jul 15, 2025 at 11:43:19AM +0800, Jacky Chou wrote:
> Introduce PCIe Root Complex driver for ASPEED SoCs. Support RC
> initialization, reset, clock, IRQ domain, and MSI domain setup.
> Implement platform-specific setup and register configuration for
> ASPEED. And provide PCI config space read/write and INTx/MSI
> interrupt handling.

> +config PCIE_ASPEED
> +	bool "ASPEED PCIe controller"
> +	depends on ARCH_ASPEED || COMPILE_TEST
> +	depends on OF
> +	select PCI_MSI_ARCH_FALLBACKS
> +	help
> +	  Enable this option to add support for the PCIe controller
> +	  found on ASPEED SoCs.
> +	  This driver provides initialization and management for PCIe
> +	  Root Complex functionality, including interrupt and MSI support.
> +	  Select Y if your platform uses an ASPEED SoC and requires PCIe
> +	  connectivity.

Add blank line between paragraphs or reflow into single paragraph
(mentioned before).

Alphabetize this entry by the menu item ("ASPEED PCIe controller",
"ARM Versatile PB PCI controller").

>  config PCI_VERSATILE
>  	bool "ARM Versatile PB PCI controller"
>  	depends on ARCH_VERSATILE || COMPILE_TEST

> +#define MAX_MSI_HOST_IRQS	64
> +#define PCIE_RESET_CONFIG_DEVICE_WAIT_MS	500

Where does this value come from?  Is there a generic value from
drivers/pci/pci.h you can use?

> +#define PCIE_RESET_CONFIG_RC_WAIT_MS		10

Ditto.  If it's an Aspeed-specific value, can you point to the source
in the Aspeed datasheet?

> +#define CRG0_READ_FMTTYPE                                                      \
> +	FIELD_PREP(GENMASK(31, 24), PCIE_TLP_FMT_TYPE(PCI_TLP_FMT_3DW_NO_DATA, \
> +						      PCI_TLP_TYPE_CFG0_RD))
> +#define CRG0_WRITE_FMTTYPE                                                  \
> +	FIELD_PREP(GENMASK(31, 24), PCIE_TLP_FMT_TYPE(PCI_TLP_FMT_3DW_DATA, \
> +						      PCI_TLP_TYPE_CFG0_WR))
> +#define CRG1_READ_FMTTYPE                                                      \
> +	FIELD_PREP(GENMASK(31, 24), PCIE_TLP_FMT_TYPE(PCI_TLP_FMT_3DW_NO_DATA, \
> +						      PCI_TLP_TYPE_CFG1_RD))
> +#define CRG1_WRITE_FMTTYPE                                                  \
> +	FIELD_PREP(GENMASK(31, 24), PCIE_TLP_FMT_TYPE(PCI_TLP_FMT_3DW_DATA, \
> +						      PCI_TLP_TYPE_CFG1_WR))

Looks like a #define for GENMASK(31, 24) might make sense?

> + * struct aspeed_pcie_port - PCIe port information
> + * @list: port list
> + * @pcie: pointer to PCIe host info
> + * @clk: pointer to the port clock gate
> + * @phy: pointer to PHY control block
> + * @perst: pointer to port reset control
> + * @slot: port slot
> + */
> +struct aspeed_pcie_port {
> +	struct list_head list;
> +	struct aspeed_pcie *pcie;
> +	struct clk *clk;
> +	struct regmap *pciephy;
> +	struct reset_control *perst;
> +	u32 slot;
> +};
> +
> +/**
> + * struct aspeed_pcie - PCIe port information

I think aspeed_pcie_port is for a Root Port; this looks like it might
be for the Root Complex as a whole.

> + * @host: pointer to pcie host bridge

s/pcie/PCIe/

> + * @dev: pointer to device structure
> + * @reg: PCIe Host register base address
> + * @ahbc: pointer to AHHC register map
> + * @cfg: pointer to Aspeed PCIe configuration register map
> + * @platform: platform specific information
> + * @ports: list of PCIe ports
> + * @domain: PCI domain number
> + * @tx_tag: current TX tag for the port
> + * @h2xrst: pointer to H2X reset control
> + * @irq_domain: IRQ domain for INTx interrupts
> + * @dev_domain: IRQ domain for device interrupts
> + * @msi_domain: IRQ domain for MSI interrupts
> + * @lock: mutex to protect MSI bitmap variable
> + * @msi_irq_in_use: bitmap to track used MSI host IRQs
> + */
> +struct aspeed_pcie {
> +	struct pci_host_bridge *host;
> +	struct device *dev;
> +	void __iomem *reg;
> +	struct regmap *ahbc;
> +	struct regmap *cfg;
> +	const struct aspeed_pcie_rc_platform *platform;
> +	struct list_head ports;
> +
> +	int domain;
> +	u8 tx_tag;
> +
> +	struct reset_control *h2xrst;
> +
> +	struct irq_domain *irq_domain;

Name "intx_domain" to remove ambiguity.

> +	struct irq_domain *dev_domain;
> +	struct irq_domain *msi_domain;
> +	struct mutex lock;		/* Protect MSI bitmap variable */
> +	DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_HOST_IRQS);
> +};

> +static void aspeed_pcie_intx_irq_ack(struct irq_data *d)
> +{
> +	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(d);
> +	int intx_en = pcie->platform->reg_intx_en;
> +	u32 en;
> +
> +	en = readl(pcie->reg + intx_en);
> +	en |= BIT(d->hwirq);
> +	writel(en, pcie->reg + intx_en);
> +}
> +
> +static void aspeed_pcie_intx_irq_mask(struct irq_data *d)
> +{
> +	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(d);
> +	int intx_en = pcie->platform->reg_intx_en;
> +	u32 en;
> +
> +	en = readl(pcie->reg + intx_en);
> +	en |= BIT(d->hwirq);
> +	writel(en, pcie->reg + intx_en);
> +}
> +
> +static void aspeed_pcie_intx_irq_unmask(struct irq_data *d)
> +{
> +	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(d);
> +	int intx_en = pcie->platform->reg_intx_en;
> +	u32 en;
> +
> +	en = readl(pcie->reg + intx_en);
> +	en |= BIT(d->hwirq);
> +	writel(en, pcie->reg + intx_en);
> +}

aspeed_pcie_intx_irq_ack(), aspeed_pcie_intx_irq_mask(), and
aspeed_pcie_intx_irq_unmask() all look identical.  Am I missing
something?

> +static struct irq_chip aspeed_intx_irq_chip = {
> +	.name = "IntX",

"INTx" (mentioned before).

> +static irqreturn_t aspeed_pcie_intr_handler(int irq, void *dev_id)
> +{
> +	struct aspeed_pcie *pcie = dev_id;
> +	const struct aspeed_pcie_rc_platform *platform = pcie->platform;
> +	unsigned long status;
> +	unsigned long intx;

Looks like status and intx should both be u32?

> +	u32 bit;
> +	int i;
> +
> +	intx = readl(pcie->reg + platform->reg_intx_sts) & PCIE_INTX_STS;

Use FIELD_GET() to avoid assumption that PCIE_INTX_STS starts at
bit 0.

> +	for_each_set_bit(bit, &intx, PCI_NUM_INTX)
> +		generic_handle_domain_irq(pcie->irq_domain, bit);
> +
> +	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> +		for (i = 0; i < 2; i++) {
> +			status = readl(pcie->reg + platform->reg_msi_sts + (i * 4));
> +			writel(status, pcie->reg + platform->reg_msi_sts + (i * 4));
> +
> +			for_each_set_bit(bit, &status, 32) {
> +				bit += (i * 32);
> +				generic_handle_domain_irq(pcie->dev_domain, bit);
> +			}
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}

> +static int aspeed_ast2600_rd_conf(struct pci_bus *bus, unsigned int devfn,
> +				  int where, int size, u32 *val)
> +{
> +	int slot = PCI_SLOT(devfn);
> +
> +	if (slot != 0 && slot != 8)
> +		return PCIBIOS_DEVICE_NOT_FOUND;

I previously asked for a hint about why certain device addresses
aren't valid.  I meant a comment with a hint so the same question
doesn't arise every time.

> +	return aspeed_ast2600_conf(bus, devfn, where, size, val, CRG0_READ_FMTTYPE, false);
> +}

> +static bool aspeed_ast2700_get_link(struct aspeed_pcie_port *port)
> +{
> +	u32 reg;
> +
> +	/* AST2700 has Gen2 and Gen4 RCs.
> +	 * Read register to distinguish between Gen2 or Gen4.
> +	 * Then read the corresonding register that is from Aspeed
> +	 * design to get whether it linked up or not.
> +	 */

Comment style is:

  /*
   * Text ...
   */

Add blank line between paragraphs or reflow to a single paragraph.

s/corresonding/corresponding/

"that is from Aspeed design" seems unnecessary.

> +static int aspeed_ast2700_child_config(struct pci_bus *bus, unsigned int devfn,
> +				       int where, int size, u32 *val,
> +				       bool write)
> +{
> +	struct aspeed_pcie *pcie = bus->sysdata;
> +	u32 bdf_offset, status, cfg_val;
> +	int ret;
> +
> +	bdf_offset = aspeed_pcie_get_bdf_offset(bus, devfn, where);
> +
> +	cfg_val = CRG_PAYLOAD_SIZE;
> +	if (write)
> +		cfg_val |= (bus->number == 1) ? CRG0_WRITE_FMTTYPE : CRG1_WRITE_FMTTYPE;
> +	else
> +		cfg_val |= (bus->number == 1) ? CRG0_READ_FMTTYPE : CRG1_READ_FMTTYPE;

I don't think you should assume that bus 0 is the root bus.  The root
bus number should come from the DT bus-range.

> +static int aspeed_ast2700_rd_conf(struct pci_bus *bus, unsigned int devfn,
> +				  int where, int size, u32 *val)
> +{
> +	if (devfn != 0)
> +		return PCIBIOS_DEVICE_NOT_FOUND;

Another good place for a hint about why this restriction exists.

> +static int aspeed_pcie_msi_init(struct aspeed_pcie *pcie)
> +{
> +	int ret = 0;
> +
> +	writel(~0, pcie->reg + pcie->platform->reg_msi_en);
> +	writel(~0, pcie->reg + pcie->platform->reg_msi_en + 0x04);
> +	writel(~0, pcie->reg + pcie->platform->reg_msi_sts);
> +	writel(~0, pcie->reg + pcie->platform->reg_msi_sts + 0x04);
> +
> +	pcie->dev_domain =
> +		irq_domain_add_linear(NULL, MAX_MSI_HOST_IRQS, &aspeed_msi_domain_ops, pcie);
> +	if (!pcie->dev_domain)
> +		return dev_err_probe(pcie->dev, -ENOMEM, "failed to create IRQ domain\n");
> +
> +	pcie->msi_domain = pci_msi_create_irq_domain(dev_fwnode(pcie->dev), &aspeed_msi_domain_info,
> +						     pcie->dev_domain);
> +	if (!pcie->msi_domain)
> +		return dev_err_probe(pcie->dev, -ENOMEM, "failed to create MSI domain\n");

Can you please rework this to follow what Nam Cao is doing for
existing drivers:
https://lore.kernel.org/r/cover.1750858083.git.namcao@linutronix.de

> +static int aspeed_ast2700_setup(struct platform_device *pdev)
> +{
> +	struct aspeed_pcie *pcie = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	regmap_update_bits(pcie->cfg, SCU_60,
> +			   RC_E2M_PATH_EN | RC_H2XS_PATH_EN | RC_H2XD_PATH_EN | RC_H2XX_PATH_EN |
> +			   RC_UPSTREAM_MEM_EN,
> +			   RC_E2M_PATH_EN | RC_H2XS_PATH_EN | RC_H2XD_PATH_EN | RC_H2XX_PATH_EN |
> +			   RC_UPSTREAM_MEM_EN);
> +	regmap_write(pcie->cfg, SCU_64,
> +		     RC0_DECODE_DMA_BASE(0) | RC0_DECODE_DMA_LIMIT(0xFF) | RC1_DECODE_DMA_BASE(0) |
> +		     RC1_DECODE_DMA_LIMIT(0xFF));
> +	regmap_write(pcie->cfg, SCU_70, DISABLE_EP_FUNC);
> +
> +	aspeed_host_reset(pcie);
> +
> +	writel(0, pcie->reg + H2X_CTRL);
> +	writel(H2X_BRIDGE_EN | H2X_BRIDGE_DIRECT_EN, pcie->reg + H2X_CTRL);
> +
> +	ret = aspeed_ast2700_bar_assign(pcie);
> +	if (ret)
> +		return dev_err_probe(pcie->dev, ret, "Failed to assign bar\n");

s/bar/BAR/ in the message since it's an acronym.

> +	/* Prepare for 64-bit BAR pref */
> +	writel(REMAP_PREF_ADDR_63_32(0x3), pcie->reg + H2X_REMAP_PREF_ADDR);
> +
> +	pcie->host->ops = &aspeed_ast2700_pcie_ops;
> +	pcie->host->child_ops = &aspeed_ast2700_pcie_child_ops;
> +
> +	return 0;
> +}

> +static int aspeed_pcie_parse_port(struct aspeed_pcie *pcie,
> +				  struct device_node *node,
> +				  int slot)
> +{
> +	struct aspeed_pcie_port *port;
> +	struct device *dev = pcie->dev;
> +
> +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return -ENOMEM;
> +
> +	port->pciephy = syscon_regmap_lookup_by_phandle(node, "aspeed,pciephy");
> +	if (IS_ERR(port->pciephy))
> +		return dev_err_probe(dev, PTR_ERR(port->pciephy),
> +				     "Failed to map pcie%d pciephy base\n", slot);
> +
> +	port->clk = devm_get_clk_from_child(dev, node, NULL);
> +	if (IS_ERR(port->clk))
> +		return dev_err_probe(dev, PTR_ERR(port->clk),
> +				     "Failed to get pcie%d clock\n", slot);
> +
> +	port->perst = of_reset_control_get_exclusive(node, "perst");
> +	if (IS_ERR(port->perst))
> +		return dev_err_probe(dev, PTR_ERR(port->perst),
> +				     "Failed to get pcie%d reset control\n", slot);

Driver messages are inconsistently capitalized.

> +	reset_control_assert(port->perst);
> +
> +	port->slot = slot;
> +	port->pcie = pcie;
> +
> +	INIT_LIST_HEAD(&port->list);
> +	list_add_tail(&port->list, &pcie->ports);
> +
> +	return 0;
> +}

> +static int aspeed_pcie_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct pci_host_bridge *host;
> +	struct aspeed_pcie *pcie;
> +	struct aspeed_pcie_port *port;
> +	struct device_node *node = dev->of_node;
> +	const struct aspeed_pcie_rc_platform *md = of_device_get_match_data(dev);
> +	int irq, ret;
> +
> +	if (!md)
> +		return -ENODEV;
> +
> +	host = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
> +	if (!host)
> +		return -ENOMEM;
> +
> +	pcie = pci_host_bridge_priv(host);
> +	pcie->dev = dev;
> +	pcie->tx_tag = 0;
> +	platform_set_drvdata(pdev, pcie);
> +
> +	pcie->platform = md;
> +	pcie->host = host;
> +	INIT_LIST_HEAD(&pcie->ports);
> +
> +	pcie->reg = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(pcie->reg))
> +		return PTR_ERR(pcie->reg);
> +
> +	of_property_read_u32(node, "linux,pci-domain", &pcie->domain);

Normally not needed in a controller driver.  See
of_get_pci_domain_nr() and related interfaces.

> +	pcie->cfg = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,pciecfg");
> +	if (IS_ERR(pcie->cfg))
> +		return dev_err_probe(dev, PTR_ERR(pcie->cfg), "Failed to map pciecfg base\n");
> +
> +	pcie->h2xrst = devm_reset_control_get_exclusive(dev, "h2x");
> +	if (IS_ERR(pcie->h2xrst))
> +		return dev_err_probe(dev, PTR_ERR(pcie->h2xrst), "Failed to get h2x reset\n");
> +
> +	ret = devm_mutex_init(dev, &pcie->lock);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to init mutex\n");
> +
> +	ret = pcie->platform->setup(pdev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to setup PCIe RC\n");
> +
> +	ret = aspeed_pcie_parse_dt(pcie);
> +	if (ret)
> +		return ret;
> +
> +	ret = aspeed_pcie_init_ports(pcie);
> +	if (ret)
> +		goto err_remove_resets;
> +
> +	host->sysdata = pcie;
> +
> +	ret = aspeed_pcie_init_irq_domain(pcie);
> +	if (ret)
> +		goto err_irq_init;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		ret = irq;
> +		goto err_irq;
> +	}
> +
> +	ret = devm_request_irq(dev, irq, aspeed_pcie_intr_handler, IRQF_SHARED, dev_name(dev),
> +			       pcie);

Most of this file is formatted to fit in 80 columns; would be nice to
make the few outliers also fit.

> +	if (ret)
> +		goto err_irq;
> +
> +	ret = pci_host_probe(host);
> +	if (ret)
> +		goto err_irq;
> +
> +	return 0;
> +err_irq:
> +	aspeed_pcie_irq_domain_free(pcie);
> +err_irq_init:
> +err_remove_resets:
> +	list_for_each_entry(port, &pcie->ports, list)
> +		reset_control_put(port->perst);
> +	dev_err_probe(dev, ret, "Failed to initial RC\n");
> +	return ret;
> +}
> +
> +const struct aspeed_pcie_rc_platform pcie_rc_ast2600 = {
> +	.setup = aspeed_ast2600_setup,
> +	.get_link = aspeed_ast2600_get_link,
> +	.port_init = aspeed_ast2600_port_init,
> +	.reg_intx_en = 0x04,
> +	.reg_intx_sts = 0x08,
> +	.reg_msi_en = 0x20,
> +	.reg_msi_sts = 0x28,
> +	.msi_address = 0x1e77005c,

Capitalize hex numbers consistently.  It appears upper-case is the
convention in this file.

> +};
> +
> +const struct aspeed_pcie_rc_platform pcie_rc_ast2700 = {
> +	.setup = aspeed_ast2700_setup,
> +	.get_link = aspeed_ast2700_get_link,
> +	.port_init = aspeed_ast2700_port_init,
> +	.reg_intx_en = 0x40,
> +	.reg_intx_sts = 0x48,
> +	.reg_msi_en = 0x50,
> +	.reg_msi_sts = 0x58,
> +	.msi_address = 0x000000F0,
> +};

Bjorn

回覆: [PATCH v2 09/10] PCI: aspeed: Add ASPEED PCIe RC driver

[Jacky Chou]

Hi Bjorn,

Thank you for your reply.

> v1 posting was
> https://lore.kernel.org/r/20250613033001.3153637-1-jacky_chou@aspeedtech
> .com
> Links to previous postings are helpful in the cover letter.
> 
> On Tue, Jul 15, 2025 at 11:43:19AM +0800, Jacky Chou wrote:
> > Introduce PCIe Root Complex driver for ASPEED SoCs. Support RC
> > initialization, reset, clock, IRQ domain, and MSI domain setup.
> > Implement platform-specific setup and register configuration for
> > ASPEED. And provide PCI config space read/write and INTx/MSI interrupt
> > handling.
> 
> > +config PCIE_ASPEED
> > +	bool "ASPEED PCIe controller"
> > +	depends on ARCH_ASPEED || COMPILE_TEST
> > +	depends on OF
> > +	select PCI_MSI_ARCH_FALLBACKS
> > +	help
> > +	  Enable this option to add support for the PCIe controller
> > +	  found on ASPEED SoCs.
> > +	  This driver provides initialization and management for PCIe
> > +	  Root Complex functionality, including interrupt and MSI support.
> > +	  Select Y if your platform uses an ASPEED SoC and requires PCIe
> > +	  connectivity.
> 
> Add blank line between paragraphs or reflow into single paragraph (mentioned
> before).
> 
> Alphabetize this entry by the menu item ("ASPEED PCIe controller", "ARM
> Versatile PB PCI controller").
> 

Sorry, I miss it.
I will modify it in next version.

> >  config PCI_VERSATILE
> >  	bool "ARM Versatile PB PCI controller"
> >  	depends on ARCH_VERSATILE || COMPILE_TEST
> 
> > +#define MAX_MSI_HOST_IRQS	64
> > +#define PCIE_RESET_CONFIG_DEVICE_WAIT_MS	500
> 
> Where does this value come from?  Is there a generic value from
> drivers/pci/pci.h you can use?
> 

We check the PCIe specification to find these contents.
"With a Downstream Port that supports Link speeds greater than 5.0 GT/s, software must wait a
minimum of 100 ms after Link training completes before sending a Configuration Request to the
device immediately below that Port."
So, we think delay 500ms to let kernel issue the first configuration command is enough
after deassert PERST.

> > +#define PCIE_RESET_CONFIG_RC_WAIT_MS		10
> 
> Ditto.  If it's an Aspeed-specific value, can you point to the source in the
> Aspeed datasheet?
> 

This delay is set to ensure that the RC internal settings are completely reset.
We do not put its usage in our datasheet.

> > +#define CRG0_READ_FMTTYPE
> \
> > +	FIELD_PREP(GENMASK(31, 24),
> PCIE_TLP_FMT_TYPE(PCI_TLP_FMT_3DW_NO_DATA, \
> > +						      PCI_TLP_TYPE_CFG0_RD))
> > +#define CRG0_WRITE_FMTTYPE
> \
> > +	FIELD_PREP(GENMASK(31, 24),
> PCIE_TLP_FMT_TYPE(PCI_TLP_FMT_3DW_DATA, \
> > +						      PCI_TLP_TYPE_CFG0_WR))
> > +#define CRG1_READ_FMTTYPE
> \
> > +	FIELD_PREP(GENMASK(31, 24),
> PCIE_TLP_FMT_TYPE(PCI_TLP_FMT_3DW_NO_DATA, \
> > +						      PCI_TLP_TYPE_CFG1_RD))
> > +#define CRG1_WRITE_FMTTYPE
> \
> > +	FIELD_PREP(GENMASK(31, 24),
> PCIE_TLP_FMT_TYPE(PCI_TLP_FMT_3DW_DATA, \
> > +						      PCI_TLP_TYPE_CFG1_WR))
> 
> Looks like a #define for GENMASK(31, 24) might make sense?
> 
> > + * struct aspeed_pcie_port - PCIe port information
> > + * @list: port list
> > + * @pcie: pointer to PCIe host info
> > + * @clk: pointer to the port clock gate
> > + * @phy: pointer to PHY control block
> > + * @perst: pointer to port reset control
> > + * @slot: port slot
> > + */
> > +struct aspeed_pcie_port {
> > +	struct list_head list;
> > +	struct aspeed_pcie *pcie;
> > +	struct clk *clk;
> > +	struct regmap *pciephy;
> > +	struct reset_control *perst;
> > +	u32 slot;
> > +};
> > +
> > +/**
> > + * struct aspeed_pcie - PCIe port information
> 
> I think aspeed_pcie_port is for a Root Port; this looks like it might be for the
> Root Complex as a whole.
> 

Yes, struct aspeed_pcie is for Root Complex.
I will correct it in next version.

> > + * @host: pointer to pcie host bridge
> 
> s/pcie/PCIe/
> 
> > + * @dev: pointer to device structure
> > + * @reg: PCIe Host register base address
> > + * @ahbc: pointer to AHHC register map
> > + * @cfg: pointer to Aspeed PCIe configuration register map
> > + * @platform: platform specific information
> > + * @ports: list of PCIe ports
> > + * @domain: PCI domain number
> > + * @tx_tag: current TX tag for the port
> > + * @h2xrst: pointer to H2X reset control
> > + * @irq_domain: IRQ domain for INTx interrupts
> > + * @dev_domain: IRQ domain for device interrupts
> > + * @msi_domain: IRQ domain for MSI interrupts
> > + * @lock: mutex to protect MSI bitmap variable
> > + * @msi_irq_in_use: bitmap to track used MSI host IRQs  */ struct
> > +aspeed_pcie {
> > +	struct pci_host_bridge *host;
> > +	struct device *dev;
> > +	void __iomem *reg;
> > +	struct regmap *ahbc;
> > +	struct regmap *cfg;
> > +	const struct aspeed_pcie_rc_platform *platform;
> > +	struct list_head ports;
> > +
> > +	int domain;
> > +	u8 tx_tag;
> > +
> > +	struct reset_control *h2xrst;
> > +
> > +	struct irq_domain *irq_domain;
> 
> Name "intx_domain" to remove ambiguity.
> 

Agreed

> > +	struct irq_domain *dev_domain;
> > +	struct irq_domain *msi_domain;
> > +	struct mutex lock;		/* Protect MSI bitmap variable */
> > +	DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_HOST_IRQS); };
> 
> > +static void aspeed_pcie_intx_irq_ack(struct irq_data *d) {
> > +	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(d);
> > +	int intx_en = pcie->platform->reg_intx_en;
> > +	u32 en;
> > +
> > +	en = readl(pcie->reg + intx_en);
> > +	en |= BIT(d->hwirq);
> > +	writel(en, pcie->reg + intx_en);
> > +}
> > +
> > +static void aspeed_pcie_intx_irq_mask(struct irq_data *d) {
> > +	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(d);
> > +	int intx_en = pcie->platform->reg_intx_en;
> > +	u32 en;
> > +
> > +	en = readl(pcie->reg + intx_en);
> > +	en |= BIT(d->hwirq);
> > +	writel(en, pcie->reg + intx_en);
> > +}
> > +
> > +static void aspeed_pcie_intx_irq_unmask(struct irq_data *d) {
> > +	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(d);
> > +	int intx_en = pcie->platform->reg_intx_en;
> > +	u32 en;
> > +
> > +	en = readl(pcie->reg + intx_en);
> > +	en |= BIT(d->hwirq);
> > +	writel(en, pcie->reg + intx_en);
> > +}
> 
> aspeed_pcie_intx_irq_ack(), aspeed_pcie_intx_irq_mask(), and
> aspeed_pcie_intx_irq_unmask() all look identical.  Am I missing something?
> 

No.
Here is something wrong in aspeed_pcie_intx_irq_mask function.
I will fix it in next version.

> > +static struct irq_chip aspeed_intx_irq_chip = {
> > +	.name = "IntX",
> 
> "INTx" (mentioned before).
> 
> > +static irqreturn_t aspeed_pcie_intr_handler(int irq, void *dev_id) {
> > +	struct aspeed_pcie *pcie = dev_id;
> > +	const struct aspeed_pcie_rc_platform *platform = pcie->platform;
> > +	unsigned long status;
> > +	unsigned long intx;
> 
> Looks like status and intx should both be u32?
> 

Agreed.
It is 32-bit register, so I will change to u32.

> > +	u32 bit;
> > +	int i;
> > +
> > +	intx = readl(pcie->reg + platform->reg_intx_sts) & PCIE_INTX_STS;
> 
> Use FIELD_GET() to avoid assumption that PCIE_INTX_STS starts at bit 0.
> 

Agreed.

> > +	for_each_set_bit(bit, &intx, PCI_NUM_INTX)
> > +		generic_handle_domain_irq(pcie->irq_domain, bit);
> > +
> > +	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > +		for (i = 0; i < 2; i++) {
> > +			status = readl(pcie->reg + platform->reg_msi_sts + (i * 4));
> > +			writel(status, pcie->reg + platform->reg_msi_sts + (i * 4));
> > +
> > +			for_each_set_bit(bit, &status, 32) {
> > +				bit += (i * 32);
> > +				generic_handle_domain_irq(pcie->dev_domain, bit);
> > +			}
> > +		}
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> 
> > +static int aspeed_ast2600_rd_conf(struct pci_bus *bus, unsigned int devfn,
> > +				  int where, int size, u32 *val)
> > +{
> > +	int slot = PCI_SLOT(devfn);
> > +
> > +	if (slot != 0 && slot != 8)
> > +		return PCIBIOS_DEVICE_NOT_FOUND;
> 
> I previously asked for a hint about why certain device addresses aren't valid.  I
> meant a comment with a hint so the same question doesn't arise every time.
> 

I will add some comments in the same places that need to explain.

> > +	return aspeed_ast2600_conf(bus, devfn, where, size, val,
> > +CRG0_READ_FMTTYPE, false); }
> 
> > +static bool aspeed_ast2700_get_link(struct aspeed_pcie_port *port) {
> > +	u32 reg;
> > +
> > +	/* AST2700 has Gen2 and Gen4 RCs.
> > +	 * Read register to distinguish between Gen2 or Gen4.
> > +	 * Then read the corresonding register that is from Aspeed
> > +	 * design to get whether it linked up or not.
> > +	 */
> 
> Comment style is:
> 
>   /*
>    * Text ...
>    */
> 
> Add blank line between paragraphs or reflow to a single paragraph.
> 
> s/corresonding/corresponding/
> 
> "that is from Aspeed design" seems unnecessary.
> 

Agreed.

> > +static int aspeed_ast2700_child_config(struct pci_bus *bus, unsigned int
> devfn,
> > +				       int where, int size, u32 *val,
> > +				       bool write)
> > +{
> > +	struct aspeed_pcie *pcie = bus->sysdata;
> > +	u32 bdf_offset, status, cfg_val;
> > +	int ret;
> > +
> > +	bdf_offset = aspeed_pcie_get_bdf_offset(bus, devfn, where);
> > +
> > +	cfg_val = CRG_PAYLOAD_SIZE;
> > +	if (write)
> > +		cfg_val |= (bus->number == 1) ? CRG0_WRITE_FMTTYPE :
> CRG1_WRITE_FMTTYPE;
> > +	else
> > +		cfg_val |= (bus->number == 1) ? CRG0_READ_FMTTYPE :
> > +CRG1_READ_FMTTYPE;
> 
> I don't think you should assume that bus 0 is the root bus.  The root bus
> number should come from the DT bus-range.
> 
> > +static int aspeed_ast2700_rd_conf(struct pci_bus *bus, unsigned int devfn,
> > +				  int where, int size, u32 *val)
> > +{
> > +	if (devfn != 0)
> > +		return PCIBIOS_DEVICE_NOT_FOUND;
> 
> Another good place for a hint about why this restriction exists.
> 
> > +static int aspeed_pcie_msi_init(struct aspeed_pcie *pcie) {
> > +	int ret = 0;
> > +
> > +	writel(~0, pcie->reg + pcie->platform->reg_msi_en);
> > +	writel(~0, pcie->reg + pcie->platform->reg_msi_en + 0x04);
> > +	writel(~0, pcie->reg + pcie->platform->reg_msi_sts);
> > +	writel(~0, pcie->reg + pcie->platform->reg_msi_sts + 0x04);
> > +
> > +	pcie->dev_domain =
> > +		irq_domain_add_linear(NULL, MAX_MSI_HOST_IRQS,
> &aspeed_msi_domain_ops, pcie);
> > +	if (!pcie->dev_domain)
> > +		return dev_err_probe(pcie->dev, -ENOMEM, "failed to create IRQ
> > +domain\n");
> > +
> > +	pcie->msi_domain = pci_msi_create_irq_domain(dev_fwnode(pcie->dev),
> &aspeed_msi_domain_info,
> > +						     pcie->dev_domain);
> > +	if (!pcie->msi_domain)
> > +		return dev_err_probe(pcie->dev, -ENOMEM, "failed to create MSI
> > +domain\n");
> 
> Can you please rework this to follow what Nam Cao is doing for existing
> drivers:
> https://lore.kernel.org/r/cover.1750858083.git.namcao@linutronix.de
> 

Agreed.
I will refactor this part in next version.

> > +static int aspeed_ast2700_setup(struct platform_device *pdev) {
> > +	struct aspeed_pcie *pcie = platform_get_drvdata(pdev);
> > +	int ret;
> > +
> > +	regmap_update_bits(pcie->cfg, SCU_60,
> > +			   RC_E2M_PATH_EN | RC_H2XS_PATH_EN |
> RC_H2XD_PATH_EN | RC_H2XX_PATH_EN |
> > +			   RC_UPSTREAM_MEM_EN,
> > +			   RC_E2M_PATH_EN | RC_H2XS_PATH_EN |
> RC_H2XD_PATH_EN | RC_H2XX_PATH_EN |
> > +			   RC_UPSTREAM_MEM_EN);
> > +	regmap_write(pcie->cfg, SCU_64,
> > +		     RC0_DECODE_DMA_BASE(0) |
> RC0_DECODE_DMA_LIMIT(0xFF) | RC1_DECODE_DMA_BASE(0) |
> > +		     RC1_DECODE_DMA_LIMIT(0xFF));
> > +	regmap_write(pcie->cfg, SCU_70, DISABLE_EP_FUNC);
> > +
> > +	aspeed_host_reset(pcie);
> > +
> > +	writel(0, pcie->reg + H2X_CTRL);
> > +	writel(H2X_BRIDGE_EN | H2X_BRIDGE_DIRECT_EN, pcie->reg +
> H2X_CTRL);
> > +
> > +	ret = aspeed_ast2700_bar_assign(pcie);
> > +	if (ret)
> > +		return dev_err_probe(pcie->dev, ret, "Failed to assign bar\n");
> 
> s/bar/BAR/ in the message since it's an acronym.
> 
> > +	/* Prepare for 64-bit BAR pref */
> > +	writel(REMAP_PREF_ADDR_63_32(0x3), pcie->reg +
> H2X_REMAP_PREF_ADDR);
> > +
> > +	pcie->host->ops = &aspeed_ast2700_pcie_ops;
> > +	pcie->host->child_ops = &aspeed_ast2700_pcie_child_ops;
> > +
> > +	return 0;
> > +}
> 
> > +static int aspeed_pcie_parse_port(struct aspeed_pcie *pcie,
> > +				  struct device_node *node,
> > +				  int slot)
> > +{
> > +	struct aspeed_pcie_port *port;
> > +	struct device *dev = pcie->dev;
> > +
> > +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> > +	if (!port)
> > +		return -ENOMEM;
> > +
> > +	port->pciephy = syscon_regmap_lookup_by_phandle(node,
> "aspeed,pciephy");
> > +	if (IS_ERR(port->pciephy))
> > +		return dev_err_probe(dev, PTR_ERR(port->pciephy),
> > +				     "Failed to map pcie%d pciephy base\n", slot);
> > +
> > +	port->clk = devm_get_clk_from_child(dev, node, NULL);
> > +	if (IS_ERR(port->clk))
> > +		return dev_err_probe(dev, PTR_ERR(port->clk),
> > +				     "Failed to get pcie%d clock\n", slot);
> > +
> > +	port->perst = of_reset_control_get_exclusive(node, "perst");
> > +	if (IS_ERR(port->perst))
> > +		return dev_err_probe(dev, PTR_ERR(port->perst),
> > +				     "Failed to get pcie%d reset control\n", slot);
> 
> Driver messages are inconsistently capitalized.
> 

I will confirm all codes.

> > +	reset_control_assert(port->perst);
> > +
> > +	port->slot = slot;
> > +	port->pcie = pcie;
> > +
> > +	INIT_LIST_HEAD(&port->list);
> > +	list_add_tail(&port->list, &pcie->ports);
> > +
> > +	return 0;
> > +}
> 
> > +static int aspeed_pcie_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct pci_host_bridge *host;
> > +	struct aspeed_pcie *pcie;
> > +	struct aspeed_pcie_port *port;
> > +	struct device_node *node = dev->of_node;
> > +	const struct aspeed_pcie_rc_platform *md =
> of_device_get_match_data(dev);
> > +	int irq, ret;
> > +
> > +	if (!md)
> > +		return -ENODEV;
> > +
> > +	host = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
> > +	if (!host)
> > +		return -ENOMEM;
> > +
> > +	pcie = pci_host_bridge_priv(host);
> > +	pcie->dev = dev;
> > +	pcie->tx_tag = 0;
> > +	platform_set_drvdata(pdev, pcie);
> > +
> > +	pcie->platform = md;
> > +	pcie->host = host;
> > +	INIT_LIST_HEAD(&pcie->ports);
> > +
> > +	pcie->reg = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(pcie->reg))
> > +		return PTR_ERR(pcie->reg);
> > +
> > +	of_property_read_u32(node, "linux,pci-domain", &pcie->domain);
> 
> Normally not needed in a controller driver.  See
> of_get_pci_domain_nr() and related interfaces.
> 

Agreed.

> > +	pcie->cfg = syscon_regmap_lookup_by_phandle(dev->of_node,
> "aspeed,pciecfg");
> > +	if (IS_ERR(pcie->cfg))
> > +		return dev_err_probe(dev, PTR_ERR(pcie->cfg), "Failed to map
> > +pciecfg base\n");
> > +
> > +	pcie->h2xrst = devm_reset_control_get_exclusive(dev, "h2x");
> > +	if (IS_ERR(pcie->h2xrst))
> > +		return dev_err_probe(dev, PTR_ERR(pcie->h2xrst), "Failed to get h2x
> > +reset\n");
> > +
> > +	ret = devm_mutex_init(dev, &pcie->lock);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to init mutex\n");
> > +
> > +	ret = pcie->platform->setup(pdev);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to setup PCIe RC\n");
> > +
> > +	ret = aspeed_pcie_parse_dt(pcie);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = aspeed_pcie_init_ports(pcie);
> > +	if (ret)
> > +		goto err_remove_resets;
> > +
> > +	host->sysdata = pcie;
> > +
> > +	ret = aspeed_pcie_init_irq_domain(pcie);
> > +	if (ret)
> > +		goto err_irq_init;
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0) {
> > +		ret = irq;
> > +		goto err_irq;
> > +	}
> > +
> > +	ret = devm_request_irq(dev, irq, aspeed_pcie_intr_handler, IRQF_SHARED,
> dev_name(dev),
> > +			       pcie);
> 
> Most of this file is formatted to fit in 80 columns; would be nice to make the
> few outliers also fit.
> 

I will adjust this driver to be consistent in style.

> > +	if (ret)
> > +		goto err_irq;
> > +
> > +	ret = pci_host_probe(host);
> > +	if (ret)
> > +		goto err_irq;
> > +
> > +	return 0;
> > +err_irq:
> > +	aspeed_pcie_irq_domain_free(pcie);
> > +err_irq_init:
> > +err_remove_resets:
> > +	list_for_each_entry(port, &pcie->ports, list)
> > +		reset_control_put(port->perst);
> > +	dev_err_probe(dev, ret, "Failed to initial RC\n");
> > +	return ret;
> > +}
> > +
> > +const struct aspeed_pcie_rc_platform pcie_rc_ast2600 = {
> > +	.setup = aspeed_ast2600_setup,
> > +	.get_link = aspeed_ast2600_get_link,
> > +	.port_init = aspeed_ast2600_port_init,
> > +	.reg_intx_en = 0x04,
> > +	.reg_intx_sts = 0x08,
> > +	.reg_msi_en = 0x20,
> > +	.reg_msi_sts = 0x28,
> > +	.msi_address = 0x1e77005c,
> 
> Capitalize hex numbers consistently.  It appears upper-case is the convention
> in this file.
> 

Agreed.
I will adjust this driver to be consistent in style.

> > +};
> > +
> > +const struct aspeed_pcie_rc_platform pcie_rc_ast2700 = {
> > +	.setup = aspeed_ast2700_setup,
> > +	.get_link = aspeed_ast2700_get_link,
> > +	.port_init = aspeed_ast2700_port_init,
> > +	.reg_intx_en = 0x40,
> > +	.reg_intx_sts = 0x48,
> > +	.reg_msi_en = 0x50,
> > +	.reg_msi_sts = 0x58,
> > +	.msi_address = 0x000000F0,
> > +}

Thanks,
Jacky

Re: 回覆: [PATCH v2 09/10] PCI: aspeed: Add ASPEED PCIe RC driver

[Bjorn Helgaas]

On Fri, Aug 22, 2025 at 07:00:25AM +0000, Jacky Chou wrote:
> > v1 posting was
> > https://lore.kernel.org/r/20250613033001.3153637-1-jacky_chou@aspeedtech
> > .com
> > Links to previous postings are helpful in the cover letter.
> > 
> > On Tue, Jul 15, 2025 at 11:43:19AM +0800, Jacky Chou wrote:
> > > Introduce PCIe Root Complex driver for ASPEED SoCs. Support RC
> > > initialization, reset, clock, IRQ domain, and MSI domain setup.
> > > Implement platform-specific setup and register configuration for
> > > ASPEED. And provide PCI config space read/write and INTx/MSI interrupt
> > > handling.

> > > +#define MAX_MSI_HOST_IRQS	64
> > > +#define PCIE_RESET_CONFIG_DEVICE_WAIT_MS	500
> > 
> > Where does this value come from?  Is there a generic value from
> > drivers/pci/pci.h you can use?
> 
> We check the PCIe specification to find these contents.
>
> "With a Downstream Port that supports Link speeds greater than 5.0
> GT/s, software must wait a minimum of 100 ms after Link training
> completes before sending a Configuration Request to the device
> immediately below that Port."
>
> So, we think delay 500ms to let kernel issue the first configuration
> command is enough after deassert PERST.

Isn't this PCIE_RESET_CONFIG_WAIT_MS?

I prefer to use #defines from the PCI core whenever possible because
it makes it easier to ensure that all drivers include the required
delays.

> > > +#define PCIE_RESET_CONFIG_RC_WAIT_MS		10
> > 
> > Ditto.  If it's an Aspeed-specific value, can you point to the
> > source in the Aspeed datasheet?
> 
> This delay is set to ensure that the RC internal settings are
> completely reset.  We do not put its usage in our datasheet.

The "PCIE_" prefix suggests something required by the PCIe base spec.
If this is an Aspeed-specific value, perhaps remove the "PCIE_"
prefix?

> > > +static int aspeed_ast2700_child_config(struct pci_bus *bus, unsigned int
> > devfn,
> > > +				       int where, int size, u32 *val,
> > > +				       bool write)
> > > +{
> > > +	struct aspeed_pcie *pcie = bus->sysdata;
> > > +	u32 bdf_offset, status, cfg_val;
> > > +	int ret;
> > > +
> > > +	bdf_offset = aspeed_pcie_get_bdf_offset(bus, devfn, where);
> > > +
> > > +	cfg_val = CRG_PAYLOAD_SIZE;
> > > +	if (write)
> > > +		cfg_val |= (bus->number == 1) ? CRG0_WRITE_FMTTYPE :
> > CRG1_WRITE_FMTTYPE;
> > > +	else
> > > +		cfg_val |= (bus->number == 1) ? CRG0_READ_FMTTYPE :
> > > +CRG1_READ_FMTTYPE;
> > 
> > I don't think you should assume that bus 0 is the root bus.  The root bus
> > number should come from the DT bus-range.

Just making sure you saw this part since you didn't mention it.

Bjorn

[PATCH v2 09/10] PCI: aspeed: Add ASPEED PCIe RC driver

[Jacky Chou]

> > > > +#define MAX_MSI_HOST_IRQS	64
> > > > +#define PCIE_RESET_CONFIG_DEVICE_WAIT_MS	500
> > >
> > > Where does this value come from?  Is there a generic value from
> > > drivers/pci/pci.h you can use?
> >
> > We check the PCIe specification to find these contents.
> >
> > "With a Downstream Port that supports Link speeds greater than 5.0
> > GT/s, software must wait a minimum of 100 ms after Link training
> > completes before sending a Configuration Request to the device
> > immediately below that Port."
> >
> > So, we think delay 500ms to let kernel issue the first configuration
> > command is enough after deassert PERST.
> 
> Isn't this PCIE_RESET_CONFIG_WAIT_MS?
> 
> I prefer to use #defines from the PCI core whenever possible because it makes
> it easier to ensure that all drivers include the required delays.
> 

Thank you for pointing this define.
I change to this define in next version.

> > > > +#define PCIE_RESET_CONFIG_RC_WAIT_MS		10
> > >
> > > Ditto.  If it's an Aspeed-specific value, can you point to the
> > > source in the Aspeed datasheet?
> >
> > This delay is set to ensure that the RC internal settings are
> > completely reset.  We do not put its usage in our datasheet.
> 
> The "PCIE_" prefix suggests something required by the PCIe base spec.
> If this is an Aspeed-specific value, perhaps remove the "PCIE_"
> prefix?
> 

Agreed.

> > > > +static int aspeed_ast2700_child_config(struct pci_bus *bus,
> > > > +unsigned int
> > > devfn,
> > > > +				       int where, int size, u32 *val,
> > > > +				       bool write)
> > > > +{
> > > > +	struct aspeed_pcie *pcie = bus->sysdata;
> > > > +	u32 bdf_offset, status, cfg_val;
> > > > +	int ret;
> > > > +
> > > > +	bdf_offset = aspeed_pcie_get_bdf_offset(bus, devfn, where);
> > > > +
> > > > +	cfg_val = CRG_PAYLOAD_SIZE;
> > > > +	if (write)
> > > > +		cfg_val |= (bus->number == 1) ? CRG0_WRITE_FMTTYPE :
> > > CRG1_WRITE_FMTTYPE;
> > > > +	else
> > > > +		cfg_val |= (bus->number == 1) ? CRG0_READ_FMTTYPE :
> > > > +CRG1_READ_FMTTYPE;
> > >
> > > I don't think you should assume that bus 0 is the root bus.  The
> > > root bus number should come from the DT bus-range.
> 
> Just making sure you saw this part since you didn't mention it.
> 

Agreed.
I have checked our design and verified in different root bus number.
I will modify this part in next version.

Thanks,
Jacky

Re: [PATCH v2 09/10] PCI: aspeed: Add ASPEED PCIe RC driver

[Markus Elfring]

…
> +++ b/drivers/pci/controller/pcie-aspeed.c
> @@ -0,0 +1,1137 @@
…
> +static int aspeed_irq_msi_domain_alloc(struct irq_domain *domain,
> +				       unsigned int virq, unsigned int nr_irqs,
> +				       void *args)
> +{
…
> +	mutex_lock(&pcie->lock);
> +
> +	bit = bitmap_find_free_region(pcie->msi_irq_in_use, MAX_MSI_HOST_IRQS,
> +				      get_count_order(nr_irqs));
> +
> +	mutex_unlock(&pcie->lock);
…

Under which circumstances would you become interested to apply a statement
like “guard(mutex)(&pcie->lock);”?
https://elixir.bootlin.com/linux/v6.16-rc6/source/include/linux/mutex.h#L225

Regards,
Markus

[PATCH v2 09/10] PCI: aspeed: Add ASPEED PCIe RC driver

[Jacky Chou]

Hi Markus,

Thank you for your reply.

> > +++ b/drivers/pci/controller/pcie-aspeed.c
> > @@ -0,0 +1,1137 @@
> …
> > +static int aspeed_irq_msi_domain_alloc(struct irq_domain *domain,
> > +				       unsigned int virq, unsigned int nr_irqs,
> > +				       void *args)
> > +{
> …
> > +	mutex_lock(&pcie->lock);
> > +
> > +	bit = bitmap_find_free_region(pcie->msi_irq_in_use,
> MAX_MSI_HOST_IRQS,
> > +				      get_count_order(nr_irqs));
> > +
> > +	mutex_unlock(&pcie->lock);
> …
> 
> Under which circumstances would you become interested to apply a statement
> like “guard(mutex)(&pcie->lock);”?
> https://elixir.bootlin.com/linux/v6.16-rc6/source/include/linux/mutex.h#L225
> 

Thanks for the information that made me know this method.
I just referred to the existed drivers in Linux before, and it seems there are a lot of drivers keeping the original usage.
So, I have no idea Linux have introduced these methods.
I will take care of these usages in my develop and will change it in this driver in next version.

Thanks,
Jacky

Re: [PATCH v2 09/10] PCI: aspeed: Add ASPEED PCIe RC driver

[kernel test robot]

Hi Jacky,

kernel test robot noticed the following build warnings:

[auto build test WARNING on pci/next]
[also build test WARNING on pci/for-linus robh/for-next linusw-pinctrl/devel linusw-pinctrl/for-next linus/master v6.16-rc6 next-20250715]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jacky-Chou/dt-bindings-soc-aspeed-Add-ASPEED-PCIe-Config-support/20250715-114814
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20250715034320.2553837-10-jacky_chou%40aspeedtech.com
patch subject: [PATCH v2 09/10] PCI: aspeed: Add ASPEED PCIe RC driver
config: loongarch-allyesconfig (https://download.01.org/0day-ci/archive/20250716/202507160642.yzIrPY1i-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project 16534d19bf50bde879a83f0ae62875e2c5120e64)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250716/202507160642.yzIrPY1i-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507160642.yzIrPY1i-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> Warning: drivers/pci/controller/pcie-aspeed.c:179 struct member 'pciephy' not described in 'aspeed_pcie_port'
   Warning: drivers/pci/controller/pcie-aspeed.c:179 Excess struct member 'phy' description in 'aspeed_pcie_port'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v2 10/10] MAINTAINERS: Add ASPEED PCIe RC driver

[Jacky Chou]

Add maintainer for ASPEED PCIe RC driver.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 MAINTAINERS | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3ecb44458a7e..e1839dc240bc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3696,6 +3696,16 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/media/aspeed,video-engine.yaml
 F:	drivers/media/platform/aspeed/
 
+ASPEED PCIE CONTROLLER DRIVER
+M:	Jacky Chou <jacky_chou@aspeedtech.com>
+L:	linux-aspeed@lists.ozlabs.org (moderated for non-subscribers)
+L:	linux-pci@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/pci/aspeed,ast2600-pcie.yaml
+F:	Documentation/devicetree/bindings/soc/aspeed/aspeed,pcie-cfg.yaml
+F:	Documentation/devicetree/bindings/soc/aspeed/aspeed,pcie-phy.yaml
+F:	drivers/pci/controller/pcie-aspeed.c
+
 ASUS EC HARDWARE MONITOR DRIVER
 M:	Eugene Shalygin <eugene.shalygin@gmail.com>
 L:	linux-hwmon@vger.kernel.org
-- 
2.43.0