[PATCH v1 0/3] Add JH7110 PCIe driver support

[PATCH v1 0/3] Add JH7110 PCIe driver support

[Minda Chen]

This patchset adds PCIe driver for the StarFive JH7110 SoC.
The patch has been tested on the VisionFive 2 board. The test
devices include M.2 NVMe SSD and Realtek 8169 Ethernet adapter.

This patchset should be applied after the patchset [1], [2], [3] and[4]:
[1] https://patchwork.kernel.org/project/linux-riscv/cover/20230314124404.117592-1-xingyu.wu@starfivetech.com/
[2] https://lore.kernel.org/all/20230315055813.94740-1-william.qiu@starfivetech.com/
[3] https://patchwork.kernel.org/project/linux-phy/cover/20230315100421.133428-1-changhuang.liang@starfivetech.com/
[4] https://patchwork.kernel.org/project/linux-usb/cover/20230406015216.27034-1-minda.chen@starfivetech.com/

This patchset is base on v6.3-rc4

patch 1 is PCIe dt-binding document.
patch 2 is PCIe 2.0 driver codes.
patch 3 is PCIe device tree configuration.


Minda Chen (3):
  dt-binding: pci: add JH7110 PCIe dt-binding documents.
  pcie: starfive: add StarFive JH7110 PCIe driver.
  riscv: dts: starfive: add PCIe dts configuration for JH7110

 .../bindings/pci/starfive,jh7110-pcie.yaml    | 163 +++
 MAINTAINERS                                   |   6 +
 .../jh7110-starfive-visionfive-2.dtsi         |  58 ++
 arch/riscv/boot/dts/starfive/jh7110.dtsi      |  88 ++
 drivers/pci/controller/Kconfig                |   8 +
 drivers/pci/controller/Makefile               |   1 +
 drivers/pci/controller/pcie-starfive.c        | 958 ++++++++++++++++++
 7 files changed, 1282 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
 create mode 100644 drivers/pci/controller/pcie-starfive.c


base-commit: 197b6b60ae7bc51dd0814953c562833143b292aa
prerequisite-patch-id: 55390537360f25c8b9cbfdc30b73ade004f436f7
prerequisite-patch-id: bb939c0c7c26b08addfccd890f9d3974b6eaec53
prerequisite-patch-id: 8a6f135bcabdad4a4bfb21f0c6a0ffd2bb57efe7
prerequisite-patch-id: c2366f993a9d85e28c06d8d09f064dd5e8b29a61
prerequisite-patch-id: 50d53a21f91f4087fc80b6f1f72864adfb0002b9
prerequisite-patch-id: 0df3703af91c30f1ca2c47f5609012f2d7200028
prerequisite-patch-id: 89f049f951e5acf75aab92541992f816fd0acc0d
prerequisite-patch-id: 551fae54377090044c3612fca9740a9b359abdd2
prerequisite-patch-id: c7fdf904f398d478f0ed6d57eb878982bc73329d
prerequisite-patch-id: 1b2d0982b18da060c82134f05bf3ce16425bac8d
prerequisite-patch-id: 090ba4b78d47bc19204916e76fdbc70021785388
prerequisite-patch-id: a5d9e0f7d4f8163f566678894cf693015119f2d9
prerequisite-patch-id: 4637a8fa2334a45fa6b64351f4e9e28d3e2d60d3
prerequisite-patch-id: 32647ec60a3b614e1c59ec8e54cb511ae832c22f
prerequisite-patch-id: aa06658ecf89c92d0dfdd6a4ba6d9e6e67532971
prerequisite-patch-id: 258ea5f9b8bf41b6981345dcc81795f25865d38f
prerequisite-patch-id: 8b6f2c9660c0ac0ee4e73e4c21aca8e6b75e81b9
prerequisite-patch-id: dbb0c0151b8bdf093e6ce79fd2fe3f60791a6e0b
prerequisite-patch-id: e7773c977a7b37692e9792b21cc4f17fa58f9215
prerequisite-patch-id: d57e95d31686772abc4c4d5aa1cadc344dc293cd
prerequisite-patch-id: 9f911969d0a550648493952c99096d26e05d4d83
prerequisite-patch-id: 41eddeabff082d08a76d8da523f90da4b5218d28
prerequisite-patch-id: 2ddada18ab6ea5cd1da14212aaf59632f5203d40
prerequisite-patch-id: 398744c61913c76a35754de867c4f820ca7a8d99
prerequisite-patch-id: 1a2c49c1cf81607f062f35898457037d86598cf9
prerequisite-patch-id: f59269382164b5d642a5e10443ca447f5caa595c
prerequisite-patch-id: 1babe83d6bf999bad17584dc595480f9070a5369
prerequisite-patch-id: d95ea69f88a048ef702dceed0f2edee31e6fdfd2
prerequisite-patch-id: 77be3d122d66df813f13088141ce27b21107a341
prerequisite-patch-id: 9fbb7ad1dd258bb8ff5946c4a0e59de4bfd82a04
prerequisite-patch-id: a02411a8fe009acc0888e4a7d60233c9ee5a1e71
prerequisite-patch-id: 6f6984916dffd0cc66aa733c9b6bd3a55495a50c
prerequisite-patch-id: 584c256c9acb52ee2773d0c81c3f4977fc18155a
prerequisite-patch-id: b37ac15032973e1fcd918f157c82a0606775c9e9
prerequisite-patch-id: 999c243dca89d56d452aa52ea3e181358b5c1d80
prerequisite-patch-id: ca653566085079839fb3dc1e722effafbc8109a4
prerequisite-patch-id: 2fe72c216673efe690db54cbf500ba7f15e6247a
prerequisite-patch-id: 776afb78743657e4a6bfebd2cd8a44c5c9017ee2
prerequisite-patch-id: c9b92e6d1531d0a9fb122d9d038cc3d9df380e0f
prerequisite-patch-id: c0fa5b1d697ab8434954a81a5992cf66d0cfafb8
prerequisite-patch-id: 1cca26d07ec4bc7ea07b1c6815631c8bf8224366
prerequisite-patch-id: 331bafaf79b4bb7c09152eb16598cdc9ba8096e9
prerequisite-patch-id: 9f2286829c70b6940c83834b150df685ab02c591
prerequisite-patch-id: b25b8295d7b8fdf1b45b634df5c2a7a7f9dfba76
prerequisite-patch-id: 9d3aab2e4428be5b1235a57dad3bf14eae5275fd
prerequisite-patch-id: 6090abbef6164dd0cc87d44c486a7eb1b5f64946
-- 
2.17.1

[PATCH v1 1/3] dt-binding: pci: add JH7110 PCIe dt-binding documents.

[Minda Chen]

Add PCIe controller driver dt-binding documents
for StarFive JH7110 SoC platform.

Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
---
 .../bindings/pci/starfive,jh7110-pcie.yaml    | 163 ++++++++++++++++++
 1 file changed, 163 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml

diff --git a/Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml b/Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
new file mode 100644
index 000000000000..fa4829766195
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
@@ -0,0 +1,163 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/starfive,jh7110-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive JH7110 PCIe 2.0 host controller
+
+maintainers:
+  - Minda Chen <minda.chen@starfivetech.com>
+
+allOf:
+  - $ref: /schemas/pci/pci-bus.yaml#
+  - $ref: /schemas/interrupt-controller/msi-controller.yaml#
+
+properties:
+  compatible:
+    const: starfive,jh7110-pcie
+
+  reg:
+    maxItems: 2
+
+  reg-names:
+    items:
+      - const: reg
+      - const: config
+
+  msi-parent: true
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 4
+
+  clock-names:
+    items:
+      - const: noc
+      - const: tl
+      - const: axi_mst0
+      - const: apb
+
+  resets:
+    items:
+      - description: AXI MST0 reset
+      - description: AXI SLAVE reset
+      - description: AXI SLAVE0 reset
+      - description: PCIE BRIDGE reset
+      - description: PCIE CORE reset
+      - description: PCIE APB reset
+
+  reset-names:
+    items:
+      - const: mst0
+      - const: slv0
+      - const: slv
+      - const: brg
+      - const: core
+      - const: apb
+
+  starfive,stg-syscon:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      items:
+        - description: phandle to System Register Controller stg_syscon node.
+        - description: register0 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
+        - description: register1 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
+        - description: register2 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
+        - description: register3 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
+    description:
+      The phandle to System Register Controller syscon node and the offset
+      of STG_SYSCONSAIF__SYSCFG register for PCIe. Total 4 regsisters offset
+      for PCIe.
+
+  pwren-gpios:
+    description: Should specify the GPIO for controlling the PCI bus device power on.
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+  phys:
+    maxItems: 1
+
+  interrupt-controller:
+    type: object
+    properties:
+      '#address-cells':
+        const: 0
+
+      '#interrupt-cells':
+        const: 1
+
+      interrupt-controller: true
+
+    required:
+      - '#address-cells'
+      - '#interrupt-cells'
+      - interrupt-controller
+
+    additionalProperties: false
+
+required:
+  - reg
+  - reg-names
+  - "#interrupt-cells"
+  - interrupts
+  - interrupt-map-mask
+  - interrupt-map
+  - clocks
+  - clock-names
+  - resets
+  - msi-controller
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    bus {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        pcie0: pcie@2B000000 {
+            compatible = "starfive,jh7110-pcie";
+            #address-cells = <3>;
+            #size-cells = <2>;
+            #interrupt-cells = <1>;
+            reg = <0x0 0x2B000000 0x0 0x1000000>,
+                  <0x9 0x40000000 0x0 0x10000000>;
+            reg-names = "reg", "config";
+            device_type = "pci";
+            starfive,stg-syscon = <&stg_syscon 0xc0 0xc4 0x130 0x1b8>;
+            bus-range = <0x0 0xff>;
+            ranges = <0x82000000  0x0 0x30000000  0x0 0x30000000 0x0 0x08000000>,
+                     <0xc3000000  0x9 0x00000000  0x9 0x00000000 0x0 0x40000000>;
+            interrupt-parent = <&plic>;
+            interrupts = <56>;
+            interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+            interrupt-map = <0x0 0x0 0x0 0x1 &pcie_intc0 0x1>,
+                            <0x0 0x0 0x0 0x2 &pcie_intc0 0x2>,
+                            <0x0 0x0 0x0 0x3 &pcie_intc0 0x3>,
+                            <0x0 0x0 0x0 0x4 &pcie_intc0 0x4>;
+            msi-parent = <&pcie0>;
+            msi-controller;
+            clocks = <&syscrg 86>,
+                     <&stgcrg 10>,
+                     <&stgcrg 8>,
+                     <&stgcrg 9>;
+            clock-names = "noc", "tl", "axi_mst0", "apb";
+            resets = <&stgcrg 11>,
+                     <&stgcrg 12>,
+                     <&stgcrg 13>,
+                     <&stgcrg 14>,
+                     <&stgcrg 15>,
+                     <&stgcrg 16>;
+
+            pcie_intc0: interrupt-controller {
+                #address-cells = <0>;
+                #interrupt-cells = <1>;
+                interrupt-controller;
+            };
+        };
+    };
-- 
2.17.1

Re: [PATCH v1 1/3] dt-binding: pci: add JH7110 PCIe dt-binding documents.

[Krzysztof Kozlowski]

On 06/04/2023 13:11, Minda Chen wrote:
> Add PCIe controller driver dt-binding documents
> for StarFive JH7110 SoC platform.

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching). Missing: 's'

Subject: drop second/last, redundant "dt-binding documents". The
"dt-bindings" prefix is already stating that these are bindings and
documentation.

Drop also full stop.

