[PATCH v5 0/8] Add support for Hikey 970 PCIe

[PATCH v5 0/8] Add support for Hikey 970 PCIe

[Mauro Carvalho Chehab]

As requested by Rob Herring, this series split the PHY part into a separate driver.
Then, it adds support for Kirin 970 on a single patch.

With this change, the PHY-specific device tree bindings for Kirin 960 moved
to its own PHY properties.

Tested on Hikey970:

  $ lspci
  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)

  $ ethtool enp6s0
  Settings for enp6s0:
	Supported ports: [ TP	 MII ]
	Supported link modes:   10baseT/Half 10baseT/Full
	                        100baseT/Half 100baseT/Full
	                        1000baseT/Half 1000baseT/Full
	Supported pause frame use: Symmetric Receive-only
	Supports auto-negotiation: Yes
	Supported FEC modes: Not reported
	Advertised link modes:  10baseT/Half 10baseT/Full
	                        100baseT/Half 100baseT/Full
	                        1000baseT/Half 1000baseT/Full
	Advertised pause frame use: Symmetric Receive-only
	Advertised auto-negotiation: Yes
	Advertised FEC modes: Not reported
	Link partner advertised link modes:  10baseT/Half 10baseT/Full
	                                     100baseT/Half 100baseT/Full
	Link partner advertised pause frame use: Symmetric Receive-only
	Link partner advertised auto-negotiation: Yes
	Link partner advertised FEC modes: Not reported
	Speed: 100Mb/s
	Duplex: Full
	Auto-negotiation: on
	master-slave cfg: preferred slave
	master-slave status: slave
	Port: Twisted Pair
	PHYAD: 0
	Transceiver: external
	MDI-X: Unknown
  netlink error: Operation not permitted
	Link detected: yes

Partially tested on Hikey 960[1]:

  $ lspci
  00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3660 (rev 01)

[1] The Hikey 960 doesn't come with any internal PCIe device.
    Its hardware supports just an external device via a M.2 slot that
    doesn't support SATA. I ordered a NVMe device to test, but the vendor
    is currently out of supply. It should take 3-4 weeks to arrive here. I'll
    run an extra test on it once it arrives.

---

v5:
- added "static" to hi3670_pcie_get_eyeparam() declaration on patch 6/8

v4:

- dropped the DTS patch, as it depends on a PMIC-related patch series;
- minor changes at the patch description;
- HiKey and HiSilicon are now using the preferred CamelCase format.


Mauro Carvalho Chehab (8):
  dt-bindings: phy: Add bindings for HiKey 960 PCIe PHY
  dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY
  dt-bindings: PCI: kirin: Fix compatible string
  dt-bindings: PCI: kirin: Drop PHY properties
  phy: HiSilicon: Add driver for Kirin 960 PCIe PHY
  phy: HiSilicon: add driver for Kirin 970 PCIe PHY
  PCI: kirin: Drop the PHY logic from the driver
  PCI: kirin: Use regmap for APB registers

 .../devicetree/bindings/pci/kirin-pcie.txt    |  21 +-
 .../phy/hisilicon,phy-hi3660-pcie.yaml        |  82 ++
 .../phy/hisilicon,phy-hi3670-pcie.yaml        | 101 ++
 arch/arm64/boot/dts/hisilicon/hi3660.dtsi     |  29 +-
 drivers/pci/controller/dwc/pcie-kirin.c       | 298 ++----
 drivers/phy/hisilicon/Kconfig                 |  20 +
 drivers/phy/hisilicon/Makefile                |   2 +
 drivers/phy/hisilicon/phy-hi3660-pcie.c       | 325 +++++++
 drivers/phy/hisilicon/phy-hi3670-pcie.c       | 892 ++++++++++++++++++
 9 files changed, 1500 insertions(+), 270 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/phy/hisilicon,phy-hi3660-pcie.yaml
 create mode 100644 Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
 create mode 100644 drivers/phy/hisilicon/phy-hi3660-pcie.c
 create mode 100644 drivers/phy/hisilicon/phy-hi3670-pcie.c

-- 
2.31.1

[PATCH v5 1/8] dt-bindings: phy: Add bindings for HiKey 960 PCIe PHY

[Mauro Carvalho Chehab]

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

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 .../phy/hisilicon,phy-hi3660-pcie.yaml        | 82 +++++++++++++++++++
 1 file changed, 82 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/hisilicon,phy-hi3660-pcie.yaml

diff --git a/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3660-pcie.yaml b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3660-pcie.yaml
new file mode 100644
index 000000000000..81c93e76cef4
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3660-pcie.yaml
@@ -0,0 +1,82 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/hisilicon,phy-hi3660-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HiSilicon Kirin960 PCIe PHY
+
+maintainers:
+  - Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
+
+description: |+
+  Bindings for PCIe PHY on HiSilicon Kirin 960.
+
+properties:
+  compatible:
+    const: hisilicon,hi960-pcie-phy
+
+  "#phy-cells":
+    const: 0
+
+  reg:
+    maxItems: 1
+    description: PHY Control registers
+
+  reg-names:
+    const: phy
+
+  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: pcie_phy_ref
+      - const: pcie_aux
+      - const: pcie_apb_phy
+      - const: pcie_apb_sys
+      - const: pcie_aclk
+
+  reset-gpios:
+    description: PCI PERST reset GPIO
+
+required:
+  - "#phy-cells"
+  - compatible
+  - reg
+  - reg-names
+  - clocks
+  - clock-names
+  - reset-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/hi3660-clock.h>
+
+    bus {
+      #address-cells = <2>;
+      #size-cells = <2>;
+      pcie_phy: pcie-phy@f3f2000 {
+        compatible = "hisilicon,hi960-pcie-phy";
+        reg = <0x0 0xf3f20000 0x0 0x40000>;
+        reg-names = "phy";
+        clocks = <&crg_ctrl HI3660_PCIEPHY_REF>,
+                 <&crg_ctrl HI3660_CLK_GATE_PCIEAUX>,
+                 <&crg_ctrl HI3660_PCLK_GATE_PCIE_PHY>,
+                 <&crg_ctrl HI3660_PCLK_GATE_PCIE_SYS>,
+                 <&crg_ctrl HI3660_ACLK_GATE_PCIE>;
+        clock-names = "pcie_phy_ref", "pcie_aux",
+                 "pcie_apb_phy", "pcie_apb_sys",
+                 "pcie_aclk";
+        reset-gpios = <&gpio11 1 0 >;
+        #phy-cells = <0>;
+      };
+    };
+...
-- 
2.31.1

Re: [PATCH v5 1/8] dt-bindings: phy: Add bindings for HiKey 960 PCIe PHY

[Rob Herring]

On Tue, Jul 13, 2021 at 08:28:34AM +0200, Mauro Carvalho Chehab wrote:
> Document the bindings for HiKey 960 (hi3660) PCIe PHY
> interface, supported via the pcie-kirin driver.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  .../phy/hisilicon,phy-hi3660-pcie.yaml        | 82 +++++++++++++++++++
>  1 file changed, 82 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/hisilicon,phy-hi3660-pcie.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3660-pcie.yaml b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3660-pcie.yaml
> new file mode 100644
> index 000000000000..81c93e76cef4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3660-pcie.yaml
> @@ -0,0 +1,82 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/hisilicon,phy-hi3660-pcie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HiSilicon Kirin960 PCIe PHY
> +
> +maintainers:
> +  - Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> +
> +description: |+
> +  Bindings for PCIe PHY on HiSilicon Kirin 960.
> +
> +properties:
> +  compatible:
> +    const: hisilicon,hi960-pcie-phy
> +
> +  "#phy-cells":
> +    const: 0
> +
> +  reg:
> +    maxItems: 1
> +    description: PHY Control registers
> +
> +  reg-names:
> +    const: phy

You don't really need reg-names with only 1.

> +
> +  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: pcie_phy_ref
> +      - const: pcie_aux
> +      - const: pcie_apb_phy
> +      - const: pcie_apb_sys
> +      - const: pcie_aclk

'pcie_' is redundant. Drop.

> +
> +  reset-gpios:
> +    description: PCI PERST reset GPIO

maxItems: 1

Though this belongs in the PCIE node.

> +
> +required:
> +  - "#phy-cells"
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks
> +  - clock-names
> +  - reset-gpios
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/hi3660-clock.h>
> +
> +    bus {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +      pcie_phy: pcie-phy@f3f2000 {
> +        compatible = "hisilicon,hi960-pcie-phy";
> +        reg = <0x0 0xf3f20000 0x0 0x40000>;
> +        reg-names = "phy";
> +        clocks = <&crg_ctrl HI3660_PCIEPHY_REF>,
> +                 <&crg_ctrl HI3660_CLK_GATE_PCIEAUX>,
> +                 <&crg_ctrl HI3660_PCLK_GATE_PCIE_PHY>,
> +                 <&crg_ctrl HI3660_PCLK_GATE_PCIE_SYS>,
> +                 <&crg_ctrl HI3660_ACLK_GATE_PCIE>;
> +        clock-names = "pcie_phy_ref", "pcie_aux",
> +                 "pcie_apb_phy", "pcie_apb_sys",
> +                 "pcie_aclk";
> +        reset-gpios = <&gpio11 1 0 >;
> +        #phy-cells = <0>;
> +      };
> +    };
> +...
> -- 
> 2.31.1
> 
> 

[PATCH v5 2/8] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY

[Mauro Carvalho Chehab]

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

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 .../phy/hisilicon,phy-hi3670-pcie.yaml        | 101 ++++++++++++++++++
 1 file changed, 101 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml

diff --git a/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
new file mode 100644
index 000000000000..976ab6fe7b0a
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
@@ -0,0 +1,101 @@
+# 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 <mchehab+huawei@kernel.org>
+
+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
+
+  reg-names:
+    const: phy
+
+  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: pcie_phy_ref
+      - const: pcie_aux
+      - const: pcie_apb_phy
+      - const: pcie_apb_sys
+      - const: pcie_aclk
+
+  reset-gpios:
+    description: PCI PERST reset GPIOs
+    maxItems: 4
+
+  clkreq-gpios:
+    description: Clock request GPIOs
+    maxItems: 3
+
+  hisilicon,eye-diagram-param:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description: Eye diagram for phy.
+
+required:
+  - "#phy-cells"
+  - compatible
+  - reg
+  - reg-names
+  - clocks
+  - clock-names
+  - reset-gpios
+  - clkreq-gpios
+  - hisilicon,eye-diagram-param
+  - phy-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/hi3670-clock.h>
+
+    bus {
+      #address-cells = <2>;
+      #size-cells = <2>;
+      pcie_phy: pcie-phy@fc000000 {
+        compatible = "hisilicon,hi970-pcie-phy";
+        reg = <0x0 0xfc000000 0x0 0x80000>;
+        reg-names = "phy";
+        #phy-cells = <0>;
+        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 = "pcie_phy_ref", "pcie_aux",
+                      "pcie_apb_phy", "pcie_apb_sys", "pcie_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 >;
+        hisilicon,eye-diagram-param = <0xFFFFFFFF 0xFFFFFFFF
+                                       0xFFFFFFFF 0xFFFFFFFF 0xFFFFFFFF>;
+      };
+    };
+...
-- 
2.31.1

Re: [PATCH v5 2/8] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY

[Rob Herring]

On Tue, Jul 13, 2021 at 08:28:35AM +0200, Mauro Carvalho Chehab wrote:
> Document the bindings for HiKey 970 (hi3670) PCIe PHY
> interface, supported via the pcie-kirin driver.

Same comments on this one.

> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  .../phy/hisilicon,phy-hi3670-pcie.yaml        | 101 ++++++++++++++++++
>  1 file changed, 101 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
> new file mode 100644
> index 000000000000..976ab6fe7b0a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
> @@ -0,0 +1,101 @@
> +# 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 <mchehab+huawei@kernel.org>
> +
> +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
> +
> +  reg-names:
> +    const: phy
> +
> +  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: pcie_phy_ref
> +      - const: pcie_aux
> +      - const: pcie_apb_phy
> +      - const: pcie_apb_sys
> +      - const: pcie_aclk
> +
> +  reset-gpios:
> +    description: PCI PERST reset GPIOs
> +    maxItems: 4

Hiding the 4 ports in the phy?

> +
> +  clkreq-gpios:
> +    description: Clock request GPIOs
> +    maxItems: 3
> +
> +  hisilicon,eye-diagram-param:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description: Eye diagram for phy.

Is there a size to this array?

> +
> +required:
> +  - "#phy-cells"
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks
> +  - clock-names
> +  - reset-gpios
> +  - clkreq-gpios
> +  - hisilicon,eye-diagram-param
> +  - phy-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/hi3670-clock.h>
> +
> +    bus {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +      pcie_phy: pcie-phy@fc000000 {
> +        compatible = "hisilicon,hi970-pcie-phy";
> +        reg = <0x0 0xfc000000 0x0 0x80000>;
> +        reg-names = "phy";
> +        #phy-cells = <0>;
> +        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 = "pcie_phy_ref", "pcie_aux",
> +                      "pcie_apb_phy", "pcie_apb_sys", "pcie_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 >;
> +        hisilicon,eye-diagram-param = <0xFFFFFFFF 0xFFFFFFFF
> +                                       0xFFFFFFFF 0xFFFFFFFF 0xFFFFFFFF>;
> +      };
> +    };
> +...
> -- 
> 2.31.1
> 
> 

Re: [PATCH v5 2/8] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY

[Mauro Carvalho Chehab]

Em Tue, 13 Jul 2021 20:26:49 -0600
Rob Herring <robh@kernel.org> escreveu:

> On Tue, Jul 13, 2021 at 08:28:35AM +0200, Mauro Carvalho Chehab wrote:

> > +  reset-gpios:
> > +    description: PCI PERST reset GPIOs
> > +    maxItems: 4  
> 
> Hiding the 4 ports in the phy?

