Add support for Hikey 970 PCIe

Document the bindings for HiKey 970 (hi3670) PCIe PHY interface, supported via the pcie-kirin driver.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 .../phy/hisilicon,phy-hi3670-pcie.yaml        | 95 +++++++++++++++++++
 1 file changed, 95 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml

Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml new file +41 -0

new file mode 100644
index 000000000000..a5ea13332cac
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/hisilicon,phy-hi3670-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HiSilicon Kirin970 PCIe PHY
+
+maintainers:
+  - Mauro Carvalho Chehab <[email protected]>
+
+description: |+
+  Bindings for PCIe PHY on HiSilicon Kirin 970.
+
+properties:
+  compatible:
+    const: hisilicon,hi970-pcie-phy
+
+  "#phy-cells":
+    const: 0
+
+  reg:
+    maxItems: 1
+    description: PHY Control registers
+
+  phy-supply:
+    description: The PCIe PHY power supply
+
+  clocks:
+    items:
+      - description: PCIe PHY clock
+      - description: PCIe AUX clock
+      - description: PCIe APB PHY clock
+      - description: PCIe APB SYS clock
+      - description: PCIe ACLK clock
+
+  clock-names:
+    items:
+      - const: phy_ref
+      - const: aux
+      - const: apb_phy

Quoted 60 lines

Document the bindings for HiKey 970 (hi3670) PCIe PHY interface, supported via the pcie-kirin driver.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 .../phy/hisilicon,phy-hi3670-pcie.yaml        | 95 +++++++++++++++++++
 1 file changed, 95 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml

Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml new file +45 -0

new file mode 100644
index 000000000000..a5ea13332cac
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/hisilicon,phy-hi3670-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HiSilicon Kirin970 PCIe PHY
+
+maintainers:
+  - Mauro Carvalho Chehab <[email protected]>
+
+description: |+
+  Bindings for PCIe PHY on HiSilicon Kirin 970.
+
+properties:
+  compatible:
+    const: hisilicon,hi970-pcie-phy
+
+  "#phy-cells":
+    const: 0
+
+  reg:
+    maxItems: 1
+    description: PHY Control registers
+
+  phy-supply:
+    description: The PCIe PHY power supply
+
+  clocks:
+    items:
+      - description: PCIe PHY clock
+      - description: PCIe AUX clock
+      - description: PCIe APB PHY clock
+      - description: PCIe APB SYS clock
+      - description: PCIe ACLK clock
+
+  clock-names:
+    items:
+      - const: phy_ref
+      - const: aux
+      - const: apb_phy
+      - const: apb_sys
+      - const: aclk
+
+  reset-gpios:

Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml line 51

Quoted 6 lines

Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml new file +6 -0

+    description: PCI PERST reset GPIOs
+    maxItems: 4
+
+  clkreq-gpios:
+    description: Clock request GPIOs
+    maxItems: 3  

Again, this will not work.

It boils down to this fails to describe how the GPIOs are connected which is the point of GPIOs in DT. This in no way captures the hierarchy of devices. While you may be lucky that you can just assert or deassert all the lines at one time, that's not likely to work in a more complicated case (such as having to power up/down each device).

I realize the right solution is more complex, but that's the only way to handle this in a host bridge and board independent way.

If you want the simple solution, just configure all these GPIOs in firmware before Linux boots.

Em Fri, 23 Jul 2021 16:50:59 -0600
Rob Herring <[email protected]> escreveu:

Quoted 61 lines · Mauro Carvalho Chehab

Quoted 60 lines

Document the bindings for HiKey 970 (hi3670) PCIe PHY interface, supported via the pcie-kirin driver.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 .../phy/hisilicon,phy-hi3670-pcie.yaml        | 95 +++++++++++++++++++
 1 file changed, 95 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml

Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml new file +45 -0

new file mode 100644
index 000000000000..a5ea13332cac
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/hisilicon,phy-hi3670-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HiSilicon Kirin970 PCIe PHY
+
+maintainers:
+  - Mauro Carvalho Chehab <[email protected]>
+
+description: |+
+  Bindings for PCIe PHY on HiSilicon Kirin 970.
+
+properties:
+  compatible:
+    const: hisilicon,hi970-pcie-phy
+
+  "#phy-cells":
+    const: 0
+
+  reg:
+    maxItems: 1
+    description: PHY Control registers
+
+  phy-supply:
+    description: The PCIe PHY power supply
+
+  clocks:
+    items:
+      - description: PCIe PHY clock
+      - description: PCIe AUX clock
+      - description: PCIe APB PHY clock
+      - description: PCIe APB SYS clock
+      - description: PCIe ACLK clock
+
+  clock-names:
+    items:
+      - const: phy_ref
+      - const: aux
+      - const: apb_phy
+      - const: apb_sys
+      - const: aclk
+
+  reset-gpios:

Quoted 8 lines

Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml line 51

Quoted 6 lines

Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml new file +6 -0

+    description: PCI PERST reset GPIOs
+    maxItems: 4
+
+  clkreq-gpios:
+    description: Clock request GPIOs
+    maxItems: 3

Again, this will not work.

Just to be sure: you're talking about the PERST# gpios (e. g. reset-gpios) here, right?

Quoted 5 lines

It boils down to this fails to describe how the GPIOs are connected which is the point of GPIOs in DT. This in no way captures the hierarchy of devices. While you may be lucky that you can just assert or deassert all the lines at one time, that's not likely to work in a more complicated case (such as having to power up/down each device).

There's no way to power up/down each device, as they all share the same regulator line (LDO33). So, when this is powered on, all PCI devices are powered at the same time.

The original DT had names for each reset-gpio, but this was just informative, as the only possible way for this hardware to work is to send the PERST# signal via all GPIOs at the same time.

Ok, we might overdesign the DT, in order to consider a non-existent scenario where it would be possible to power on and reset the devices in separate, but I can't think on a way to do that, except by maybe creating virtual phy (or pcie) DT nodes, one for each combination of regulator + PERST#, and have separate drivers for each one. Such kind of scenario only makes sense when each PCIe device can be powered on independently (which is not the case here).

If you have a better idea, I'm all ears.

Quoted 5 lines

I realize the right solution is more complex, but that's the only way to handle this in a host bridge and board independent way.