> 
> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
> ---
>  .../bindings/pci/starfive,jh7110-pcie.yaml    | 163 ++++++++++++++++++
>  1 file changed, 163 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml b/Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
> new file mode 100644
> index 000000000000..fa4829766195
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
> @@ -0,0 +1,163 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/starfive,jh7110-pcie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive JH7110 PCIe 2.0 host controller
> +
> +maintainers:
> +  - Minda Chen <minda.chen@starfivetech.com>
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-bus.yaml#
> +  - $ref: /schemas/interrupt-controller/msi-controller.yaml#
> +
> +properties:
> +  compatible:
> +    const: starfive,jh7110-pcie
> +
> +  reg:
> +    maxItems: 2
> +
> +  reg-names:
> +    items:
> +      - const: reg
> +      - const: config
> +
> +  msi-parent: true
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 4
> +
> +  clock-names:
> +    items:
> +      - const: noc
> +      - const: tl
> +      - const: axi_mst0
> +      - const: apb
> +
> +  resets:
> +    items:
> +      - description: AXI MST0 reset
> +      - description: AXI SLAVE reset
> +      - description: AXI SLAVE0 reset
> +      - description: PCIE BRIDGE reset
> +      - description: PCIE CORE reset
> +      - description: PCIE APB reset
> +
> +  reset-names:
> +    items:
> +      - const: mst0
> +      - const: slv0
> +      - const: slv
> +      - const: brg
> +      - const: core
> +      - const: apb
> +
> +  starfive,stg-syscon:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      items:
> +        - description: phandle to System Register Controller stg_syscon node.
> +        - description: register0 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
> +        - description: register1 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
> +        - description: register2 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
> +        - description: register3 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
> +    description:
> +      The phandle to System Register Controller syscon node and the offset
> +      of STG_SYSCONSAIF__SYSCFG register for PCIe. Total 4 regsisters offset
> +      for PCIe.
> +
> +  pwren-gpios:
> +    description: Should specify the GPIO for controlling the PCI bus device power on.

What are these? Different than defined in gpio-consumer-common?

> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  phys:
> +    maxItems: 1
> +
> +  interrupt-controller:
> +    type: object
> +    properties:
> +      '#address-cells':
> +        const: 0
> +
> +      '#interrupt-cells':
> +        const: 1
> +
> +      interrupt-controller: true
> +
> +    required:
> +      - '#address-cells'
> +      - '#interrupt-cells'
> +      - interrupt-controller
> +
> +    additionalProperties: false
> +
> +required:
> +  - reg
> +  - reg-names
> +  - "#interrupt-cells"

Keep consistent quotes - either ' or "

Are you sure this is correct? You have interrupt controller as child node.


> +  - interrupts
> +  - interrupt-map-mask
> +  - interrupt-map
> +  - clocks
> +  - clock-names
> +  - resets
> +  - msi-controller
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    bus {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        pcie0: pcie@2B000000 {

Lowercase hex. Everywhere.

> +            compatible = "starfive,jh7110-pcie";
> +            #address-cells = <3>;
> +            #size-cells = <2>;
> +            #interrupt-cells = <1>;
> +            reg = <0x0 0x2B000000 0x0 0x1000000>,
> +                  <0x9 0x40000000 0x0 0x10000000>;

reg (and reg-names and ranges) is always second property.

> +            reg-names = "reg", "config";
> +            device_type = "pci";
> +            starfive,stg-syscon = <&stg_syscon 0xc0 0xc4 0x130 0x1b8>;
> +            bus-range = <0x0 0xff>;
> +            ranges = <0x82000000  0x0 0x30000000  0x0 0x30000000 0x0 0x08000000>,
> +                     <0xc3000000  0x9 0x00000000  0x9 0x00000000 0x0 0x40000000>;
> +            interrupt-parent = <&plic>;
> +            interrupts = <56>;
> +            interrupt-map-mask = <0x0 0x0 0x0 0x7>;
> +            interrupt-map = <0x0 0x0 0x0 0x1 &pcie_intc0 0x1>,
> +                            <0x0 0x0 0x0 0x2 &pcie_intc0 0x2>,
> +                            <0x0 0x0 0x0 0x3 &pcie_intc0 0x3>,
> +                            <0x0 0x0 0x0 0x4 &pcie_intc0 0x4>;


Best regards,
Krzysztof

Re: [PATCH v1 1/3] dt-binding: pci: add JH7110 PCIe dt-binding documents.

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1525 bytes --]

On Thu, Apr 06, 2023 at 08:24:55PM +0200, Krzysztof Kozlowski wrote:
> On 06/04/2023 13:11, Minda Chen wrote:
> > +
> > +  interrupt-controller:
> > +    type: object
> > +    properties:
> > +      '#address-cells':
> > +        const: 0
> > +
> > +      '#interrupt-cells':
> > +        const: 1
> > +
> > +      interrupt-controller: true
> > +
> > +    required:
> > +      - '#address-cells'
> > +      - '#interrupt-cells'
> > +      - interrupt-controller
> > +
> > +    additionalProperties: false
> > +
> > +required:
> > +  - reg
> > +  - reg-names
> > +  - "#interrupt-cells"
> 
> Keep consistent quotes - either ' or "
> 
> Are you sure this is correct? You have interrupt controller as child node.

I know existing stuff in-tree is far from a guarantee that it'll be
right, but this does at least follow what we've got for PolarFire SoC:
Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml

Both PLDA and both RISC-V w/ a PLIC as the interrupt controller, so in
similar waters.
This note existed in the original text form binding of the Microchip
PCI controller:
| +NOTE:
| +The core provides a single interrupt for both INTx/MSI messages. So,
| +create an interrupt controller node to support 'interrupt-map' DT
| +functionality.  The driver will create an IRQ domain for this map, decode
| +the four INTx interrupts in ISR and route them to this domain.

Given the similarities, I figure the same requirement applies here too.
Minda?

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 1/3] dt-binding: pci: add JH7110 PCIe dt-binding documents.

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1913 bytes --]

On Thu, Apr 06, 2023 at 07:35:09PM +0100, Conor Dooley wrote:
> On Thu, Apr 06, 2023 at 08:24:55PM +0200, Krzysztof Kozlowski wrote:
> > On 06/04/2023 13:11, Minda Chen wrote:
> > > +
> > > +  interrupt-controller:
> > > +    type: object
> > > +    properties:
> > > +      '#address-cells':
> > > +        const: 0
> > > +
> > > +      '#interrupt-cells':
> > > +        const: 1
> > > +
> > > +      interrupt-controller: true
> > > +
> > > +    required:
> > > +      - '#address-cells'
> > > +      - '#interrupt-cells'
> > > +      - interrupt-controller
> > > +
> > > +    additionalProperties: false
> > > +
> > > +required:
> > > +  - reg
> > > +  - reg-names
> > > +  - "#interrupt-cells"
> > 
> > Keep consistent quotes - either ' or "
> > 
> > Are you sure this is correct? You have interrupt controller as child node.
> 
> I know existing stuff in-tree is far from a guarantee that it'll be
> right, but this does at least follow what we've got for PolarFire SoC:
> Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> 
> Both PLDA and both RISC-V w/ a PLIC as the interrupt controller, so in
> similar waters.
> This note existed in the original text form binding of the Microchip
> PCI controller:
> | +NOTE:
> | +The core provides a single interrupt for both INTx/MSI messages. So,
> | +create an interrupt controller node to support 'interrupt-map' DT
> | +functionality.  The driver will create an IRQ domain for this map, decode
> | +the four INTx interrupts in ISR and route them to this domain.
> 
> Given the similarities, I figure the same requirement applies here too.
> Minda?

Further, if, as I currently suspect, there's a lot of commonality here,
should the binding as well as the driver be split into common pdla bits
and microchip/starfive specific ones?

Suppose that's more one for you Krzysztof.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 1/3] dt-binding: pci: add JH7110 PCIe dt-binding documents.

[Krzysztof Kozlowski]

On 06/04/2023 20:45, Conor Dooley wrote:
> On Thu, Apr 06, 2023 at 07:35:09PM +0100, Conor Dooley wrote:
>> On Thu, Apr 06, 2023 at 08:24:55PM +0200, Krzysztof Kozlowski wrote:
>>> On 06/04/2023 13:11, Minda Chen wrote:
>>>> +
>>>> +  interrupt-controller:
>>>> +    type: object
>>>> +    properties:
>>>> +      '#address-cells':
>>>> +        const: 0
>>>> +
>>>> +      '#interrupt-cells':
>>>> +        const: 1
>>>> +
>>>> +      interrupt-controller: true
>>>> +
>>>> +    required:
>>>> +      - '#address-cells'
>>>> +      - '#interrupt-cells'
>>>> +      - interrupt-controller
>>>> +
>>>> +    additionalProperties: false
>>>> +
>>>> +required:
>>>> +  - reg
>>>> +  - reg-names
>>>> +  - "#interrupt-cells"
>>>
>>> Keep consistent quotes - either ' or "
>>>
>>> Are you sure this is correct? You have interrupt controller as child node.
>>
>> I know existing stuff in-tree is far from a guarantee that it'll be
>> right, but this does at least follow what we've got for PolarFire SoC:
>> Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
>>
>> Both PLDA and both RISC-V w/ a PLIC as the interrupt controller, so in
>> similar waters.
>> This note existed in the original text form binding of the Microchip
>> PCI controller:
>> | +NOTE:
>> | +The core provides a single interrupt for both INTx/MSI messages. So,
>> | +create an interrupt controller node to support 'interrupt-map' DT
>> | +functionality.  The driver will create an IRQ domain for this map, decode
>> | +the four INTx interrupts in ISR and route them to this domain.
>>
>> Given the similarities, I figure the same requirement applies here too.
>> Minda?
> 
> Further, if, as I currently suspect, there's a lot of commonality here,
> should the binding as well as the driver be split into common pdla bits
> and microchip/starfive specific ones?
> 
> Suppose that's more one for you Krzysztof.

Yeah, looks like only clocks and resets are different. At the end it
depends how much code you would remove...

Best regards,
Krzysztof

Re: [PATCH v1 1/3] dt-binding: pci: add JH7110 PCIe dt-binding documents.

[Minda Chen]

On 2023/4/7 2:35, Conor Dooley wrote:
> On Thu, Apr 06, 2023 at 08:24:55PM +0200, Krzysztof Kozlowski wrote:
>> On 06/04/2023 13:11, Minda Chen wrote:
>> > +
>> > +  interrupt-controller:
>> > +    type: object
>> > +    properties:
>> > +      '#address-cells':
>> > +        const: 0
>> > +
>> > +      '#interrupt-cells':
>> > +        const: 1
>> > +
>> > +      interrupt-controller: true
>> > +
>> > +    required:
>> > +      - '#address-cells'
>> > +      - '#interrupt-cells'
>> > +      - interrupt-controller
>> > +
>> > +    additionalProperties: false
>> > +
>> > +required:
>> > +  - reg
>> > +  - reg-names
>> > +  - "#interrupt-cells"
>> 
>> Keep consistent quotes - either ' or "
>> 
>> Are you sure this is correct? You have interrupt controller as child node.
> 
> I know existing stuff in-tree is far from a guarantee that it'll be
> right, but this does at least follow what we've got for PolarFire SoC:
> Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> 
> Both PLDA and both RISC-V w/ a PLIC as the interrupt controller, so in
> similar waters.
> This note existed in the original text form binding of the Microchip
> PCI controller:
> | +NOTE:
> | +The core provides a single interrupt for both INTx/MSI messages. So,
> | +create an interrupt controller node to support 'interrupt-map' DT
> | +functionality.  The driver will create an IRQ domain for this map, decode
> | +the four INTx interrupts in ISR and route them to this domain.
> 
> Given the similarities, I figure the same requirement applies here too.
> Minda?
> 
> Cheers,
> Conor.

Yes, Thanks very much.

Re: [PATCH v1 1/3] dt-binding: pci: add JH7110 PCIe dt-binding documents.

[Minda Chen]