Rob,

I'm not trying to hide anything.

There are several differences with regards to how PERST# is handled between
HiKey 960 and HiKey 970.

From hardware perspective, you can see the schematics of both boards:

	https://github.com/96boards/documentation/raw/master/consumer/hikey/hikey960/hardware-docs/HiKey960_SoC_Reference_Manual.pdf
	https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf

The 960 PHY has the SoC directly connected to a PCIE M.2 slot 
(model 10130616) without any external bridge chipset. It uses a single 
GPIO (GPIO 089) for the PERST# signal, connected via a voltage converter
(from 1.8V to 3.3V).

	$ lspci
	00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3660 (rev 01)

The 970 PHY has an external PCI bridge chipset (PLX Technology PEX 8606).
Besides the bridge, the hardware comes with an Ethernet PCI adapter, a
M.2 slot and a mini-PCIe connector. Each one with its own PERST# signal,
mapped to different GPIO pins, and each one using its own voltage
converter.

	$ lspci
	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)

On other words, there are 4 GPIOs mapped to different PERST# pins in
the hardware:

- GPIO 56 is connected to the PERST# pin at PEX 8606;
- GPIO 25 is connected to the PERST# pin at the M.2 slot;
- GPIO 220 is connected to the PERST# pin at the PCIe mini slot;
- GPIO 203 is connected to the PERST# pin at the Ethernet chipset.

Maybe due to different electrical requirements, the hardware design
use different GPIOs instead of feeding them altogether.

Anyway, the fact is that the PHY on 970 has 4 different GPIOs that are
need in order for the hardware to work. and this is specific to this
particular PHY.

Now, from software perspective, the power on sequence on Hikey 960
finishes sending PERST# signal to the M.2 slot:

	static int hi3660_pcie_phy_power_on(struct phy *generic_phy)
	{
...
		/* perst assert Endpoint */
		if (!gpio_request(phy->gpio_id_reset, "pcie_perst")) {
			usleep_range(REF_2_PERST_MIN, REF_2_PERST_MAX);
			ret = gpio_direction_output(phy->gpio_id_reset, 1);
			if (ret)
				goto disable_clks;
			usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX);
			return 0;
		}

	disable_clks:
		kirin_pcie_clk_ctrl(phy, false);
		return ret;
	}
	
The 970 PHY, however, sends PERST# signal in the middle of the power
on sequence, as, after sending reset, it needs to wait for the hardware
to stabilize, in order to setup an eye diagram at the PHY:

	static int hi3670_pcie_phy_power_on(struct phy *generic_phy)
	{
...
		/* perst assert Endpoints */
		usleep_range(21000, 23000);
		for (i = 0; i < phy->n_gpio_resets; i++) {
			ret = gpio_direction_output(phy->gpio_id_reset[i], 1);
			if (ret)
				return ret;
		}
		usleep_range(10000, 11000);

		ret = is_pipe_clk_stable(phy);
		if (!ret)
			goto disable_clks;

		hi3670_pcie_set_eyeparam(phy);

		ret = hi3670_pcie_noc_power(phy, false);
		if (ret)
			goto disable_clks;

		return 0;

	disable_clks:
		kirin_pcie_clk_ctrl(phy, false);
		return ret;
	}

IMO, it makes a lot more sense to map this on DT as part of the
PHY and not as part of the PCIe, but no matter how it is mapped,
this PHY still requires 4 GPIOs for PERST#.

Thanks,
Mauro

Re: [PATCH v5 2/8] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY

[Rob Herring]

On Wed, Jul 14, 2021 at 1:14 AM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> Em Tue, 13 Jul 2021 20:26:49 -0600
> Rob Herring <robh@kernel.org> escreveu:
>
> > On Tue, Jul 13, 2021 at 08:28:35AM +0200, Mauro Carvalho Chehab wrote:
>
> > > +  reset-gpios:
> > > +    description: PCI PERST reset GPIOs
> > > +    maxItems: 4
> >
> > Hiding the 4 ports in the phy?
>
> Rob,
>
> I'm not trying to hide anything.
>
> There are several differences with regards to how PERST# is handled between
> HiKey 960 and HiKey 970.
>
> From hardware perspective, you can see the schematics of both boards:
>
>         https://github.com/96boards/documentation/raw/master/consumer/hikey/hikey960/hardware-docs/HiKey960_SoC_Reference_Manual.pdf
>         https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf
>
> The 960 PHY has the SoC directly connected to a PCIE M.2 slot
> (model 10130616) without any external bridge chipset. It uses a single
> GPIO (GPIO 089) for the PERST# signal, connected via a voltage converter
> (from 1.8V to 3.3V).
>
>         $ lspci
>         00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3660 (rev 01)
>
> The 970 PHY has an external PCI bridge chipset (PLX Technology PEX 8606).
> Besides the bridge, the hardware comes with an Ethernet PCI adapter, a
> M.2 slot and a mini-PCIe connector. Each one with its own PERST# signal,
> mapped to different GPIO pins, and each one using its own voltage
> converter.
>
>         $ lspci
>         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)
>
> On other words, there are 4 GPIOs mapped to different PERST# pins in
> the hardware:
>
> - GPIO 56 is connected to the PERST# pin at PEX 8606;
> - GPIO 25 is connected to the PERST# pin at the M.2 slot;
> - GPIO 220 is connected to the PERST# pin at the PCIe mini slot;
> - GPIO 203 is connected to the PERST# pin at the Ethernet chipset.
>
> Maybe due to different electrical requirements, the hardware design
> use different GPIOs instead of feeding them altogether.
>
> Anyway, the fact is that the PHY on 970 has 4 different GPIOs that are
> need in order for the hardware to work. and this is specific to this
> particular PHY.

This hierarchy could be done on any board. It has nothing to do with the PHY.

> Now, from software perspective, the power on sequence on Hikey 960
> finishes sending PERST# signal to the M.2 slot:
>
>         static int hi3660_pcie_phy_power_on(struct phy *generic_phy)
>         {
> ...
>                 /* perst assert Endpoint */
>                 if (!gpio_request(phy->gpio_id_reset, "pcie_perst")) {
>                         usleep_range(REF_2_PERST_MIN, REF_2_PERST_MAX);
>                         ret = gpio_direction_output(phy->gpio_id_reset, 1);
>                         if (ret)
>                                 goto disable_clks;
>                         usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX);
>                         return 0;
>                 }
>
>         disable_clks:
>                 kirin_pcie_clk_ctrl(phy, false);
>                 return ret;
>         }
>
> The 970 PHY, however, sends PERST# signal in the middle of the power
> on sequence, as, after sending reset, it needs to wait for the hardware
> to stabilize, in order to setup an eye diagram at the PHY:
>
>         static int hi3670_pcie_phy_power_on(struct phy *generic_phy)
>         {
> ...
>                 /* perst assert Endpoints */
>                 usleep_range(21000, 23000);
>                 for (i = 0; i < phy->n_gpio_resets; i++) {
>                         ret = gpio_direction_output(phy->gpio_id_reset[i], 1);
>                         if (ret)
>                                 return ret;
>                 }
>                 usleep_range(10000, 11000);
>
>                 ret = is_pipe_clk_stable(phy);
>                 if (!ret)
>                         goto disable_clks;
>
>                 hi3670_pcie_set_eyeparam(phy);
>
>                 ret = hi3670_pcie_noc_power(phy, false);
>                 if (ret)
>                         goto disable_clks;
>
>                 return 0;
>
>         disable_clks:
>                 kirin_pcie_clk_ctrl(phy, false);
>                 return ret;
>         }
>
> IMO, it makes a lot more sense to map this on DT as part of the
> PHY and not as part of the PCIe, but no matter how it is mapped,
> this PHY still requires 4 GPIOs for PERST#.

It does not because PERST# control is part of PCIe for every other driver.

Rob

Re: [PATCH v5 2/8] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY

[Mauro Carvalho Chehab]

Em Wed, 14 Jul 2021 08:17:05 -0600
Rob Herring <robh@kernel.org> escreveu:

> On Wed, Jul 14, 2021 at 1:14 AM Mauro Carvalho Chehab
> <mchehab+huawei@kernel.org> wrote:
> >
> > Em Tue, 13 Jul 2021 20:26:49 -0600
> > Rob Herring <robh@kernel.org> escreveu:
> >  
> > > On Tue, Jul 13, 2021 at 08:28:35AM +0200, Mauro Carvalho Chehab wrote:  
> >  
> > > > +  reset-gpios:
> > > > +    description: PCI PERST reset GPIOs
> > > > +    maxItems: 4  
> > >
> > > Hiding the 4 ports in the phy?  
> >
> > Rob,
> >
> > I'm not trying to hide anything.
> >
> > There are several differences with regards to how PERST# is handled between
> > HiKey 960 and HiKey 970.
> >
> > From hardware perspective, you can see the schematics of both boards:
> >
> >         https://github.com/96boards/documentation/raw/master/consumer/hikey/hikey960/hardware-docs/HiKey960_SoC_Reference_Manual.pdf
> >         https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf
> >
> > The 960 PHY has the SoC directly connected to a PCIE M.2 slot
> > (model 10130616) without any external bridge chipset. It uses a single
> > GPIO (GPIO 089) for the PERST# signal, connected via a voltage converter
> > (from 1.8V to 3.3V).
> >
> >         $ lspci
> >         00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3660 (rev 01)
> >
> > The 970 PHY has an external PCI bridge chipset (PLX Technology PEX 8606).
> > Besides the bridge, the hardware comes with an Ethernet PCI adapter, a
> > M.2 slot and a mini-PCIe connector. Each one with its own PERST# signal,
> > mapped to different GPIO pins, and each one using its own voltage
> > converter.
> >
> >         $ lspci
> >         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)
> >
> > On other words, there are 4 GPIOs mapped to different PERST# pins in
> > the hardware:
> >
> > - GPIO 56 is connected to the PERST# pin at PEX 8606;
> > - GPIO 25 is connected to the PERST# pin at the M.2 slot;
> > - GPIO 220 is connected to the PERST# pin at the PCIe mini slot;
> > - GPIO 203 is connected to the PERST# pin at the Ethernet chipset.
> >
> > Maybe due to different electrical requirements, the hardware design
> > use different GPIOs instead of feeding them altogether.
> >
> > Anyway, the fact is that the PHY on 970 has 4 different GPIOs that are
> > need in order for the hardware to work. and this is specific to this
> > particular PHY.  
> 
> This hierarchy could be done on any board. It has nothing to do with the PHY.

True, but right now, the pci-bus.yaml prevents it, as it allows just 
one reset GPIO[1]:

  reset-gpios:
    description: GPIO controlled connection to PERST# signal
    maxItems: 1

[1] https://github.com/robherring/dt-schema/blob/master/schemas/pci/pci-bus.yaml

If the schema will be changed to allow multiple reset-gpios, It should
be possible to keep this at the pcie-kirin.c driver with something like:

	static int kirin_pcie_power_on(struct kirin_pcie *kirin_pcie)
	{
		int ret;

		ret = phy_init(kirin_pcie->phy);
		if (ret)
			return ret;

		ret = phy_power_on(kirin_pcie->phy);

                /* perst assert Endpoints */
                usleep_range(21000, 23000);
                for (i = 0; i < phy->n_gpio_resets; i++) {
                        ret = gpio_direction_output(phy->gpio_id_reset[i], 1);
                        if (ret) {
				phy_power_off(kirin_pcie->phy);
                                return ret;
			}
                }
                usleep_range(10000, 11000);

		return phy_reset(kirin_pcie->phy);
	}

This would work for both 960 - where phy_reset() is not needed,
and for 970, where it would set the eye diagram for the PHY.

Should I send a patch for pci-bus.yaml via github?

Thanks,
Mauro

Re: [PATCH v5 2/8] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY

[Manivannan Sadhasivam]

Hi Mauro,

On Wed, Jul 14, 2021 at 09:14:35AM +0200, Mauro Carvalho Chehab wrote:
> Em Tue, 13 Jul 2021 20:26:49 -0600
> Rob Herring <robh@kernel.org> escreveu:
> 
> > On Tue, Jul 13, 2021 at 08:28:35AM +0200, Mauro Carvalho Chehab wrote:
> 
> > > +  reset-gpios:
> > > +    description: PCI PERST reset GPIOs
> > > +    maxItems: 4  
> > 
> > Hiding the 4 ports in the phy?
> 
> Rob,
> 
> I'm not trying to hide anything.
> 
> There are several differences with regards to how PERST# is handled between
> HiKey 960 and HiKey 970.
> 
> From hardware perspective, you can see the schematics of both boards:
> 
> 	https://github.com/96boards/documentation/raw/master/consumer/hikey/hikey960/hardware-docs/HiKey960_SoC_Reference_Manual.pdf
> 	https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf
> 
> The 960 PHY has the SoC directly connected to a PCIE M.2 slot 
> (model 10130616) without any external bridge chipset. It uses a single 
> GPIO (GPIO 089) for the PERST# signal, connected via a voltage converter
> (from 1.8V to 3.3V).
> 
> 	$ lspci
> 	00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3660 (rev 01)
> 
> The 970 PHY has an external PCI bridge chipset (PLX Technology PEX 8606).
> Besides the bridge, the hardware comes with an Ethernet PCI adapter, a
> M.2 slot and a mini-PCIe connector. Each one with its own PERST# signal,
> mapped to different GPIO pins, and each one using its own voltage
> converter.
> 
> 	$ lspci
> 	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)
> 
> On other words, there are 4 GPIOs mapped to different PERST# pins in
> the hardware:
> 
> - GPIO 56 is connected to the PERST# pin at PEX 8606;
> - GPIO 25 is connected to the PERST# pin at the M.2 slot;
> - GPIO 220 is connected to the PERST# pin at the PCIe mini slot;
> - GPIO 203 is connected to the PERST# pin at the Ethernet chipset.
> 
> Maybe due to different electrical requirements, the hardware design
> use different GPIOs instead of feeding them altogether.
> 
> Anyway, the fact is that the PHY on 970 has 4 different GPIOs that are
> need in order for the hardware to work. and this is specific to this
> particular PHY.
> 