If you want the simple solution, just configure all these GPIOs in firmware before Linux boots.

This won't work. The PERST# signal should be sent after initializing the PCIe + PHY and powering up the PEX8606 PCIe bridge chipset (via LDO33). That happens when the PCIe driver is loaded.

Quoted 74 lines · Mauro Carvalho Chehab

Em Fri, 23 Jul 2021 16:50:59 -0600
Rob Herring <[email protected]> escreveu:

Quoted 61 lines · Mauro Carvalho Chehab

Quoted 60 lines

Document the bindings for HiKey 970 (hi3670) PCIe PHY interface, supported via the pcie-kirin driver.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 .../phy/hisilicon,phy-hi3670-pcie.yaml        | 95 +++++++++++++++++++
 1 file changed, 95 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml

Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml new file +45 -0

new file mode 100644
index 000000000000..a5ea13332cac
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/hisilicon,phy-hi3670-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HiSilicon Kirin970 PCIe PHY
+
+maintainers:
+  - Mauro Carvalho Chehab <[email protected]>
+
+description: |+
+  Bindings for PCIe PHY on HiSilicon Kirin 970.
+
+properties:
+  compatible:
+    const: hisilicon,hi970-pcie-phy
+
+  "#phy-cells":
+    const: 0
+
+  reg:
+    maxItems: 1
+    description: PHY Control registers
+
+  phy-supply:
+    description: The PCIe PHY power supply
+
+  clocks:
+    items:
+      - description: PCIe PHY clock
+      - description: PCIe AUX clock
+      - description: PCIe APB PHY clock
+      - description: PCIe APB SYS clock
+      - description: PCIe ACLK clock
+
+  clock-names:
+    items:
+      - const: phy_ref
+      - const: aux
+      - const: apb_phy
+      - const: apb_sys
+      - const: aclk
+
+  reset-gpios:

Quoted 8 lines

Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml line 51

Quoted 6 lines

Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml new file +6 -0

+    description: PCI PERST reset GPIOs
+    maxItems: 4
+
+  clkreq-gpios:
+    description: Clock request GPIOs
+    maxItems: 3  

Again, this will not work.

Quoted 2 lines

Just to be sure: you're talking about the PERST# gpios (e. g. reset-gpios) here, right?

Both that and CLKREQ.

Quoted 6 lines

Quoted 5 lines

It boils down to this fails to describe how the GPIOs are connected which is the point of GPIOs in DT. This in no way captures the hierarchy of devices. While you may be lucky that you can just assert or deassert all the lines at one time, that's not likely to work in a more complicated case (such as having to power up/down each device).

Quoted 3 lines

There's no way to power up/down each device, as they all share the same regulator line (LDO33). So, when this is powered on, all PCI devices are powered at the same time.

I understand that for your board, but you could easily have a power supply per device (or multiple supplies per device).

Quoted 4 lines

The original DT had names for each reset-gpio, but this was just informative, as the only possible way for this hardware to work is to send the PERST# signal via all GPIOs at the same time.

What's the timing requirement here? I doubt 'at the same time' is the actual h/w requirement. My guess is it is before the PCI bus scan if you don't have any hook before each child bus is scanned.

Quoted 7 lines

Ok, we might overdesign the DT, in order to consider a non-existent scenario where it would be possible to power on and reset the devices in separate, but I can't think on a way to do that, except by maybe creating virtual phy (or pcie) DT nodes, one for each combination of regulator + PERST#, and have separate drivers for each one. Such kind of scenario only makes sense when each PCIe device can be powered on independently (which is not the case here).

If someone made hikey970 with the topology you have, then someone can just as easily make a different topology and one that doesn't work with the assumptions you've made. We're only going to see more and more embedded boards with multiple PCI devices.

Quoted 2 lines

If you have a better idea, I'm all ears.

There's already a spec for populating PCI devices in DT. It's existed for over 20 years with OpenFirmware[1]. It's not widely used on FDT systems because most cases to date are just a single device attached and they don't have extra things needing to be described in DT. There are a few, but not many examples in the tree of PCI devices with DT nodes. That's the only way to generically describe the topology you have.

While I haven't seen another case exactly like yours yet, there are frequent cases of PCI devices (and other discoverable buses) that have extra resources that are not discoverable. And some of those need control before the device can be discovered. I see various work-arounds to the problem, but describing the devices in DT is the right way. It's only going to get solved if the work-arounds are rejected. I care more that the DT binding is correct and less if the kernel side is clean. The kernel implementation can evolve, the DT cannot.

Quoted 9 lines

Quoted 5 lines

I realize the right solution is more complex, but that's the only way to handle this in a host bridge and board independent way.

If you want the simple solution, just configure all these GPIOs in firmware before Linux boots.

This won't work. The PERST# signal should be sent after initializing the PCIe + PHY and powering up the PEX8606 PCIe bridge chipset (via LDO33). That happens when the PCIe driver is loaded.

Only because you have no hooks for handling PERST# on devices downstream of the PEX8606. Surely a sequence like this would work:
deassert root PERST# (to PEX8606), scan root bus, find and init PCIe bridge, deassert PEX8606 child bus(es) PERST#, scan child bus(es), find and init child devices. I think the .add_bus() hook could work for you. IIRC, that's called before a child bus is scanned.

[1] https://www.devicetree.org/open-firmware/home.html#OFDbussupps

Em Mon, 26 Jul 2021 15:37:28 -0600
Rob Herring <[email protected]> escreveu:

Quoted 14 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml new file +7 -0

+  reset-gpios:
+    description: PCI PERST reset GPIOs
+    maxItems: 4
+
+  clkreq-gpios:
+    description: Clock request GPIOs
+    maxItems: 3    

Quoted 4 lines

Quoted 1 line

Again, this will not work.

Just to be sure: you're talking about the PERST# gpios (e. g. reset-gpios) here, right?

Both that and CLKREQ.

Quoted 11 lines

Quoted 1 line

If you have a better idea, I'm all ears.

There's already a spec for populating PCI devices in DT. It's existed for over 20 years with OpenFirmware[1]. It's not widely used on FDT systems because most cases to date are just a single device attached and they don't have extra things needing to be described in DT. There are a few, but not many examples in the tree of PCI devices with DT nodes. That's the only way to generically describe the topology you have.