On 2023/4/7 2:24, Krzysztof Kozlowski wrote:
> On 06/04/2023 13:11, Minda Chen wrote:
>> Add PCIe controller driver dt-binding documents
>> for StarFive JH7110 SoC platform.
> 
> Use subject prefixes matching the subsystem (which you can get for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching). Missing: 's'
> 
> Subject: drop second/last, redundant "dt-binding documents". The
> "dt-bindings" prefix is already stating that these are bindings and
> documentation.
> 
> Drop also full stop.
> 
ok, thanks
>> 
>> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
>> ---
>>  .../bindings/pci/starfive,jh7110-pcie.yaml    | 163 ++++++++++++++++++
>>  1 file changed, 163 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml b/Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
>> new file mode 100644
>> index 000000000000..fa4829766195
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
>> @@ -0,0 +1,163 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pci/starfive,jh7110-pcie.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: StarFive JH7110 PCIe 2.0 host controller
>> +
>> +maintainers:
>> +  - Minda Chen <minda.chen@starfivetech.com>
>> +
>> +allOf:
>> +  - $ref: /schemas/pci/pci-bus.yaml#
>> +  - $ref: /schemas/interrupt-controller/msi-controller.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    const: starfive,jh7110-pcie
>> +
>> +  reg:
>> +    maxItems: 2
>> +
>> +  reg-names:
>> +    items:
>> +      - const: reg
>> +      - const: config
>> +
>> +  msi-parent: true
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 4
>> +
>> +  clock-names:
>> +    items:
>> +      - const: noc
>> +      - const: tl
>> +      - const: axi_mst0
>> +      - const: apb
>> +
>> +  resets:
>> +    items:
>> +      - description: AXI MST0 reset
>> +      - description: AXI SLAVE reset
>> +      - description: AXI SLAVE0 reset
>> +      - description: PCIE BRIDGE reset
>> +      - description: PCIE CORE reset
>> +      - description: PCIE APB reset
>> +
>> +  reset-names:
>> +    items:
>> +      - const: mst0
>> +      - const: slv0
>> +      - const: slv
>> +      - const: brg
>> +      - const: core
>> +      - const: apb
>> +
>> +  starfive,stg-syscon:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    items:
>> +      items:
>> +        - description: phandle to System Register Controller stg_syscon node.
>> +        - description: register0 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
>> +        - description: register1 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
>> +        - description: register2 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
>> +        - description: register3 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
>> +    description:
>> +      The phandle to System Register Controller syscon node and the offset
>> +      of STG_SYSCONSAIF__SYSCFG register for PCIe. Total 4 regsisters offset
>> +      for PCIe.
>> +
>> +  pwren-gpios:
>> +    description: Should specify the GPIO for controlling the PCI bus device power on.
> 
> What are these? Different than defined in gpio-consumer-common?
> 
power gpio board level configuration. It it not a requried property
>> +    maxItems: 1
>> +
>> +  reset-gpios:
>> +    maxItems: 1
>> +
>> +  phys:
>> +    maxItems: 1
>> +
>> +  interrupt-controller:
>> +    type: object
>> +    properties:
>> +      '#address-cells':
>> +        const: 0
>> +
>> +      '#interrupt-cells':
>> +        const: 1
>> +
>> +      interrupt-controller: true
>> +
>> +    required:
>> +      - '#address-cells'
>> +      - '#interrupt-cells'
>> +      - interrupt-controller
>> +
>> +    additionalProperties: false
>> +
>> +required:
>> +  - reg
>> +  - reg-names
>> +  - "#interrupt-cells"
> 
> Keep consistent quotes - either ' or "
> 
> Are you sure this is correct? You have interrupt controller as child node.
> 
> 
>> +  - interrupts
>> +  - interrupt-map-mask
>> +  - interrupt-map
>> +  - clocks
>> +  - clock-names
>> +  - resets
>> +  - msi-controller
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    bus {
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
>> +        pcie0: pcie@2B000000 {
> 
> Lowercase hex. Everywhere.
> 
ok
>> +            compatible = "starfive,jh7110-pcie";
>> +            #address-cells = <3>;
>> +            #size-cells = <2>;
>> +            #interrupt-cells = <1>;
>> +            reg = <0x0 0x2B000000 0x0 0x1000000>,
>> +                  <0x9 0x40000000 0x0 0x10000000>;
> 
> reg (and reg-names and ranges) is always second property.
> 
ok
>> +            reg-names = "reg", "config";
>> +            device_type = "pci";
>> +            starfive,stg-syscon = <&stg_syscon 0xc0 0xc4 0x130 0x1b8>;
>> +            bus-range = <0x0 0xff>;
>> +            ranges = <0x82000000  0x0 0x30000000  0x0 0x30000000 0x0 0x08000000>,
>> +                     <0xc3000000  0x9 0x00000000  0x9 0x00000000 0x0 0x40000000>;
>> +            interrupt-parent = <&plic>;
>> +            interrupts = <56>;
>> +            interrupt-map-mask = <0x0 0x0 0x0 0x7>;
>> +            interrupt-map = <0x0 0x0 0x0 0x1 &pcie_intc0 0x1>,
>> +                            <0x0 0x0 0x0 0x2 &pcie_intc0 0x2>,
>> +                            <0x0 0x0 0x0 0x3 &pcie_intc0 0x3>,
>> +                            <0x0 0x0 0x0 0x4 &pcie_intc0 0x4>;
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v1 1/3] dt-binding: pci: add JH7110 PCIe dt-binding documents.

[Krzysztof Kozlowski]

On 10/04/2023 11:05, Minda Chen wrote:
>>> +
>>> +  starfive,stg-syscon:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>> +    items:
>>> +      items:
>>> +        - description: phandle to System Register Controller stg_syscon node.
>>> +        - description: register0 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
>>> +        - description: register1 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
>>> +        - description: register2 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
>>> +        - description: register3 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
>>> +    description:
>>> +      The phandle to System Register Controller syscon node and the offset
>>> +      of STG_SYSCONSAIF__SYSCFG register for PCIe. Total 4 regsisters offset
>>> +      for PCIe.
>>> +
>>> +  pwren-gpios:
>>> +    description: Should specify the GPIO for controlling the PCI bus device power on.
>>
>> What are these? Different than defined in gpio-consumer-common?
>>
> power gpio board level configuration. It it not a requried property

What is "board level configuration"? Again - is it different than
powerdown-gpios from gpio-consumer-common.yaml?



Best regards,
Krzysztof

Re: [PATCH v1 1/3] dt-binding: pci: add JH7110 PCIe dt-binding documents.

[Minda Chen]

On 2023/4/10 23:21, Krzysztof Kozlowski wrote:
> On 10/04/2023 11:05, Minda Chen wrote:
>>>> +
>>>> +  starfive,stg-syscon:
>>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>>> +    items:
>>>> +      items:
>>>> +        - description: phandle to System Register Controller stg_syscon node.
>>>> +        - description: register0 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
>>>> +        - description: register1 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
>>>> +        - description: register2 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
>>>> +        - description: register3 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
>>>> +    description:
>>>> +      The phandle to System Register Controller syscon node and the offset
>>>> +      of STG_SYSCONSAIF__SYSCFG register for PCIe. Total 4 regsisters offset
>>>> +      for PCIe.
>>>> +
>>>> +  pwren-gpios:
>>>> +    description: Should specify the GPIO for controlling the PCI bus device power on.
>>>
>>> What are these? Different than defined in gpio-consumer-common?
>>>
>> power gpio board level configuration. It it not a requried property
> 
> What is "board level configuration"? Again - is it different than
> powerdown-gpios from gpio-consumer-common.yaml?
> 
> 
I am sorry. I will change to powerdown-gpios follow gpio-consumer-common.yaml
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v1 1/3] dt-binding: pci: add JH7110 PCIe dt-binding documents.

[Bin Meng]

On Thu, Apr 6, 2023 at 7:59 PM Minda Chen <minda.chen@starfivetech.com> wrote:
>
> Add PCIe controller driver dt-binding documents
> for StarFive JH7110 SoC platform.
>
> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
> ---
>  .../bindings/pci/starfive,jh7110-pcie.yaml    | 163 ++++++++++++++++++
>  1 file changed, 163 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
>
> diff --git a/Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml b/Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
> new file mode 100644
> index 000000000000..fa4829766195
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
> @@ -0,0 +1,163 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/starfive,jh7110-pcie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive JH7110 PCIe 2.0 host controller
> +
> +maintainers:
> +  - Minda Chen <minda.chen@starfivetech.com>
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-bus.yaml#
> +  - $ref: /schemas/interrupt-controller/msi-controller.yaml#
> +
> +properties:
> +  compatible:
> +    const: starfive,jh7110-pcie

Since this is a PLDA IP that is likely to be reused by other vendors,
should this indicate the PLDA in the string?

Or the file name should be renamed to something like the synopsis dw-pcie?

> +
> +  reg:
> +    maxItems: 2
> +
> +  reg-names:
> +    items:
> +      - const: reg
> +      - const: config
> +
> +  msi-parent: true
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 4
> +
> +  clock-names:
> +    items:
> +      - const: noc
> +      - const: tl
> +      - const: axi_mst0
> +      - const: apb
> +
> +  resets:
> +    items:
> +      - description: AXI MST0 reset
> +      - description: AXI SLAVE reset
> +      - description: AXI SLAVE0 reset
> +      - description: PCIE BRIDGE reset
> +      - description: PCIE CORE reset
> +      - description: PCIE APB reset
> +
> +  reset-names:
> +    items:
> +      - const: mst0
> +      - const: slv0
> +      - const: slv
> +      - const: brg
> +      - const: core
> +      - const: apb
> +
> +  starfive,stg-syscon:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      items:
> +        - description: phandle to System Register Controller stg_syscon node.
> +        - description: register0 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
> +        - description: register1 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
> +        - description: register2 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
> +        - description: register3 offset of STG_SYSCONSAIF__SYSCFG register for PCIe.
> +    description:
> +      The phandle to System Register Controller syscon node and the offset
> +      of STG_SYSCONSAIF__SYSCFG register for PCIe. Total 4 regsisters offset
> +      for PCIe.
> +
> +  pwren-gpios:
> +    description: Should specify the GPIO for controlling the PCI bus device power on.
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  phys:
> +    maxItems: 1
> +
> +  interrupt-controller:
> +    type: object
> +    properties:
> +      '#address-cells':
> +        const: 0
> +
> +      '#interrupt-cells':
> +        const: 1
> +
> +      interrupt-controller: true
> +
> +    required:
> +      - '#address-cells'
> +      - '#interrupt-cells'
> +      - interrupt-controller
> +
> +    additionalProperties: false
> +
> +required:
> +  - reg
> +  - reg-names
> +  - "#interrupt-cells"
> +  - interrupts
> +  - interrupt-map-mask
> +  - interrupt-map
> +  - clocks
> +  - clock-names
> +  - resets
> +  - msi-controller
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    bus {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        pcie0: pcie@2B000000 {
> +            compatible = "starfive,jh7110-pcie";
> +            #address-cells = <3>;
> +            #size-cells = <2>;
> +            #interrupt-cells = <1>;
> +            reg = <0x0 0x2B000000 0x0 0x1000000>,
> +                  <0x9 0x40000000 0x0 0x10000000>;
> +            reg-names = "reg", "config";
> +            device_type = "pci";
> +            starfive,stg-syscon = <&stg_syscon 0xc0 0xc4 0x130 0x1b8>;
> +            bus-range = <0x0 0xff>;
> +            ranges = <0x82000000  0x0 0x30000000  0x0 0x30000000 0x0 0x08000000>,
> +                     <0xc3000000  0x9 0x00000000  0x9 0x00000000 0x0 0x40000000>;
> +            interrupt-parent = <&plic>;
> +            interrupts = <56>;
> +            interrupt-map-mask = <0x0 0x0 0x0 0x7>;
> +            interrupt-map = <0x0 0x0 0x0 0x1 &pcie_intc0 0x1>,
> +                            <0x0 0x0 0x0 0x2 &pcie_intc0 0x2>,
> +                            <0x0 0x0 0x0 0x3 &pcie_intc0 0x3>,
> +                            <0x0 0x0 0x0 0x4 &pcie_intc0 0x4>;
> +            msi-parent = <&pcie0>;
> +            msi-controller;
> +            clocks = <&syscrg 86>,
> +                     <&stgcrg 10>,
> +                     <&stgcrg 8>,
> +                     <&stgcrg 9>;
> +            clock-names = "noc", "tl", "axi_mst0", "apb";
> +            resets = <&stgcrg 11>,
> +                     <&stgcrg 12>,
> +                     <&stgcrg 13>,
> +                     <&stgcrg 14>,
> +                     <&stgcrg 15>,
> +                     <&stgcrg 16>;
> +
> +            pcie_intc0: interrupt-controller {
> +                #address-cells = <0>;
> +                #interrupt-cells = <1>;
> +                interrupt-controller;
> +            };
> +        };
> +    };
> --

Regards,
Bin

[PATCH v1 2/3] pcie: starfive: add StarFive JH7110 PCIe driver.

[Minda Chen]

Add PCIe controller driver for StarFive JH7110
SoC platform. The PCIe controller is PCIe 2.0, single lane.

Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
---
 MAINTAINERS                            |   6 +
 drivers/pci/controller/Kconfig         |   8 +
 drivers/pci/controller/Makefile        |   1 +
 drivers/pci/controller/pcie-starfive.c | 958 +++++++++++++++++++++++++
 4 files changed, 973 insertions(+)
 create mode 100644 drivers/pci/controller/pcie-starfive.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 0610bbf921bb..52501bcfd1ef 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19977,6 +19977,12 @@ F:	Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yaml
 F:	drivers/phy/starfive/phy-jh7110-pcie.c
 F:	drivers/phy/starfive/phy-jh7110-usb.c
 