I'm not sure about this. That fact that the PCIe device's PERST# signal
wired to different GPIOs doesn't mean that those GPIOs belong to the PHY.
Those GPIOs should be independent of the PCIe core controlled manually
by the driver.

I think this issue is somewhat similar to the one we are dealing on the
Qcom platforms [1] where each PCIe device uses a different GPIO and voltage
config to operate. And those need to be active for the link training to
succeed.

So perhaps we should aim for a common solution? The GPIO and voltage
layout should be described in DT for each port exposed by the SoC/board.

Thanks,
Mani

[1] https://lkml.org/lkml/2021/6/21/1524

Re: [PATCH v5 2/8] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY

[Mauro Carvalho Chehab]

Hi Mani,

Em Wed, 14 Jul 2021 23:12:25 +0530
Manivannan Sadhasivam <mani@kernel.org> escreveu:

> Hi Mauro,
> 
> On Wed, Jul 14, 2021 at 09:14:35AM +0200, Mauro Carvalho Chehab wrote:
> > Em Tue, 13 Jul 2021 20:26:49 -0600
> > Rob Herring <robh@kernel.org> escreveu:
> >   
> > > On Tue, Jul 13, 2021 at 08:28:35AM +0200, Mauro Carvalho Chehab wrote:  
> >   
> > > > +  reset-gpios:
> > > > +    description: PCI PERST reset GPIOs
> > > > +    maxItems: 4    
> > > 
> > > Hiding the 4 ports in the phy?  
> > 
> > Rob,
> > 
> > I'm not trying to hide anything.
> > 
> > There are several differences with regards to how PERST# is handled between
> > HiKey 960 and HiKey 970.
> > 
> > From hardware perspective, you can see the schematics of both boards:
> > 
> > 	https://github.com/96boards/documentation/raw/master/consumer/hikey/hikey960/hardware-docs/HiKey960_SoC_Reference_Manual.pdf
> > 	https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf
> > 
> > The 960 PHY has the SoC directly connected to a PCIE M.2 slot 
> > (model 10130616) without any external bridge chipset. It uses a single 
> > GPIO (GPIO 089) for the PERST# signal, connected via a voltage converter
> > (from 1.8V to 3.3V).
> > 
> > 	$ lspci
> > 	00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3660 (rev 01)
> > 
> > The 970 PHY has an external PCI bridge chipset (PLX Technology PEX 8606).
> > Besides the bridge, the hardware comes with an Ethernet PCI adapter, a
> > M.2 slot and a mini-PCIe connector. Each one with its own PERST# signal,
> > mapped to different GPIO pins, and each one using its own voltage
> > converter.
> > 
> > 	$ lspci
> > 	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)
> > 
> > On other words, there are 4 GPIOs mapped to different PERST# pins in
> > the hardware:
> > 
> > - GPIO 56 is connected to the PERST# pin at PEX 8606;
> > - GPIO 25 is connected to the PERST# pin at the M.2 slot;
> > - GPIO 220 is connected to the PERST# pin at the PCIe mini slot;
> > - GPIO 203 is connected to the PERST# pin at the Ethernet chipset.
> > 
> > Maybe due to different electrical requirements, the hardware design
> > use different GPIOs instead of feeding them altogether.
> > 
> > Anyway, the fact is that the PHY on 970 has 4 different GPIOs that are
> > need in order for the hardware to work. and this is specific to this
> > particular PHY.
> >   
> 
> I'm not sure about this. That fact that the PCIe device's PERST# signal
> wired to different GPIOs doesn't mean that those GPIOs belong to the PHY.
> Those GPIOs should be independent of the PCIe core controlled manually
> by the driver.
> 
> I think this issue is somewhat similar to the one we are dealing on the
> Qcom platforms [1] where each PCIe device uses a different GPIO and voltage
> config to operate. And those need to be active for the link training to
> succeed.
> 
> So perhaps we should aim for a common solution? The GPIO and voltage
> layout should be described in DT for each port exposed by the SoC/board.

It doesn't seem to be the same case. In the case of Hikey 970[1], it uses
one pullup for each GPIO to convert from 1V8 to the voltage needed by
each chipset. There's no regulators envolved: the output voltage is
fixed. From the diagram, it seems that PEX8606 uses 2V5 (via U2602), while 
the other 3 GPIOs (M.2, PCIe mini and Ethernet) use 3V3. Interesting
enough, the layout uses a separate gate chip for Ethernet (U3107),
while both M.2 and PCIe mini use two ports at the same gate chip (U2801).

[1] https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf

I can think on a couple of reasons why the hardware designers opted to
use 4 different GPIOs:

- the PCIe external bridge required a lower voltage;
- the current drained by each output port could be different;
- the chips used by the pullup could have some sort of protection to
  minimize the risk of damaging the hardware due to a bad contact
  and/or short circuit (for M.2 and PCIe mini connectors).

In any case, from DT perspective, the only thing that makes sense to
expose are the 4 GPIOs used by the PERST# signal, as the output voltages
and max currents are fixed.

Conceptually, IMO, such pullup logic can be considered as part of 
the physical layer, but, as Rob pointed, all PCIe devices need to
have PERST#. So, they could be just mapped as gpio-resets at the
PCIe OF node.

In summary, IMO, mapping those 4 GPIOs via DT can be done by
either:

1. Place all of them at the PCIe OF node;
2. Place all of them at the PHY OF node;
3. Place one of them at the PCIe, and the other three at PHY.

IMO, (3) is messier. So, I would go either for (1) or (2). Between them,
I don't have any preferences, as both would work.

Right now, pci-bus.yaml doesn't allow (1), as it limits reset-gpios
to a maximum of 1 item, but this is easily fixable, but it requires
a patch against https://github.com/devicetree-org/dt-schema to raise
MaxItems to at least 4.

Rob,

Among the above alternatives, what do you prefer?

Thanks,
Mauro

Re: [PATCH v5 2/8] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY

[Mauro Carvalho Chehab]

Hi Rob/Bjorn,

I rebased my repository in a way that the PHY for Kirin 960 won't
be touched. As I explained before:

	https://lore.kernel.org/lkml/20210716132208.3cd8f404@coco.lan/

the only way of doing that without modifying the PHY core and doing ugly
hacks is to not split Kirin 960 PHY from the pcie-kirin driver. 

So, the approach I took was to make the core code at the pcie-kirin driver
generic, ensuring that the internal PHY code is self-contained.

Then, I added support for using a PHY from drivers/phy. This way, the DT 
schema for Kirin 960 won't be touched. The patch series is at:

	https://github.com/mchehab/linux/commits/pcie-alternate

Now, this is the only pending issue on my TODO list:

Em Thu, 15 Jul 2021 08:37:09 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> > > > > +  reset-gpios:
> > > > > +    description: PCI PERST reset GPIOs
> > > > > +    maxItems: 4      
> > > > 
> > > > Hiding the 4 ports in the phy?    
> > > 
> > > Rob,
> > > 
> > > I'm not trying to hide anything.
> > > 
> > > There are several differences with regards to how PERST# is handled between
> > > HiKey 960 and HiKey 970.
> > > 
> > > From hardware perspective, you can see the schematics of both boards:
> > > 
> > > 	https://github.com/96boards/documentation/raw/master/consumer/hikey/hikey960/hardware-docs/HiKey960_SoC_Reference_Manual.pdf
> > > 	https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf
> > > 
> > > The 960 PHY has the SoC directly connected to a PCIE M.2 slot 
> > > (model 10130616) without any external bridge chipset. It uses a single 
> > > GPIO (GPIO 089) for the PERST# signal, connected via a voltage converter
> > > (from 1.8V to 3.3V).
> > > 
> > > 	$ lspci
> > > 	00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3660 (rev 01)
> > > 
> > > The 970 PHY has an external PCI bridge chipset (PLX Technology PEX 8606).
> > > Besides the bridge, the hardware comes with an Ethernet PCI adapter, a
> > > M.2 slot and a mini-PCIe connector. Each one with its own PERST# signal,
> > > mapped to different GPIO pins, and each one using its own voltage
> > > converter.
> > > 
> > > 	$ lspci
> > > 	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)
> > > 
> > > On other words, there are 4 GPIOs mapped to different PERST# pins in
> > > the hardware:
> > > 
> > > - GPIO 56 is connected to the PERST# pin at PEX 8606;
> > > - GPIO 25 is connected to the PERST# pin at the M.2 slot;
> > > - GPIO 220 is connected to the PERST# pin at the PCIe mini slot;
> > > - GPIO 203 is connected to the PERST# pin at the Ethernet chipset.
> > > 
> > > Maybe due to different electrical requirements, the hardware design
> > > use different GPIOs instead of feeding them altogether.
> > > 
> > > Anyway, the fact is that the PHY on 970 has 4 different GPIOs that are
> > > need in order for the hardware to work. and this is specific to this
> > > particular PHY.
> > >     
> > 
> > I'm not sure about this. That fact that the PCIe device's PERST# signal
> > wired to different GPIOs doesn't mean that those GPIOs belong to the PHY.
> > Those GPIOs should be independent of the PCIe core controlled manually
> > by the driver.
> > 
> > I think this issue is somewhat similar to the one we are dealing on the
> > Qcom platforms [1] where each PCIe device uses a different GPIO and voltage
> > config to operate. And those need to be active for the link training to
> > succeed.
> > 
> > So perhaps we should aim for a common solution? The GPIO and voltage
> > layout should be described in DT for each port exposed by the SoC/board.  
> 
> It doesn't seem to be the same case. In the case of Hikey 970[1], it uses
> one pullup for each GPIO to convert from 1V8 to the voltage needed by
> each chipset. There's no regulators envolved: the output voltage is
> fixed. From the diagram, it seems that PEX8606 uses 2V5 (via U2602), while 
> the other 3 GPIOs (M.2, PCIe mini and Ethernet) use 3V3. Interesting
> enough, the layout uses a separate gate chip for Ethernet (U3107),
> while both M.2 and PCIe mini use two ports at the same gate chip (U2801).
> 
> [1] https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf
> 
> I can think on a couple of reasons why the hardware designers opted to
> use 4 different GPIOs:
> 
> - the PCIe external bridge required a lower voltage;
> - the current drained by each output port could be different;
> - the chips used by the pullup could have some sort of protection to
>   minimize the risk of damaging the hardware due to a bad contact
>   and/or short circuit (for M.2 and PCIe mini connectors).
> 
> In any case, from DT perspective, the only thing that makes sense to
> expose are the 4 GPIOs used by the PERST# signal, as the output voltages
> and max currents are fixed.
> 
> Conceptually, IMO, such pullup logic can be considered as part of 
> the physical layer, but, as Rob pointed, all PCIe devices need to
> have PERST#. So, they could be just mapped as gpio-resets at the
> PCIe OF node.
> 
> In summary, IMO, mapping those 4 GPIOs via DT can be done by
> either:
> 
> 1. Place all of them at the PCIe OF node;
> 2. Place all of them at the PHY OF node;
> 3. Place one of them at the PCIe, and the other three at PHY.
> 
> IMO, (3) is messier. So, I would go either for (1) or (2). Between them,
> I don't have any preferences, as both would work.
> 
> Right now, pci-bus.yaml doesn't allow (1), as it limits reset-gpios
> to a maximum of 1 item, but this is easily fixable, but it requires
> a patch against https://github.com/devicetree-org/dt-schema to raise
> MaxItems to at least 4.
> 
> Rob,
> 
> Among the above alternatives, what do you prefer?

At the patch series I mentioned, I'm keeping the PERST# signals as
part of the PHY layer, just because DT currently produces a warning
if maxItems > 1 for gpio-resets for PCI bus.

In the case it was decided to allow the PCI bus to have more than
one PERST# pin, I already sent a patch to DT-schema:

	https://github.com/devicetree-org/dt-schema/pull/56

But I'll need to change the patch series to consider such
approach, if this will be the case.

So, please let me know how do you prefer to proceed with that.

Thanks!
Mauro

Re: [PATCH v5 2/8] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY

[Mauro Carvalho Chehab]

Hi Mani,

Em Wed, 14 Jul 2021 23:12:25 +0530
Manivannan Sadhasivam <mani@kernel.org> escreveu:

> I'm not sure about this. That fact that the PCIe device's PERST# signal
> wired to different GPIOs doesn't mean that those GPIOs belong to the PHY.
> Those GPIOs should be independent of the PCIe core controlled manually
> by the driver.
> 
> I think this issue is somewhat similar to the one we are dealing on the
> Qcom platforms [1] where each PCIe device uses a different GPIO and voltage
> config to operate. And those need to be active for the link training to
> succeed.
> 
> So perhaps we should aim for a common solution? The GPIO and voltage
> layout should be described in DT for each port exposed by the SoC/board.
> 
> Thanks,
> Mani
> 
> [1] https://lkml.org/lkml/2021/6/21/1524

After re-visiting this issue, I'm starting to think that this should
be mapped as something similar to:

	pcie@xxxx {
...
		slot {
			slot#1 {
				// clock, power supply, reset pins, etc
			}
			slot#2 {
				// clock, power supply, reset pins, etc
			}
...
		}
	};

E. g. placing each specific PCIe device requirement inside the pcie
or phy, as it should be up to the driver to initialize each PCIe 
child-specific requirements when the hardware is ready for that.

---

A longer explanation why this should be initialized during PHY
power on sequence:

On my tests with Kirin 970, there are some steps to be done before
enabling the clocks and sending PERST# signals, plus some extra
steps to run after PERST# is sent to all devices.