[1] https://www.devicetree.org/open-firmware/home.html#OFDbussupps

Em Tue, 27 Jul 2021 01:50:20 +0200
Mauro Carvalho Chehab <[email protected]> escreveu:

Quoted 18 lines

Em Mon, 26 Jul 2021 15:37:28 -0600
Rob Herring <[email protected]> escreveu:

Quoted 14 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml new file +7 -0

+  reset-gpios:
+    description: PCI PERST reset GPIOs
+    maxItems: 4
+
+  clkreq-gpios:
+    description: Clock request GPIOs
+    maxItems: 3

Quoted 4 lines

Quoted 1 line

Again, this will not work.

Just to be sure: you're talking about the PERST# gpios (e. g. reset-gpios) here, right?

Both that and CLKREQ.

The original DT from the downstream version (found at Linaro's tree) has:

        pcie@f4000000 {
                compatible = "hisilicon,hikey970";
...
                switch,reset-gpios = <&gpio7 0 0 >;
                eth,reset-gpios = <&gpio25 2 0 >;
                m_2,reset-gpios = <&gpio3 1 0 >;
                mini1,reset-gpios = <&gpio27 4 0 >;

                eth,clkreq-gpios = <&gpio20 6 0 >;
                m_2,clkreq-gpios = <&gpio27 3 0 >;
                mini1,clkreq-gpios = <&gpio17 0 0 >;
        };

So, if we're willing to have a single reset-gpios for the PCIe interface, in order to follow the current pci-bus.yaml schema, this would probably be:

        reset-gpios = <&gpio7 0 0 >;

which maps to the PEX8606 PCIe bridge chip.

With that, DT still need to point a per-slot clkreq and reset-gpio.

One alternative would be to map it as either 3 PCI or PHY child nodes. E. g. something like this:

        pcie@f4000000 {
                compatible = "hisilicon,kirin970-pcie";
...
                reset-gpios = <&gpio7 0 0 >;

                slot {
                        eth {
                                reset-gpios = <&gpio25 2 0>;
                                clkreq-gpios = <&gpio20 6 0>;
                        };
                        m2 {
                                reset-gpios = <&gpio3 1 0>;
                                clkreq-gpios = <&gpio27 3 0>;
                        };
                        mini1 {
                                reset-gpios = <&gpio27 4 0>;
                                clkreq-gpios = <&gpio17 0 0>;
                        };
                };
        };

Placing the child nodes ("slot"?) at the pci bus properties makes more sense to me, but placing them at the PHY node has the advantage of only affecting Kirin 970.

In either case, if each child would need a different power supply, it won't be hard to add a "slot-supply" property later on.

Would something like that be acceptable for you?

Quoted 75 lines · Mauro Carvalho Chehab

Em Tue, 27 Jul 2021 01:50:20 +0200
Mauro Carvalho Chehab <[email protected]> escreveu:

Quoted 18 lines

Em Mon, 26 Jul 2021 15:37:28 -0600
Rob Herring <[email protected]> escreveu:

Quoted 14 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

(quoted diff) +7 -0

+  reset-gpios:
+    description: PCI PERST reset GPIOs
+    maxItems: 4
+
+  clkreq-gpios:
+    description: Clock request GPIOs
+    maxItems: 3  

Quoted 4 lines

Quoted 1 line

Again, this will not work.

Just to be sure: you're talking about the PERST# gpios (e. g. reset-gpios) here, right?

Both that and CLKREQ.

The original DT from the downstream version (found at Linaro's tree) has:

        pcie@f4000000 {
                compatible = "hisilicon,hikey970";
...
                switch,reset-gpios = <&gpio7 0 0 >;
                eth,reset-gpios = <&gpio25 2 0 >;
                m_2,reset-gpios = <&gpio3 1 0 >;
                mini1,reset-gpios = <&gpio27 4 0 >;

                eth,clkreq-gpios = <&gpio20 6 0 >;
                m_2,clkreq-gpios = <&gpio27 3 0 >;
                mini1,clkreq-gpios = <&gpio17 0 0 >;
        };

So, if we're willing to have a single reset-gpios for the PCIe interface, in order to follow the current pci-bus.yaml schema, this would probably be:

        reset-gpios = <&gpio7 0 0 >;

which maps to the PEX8606 PCIe bridge chip.

With that, DT still need to point a per-slot clkreq and reset-gpio.

One alternative would be to map it as either 3 PCI or PHY child nodes. E. g. something like this:

        pcie@f4000000 {
                compatible = "hisilicon,kirin970-pcie";
...
                reset-gpios = <&gpio7 0 0 >;

                slot {
                        eth {
                                reset-gpios = <&gpio25 2 0>;
                                clkreq-gpios = <&gpio20 6 0>;
                        };
                        m2 {
                                reset-gpios = <&gpio3 1 0>;
                                clkreq-gpios = <&gpio27 3 0>;
                        };
                        mini1 {
                                reset-gpios = <&gpio27 4 0>;
                                clkreq-gpios = <&gpio17 0 0>;
                        };
                };
        };

Quoted 8 lines

Placing the child nodes ("slot"?) at the pci bus properties makes more sense to me, but placing them at the PHY node has the advantage of only affecting Kirin 970.

In either case, if each child would need a different power supply, it won't be hard to add a "slot-supply" property later on.

Would something like that be acceptable for you?

On the right track, but there's already a definition for what child devices look like in pci2_1.pdf. I think you want something like this:

pcie@f4000000 { // RP: Bus 0, Device 0

    compatible = "hisilicon,kirin970-pcie";
    ...
    reset-gpios = <&gpio7 0 0>;  // PERST to switch

    pcie@0 { // PCIe switch: Bus 1, Device 0
        reg = <0 0 0 0 0>;
        compatible = "pciclass,0604";
        device_type = "pci";

        pcie@1 { // NC (Can omit this node)
            reg = <0x80 0 0 0 0>;
            compatible = "pciclass,0604";
            device_type = "pci";
        };

        pcie@4 { // M.2
            reg = <0x200 0 0 0 0>;

            compatible = "pciclass,0604";
            device_type = "pci";
            reset-gpios = <&gpio7 1 0>; // PERST to M.2 slot

       };

        pcie@5 { // Mini
            reg = <0x280 0 0 0 0>;
            compatible = "pciclass,0604";
            device_type = "pci";
            reset-gpios = <&gpio7 2 0>; // PERST to Mini slot
        };

        pcie@7 { // Ethernet
            reg = <0x380 0 0 0 0>;

            compatible = "pciclass,0604";
            device_type = "pci";
            reset-gpios = <&gpio7 3 0>; // PERST to Ethernet

            ethernet@0 {
                reg = <0 0 0 0 0>;
                local-mac-address = [ 00 01 02 03 04 05 06 ];
            };

        };

        pcie@9 { // NC
            reg = <0x480 0 0 0 0>;
            compatible = "pciclass,0604";
            device_type = "pci";
       };
};

This is based on what you previously sent:
00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3670 (rev 01) 01:00.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
02:01.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
02:04.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
02:05.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
02:07.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
02:09.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
06:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 07)

A few notes:
I left out #size-cells, #address-cells, and ranges for brevity.

I'm not completely sure I've got the bridges mapped to the right functions on Hikey970. That's my best guess looking at the schematics. You should be able to confirm which bridge is the parent bridge for ethernet at least.

The compatible strings aren't strictly needed. Linux doesn't look at them.

There's a pretty complete example in:
arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi

The simplest Linux implementation to handle the above is just walk the child nodes and get all the 'reset-gpios' properties. That's not the implementation I think we should have though. We should handle the GPIO as each bridge is probed and children scanned.

Em Tue, 27 Jul 2021 16:17:43 -0600
Rob Herring <[email protected]> escreveu:

Quoted 105 lines

Quoted 75 lines · Mauro Carvalho Chehab

Em Tue, 27 Jul 2021 01:50:20 +0200
Mauro Carvalho Chehab <[email protected]> escreveu:

Quoted 18 lines

Em Mon, 26 Jul 2021 15:37:28 -0600
Rob Herring <[email protected]> escreveu:

Quoted 14 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

(quoted diff) +7 -0

+  reset-gpios:
+    description: PCI PERST reset GPIOs
+    maxItems: 4
+
+  clkreq-gpios:
+    description: Clock request GPIOs
+    maxItems: 3

Quoted 4 lines

Quoted 1 line

Again, this will not work.

Just to be sure: you're talking about the PERST# gpios (e. g. reset-gpios) here, right?

Both that and CLKREQ.

The original DT from the downstream version (found at Linaro's tree) has:

        pcie@f4000000 {
                compatible = "hisilicon,hikey970";
...
                switch,reset-gpios = <&gpio7 0 0 >;
                eth,reset-gpios = <&gpio25 2 0 >;
                m_2,reset-gpios = <&gpio3 1 0 >;
                mini1,reset-gpios = <&gpio27 4 0 >;

                eth,clkreq-gpios = <&gpio20 6 0 >;
                m_2,clkreq-gpios = <&gpio27 3 0 >;
                mini1,clkreq-gpios = <&gpio17 0 0 >;
        };

So, if we're willing to have a single reset-gpios for the PCIe interface, in order to follow the current pci-bus.yaml schema, this would probably be:

        reset-gpios = <&gpio7 0 0 >;

which maps to the PEX8606 PCIe bridge chip.

With that, DT still need to point a per-slot clkreq and reset-gpio.

One alternative would be to map it as either 3 PCI or PHY child nodes. E. g. something like this:

        pcie@f4000000 {
                compatible = "hisilicon,kirin970-pcie";
...
                reset-gpios = <&gpio7 0 0 >;

                slot {
                        eth {
                                reset-gpios = <&gpio25 2 0>;
                                clkreq-gpios = <&gpio20 6 0>;
                        };
                        m2 {
                                reset-gpios = <&gpio3 1 0>;
                                clkreq-gpios = <&gpio27 3 0>;
                        };
                        mini1 {
                                reset-gpios = <&gpio27 4 0>;
                                clkreq-gpios = <&gpio17 0 0>;
                        };
                };
        };

Quoted 8 lines

Placing the child nodes ("slot"?) at the pci bus properties makes more sense to me, but placing them at the PHY node has the advantage of only affecting Kirin 970.

In either case, if each child would need a different power supply, it won't be hard to add a "slot-supply" property later on.

Would something like that be acceptable for you?

On the right track, but there's already a definition for what child devices look like in pci2_1.pdf. I think you want something like this:

pcie@f4000000 { // RP: Bus 0, Device 0

    compatible = "hisilicon,kirin970-pcie";
    ...
    reset-gpios = <&gpio7 0 0>;  // PERST to switch

    pcie@0 { // PCIe switch: Bus 1, Device 0
        reg = <0 0 0 0 0>;
        compatible = "pciclass,0604";
        device_type = "pci";

        pcie@1 { // NC (Can omit this node)
            reg = <0x80 0 0 0 0>;
            compatible = "pciclass,0604";
            device_type = "pci";
        };

Quoted 2 lines

        pcie@4 { // M.2
            reg = <0x200 0 0 0 0>;

Not sure what to put at reg. I suspect that the best would be to follow the PEX port number, as, if one day someone decides to implement an I2C driver, this might be useful.

Quoted 3 lines

            compatible = "pciclass,0604";
            device_type = "pci";
            reset-gpios = <&gpio7 1 0>; // PERST to M.2 slot

We also need the clock-req phandle for the three devices.

Quoted 16 lines

       };

        pcie@5 { // Mini
            reg = <0x280 0 0 0 0>;
            compatible = "pciclass,0604";
            device_type = "pci";
            reset-gpios = <&gpio7 2 0>; // PERST to Mini slot
        };

        pcie@7 { // Ethernet
            reg = <0x380 0 0 0 0>;
            compatible = "pciclass,0604";
            device_type = "pci";
            reset-gpios = <&gpio7 3 0>; // PERST to Ethernet

Quoted 4 lines

            ethernet@0 {
                reg = <0 0 0 0 0>;
                local-mac-address = [ 00 01 02 03 04 05 06 ];
            };

No need to add a mac address here. The Ethernet card has a mac already:

        60:fa:9d:xx:xx:xx

Which seems to be a valid one:

        https://hwaddress.com/?q=60%3Afa%3A9d

Quoted 26 lines

        };

        pcie@9 { // NC
            reg = <0x480 0 0 0 0>;
            compatible = "pciclass,0604";
            device_type = "pci";
       };
};

This is based on what you previously sent:
00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3670 (rev 01) 01:00.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
02:01.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
02:04.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
02:05.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
02:07.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
02:09.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
06:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 07)

Quoted 7 lines

A few notes:
I left out #size-cells, #address-cells, and ranges for brevity.

I'm not completely sure I've got the bridges mapped to the right functions on Hikey970. That's my best guess looking at the schematics. You should be able to confirm which bridge is the parent bridge for ethernet at least.

I added a NVMe to M.2 slot:

$ lspci -D -P -PP
0000:00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3670 (rev 01)
0000:00:00.0/01:00.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
0000:00:00.0/01:00.0/02:01.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
0000:00:00.0/01:00.0/02:04.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
0000:00:00.0/01:00.0/02:05.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
0000:00:00.0/01:00.0/02:07.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
0000:00:00.0/01:00.0/02:09.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
0000:00:00.0/01:00.0/02:01.0/03:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd Device a809
0000:00:00.0/01:00.0/02:07.0/06:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 07)

it sounds that all devices are behind the PEX 8606 bridge:
- port 1 seems to be the M.2 slot
- port 7 seems to be the Ethernet adapter

I don't have any mini PCIe devices here. I'll try to get one in order to be sure about the topology.

Quoted 2 lines

The compatible strings aren't strictly needed. Linux doesn't look at them.

If not needed, IMO the best would be to not add it, keeping it as simple as possible.

Quoted 3 lines

There's a pretty complete example in:
arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi

Thanks!

Quoted 4 lines

The simplest Linux implementation to handle the above is just walk the child nodes and get all the 'reset-gpios' properties. That's not the implementation I think we should have though. We should handle the GPIO as each bridge is probed and children scanned.

The power-on sequence is:

1. CLK, PHY and DWC initialization;
2. 21ms delay;
3. PERST# sent to each device;
4. 10ms delay;
5. adjust the eye diagram;
6. power off NOC.

Most of the above are at the PHY driver. Now, it would be possible to map those as:

        phy->init() - steps 1 and 2;
        phy->power_on() - steps 4, 5 and 6.

And change somehow the pcie-kirin driver to only call phy->power_on() after doing the bus probing sequence, but a change like that would mean that the eye diagram will only be adjusted at the end. That doesn't sound a good idea to me, as probing devices with a wrong eye diagram could cause bit errors when talking to the devices inside the PCI bus. This is likely not a problem if all devices are directly connected to the hardware, but it could be an issue if someone uses either a M.2 or a mini-PCI extensor.

So, IMO, the best would be for the PHY driver to walk the child nodes and get all the 'reset-gpios' properties.

With regards to the clock-req phandles, those should be enabled before the PHY clocks, in order to avoid the SError issue.

It should be easy to implement this at the the PCIe driver, but, this should happen in early stages at the power-on sequence (before enabling the DWC PHY clocks). So, the PCIe driver (or the PHY) will need to walk the child nodes and get all the 'reset-gpios' properties.

For the sake of avoiding to duplicate the walk-though and parsing logic, I would do it only at the PHY driver.

Quoted 19 lines

Quoted 18 lines

pcie@f4000000 { // RP: Bus 0, Device 0

    compatible = "hisilicon,kirin970-pcie";
    ...
    reset-gpios = <&gpio7 0 0>;  // PERST to switch

    pcie@0 { // PCIe switch: Bus 1, Device 0
        reg = <0 0 0 0 0>;
        compatible = "pciclass,0604";
        device_type = "pci";

        pcie@1 { // NC (Can omit this node)
            reg = <0x80 0 0 0 0>;
            compatible = "pciclass,0604";
            device_type = "pci";
        };

        pcie@4 { // M.2
            reg = <0x200 0 0 0 0>;  

Quoted 3 lines

Not sure what to put at reg. I suspect that the best would be to follow the PEX port number, as, if one day someone decides to implement an I2C driver, this might be useful.

It's defined in the PCI bus binding. Basically, it's the BDF of the device. However, as the bus number is dynamic, I think we want to leave that as 0 for FDT. The function is optional and always 0 in this case.

Quoted 5 lines

Quoted 3 lines

            compatible = "pciclass,0604";
            device_type = "pci";
            reset-gpios = <&gpio7 1 0>; // PERST to M.2 slot  

We also need the clock-req phandle for the three devices.

Hopefully, you can figure out where those belong now...

Quoted 2 lines

With regards to the clock-req phandles, those should be enabled before the PHY clocks, in order to avoid the SError issue.

Huh? What exactly causes an SError. Has to be some bus access.

Quoted 6 lines

It should be easy to implement this at the the PCIe driver, but, this should happen in early stages at the power-on sequence (before enabling the DWC PHY clocks). So, the PCIe driver (or the PHY) will need to walk the child nodes and get all the 'reset-gpios' properties.

Quoted 2 lines

For the sake of avoiding to duplicate the walk-though and parsing logic, I would do it only at the PHY driver.

Everyone else handles this stuff in their PCIe driver. You are not special...

I have a plan to make the PERST handling common across all PCIe host drivers and also make the PHY handling common across DWC drivers. Don't make that harder.

From: Manivannan Sadhasivam <[email protected]>

Add DTS bindings for the HiKey 970 board's PCIe hardware.

Co-developed-by Mauro Carvalho Chehab <[email protected]>

Signed-off-by Manivannan Sadhasivam <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 arch/arm64/boot/dts/hisilicon/hi3670.dtsi     | 71 +++++++++++++++++++
 .../boot/dts/hisilicon/hikey970-pmic.dtsi     |  1 -
 drivers/pci/controller/dwc/pcie-kirin.c       | 12 ----
 3 files changed, 71 insertions(+), 13 deletions(-)

arch/arm64/boot/dts/hisilicon/hi3670.dtsi +0 -0

index 1f228612192c..6dfcfcfeedae 100644

arch/arm64/boot/dts/hisilicon/hi3670.dtsi +0 -0

 		iomcu: iomcu@ffd7e000 {
 			compatible = "hisilicon,hi3670-iomcu", "syscon";
 			reg = <0x0 0xffd7e000 0x0 0x1000>;

@@ -660,6 +666,71 @@ gpio28: gpio@fff1d000 {
 			clock-names = "apb_pclk";
 		};
 

arch/arm64/boot/dts/hisilicon/hi3670.dtsi +12 -0

+			interrupt-names = "msi";
+			interrupt-map-mask = <0 0 0 7>;
+			interrupt-map = <0x0 0 0 1
+					 &gic GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>,
+					<0x0 0 0 2
+					 &gic GIC_SPI 283 IRQ_TYPE_LEVEL_HIGH>,
+					<0x0 0 0 3
+					 &gic GIC_SPI 284 IRQ_TYPE_LEVEL_HIGH>,
+					<0x0 0 0 4
+					 &gic GIC_SPI 285 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
 		/* UFS */
 		ufs: ufs@ff3c0000 {
 			compatible = "hisilicon,hi3670-ufs", "jedec,ufs-2.1";

arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi +0 -0

index 48c739eacba0..03452e627641 100644

arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi +0 -0

 				};
 
 				ldo34: LDO34 { /* GPS AUX IN VDD */

drivers/pci/controller/dwc/pcie-kirin.c +0 -2

index bfc0513f7b15..9dad14929538 100644
@@ -347,18 +347,6 @@ static const struct regmap_config pcie_kirin_regmap_conf = {
 	.reg_stride = 4,
 };
 
-/* Registers in PCIeCTRL */
-static inline void kirin_apb_ctrl_writel(struct kirin_pcie *kirin_pcie,

Quoted 21 lines

From: Manivannan Sadhasivam <[email protected]>

Add DTS bindings for the HiKey 970 board's PCIe hardware.

Co-developed-by Mauro Carvalho Chehab <[email protected]>

Signed-off-by Manivannan Sadhasivam <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 arch/arm64/boot/dts/hisilicon/hi3670.dtsi     | 71 +++++++++++++++++++
 .../boot/dts/hisilicon/hikey970-pmic.dtsi     |  1 -
 drivers/pci/controller/dwc/pcie-kirin.c       | 12 ----
 3 files changed, 71 insertions(+), 13 deletions(-)

arch/arm64/boot/dts/hisilicon/hi3670.dtsi +0 -0

index 1f228612192c..6dfcfcfeedae 100644
@@ -177,6 +177,12 @@ sctrl: sctrl@fff0a000 {
 			#clock-cells = <1>;
 		};
 

arch/arm64/boot/dts/hisilicon/hi3670.dtsi sctrl: sctrl@fff0a000 { · line 185

Quoted 6 lines

arch/arm64/boot/dts/hisilicon/hi3670.dtsi +6 -0

+		pmctrl: pmctrl@fff31000 {
+			compatible = "hisilicon,hi3670-pmctrl", "syscon";
+			reg = <0x0 0xfff31000 0x0 0x1000>;
+			#clock-cells = <1>;
+		};
+  

Irrelevant change to this patch.

Quoted 8 lines

arch/arm64/boot/dts/hisilicon/hi3670.dtsi +0 -0

 		iomcu: iomcu@ffd7e000 {
 			compatible = "hisilicon,hi3670-iomcu", "syscon";
 			reg = <0x0 0xffd7e000 0x0 0x1000>;

@@ -660,6 +666,71 @@ gpio28: gpio@fff1d000 {
 			clock-names = "apb_pclk";
 		};
 

[...]

arch/arm64/boot/dts/hisilicon/hi3670.dtsi gpio28: gpio@fff1d000 { · line 721

Quoted 2 lines

arch/arm64/boot/dts/hisilicon/hi3670.dtsi +2 -0

+			#interrupt-cells = <1>;
+			interrupts = <0 283 4>;  

Use the DT flag for interrupts instead of hardcoded value

Quoted 19 lines

arch/arm64/boot/dts/hisilicon/hi3670.dtsi +12 -0

+			interrupt-names = "msi";
+			interrupt-map-mask = <0 0 0 7>;
+			interrupt-map = <0x0 0 0 1
+					 &gic GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>,
+					<0x0 0 0 2
+					 &gic GIC_SPI 283 IRQ_TYPE_LEVEL_HIGH>,
+					<0x0 0 0 3
+					 &gic GIC_SPI 284 IRQ_TYPE_LEVEL_HIGH>,
+					<0x0 0 0 4
+					 &gic GIC_SPI 285 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
 		/* UFS */
 		ufs: ufs@ff3c0000 {
 			compatible = "hisilicon,hi3670-ufs", "jedec,ufs-2.1";

arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi +0 -1

index 48c739eacba0..03452e627641 100644
--- a/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi

arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi ldo33: LDO33 { /* PEX8606 */ · line 76

Quoted 6 lines

+++ b/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi

arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi +0 -1

@@ -73,7 +73,6 @@ ldo33: LDO33 { /* PEX8606 */
 					regulator-name = "ldo33";
 					regulator-min-microvolt = <2500000>;
 					regulator-max-microvolt = <3300000>;
-					regulator-boot-on;  

Again, irrelevant.

Quoted 12 lines

arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi +0 -0

 				};
 
 				ldo34: LDO34 { /* GPS AUX IN VDD */

drivers/pci/controller/dwc/pcie-kirin.c +0 -0

index bfc0513f7b15..9dad14929538 100644
@@ -347,18 +347,6 @@ static const struct regmap_config pcie_kirin_regmap_conf = {
 	.reg_stride = 4,
 };
 

drivers/pci/controller/dwc/pcie-kirin.c static const struct regmap_config pcie_kirin_regmap_conf = { · line 361

Quoted 12 lines

drivers/pci/controller/dwc/pcie-kirin.c +0 -12

-/* Registers in PCIeCTRL */
-static inline void kirin_apb_ctrl_writel(struct kirin_pcie *kirin_pcie,
-					 u32 val, u32 reg)
-{
-	writel(val, kirin_pcie->apb_base + reg);
-}
-
-static inline u32 kirin_apb_ctrl_readl(struct kirin_pcie *kirin_pcie, u32 reg)
-{
-	return readl(kirin_pcie->apb_base + reg);
-}
-  

Same here...

Em Thu, 22 Jul 2021 19:06:28 +0530
Manivannan Sadhasivam <[email protected]> escreveu:

Quoted 22 lines · Mauro Carvalho Chehab

Quoted 21 lines

From: Manivannan Sadhasivam <[email protected]>

Add DTS bindings for the HiKey 970 board's PCIe hardware.

Co-developed-by Mauro Carvalho Chehab <[email protected]>

Signed-off-by Manivannan Sadhasivam <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 arch/arm64/boot/dts/hisilicon/hi3670.dtsi     | 71 +++++++++++++++++++
 .../boot/dts/hisilicon/hikey970-pmic.dtsi     |  1 -
 drivers/pci/controller/dwc/pcie-kirin.c       | 12 ----
 3 files changed, 71 insertions(+), 13 deletions(-)

arch/arm64/boot/dts/hisilicon/hi3670.dtsi +0 -0

index 1f228612192c..6dfcfcfeedae 100644
@@ -177,6 +177,12 @@ sctrl: sctrl@fff0a000 {
 			#clock-cells = <1>;
 		};
 

Quoted 8 lines

arch/arm64/boot/dts/hisilicon/hi3670.dtsi sctrl: sctrl@fff0a000 { · line 185

Quoted 6 lines

arch/arm64/boot/dts/hisilicon/hi3670.dtsi +6 -0

+		pmctrl: pmctrl@fff31000 {
+			compatible = "hisilicon,hi3670-pmctrl", "syscon";
+			reg = <0x0 0xfff31000 0x0 0x1000>;
+			#clock-cells = <1>;
+		};
+  

Irrelevant change to this patch.

Huh?

This is used by PCIe PHY, as part of the power on procedures:

+static int hi3670_pcie_noc_power(struct hi3670_pcie_phy *phy, bool enable) +{
+ struct device *dev = phy->dev;
+ u32 time = 100;
+ unsigned int val = NOC_PW_MASK;
+ int rst;
+
+ if (enable)
+ val = NOC_PW_MASK | NOC_PW_SET_BIT;
+ else
+ val = NOC_PW_MASK;
+ rst = enable ? 1 : 0;
+
+ regmap_write(phy->pmctrl, NOC_POWER_IDLEREQ_1, val);

Quoted 12 lines

Quoted 9 lines

arch/arm64/boot/dts/hisilicon/hi3670.dtsi +0 -0

 		iomcu: iomcu@ffd7e000 {
 			compatible = "hisilicon,hi3670-iomcu", "syscon";
 			reg = <0x0 0xffd7e000 0x0 0x1000>;

@@ -660,6 +666,71 @@ gpio28: gpio@fff1d000 {
 			clock-names = "apb_pclk";
 		};
 

[...]

Quoted 4 lines

arch/arm64/boot/dts/hisilicon/hi3670.dtsi gpio28: gpio@fff1d000 { · line 721

Quoted 2 lines

arch/arm64/boot/dts/hisilicon/hi3670.dtsi +2 -0

+			#interrupt-cells = <1>;
+			interrupts = <0 283 4>;  

Use the DT flag for interrupts instead of hardcoded value

Do you mean like this?

	interrupts = <0 283 IRQ_TYPE_LEVEL_HIGH>;

Quoted 38 lines

Em Thu, 22 Jul 2021 19:06:28 +0530
Manivannan Sadhasivam <[email protected]> escreveu:

Quoted 22 lines · Mauro Carvalho Chehab

Quoted 21 lines

From: Manivannan Sadhasivam <[email protected]>

Add DTS bindings for the HiKey 970 board's PCIe hardware.

Co-developed-by Mauro Carvalho Chehab <[email protected]>

Signed-off-by Manivannan Sadhasivam <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 arch/arm64/boot/dts/hisilicon/hi3670.dtsi     | 71 +++++++++++++++++++
 .../boot/dts/hisilicon/hikey970-pmic.dtsi     |  1 -
 drivers/pci/controller/dwc/pcie-kirin.c       | 12 ----
 3 files changed, 71 insertions(+), 13 deletions(-)

arch/arm64/boot/dts/hisilicon/hi3670.dtsi +0 -0

index 1f228612192c..6dfcfcfeedae 100644
@@ -177,6 +177,12 @@ sctrl: sctrl@fff0a000 {
 			#clock-cells = <1>;
 		};
 

Quoted 8 lines

arch/arm64/boot/dts/hisilicon/hi3670.dtsi sctrl: sctrl@fff0a000 { · line 185

Quoted 6 lines

arch/arm64/boot/dts/hisilicon/hi3670.dtsi +6 -0

+		pmctrl: pmctrl@fff31000 {
+			compatible = "hisilicon,hi3670-pmctrl", "syscon";
+			reg = <0x0 0xfff31000 0x0 0x1000>;
+			#clock-cells = <1>;
+		};
+    

Irrelevant change to this patch.

Huh?

This is used by PCIe PHY, as part of the power on procedures:

Quoted 16 lines

+static int hi3670_pcie_noc_power(struct hi3670_pcie_phy *phy, bool enable) +{
+ struct device *dev = phy->dev;
+ u32 time = 100;
+ unsigned int val = NOC_PW_MASK;
+ int rst;
+
+ if (enable)
+ val = NOC_PW_MASK | NOC_PW_SET_BIT;
+ else
+ val = NOC_PW_MASK;
+ rst = enable ? 1 : 0;
+
+ regmap_write(phy->pmctrl, NOC_POWER_IDLEREQ_1, val);

Ah... you're hardcoding the syscon compatible in driver. Sorry missed that.

But if these syscon nodes are independent memory regions or belong to non PCI/PHY memory map, you could've fetched the reference through a DT property along with the offset then used it in driver.

Like,

	pcie_phy: pcie-phy@fc000000 {
		...
		hisilicon,noc-power-regs = <&pmctrl 0x38c>;
		hisilicon,sctrl-cmos-regs = <&sctrl 0x60>;
		...
	};

The benefit of doing this way is, if the pmctrl, sctrl register layout changes in future, you can handle it without any issues.

From: Manivannan Sadhasivam <[email protected]>

Add DTS bindings for the HiKey 970 board's PCIe hardware.

Co-developed-by Mauro Carvalho Chehab <[email protected]>

Signed-off-by Manivannan Sadhasivam <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 arch/arm64/boot/dts/hisilicon/hi3670.dtsi     | 71 +++++++++++++++++++
 .../boot/dts/hisilicon/hikey970-pmic.dtsi     |  1 -
 drivers/pci/controller/dwc/pcie-kirin.c       | 12 ----
 3 files changed, 71 insertions(+), 13 deletions(-)

arch/arm64/boot/dts/hisilicon/hi3670.dtsi +6 -0

index 1f228612192c..6dfcfcfeedae 100644
@@ -177,6 +177,12 @@ sctrl: sctrl@fff0a000 {
                        #clock-cells = <1>;
                };
 
+               pmctrl: pmctrl@fff31000 {
+                       compatible = "hisilicon,hi3670-pmctrl", "syscon";
+                       reg = <0x0 0xfff31000 0x0 0x1000>;
+                       #clock-cells = <1>;
+               };
+
                iomcu: iomcu@ffd7e000 {
                        compatible = "hisilicon,hi3670-iomcu", "syscon";
                        reg = <0x0 0xffd7e000 0x0 0x1000>;

arch/arm64/boot/dts/hisilicon/hi3670.dtsi +29 -0

+               };
+
+               pcie_phy: pcie-phy@fc000000 {
+                       compatible = "hisilicon,hi970-pcie-phy";
+                       reg = <0x0 0xfc000000 0x0 0x80000>;
+
+                       phy-supply = <&ldo33>;
+
+                       clocks = <&crg_ctrl HI3670_CLK_GATE_PCIEPHY_REF>,
+                                <&crg_ctrl HI3670_CLK_GATE_PCIEAUX>,
+                                <&crg_ctrl HI3670_PCLK_GATE_PCIE_PHY>,
+                                <&crg_ctrl HI3670_PCLK_GATE_PCIE_SYS>,
+                                <&crg_ctrl HI3670_ACLK_GATE_PCIE>;
+                       clock-names = "phy_ref", "aux",
+                                     "apb_phy", "apb_sys",
+                                     "aclk";
+
+                       reset-gpios = <&gpio7 0 0 >, <&gpio25 2 0 >,
+                                     <&gpio3 1 0 >, <&gpio27 4 0 >;
+
+                       clkreq-gpios = <&gpio20 6 0 >, <&gpio27 3 0 >,
+                                      <&gpio17 0 0 >;
+
+                       /* vboost iboost pre post main */
+                       hisilicon,eye-diagram-param = <0xFFFFFFFF 0xFFFFFFFF
+                                                      0xFFFFFFFF 0xFFFFFFFF
+                                                      0xFFFFFFFF>;
+
+                       #phy-cells = <0>;

Convert the file into a JSON description at the yaml format.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 .../bindings/pci/hisilicon,kirin-pcie.yaml    | 87 +++++++++++++++++++
 .../devicetree/bindings/pci/kirin-pcie.txt    | 50 -----------
 .../devicetree/bindings/pci/snps,dw-pcie.yaml |  2 +-
 MAINTAINERS                                   |  2 +-
 4 files changed, 89 insertions(+), 52 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/hisilicon,kirin-pcie.yaml
 delete mode 100644 Documentation/devicetree/bindings/pci/kirin-pcie.txt

Documentation/devicetree/bindings/pci/hisilicon,kirin-pcie.yaml new file +37 -0

new file mode 100644
index 000000000000..eabc651c9766
@@ -0,0 +1,87 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/hisilicon,kirin-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HiSilicon Kirin SoCs PCIe host DT description
+
+maintainers:
+  - Xiaowei Song <[email protected]>
+  - Binghui Wang <[email protected]>
+
+description: |
+  Kirin PCIe host controller is based on the Synopsys DesignWare PCI core.
+  It shares common functions with the PCIe DesignWare core driver and
+  inherits common properties defined in
+  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml.
+
+allOf:
+  - $ref: /schemas/pci/snps,dw-pcie.yaml#
+
+properties:
+  compatible:
+    contains:
+      enum:
+        - hisilicon,kirin960-pcie
+        - hisilicon,kirin970-pcie
+
+  reg:
+    description: |
+      Should contain rc_dbi, apb, config registers location and length.
+    minItems: 3
+    maxItems: 4
+
+  reg-names:
+    items:
+      - const: dbi          # controller configuration registers

Documentation/devicetree/bindings/pci/hisilicon,kirin-pcie.yaml new file +40 -0

+
+required:
+  - compatible
+  - reg
+  - reg-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      pcie: pcie@f4000000 {
+        compatible = "hisilicon,kirin970-pcie";
+        reg = <0x0 0xf4000000 0x0 0x1000>,
+              <0x0 0xff3fe000 0x0 0x1000>,
+              <0x0 0xf4000000 0 0x2000>;
+        reg-names = "dbi", "apb", "config";
+        bus-range = <0x0  0x1>;
+        #address-cells = <3>;
+        #size-cells = <2>;
+        device_type = "pci";
+        ranges = <0x02000000 0x0 0x00000000 0x0 0xf5000000 0x0 0x2000000>;
+        num-lanes = <1>;
+        #interrupt-cells = <1>;
+        interrupts = <0 283 4>;
+        interrupt-names = "msi";
+        interrupt-map-mask = <0xf800 0 0 7>;
+        interrupt-map = <0x0 0 0 1 &gic GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>,
+                        <0x0 0 0 2 &gic GIC_SPI 283 IRQ_TYPE_LEVEL_HIGH>,
+                        <0x0 0 0 3 &gic GIC_SPI 284 IRQ_TYPE_LEVEL_HIGH>,
+                        <0x0 0 0 4 &gic GIC_SPI 285 IRQ_TYPE_LEVEL_HIGH>;
+        reset-gpios = <&gpio7 0 0 >, <&gpio25 2 0 >,
+                      <&gpio3 1 0 >, <&gpio27 4 0 >;
+      };
+    };

There's nothing preventing this driver to be loaded as a module. So, change its config from bool to tristate.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Quoted 4 lines · Mauro Carvalho Chehab

There's nothing preventing this driver to be loaded as a module. So, change its config from bool to tristate.

Quoted 1 line

Signed-off-by Mauro Carvalho Chehab <[email protected]>

No objections from me, but I wonder if you would also consider making the module removable. It's currently marked as 'builtin_platform_driver', so you can load but not remove it. Rob has done some bug fixes that make it possible to remove similar drivers, so it's probably not much work here either.