+STARFIVE JH71X0 PCIE DRIVERS
+M:	Minda Chen <minda.chen@starfivetech.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
+F:	drivers/pci/controller/pcie-starfive.c
+
 STARFIVE JH71X0 USB DRIVERS
 M:	Emil Renner Berthing <kernel@esmil.dk>
 M:	Minda Chen <minda.chen@starfivetech.com>
diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 42654035654a..3b39080018a5 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -342,6 +342,14 @@ config PCIE_MT7621
 	help
 	  This selects a driver for the MediaTek MT7621 PCIe Controller.
 
+config PCIE_STARFIVE
+	tristate "StarFive JH7110 PCIe controller"
+	depends on PCI_MSI && OF
+	select PCI_MSI_IRQ_DOMAIN
+	help
+	  Say 'Y' here if you want kernel to support the StarFive JH7110
+	  PCIe Host driver.
+
 source "drivers/pci/controller/dwc/Kconfig"
 source "drivers/pci/controller/mobiveil/Kconfig"
 source "drivers/pci/controller/cadence/Kconfig"
diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
index 37c8663de7fe..23708222db8a 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_STARFIVE) += pcie-starfive.o
 
 # pcie-hisi.o quirks are needed even without CONFIG_PCIE_DW
 obj-y				+= dwc/