While playing with PHY split, I noticed that Linux and/or the SoC
is very sensitive to an specific probing order. If such order is
not followed, an ARM SError happens and the Kernel panics
with something similar to:

  [    1.837458] SError Interrupt on CPU0, code 0xbf000002 -- SError
  [    1.837462] CPU: 0 PID: 74 Comm: kworker/0:1 Not tainted 5.8.0+ #205
  [    1.837463] Hardware name: HiKey970 (DT)
  [    1.837465] Workqueue: events deferred_probe_work_func
  [    1.837467] pstate: 20000005 (nzCv daif -PAN -UAO BTYPE=--)
  [    1.837468] pc : _raw_spin_unlock_irqrestore+0x18/0x50
  [    1.837469] lr : regmap_unlock_spinlock+0x14/0x20
...
  [    1.837507] Kernel panic - not syncing: Asynchronous SError Interrupt


One example is with regards to the clocks required for the PCIe
to work:

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

If them aren't initialized at the expected order, the Kernel
hangs. The same applies to the slot-specific clocks.

So, basically, the driver needs to initialize them on this
sequence:

	1. PHY ref clock;
	2. APB sys and phy clock;
	3. aclk and aux_clk;
	<some settings at the PHY hardware>
	4. slot-specific clocks.

failing to follow a valid power-on sequence crashes the Kernel.


Thanks,
Mauro

[PATCH v5 3/8] dt-bindings: PCI: kirin: Fix compatible string

[Mauro Carvalho Chehab]

The pcie-kirin driver doesn't declare a hisilicon,kirin-pcie.
Also, remove the useless comment after the description, as other
compat will be supported by the same driver in the future.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 Documentation/devicetree/bindings/pci/kirin-pcie.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/kirin-pcie.txt b/Documentation/devicetree/bindings/pci/kirin-pcie.txt
index 6bbe43818ad5..71cac2b74002 100644
--- a/Documentation/devicetree/bindings/pci/kirin-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/kirin-pcie.txt
@@ -9,7 +9,7 @@ Additional properties are described here:
 
 Required properties
 - compatible:
-	"hisilicon,kirin960-pcie" for PCIe of Kirin960 SoC
+	"hisilicon,kirin960-pcie"
 - reg: Should contain rc_dbi, apb, phy, config registers location and length.
 - reg-names: Must include the following entries:
   "dbi": controller configuration registers;
@@ -23,7 +23,7 @@ Optional properties:
 Example based on kirin960:
 
 	pcie@f4000000 {
-		compatible = "hisilicon,kirin-pcie";
+		compatible = "hisilicon,kirin960-pcie";
 		reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>,
 		      <0x0 0xf3f20000 0x0 0x40000>, <0x0 0xF4000000 0 0x2000>;
 		reg-names = "dbi","apb","phy", "config";
-- 
2.31.1

Re: [PATCH v5 3/8] dt-bindings: PCI: kirin: Fix compatible string

[Rob Herring]

On Tue, 13 Jul 2021 08:28:36 +0200, Mauro Carvalho Chehab wrote:
> The pcie-kirin driver doesn't declare a hisilicon,kirin-pcie.
> Also, remove the useless comment after the description, as other
> compat will be supported by the same driver in the future.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  Documentation/devicetree/bindings/pci/kirin-pcie.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

[PATCH v5 4/8] dt-bindings: PCI: kirin: Drop PHY properties

[Mauro Carvalho Chehab]

There are several properties there that belong to the PHY
interface. Drop them, as a new binding file will describe
the PHY properties for Kirin 960.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 .../devicetree/bindings/pci/kirin-pcie.txt       | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/kirin-pcie.txt b/Documentation/devicetree/bindings/pci/kirin-pcie.txt
index 71cac2b74002..a93a8cfa1afb 100644
--- a/Documentation/devicetree/bindings/pci/kirin-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/kirin-pcie.txt
@@ -10,13 +10,11 @@ Additional properties are described here:
 Required properties
 - compatible:
 	"hisilicon,kirin960-pcie"
-- reg: Should contain rc_dbi, apb, phy, config registers location and length.
+- reg: Should contain rc_dbi, apb, config registers location and length.
 - reg-names: Must include the following entries:
   "dbi": controller configuration registers;
   "apb": apb Ctrl register defined by Kirin;
-  "phy": apb PHY register defined by Kirin;
   "config": PCIe configuration space registers.
-- reset-gpios: The GPIO to generate PCIe PERST# assert and deassert signal.
 
 Optional properties:
 
@@ -25,8 +23,8 @@ Example based on kirin960:
 	pcie@f4000000 {
 		compatible = "hisilicon,kirin960-pcie";
 		reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>,
-		      <0x0 0xf3f20000 0x0 0x40000>, <0x0 0xF4000000 0 0x2000>;
-		reg-names = "dbi","apb","phy", "config";
+		      <0x0 0xF4000000 0 0x2000>;
+		reg-names = "dbi","apb", "config";
 		bus-range = <0x0  0x1>;
 		#address-cells = <3>;
 		#size-cells = <2>;
@@ -39,12 +37,4 @@ Example based on kirin960:
 				<0x0 0 0 2 &gic 0 0 0  283 4>,
 				<0x0 0 0 3 &gic 0 0 0  284 4>,
 				<0x0 0 0 4 &gic 0 0 0  285 4>;
-		clocks = <&crg_ctrl HI3660_PCIEPHY_REF>,
-			 <&crg_ctrl HI3660_CLK_GATE_PCIEAUX>,
-			 <&crg_ctrl HI3660_PCLK_GATE_PCIE_PHY>,
-			 <&crg_ctrl HI3660_PCLK_GATE_PCIE_SYS>,
-			 <&crg_ctrl HI3660_ACLK_GATE_PCIE>;
-		clock-names = "pcie_phy_ref", "pcie_aux",
-			      "pcie_apb_phy", "pcie_apb_sys", "pcie_aclk";
-		reset-gpios = <&gpio11 1 0 >;
 	};
-- 
2.31.1

Re: [PATCH v5 4/8] dt-bindings: PCI: kirin: Drop PHY properties

[Rob Herring]

On Tue, Jul 13, 2021 at 08:28:37AM +0200, Mauro Carvalho Chehab wrote:
> There are several properties there that belong to the PHY
> interface. Drop them, as a new binding file will describe
> the PHY properties for Kirin 960.

Folks are okay with an incompatible change on hikey960?

> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  .../devicetree/bindings/pci/kirin-pcie.txt       | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/kirin-pcie.txt b/Documentation/devicetree/bindings/pci/kirin-pcie.txt
> index 71cac2b74002..a93a8cfa1afb 100644
> --- a/Documentation/devicetree/bindings/pci/kirin-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/kirin-pcie.txt
> @@ -10,13 +10,11 @@ Additional properties are described here:
>  Required properties
>  - compatible:
>  	"hisilicon,kirin960-pcie"
> -- reg: Should contain rc_dbi, apb, phy, config registers location and length.
> +- reg: Should contain rc_dbi, apb, config registers location and length.
>  - reg-names: Must include the following entries:
>    "dbi": controller configuration registers;
>    "apb": apb Ctrl register defined by Kirin;
> -  "phy": apb PHY register defined by Kirin;
>    "config": PCIe configuration space registers.
> -- reset-gpios: The GPIO to generate PCIe PERST# assert and deassert signal.
>  
>  Optional properties:
>  
> @@ -25,8 +23,8 @@ Example based on kirin960:
>  	pcie@f4000000 {
>  		compatible = "hisilicon,kirin960-pcie";
>  		reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>,
> -		      <0x0 0xf3f20000 0x0 0x40000>, <0x0 0xF4000000 0 0x2000>;
> -		reg-names = "dbi","apb","phy", "config";
> +		      <0x0 0xF4000000 0 0x2000>;
> +		reg-names = "dbi","apb", "config";
>  		bus-range = <0x0  0x1>;
>  		#address-cells = <3>;
>  		#size-cells = <2>;
> @@ -39,12 +37,4 @@ Example based on kirin960:
>  				<0x0 0 0 2 &gic 0 0 0  283 4>,
>  				<0x0 0 0 3 &gic 0 0 0  284 4>,
>  				<0x0 0 0 4 &gic 0 0 0  285 4>;
> -		clocks = <&crg_ctrl HI3660_PCIEPHY_REF>,
> -			 <&crg_ctrl HI3660_CLK_GATE_PCIEAUX>,
> -			 <&crg_ctrl HI3660_PCLK_GATE_PCIE_PHY>,
> -			 <&crg_ctrl HI3660_PCLK_GATE_PCIE_SYS>,
> -			 <&crg_ctrl HI3660_ACLK_GATE_PCIE>;
> -		clock-names = "pcie_phy_ref", "pcie_aux",
> -			      "pcie_apb_phy", "pcie_apb_sys", "pcie_aclk";
> -		reset-gpios = <&gpio11 1 0 >;
>  	};
> -- 
> 2.31.1
> 
> 

Re: [PATCH v5 4/8] dt-bindings: PCI: kirin: Drop PHY properties

[Mauro Carvalho Chehab]

Em Tue, 13 Jul 2021 20:28:49 -0600
Rob Herring <robh@kernel.org> escreveu:

> On Tue, Jul 13, 2021 at 08:28:37AM +0200, Mauro Carvalho Chehab wrote:
> > There are several properties there that belong to the PHY
> > interface. Drop them, as a new binding file will describe
> > the PHY properties for Kirin 960.  
> 
> Folks are okay with an incompatible change on hikey960?

I hope so ;-)

I mean, it should be easy to add a backward-compatible code that
would make the PHY driver to use the pci-bus old schema if there's
no PHY entry at DT.

However, this is not enough, as the PHY driver won't be loaded/probed
without at least this at hi3660.dtsi:

	pcie_phy: pcie-phy@f3f2000 {
		compatible = "hisilicon,hi960-pcie-phy";
	};


So, some (probably ugly) hack would be needed at pcie-kirin, in order
to make it to manually load and probe the PHY driver, if it
founds (for instance) "phy" reg-name as a pcie-kirin property.

Thanks,
Mauro

Re: [PATCH v5 4/8] dt-bindings: PCI: kirin: Drop PHY properties

[Mauro Carvalho Chehab]

Em Tue, 13 Jul 2021 20:28:49 -0600
Rob Herring <robh@kernel.org> escreveu:

> On Tue, Jul 13, 2021 at 08:28:37AM +0200, Mauro Carvalho Chehab wrote:
> > There are several properties there that belong to the PHY
> > interface. Drop them, as a new binding file will describe
> > the PHY properties for Kirin 960.  
> 
> Folks are okay with an incompatible change on hikey960?

Accepting an incompatible change here seems the right thing to do.

Another possibility would be to create a "pcie-kirin-with-phy" driver
that would be identical to the existing one, except for the absence
of a PHY and using a different compatible string.

-

Long answer:

There aren't many alternatives here, if we want to split the PHY out of
the driver, as you requested.

I've been scratching my head in order to find a way that would keep
the Hikey960 a separate PHY driver, with a proper DT schema, but
capable of also parse the original DT schema.

See, making the phy driver parse the PCIE-based OF-node data is 
trivial (I have already a patch doing that), but it will require at
least some DT schema additions, in order to add a pcie_phy node[1]:

<snip>
diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
index e0eca598af1f..6aaa2f966d74 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
@@ -1001,6 +1001,11 @@ spi3: spi@ff3b3000 {
                        status = "disabled";
                };
 