diff --git a/drivers/pci/controller/pcie-starfive.c b/drivers/pci/controller/pcie-starfive.c
new file mode 100644
index 000000000000..e1dc8ecc769a
--- /dev/null
+++ b/drivers/pci/controller/pcie-starfive.c
@@ -0,0 +1,958 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * PCIe host controller driver for Starfive JH7110 Soc.
+ *
+ * Based on pcie-altera.c, pcie-altera-msi.c.
+ *
+ * Copyright (C) StarFive Technology Co., Ltd.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/msi.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_pci.h>
+#include <linux/pci.h>
+#include <linux/pci-ecam.h>
+#include <linux/phy/phy.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+#include "../pci.h"
+
+#define IMASK_LOCAL			0x180
+#define ISTATUS_LOCAL			0x184
+#define IMSI_ADDR			0x190
+#define ISTATUS_MSI			0x194
+#define GEN_SETTINGS			0x80
+#define PCIE_PCI_IDS			0x9C
+#define PCIE_WINROM			0xFC
+#define PMSG_SUPPORT_RX			0x3F0
+
+#define PCI_MISC			0xB4
+
+#define RP_ENABLE			1
+
+#define IDS_CLASS_CODE_SHIFT		16
+
+#define DATA_LINK_ACTIVE		BIT(5)
+#define PREF_MEM_WIN_64_SUPPORT		BIT(3)
+#define PMSG_LTR_SUPPORT		BIT(2)
+#define LINK_SPEED_GEN2			BIT(12)
+#define PHY_FUNCTION_DIS		BIT(15)
+#define PCIE_FUNC_NUM			4
+#define PHY_FUNC_SHIFT			9
+
+#define XR3PCI_ATR_AXI4_SLV0		0x800
+#define XR3PCI_ATR_SRC_ADDR_LOW		0x0
+#define XR3PCI_ATR_SRC_ADDR_HIGH	0x4
+#define XR3PCI_ATR_TRSL_ADDR_LOW	0x8
+#define XR3PCI_ATR_TRSL_ADDR_HIGH	0xc
+#define XR3PCI_ATR_TRSL_PARAM		0x10
+#define XR3PCI_ATR_TABLE_OFFSET		0x20
+#define XR3PCI_ATR_MAX_TABLE_NUM	8
+
+#define XR3PCI_ATR_SRC_WIN_SIZE_SHIFT	1
+#define XR3PCI_ATR_SRC_ADDR_MASK	GENMASK(31, 12)
+#define XR3PCI_ATR_TRSL_ADDR_MASK	GENMASK(31, 12)
+#define XR3PCI_ECAM_SIZE		BIT(28)
+#define XR3PCI_ATR_TRSL_DIR		BIT(22)
+/* IDs used in the XR3PCI_ATR_TRSL_PARAM */
+#define XR3PCI_ATR_TRSLID_PCIE_MEMORY	0x0
+#define XR3PCI_ATR_TRSLID_PCIE_CONFIG	0x1
+
+#define INT_AXI_POST_ERROR		BIT(16)
+#define INT_AXI_FETCH_ERROR		BIT(17)
+#define INT_AXI_DISCARD_ERROR		BIT(18)
+#define INT_PCIE_POST_ERROR		BIT(20)
+#define INT_PCIE_FETCH_ERROR		BIT(21)
+#define INT_PCIE_DISCARD_ERROR		BIT(22)
+#define INT_ERRORS		(INT_AXI_POST_ERROR | INT_AXI_FETCH_ERROR | \
+				 INT_AXI_DISCARD_ERROR | INT_PCIE_POST_ERROR | \
+				 INT_PCIE_FETCH_ERROR | INT_PCIE_DISCARD_ERROR)
+
+#define INTA_OFFSET		24
+#define INTA			BIT(24)
+#define INTB			BIT(25)
+#define INTC			BIT(26)
+#define INTD			BIT(27)
+#define INT_MSI			BIT(28)
+#define INT_INTX_MASK		(INTA | INTB | INTC | INTD)
+#define INT_MASK		(INT_INTX_MASK | INT_MSI | INT_ERRORS)
+
+#define INT_PCI_MSI_NR		32
+
+/* system control */
+#define STG_SYSCON_K_RP_NEP			BIT(8)
+#define STG_SYSCON_AXI4_SLVL_ARFUNC_MASK	GENMASK(22, 8)
+#define STG_SYSCON_AXI4_SLVL_ARFUNC_SHIFT	8
+#define STG_SYSCON_AXI4_SLVL_AWFUNC_MASK	GENMASK(14, 0)
+#define STG_SYSCON_CLKREQ			BIT(22)
+#define STG_SYSCON_CKREF_SRC_SHIFT		18
+#define STG_SYSCON_CKREF_SRC_MASK		GENMASK(19, 18)
+
+/* MSI information */
+struct jh7110_pcie_msi {
+	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
+	struct irq_domain *msi_domain;
+	struct irq_domain *inner_domain;
+	/* Protect bitmap variable */
+	struct mutex lock;
+};
+
+struct starfive_jh7110_pcie {
+	struct platform_device	*pdev;
+	void __iomem		*reg_base;
+	void __iomem		*config_base;
+	phys_addr_t config_phyaddr;
+	struct regmap *reg_syscon;
+	struct phy *phy;
+	u32 stg_arfun;
+	u32 stg_awfun;
+	u32 stg_rp_nep;
+	u32 stg_lnksta;
+	int			irq;
+	struct irq_domain	*legacy_irq_domain;
+	struct pci_host_bridge  *bridge;
+	struct jh7110_pcie_msi	msi;
+	struct reset_control *resets;
+	struct clk_bulk_data *clks;
+	int num_clks;
+	int atr_table_num;
+	struct gpio_desc *power_gpio;
+	struct gpio_desc *reset_gpio;
+};
+
+/*
+ * StarFive PCIe port uses BAR0-BAR1 of RC's configuration space as
+ * the translation from PCI bus to native BUS.  Entire DDR region
+ * is mapped into PCIe space using these registers, so it can be
+ * reached by DMA from EP devices.  The BAR0/1 of bridge should be
+ * hidden during enumeration to avoid the sizing and resource allocation
+ * by PCIe core.
+ */
+static bool starfive_pcie_hide_rc_bar(struct pci_bus *bus, unsigned int  devfn,
+				  int offset)
+{
+	if (pci_is_root_bus(bus) && (devfn == 0)
+		&& ((offset == PCI_BASE_ADDRESS_0)
+		|| (offset == PCI_BASE_ADDRESS_1)))
+		return true;
+
+	return false;
+}
+
+void __iomem *starfive_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
+			   int where)
+{
+	struct starfive_jh7110_pcie *pcie = bus->sysdata;
+
+	return pcie->config_base + PCIE_ECAM_OFFSET(bus->number, devfn, where);
+}
+
+int starfive_pcie_config_write(struct pci_bus *bus, unsigned int devfn,
+			   int where, int size, u32 value)
+{
+	if (starfive_pcie_hide_rc_bar(bus, devfn, where))
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+
+	return pci_generic_config_write(bus, devfn, where, size, value);
+}
+
+static void starfive_pcie_handle_msi_irq(struct starfive_jh7110_pcie *pcie)
+{
+	struct jh7110_pcie_msi *msi = &pcie->msi;
+	u32 bit;
+	u32 virq;
+	unsigned long status = readl(pcie->reg_base + ISTATUS_MSI);
+
+	for_each_set_bit(bit, &status, INT_PCI_MSI_NR) {
+		/* Clear interrupts */
+		writel(1 << bit, pcie->reg_base + ISTATUS_MSI);
+		virq = irq_find_mapping(msi->inner_domain, bit);
+		if (virq) {
+			if (test_bit(bit, msi->used))
+				generic_handle_irq(virq);
+			else
+				dev_err(&pcie->pdev->dev,
+					"Unhandled MSI, MSI%d virq %d\n", bit,
+					virq);
+		} else
+			dev_err(&pcie->pdev->dev, "Unexpected MSI, MSI%d\n",
+				bit);
+	}
+	writel(INT_MSI, pcie->reg_base + ISTATUS_LOCAL);
+}
+
+static void starfive_pcie_handle_intx_irq(struct starfive_jh7110_pcie *pcie,
+				      unsigned long status)
+{
+	u32 bit;
+	u32 virq;
+
+	status >>= INTA_OFFSET;
+
+	for_each_set_bit(bit, &status, PCI_NUM_INTX) {
+		/* Clear interrupts */
+		writel(1 << (bit + INTA_OFFSET), pcie->reg_base + ISTATUS_LOCAL);
+
+		virq = irq_find_mapping(pcie->legacy_irq_domain, bit);
+		if (virq)
+			generic_handle_irq(virq);
+		else
+			dev_err(&pcie->pdev->dev,
+				"unexpected IRQ, INT%d\n", bit);
+	}
+}
+
+static void starfive_pcie_handle_errors_irq(struct starfive_jh7110_pcie *pcie, u32 status)
+{
+	if (status & INT_AXI_POST_ERROR)
+		dev_err(&pcie->pdev->dev, "AXI post error\n");
+	if (status & INT_AXI_FETCH_ERROR)
+		dev_err(&pcie->pdev->dev, "AXI fetch error\n");
+	if (status & INT_AXI_DISCARD_ERROR)
+		dev_err(&pcie->pdev->dev, "AXI discard error\n");
+	if (status & INT_PCIE_POST_ERROR)
+		dev_err(&pcie->pdev->dev, "PCIe post error\n");
+	if (status & INT_PCIE_FETCH_ERROR)
+		dev_err(&pcie->pdev->dev, "PCIe fetch error\n");
+	if (status & INT_PCIE_DISCARD_ERROR)
+		dev_err(&pcie->pdev->dev, "PCIe discard error\n");
+
+	writel(INT_ERRORS, pcie->reg_base + ISTATUS_LOCAL);
+}
+
+static void starfive_pcie_isr(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct starfive_jh7110_pcie *pcie;
+	u32 status;
+
+	chained_irq_enter(chip, desc);
+	pcie = irq_desc_get_handler_data(desc);
+
+	status = readl(pcie->reg_base + ISTATUS_LOCAL);
+	while ((status = (readl(pcie->reg_base + ISTATUS_LOCAL) & INT_MASK))) {
+		if (status & INT_INTX_MASK)
+			starfive_pcie_handle_intx_irq(pcie, status);
+
+		if (status & INT_MSI)
+			starfive_pcie_handle_msi_irq(pcie);
+
+		if (status & INT_ERRORS)
+			starfive_pcie_handle_errors_irq(pcie, status);
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+#ifdef CONFIG_PCI_MSI
+static struct irq_chip starfive_pcie_msi_irq_chip = {
+	.name = "StarFive PCIe MSI",
+	.irq_mask = pci_msi_mask_irq,
+	.irq_unmask = pci_msi_unmask_irq,
+};
+
+static struct msi_domain_info starfive_pcie_msi_domain_info = {
+	.flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
+		  MSI_FLAG_PCI_MSIX),
+	.chip = &starfive_pcie_msi_irq_chip,
+};
+#endif
+
+static void starfive_pcie_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
+{
+	struct starfive_jh7110_pcie *pcie = irq_data_get_irq_chip_data(data);
+	phys_addr_t msi_addr = readl(pcie->reg_base + IMSI_ADDR);
+
+	msg->address_lo = lower_32_bits(msi_addr);
+	msg->address_hi = upper_32_bits(msi_addr);
+	msg->data = data->hwirq;
+
+	dev_info(&pcie->pdev->dev, "msi#%d address_hi %#x address_lo %#x\n",
+		(int)data->hwirq, msg->address_hi, msg->address_lo);
+}
+
+static int starfive_pcie_msi_set_affinity(struct irq_data *irq_data,
+				 const struct cpumask *mask, bool force)
+{
+	return -EINVAL;
+}
+
+static struct irq_chip starfive_irq_chip = {
+	.name = "StarFive MSI",
+	.irq_compose_msi_msg = starfive_pcie_compose_msi_msg,
+	.irq_set_affinity = starfive_pcie_msi_set_affinity,
+};
+
+static int starfive_pcie_msi_alloc(struct irq_domain *domain, unsigned int virq,
+			  unsigned int nr_irqs, void *args)
+{
+	struct starfive_jh7110_pcie *pcie = domain->host_data;
+	struct jh7110_pcie_msi *msi = &pcie->msi;
+	int bit;
+
+	WARN_ON(nr_irqs != 1);
+	mutex_lock(&msi->lock);
+
+	bit = find_first_zero_bit(msi->used, INT_PCI_MSI_NR);
+	if (bit >= INT_PCI_MSI_NR) {
+		mutex_unlock(&msi->lock);
+		return -ENOSPC;
+	}
+
+	set_bit(bit, msi->used);
+
+	irq_domain_set_info(domain, virq, bit, &starfive_irq_chip,
+			    domain->host_data, handle_simple_irq,
+			    NULL, NULL);
+	mutex_unlock(&msi->lock);
+
+	return 0;
+}
+
+static void starfive_pcie_msi_free(struct irq_domain *domain, unsigned int virq,
+			  unsigned int nr_irqs)
+{
+	struct irq_data *data = irq_domain_get_irq_data(domain, virq);
+	struct starfive_jh7110_pcie *pcie = irq_data_get_irq_chip_data(data);
+	struct jh7110_pcie_msi *msi = &pcie->msi;
+
+	mutex_lock(&msi->lock);
+
+	if (!test_bit(data->hwirq, msi->used))
+		dev_err(&pcie->pdev->dev, "Trying to free unused MSI#%lu\n",
+			data->hwirq);
+	else
+		__clear_bit(data->hwirq, msi->used);
+
+	writel(0xffffffff, pcie->reg_base + ISTATUS_MSI);
+	mutex_unlock(&msi->lock);
+}
+
+static const struct irq_domain_ops dev_msi_domain_ops = {
+	.alloc  = starfive_pcie_msi_alloc,
+	.free   = starfive_pcie_msi_free,
+};
+
+static void starfive_pcie_msi_free_irq_domain(struct starfive_jh7110_pcie *pcie)
+{
+#ifdef CONFIG_PCI_MSI
+	struct jh7110_pcie_msi *msi = &pcie->msi;
+	u32 irq;
+	int i;
+
+	for (i = 0; i < INT_PCI_MSI_NR; i++) {
+		irq = irq_find_mapping(msi->inner_domain, i);
+		if (irq > 0)
+			irq_dispose_mapping(irq);
+	}
+
+	if (msi->msi_domain)
+		irq_domain_remove(msi->msi_domain);
+
+	if (msi->inner_domain)
+		irq_domain_remove(msi->inner_domain);
+#endif
+}
+
+static void starfive_pcie_free_irq_domain(struct starfive_jh7110_pcie *pcie)
+{
+	int i;
+	u32 irq;
+
+	/* Disable all interrupts */
+	writel(0, pcie->reg_base + IMASK_LOCAL);
+
+	if (pcie->legacy_irq_domain) {
+		for (i = 0; i < PCI_NUM_INTX; i++) {
+			irq = irq_find_mapping(pcie->legacy_irq_domain, i);
+			if (irq > 0)
+				irq_dispose_mapping(irq);
+		}
+		irq_domain_remove(pcie->legacy_irq_domain);
+	}
+
+	if (pci_msi_enabled())
+		starfive_pcie_msi_free_irq_domain(pcie);
+	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
+}
+
+static int starfive_pcie_init_msi_irq_domain(struct starfive_jh7110_pcie *pcie)
+{
+#ifdef CONFIG_PCI_MSI
+	struct fwnode_handle *fwn = of_node_to_fwnode(pcie->pdev->dev.of_node);
+	struct jh7110_pcie_msi *msi = &pcie->msi;
+
+	msi->inner_domain = irq_domain_add_linear(NULL, INT_PCI_MSI_NR,
+						  &dev_msi_domain_ops, pcie);
+	if (!msi->inner_domain) {
+		dev_err(&pcie->pdev->dev, "Failed to create dev IRQ domain\n");
+		return -ENOMEM;
+	}
+	msi->msi_domain = pci_msi_create_irq_domain(fwn, &starfive_pcie_msi_domain_info,
+						    msi->inner_domain);
+	if (!msi->msi_domain) {
+		dev_err(&pcie->pdev->dev, "Failed to create msi IRQ domain\n");
+		irq_domain_remove(msi->inner_domain);
+		return -ENOMEM;
+	}
+#endif
+	return 0;
+}
+
+static int starfive_pcie_enable_msi(struct starfive_jh7110_pcie *pcie, struct pci_bus *bus)
+{
+	struct jh7110_pcie_msi *msi = &pcie->msi;
+	u32 reg;
+
+	mutex_init(&msi->lock);
+
+	/* Enable MSI */
+	reg = readl(pcie->reg_base + IMASK_LOCAL);
+	reg |= INT_MSI;
+	writel(reg, pcie->reg_base + IMASK_LOCAL);
+	return 0;
+}
+
+static int starfive_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
+			      irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
+	irq_set_chip_data(irq, domain->host_data);
+
+	return 0;
+}
+
+static const struct irq_domain_ops intx_domain_ops = {
+	.map = starfive_pcie_intx_map,
+	.xlate = pci_irqd_intx_xlate,
+};
+
+static int starfive_pcie_init_irq_domain(struct starfive_jh7110_pcie *pcie)
+{
+	struct device *dev = &pcie->pdev->dev;
+	struct device_node *node = dev->of_node;
+	int ret;
+
+	if (pci_msi_enabled()) {
+		ret = starfive_pcie_init_msi_irq_domain(pcie);
+		if (ret != 0)
+			return -ENOMEM;
+	}
+
+	/* Setup INTx */
+	pcie->legacy_irq_domain = irq_domain_add_linear(node, PCI_NUM_INTX,
+					&intx_domain_ops, pcie);
+
+	if (!pcie->legacy_irq_domain) {
+		dev_err(dev, "Failed to get a INTx IRQ domain\n");
+		return -ENOMEM;
+	}
+
+	irq_set_chained_handler_and_data(pcie->irq, starfive_pcie_isr, pcie);
+
+	return 0;
+}
+
+static int starfive_pcie_parse_dt(struct starfive_jh7110_pcie *pcie)
+{
+	struct resource *cfg_res;
+	struct platform_device *pdev = pcie->pdev;
+	unsigned int args[4];
+
+	pcie->reg_base =
+		devm_platform_ioremap_resource_byname(pdev, "reg");
+
+	if (IS_ERR(pcie->reg_base))
+		return dev_err_probe(&pdev->dev, PTR_ERR(pcie->reg_base),
+			"Failed to map reg memory\n");
+
+	cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
+	if (!cfg_res)
+		return dev_err_probe(&pdev->dev, -ENODEV,
+			"Failed to get config memory\n");
+
+	pcie->config_base = devm_ioremap_resource(&pdev->dev, cfg_res);
+	if (IS_ERR(pcie->config_base))
+		return dev_err_probe(&pdev->dev, PTR_ERR(pcie->config_base),
+			"Failed to map config memory\n");
+
+	pcie->config_phyaddr = cfg_res->start;
+
+	pcie->phy = devm_phy_optional_get(&pdev->dev, NULL);
+	if (IS_ERR(pcie->phy))
+		return dev_err_probe(&pdev->dev, PTR_ERR(pcie->phy),
+			"Failed to get pcie phy\n");
+
+	pcie->irq = platform_get_irq(pdev, 0);
+	if (pcie->irq < 0)
+		return dev_err_probe(&pdev->dev, -EINVAL,
+			"Failed to get IRQ: %d\n", pcie->irq);
+
+	pcie->reg_syscon = syscon_regmap_lookup_by_phandle_args(pdev->dev.of_node,
+		"starfive,stg-syscon", 4, args);
+
+	if (IS_ERR(pcie->reg_syscon))
+		return dev_err_probe(&pdev->dev, PTR_ERR(pcie->reg_syscon),
+			"Failed to parse starfive,stg-syscon\n");
+
+	pcie->stg_arfun = args[0];
+	pcie->stg_awfun = args[1];
+	pcie->stg_rp_nep = args[2];
+	pcie->stg_lnksta = args[3];
+
+	/* Clear all interrupts */
+	writel(0xffffffff, pcie->reg_base + ISTATUS_LOCAL);
+	writel(INT_INTX_MASK | INT_ERRORS, pcie->reg_base + IMASK_LOCAL);
+
+	return 0;
+}
+
+static struct pci_ops starfive_pcie_ops = {
+	.map_bus	= starfive_pcie_map_bus,
+	.read           = pci_generic_config_read,
+	.write          = starfive_pcie_config_write,
+};
+
+static void starfive_pcie_set_atr_entry(struct starfive_jh7110_pcie *pcie,
+			phys_addr_t src_addr, phys_addr_t trsl_addr,
+			size_t window_size, int trsl_param)
+{
+	void __iomem *base =
+		pcie->reg_base + XR3PCI_ATR_AXI4_SLV0;
+
+	/* Support AXI4 Slave 0 Address Translation Tables 0-7. */
+	if (pcie->atr_table_num >= XR3PCI_ATR_MAX_TABLE_NUM)
+		pcie->atr_table_num = XR3PCI_ATR_MAX_TABLE_NUM - 1;
+	base +=  XR3PCI_ATR_TABLE_OFFSET * pcie->atr_table_num;
+	pcie->atr_table_num++;
+
+	/*
+	 * X3PCI_ATR_SRC_ADDR_LOW:
+	 *   - bit 0: enable entry,
+	 *   - bits 1-6: ATR window size: total size in bytes: 2^(ATR_WSIZE + 1)
+	 *   - bits 7-11: reserved
+	 *   - bits 12-31: start of source address
+	 */
+	writel((lower_32_bits(src_addr) & XR3PCI_ATR_SRC_ADDR_MASK) |
+			(fls(window_size) - 1) << XR3PCI_ATR_SRC_WIN_SIZE_SHIFT | 1,
+			base + XR3PCI_ATR_SRC_ADDR_LOW);
+	writel(upper_32_bits(src_addr), base + XR3PCI_ATR_SRC_ADDR_HIGH);
+	writel((lower_32_bits(trsl_addr) & XR3PCI_ATR_TRSL_ADDR_MASK),
+			base + XR3PCI_ATR_TRSL_ADDR_LOW);
+	writel(upper_32_bits(trsl_addr), base + XR3PCI_ATR_TRSL_ADDR_HIGH);
+	writel(trsl_param, base + XR3PCI_ATR_TRSL_PARAM);
+
+	dev_info(&pcie->pdev->dev, "ATR entry: 0x%010llx %s 0x%010llx [0x%010llx] (param: 0x%06x)\n",
+	       src_addr, (trsl_param & XR3PCI_ATR_TRSL_DIR) ? "<-" : "->",
+	       trsl_addr, (u64)window_size, trsl_param);
+}
+
+static int starfive_pcie_setup_windows(struct starfive_jh7110_pcie *pcie)
+{
+	struct pci_host_bridge *bridge = pcie->bridge;
+	struct resource_entry *entry;
+	u64 pci_addr;
+
+	resource_list_for_each_entry(entry, &bridge->windows) {
+		if (resource_type(entry->res) == IORESOURCE_MEM) {
+			pci_addr = entry->res->start - entry->offset;
+			starfive_pcie_set_atr_entry(pcie,
+						entry->res->start, pci_addr,
+						resource_size(entry->res),
+						XR3PCI_ATR_TRSLID_PCIE_MEMORY);
+		}
+	}
+
+	return 0;
+}
+
+static int starfive_pcie_clk_rst_init(struct starfive_jh7110_pcie *pcie)
+{
+	int ret;
+	struct device *dev = &pcie->pdev->dev;
+
+	pcie->num_clks = devm_clk_bulk_get_all(dev, &pcie->clks);
+	if (pcie->num_clks < 0)
+		return dev_err_probe(dev, -ENODEV,
+			"Failed to get pcie clocks\n");
+
+	ret = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks);
+	if (ret)
+		return dev_err_probe(&pcie->pdev->dev, ret,
+			"Failed to enable clocks\n");
+
+	pcie->resets = devm_reset_control_array_get_exclusive(dev);
+	if (IS_ERR(pcie->resets)) {
+		clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
+		return dev_err_probe(dev, PTR_ERR(pcie->resets),
+			"Failed to get pcie resets");
+	}
+
+	return reset_control_deassert(pcie->resets);
+}
+
+static void starfive_pcie_clk_rst_deinit(struct starfive_jh7110_pcie *pcie)
+{
+	reset_control_assert(pcie->resets);
+	clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
+}
+
+int starfive_pcie_gpio_init(struct starfive_jh7110_pcie *pcie)
+{
+	struct device *dev = &pcie->pdev->dev;
+
+	pcie->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR_OR_NULL(pcie->reset_gpio)) {
+		dev_warn(dev, "Failed to get reset-gpio.\n");
+		return -EINVAL;
+	}
+
+	pcie->power_gpio = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW);
+	if (IS_ERR_OR_NULL(pcie->power_gpio))
+		pcie->power_gpio = NULL;
+
+	return 0;
+}
+
+static void starfive_pcie_hw_init(struct starfive_jh7110_pcie *pcie)
+{
+	unsigned int value;
+	int i;
+
+	if (pcie->power_gpio)
+		gpiod_set_value_cansleep(pcie->power_gpio, 1);
+
+	if (pcie->reset_gpio)
+		gpiod_set_value_cansleep(pcie->reset_gpio, 1);
+
+	/* Disable physical functions except #0 */
+	for (i = 1; i < PCIE_FUNC_NUM; i++) {
+		regmap_update_bits(pcie->reg_syscon,
+				pcie->stg_arfun,
+				STG_SYSCON_AXI4_SLVL_ARFUNC_MASK,
+				(i << PHY_FUNC_SHIFT) <<
+				STG_SYSCON_AXI4_SLVL_ARFUNC_SHIFT);
+		regmap_update_bits(pcie->reg_syscon,
+				pcie->stg_awfun,
+				STG_SYSCON_AXI4_SLVL_AWFUNC_MASK,
+				i << PHY_FUNC_SHIFT);
+
+		value = readl(pcie->reg_base + PCI_MISC);
+		value |= PHY_FUNCTION_DIS;
+		writel(value, pcie->reg_base + PCI_MISC);
+	}
+
+
+	regmap_update_bits(pcie->reg_syscon,
+				pcie->stg_arfun,
+				STG_SYSCON_AXI4_SLVL_ARFUNC_MASK,
+				0);
+	regmap_update_bits(pcie->reg_syscon,
+				pcie->stg_awfun,
+				STG_SYSCON_AXI4_SLVL_AWFUNC_MASK,
+				0);
+
+	/* Enable root port */
+	value = readl(pcie->reg_base + GEN_SETTINGS);
+	value |= RP_ENABLE;
+	writel(value, pcie->reg_base + GEN_SETTINGS);
+
+	/* PCIe PCI Standard Configuration Identification Settings. */
+	value = (PCI_CLASS_BRIDGE_PCI << IDS_CLASS_CODE_SHIFT);
+	writel(value, pcie->reg_base + PCIE_PCI_IDS);
+
+	/*
+	 * The LTR message forwarding of PCIe Message Reception was set by core
+	 * as default, but the forward id & addr are also need to be reset.
+	 * If we do not disable LTR message forwarding here, or set a legal
+	 * forwarding address, the kernel will get stuck after this driver probe.
+	 * To workaround, disable the LTR message forwarding support on
+	 * PCIe Message Reception.
+	 */
+	value = readl(pcie->reg_base + PMSG_SUPPORT_RX);
+	value &= ~PMSG_LTR_SUPPORT;
+	writel(value, pcie->reg_base + PMSG_SUPPORT_RX);
+
+	/* Prefetchable memory window 64-bit addressing support */
+	value = readl(pcie->reg_base + PCIE_WINROM);
+	value |= PREF_MEM_WIN_64_SUPPORT;
+	writel(value, pcie->reg_base + PCIE_WINROM);
+
+	/*
+	 * As the two host bridges in JH7110 soc have the same default
+	 * address translation table, this cause the second root port can't
+	 * access it's host bridge config space correctly.
+	 * To workaround, config the ATR of host bridge config space by SW.
+	 */
+	starfive_pcie_set_atr_entry(pcie,
+			pcie->config_phyaddr, 0,
+			XR3PCI_ECAM_SIZE,
+			XR3PCI_ATR_TRSLID_PCIE_CONFIG);
+
+	starfive_pcie_setup_windows(pcie);
+
+	/* Ensure that PERST has been asserted for at least 100 ms */
+	msleep(300);
+	if (pcie->reset_gpio)
+		gpiod_set_value_cansleep(pcie->reset_gpio, 0);
+}
+
+static bool starfive_pcie_is_link_up(struct starfive_jh7110_pcie *pcie)
+{
+	struct device *dev = &pcie->pdev->dev;
+	int ret;
+	u32 stg_reg_val;
+
+	/* 100ms timeout value should be enough for Gen1/2 training */
+	ret = regmap_read_poll_timeout(pcie->reg_syscon,
+					pcie->stg_lnksta,
+					stg_reg_val,
+					stg_reg_val & DATA_LINK_ACTIVE,
+					10 * 1000, 100 * 1000);
+
+	/* If the link is down (no device in slot), then exit. */
+	if (ret == -ETIMEDOUT) {
+		dev_info(dev, "Port link down, exit.\n");
+		return false;
+	} else if (ret == 0) {
+		dev_info(dev, "Port link up.\n");
+		return true;
+	}
+
+	dev_warn(dev, "Read stg_linksta failed.\n");
+
+	return false;
+}
+
+static int starfive_pcie_enable_phy(struct device *dev,
+		struct starfive_jh7110_pcie *pcie)
+{
+	int ret;
+
+	if (!pcie->phy)
+		return 0;
+
+	ret = phy_init(pcie->phy);
+	if (ret)
+		return dev_err_probe(dev, ret,
+			"failed to initialize pcie phy\n");
+
+	ret = phy_set_mode(pcie->phy, PHY_MODE_PCIE);
+	if (ret) {
+		ret = dev_err_probe(dev, ret,
+			"failed to set pcie mode\n");
+		goto err_phy_on;
+	}
+
+	ret = phy_power_on(pcie->phy);
+	if (ret) {
+		ret = dev_err_probe(dev, ret, "failed to power on pcie phy\n");
+		goto err_phy_on;
+	}
+
+	return 0;
+
+err_phy_on:
+	phy_exit(pcie->phy);
+	return ret;
+}
+
+static void starfive_pcie_disable_phy(struct starfive_jh7110_pcie *pcie)
+{
+	phy_power_off(pcie->phy);
+	phy_exit(pcie->phy);
+}
+
+static int starfive_pcie_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct starfive_jh7110_pcie *pcie;
+	struct pci_bus *bus;
+	struct pci_host_bridge *bridge;
+	int ret;
+
+	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	pcie->pdev = pdev;
+	pcie->atr_table_num = 0;
+
+	ret = starfive_pcie_parse_dt(pcie);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, pcie);
+
+	ret = starfive_pcie_gpio_init(pcie);
+	if (ret)
+		return ret;
+
+	regmap_update_bits(pcie->reg_syscon,
+				pcie->stg_rp_nep,
+				STG_SYSCON_K_RP_NEP,
+				STG_SYSCON_K_RP_NEP);
+
+	regmap_update_bits(pcie->reg_syscon,
+				pcie->stg_awfun,
+				STG_SYSCON_CKREF_SRC_MASK,
+				2 << STG_SYSCON_CKREF_SRC_SHIFT);
+
+	regmap_update_bits(pcie->reg_syscon,
+				pcie->stg_awfun,
+				STG_SYSCON_CLKREQ,
+				STG_SYSCON_CLKREQ);
+
+	ret = starfive_pcie_clk_rst_init(pcie);
+	if (ret)
+		return ret;
+
+	ret = starfive_pcie_init_irq_domain(pcie);
+	if (ret)
+		return ret;
+
+	bridge = devm_pci_alloc_host_bridge(dev, 0);
+	if (!bridge)
+		return -ENOMEM;
+
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
+
+	/* Set default bus ops */
+	bridge->ops = &starfive_pcie_ops;
+	bridge->sysdata = pcie;
+	pcie->bridge = bridge;
+
+	starfive_pcie_hw_init(pcie);
+
+	if (starfive_pcie_is_link_up(pcie) == false)
+		goto release;
+
+	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+		ret = starfive_pcie_enable_msi(pcie, bus);
+		if (ret < 0) {
+			dev_err(dev, "Failed to enable MSI support: %d\n", ret);
+			goto release;
+		}
+	}
+
+	ret = starfive_pcie_enable_phy(dev, pcie);
+	if (ret)
+		goto release;
+
+	ret = pci_host_probe(bridge);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "Failed to pci host probe: %d\n", ret);
+		goto err_phy_on;
+	}
+
+	return ret;
+
+err_phy_on:
+	starfive_pcie_disable_phy(pcie);
+release:
+	if (pcie->power_gpio)
+		gpiod_set_value_cansleep(pcie->power_gpio, 0);
+
+	starfive_pcie_clk_rst_deinit(pcie);
+
+	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+
+	pci_free_host_bridge(pcie->bridge);
+	platform_set_drvdata(pdev, NULL);
+
+	return ret;
+}
+
+static int starfive_pcie_remove(struct platform_device *pdev)
+{
+	struct starfive_jh7110_pcie *pcie = platform_get_drvdata(pdev);
+
+	starfive_pcie_disable_phy(pcie);
+	if (pcie->power_gpio)
+		gpiod_set_value_cansleep(pcie->power_gpio, 0);
+	starfive_pcie_free_irq_domain(pcie);
+	starfive_pcie_clk_rst_deinit(pcie);
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int __maybe_unused starfive_pcie_suspend_noirq(struct device *dev)
+{
+	struct starfive_jh7110_pcie *pcie = dev_get_drvdata(dev);
+
+	if (!pcie)
+		return 0;
+
+	starfive_pcie_disable_phy(pcie);
+	clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
+
+	return 0;
+}
+
+static int __maybe_unused starfive_pcie_resume_noirq(struct device *dev)
+{
+	struct starfive_jh7110_pcie *pcie = dev_get_drvdata(dev);
+	int ret;
+
+	if (!pcie)
+		return 0;
+
+	ret = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks);
+	if (ret)
+		return dev_err_probe(dev, ret,
+			"Failed to enable clocks\n");
+
+	ret = starfive_pcie_enable_phy(dev, pcie);
+	if (ret)
+		clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
+
+	return ret;
+}
+
+static const struct dev_pm_ops starfive_pcie_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(starfive_pcie_suspend_noirq,
+				      starfive_pcie_resume_noirq)
+};
+#endif
+
+static const struct of_device_id starfive_pcie_of_match[] = {
+	{ .compatible = "starfive,jh7110-pcie"},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, starfive_pcie_of_match);
+
+static struct platform_driver starfive_pcie_driver = {
+	.driver = {
+		.name = "pcie-starfive",
+		.of_match_table = of_match_ptr(starfive_pcie_of_match),
+#ifdef CONFIG_PM_SLEEP
+		.pm = &starfive_pcie_pm_ops,
+#endif
+	},
+	.probe = starfive_pcie_probe,
+	.remove = starfive_pcie_remove,
+};
+module_platform_driver(starfive_pcie_driver);
+
+MODULE_DESCRIPTION("StarFive JH7110 PCIe host driver");
+MODULE_AUTHOR("Mason Huo <mason.huo@starfivetech.com>");
+MODULE_AUTHOR("Kevin Xie <kevin.xie@starfivetech.com>");
+MODULE_AUTHOR("Minda Chen <minda.chen@starfivetech.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

[PATCH v1 3/3] riscv: dts: starfive: add PCIe dts configuration for JH7110

[Minda Chen]

The PCIe is a PCIe2, single lane PCIe compliant controller.

Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
---
 .../jh7110-starfive-visionfive-2.dtsi         | 58 ++++++++++++
 arch/riscv/boot/dts/starfive/jh7110.dtsi      | 88 +++++++++++++++++++
 2 files changed, 146 insertions(+)

diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
index cf0a66faf5d3..4552919e69b0 100644
--- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
@@ -191,6 +191,50 @@
 		};
 	};
 