+               pcie_phy: pcie-phy@f3f2000 {
+                       compatible = "hisilicon,hi960-pcie-phy";
+                       #phy-cells = <0>;
+               };
+
                pcie@f4000000 {
                        compatible = "hisilicon,kirin960-pcie";
                        reg = <0x0 0xf4000000 0x0 0x1000>,
@@ -1012,6 +1017,7 @@ pcie@f4000000 {
                        #address-cells = <3>;
                        #size-cells = <2>;
                        device_type = "pci";
+                       phys = <&pcie_phy>;
                        ranges = <0x02000000 0x0 0x00000000
                                  0x0 0xf6000000
                                  0x0 0x02000000>;
</snip>

[1] or, alternatively, the pcie-kirin driver would need to dynamically
    populate DT with the above, as some ACPI drivers do when the
    firmware is broken.

Without a PHY representation at the DT schema, the PHY driver won't 
be recognized by pcie-kirin.

See, even if the pcie-kirin driver would be changed to register
the PHY without DT, with:

	phy = devm_of_phy_get(dev, NULL, "hi3660_pcie_phy");

The phy_get() implementation will internally ignore a non-DT PHY,
as internally, it uses of_property_match_string() if the caller driver
has of_node:

	struct phy *phy_get(struct device *dev, const char *string)
	{
		int index = 0;
		struct phy *phy;
		struct device_link *link;

		if (dev->of_node) {
			if (string)
				index = of_property_match_string(dev->of_node, "phy-names",
					string);
			else
				index = 0;
			phy = _of_phy_get(dev->of_node, index);
		} else {
			if (string == NULL) {
				dev_WARN(dev, "missing string\n");
				return ERR_PTR(-EINVAL);
			}
			phy = phy_find(dev, string);
		}

Thanks,
Mauro

[PATCH v5 6/8] phy: HiSilicon: add driver for Kirin 970 PCIe PHY

[Mauro Carvalho Chehab]

The Kirin 970 PHY is somewhat similar to the Kirin 960, but it
does a lot more. Add the needed bits for PCIe to start working on
HiKey 970 boards.

Co-developed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 .../devicetree/bindings/pci/kirin-pcie.txt    |   1 +
 drivers/pci/controller/dwc/pcie-kirin.c       |   1 +
 drivers/phy/hisilicon/Kconfig                 |  10 +
 drivers/phy/hisilicon/Makefile                |   1 +
 drivers/phy/hisilicon/phy-hi3670-pcie.c       | 892 ++++++++++++++++++
 5 files changed, 905 insertions(+)
 create mode 100644 drivers/phy/hisilicon/phy-hi3670-pcie.c

diff --git a/Documentation/devicetree/bindings/pci/kirin-pcie.txt b/Documentation/devicetree/bindings/pci/kirin-pcie.txt
index a93a8cfa1afb..585aadfeafd1 100644
--- a/Documentation/devicetree/bindings/pci/kirin-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/kirin-pcie.txt
@@ -10,6 +10,7 @@ Additional properties are described here:
 Required properties
 - compatible:
 	"hisilicon,kirin960-pcie"
+	"hisilicon,kirin970-pcie"
 - reg: Should contain rc_dbi, apb, config registers location and length.
 - reg-names: Must include the following entries:
   "dbi": controller configuration registers;
diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index 026fd1e42a55..7b0f87535960 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -472,6 +472,7 @@ static int kirin_pcie_probe(struct platform_device *pdev)
 
 static const struct of_device_id kirin_pcie_match[] = {
 	{ .compatible = "hisilicon,kirin960-pcie" },
+	{ .compatible = "hisilicon,kirin970-pcie" },
 	{},
 };
 
diff --git a/drivers/phy/hisilicon/Kconfig b/drivers/phy/hisilicon/Kconfig
index 30b7f1187965..65e0ff6e85ad 100644
--- a/drivers/phy/hisilicon/Kconfig
+++ b/drivers/phy/hisilicon/Kconfig
@@ -43,6 +43,16 @@ config PHY_HI3670_USB
 
 	  To compile this driver as a module, choose M here.
 
+config PHY_HI3670_PCIE
+	tristate "hi3670 PCIe PHY support"
+	depends on (ARCH_HISI && ARM64) || COMPILE_TEST
+	select GENERIC_PHY
+	select MFD_SYSCON
+	help
+	  Enable this to support the HiSilicon hi3670 PCIe PHY.
+
+	  To compile this driver as a module, choose M here.
+
 config PHY_HISTB_COMBPHY
 	tristate "HiSilicon STB SoCs COMBPHY support"
 	depends on (ARCH_HISI && ARM64) || COMPILE_TEST
diff --git a/drivers/phy/hisilicon/Makefile b/drivers/phy/hisilicon/Makefile
index 3c3d70dd7469..2670f2f382e1 100644
--- a/drivers/phy/hisilicon/Makefile
+++ b/drivers/phy/hisilicon/Makefile
@@ -3,6 +3,7 @@ obj-$(CONFIG_PHY_HI6220_USB)		+= phy-hi6220-usb.o
 obj-$(CONFIG_PHY_HI3660_USB)		+= phy-hi3660-usb3.o
 obj-$(CONFIG_PHY_HI3660_PCIE)		+= phy-hi3660-pcie.o
 obj-$(CONFIG_PHY_HI3670_USB)		+= phy-hi3670-usb3.o
+obj-$(CONFIG_PHY_HI3670_PCIE)		+= phy-hi3670-pcie.o
 obj-$(CONFIG_PHY_HISTB_COMBPHY)		+= phy-histb-combphy.o
 obj-$(CONFIG_PHY_HISI_INNO_USB2)	+= phy-hisi-inno-usb2.o
 obj-$(CONFIG_PHY_HIX5HD2_SATA)		+= phy-hix5hd2-sata.o
diff --git a/drivers/phy/hisilicon/phy-hi3670-pcie.c b/drivers/phy/hisilicon/phy-hi3670-pcie.c
new file mode 100644
index 000000000000..3e3a15d9006d
--- /dev/null
+++ b/drivers/phy/hisilicon/phy-hi3670-pcie.c
@@ -0,0 +1,892 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCIe phy driver for Kirin 970
+ *
+ * Copyright (C) 2017 HiSilicon Electronics Co., Ltd.
+ *		https://www.huawei.com
+ * Copyright (C) 2021 Huawei Technologies Co., Ltd.
+ *		https://www.huawei.com
+ *
+ * Authors:
+ *	Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
+ *	Manivannan Sadhasivam <mani@kernel.org>
+ *
+ * Based on:
+ * 	https://lore.kernel.org/lkml/4c9d6581478aa966698758c0420933f5defab4dd.1612335031.git.mchehab+huawei@kernel.org/
+ */
+
+#include <linux/clk.h>
+#include <linux/gpio.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of_gpio.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define AXI_CLK_FREQ			207500000
+#define REF_CLK_FREQ			100000000
+
+/* PCIe CTRL registers */
+#define SOC_PCIECTRL_CTRL0_ADDR   0x000
+#define SOC_PCIECTRL_CTRL1_ADDR   0x004
+#define SOC_PCIECTRL_CTRL7_ADDR   0x01c
+#define SOC_PCIECTRL_CTRL12_ADDR  0x030
+#define SOC_PCIECTRL_CTRL20_ADDR  0x050
+#define SOC_PCIECTRL_CTRL21_ADDR  0x054
+#define SOC_PCIECTRL_STATE0_ADDR  0x400
+
+/* PCIe PHY registers */
+#define SOC_PCIEPHY_CTRL0_ADDR    0x000
+#define SOC_PCIEPHY_CTRL1_ADDR    0x004
+#define SOC_PCIEPHY_CTRL2_ADDR    0x008
+#define SOC_PCIEPHY_CTRL3_ADDR    0x00c
+#define SOC_PCIEPHY_CTRL38_ADDR   0x0098
+#define SOC_PCIEPHY_STATE0_ADDR   0x400
+
+#define PCIE_LINKUP_ENABLE            (0x8020)
+#define PCIE_ELBI_SLV_DBI_ENABLE      (0x1 << 21)
+#define PCIE_LTSSM_ENABLE_BIT         (0x1 << 11)
+#define PCIEPHY_RESET_BIT             (0x1 << 17)
+#define PCIEPHY_PIPE_LINE0_RESET_BIT  (0x1 << 19)
+
+#define PORT_MSI_CTRL_ADDR            0x820
+#define PORT_MSI_CTRL_UPPER_ADDR      0x824
+#define PORT_MSI_CTRL_INT0_ENABLE     0x828
+
+#define EYEPARAM_NOCFG 0xFFFFFFFF
+#define RAWLANEN_DIG_PCS_XF_TX_OVRD_IN_1 0x3001
+#define SUP_DIG_LVL_OVRD_IN 0xf
+#define LANEN_DIG_ASIC_TX_OVRD_IN_1 0x1002
+#define LANEN_DIG_ASIC_TX_OVRD_IN_2 0x1003
+
+/* hi3670 pciephy register */
+#define SOC_PCIEPHY_MMC1PLL_CTRL1  0xc04
+#define SOC_PCIEPHY_MMC1PLL_CTRL16 0xC40
+#define SOC_PCIEPHY_MMC1PLL_CTRL17 0xC44
+#define SOC_PCIEPHY_MMC1PLL_CTRL20 0xC50
+#define SOC_PCIEPHY_MMC1PLL_CTRL21 0xC54
+#define SOC_PCIEPHY_MMC1PLL_STAT0  0xE00
+
+#define CRGPERIPH_PEREN12   0x470
+#define CRGPERIPH_PERDIS12  0x474
+#define CRGPERIPH_PCIECTRL0 0x800
+
+/* define ie,oe cfg */
+#define IO_IE_EN_HARD_BYPASS         (0x1 << 27)
+#define IO_OE_EN_HARD_BYPASS         (0x1 << 11)
+#define IO_HARD_CTRL_DEBOUNCE_BYPASS (0x1 << 10)
+#define IO_OE_GT_MODE                (0x2 << 7)
+#define DEBOUNCE_WAITCFG_IN          (0xf << 20)
+#define DEBOUNCE_WAITCFG_OUT         (0xf << 13)
+
+/* noc power domain */
+#define NOC_POWER_IDLEREQ_1 0x38c
+#define NOC_POWER_IDLE_1    0x394
+#define NOC_PW_MASK         0x10000
+#define NOC_PW_SET_BIT      0x1
+
+/* Number of GPIOs required by PHY */
+#define MAX_GPIO_RESETS		4
+#define MAX_GPIO_CLKREQ		3
+#define NUM_EYEPARAM		5
+
+/* info located in sysctrl */
+#define SCTRL_PCIE_CMOS_OFFSET	0x60
+#define SCTRL_PCIE_CMOS_BIT	0x10
+#define SCTRL_PCIE_ISO_OFFSET	0x44
+#define SCTRL_PCIE_ISO_BIT	0x30
+#define SCTRL_PCIE_HPCLK_OFFSET	0x190
+#define SCTRL_PCIE_HPCLK_BIT	0x184000
+#define SCTRL_PCIE_OE_OFFSET	0x14a
+#define PCIE_DEBOUNCE_PARAM	0xF0F400
+#define PCIE_OE_BYPASS		(0x3 << 28)
+
+/* peri_crg ctrl */
+#define CRGCTRL_PCIE_ASSERT_OFFSET	0x88
+#define CRGCTRL_PCIE_ASSERT_BIT		0x8c000000
+
+/* Time for delay */
+#define REF_2_PERST_MIN		20000
+#define REF_2_PERST_MAX		25000
+#define PERST_2_ACCESS_MIN	10000
+#define PERST_2_ACCESS_MAX	12000
+#define PIPE_CLK_WAIT_MIN	550
+#define PIPE_CLK_WAIT_MAX	600
+#define TIME_CMOS_MIN		100
+#define TIME_CMOS_MAX		105
+#define TIME_PHY_PD_MIN		10
+#define TIME_PHY_PD_MAX		11
+
+struct hi3670_pcie_phy {
+	struct device	*dev;
+	void __iomem	*base;
+	struct regmap	*apb;
+	struct regmap	*crgctrl;
+	struct regmap	*sysctrl;
+	struct regmap	*pmctrl;
+	struct clk	*apb_sys_clk;
+	struct clk	*apb_phy_clk;
+	struct clk	*phy_ref_clk;
+	struct clk	*pcie_aclk;
+	struct clk	*pcie_aux_clk;
+	int		n_gpio_resets;
+	int		n_gpio_clkreq;
+	int		gpio_id_reset[MAX_GPIO_RESETS];
+	const char	*reset_names[MAX_GPIO_RESETS];
+	int		gpio_id_clkreq[MAX_GPIO_CLKREQ];
+	const char	*clkreq_names[MAX_GPIO_CLKREQ];
+	u32		eye_param[NUM_EYEPARAM];
+};
+
+
+/* Registers in PCIePHY */
+static inline void hi3670_apb_phy_writel(struct hi3670_pcie_phy *phy,
+					 u32 val, u32 reg)
+{
+	writel(val, phy->base + 0x40000 + reg);
+}
+
+static inline u32 hi3670_apb_phy_readl(struct hi3670_pcie_phy *phy, u32 reg)
+{
+	return readl(phy->base + 0x40000 + reg);
+}
+
+static inline void kirin_apb_natural_phy_writel(struct hi3670_pcie_phy *phy,
+						u32 val, u32 reg)
+{
+	writel(val, phy->base + reg * 4);
+}
+
+static inline u32 kirin_apb_natural_phy_readl(struct hi3670_pcie_phy *phy,
+					      u32 reg)
+{
+	return readl(phy->base + reg * 4);
+}
+
+static void hi3670_pcie_phy_oe_enable(struct hi3670_pcie_phy *phy)
+{
+	u32 val;
+
+	regmap_read(phy->sysctrl, SCTRL_PCIE_OE_OFFSET, &val);
+	val |= PCIE_DEBOUNCE_PARAM;
+	val &= ~PCIE_OE_BYPASS;
+	regmap_write(phy->sysctrl, SCTRL_PCIE_OE_OFFSET, val);
+}
+
+static void hi3670_pcie_get_eyeparam(struct hi3670_pcie_phy *phy)
+{
+	struct device *dev = phy->dev;
+	struct device_node *np;
+	int ret, i;
+
+	np = dev->of_node;
+
+	ret = of_property_read_u32_array(np, "hisilicon,eye-diagram-param",
+					 phy->eye_param, NUM_EYEPARAM);
+	if (!ret)
+		return;
+
+	/* There's no optional eye_param property. Set array to default */
+	for (i = 0; i < NUM_EYEPARAM; i++)
+		phy->eye_param[i] = EYEPARAM_NOCFG;
+}
+
+static void hi3670_pcie_set_eyeparam(struct hi3670_pcie_phy *phy)
+{
+	u32 val;
+
+	val = kirin_apb_natural_phy_readl(phy, RAWLANEN_DIG_PCS_XF_TX_OVRD_IN_1);
+
+	if (phy->eye_param[1] != EYEPARAM_NOCFG) {
+		val &= (~0xf00);
+		val |= (phy->eye_param[1] << 8) | (0x1 << 12);
+	}
+	kirin_apb_natural_phy_writel(phy, val, RAWLANEN_DIG_PCS_XF_TX_OVRD_IN_1);
+
+	val = kirin_apb_natural_phy_readl(phy, LANEN_DIG_ASIC_TX_OVRD_IN_2);
+	val &= (~0x1FBF);
+	if (phy->eye_param[2] != EYEPARAM_NOCFG)
+		val |= (phy->eye_param[2]<< 0) | (0x1 << 6);
+
+	if (phy->eye_param[3] != EYEPARAM_NOCFG)
+		val |= (phy->eye_param[3] << 7) | (0x1 << 13);
+
+	kirin_apb_natural_phy_writel(phy, val, LANEN_DIG_ASIC_TX_OVRD_IN_2);
+
+	val = kirin_apb_natural_phy_readl(phy, SUP_DIG_LVL_OVRD_IN);
+	if (phy->eye_param[0] != EYEPARAM_NOCFG) {
+		val &= (~0x1C0);
+		val |= (phy->eye_param[0] << 6) | (0x1 << 9);
+	}
+	kirin_apb_natural_phy_writel(phy, val, SUP_DIG_LVL_OVRD_IN);
+
+	val = kirin_apb_natural_phy_readl(phy, LANEN_DIG_ASIC_TX_OVRD_IN_1);
+	if (phy->eye_param[4] != EYEPARAM_NOCFG) {
+		val &= (~0x7E00);
+		val |= (phy->eye_param[4] << 9) | (0x1 << 15);
+	}
+	kirin_apb_natural_phy_writel(phy, val, LANEN_DIG_ASIC_TX_OVRD_IN_1);
+}
+
+static int hi3670_pcie_gpio_request(struct hi3670_pcie_phy *phy,
+				    struct device *dev)
+{
+	int ret, i;
+
+	for (i = 0; i < phy->n_gpio_resets; i++) {
+		if (!gpio_is_valid(phy->gpio_id_reset[i])) {
+			dev_err(dev, "unable to get a valid %s gpio\n",
+				phy->reset_names[i]);
+			return -ENODEV;
+		}
+
+		ret = devm_gpio_request(dev, phy->gpio_id_reset[i],
+					phy->reset_names[i]);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; i < phy->n_gpio_clkreq; i++) {
+		if (!gpio_is_valid(phy->gpio_id_clkreq[i])) {
+			dev_err(dev, "unable to get a valid %s gpio\n",
+				phy->clkreq_names[i]);
+			return -ENODEV;
+		}
+
+		ret = devm_gpio_request(dev, phy->gpio_id_clkreq[i],
+					phy->clkreq_names[i]);
+		if (ret)
+			return ret;
+	}
+
+	return ret;
+}
+
+static void hi3670_pcie_natural_cfg(struct hi3670_pcie_phy *phy)
+{
+	u32 val;
+
+	/* change 2p mem_ctrl */
+	regmap_write(phy->apb, SOC_PCIECTRL_CTRL20_ADDR, 0x02605550);
+
+	/* pull up sys_aux_pwr_det */
+	regmap_read(phy->apb, SOC_PCIECTRL_CTRL7_ADDR, &val);
+	val |= (0x1 << 10);
+	regmap_write(phy->apb, SOC_PCIECTRL_CTRL7_ADDR, val);
+
+	/* output, pull down */
+	regmap_read(phy->apb, SOC_PCIECTRL_CTRL12_ADDR, &val);
+	val &= ~(0x3 << 2);
+	val |= (0x1 << 1);
+	val &= ~(0x1 << 0);
+	regmap_write(phy->apb, SOC_PCIECTRL_CTRL12_ADDR, val);
+
+	/* Handle phy_reset and lane0_reset to HW */
+	val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_CTRL1_ADDR);
+	val |= PCIEPHY_RESET_BIT;
+	val &= ~PCIEPHY_PIPE_LINE0_RESET_BIT;
+	hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_CTRL1_ADDR);
+
+	/* fix chip bug: TxDetectRx fail */
+	val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_CTRL38_ADDR);
+	val |= (0x1 << 2);
+	hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_CTRL38_ADDR);
+}
+
+static void hi3670_pcie_pll_init(struct hi3670_pcie_phy *phy)
+{
+	u32 val;
+
+	/* choose FNPLL */
+	val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_MMC1PLL_CTRL1);
+	val |= (0x1 << 27);
+	hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_MMC1PLL_CTRL1);
+
+	val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_MMC1PLL_CTRL16);
+	val &= 0xF000FFFF;
+	/* fnpll fbdiv = 0xD0 */
+	val |= (0xd0 << 16);
+	hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_MMC1PLL_CTRL16);
+
+	val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_MMC1PLL_CTRL17);
+	val &= 0xFF000000;
+	/* fnpll fracdiv = 0x555555 */
+	val |= (0x555555 << 0);
+	hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_MMC1PLL_CTRL17);
+
+	val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_MMC1PLL_CTRL20);
+	val &= 0xF5FF88FF;
+	/* fnpll dll_en = 0x1 */
+	val |= (0x1 << 27);
+	/* fnpll postdiv1 = 0x5 */
+	val |= (0x5 << 8);
+	/* fnpll postdiv2 = 0x4 */
+	val |= (0x4 << 12);
+	/* fnpll pll_mode = 0x0 */
+	val &= ~(0x1 << 25);
+	hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_MMC1PLL_CTRL20);
+
+	hi3670_apb_phy_writel(phy, 0x20, SOC_PCIEPHY_MMC1PLL_CTRL21);
+}
+
+static int hi3670_pcie_pll_ctrl(struct hi3670_pcie_phy *phy, bool enable)
+{
+	struct device *dev = phy->dev;
+	u32 val;
+	int time = 200;
+
+	if (enable) {
+		/* pd = 0 */
+		val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_MMC1PLL_CTRL16);
+		val &= ~(0x1 << 0);
+		hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_MMC1PLL_CTRL16);
+
+		val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_MMC1PLL_STAT0);
+
+		/* choose FNPLL */
+		while (!(val & 0x10)) {
+			if (!time) {
+				dev_err(dev, "wait for pll_lock timeout\n");
+				return -1;
+			}
+			time --;
+			udelay(1);
+			val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_MMC1PLL_STAT0);
+		}
+
+		/* pciepll_bp = 0 */
+		val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_MMC1PLL_CTRL20);
+		val &= ~(0x1 << 16);
+		hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_MMC1PLL_CTRL20);
+
+	} else {
+		/* pd = 1 */
+		val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_MMC1PLL_CTRL16);
+		val |= (0x1 << 0);
+		hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_MMC1PLL_CTRL16);
+
+		/* pciepll_bp = 1 */
+		val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_MMC1PLL_CTRL20);
+		val |= (0x1 << 16);
+		hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_MMC1PLL_CTRL20);
+	}
+
+	 return 0;
+}
+
+static void hi3670_pcie_hp_debounce_gt(struct hi3670_pcie_phy *phy, bool open)
+{
+	if (open)
+		/* gt_clk_pcie_hp/gt_clk_pcie_debounce open */
+		regmap_write(phy->crgctrl, CRGPERIPH_PEREN12, 0x9000);
+	else
+		/* gt_clk_pcie_hp/gt_clk_pcie_debounce close */
+		regmap_write(phy->crgctrl, CRGPERIPH_PERDIS12, 0x9000);
+}
+
+static void hi3670_pcie_phyref_gt(struct hi3670_pcie_phy *phy, bool open)
+{
+	unsigned int val;
+
+	regmap_read(phy->crgctrl, CRGPERIPH_PCIECTRL0, &val);
+
+	if (open)
+		val &= ~(0x1 << 1); //enable hard gt mode
+	else
+		val |= (0x1 << 1); //disable hard gt mode
+
+	regmap_write(phy->crgctrl, CRGPERIPH_PCIECTRL0, val);
+
+	/* disable soft gt mode */
+	regmap_write(phy->crgctrl, CRGPERIPH_PERDIS12, 0x4000);
+}
+
+static void hi3670_pcie_oe_ctrl(struct hi3670_pcie_phy *phy, bool en_flag)
+{
+	unsigned int val;
+
+	regmap_read(phy->crgctrl , CRGPERIPH_PCIECTRL0, &val);
+
+	/* set ie cfg */
+	val |= IO_IE_EN_HARD_BYPASS;
+
+	/* set oe cfg */
+	val &= ~IO_HARD_CTRL_DEBOUNCE_BYPASS;
+
+	/* set phy_debounce in&out time */
+	val |= (DEBOUNCE_WAITCFG_IN | DEBOUNCE_WAITCFG_OUT);
+
+	/* select oe_gt_mode */
+	val |= IO_OE_GT_MODE;
+
+	if (en_flag)
+		val &= ~IO_OE_EN_HARD_BYPASS;
+	else
+		val |= IO_OE_EN_HARD_BYPASS;
+
+	regmap_write(phy->crgctrl, CRGPERIPH_PCIECTRL0, val);
+}
+
+static void hi3670_pcie_ioref_gt(struct hi3670_pcie_phy *phy, bool open)
+{
+	unsigned int val;
+
+	if (open) {
+		regmap_write(phy->apb, SOC_PCIECTRL_CTRL21_ADDR, 0x20000070);
+
+		hi3670_pcie_oe_ctrl(phy, true);
+
+		/* en hard gt mode */
+		regmap_read(phy->crgctrl, CRGPERIPH_PCIECTRL0, &val);
+		val &= ~(0x1 << 0);
+		regmap_write(phy->crgctrl, CRGPERIPH_PCIECTRL0, val);
+
+		/* disable soft gt mode */
+		regmap_write(phy->crgctrl, CRGPERIPH_PERDIS12, 0x2000);
+
+	} else {
+		/* disable hard gt mode */
+		regmap_read(phy->crgctrl, CRGPERIPH_PCIECTRL0, &val);
+		val |= (0x1 << 0);
+		regmap_write(phy->crgctrl, CRGPERIPH_PCIECTRL0, val);
+
+		/* disable soft gt mode */
+		regmap_write(phy->crgctrl, CRGPERIPH_PERDIS12, 0x2000);
+
+		hi3670_pcie_oe_ctrl(phy, false);
+       }
+}
+
+static int hi3670_pcie_allclk_ctrl(struct hi3670_pcie_phy *phy, bool clk_on)
+{
+	struct device *dev = phy->dev;
+	u32 val;
+	int ret = 0;
+
+	if (!clk_on)
+		goto close_clocks;
+
+	/* choose 100MHz clk src: Bit[8]==1 pad, Bit[8]==0 pll */
+	val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_CTRL1_ADDR);
+	val &= ~(0x1 << 8);
+	hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_CTRL1_ADDR);
+
+	hi3670_pcie_pll_init(phy);
+
+	ret = hi3670_pcie_pll_ctrl(phy, true);
+	if (ret) {
+		dev_err(dev, "Failed to enable pll\n");
+		return -1;
+	}
+	hi3670_pcie_hp_debounce_gt(phy, true);
+	hi3670_pcie_phyref_gt(phy, true);
+	hi3670_pcie_ioref_gt(phy, true);
+
+	ret = clk_set_rate(phy->pcie_aclk, AXI_CLK_FREQ);
+	if (ret) {
+		dev_err(dev, "Failed to set rate\n");
+		goto close_clocks;
+	}
+
+	return 0;
+
+close_clocks:
+	hi3670_pcie_ioref_gt(phy, false);
+	hi3670_pcie_phyref_gt(phy, false);
+	hi3670_pcie_hp_debounce_gt(phy, false);
+
+	hi3670_pcie_pll_ctrl(phy, false);
+
+	return ret;
+}
+
+static bool is_pipe_clk_stable(struct hi3670_pcie_phy *phy)
+{
+	struct device *dev = phy->dev;
+	u32 val;
+	u32 time = 100;
+	u32 pipe_clk_stable = 0x1 << 19;
+
+	val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_STATE0_ADDR);
+	while (val & pipe_clk_stable) {
+		mdelay(1);
+		if (time == 0) {
+			dev_err(dev, "PIPE clk is not stable\n");
+			return false;
+		}
+		time--;
+		val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_STATE0_ADDR);
+	}
+
+	return true;
+}
+
+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);
+
+	time = 100;
+	regmap_read(phy->pmctrl, NOC_POWER_IDLE_1, &val);
+	while((val & NOC_PW_SET_BIT) != rst) {
+		udelay(10);
+		if (!time) {
+			dev_err(dev, "Failed to reverse noc power-status\n");
+			return -1;
+		}
+		time--;
+		regmap_read(phy->pmctrl, NOC_POWER_IDLE_1, &val);
+	}
+
+	return 0;
+}
+
+static int hi3670_pcie_get_apb(struct hi3670_pcie_phy *phy)
+{
+	struct device_node *pcie_port;
+	struct device *dev = phy->dev;
+	struct device *pcie_dev;
+
+	pcie_port = of_get_child_by_name(dev->parent->of_node, "pcie");
+	if (!pcie_port) {
+		dev_err(dev, "no pcie node found in %s\n",
+			dev->parent->of_node->full_name);
+		return -ENODEV;
+	}
+
+	pcie_dev = bus_find_device_by_of_node(&platform_bus_type, pcie_port);
+	if (!pcie_dev) {
+                dev_err(dev, "Didn't find pcie device\n");
+                return -ENODEV;
+        }
+
+        /*
+	 * We might just use NULL instead of the APB name, as the
+	 * pcie-kirin currently registers directly just one regmap (although
+	 * the DWC driver register other regmaps).
+	 *
+	 * Yet, it sounds safer to warrant that it will be accessing the
+	 * right regmap. So, let's use the named version.
+	 */
+	phy->apb = dev_get_regmap(pcie_dev, "kirin_pcie_apb");
+	if (!phy->apb) {
+		dev_err(dev, "Failed to get APB regmap\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+
+static int kirin_pcie_clk_ctrl(struct hi3670_pcie_phy *phy, bool enable)
+{
+	int ret = 0;
+
+	if (!enable)
+		goto close_clk;
+
+	ret = clk_set_rate(phy->phy_ref_clk, REF_CLK_FREQ);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(phy->phy_ref_clk);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(phy->apb_sys_clk);
+	if (ret)
+		goto apb_sys_fail;
+
+	ret = clk_prepare_enable(phy->apb_phy_clk);
+	if (ret)
+		goto apb_phy_fail;
+
+	ret = clk_prepare_enable(phy->pcie_aclk);
+	if (ret)
+		goto aclk_fail;
+
+	ret = clk_prepare_enable(phy->pcie_aux_clk);
+	if (ret)
+		goto aux_clk_fail;
+
+	return 0;
+
+close_clk:
+	clk_disable_unprepare(phy->pcie_aux_clk);
+aux_clk_fail:
+	clk_disable_unprepare(phy->pcie_aclk);
+aclk_fail:
+	clk_disable_unprepare(phy->apb_phy_clk);
+apb_phy_fail:
+	clk_disable_unprepare(phy->apb_sys_clk);
+apb_sys_fail:
+	clk_disable_unprepare(phy->phy_ref_clk);
+
+	return ret;
+}
+
+static int hi3670_pcie_phy_init(struct phy *generic_phy)
+{
+	struct hi3670_pcie_phy *phy = phy_get_drvdata(generic_phy);
+	struct device *dev = phy->dev;
+	int ret;
+
+	/*
+	 * The code under hi3670_pcie_get_apb() need to access the
+	 * DWC APB registers. So, get them from
+	 * the pcie driver's regmap (see pcie-kirin regmap).
+	 *
+	 * Such kind of resource can only be obtained during the PCIe
+	 * power_on sequence, as the code inside pcie-kirin needs to
+	 * be already probed, as it needs to register the APB regmap.
+	 */
+
+	ret = hi3670_pcie_get_apb(phy);
+	if (ret)
+		return ret;
+
+	/* phy regulator needs to be powered on before calling it */
+	return hi3670_pcie_gpio_request(phy, dev);
+}
+
+static int hi3670_pcie_phy_power_on(struct phy *generic_phy)
+{
+	struct hi3670_pcie_phy *phy = phy_get_drvdata(generic_phy);
+	int val, ret, i;
+
+	/* Power supply for Host */
+	regmap_write(phy->sysctrl,
+		     SCTRL_PCIE_CMOS_OFFSET, SCTRL_PCIE_CMOS_BIT);
+	usleep_range(TIME_CMOS_MIN, TIME_CMOS_MAX);
+
+	hi3670_pcie_phy_oe_enable(phy);
+
+	for (i = 0; i < phy->n_gpio_clkreq; i++) {
+		ret = gpio_direction_output(phy->gpio_id_clkreq[i], 0);
+		if (ret)
+			return ret;
+	}
+
+	ret = kirin_pcie_clk_ctrl(phy, true);
+	if (ret)
+		return ret;
+
+	/* ISO disable, PCIeCtrl, PHY assert and clk gate clear */
+	regmap_write(phy->sysctrl,
+		     SCTRL_PCIE_ISO_OFFSET, SCTRL_PCIE_ISO_BIT);
+	regmap_write(phy->crgctrl,
+		     CRGCTRL_PCIE_ASSERT_OFFSET, CRGCTRL_PCIE_ASSERT_BIT);
+	regmap_write(phy->sysctrl,
+		     SCTRL_PCIE_HPCLK_OFFSET, SCTRL_PCIE_HPCLK_BIT);
+
+	hi3670_pcie_natural_cfg(phy);
+
+	ret = hi3670_pcie_allclk_ctrl(phy, true);
+	if (ret)
+		goto disable_clks;
+
+	/* pull down phy_test_powerdown signal */
+	val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_CTRL0_ADDR);
+	val &= ~(0x1 << 22);
+	hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_CTRL0_ADDR);
+
+	/* deassert controller perst_n */
+	regmap_read(phy->apb, SOC_PCIECTRL_CTRL12_ADDR, &val);
+	val |= (0x1 << 2);
+	regmap_write(phy->apb, SOC_PCIECTRL_CTRL12_ADDR, val);
+	udelay(10);
+
+	/* perst assert Endpoints */
+	usleep_range(21000, 23000);
+	for (i = 0; i < phy->n_gpio_resets; i++) {
+		ret = gpio_direction_output(phy->gpio_id_reset[i], 1);
+		if (ret)
+			return ret;
+	}
+	usleep_range(10000, 11000);
+
+	ret = is_pipe_clk_stable(phy);
+	if (!ret)
+		goto disable_clks;
+
+	hi3670_pcie_set_eyeparam(phy);
+
+	ret = hi3670_pcie_noc_power(phy, false);
+	if (ret)
+		goto disable_clks;
+
+	return 0;
+
+disable_clks:
+	kirin_pcie_clk_ctrl(phy, false);
+	return ret;
+}
+
+static int hi3670_pcie_phy_power_off(struct phy *generic_phy)
+{
+	struct hi3670_pcie_phy *phy = phy_get_drvdata(generic_phy);
+
+	/* Drop power supply for Host */
+	regmap_write(phy->sysctrl, SCTRL_PCIE_CMOS_OFFSET, 0x00);
+
+	kirin_pcie_clk_ctrl(phy, false);
+
+	return 0;
+}
+
+static const struct phy_ops hi3670_phy_ops = {
+	.init		= hi3670_pcie_phy_init,
+	.power_on	= hi3670_pcie_phy_power_on,
+	.power_off	= hi3670_pcie_phy_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static int hi3670_pcie_phy_get_resources(struct hi3670_pcie_phy *phy,
+					 struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	char name[32];
+	int i;
+
+	/* syscon */
+	phy->crgctrl = syscon_regmap_lookup_by_compatible("hisilicon,hi3670-crgctrl");
+	if (IS_ERR(phy->crgctrl))
+		return PTR_ERR(phy->crgctrl);
+
+	phy->sysctrl = syscon_regmap_lookup_by_compatible("hisilicon,hi3670-sctrl");
+	if (IS_ERR(phy->sysctrl))
+		return PTR_ERR(phy->sysctrl);
+
+	phy->pmctrl = syscon_regmap_lookup_by_compatible("hisilicon,hi3670-pmctrl");
+	if (IS_ERR(phy->sysctrl))
+		return PTR_ERR(phy->sysctrl);
+
+	/* clocks */
+	phy->phy_ref_clk = devm_clk_get(dev, "pcie_phy_ref");
+	if (IS_ERR(phy->phy_ref_clk))
+		return PTR_ERR(phy->phy_ref_clk);
+
+	phy->pcie_aux_clk = devm_clk_get(dev, "pcie_aux");
+	if (IS_ERR(phy->pcie_aux_clk))
+		return PTR_ERR(phy->pcie_aux_clk);
+
+	phy->apb_phy_clk = devm_clk_get(dev, "pcie_apb_phy");
+	if (IS_ERR(phy->apb_phy_clk))
+		return PTR_ERR(phy->apb_phy_clk);
+
+	phy->apb_sys_clk = devm_clk_get(dev, "pcie_apb_sys");
+	if (IS_ERR(phy->apb_sys_clk))
+		return PTR_ERR(phy->apb_sys_clk);
+
+	phy->pcie_aclk = devm_clk_get(dev, "pcie_aclk");
+	if (IS_ERR(phy->pcie_aclk))
+		return PTR_ERR(phy->pcie_aclk);
+
+	/* registers */
+	phy->base = devm_platform_ioremap_resource_byname(pdev, "phy");
+	if (IS_ERR(phy->base))
+		return PTR_ERR(phy->base);
+
+	/* perst reset gpios */
+	phy->n_gpio_resets = of_gpio_named_count(np, "reset-gpios");
+	if (phy->n_gpio_resets > MAX_GPIO_RESETS) {
+		dev_err(dev, "Too many GPIO resets!\n");
+		return -EINVAL;
+	}
+	for (i = 0; i < phy->n_gpio_resets; i++) {
+		phy->gpio_id_reset[i] = of_get_named_gpio(dev->of_node,
+							  "reset-gpios", i);
+		if (phy->gpio_id_reset[i] < 0)
+			return phy->gpio_id_reset[i];
+
+		sprintf(name, "pcie_perst_%d", i);
+
+		phy->reset_names[i] = devm_kstrdup_const(dev, name,
+							 GFP_KERNEL);
+		if (!phy->reset_names[i])
+			return -ENOMEM;
+	}
+
+	/* clock request gpios */
+	phy->n_gpio_clkreq = of_gpio_named_count(np, "clkreq-gpios");
+	if (phy->n_gpio_clkreq > MAX_GPIO_CLKREQ) {
+		dev_err(dev, "Too many GPIO clock requests!\n");
+		return -EINVAL;
+	}
+	for (i = 0; i < phy->n_gpio_clkreq; i++) {
+		phy->gpio_id_clkreq[i] = of_get_named_gpio(dev->of_node,
+							   "clkreq-gpios", i);
+		if (phy->gpio_id_clkreq[i] < 0)
+			return phy->gpio_id_clkreq[i];
+
+		sprintf(name, "pcie_clkreq_%d", i);
+		phy->clkreq_names[i] = devm_kstrdup_const(dev, name,
+							  GFP_KERNEL);
+		if (!phy->clkreq_names[i])
+			return -ENOMEM;
+	}
+
+	hi3670_pcie_get_eyeparam(phy);
+
+	return 0;
+}
+
+static int hi3670_pcie_phy_probe(struct platform_device *pdev)
+{
+	struct phy_provider *phy_provider;
+	struct device *dev = &pdev->dev;
+	struct hi3670_pcie_phy *phy;
+	struct phy *generic_phy;
+	int ret;
+
+	phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL);
+	if (!phy)
+		return -ENOMEM;
+
+	phy->dev = dev;
+
+	ret = hi3670_pcie_phy_get_resources(phy, pdev);
+	if (ret)
+		return ret;
+
+	generic_phy = devm_phy_create(dev, dev->of_node, &hi3670_phy_ops);
+	if (IS_ERR(generic_phy)) {
+		dev_err(dev, "failed to create PHY\n");
+		return PTR_ERR(generic_phy);
+	}
+
+	phy_set_drvdata(generic_phy, phy);
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id hi3670_pcie_phy_match[] = {
+	{
+		.compatible = "hisilicon,hi970-pcie-phy",
+	},
+	{},
+};
+
+static struct platform_driver hi3670_pcie_phy_driver = {
+	.probe	= hi3670_pcie_phy_probe,
+	.driver = {
+		.of_match_table	= hi3670_pcie_phy_match,
+		.name		= "hi3670_pcie_phy",
+		.suppress_bind_attrs = true,
+	}
+};
+builtin_platform_driver(hi3670_pcie_phy_driver);
-- 
2.31.1

[PATCH v5 7/8] PCI: kirin: Drop the PHY logic from the driver

[Mauro Carvalho Chehab]

The pcie-kirin PCIe driver contains internally a PHY interface for
a Kirin 960. Drop it and add support for using the new PHY driver
for Kirin 960, updating DT accordingly.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 arch/arm64/boot/dts/hisilicon/hi3660.dtsi |  29 ++-
 drivers/pci/controller/dwc/pcie-kirin.c   | 252 +++-------------------
 2 files changed, 47 insertions(+), 234 deletions(-)

diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
index f1ec87c05842..772bb632814f 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
@@ -995,17 +995,33 @@ spi3: spi@ff3b3000 {
 			status = "disabled";
 		};
 
+		pcie_phy: pcie-phy@f3f2000 {
+			compatible = "hisilicon,hi960-pcie-phy";
+			reg = <0x0 0xf3f20000 0x0 0x40000>;
+			reg-names = "phy";
+			clocks = <&crg_ctrl HI3660_PCIEPHY_REF>,
+				 <&crg_ctrl HI3660_CLK_GATE_PCIEAUX>,
+				 <&crg_ctrl HI3660_PCLK_GATE_PCIE_PHY>,
+				 <&crg_ctrl HI3660_PCLK_GATE_PCIE_SYS>,
+				 <&crg_ctrl HI3660_ACLK_GATE_PCIE>;
+			clock-names = "pcie_phy_ref", "pcie_aux",
+				      "pcie_apb_phy", "pcie_apb_sys",
+				      "pcie_aclk";
+			reset-gpios = <&gpio11 1 0 >;
+			#phy-cells = <0>;
+		};
+
 		pcie@f4000000 {
 			compatible = "hisilicon,kirin960-pcie";
 			reg = <0x0 0xf4000000 0x0 0x1000>,
 			      <0x0 0xff3fe000 0x0 0x1000>,
-			      <0x0 0xf3f20000 0x0 0x40000>,
 			      <0x0 0xf5000000 0x0 0x2000>;
-			reg-names = "dbi", "apb", "phy", "config";
+			reg-names = "dbi", "apb", "config";
 			bus-range = <0x0  0x1>;
 			#address-cells = <3>;
 			#size-cells = <2>;
 			device_type = "pci";
+			phys = <&pcie_phy>;
 			ranges = <0x02000000 0x0 0x00000000
 				  0x0 0xf6000000
 				  0x0 0x02000000>;
@@ -1022,15 +1038,6 @@ &gic GIC_SPI 283 IRQ_TYPE_LEVEL_HIGH>,
 					 &gic GIC_SPI 284 IRQ_TYPE_LEVEL_HIGH>,
 					<0x0 0 0 4
 					 &gic GIC_SPI 285 IRQ_TYPE_LEVEL_HIGH>;
-			clocks = <&crg_ctrl HI3660_PCIEPHY_REF>,
-				 <&crg_ctrl HI3660_CLK_GATE_PCIEAUX>,
-				 <&crg_ctrl HI3660_PCLK_GATE_PCIE_PHY>,
-				 <&crg_ctrl HI3660_PCLK_GATE_PCIE_SYS>,
-				 <&crg_ctrl HI3660_ACLK_GATE_PCIE>;
-			clock-names = "pcie_phy_ref", "pcie_aux",
-				      "pcie_apb_phy", "pcie_apb_sys",
-				      "pcie_aclk";
-			reset-gpios = <&gpio11 1 0 >;
 		};
 
 		/* UFS */
diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index 7b0f87535960..66662f7b0fc9 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -9,7 +9,6 @@
  */
 
 #include <linux/compiler.h>
-#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/gpio.h>
@@ -21,6 +20,7 @@
 #include <linux/pci.h>
 #include <linux/pci_regs.h>
 #include <linux/platform_device.h>
+#include <linux/phy/phy.h>
 #include <linux/regmap.h>
 #include <linux/resource.h>
 #include <linux/types.h>
@@ -28,26 +28,17 @@
 
 #define to_kirin_pcie(x) dev_get_drvdata((x)->dev)
 
-#define REF_CLK_FREQ			100000000
-
 /* PCIe ELBI registers */
 #define SOC_PCIECTRL_CTRL0_ADDR		0x000
 #define SOC_PCIECTRL_CTRL1_ADDR		0x004
-#define SOC_PCIEPHY_CTRL2_ADDR		0x008
-#define SOC_PCIEPHY_CTRL3_ADDR		0x00c
 #define PCIE_ELBI_SLV_DBI_ENABLE	(0x1 << 21)
 
 /* info located in APB */
 #define PCIE_APP_LTSSM_ENABLE	0x01c
-#define PCIE_APB_PHY_CTRL0	0x0
-#define PCIE_APB_PHY_CTRL1	0x4
 #define PCIE_APB_PHY_STATUS0	0x400
 #define PCIE_LINKUP_ENABLE	(0x8020)
 #define PCIE_LTSSM_ENABLE_BIT	(0x1 << 11)
 #define PIPE_CLK_STABLE		(0x1 << 19)
-#define PHY_REF_PAD_BIT		(0x1 << 8)
-#define PHY_PWR_DOWN_BIT	(0x1 << 22)
-#define PHY_RST_ACK_BIT		(0x1 << 16)
 
 /* info located in sysctrl */
 #define SCTRL_PCIE_CMOS_OFFSET	0x60
@@ -60,36 +51,10 @@
 #define PCIE_DEBOUNCE_PARAM	0xF0F400
 #define PCIE_OE_BYPASS		(0x3 << 28)
 
-/* peri_crg ctrl */
-#define CRGCTRL_PCIE_ASSERT_OFFSET	0x88
-#define CRGCTRL_PCIE_ASSERT_BIT		0x8c000000
-
-/* Time for delay */
-#define REF_2_PERST_MIN		20000
-#define REF_2_PERST_MAX		25000
-#define PERST_2_ACCESS_MIN	10000
-#define PERST_2_ACCESS_MAX	12000
-#define LINK_WAIT_MIN		900
-#define LINK_WAIT_MAX		1000
-#define PIPE_CLK_WAIT_MIN	550
-#define PIPE_CLK_WAIT_MAX	600
-#define TIME_CMOS_MIN		100
-#define TIME_CMOS_MAX		105
-#define TIME_PHY_PD_MIN		10
-#define TIME_PHY_PD_MAX		11
-
 struct kirin_pcie {
 	struct dw_pcie	*pci;
+	struct phy	*phy;
 	void __iomem	*apb_base;
-	void __iomem	*phy_base;
-	struct regmap	*crgctrl;
-	struct regmap	*sysctrl;
-	struct clk	*apb_sys_clk;
-	struct clk	*apb_phy_clk;
-	struct clk	*phy_ref_clk;
-	struct clk	*pcie_aclk;
-	struct clk	*pcie_aux_clk;
-	int		gpio_id_reset;
 };
 
 /* Registers in PCIeCTRL */
@@ -104,46 +69,6 @@ static inline u32 kirin_apb_ctrl_readl(struct kirin_pcie *kirin_pcie, u32 reg)
 	return readl(kirin_pcie->apb_base + reg);
 }
 
-/* Registers in PCIePHY */
-static inline void kirin_apb_phy_writel(struct kirin_pcie *kirin_pcie,
-					u32 val, u32 reg)
-{
-	writel(val, kirin_pcie->phy_base + reg);
-}
-
-static inline u32 kirin_apb_phy_readl(struct kirin_pcie *kirin_pcie, u32 reg)
-{
-	return readl(kirin_pcie->phy_base + reg);
-}
-
-static long kirin_pcie_get_clk(struct kirin_pcie *kirin_pcie,
-			       struct platform_device *pdev)
-{
-	struct device *dev = &pdev->dev;
-
-	kirin_pcie->phy_ref_clk = devm_clk_get(dev, "pcie_phy_ref");
-	if (IS_ERR(kirin_pcie->phy_ref_clk))
-		return PTR_ERR(kirin_pcie->phy_ref_clk);
-
-	kirin_pcie->pcie_aux_clk = devm_clk_get(dev, "pcie_aux");
-	if (IS_ERR(kirin_pcie->pcie_aux_clk))
-		return PTR_ERR(kirin_pcie->pcie_aux_clk);
-
-	kirin_pcie->apb_phy_clk = devm_clk_get(dev, "pcie_apb_phy");
-	if (IS_ERR(kirin_pcie->apb_phy_clk))
-		return PTR_ERR(kirin_pcie->apb_phy_clk);
-
-	kirin_pcie->apb_sys_clk = devm_clk_get(dev, "pcie_apb_sys");
-	if (IS_ERR(kirin_pcie->apb_sys_clk))
-		return PTR_ERR(kirin_pcie->apb_sys_clk);
-
-	kirin_pcie->pcie_aclk = devm_clk_get(dev, "pcie_aclk");
-	if (IS_ERR(kirin_pcie->pcie_aclk))
-		return PTR_ERR(kirin_pcie->pcie_aclk);
-
-	return 0;
-}
-
 static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
 				    struct platform_device *pdev)
 {
@@ -152,149 +77,18 @@ static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
 	if (IS_ERR(kirin_pcie->apb_base))
 		return PTR_ERR(kirin_pcie->apb_base);
 
-	kirin_pcie->phy_base =
-		devm_platform_ioremap_resource_byname(pdev, "phy");
-	if (IS_ERR(kirin_pcie->phy_base))
-		return PTR_ERR(kirin_pcie->phy_base);
-
-	kirin_pcie->crgctrl =
-		syscon_regmap_lookup_by_compatible("hisilicon,hi3660-crgctrl");
-	if (IS_ERR(kirin_pcie->crgctrl))
-		return PTR_ERR(kirin_pcie->crgctrl);
-
-	kirin_pcie->sysctrl =
-		syscon_regmap_lookup_by_compatible("hisilicon,hi3660-sctrl");
-	if (IS_ERR(kirin_pcie->sysctrl))
-		return PTR_ERR(kirin_pcie->sysctrl);
-
 	return 0;
 }
 