+	pcie0_wake_default: pcie0_wake_default {
+		wake-pins {
+			pinmux = <GPIOMUX(32, GPOUT_HIGH, GPOEN_ENABLE, GPI_NONE)>;
+			bias-disable;
+			drive-strength = <2>;
+			input-enable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+	};
+
+	pcie0_clkreq_default: pcie0_clkreq_default {
+		clkreq-pins {
+			bias-disable;
+			pinmux = <GPIOMUX(27, GPOUT_HIGH, GPOEN_ENABLE, GPI_NONE)>;
+			drive-strength = <2>;
+			input-enable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+	};
+
+	pcie1_wake_default: pcie1_wake_default {
+		wake-pins {
+			bias-disable;
+			pinmux = <GPIOMUX(21, GPOUT_HIGH, GPOEN_ENABLE, GPI_NONE)>;
+			drive-strength = <2>;
+			input-enable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+	};
+
+	pcie1_clkreq_default: pcie1_clkreq_default {
+		clkreq-pins {
+			bias-disable;
+			pinmux = <GPIOMUX(29, GPOUT_HIGH, GPOEN_ENABLE, GPI_NONE)>;
+			drive-strength = <2>;
+			input-enable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+	};
+
 	uart0_pins: uart0-0 {
 		tx-pins {
 			pinmux = <GPIOMUX(5, GPOUT_SYS_UART0_TX,
@@ -228,3 +272,17 @@
 	dr_mode = "peripheral";
 	status = "okay";
 };
+
+&pcie0 {
+	pinctrl-names = "default";
+	reset-gpios = <&sysgpio 26 GPIO_ACTIVE_LOW>;
+	phys = <&pciephy0>;
+	status = "okay";
+};
+
+&pcie1 {
+	pinctrl-names = "default";
+	reset-gpios = <&sysgpio 28 GPIO_ACTIVE_LOW>;
+	phys = <&pciephy1>;
+	status = "okay";
+};
diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
index 2f67196ffac0..c309ec550ba7 100644
--- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
@@ -642,5 +642,93 @@
 			#reset-cells = <1>;
 			power-domains = <&pwrc JH7110_PD_VOUT>;
 		};
+
+		pcie0: pcie@2B000000 {
+			compatible = "starfive,jh7110-pcie";
+			#address-cells = <3>;
+			#size-cells = <2>;
+			#interrupt-cells = <1>;
+			reg = <0x0 0x2B000000 0x0 0x1000000
+			       0x9 0x40000000 0x0 0x10000000>;
+			reg-names = "reg", "config";
+			device_type = "pci";
+			starfive,stg-syscon = <&stg_syscon 0xc0 0xc4 0x130 0x1b8>;
+			bus-range = <0x0 0xff>;
+			ranges = <0x82000000  0x0 0x30000000  0x0 0x30000000 0x0 0x08000000>,
+				 <0xc3000000  0x9 0x00000000  0x9 0x00000000 0x0 0x40000000>;
+			interrupts = <56>;
+			interrupt-parent = <&plic>;
+			interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+			interrupt-map = <0x0 0x0 0x0 0x1 &pcie_intc0 0x1>,
+					<0x0 0x0 0x0 0x2 &pcie_intc0 0x2>,
+					<0x0 0x0 0x0 0x3 &pcie_intc0 0x3>,
+					<0x0 0x0 0x0 0x4 &pcie_intc0 0x4>;
+			msi-parent = <&pcie0>;
+			msi-controller;
+			clocks = <&syscrg JH7110_SYSCLK_NOC_BUS_STG_AXI>,
+				 <&stgcrg JH7110_STGCLK_PCIE0_TL>,
+				 <&stgcrg JH7110_STGCLK_PCIE0_AXI_MST0>,
+				 <&stgcrg JH7110_STGCLK_PCIE0_APB>;
+			clock-names = "noc", "tl", "axi_mst0", "apb";
+			resets = <&stgcrg JH7110_STGRST_PCIE0_AXI_MST0>,
+				 <&stgcrg JH7110_STGRST_PCIE0_AXI_SLV0>,
+				 <&stgcrg JH7110_STGRST_PCIE0_AXI_SLV>,
+				 <&stgcrg JH7110_STGRST_PCIE0_BRG>,
+				 <&stgcrg JH7110_STGRST_PCIE0_CORE>,
+				 <&stgcrg JH7110_STGRST_PCIE0_APB>;
+			reset-names = "mst0", "slv0", "slv", "brg",
+				      "core", "apb";
+			status = "disabled";
+
+			pcie_intc0: interrupt-controller {
+				#address-cells = <0>;
+				#interrupt-cells = <1>;
+				interrupt-controller;
+			};
+		};
+
+		pcie1: pcie@2C000000 {
+			compatible = "starfive,jh7110-pcie";
+			#address-cells = <3>;
+			#size-cells = <2>;
+			#interrupt-cells = <1>;
+			reg = <0x0 0x2C000000 0x0 0x1000000
+			       0x9 0xc0000000 0x0 0x10000000>;
+			reg-names = "reg", "config";
+			device_type = "pci";
+			starfive,stg-syscon = <&stg_syscon 0x270 0x274 0x2e0 0x368>;
+			bus-range = <0x0 0xff>;
+			ranges = <0x82000000  0x0 0x38000000  0x0 0x38000000 0x0 0x08000000>,
+				 <0xc3000000  0x9 0x80000000  0x9 0x80000000 0x0 0x40000000>;
+			interrupts = <57>;
+			interrupt-parent = <&plic>;
+			interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+			interrupt-map = <0x0 0x0 0x0 0x1 &pcie_intc1 0x1>,
+					<0x0 0x0 0x0 0x2 &pcie_intc1 0x2>,
+					<0x0 0x0 0x0 0x3 &pcie_intc1 0x3>,
+					<0x0 0x0 0x0 0x4 &pcie_intc1 0x4>;
+			msi-parent = <&pcie1>;
+			msi-controller;
+			clocks = <&syscrg JH7110_SYSCLK_NOC_BUS_STG_AXI>,
+				 <&stgcrg JH7110_STGCLK_PCIE1_TL>,
+				 <&stgcrg JH7110_STGCLK_PCIE1_AXI_MST0>,
+				 <&stgcrg JH7110_STGCLK_PCIE1_APB>;
+			clock-names = "noc", "tl", "axi_mst0", "apb";
+			resets = <&stgcrg JH7110_STGRST_PCIE1_AXI_MST0>,
+				 <&stgcrg JH7110_STGRST_PCIE1_AXI_SLV0>,
+				 <&stgcrg JH7110_STGRST_PCIE1_AXI_SLV>,
+				 <&stgcrg JH7110_STGRST_PCIE1_BRG>,
+				 <&stgcrg JH7110_STGRST_PCIE1_CORE>,
+				 <&stgcrg JH7110_STGRST_PCIE1_APB>;
+			reset-names = "mst0", "slv0", "slv", "brg",
+				      "core", "apb";
+			status = "disabled";
+
+			pcie_intc1: interrupt-controller {
+				#address-cells = <0>;
+				#interrupt-cells = <1>;
+				interrupt-controller;
+			};
+		};
 	};
 };
-- 
2.17.1

Re: [PATCH v1 3/3] riscv: dts: starfive: add PCIe dts configuration for JH7110

[Krzysztof Kozlowski]

On 06/04/2023 13:11, Minda Chen wrote:
> The PCIe is a PCIe2, single lane PCIe compliant controller.
> 
> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
> ---
>  .../jh7110-starfive-visionfive-2.dtsi         | 58 ++++++++++++
>  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 88 +++++++++++++++++++
>  2 files changed, 146 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> index cf0a66faf5d3..4552919e69b0 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> @@ -191,6 +191,50 @@
>  		};
>  	};
>  
> +	pcie0_wake_default: pcie0_wake_default {

No underscores in node names. Test your patches against bindings before
sending.


>  	uart0_pins: uart0-0 {
>  		tx-pins {
>  			pinmux = <GPIOMUX(5, GPOUT_SYS_UART0_TX,
> @@ -228,3 +272,17 @@
>  	dr_mode = "peripheral";
>  	status = "okay";
>  };
> +
> +&pcie0 {
> +	pinctrl-names = "default";
> +	reset-gpios = <&sysgpio 26 GPIO_ACTIVE_LOW>;
> +	phys = <&pciephy0>;
> +	status = "okay";
> +};
> +
> +&pcie1 {
> +	pinctrl-names = "default";
> +	reset-gpios = <&sysgpio 28 GPIO_ACTIVE_LOW>;
> +	phys = <&pciephy1>;
> +	status = "okay";
> +};
> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> index 2f67196ffac0..c309ec550ba7 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> @@ -642,5 +642,93 @@
>  			#reset-cells = <1>;
>  			power-domains = <&pwrc JH7110_PD_VOUT>;
>  		};
> +
> +		pcie0: pcie@2B000000 {

Lower case hex.

> +			compatible = "starfive,jh7110-pcie";
> +			#address-cells = <3>;
> +			#size-cells = <2>;
> +			#interrupt-cells = <1>;

Why reg is not second property?

> +			reg = <0x0 0x2B000000 0x0 0x1000000
> +			       0x9 0x40000000 0x0 0x10000000>;
> +			reg-names = "reg", "config";
> +			device_type = "pci";
> +			starfive,stg-syscon = <&stg_syscon 0xc0 0xc4 0x130 0x1b8>;
> +			bus-range = <0x0 0xff>;
> +			ranges = <0x82000000  0x0 0x30000000  0x0 0x30000000 0x0 0x08000000>,
> +				 <0xc3000000  0x9 0x00000000  0x9 0x00000000 0x0 0x40000000>;
> +			interrupts = <56>;

Your binding requires cells, so I am pretty sure you did not test what
you wrote.



Best regards,
Krzysztof

Re: [PATCH v1 0/3] Add JH7110 PCIe driver support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1992 bytes --]

+CC Daire

Hey Minda,

On Thu, Apr 06, 2023 at 07:11:39PM +0800, Minda Chen wrote:
> This patchset adds PCIe driver for the StarFive JH7110 SoC.
> The patch has been tested on the VisionFive 2 board. The test
> devices include M.2 NVMe SSD and Realtek 8169 Ethernet adapter.

I was talking with Daire last week about some changes he's working on
for the microchip driver, and we seemed to recall an off-list email
sent to Daire & Bjorn about extracting the common PLDA bits from the
pcie-microchip-host driver to be used with an (at that point)
unreleased SoC. Perhaps Bjorn has this in his mailbox somewhere still,
our corporate mail policy scrubs things from over a year ago & I could
not find it.

I realised that that may actually have been StarFive, and the driver on
your GitHub [1] certainly felt very familiar to Daire (he said it was
very similar to his earlier revisions of his driver).

I've not looked at a diff between this and the version you ship on
GitHub, but first a quick inspection it mostly just looks like you
did s/plda/sifive/ on the file.

I'm obviously not a PCI maintainer, but if there are common bits between
the two drivers, extracting common bits seems like a good idea to me...

https://github.com/starfive-tech/linux/blob/JH7110_VisionFive2_devel/drivers/pci/controller/pcie-plda.c
> 
> This patchset should be applied after the patchset [1], [2], [3] and[4]:
> [1] https://patchwork.kernel.org/project/linux-riscv/cover/20230314124404.117592-1-xingyu.wu@starfivetech.com/
> [2] https://lore.kernel.org/all/20230315055813.94740-1-william.qiu@starfivetech.com/
> [3] https://patchwork.kernel.org/project/linux-phy/cover/20230315100421.133428-1-changhuang.liang@starfivetech.com/
> [4] https://patchwork.kernel.org/project/linux-usb/cover/20230406015216.27034-1-minda.chen@starfivetech.com/

How many of the dependencies here are compiletime for the driver & how
many of them are just for the dts patch?

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 0/3] Add JH7110 PCIe driver support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2214 bytes --]

Gah, I never actually CCed Daire. Apologies for the additional email.

On Thu, Apr 06, 2023 at 12:47:41PM +0100, Conor Dooley wrote:
> +CC Daire
> 
> Hey Minda,
> 
> On Thu, Apr 06, 2023 at 07:11:39PM +0800, Minda Chen wrote:
> > This patchset adds PCIe driver for the StarFive JH7110 SoC.
> > The patch has been tested on the VisionFive 2 board. The test
> > devices include M.2 NVMe SSD and Realtek 8169 Ethernet adapter.
> 
> I was talking with Daire last week about some changes he's working on
> for the microchip driver, and we seemed to recall an off-list email
> sent to Daire & Bjorn about extracting the common PLDA bits from the
> pcie-microchip-host driver to be used with an (at that point)
> unreleased SoC. Perhaps Bjorn has this in his mailbox somewhere still,
> our corporate mail policy scrubs things from over a year ago & I could
> not find it.
> 
> I realised that that may actually have been StarFive, and the driver on
> your GitHub [1] certainly felt very familiar to Daire (he said it was
> very similar to his earlier revisions of his driver).
> 
> I've not looked at a diff between this and the version you ship on
> GitHub, but first a quick inspection it mostly just looks like you
> did s/plda/sifive/ on the file.
> 
> I'm obviously not a PCI maintainer, but if there are common bits between
> the two drivers, extracting common bits seems like a good idea to me...
> 
> https://github.com/starfive-tech/linux/blob/JH7110_VisionFive2_devel/drivers/pci/controller/pcie-plda.c
> > 
> > This patchset should be applied after the patchset [1], [2], [3] and[4]:
> > [1] https://patchwork.kernel.org/project/linux-riscv/cover/20230314124404.117592-1-xingyu.wu@starfivetech.com/
> > [2] https://lore.kernel.org/all/20230315055813.94740-1-william.qiu@starfivetech.com/
> > [3] https://patchwork.kernel.org/project/linux-phy/cover/20230315100421.133428-1-changhuang.liang@starfivetech.com/
> > [4] https://patchwork.kernel.org/project/linux-usb/cover/20230406015216.27034-1-minda.chen@starfivetech.com/
> 
> How many of the dependencies here are compiletime for the driver & how
> many of them are just for the dts patch?
> 
> Cheers,
> Conor.



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 0/3] Add JH7110 PCIe driver support

[Minda Chen]

On 2023/4/6 19:54, Conor Dooley wrote:
> Gah, I never actually CCed Daire. Apologies for the additional email.
> 
> On Thu, Apr 06, 2023 at 12:47:41PM +0100, Conor Dooley wrote:
>> +CC Daire
>> 
>> Hey Minda,
>> 
>> On Thu, Apr 06, 2023 at 07:11:39PM +0800, Minda Chen wrote:
>> > This patchset adds PCIe driver for the StarFive JH7110 SoC.
>> > The patch has been tested on the VisionFive 2 board. The test
>> > devices include M.2 NVMe SSD and Realtek 8169 Ethernet adapter.
>> 
>> I was talking with Daire last week about some changes he's working on
>> for the microchip driver, and we seemed to recall an off-list email
>> sent to Daire & Bjorn about extracting the common PLDA bits from the
>> pcie-microchip-host driver to be used with an (at that point)
>> unreleased SoC. Perhaps Bjorn has this in his mailbox somewhere still,
>> our corporate mail policy scrubs things from over a year ago & I could
>> not find it.
>>
>> I realised that that may actually have been StarFive, and the driver on
>> your GitHub [1] certainly felt very familiar to Daire (he said it was
>> very similar to his earlier revisions of his driver).
>> 
>> I've not looked at a diff between this and the version you ship on
>> GitHub, but first a quick inspection it mostly just looks like you
>> did s/plda/sifive/ on the file.
>> 
>> I'm obviously not a PCI maintainer, but if there are common bits between
>> the two drivers, extracting common bits seems like a good idea to me...
>Thanks. It is pleasure to using same common codes. Does common bits changes
will upstream soon?
And I see there are many difference between pcie-microchip-host and our codes.
>> https://github.com/starfive-tech/linux/blob/JH7110_VisionFive2_devel/drivers/pci/controller/pcie-plda.c
>> > 
>> > This patchset should be applied after the patchset [1], [2], [3] and[4]:
>> > [1] https://patchwork.kernel.org/project/linux-riscv/cover/20230314124404.117592-1-xingyu.wu@starfivetech.com/
>> > [2] https://lore.kernel.org/all/20230315055813.94740-1-william.qiu@starfivetech.com/
>> > [3] https://patchwork.kernel.org/project/linux-phy/cover/20230315100421.133428-1-changhuang.liang@starfivetech.com/
>> > [4] https://patchwork.kernel.org/project/linux-usb/cover/20230406015216.27034-1-minda.chen@starfivetech.com/
>> 
>> How many of the dependencies here are compiletime for the driver & how
>> many of them are just for the dts patch?
>> 
PCIe rely on stg clock in [1], rely on stg syscon in [2].
Patch [2] is accepted now. Maybe I will delete this.
both [3] and [4] is PHY dependency. 
>> Cheers,
>> Conor.
> 
> 