-static int kirin_pcie_phy_init(struct kirin_pcie *kirin_pcie)
-{
-	struct device *dev = kirin_pcie->pci->dev;
-	u32 reg_val;
-
-	reg_val = kirin_apb_phy_readl(kirin_pcie, PCIE_APB_PHY_CTRL1);
-	reg_val &= ~PHY_REF_PAD_BIT;
-	kirin_apb_phy_writel(kirin_pcie, reg_val, PCIE_APB_PHY_CTRL1);
-
-	reg_val = kirin_apb_phy_readl(kirin_pcie, PCIE_APB_PHY_CTRL0);
-	reg_val &= ~PHY_PWR_DOWN_BIT;
-	kirin_apb_phy_writel(kirin_pcie, reg_val, PCIE_APB_PHY_CTRL0);
-	usleep_range(TIME_PHY_PD_MIN, TIME_PHY_PD_MAX);
-
-	reg_val = kirin_apb_phy_readl(kirin_pcie, PCIE_APB_PHY_CTRL1);
-	reg_val &= ~PHY_RST_ACK_BIT;
-	kirin_apb_phy_writel(kirin_pcie, reg_val, PCIE_APB_PHY_CTRL1);
-
-	usleep_range(PIPE_CLK_WAIT_MIN, PIPE_CLK_WAIT_MAX);
-	reg_val = kirin_apb_phy_readl(kirin_pcie, PCIE_APB_PHY_STATUS0);
-	if (reg_val & PIPE_CLK_STABLE) {
-		dev_err(dev, "PIPE clk is not stable\n");
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
-static void kirin_pcie_oe_enable(struct kirin_pcie *kirin_pcie)
-{
-	u32 val;
-
-	regmap_read(kirin_pcie->sysctrl, SCTRL_PCIE_OE_OFFSET, &val);
-	val |= PCIE_DEBOUNCE_PARAM;
-	val &= ~PCIE_OE_BYPASS;
-	regmap_write(kirin_pcie->sysctrl, SCTRL_PCIE_OE_OFFSET, val);
-}
-
-static int kirin_pcie_clk_ctrl(struct kirin_pcie *kirin_pcie, bool enable)
-{
-	int ret = 0;
-
-	if (!enable)
-		goto close_clk;
-
-	ret = clk_set_rate(kirin_pcie->phy_ref_clk, REF_CLK_FREQ);
-	if (ret)
-		return ret;
-
-	ret = clk_prepare_enable(kirin_pcie->phy_ref_clk);
-	if (ret)
-		return ret;
-
-	ret = clk_prepare_enable(kirin_pcie->apb_sys_clk);
-	if (ret)
-		goto apb_sys_fail;
-
-	ret = clk_prepare_enable(kirin_pcie->apb_phy_clk);
-	if (ret)
-		goto apb_phy_fail;
-
-	ret = clk_prepare_enable(kirin_pcie->pcie_aclk);
-	if (ret)
-		goto aclk_fail;
-
-	ret = clk_prepare_enable(kirin_pcie->pcie_aux_clk);
-	if (ret)
-		goto aux_clk_fail;
-
-	return 0;
-
-close_clk:
-	clk_disable_unprepare(kirin_pcie->pcie_aux_clk);
-aux_clk_fail:
-	clk_disable_unprepare(kirin_pcie->pcie_aclk);
-aclk_fail:
-	clk_disable_unprepare(kirin_pcie->apb_phy_clk);
-apb_phy_fail:
-	clk_disable_unprepare(kirin_pcie->apb_sys_clk);
-apb_sys_fail:
-	clk_disable_unprepare(kirin_pcie->phy_ref_clk);
-
-	return ret;
-}
-
 static int kirin_pcie_power_on(struct kirin_pcie *kirin_pcie)
 {
 	int ret;
 
-	/* Power supply for Host */
-	regmap_write(kirin_pcie->sysctrl,
-		     SCTRL_PCIE_CMOS_OFFSET, SCTRL_PCIE_CMOS_BIT);
-	usleep_range(TIME_CMOS_MIN, TIME_CMOS_MAX);
-	kirin_pcie_oe_enable(kirin_pcie);
-
-	ret = kirin_pcie_clk_ctrl(kirin_pcie, true);
+	ret = phy_init(kirin_pcie->phy);
 	if (ret)
 		return ret;
 
-	/* ISO disable, PCIeCtrl, PHY assert and clk gate clear */
-	regmap_write(kirin_pcie->sysctrl,
-		     SCTRL_PCIE_ISO_OFFSET, SCTRL_PCIE_ISO_BIT);
-	regmap_write(kirin_pcie->crgctrl,
-		     CRGCTRL_PCIE_ASSERT_OFFSET, CRGCTRL_PCIE_ASSERT_BIT);
-	regmap_write(kirin_pcie->sysctrl,
-		     SCTRL_PCIE_HPCLK_OFFSET, SCTRL_PCIE_HPCLK_BIT);
-
-	ret = kirin_pcie_phy_init(kirin_pcie);
-	if (ret)
-		goto close_clk;
-
-	/* perst assert Endpoint */
-	if (!gpio_request(kirin_pcie->gpio_id_reset, "pcie_perst")) {
-		usleep_range(REF_2_PERST_MIN, REF_2_PERST_MAX);
-		ret = gpio_direction_output(kirin_pcie->gpio_id_reset, 1);
-		if (ret)
-			goto close_clk;
-		usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX);
-
-		return 0;
-	}
-
-close_clk:
-	kirin_pcie_clk_ctrl(kirin_pcie, false);
-	return ret;
+	return phy_power_on(kirin_pcie->phy);
 }
 
 static void kirin_pcie_sideband_dbi_w_mode(struct kirin_pcie *kirin_pcie,
@@ -444,30 +238,41 @@ static int kirin_pcie_probe(struct platform_device *pdev)
 	pci->pp.ops = &kirin_pcie_host_ops;
 	kirin_pcie->pci = pci;
 
-	ret = kirin_pcie_get_clk(kirin_pcie, pdev);
-	if (ret)
-		return ret;
+	kirin_pcie->phy = devm_of_phy_get(dev, dev->of_node, NULL);
+	if (IS_ERR(kirin_pcie->phy))
+		return PTR_ERR(kirin_pcie->phy);
 
 	ret = kirin_pcie_get_resource(kirin_pcie, pdev);
 	if (ret)
 		return ret;
 
-	kirin_pcie->gpio_id_reset = of_get_named_gpio(dev->of_node,
-						      "reset-gpios", 0);
-	if (kirin_pcie->gpio_id_reset == -EPROBE_DEFER) {
-		return -EPROBE_DEFER;
-	} else if (!gpio_is_valid(kirin_pcie->gpio_id_reset)) {
-		dev_err(dev, "unable to get a valid gpio pin\n");
-		return -ENODEV;
-	}
-
 	ret = kirin_pcie_power_on(kirin_pcie);
 	if (ret)
 		return ret;
 
 	platform_set_drvdata(pdev, kirin_pcie);
 
-	return dw_pcie_host_init(&pci->pp);
+	ret = dw_pcie_host_init(&pci->pp);
+	if (ret) {
+		phy_power_off(kirin_pcie->phy);
+		goto err;
+	}
+
+	return 0;
+err:
+	phy_exit(kirin_pcie->phy);
+
+	return ret;
+}
+
+static int __exit kirin_pcie_remove(struct platform_device *pdev)
+{
+	struct kirin_pcie *kirin_pcie = platform_get_drvdata(pdev);
+
+	phy_power_off(kirin_pcie->phy);
+	phy_exit(kirin_pcie->phy);
+
+	return 0;
 }
 
 static const struct of_device_id kirin_pcie_match[] = {
@@ -478,6 +283,7 @@ static const struct of_device_id kirin_pcie_match[] = {
 
 static struct platform_driver kirin_pcie_driver = {
 	.probe			= kirin_pcie_probe,
+	.remove	        	= __exit_p(kirin_pcie_remove),
 	.driver			= {
 		.name			= "kirin-pcie",
 		.of_match_table = kirin_pcie_match,
-- 
2.31.1