Re: [PATCH v1 0/3] Add JH7110 PCIe driver support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3292 bytes --]

Hey Minda,

On Fri, Apr 07, 2023 at 10:32:51AM +0800, Minda Chen wrote:
> On 2023/4/6 19:54, Conor Dooley wrote:
> > On Thu, Apr 06, 2023 at 12:47:41PM +0100, Conor Dooley wrote:
> >> On Thu, Apr 06, 2023 at 07:11:39PM +0800, Minda Chen wrote:
> >> > This patchset adds PCIe driver for the StarFive JH7110 SoC.
> >> > The patch has been tested on the VisionFive 2 board. The test
> >> > devices include M.2 NVMe SSD and Realtek 8169 Ethernet adapter.
> >> 
> >> I was talking with Daire last week about some changes he's working on
> >> for the microchip driver, and we seemed to recall an off-list email
> >> sent to Daire & Bjorn about extracting the common PLDA bits from the
> >> pcie-microchip-host driver to be used with an (at that point)
> >> unreleased SoC. Perhaps Bjorn has this in his mailbox somewhere still,
> >> our corporate mail policy scrubs things from over a year ago & I could
> >> not find it.
> >>
> >> I realised that that may actually have been StarFive, and the driver on
> >> your GitHub [1] certainly felt very familiar to Daire (he said it was
> >> very similar to his earlier revisions of his driver).
> >> 
> >> I've not looked at a diff between this and the version you ship on
> >> GitHub, but first a quick inspection it mostly just looks like you
> >> did s/plda/sifive/ on the file.
> >> 
> >> I'm obviously not a PCI maintainer, but if there are common bits between
> >> the two drivers, extracting common bits seems like a good idea to me...

> Thanks. It is pleasure to using same common codes. Does common bits changes
> will upstream soon?

I don't quite get what you mean. We've got some changes that are in
progress here:
https://lore.kernel.org/linux-pci/20230111125323.1911373-1-daire.mcnamara@microchip.com/
We've been quiet there for a while, but Daire's back looking into Robin's
comments in there about the range parsing/window setup at the moment.

I'm not sure if that's what you mean though, since you said "common
bits" & Daire was doing that work in a world where there was no jh7110
driver in the mix.
Extracting common bits would be part of the process of adding a new
driver, as I don't think there's any real reason to do so without
another in-tree user.

> And I see there are many difference between pcie-microchip-host and our codes.

Right. I'd expect there to be a fair difference between our integrations
of the IP, and therefore there'll be a bunch of non-shareable bits.

You need the stg,syscon & phy bits, and the clock/reset handling is
clearly different too.

> >> https://github.com/starfive-tech/linux/blob/JH7110_VisionFive2_devel/drivers/pci/controller/pcie-plda.c

I had a bit of a read through this again today with Daire to check what
the differences actually are and it *looked* like the main,
non-implementation related, differences were the extra "event" domain
that was created to simplify the driver & the bottom half interrupt
handling.
That all came out of the review process, so it's likely that some of the
same requests would be made of you by the PCI maintainers anyway.

As an aside, you should probably run checkpatch --strict on this
submission, there's a rake of coding style "issues" in the new code
you've added.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 0/3] Add JH7110 PCIe driver support

[Minda Chen]

On 2023/4/7 17:57, Conor Dooley wrote:
> Hey Minda,
> 
> On Fri, Apr 07, 2023 at 10:32:51AM +0800, Minda Chen wrote:
>> On 2023/4/6 19:54, Conor Dooley wrote:
>> > On Thu, Apr 06, 2023 at 12:47:41PM +0100, Conor Dooley wrote:
>> >> On Thu, Apr 06, 2023 at 07:11:39PM +0800, Minda Chen wrote:
>> >> > This patchset adds PCIe driver for the StarFive JH7110 SoC.
>> >> > The patch has been tested on the VisionFive 2 board. The test
>> >> > devices include M.2 NVMe SSD and Realtek 8169 Ethernet adapter.
>> >> 
>> >> I was talking with Daire last week about some changes he's working on
>> >> for the microchip driver, and we seemed to recall an off-list email
>> >> sent to Daire & Bjorn about extracting the common PLDA bits from the
>> >> pcie-microchip-host driver to be used with an (at that point)
>> >> unreleased SoC. Perhaps Bjorn has this in his mailbox somewhere still,
>> >> our corporate mail policy scrubs things from over a year ago & I could
>> >> not find it.
>> >>
>> >> I realised that that may actually have been StarFive, and the driver on
>> >> your GitHub [1] certainly felt very familiar to Daire (he said it was
>> >> very similar to his earlier revisions of his driver).
>> >> 
>> >> I've not looked at a diff between this and the version you ship on
>> >> GitHub, but first a quick inspection it mostly just looks like you
>> >> did s/plda/sifive/ on the file.
>> >> 
>> >> I'm obviously not a PCI maintainer, but if there are common bits between
>> >> the two drivers, extracting common bits seems like a good idea to me...
> 
>> Thanks. It is pleasure to using same common codes. Does common bits changes
>> will upstream soon?
> 
> I don't quite get what you mean. We've got some changes that are in
> progress here:
> https://lore.kernel.org/linux-pci/20230111125323.1911373-1-daire.mcnamara@microchip.com/
> We've been quiet there for a while, but Daire's back looking into Robin's
> comments in there about the range parsing/window setup at the moment.
> 
> I'm not sure if that's what you mean though, since you said "common
> bits" & Daire was doing that work in a world where there was no jh7110
> driver in the mix.
> Extracting common bits would be part of the process of adding a new
> driver, as I don't think there's any real reason to do so without
> another in-tree user.
> 
OK, I know extracting common bits is microchip new PCIe driver codes changed.
Just ignore my previous comments.
Maybe I will try to restructuring the driver code according to corporate e-mail which has been sent one year ago.
>> And I see there are many difference between pcie-microchip-host and our codes.
> 
> Right. I'd expect there to be a fair difference between our integrations
> of the IP, and therefore there'll be a bunch of non-shareable bits.
> 
> You need the stg,syscon & phy bits, and the clock/reset handling is
> clearly different too.
> 
>> >> https://github.com/starfive-tech/linux/blob/JH7110_VisionFive2_devel/drivers/pci/controller/pcie-plda.c
> 
> I had a bit of a read through this again today with Daire to check what
> the differences actually are and it *looked* like the main,
> non-implementation related, differences were the extra "event" domain
> that was created to simplify the driver & the bottom half interrupt
> handling.
> That all came out of the review process, so it's likely that some of the
> same requests would be made of you by the PCI maintainers anyway.
> 
Thanks. I will check it and change my codes.
> As an aside, you should probably run checkpatch --strict on this
> submission, there's a rake of coding style "issues" in the new code
> you've added.
> 
I do not run checkpatch with "--strict". I will run with it.
> Cheers,
> Conor.