devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v7 00/10] Add support for Hikey 970 PCIe
@ 2021-07-21  8:39 Mauro Carvalho Chehab
  2021-07-21  8:39 ` [PATCH v7 05/10] dt-bindings: PCI: kirin: Fix compatible string Mauro Carvalho Chehab
                   ` (4 more replies)
  0 siblings, 5 replies; 21+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-21  8:39 UTC (permalink / raw)
  To: Vinod Koul, Bjorn Helgaas, Rob Herring
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab,
	Krzysztof Wilczyński, Binghui Wang, Gustavo Pimentel,
	Jingoo Han, Rob Herring, Xiaowei Song, devicetree,
	linux-arm-kernel, linux-kernel, linux-pci, linux-phy

This series depends on this one:
	https://lore.kernel.org/lkml/cover.1626515862.git.mchehab+huawei@kernel.org/

It is available, with its patch dependencies against v5.14-rc1 at:
	https://github.com/mchehab/linux/commits/pcie-alternate

This series add support at the pcie-kirin dirver for it to use a separate PHY driver.

Yet, in order to preserve the existing DT schema for Kirin 960, it keeps the
Kirin 960 PHY inside the pci driver. I tried to find a way to split it while keeping
the DT schema backward-compatible, but currently the PHY core doesn't allow
that, as it relies on a "phy" property at the pcie node in order to recognize the
PHY driver.

Once the pci-kiring is modified to support an external PHY driver, add a
Kirin 970 PHY and add the needed properties for the HiKey 970 board to
detect the PCIe.

It should be noticed that the HiKey 970 design uses 4 different GPIO pins, one 
for each PERST# signal for each PCIe bus device:

    - GPIO 56 has a pullup logic from 1V8 to 2V5
      connected to a PCIe bridge chip (PEX 8606);
    - GPIO 25 has a pullup logic from 1V8 to 3V3
      connected to the PERST# pin at the M.2 slot;
    - GPIO 220 has a pullup logic from 1V8 to 3V3
      connected to the PERST# pin at the PCIe mini slot;
    - GPIO 203 has a pullup logic from 1V8 to 3V3
      connected to the PERST# pin at the Ethernet chipset.

At the first versions, those were mapped as part of the pci-bus, but the
pci-bus.yaml schema only allows a single PERST# GPIO. So, on v5, those
were moved to the PHY DT schema. However, as Rob complained, on this
version, I opted to add a separate patch (the last one) that moves those
back to the PCIe of-node. 

If such patch 09/09 is accepted, then this patch for the DT schema should 
also be accepted:

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

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

Also tested  that Hikey 960 keeps being supported:

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

---

v7:
- Moved kirin_pcie_match to be closer to the probe function;
- Improved patch description for:
	"PCI: kirin: add support for a PHY layer"
- Added missing MODULE_*() macros on both PCI and PHY drivers;
- Fixed a warning at hisilicon,phy-hi3670-pcie.yaml reported by
  Rob Herring's bot.

v6:
- Use an alternative approach, in order to keep the Kirin 960 PHY internal to 
  the driver, in order to not break the DT schema. The PHY-specific code
  were made self-contained at pcie-kirin, in order to make easier to split
  it in the future, if needed.

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.


Manivannan Sadhasivam (1):
  arm64: dts: HiSilicon: Add support for HiKey 970 PCIe controller
    hardware

Mauro Carvalho Chehab (9):
  PCI: kirin: Reorganize the PHY logic inside the driver
  PCI: kirin: Add support for a PHY layer
  PCI: kirin: Use regmap for APB registers
  PCI: kirin: Add MODULE_* macros
  dt-bindings: PCI: kirin: Fix compatible string
  dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY
  phy: HiSilicon: Add driver for Kirin 970 PCIe PHY
  dt-bindings: PCI: kirin-pcie.txt: Convert it to yaml
  phy-hi3670-pcie: Move reset-gpios to the PCIe DT schema

 .../bindings/pci/hisilicon,kirin-pcie.yaml    |  87 ++
 .../devicetree/bindings/pci/kirin-pcie.txt    |  50 -
 .../devicetree/bindings/pci/snps,dw-pcie.yaml |   2 +-
 .../phy/hisilicon,phy-hi3670-pcie.yaml        |  88 ++
 MAINTAINERS                                   |   2 +-
 arch/arm64/boot/dts/hisilicon/hi3670.dtsi     |  70 ++
 .../boot/dts/hisilicon/hikey970-pmic.dtsi     |   1 -
 drivers/pci/controller/dwc/pcie-kirin.c       | 413 +++++---
 drivers/phy/hisilicon/Kconfig                 |  10 +
 drivers/phy/hisilicon/Makefile                |   1 +
 drivers/phy/hisilicon/phy-hi3670-pcie.c       | 902 ++++++++++++++++++
 11 files changed, 1429 insertions(+), 197 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/hisilicon,kirin-pcie.yaml
 delete mode 100644 Documentation/devicetree/bindings/pci/kirin-pcie.txt
 create mode 100644 Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
 create mode 100644 drivers/phy/hisilicon/phy-hi3670-pcie.c

-- 
2.31.1



^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v7 05/10] dt-bindings: PCI: kirin: Fix compatible string
  2021-07-21  8:39 [PATCH v7 00/10] Add support for Hikey 970 PCIe Mauro Carvalho Chehab
@ 2021-07-21  8:39 ` Mauro Carvalho Chehab
  2021-07-21  8:39 ` [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY Mauro Carvalho Chehab
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 21+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-21  8:39 UTC (permalink / raw)
  To: Vinod Koul, Bjorn Helgaas, Rob Herring
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Rob Herring,
	devicetree, linux-kernel, linux-pci

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.

Acked-by: Rob Herring <robh@kernel.org>
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 7db30534498f..7adab8999a6a 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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY
  2021-07-21  8:39 [PATCH v7 00/10] Add support for Hikey 970 PCIe Mauro Carvalho Chehab
  2021-07-21  8:39 ` [PATCH v7 05/10] dt-bindings: PCI: kirin: Fix compatible string Mauro Carvalho Chehab
@ 2021-07-21  8:39 ` Mauro Carvalho Chehab
  2021-07-23 22:50   ` Rob Herring
  2021-07-21  8:39 ` [PATCH v7 08/10] arm64: dts: HiSilicon: Add support for HiKey 970 PCIe controller hardware Mauro Carvalho Chehab
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 21+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-21  8:39 UTC (permalink / raw)
  To: Vinod Koul, Bjorn Helgaas, Rob Herring
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab,
	Kishon Vijay Abraham I, Rob Herring, devicetree, linux-kernel,
	linux-phy

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        | 95 +++++++++++++++++++
 1 file changed, 95 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..a5ea13332cac
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/hisilicon,phy-hi3670-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HiSilicon Kirin970 PCIe PHY
+
+maintainers:
+  - Mauro Carvalho Chehab <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
+
+  phy-supply:
+    description: The PCIe PHY power supply
+
+  clocks:
+    items:
+      - description: PCIe PHY clock
+      - description: PCIe AUX clock
+      - description: PCIe APB PHY clock
+      - description: PCIe APB SYS clock
+      - description: PCIe ACLK clock
+
+  clock-names:
+    items:
+      - const: phy_ref
+      - const: aux
+      - const: apb_phy
+      - const: apb_sys
+      - const: aclk
+
+  reset-gpios:
+    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
+  - clocks
+  - clock-names
+  - reset-gpios
+  - clkreq-gpios
+  - hisilicon,eye-diagram-param
+  - phy-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/hi3670-clock.h>
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+      pcie_phy: pcie-phy@fc000000 {
+        compatible = "hisilicon,hi970-pcie-phy";
+        reg = <0x0 0xfc000000 0x0 0x80000>;
+        #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 = "phy_ref", "aux",
+                      "apb_phy", "apb_sys", "aclk";
+        reset-gpios = <&gpio7 0 0 >, <&gpio25 2 0 >,
+                      <&gpio3 1 0 >, <&gpio27 4 0 >;
+        clkreq-gpios = <&gpio20 6 0 >, <&gpio27 3 0 >, <&gpio17 0 0 >;
+        hisilicon,eye-diagram-param = <0xFFFFFFFF 0xFFFFFFFF
+                                       0xFFFFFFFF 0xFFFFFFFF 0xFFFFFFFF>;
+      };
+    };
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v7 08/10] arm64: dts: HiSilicon: Add support for HiKey 970 PCIe controller hardware
  2021-07-21  8:39 [PATCH v7 00/10] Add support for Hikey 970 PCIe Mauro Carvalho Chehab
  2021-07-21  8:39 ` [PATCH v7 05/10] dt-bindings: PCI: kirin: Fix compatible string Mauro Carvalho Chehab
  2021-07-21  8:39 ` [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY Mauro Carvalho Chehab
@ 2021-07-21  8:39 ` Mauro Carvalho Chehab
  2021-07-22 13:36   ` Manivannan Sadhasivam
  2021-08-16 18:26   ` Rob Herring
  2021-07-21  8:39 ` [PATCH v7 09/10] dt-bindings: PCI: kirin-pcie.txt: Convert it to yaml Mauro Carvalho Chehab
  2021-07-21  8:39 ` [PATCH v7 10/10] phy-hi3670-pcie: Move reset-gpios to the PCIe DT schema Mauro Carvalho Chehab
  4 siblings, 2 replies; 21+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-21  8:39 UTC (permalink / raw)
  To: Vinod Koul, Bjorn Helgaas, Rob Herring
  Cc: linuxarm, mauro.chehab, Manivannan Sadhasivam,
	Krzysztof Wilczyński, Binghui Wang, Lorenzo Pieralisi,
	Rob Herring, Wei Xu, Xiaowei Song, devicetree, linux-arm-kernel,
	linux-kernel, linux-pci, Mauro Carvalho Chehab

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

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

Co-developed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 arch/arm64/boot/dts/hisilicon/hi3670.dtsi     | 71 +++++++++++++++++++
 .../boot/dts/hisilicon/hikey970-pmic.dtsi     |  1 -
 drivers/pci/controller/dwc/pcie-kirin.c       | 12 ----
 3 files changed, 71 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
index 1f228612192c..6dfcfcfeedae 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
@@ -177,6 +177,12 @@ sctrl: sctrl@fff0a000 {
 			#clock-cells = <1>;
 		};
 
+		pmctrl: pmctrl@fff31000 {
+			compatible = "hisilicon,hi3670-pmctrl", "syscon";
+			reg = <0x0 0xfff31000 0x0 0x1000>;
+			#clock-cells = <1>;
+		};
+
 		iomcu: iomcu@ffd7e000 {
 			compatible = "hisilicon,hi3670-iomcu", "syscon";
 			reg = <0x0 0xffd7e000 0x0 0x1000>;
@@ -660,6 +666,71 @@ gpio28: gpio@fff1d000 {
 			clock-names = "apb_pclk";
 		};
 
+		its_pcie: interrupt-controller@f4000000 {
+			compatible = "arm,gic-v3-its";
+			msi-controller;
+			reg = <0x0 0xf5100000 0x0 0x100000>;
+		};
+
+		pcie_phy: pcie-phy@fc000000 {
+			compatible = "hisilicon,hi970-pcie-phy";
+			reg = <0x0 0xfc000000 0x0 0x80000>;
+
+			phy-supply = <&ldo33>;
+
+			clocks = <&crg_ctrl HI3670_CLK_GATE_PCIEPHY_REF>,
+				 <&crg_ctrl HI3670_CLK_GATE_PCIEAUX>,
+				 <&crg_ctrl HI3670_PCLK_GATE_PCIE_PHY>,
+				 <&crg_ctrl HI3670_PCLK_GATE_PCIE_SYS>,
+				 <&crg_ctrl HI3670_ACLK_GATE_PCIE>;
+			clock-names = "phy_ref", "aux",
+				      "apb_phy", "apb_sys",
+				      "aclk";
+
+			reset-gpios = <&gpio7 0 0 >, <&gpio25 2 0 >,
+				      <&gpio3 1 0 >, <&gpio27 4 0 >;
+
+			clkreq-gpios = <&gpio20 6 0 >, <&gpio27 3 0 >,
+				       <&gpio17 0 0 >;
+
+			/* vboost iboost pre post main */
+			hisilicon,eye-diagram-param = <0xFFFFFFFF 0xFFFFFFFF
+						       0xFFFFFFFF 0xFFFFFFFF
+						       0xFFFFFFFF>;
+
+			#phy-cells = <0>;
+		};
+
+		pcie@f4000000 {
+			compatible = "hisilicon,kirin970-pcie";
+			reg = <0x0 0xf4000000 0x0 0x1000000>,
+			      <0x0 0xfc180000 0x0 0x1000>,
+			      <0x0 0xf5000000 0x0 0x2000>;
+			reg-names = "dbi", "apb", "config";
+			bus-range = <0x0  0x1>;
+			msi-parent = <&its_pcie>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+			device_type = "pci";
+			phys = <&pcie_phy>;
+			ranges = <0x02000000 0x0 0x00000000
+				  0x0 0xf6000000
+				  0x0 0x02000000>;
+			num-lanes = <1>;
+			#interrupt-cells = <1>;
+			interrupts = <0 283 4>;
+			interrupt-names = "msi";
+			interrupt-map-mask = <0 0 0 7>;
+			interrupt-map = <0x0 0 0 1
+					 &gic GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>,
+					<0x0 0 0 2
+					 &gic GIC_SPI 283 IRQ_TYPE_LEVEL_HIGH>,
+					<0x0 0 0 3
+					 &gic GIC_SPI 284 IRQ_TYPE_LEVEL_HIGH>,
+					<0x0 0 0 4
+					 &gic GIC_SPI 285 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
 		/* UFS */
 		ufs: ufs@ff3c0000 {
 			compatible = "hisilicon,hi3670-ufs", "jedec,ufs-2.1";
diff --git a/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi b/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi
index 48c739eacba0..03452e627641 100644
--- a/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi
@@ -73,7 +73,6 @@ ldo33: LDO33 { /* PEX8606 */
 					regulator-name = "ldo33";
 					regulator-min-microvolt = <2500000>;
 					regulator-max-microvolt = <3300000>;
-					regulator-boot-on;
 				};
 
 				ldo34: LDO34 { /* GPS AUX IN VDD */
diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index bfc0513f7b15..9dad14929538 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -347,18 +347,6 @@ static const struct regmap_config pcie_kirin_regmap_conf = {
 	.reg_stride = 4,
 };
 
-/* Registers in PCIeCTRL */
-static inline void kirin_apb_ctrl_writel(struct kirin_pcie *kirin_pcie,
-					 u32 val, u32 reg)
-{
-	writel(val, kirin_pcie->apb_base + reg);
-}
-
-static inline u32 kirin_apb_ctrl_readl(struct kirin_pcie *kirin_pcie, u32 reg)
-{
-	return readl(kirin_pcie->apb_base + reg);
-}
-
 static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
 				    struct platform_device *pdev)
 {
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v7 09/10] dt-bindings: PCI: kirin-pcie.txt: Convert it to yaml
  2021-07-21  8:39 [PATCH v7 00/10] Add support for Hikey 970 PCIe Mauro Carvalho Chehab
                   ` (2 preceding siblings ...)
  2021-07-21  8:39 ` [PATCH v7 08/10] arm64: dts: HiSilicon: Add support for HiKey 970 PCIe controller hardware Mauro Carvalho Chehab
@ 2021-07-21  8:39 ` Mauro Carvalho Chehab
  2021-07-23 22:56   ` Rob Herring
  2021-07-21  8:39 ` [PATCH v7 10/10] phy-hi3670-pcie: Move reset-gpios to the PCIe DT schema Mauro Carvalho Chehab
  4 siblings, 1 reply; 21+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-21  8:39 UTC (permalink / raw)
  To: Vinod Koul, Bjorn Helgaas, Rob Herring
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Binghui Wang,
	Gustavo Pimentel, Jingoo Han, Rob Herring, Xiaowei Song,
	devicetree, linux-kernel, linux-pci

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

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 .../bindings/pci/hisilicon,kirin-pcie.yaml    | 87 +++++++++++++++++++
 .../devicetree/bindings/pci/kirin-pcie.txt    | 50 -----------
 .../devicetree/bindings/pci/snps,dw-pcie.yaml |  2 +-
 MAINTAINERS                                   |  2 +-
 4 files changed, 89 insertions(+), 52 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/hisilicon,kirin-pcie.yaml
 delete mode 100644 Documentation/devicetree/bindings/pci/kirin-pcie.txt

diff --git a/Documentation/devicetree/bindings/pci/hisilicon,kirin-pcie.yaml b/Documentation/devicetree/bindings/pci/hisilicon,kirin-pcie.yaml
new file mode 100644
index 000000000000..eabc651c9766
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/hisilicon,kirin-pcie.yaml
@@ -0,0 +1,87 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/hisilicon,kirin-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HiSilicon Kirin SoCs PCIe host DT description
+
+maintainers:
+  - Xiaowei Song <songxiaowei@hisilicon.com>
+  - Binghui Wang <wangbinghui@hisilicon.com>
+
+description: |
+  Kirin PCIe host controller is based on the Synopsys DesignWare PCI core.
+  It shares common functions with the PCIe DesignWare core driver and
+  inherits common properties defined in
+  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml.
+
+allOf:
+  - $ref: /schemas/pci/snps,dw-pcie.yaml#
+
+properties:
+  compatible:
+    contains:
+      enum:
+        - hisilicon,kirin960-pcie
+        - hisilicon,kirin970-pcie
+
+  reg:
+    description: |
+      Should contain rc_dbi, apb, config registers location and length.
+    minItems: 3
+    maxItems: 4
+
+  reg-names:
+    items:
+      - const: dbi          # controller configuration registers
+      - const: apb          # apb Ctrl register defined by Kirin
+      - const: config       # PCIe configuration space registers
+      - const: phy          # apb PHY register used on Kirin 960 PHY
+    minItems: 3
+    maxItems: 4
+
+  reset-gpios:
+    description: The GPIO(s) to generate PCIe PERST# assert and deassert signal.
+    minItems: 1
+    maxItems: 4
+
+required:
+  - compatible
+  - reg
+  - reg-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      pcie: pcie@f4000000 {
+        compatible = "hisilicon,kirin970-pcie";
+        reg = <0x0 0xf4000000 0x0 0x1000>,
+              <0x0 0xff3fe000 0x0 0x1000>,
+              <0x0 0xf4000000 0 0x2000>;
+        reg-names = "dbi", "apb", "config";
+        bus-range = <0x0  0x1>;
+        #address-cells = <3>;
+        #size-cells = <2>;
+        device_type = "pci";
+        ranges = <0x02000000 0x0 0x00000000 0x0 0xf5000000 0x0 0x2000000>;
+        num-lanes = <1>;
+        #interrupt-cells = <1>;
+        interrupts = <0 283 4>;
+        interrupt-names = "msi";
+        interrupt-map-mask = <0xf800 0 0 7>;
+        interrupt-map = <0x0 0 0 1 &gic GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>,
+                        <0x0 0 0 2 &gic GIC_SPI 283 IRQ_TYPE_LEVEL_HIGH>,
+                        <0x0 0 0 3 &gic GIC_SPI 284 IRQ_TYPE_LEVEL_HIGH>,
+                        <0x0 0 0 4 &gic GIC_SPI 285 IRQ_TYPE_LEVEL_HIGH>;
+        reset-gpios = <&gpio7 0 0 >, <&gpio25 2 0 >,
+                      <&gpio3 1 0 >, <&gpio27 4 0 >;
+      };
+    };
diff --git a/Documentation/devicetree/bindings/pci/kirin-pcie.txt b/Documentation/devicetree/bindings/pci/kirin-pcie.txt
deleted file mode 100644
index 7adab8999a6a..000000000000
--- a/Documentation/devicetree/bindings/pci/kirin-pcie.txt
+++ /dev/null
@@ -1,50 +0,0 @@
-HiSilicon Kirin SoCs PCIe host DT description
-
-Kirin PCIe host controller is based on the Synopsys DesignWare PCI core.
-It shares common functions with the PCIe DesignWare core driver and
-inherits common properties defined in
-Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml.
-
-Additional properties are described here:
-
-Required properties
-- compatible:
-	"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;
-  "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:
-
-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";
-		bus-range = <0x0  0x1>;
-		#address-cells = <3>;
-		#size-cells = <2>;
-		device_type = "pci";
-		ranges = <0x02000000 0x0 0x00000000 0x0 0xf5000000 0x0 0x2000000>;
-		num-lanes = <1>;
-		#interrupt-cells = <1>;
-		interrupt-map-mask = <0xf800 0 0 7>;
-		interrupt-map = <0x0 0 0 1 &gic 0 0 0  282 4>,
-				<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 >;
-	};
diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
index a8c1db879fb9..d80894a5abf5 100644
--- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
@@ -34,7 +34,7 @@ properties:
     minItems: 2
     maxItems: 5
     items:
-      enum: [dbi, dbi2, config, atu, app, elbi, mgmt, ctrl, parf, cfg, link]
+      enum: [dbi, dbi2, config, atu, apb, app, elbi, mgmt, ctrl, parf, cfg, link]
 
   num-lanes:
     description: |
diff --git a/MAINTAINERS b/MAINTAINERS
index b54bd9dd07ec..d5f53b2d3f9c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14420,7 +14420,7 @@ M:	Xiaowei Song <songxiaowei@hisilicon.com>
 M:	Binghui Wang <wangbinghui@hisilicon.com>
 L:	linux-pci@vger.kernel.org
 S:	Maintained
-F:	Documentation/devicetree/bindings/pci/kirin-pcie.txt
+F:	Documentation/devicetree/bindings/pci/hisilicon,kirin-pcie.yaml
 F:	drivers/pci/controller/dwc/pcie-kirin.c
 
 PCIE DRIVER FOR HISILICON STB
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v7 10/10] phy-hi3670-pcie: Move reset-gpios to the PCIe DT schema
  2021-07-21  8:39 [PATCH v7 00/10] Add support for Hikey 970 PCIe Mauro Carvalho Chehab
                   ` (3 preceding siblings ...)
  2021-07-21  8:39 ` [PATCH v7 09/10] dt-bindings: PCI: kirin-pcie.txt: Convert it to yaml Mauro Carvalho Chehab
@ 2021-07-21  8:39 ` Mauro Carvalho Chehab
  4 siblings, 0 replies; 21+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-21  8:39 UTC (permalink / raw)
  To: Vinod Koul, Bjorn Helgaas, Rob Herring
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab,
	Kishon Vijay Abraham I, Manivannan Sadhasivam, Rob Herring,
	Wei Xu, devicetree, linux-arm-kernel, linux-kernel, linux-phy

The PHY interface as found on HiKey 970 uses 4 reset-gpios
instead of just one. That seems to be due to electrical
requirements, as, on HiKey 970, the PERST# signal is
provided via one GPIO per connected/available PCIe device:

- GPIO 56 has a pullup logic from 1V8 to 2V5
  connected to a PCIe bridge chip (PEX 8606);
- GPIO 25 has a pullup logic from 1V8 to 3V3
  connected to the PERST# pin at the M.2 slot;
- GPIO 220 has a pullup logic from 1V8 to 3V3
  connected to the PERST# pin at the PCIe mini slot;
- GPIO 203 has a pullup logic from 1V8 to 3V3
  connected to the PERST# pin at the Ethernet chipset.

Originally, this was mapped via the PHY interface, but, as such
design may also be used with different hardware, remap this
to use the pcie-bus DT schema.

This patch depends on a DT schema patch submitted at:
	https://github.com/devicetree-org/dt-schema/pull/56

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 .../phy/hisilicon,phy-hi3670-pcie.yaml        |  7 ---
 arch/arm64/boot/dts/hisilicon/hi3670.dtsi     |  5 +-
 drivers/phy/hisilicon/phy-hi3670-pcie.c       | 54 ++++++++++---------
 3 files changed, 31 insertions(+), 35 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
index a5ea13332cac..86767c53bc91 100644
--- a/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
+++ b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
@@ -42,10 +42,6 @@ properties:
       - const: apb_sys
       - const: aclk
 
-  reset-gpios:
-    description: PCI PERST reset GPIOs
-    maxItems: 4
-
   clkreq-gpios:
     description: Clock request GPIOs
     maxItems: 3
@@ -60,7 +56,6 @@ required:
   - reg
   - clocks
   - clock-names
-  - reset-gpios
   - clkreq-gpios
   - hisilicon,eye-diagram-param
   - phy-supply
@@ -86,8 +81,6 @@ examples:
                  <&crg_ctrl HI3670_ACLK_GATE_PCIE>;
         clock-names = "phy_ref", "aux",
                       "apb_phy", "apb_sys", "aclk";
-        reset-gpios = <&gpio7 0 0 >, <&gpio25 2 0 >,
-                      <&gpio3 1 0 >, <&gpio27 4 0 >;
         clkreq-gpios = <&gpio20 6 0 >, <&gpio27 3 0 >, <&gpio17 0 0 >;
         hisilicon,eye-diagram-param = <0xFFFFFFFF 0xFFFFFFFF
                                        0xFFFFFFFF 0xFFFFFFFF 0xFFFFFFFF>;
diff --git a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
index 6dfcfcfeedae..a07790c76b72 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
@@ -687,9 +687,6 @@ pcie_phy: pcie-phy@fc000000 {
 				      "apb_phy", "apb_sys",
 				      "aclk";
 
-			reset-gpios = <&gpio7 0 0 >, <&gpio25 2 0 >,
-				      <&gpio3 1 0 >, <&gpio27 4 0 >;
-
 			clkreq-gpios = <&gpio20 6 0 >, <&gpio27 3 0 >,
 				       <&gpio17 0 0 >;
 
@@ -729,6 +726,8 @@ &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>;
+			reset-gpios = <&gpio7 0 0 >, <&gpio25 2 0 >,
+				      <&gpio3 1 0 >, <&gpio27 4 0 >;
 		};
 
 		/* UFS */
diff --git a/drivers/phy/hisilicon/phy-hi3670-pcie.c b/drivers/phy/hisilicon/phy-hi3670-pcie.c
index 0bc0203e9140..82cc5fc4eac2 100644
--- a/drivers/phy/hisilicon/phy-hi3670-pcie.c
+++ b/drivers/phy/hisilicon/phy-hi3670-pcie.c
@@ -553,11 +553,13 @@ static int hi3670_pcie_noc_power(struct hi3670_pcie_phy *phy, bool enable)
 	return 0;
 }
 
-static int hi3670_pcie_get_apb(struct hi3670_pcie_phy *phy)
+static int hi3670_pcie_get_resources_from_pcie(struct hi3670_pcie_phy *phy)
 {
 	struct device_node *pcie_port;
 	struct device *dev = phy->dev;
 	struct device *pcie_dev;
+	char name[32];
+	int i;
 
 	pcie_port = of_get_child_by_name(dev->parent->of_node, "pcie");
 	if (!pcie_port) {
@@ -586,6 +588,27 @@ static int hi3670_pcie_get_apb(struct hi3670_pcie_phy *phy)
 		return -ENODEV;
 	}
 
+	/* perst reset gpios */
+	phy->n_gpio_resets = of_gpio_named_count(pcie_dev->of_node,
+						 "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(pcie_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;
+	}
+
 	return 0;
 }
 
@@ -644,16 +667,17 @@ static int hi3670_pcie_phy_init(struct phy *generic_phy)
 	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).
+	 * The code under hi3670_pcie_get_resources_from_pcie() need to
+	 * access the reset-gpios and the APB registers, both from the
+	 * pcie-kirin driver.
 	 *
+	 * The APB is obtained via the pcie driver's 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);
+	ret = hi3670_pcie_get_resources_from_pcie(phy);
 	if (ret)
 		return ret;
 
@@ -800,26 +824,6 @@ static int hi3670_pcie_phy_get_resources(struct hi3670_pcie_phy *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) {
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 08/10] arm64: dts: HiSilicon: Add support for HiKey 970 PCIe controller hardware
  2021-07-21  8:39 ` [PATCH v7 08/10] arm64: dts: HiSilicon: Add support for HiKey 970 PCIe controller hardware Mauro Carvalho Chehab
@ 2021-07-22 13:36   ` Manivannan Sadhasivam
  2021-07-23  6:53     ` Mauro Carvalho Chehab
  2021-08-16 18:26   ` Rob Herring
  1 sibling, 1 reply; 21+ messages in thread
From: Manivannan Sadhasivam @ 2021-07-22 13:36 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Vinod Koul, Bjorn Helgaas, Rob Herring, linuxarm, mauro.chehab,
	Krzysztof Wilczyński, Binghui Wang, Lorenzo Pieralisi,
	Rob Herring, Wei Xu, Xiaowei Song, devicetree, linux-arm-kernel,
	linux-kernel, linux-pci

On Wed, Jul 21, 2021 at 10:39:10AM +0200, Mauro Carvalho Chehab wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Add DTS bindings for the HiKey 970 board's PCIe hardware.
> 
> Co-developed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  arch/arm64/boot/dts/hisilicon/hi3670.dtsi     | 71 +++++++++++++++++++
>  .../boot/dts/hisilicon/hikey970-pmic.dtsi     |  1 -
>  drivers/pci/controller/dwc/pcie-kirin.c       | 12 ----
>  3 files changed, 71 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> index 1f228612192c..6dfcfcfeedae 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> @@ -177,6 +177,12 @@ sctrl: sctrl@fff0a000 {
>  			#clock-cells = <1>;
>  		};
>  
> +		pmctrl: pmctrl@fff31000 {
> +			compatible = "hisilicon,hi3670-pmctrl", "syscon";
> +			reg = <0x0 0xfff31000 0x0 0x1000>;
> +			#clock-cells = <1>;
> +		};
> +

Irrelevant change to this patch.

>  		iomcu: iomcu@ffd7e000 {
>  			compatible = "hisilicon,hi3670-iomcu", "syscon";
>  			reg = <0x0 0xffd7e000 0x0 0x1000>;
> @@ -660,6 +666,71 @@ gpio28: gpio@fff1d000 {
>  			clock-names = "apb_pclk";
>  		};
>  

[...]

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

Use the DT flag for interrupts instead of hardcoded value

> +			interrupt-names = "msi";
> +			interrupt-map-mask = <0 0 0 7>;
> +			interrupt-map = <0x0 0 0 1
> +					 &gic GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>,
> +					<0x0 0 0 2
> +					 &gic GIC_SPI 283 IRQ_TYPE_LEVEL_HIGH>,
> +					<0x0 0 0 3
> +					 &gic GIC_SPI 284 IRQ_TYPE_LEVEL_HIGH>,
> +					<0x0 0 0 4
> +					 &gic GIC_SPI 285 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
>  		/* UFS */
>  		ufs: ufs@ff3c0000 {
>  			compatible = "hisilicon,hi3670-ufs", "jedec,ufs-2.1";
> diff --git a/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi b/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi
> index 48c739eacba0..03452e627641 100644
> --- a/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi
> @@ -73,7 +73,6 @@ ldo33: LDO33 { /* PEX8606 */
>  					regulator-name = "ldo33";
>  					regulator-min-microvolt = <2500000>;
>  					regulator-max-microvolt = <3300000>;
> -					regulator-boot-on;

Again, irrelevant.

>  				};
>  
>  				ldo34: LDO34 { /* GPS AUX IN VDD */
> diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> index bfc0513f7b15..9dad14929538 100644
> --- a/drivers/pci/controller/dwc/pcie-kirin.c
> +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> @@ -347,18 +347,6 @@ static const struct regmap_config pcie_kirin_regmap_conf = {
>  	.reg_stride = 4,
>  };
>  
> -/* Registers in PCIeCTRL */
> -static inline void kirin_apb_ctrl_writel(struct kirin_pcie *kirin_pcie,
> -					 u32 val, u32 reg)
> -{
> -	writel(val, kirin_pcie->apb_base + reg);
> -}
> -
> -static inline u32 kirin_apb_ctrl_readl(struct kirin_pcie *kirin_pcie, u32 reg)
> -{
> -	return readl(kirin_pcie->apb_base + reg);
> -}
> -

Same here...

Thanks,
Mani

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 08/10] arm64: dts: HiSilicon: Add support for HiKey 970 PCIe controller hardware
  2021-07-22 13:36   ` Manivannan Sadhasivam
@ 2021-07-23  6:53     ` Mauro Carvalho Chehab
  2021-07-24  4:11       ` Manivannan Sadhasivam
  0 siblings, 1 reply; 21+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-23  6:53 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: Vinod Koul, Bjorn Helgaas, Rob Herring, linuxarm, mauro.chehab,
	Krzysztof Wilczyński, Binghui Wang, Lorenzo Pieralisi,
	Rob Herring, Wei Xu, Xiaowei Song, devicetree, linux-arm-kernel,
	linux-kernel, linux-pci

Em Thu, 22 Jul 2021 19:06:28 +0530
Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> escreveu:

> On Wed, Jul 21, 2021 at 10:39:10AM +0200, Mauro Carvalho Chehab wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 
> > Add DTS bindings for the HiKey 970 board's PCIe hardware.
> > 
> > Co-developed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> >  arch/arm64/boot/dts/hisilicon/hi3670.dtsi     | 71 +++++++++++++++++++
> >  .../boot/dts/hisilicon/hikey970-pmic.dtsi     |  1 -
> >  drivers/pci/controller/dwc/pcie-kirin.c       | 12 ----
> >  3 files changed, 71 insertions(+), 13 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> > index 1f228612192c..6dfcfcfeedae 100644
> > --- a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> > +++ b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> > @@ -177,6 +177,12 @@ sctrl: sctrl@fff0a000 {
> >  			#clock-cells = <1>;
> >  		};
> >  
> > +		pmctrl: pmctrl@fff31000 {
> > +			compatible = "hisilicon,hi3670-pmctrl", "syscon";
> > +			reg = <0x0 0xfff31000 0x0 0x1000>;
> > +			#clock-cells = <1>;
> > +		};
> > +  
> 
> Irrelevant change to this patch.

Huh?

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

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



> 
> >  		iomcu: iomcu@ffd7e000 {
> >  			compatible = "hisilicon,hi3670-iomcu", "syscon";
> >  			reg = <0x0 0xffd7e000 0x0 0x1000>;
> > @@ -660,6 +666,71 @@ gpio28: gpio@fff1d000 {
> >  			clock-names = "apb_pclk";
> >  		};
> >    
> 
> [...]
> 
> > +			#interrupt-cells = <1>;
> > +			interrupts = <0 283 4>;  
> 
> Use the DT flag for interrupts instead of hardcoded value

Do you mean like this?

	interrupts = <0 283 IRQ_TYPE_LEVEL_HIGH>;

> 
> > +			interrupt-names = "msi";
> > +			interrupt-map-mask = <0 0 0 7>;
> > +			interrupt-map = <0x0 0 0 1
> > +					 &gic GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>,
> > +					<0x0 0 0 2
> > +					 &gic GIC_SPI 283 IRQ_TYPE_LEVEL_HIGH>,
> > +					<0x0 0 0 3
> > +					 &gic GIC_SPI 284 IRQ_TYPE_LEVEL_HIGH>,
> > +					<0x0 0 0 4
> > +					 &gic GIC_SPI 285 IRQ_TYPE_LEVEL_HIGH>;
> > +		};
> > +
> >  		/* UFS */
> >  		ufs: ufs@ff3c0000 {
> >  			compatible = "hisilicon,hi3670-ufs", "jedec,ufs-2.1";
> > diff --git a/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi b/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi
> > index 48c739eacba0..03452e627641 100644
> > --- a/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi
> > +++ b/arch/arm64/boot/dts/hisilicon/hikey970-pmic.dtsi
> > @@ -73,7 +73,6 @@ ldo33: LDO33 { /* PEX8606 */
> >  					regulator-name = "ldo33";
> >  					regulator-min-microvolt = <2500000>;
> >  					regulator-max-microvolt = <3300000>;
> > -					regulator-boot-on;  
> 
> Again, irrelevant.

I'll move it to the USB patch series, where the PMIC is added.

> 
> >  				};
> >  
> >  				ldo34: LDO34 { /* GPS AUX IN VDD */
> > diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> > index bfc0513f7b15..9dad14929538 100644
> > --- a/drivers/pci/controller/dwc/pcie-kirin.c
> > +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> > @@ -347,18 +347,6 @@ static const struct regmap_config pcie_kirin_regmap_conf = {
> >  	.reg_stride = 4,
> >  };
> >  
> > -/* Registers in PCIeCTRL */
> > -static inline void kirin_apb_ctrl_writel(struct kirin_pcie *kirin_pcie,
> > -					 u32 val, u32 reg)
> > -{
> > -	writel(val, kirin_pcie->apb_base + reg);
> > -}
> > -
> > -static inline u32 kirin_apb_ctrl_readl(struct kirin_pcie *kirin_pcie, u32 reg)
> > -{
> > -	return readl(kirin_pcie->apb_base + reg);
> > -}
> > -  
> 
> Same here...

This hunk should be on patch 03/10. Probably some rebase added it here by
mistake. I'll fix it on v8.

Thanks,
Mauro

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY
  2021-07-21  8:39 ` [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY Mauro Carvalho Chehab
@ 2021-07-23 22:50   ` Rob Herring
  2021-07-24  0:12     ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 21+ messages in thread
From: Rob Herring @ 2021-07-23 22:50 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Vinod Koul, Bjorn Helgaas, linuxarm, mauro.chehab,
	Kishon Vijay Abraham I, devicetree, linux-kernel, linux-phy

On Wed, Jul 21, 2021 at 10:39:08AM +0200, Mauro Carvalho Chehab wrote:
> 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        | 95 +++++++++++++++++++
>  1 file changed, 95 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..a5ea13332cac
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
> @@ -0,0 +1,95 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/hisilicon,phy-hi3670-pcie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HiSilicon Kirin970 PCIe PHY
> +
> +maintainers:
> +  - Mauro Carvalho Chehab <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
> +
> +  phy-supply:
> +    description: The PCIe PHY power supply
> +
> +  clocks:
> +    items:
> +      - description: PCIe PHY clock
> +      - description: PCIe AUX clock
> +      - description: PCIe APB PHY clock
> +      - description: PCIe APB SYS clock
> +      - description: PCIe ACLK clock
> +
> +  clock-names:
> +    items:
> +      - const: phy_ref
> +      - const: aux
> +      - const: apb_phy
> +      - const: apb_sys
> +      - const: aclk
> +
> +  reset-gpios:
> +    description: PCI PERST reset GPIOs
> +    maxItems: 4
> +
> +  clkreq-gpios:
> +    description: Clock request GPIOs
> +    maxItems: 3

Again, this will not work. 

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

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

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

Rob

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 09/10] dt-bindings: PCI: kirin-pcie.txt: Convert it to yaml
  2021-07-21  8:39 ` [PATCH v7 09/10] dt-bindings: PCI: kirin-pcie.txt: Convert it to yaml Mauro Carvalho Chehab
@ 2021-07-23 22:56   ` Rob Herring
  0 siblings, 0 replies; 21+ messages in thread
From: Rob Herring @ 2021-07-23 22:56 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Vinod Koul, Bjorn Helgaas, linuxarm, mauro.chehab, Binghui Wang,
	Gustavo Pimentel, Jingoo Han, Xiaowei Song, devicetree,
	linux-kernel, linux-pci

On Wed, Jul 21, 2021 at 10:39:11AM +0200, Mauro Carvalho Chehab wrote:
> Convert the file into a JSON description at the yaml format.

And add 970...

> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  .../bindings/pci/hisilicon,kirin-pcie.yaml    | 87 +++++++++++++++++++
>  .../devicetree/bindings/pci/kirin-pcie.txt    | 50 -----------
>  .../devicetree/bindings/pci/snps,dw-pcie.yaml |  2 +-
>  MAINTAINERS                                   |  2 +-
>  4 files changed, 89 insertions(+), 52 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/pci/hisilicon,kirin-pcie.yaml
>  delete mode 100644 Documentation/devicetree/bindings/pci/kirin-pcie.txt
> 
> diff --git a/Documentation/devicetree/bindings/pci/hisilicon,kirin-pcie.yaml b/Documentation/devicetree/bindings/pci/hisilicon,kirin-pcie.yaml
> new file mode 100644
> index 000000000000..eabc651c9766
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/hisilicon,kirin-pcie.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/hisilicon,kirin-pcie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HiSilicon Kirin SoCs PCIe host DT description
> +
> +maintainers:
> +  - Xiaowei Song <songxiaowei@hisilicon.com>
> +  - Binghui Wang <wangbinghui@hisilicon.com>
> +
> +description: |
> +  Kirin PCIe host controller is based on the Synopsys DesignWare PCI core.
> +  It shares common functions with the PCIe DesignWare core driver and
> +  inherits common properties defined in
> +  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml.
> +
> +allOf:
> +  - $ref: /schemas/pci/snps,dw-pcie.yaml#
> +
> +properties:
> +  compatible:
> +    contains:
> +      enum:
> +        - hisilicon,kirin960-pcie
> +        - hisilicon,kirin970-pcie
> +
> +  reg:
> +    description: |
> +      Should contain rc_dbi, apb, config registers location and length.
> +    minItems: 3
> +    maxItems: 4
> +
> +  reg-names:
> +    items:
> +      - const: dbi          # controller configuration registers
> +      - const: apb          # apb Ctrl register defined by Kirin
> +      - const: config       # PCIe configuration space registers
> +      - const: phy          # apb PHY register used on Kirin 960 PHY
> +    minItems: 3
> +    maxItems: 4
> +
> +  reset-gpios:
> +    description: The GPIO(s) to generate PCIe PERST# assert and deassert signal.
> +    minItems: 1
> +    maxItems: 4

I'll apply this, but only with 'maxItems: 1' if you want to separate the 
discussion on that part.

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY
  2021-07-23 22:50   ` Rob Herring
@ 2021-07-24  0:12     ` Mauro Carvalho Chehab
  2021-07-26 21:37       ` Rob Herring
  0 siblings, 1 reply; 21+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-24  0:12 UTC (permalink / raw)
  To: Rob Herring
  Cc: Vinod Koul, Bjorn Helgaas, linuxarm, mauro.chehab,
	Kishon Vijay Abraham I, devicetree, linux-kernel, linux-phy

Em Fri, 23 Jul 2021 16:50:59 -0600
Rob Herring <robh@kernel.org> escreveu:

> On Wed, Jul 21, 2021 at 10:39:08AM +0200, Mauro Carvalho Chehab wrote:
> > 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        | 95 +++++++++++++++++++
> >  1 file changed, 95 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..a5ea13332cac
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
> > @@ -0,0 +1,95 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/phy/hisilicon,phy-hi3670-pcie.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: HiSilicon Kirin970 PCIe PHY
> > +
> > +maintainers:
> > +  - Mauro Carvalho Chehab <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
> > +
> > +  phy-supply:
> > +    description: The PCIe PHY power supply
> > +
> > +  clocks:
> > +    items:
> > +      - description: PCIe PHY clock
> > +      - description: PCIe AUX clock
> > +      - description: PCIe APB PHY clock
> > +      - description: PCIe APB SYS clock
> > +      - description: PCIe ACLK clock
> > +
> > +  clock-names:
> > +    items:
> > +      - const: phy_ref
> > +      - const: aux
> > +      - const: apb_phy
> > +      - const: apb_sys
> > +      - const: aclk
> > +
> > +  reset-gpios:
> > +    description: PCI PERST reset GPIOs
> > +    maxItems: 4
> > +
> > +  clkreq-gpios:
> > +    description: Clock request GPIOs
> > +    maxItems: 3  
> 
> Again, this will not work. 

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

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

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

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

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

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

> 
> I realize the right solution is more complex, but that's the only way to 
> handle this in a host bridge and board independent way.
> 
> If you want the simple solution, just configure all these GPIOs in 
> firmware before Linux boots.

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

Thanks,
Mauro

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 08/10] arm64: dts: HiSilicon: Add support for HiKey 970 PCIe controller hardware
  2021-07-23  6:53     ` Mauro Carvalho Chehab
@ 2021-07-24  4:11       ` Manivannan Sadhasivam
  2021-08-03  4:25         ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 21+ messages in thread
From: Manivannan Sadhasivam @ 2021-07-24  4:11 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Vinod Koul, Bjorn Helgaas, Rob Herring, linuxarm, mauro.chehab,
	Krzysztof Wilczyński, Binghui Wang, Lorenzo Pieralisi,
	Rob Herring, Wei Xu, Xiaowei Song, devicetree, linux-arm-kernel,
	linux-kernel, linux-pci

On Fri, Jul 23, 2021 at 08:53:18AM +0200, Mauro Carvalho Chehab wrote:
> Em Thu, 22 Jul 2021 19:06:28 +0530
> Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> escreveu:
> 
> > On Wed, Jul 21, 2021 at 10:39:10AM +0200, Mauro Carvalho Chehab wrote:
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > 
> > > Add DTS bindings for the HiKey 970 board's PCIe hardware.
> > > 
> > > Co-developed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > ---
> > >  arch/arm64/boot/dts/hisilicon/hi3670.dtsi     | 71 +++++++++++++++++++
> > >  .../boot/dts/hisilicon/hikey970-pmic.dtsi     |  1 -
> > >  drivers/pci/controller/dwc/pcie-kirin.c       | 12 ----
> > >  3 files changed, 71 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> > > index 1f228612192c..6dfcfcfeedae 100644
> > > --- a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> > > +++ b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> > > @@ -177,6 +177,12 @@ sctrl: sctrl@fff0a000 {
> > >  			#clock-cells = <1>;
> > >  		};
> > >  
> > > +		pmctrl: pmctrl@fff31000 {
> > > +			compatible = "hisilicon,hi3670-pmctrl", "syscon";
> > > +			reg = <0x0 0xfff31000 0x0 0x1000>;
> > > +			#clock-cells = <1>;
> > > +		};
> > > +  
> > 
> > Irrelevant change to this patch.
> 
> Huh?
> 
> This is used by PCIe PHY, as part of the power on procedures:
> 
> 	+static int hi3670_pcie_noc_power(struct hi3670_pcie_phy *phy, bool enable)
> 	+{
> 	+       struct device *dev = phy->dev;
> 	+       u32 time = 100;
> 	+       unsigned int val = NOC_PW_MASK;
> 	+       int rst;
> 	+
> 	+       if (enable)
> 	+               val = NOC_PW_MASK | NOC_PW_SET_BIT;
> 	+       else
> 	+               val = NOC_PW_MASK;
> 	+       rst = enable ? 1 : 0;
> 	+
> 	+       regmap_write(phy->pmctrl, NOC_POWER_IDLEREQ_1, val);
> 
> 

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

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

Like,

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

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

> 
> > 
> > >  		iomcu: iomcu@ffd7e000 {
> > >  			compatible = "hisilicon,hi3670-iomcu", "syscon";
> > >  			reg = <0x0 0xffd7e000 0x0 0x1000>;
> > > @@ -660,6 +666,71 @@ gpio28: gpio@fff1d000 {
> > >  			clock-names = "apb_pclk";
> > >  		};
> > >    
> > 
> > [...]
> > 
> > > +			#interrupt-cells = <1>;
> > > +			interrupts = <0 283 4>;  
> > 
> > Use the DT flag for interrupts instead of hardcoded value
> 
> Do you mean like this?
> 
> 	interrupts = <0 283 IRQ_TYPE_LEVEL_HIGH>;
> 

yes but you could also use,

	interrupts = <GIC_SPI 283 IRQ_TYPE_LEVEL_HIGH>;

Thanks,
Mani

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY
  2021-07-24  0:12     ` Mauro Carvalho Chehab
@ 2021-07-26 21:37       ` Rob Herring
  2021-07-26 23:50         ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 21+ messages in thread
From: Rob Herring @ 2021-07-26 21:37 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Vinod Koul, Bjorn Helgaas, Linuxarm, mauro.chehab,
	Kishon Vijay Abraham I, devicetree, linux-kernel, linux-phy

On Fri, Jul 23, 2021 at 6:12 PM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> Em Fri, 23 Jul 2021 16:50:59 -0600
> Rob Herring <robh@kernel.org> escreveu:
>
> > On Wed, Jul 21, 2021 at 10:39:08AM +0200, Mauro Carvalho Chehab wrote:
> > > 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        | 95 +++++++++++++++++++
> > >  1 file changed, 95 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..a5ea13332cac
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
> > > @@ -0,0 +1,95 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/phy/hisilicon,phy-hi3670-pcie.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: HiSilicon Kirin970 PCIe PHY
> > > +
> > > +maintainers:
> > > +  - Mauro Carvalho Chehab <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
> > > +
> > > +  phy-supply:
> > > +    description: The PCIe PHY power supply
> > > +
> > > +  clocks:
> > > +    items:
> > > +      - description: PCIe PHY clock
> > > +      - description: PCIe AUX clock
> > > +      - description: PCIe APB PHY clock
> > > +      - description: PCIe APB SYS clock
> > > +      - description: PCIe ACLK clock
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: phy_ref
> > > +      - const: aux
> > > +      - const: apb_phy
> > > +      - const: apb_sys
> > > +      - const: aclk
> > > +
> > > +  reset-gpios:
> > > +    description: PCI PERST reset GPIOs
> > > +    maxItems: 4
> > > +
> > > +  clkreq-gpios:
> > > +    description: Clock request GPIOs
> > > +    maxItems: 3
> >
> > Again, this will not work.
>
> Just to be sure: you're talking about the PERST# gpios (e. g. reset-gpios)
> here, right?

Both that and CLKREQ.

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

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

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

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

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

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

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

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

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

> > I realize the right solution is more complex, but that's the only way to
> > handle this in a host bridge and board independent way.
> >
> > If you want the simple solution, just configure all these GPIOs in
> > firmware before Linux boots.
>
> This won't work. The PERST# signal should be sent after initializing
> the PCIe + PHY and powering up the PEX8606 PCIe bridge chipset
> (via LDO33). That happens when the PCIe driver is loaded.

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

Rob

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY
  2021-07-26 21:37       ` Rob Herring
@ 2021-07-26 23:50         ` Mauro Carvalho Chehab
  2021-07-27  6:52           ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 21+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-26 23:50 UTC (permalink / raw)
  To: Rob Herring
  Cc: Vinod Koul, Bjorn Helgaas, Linuxarm, mauro.chehab,
	Kishon Vijay Abraham I, devicetree, linux-kernel, linux-phy

Em Mon, 26 Jul 2021 15:37:28 -0600
Rob Herring <robh@kernel.org> escreveu:

> On Fri, Jul 23, 2021 at 6:12 PM Mauro Carvalho Chehab
> <mchehab+huawei@kernel.org> wrote:
> >
> > Em Fri, 23 Jul 2021 16:50:59 -0600
> > Rob Herring <robh@kernel.org> escreveu:
> >  
> > > On Wed, Jul 21, 2021 at 10:39:08AM +0200, Mauro Carvalho Chehab wrote:  
> > > > 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        | 95 +++++++++++++++++++
> > > >  1 file changed, 95 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..a5ea13332cac
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
> > > > @@ -0,0 +1,95 @@
> > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/phy/hisilicon,phy-hi3670-pcie.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: HiSilicon Kirin970 PCIe PHY
> > > > +
> > > > +maintainers:
> > > > +  - Mauro Carvalho Chehab <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
> > > > +
> > > > +  phy-supply:
> > > > +    description: The PCIe PHY power supply
> > > > +
> > > > +  clocks:
> > > > +    items:
> > > > +      - description: PCIe PHY clock
> > > > +      - description: PCIe AUX clock
> > > > +      - description: PCIe APB PHY clock
> > > > +      - description: PCIe APB SYS clock
> > > > +      - description: PCIe ACLK clock
> > > > +
> > > > +  clock-names:
> > > > +    items:
> > > > +      - const: phy_ref
> > > > +      - const: aux
> > > > +      - const: apb_phy
> > > > +      - const: apb_sys
> > > > +      - const: aclk
> > > > +
> > > > +  reset-gpios:
> > > > +    description: PCI PERST reset GPIOs
> > > > +    maxItems: 4
> > > > +
> > > > +  clkreq-gpios:
> > > > +    description: Clock request GPIOs
> > > > +    maxItems: 3  
> > >
> > > Again, this will not work.  
> >
> > Just to be sure: you're talking about the PERST# gpios (e. g. reset-gpios)
> > here, right?  
> 
> Both that and CLKREQ.
> 
> > > It boils down to this fails to describe how the GPIOs are connected
> > > which is the point of GPIOs in DT. This in no way captures the hierarchy
> > > of devices. While you may be lucky that you can just assert or
> > > deassert all the lines at one time, that's not likely to work in a
> > > more complicated case (such as having to power up/down each device).  
> >
> > There's no way to power up/down each device, as they all share the
> > same regulator line (LDO33). So, when this is powered on, all PCI
> > devices are powered at the same time.  
> 
> I understand that for your board, but you could easily have a power
> supply per device (or multiple supplies per device).

There are probably thousands of alternatives, but I don't see any
benefit on trying to do write a very complex abstraction here.

> 
> > The original DT had names for each reset-gpio, but this was just
> > informative, as the only possible way for this hardware to work is
> > to send the PERST# signal via all GPIOs at the same time.  
> 
> What's the timing requirement here? I doubt 'at the same time' is the
> actual h/w requirement. My guess is it is before the PCI bus scan if
> you don't have any hook before each child bus is scanned.

No idea, as the available documentation doesn't mention. As this is an 
old hardware, finding any extra documentation about it is not easy,
and, afaikt, the developers who originally worked on it (back in 2017) 
were already moved to work with least two or three generations that came
after this SoC. So, we need to check what the code does.

Looking at the code, both the PERST# signals and the CLKREQ happen
during the PHY power on sequence, and don't require any special
order. They just need to be initialized altogether at the same
power on step. The power on steps are:

1. turn on the power supply from the regulator to feed the bridge
   and other PCI devices;
2. turn on the PHY;
3. request clocks;
4. set PHY clock and enable the other clocks;
5. configure some parameters at the PHY layer;
6. send the PERST# signals. This part has actually a 21 ms delay before
   sending the signal and will wait for 10ms after sending PERST# to
   all PCI devices. The actual PERST# code is:

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

   In summary, all PERST# signals are sent (about) the same time,
   and the driver logic will wait for 10 ms.

7. Wait for the PCIe to be stable;

8. Adjust the eye parameter;

9. power off NOC.

All the above happens before the PCI bus scan.


> > Ok, we might overdesign the DT, in order to consider a non-existent
> > scenario where it would be possible to power on and reset the devices
> > in separate, but I can't think on a way to do that, except by maybe
> > creating virtual phy (or pcie) DT nodes, one for each combination of
> > regulator + PERST#, and have separate drivers for each one. Such kind
> > of scenario only makes sense when each PCIe device can be powered on
> > independently (which is not the case here).  
> 
> If someone made hikey970 with the topology you have, then someone can
> just as easily make a different topology and one that doesn't work
> with the assumptions you've made. We're only going to see more and
> more embedded boards with multiple PCI devices.

I wouldn't expect a new device using this chipset being upstreamed.
As I said before, there are 3 generations after Kirin 970.

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

I'll try to find something, but still I think that this is overdesign,
as this is really a single event that was split on multiple GPIOs just
due to some voltage/current/temperature constraints.

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

Yeah, I understand that DT changes would be painful, but, IMHO,
writing something very complex here just because the hardware design
opted to use multiple GPIOs instead of a single one is overkill.

> > > I realize the right solution is more complex, but that's the only way to
> > > handle this in a host bridge and board independent way.
> > >
> > > If you want the simple solution, just configure all these GPIOs in
> > > firmware before Linux boots.  
> >
> > This won't work. The PERST# signal should be sent after initializing
> > the PCIe + PHY and powering up the PEX8606 PCIe bridge chipset
> > (via LDO33). That happens when the PCIe driver is loaded.  
> 
> Only because you have no hooks for handling PERST# on devices
> downstream of the PEX8606. Surely a sequence like this would work:
> deassert root PERST# (to PEX8606), scan root bus, find and init PCIe
> bridge, deassert PEX8606 child bus(es) PERST#, scan child bus(es),
> find and init child devices. I think the .add_bus() hook could work
> for you. IIRC, that's called before a child bus is scanned.

As explained above, after sending the PERST# signal and wait for
10 ms, it tunes the PHY eye diagram and powers off NOC.

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


Thanks,
Mauro

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY
  2021-07-26 23:50         ` Mauro Carvalho Chehab
@ 2021-07-27  6:52           ` Mauro Carvalho Chehab
  2021-07-27 22:17             ` Rob Herring
  0 siblings, 1 reply; 21+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-27  6:52 UTC (permalink / raw)
  To: Rob Herring
  Cc: Vinod Koul, Bjorn Helgaas, Linuxarm, mauro.chehab,
	Kishon Vijay Abraham I, devicetree, linux-kernel, linux-phy

Em Tue, 27 Jul 2021 01:50:20 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> Em Mon, 26 Jul 2021 15:37:28 -0600
> Rob Herring <robh@kernel.org> escreveu:
> 

> > > > > +  reset-gpios:
> > > > > +    description: PCI PERST reset GPIOs
> > > > > +    maxItems: 4
> > > > > +
> > > > > +  clkreq-gpios:
> > > > > +    description: Clock request GPIOs
> > > > > +    maxItems: 3    
> > > >
> > > > Again, this will not work.    
> > >
> > > Just to be sure: you're talking about the PERST# gpios (e. g. reset-gpios)
> > > here, right?    
> > 
> > Both that and CLKREQ.

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

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

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

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

	reset-gpios = <&gpio7 0 0 >;

which maps to the PEX8606 PCIe bridge chip.

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

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

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

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


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

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

Would something like that be acceptable for you?

> > > If you have a better idea, I'm all ears.    
> > 
> > There's already a spec for populating PCI devices in DT. It's existed
> > for over 20 years with OpenFirmware[1]. It's not widely used on FDT
> > systems because most cases to date are just a single device attached
> > and they don't have extra things needing to be described in DT. There
> > are a few, but not many examples in the tree of PCI devices with DT
> > nodes. That's the only way to generically describe the topology you
> > have.  
> >
> > [1] https://www.devicetree.org/open-firmware/home.html#OFDbussupps  

I was unable to find anything useful there at the two PCI documents.

This one:
	https://www.devicetree.org/open-firmware/bindings/pci/pci-express.txt

has just one property that might be useful:

	physical-slot#

The main one:
	https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf

mentions a few child properties, but it doesn't show how those were
supposed to be mapped, and none of the properties mentioned there
specify clocks, gpios, or reset pins.

Thanks,
Mauro

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY
  2021-07-27  6:52           ` Mauro Carvalho Chehab
@ 2021-07-27 22:17             ` Rob Herring
  2021-07-28  7:38               ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 21+ messages in thread
From: Rob Herring @ 2021-07-27 22:17 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Vinod Koul, Bjorn Helgaas, Linuxarm, mauro.chehab,
	Kishon Vijay Abraham I, devicetree, linux-kernel, linux-phy

On Tue, Jul 27, 2021 at 12:52 AM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> Em Tue, 27 Jul 2021 01:50:20 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
>
> > Em Mon, 26 Jul 2021 15:37:28 -0600
> > Rob Herring <robh@kernel.org> escreveu:
> >
>
> > > > > > +  reset-gpios:
> > > > > > +    description: PCI PERST reset GPIOs
> > > > > > +    maxItems: 4
> > > > > > +
> > > > > > +  clkreq-gpios:
> > > > > > +    description: Clock request GPIOs
> > > > > > +    maxItems: 3
> > > > >
> > > > > Again, this will not work.
> > > >
> > > > Just to be sure: you're talking about the PERST# gpios (e. g. reset-gpios)
> > > > here, right?
> > >
> > > Both that and CLKREQ.
>
> The original DT from the downstream version (found at Linaro's tree)
> has:
>
>         pcie@f4000000 {
>                 compatible = "hisilicon,hikey970";
> ...
>                 switch,reset-gpios = <&gpio7 0 0 >;
>                 eth,reset-gpios = <&gpio25 2 0 >;
>                 m_2,reset-gpios = <&gpio3 1 0 >;
>                 mini1,reset-gpios = <&gpio27 4 0 >;
>
>                 eth,clkreq-gpios = <&gpio20 6 0 >;
>                 m_2,clkreq-gpios = <&gpio27 3 0 >;
>                 mini1,clkreq-gpios = <&gpio17 0 0 >;
>         };
>
> So, if we're willing to have a single reset-gpios for the PCIe
> interface, in order to follow the current pci-bus.yaml schema,
> this would probably be:
>
>         reset-gpios = <&gpio7 0 0 >;
>
> which maps to the PEX8606 PCIe bridge chip.
>
> With that, DT still need to point a per-slot clkreq and
> reset-gpio.
>
> One alternative would be to map it as either 3 PCI or PHY
> child nodes. E. g. something like this:
>
>         pcie@f4000000 {
>                 compatible = "hisilicon,kirin970-pcie";
> ...
>                 reset-gpios = <&gpio7 0 0 >;
>
>                 slot {
>                         eth {
>                                 reset-gpios = <&gpio25 2 0>;
>                                 clkreq-gpios = <&gpio20 6 0>;
>                         };
>                         m2 {
>                                 reset-gpios = <&gpio3 1 0>;
>                                 clkreq-gpios = <&gpio27 3 0>;
>                         };
>                         mini1 {
>                                 reset-gpios = <&gpio27 4 0>;
>                                 clkreq-gpios = <&gpio17 0 0>;
>                         };
>                 };
>         };
>
>
> Placing the child nodes ("slot"?) at the pci bus properties makes more
> sense to me, but placing them at the PHY node has the advantage of
> only affecting Kirin 970.
>
> In either case, if each child would need a different power supply,
> it won't be hard to add a "slot-supply" property later on.
>
> Would something like that be acceptable for you?

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

pcie@f4000000 { // RP: Bus 0, Device 0
    compatible = "hisilicon,kirin970-pcie";
    ...
    reset-gpios = <&gpio7 0 0>;  // PERST to switch

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

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

        pcie@4 { // M.2
            reg = <0x200 0 0 0 0>;
            compatible = "pciclass,0604";
            device_type = "pci";
            reset-gpios = <&gpio7 1 0>; // PERST to M.2 slot
       };

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

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

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

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

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

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

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

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

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

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

Rob

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY
  2021-07-27 22:17             ` Rob Herring
@ 2021-07-28  7:38               ` Mauro Carvalho Chehab
  2021-07-28 14:28                 ` Rob Herring
  0 siblings, 1 reply; 21+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-28  7:38 UTC (permalink / raw)
  To: Rob Herring
  Cc: Vinod Koul, Bjorn Helgaas, Linuxarm, mauro.chehab,
	Kishon Vijay Abraham I, devicetree, linux-kernel, linux-phy

Em Tue, 27 Jul 2021 16:17:43 -0600
Rob Herring <robh@kernel.org> escreveu:

> On Tue, Jul 27, 2021 at 12:52 AM Mauro Carvalho Chehab
> <mchehab+huawei@kernel.org> wrote:
> >
> > Em Tue, 27 Jul 2021 01:50:20 +0200
> > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> >  
> > > Em Mon, 26 Jul 2021 15:37:28 -0600
> > > Rob Herring <robh@kernel.org> escreveu:
> > >  
> >  
> > > > > > > +  reset-gpios:
> > > > > > > +    description: PCI PERST reset GPIOs
> > > > > > > +    maxItems: 4
> > > > > > > +
> > > > > > > +  clkreq-gpios:
> > > > > > > +    description: Clock request GPIOs
> > > > > > > +    maxItems: 3  
> > > > > >
> > > > > > Again, this will not work.  
> > > > >
> > > > > Just to be sure: you're talking about the PERST# gpios (e. g. reset-gpios)
> > > > > here, right?  
> > > >
> > > > Both that and CLKREQ.  
> >
> > The original DT from the downstream version (found at Linaro's tree)
> > has:
> >
> >         pcie@f4000000 {
> >                 compatible = "hisilicon,hikey970";
> > ...
> >                 switch,reset-gpios = <&gpio7 0 0 >;
> >                 eth,reset-gpios = <&gpio25 2 0 >;
> >                 m_2,reset-gpios = <&gpio3 1 0 >;
> >                 mini1,reset-gpios = <&gpio27 4 0 >;
> >
> >                 eth,clkreq-gpios = <&gpio20 6 0 >;
> >                 m_2,clkreq-gpios = <&gpio27 3 0 >;
> >                 mini1,clkreq-gpios = <&gpio17 0 0 >;
> >         };
> >
> > So, if we're willing to have a single reset-gpios for the PCIe
> > interface, in order to follow the current pci-bus.yaml schema,
> > this would probably be:
> >
> >         reset-gpios = <&gpio7 0 0 >;
> >
> > which maps to the PEX8606 PCIe bridge chip.
> >
> > With that, DT still need to point a per-slot clkreq and
> > reset-gpio.
> >
> > One alternative would be to map it as either 3 PCI or PHY
> > child nodes. E. g. something like this:
> >
> >         pcie@f4000000 {
> >                 compatible = "hisilicon,kirin970-pcie";
> > ...
> >                 reset-gpios = <&gpio7 0 0 >;
> >
> >                 slot {
> >                         eth {
> >                                 reset-gpios = <&gpio25 2 0>;
> >                                 clkreq-gpios = <&gpio20 6 0>;
> >                         };
> >                         m2 {
> >                                 reset-gpios = <&gpio3 1 0>;
> >                                 clkreq-gpios = <&gpio27 3 0>;
> >                         };
> >                         mini1 {
> >                                 reset-gpios = <&gpio27 4 0>;
> >                                 clkreq-gpios = <&gpio17 0 0>;
> >                         };
> >                 };
> >         };
> >
> >
> > Placing the child nodes ("slot"?) at the pci bus properties makes more
> > sense to me, but placing them at the PHY node has the advantage of
> > only affecting Kirin 970.
> >
> > In either case, if each child would need a different power supply,
> > it won't be hard to add a "slot-supply" property later on.
> >
> > Would something like that be acceptable for you?  
> 
> On the right track, but there's already a definition for what child
> devices look like in pci2_1.pdf. I think you want something like this:
> 
> pcie@f4000000 { // RP: Bus 0, Device 0
>     compatible = "hisilicon,kirin970-pcie";
>     ...
>     reset-gpios = <&gpio7 0 0>;  // PERST to switch
> 
>     pcie@0 { // PCIe switch: Bus 1, Device 0
>         reg = <0 0 0 0 0>;
>         compatible = "pciclass,0604";
>         device_type = "pci";
> 
>         pcie@1 { // NC (Can omit this node)
>             reg = <0x80 0 0 0 0>;
>             compatible = "pciclass,0604";
>             device_type = "pci";
>         };
> 
>         pcie@4 { // M.2
>             reg = <0x200 0 0 0 0>;

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


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

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

>        };
> 
>         pcie@5 { // Mini
>             reg = <0x280 0 0 0 0>;
>             compatible = "pciclass,0604";
>             device_type = "pci";
>             reset-gpios = <&gpio7 2 0>; // PERST to Mini slot
>         };
> 
>         pcie@7 { // Ethernet
>             reg = <0x380 0 0 0 0>;
>             compatible = "pciclass,0604";
>             device_type = "pci";
>             reset-gpios = <&gpio7 3 0>; // PERST to Ethernet
> 
>             ethernet@0 {
>                 reg = <0 0 0 0 0>;
>                 local-mac-address = [ 00 01 02 03 04 05 06 ];
>             };

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

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

Which seems to be a valid one:

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

>         };
> 
>         pcie@9 { // NC
>             reg = <0x480 0 0 0 0>;
>             compatible = "pciclass,0604";
>             device_type = "pci";
>        };
> };
> 
> This is based on what you previously sent:
> 00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3670 (rev 01)
> 01:00.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 02:01.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 02:04.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 02:05.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 02:07.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 02:09.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 06:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd.
> RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 07)
> 
> A few notes:
> I left out #size-cells, #address-cells, and ranges for brevity.
> 
> I'm not completely sure I've got the bridges mapped to the right
> functions on Hikey970. That's my best guess looking at the schematics.
> You should be able to confirm which bridge is the parent bridge for
> ethernet at least.

I added a NVMe to M.2 slot:

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

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

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

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

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

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

Thanks!

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

The power-on sequence is:

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

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

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

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

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

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

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

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

Thanks,
Mauro

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY
  2021-07-28  7:38               ` Mauro Carvalho Chehab
@ 2021-07-28 14:28                 ` Rob Herring
  2021-07-29 10:12                   ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 21+ messages in thread
From: Rob Herring @ 2021-07-28 14:28 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Vinod Koul, Bjorn Helgaas, Linuxarm, mauro.chehab,
	Kishon Vijay Abraham I, devicetree, linux-kernel, linux-phy

On Wed, Jul 28, 2021 at 1:38 AM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> Em Tue, 27 Jul 2021 16:17:43 -0600
> Rob Herring <robh@kernel.org> escreveu:
>
> > On Tue, Jul 27, 2021 at 12:52 AM Mauro Carvalho Chehab
> > <mchehab+huawei@kernel.org> wrote:
> > >
> > > Em Tue, 27 Jul 2021 01:50:20 +0200
> > > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> > >
> > > > Em Mon, 26 Jul 2021 15:37:28 -0600
> > > > Rob Herring <robh@kernel.org> escreveu:
> > > >
> > >
> > > > > > > > +  reset-gpios:
> > > > > > > > +    description: PCI PERST reset GPIOs
> > > > > > > > +    maxItems: 4
> > > > > > > > +
> > > > > > > > +  clkreq-gpios:
> > > > > > > > +    description: Clock request GPIOs
> > > > > > > > +    maxItems: 3
> > > > > > >
> > > > > > > Again, this will not work.
> > > > > >
> > > > > > Just to be sure: you're talking about the PERST# gpios (e. g. reset-gpios)
> > > > > > here, right?
> > > > >
> > > > > Both that and CLKREQ.
> > >
> > > The original DT from the downstream version (found at Linaro's tree)
> > > has:
> > >
> > >         pcie@f4000000 {
> > >                 compatible = "hisilicon,hikey970";
> > > ...
> > >                 switch,reset-gpios = <&gpio7 0 0 >;
> > >                 eth,reset-gpios = <&gpio25 2 0 >;
> > >                 m_2,reset-gpios = <&gpio3 1 0 >;
> > >                 mini1,reset-gpios = <&gpio27 4 0 >;
> > >
> > >                 eth,clkreq-gpios = <&gpio20 6 0 >;
> > >                 m_2,clkreq-gpios = <&gpio27 3 0 >;
> > >                 mini1,clkreq-gpios = <&gpio17 0 0 >;
> > >         };
> > >
> > > So, if we're willing to have a single reset-gpios for the PCIe
> > > interface, in order to follow the current pci-bus.yaml schema,
> > > this would probably be:
> > >
> > >         reset-gpios = <&gpio7 0 0 >;
> > >
> > > which maps to the PEX8606 PCIe bridge chip.
> > >
> > > With that, DT still need to point a per-slot clkreq and
> > > reset-gpio.
> > >
> > > One alternative would be to map it as either 3 PCI or PHY
> > > child nodes. E. g. something like this:
> > >
> > >         pcie@f4000000 {
> > >                 compatible = "hisilicon,kirin970-pcie";
> > > ...
> > >                 reset-gpios = <&gpio7 0 0 >;
> > >
> > >                 slot {
> > >                         eth {
> > >                                 reset-gpios = <&gpio25 2 0>;
> > >                                 clkreq-gpios = <&gpio20 6 0>;
> > >                         };
> > >                         m2 {
> > >                                 reset-gpios = <&gpio3 1 0>;
> > >                                 clkreq-gpios = <&gpio27 3 0>;
> > >                         };
> > >                         mini1 {
> > >                                 reset-gpios = <&gpio27 4 0>;
> > >                                 clkreq-gpios = <&gpio17 0 0>;
> > >                         };
> > >                 };
> > >         };
> > >
> > >
> > > Placing the child nodes ("slot"?) at the pci bus properties makes more
> > > sense to me, but placing them at the PHY node has the advantage of
> > > only affecting Kirin 970.
> > >
> > > In either case, if each child would need a different power supply,
> > > it won't be hard to add a "slot-supply" property later on.
> > >
> > > Would something like that be acceptable for you?
> >
> > On the right track, but there's already a definition for what child
> > devices look like in pci2_1.pdf. I think you want something like this:
> >
> > pcie@f4000000 { // RP: Bus 0, Device 0
> >     compatible = "hisilicon,kirin970-pcie";
> >     ...
> >     reset-gpios = <&gpio7 0 0>;  // PERST to switch
> >
> >     pcie@0 { // PCIe switch: Bus 1, Device 0
> >         reg = <0 0 0 0 0>;
> >         compatible = "pciclass,0604";
> >         device_type = "pci";
> >
> >         pcie@1 { // NC (Can omit this node)
> >             reg = <0x80 0 0 0 0>;
> >             compatible = "pciclass,0604";
> >             device_type = "pci";
> >         };
> >
> >         pcie@4 { // M.2
> >             reg = <0x200 0 0 0 0>;
>
> Not sure what to put at reg. I suspect that the best would be to follow
> the PEX port number, as, if one day someone decides to implement an
> I2C driver, this might be useful.

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


> >             compatible = "pciclass,0604";
> >             device_type = "pci";
> >             reset-gpios = <&gpio7 1 0>; // PERST to M.2 slot
>
> We also need the clock-req phandle for the three devices.

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

>
> >        };
> >
> >         pcie@5 { // Mini
> >             reg = <0x280 0 0 0 0>;
> >             compatible = "pciclass,0604";
> >             device_type = "pci";
> >             reset-gpios = <&gpio7 2 0>; // PERST to Mini slot
> >         };
> >
> >         pcie@7 { // Ethernet
> >             reg = <0x380 0 0 0 0>;
> >             compatible = "pciclass,0604";
> >             device_type = "pci";
> >             reset-gpios = <&gpio7 3 0>; // PERST to Ethernet
> >
> >             ethernet@0 {
> >                 reg = <0 0 0 0 0>;
> >                 local-mac-address = [ 00 01 02 03 04 05 06 ];
> >             };
>
> No need to add a mac address here. The Ethernet card has a mac
> already:

That was just for illustration of what a device node would look like
should you need extra properties for a device. If you only needed to
set the MAC address, guess what, you need to create the hierarchy
above.

>
>         60:fa:9d:xx:xx:xx
>
> Which seems to be a valid one:
>
>         https://hwaddress.com/?q=60%3Afa%3A9d
>
> >         };
> >
> >         pcie@9 { // NC
> >             reg = <0x480 0 0 0 0>;
> >             compatible = "pciclass,0604";
> >             device_type = "pci";
> >        };
> > };
> >
> > This is based on what you previously sent:
> > 00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3670 (rev 01)
> > 01:00.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> > Express Gen 2 (5.0 GT/s) Switch (rev ba)
> > 02:01.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> > Express Gen 2 (5.0 GT/s) Switch (rev ba)
> > 02:04.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> > Express Gen 2 (5.0 GT/s) Switch (rev ba)
> > 02:05.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> > Express Gen 2 (5.0 GT/s) Switch (rev ba)
> > 02:07.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> > Express Gen 2 (5.0 GT/s) Switch (rev ba)
> > 02:09.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
> > Express Gen 2 (5.0 GT/s) Switch (rev ba)
> > 06:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd.
> > RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 07)
> >
> > A few notes:
> > I left out #size-cells, #address-cells, and ranges for brevity.
> >
> > I'm not completely sure I've got the bridges mapped to the right
> > functions on Hikey970. That's my best guess looking at the schematics.
> > You should be able to confirm which bridge is the parent bridge for
> > ethernet at least.
>
> I added a NVMe to M.2 slot:
>
> $ lspci -D -P -PP
> 0000:00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3670 (rev 01)
> 0000:00:00.0/01:00.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 0000:00:00.0/01:00.0/02:01.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 0000:00:00.0/01:00.0/02:04.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 0000:00:00.0/01:00.0/02:05.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 0000:00:00.0/01:00.0/02:07.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 0000:00:00.0/01:00.0/02:09.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba)
> 0000:00:00.0/01:00.0/02:01.0/03:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd Device a809
> 0000:00:00.0/01:00.0/02:07.0/06:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 07)
>
> it sounds that all devices are behind the PEX 8606 bridge:
>         - port 1 seems to be the M.2 slot
>         - port 7 seems to be the Ethernet adapter
>
> I don't have any mini PCIe devices here. I'll try to get one in order to be
> sure about the topology.

I found the mapping in table 4-1 from
https://docs.broadcom.com/doc/PEX_8606-BA_Data_Book_v1.3_31Mar11.pdf

So it is like this:
Device 0 - lane 0 - upstream
Device 1 - lane 4 - m.2
Device 5 - lane 5 - mini PCIe
Device 7 - lane 6 - ethernet

'lane' is the signal numbering in the schematics.

>
> > The compatible strings aren't strictly needed. Linux doesn't look at them.
>
> If not needed, IMO the best would be to not add it, keeping it as
> simple as possible.

I would say that anything with extra properties should have a compatible.

> >
> > There's a pretty complete example in:
> > arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi
>
> Thanks!
>
> > The simplest Linux implementation to handle the above is just walk the
> > child nodes and get all the 'reset-gpios' properties. That's not the
> > implementation I think we should have though. We should handle the
> > GPIO as each bridge is probed and children scanned.
>
> The power-on sequence is:
>
>         1. CLK, PHY and DWC initialization;
>         2. 21ms delay;
>         3. PERST# sent to each device;
>         4. 10ms delay;
>         5. adjust the eye diagram;
>         6. power off NOC.
>
> Most of the above are at the PHY driver. Now, it would be possible to
> map those as:
>
>         phy->init() - steps 1 and 2;
>         phy->power_on() - steps 4, 5 and 6.
>
> And change somehow the pcie-kirin driver to only call phy->power_on()
> after doing the bus probing sequence, but a change like that would mean
> that the eye diagram will only be adjusted at the end. That doesn't
> sound a good idea to me, as probing devices with a wrong eye diagram
> could cause bit errors when talking to the devices inside the PCI bus.
> This is likely not a problem if all devices are directly connected to
> the hardware, but it could be an issue if someone uses either a M.2 or
> a mini-PCI extensor.

The eye diagram only applies to the link between the RP and switch.
There's no way that any of the downstream PERSTs matter, that's just
not logical. So you only need to deassert PERST on that link.

What happens if you only handle the switch PERST and CLKREQ? You
should simply only discover the switch and no downstream devices.

> So, IMO, the best would be for the PHY driver to walk the child nodes
> and get all the 'reset-gpios' properties.
>
> With regards to the clock-req phandles, those should be enabled before
> the PHY clocks, in order to avoid the SError issue.

Huh? What exactly causes an SError. Has to be some bus access. But
again, it's only going to be the one for the RP link that matters
here.

> It should be easy to implement this at the the PCIe driver, but, this
> should happen in early stages at the power-on sequence (before enabling
> the DWC PHY clocks). So, the PCIe driver (or the PHY) will need to
> walk the child nodes and get all the 'reset-gpios' properties.
>
> For the sake of avoiding to duplicate the walk-though and parsing
> logic, I would do it only at the PHY driver.

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

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

Rob

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY
  2021-07-28 14:28                 ` Rob Herring
@ 2021-07-29 10:12                   ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 21+ messages in thread
From: Mauro Carvalho Chehab @ 2021-07-29 10:12 UTC (permalink / raw)
  To: Rob Herring
  Cc: Vinod Koul, Bjorn Helgaas, Linuxarm, mauro.chehab,
	Kishon Vijay Abraham I, devicetree, linux-kernel, linux-phy

Hi Rob,

Em Wed, 28 Jul 2021 08:28:26 -0600
Rob Herring <robh@kernel.org> escreveu:

> > > pcie@f4000000 { // RP: Bus 0, Device 0
> > >     compatible = "hisilicon,kirin970-pcie";
> > >     ...
> > >     reset-gpios = <&gpio7 0 0>;  // PERST to switch
> > >
> > >     pcie@0 { // PCIe switch: Bus 1, Device 0
> > >         reg = <0 0 0 0 0>;
> > >         compatible = "pciclass,0604";
> > >         device_type = "pci";
> > >
> > >         pcie@1 { // NC (Can omit this node)
> > >             reg = <0x80 0 0 0 0>;
> > >             compatible = "pciclass,0604";
> > >             device_type = "pci";
> > >         };
> > >
> > >         pcie@4 { // M.2
> > >             reg = <0x200 0 0 0 0>;  
> >
> > Not sure what to put at reg. I suspect that the best would be to follow
> > the PEX port number, as, if one day someone decides to implement an
> > I2C driver, this might be useful.  
> 
> It's defined in the PCI bus binding. Basically, it's the BDF of the
> device. However, as the bus number is dynamic, I think we want to
> leave that as 0 for FDT. The function is optional and always 0 in this
> case.

Ok. I'll use 0 there.

> > >             compatible = "pciclass,0604";
> > >             device_type = "pci";
> > >             reset-gpios = <&gpio7 1 0>; // PERST to M.2 slot  
> >
> > We also need the clock-req phandle for the three devices.  
> 
> Hopefully, you can figure out where those belong now...

I added it here too.

> > With regards to the clock-req phandles, those should be enabled before
> > the PHY clocks, in order to avoid the SError issue.  
> 
> Huh? What exactly causes an SError. Has to be some bus access.

I'm not sure, but I got lots of those when playing with drivers
for this SoC. It is not easy to check the root case when such
errors happen, as the backtrace is useless.

> 
> > It should be easy to implement this at the the PCIe driver, but, this
> > should happen in early stages at the power-on sequence (before enabling
> > the DWC PHY clocks). So, the PCIe driver (or the PHY) will need to
> > walk the child nodes and get all the 'reset-gpios' properties.
> >
> > For the sake of avoiding to duplicate the walk-though and parsing
> > logic, I would do it only at the PHY driver.  
> 
> Everyone else handles this stuff in their PCIe driver. You are not special...
> 
> I have a plan to make the PERST handling common across all PCIe host
> drivers and also make the PHY handling common across DWC drivers.
> Don't make that harder.

It turns that changing this to work the way you're expecting was
a lot simpler than I was expecting ;-)

The enclosed patch, applied on the top of my past series, does that.

I'll rebase my patch series to take this change into account.

Could you please check if the DTS there is OK from your side?
I'll then change the dt-schema to match such change.

Thanks,
Mauro

[PATCH] PCI: kirin: change DT schema to use child nodes

Instead of having multiple PERST# pins as part of pcie,
add PCIe child slots, and place there the information related
to clock request and PERST# signals.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

diff --git a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
index cae90cd0b06a..a17562bb4dff 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
@@ -687,9 +687,6 @@ pcie_phy: pcie-phy@fc000000 {
 				      "apb_phy", "apb_sys",
 				      "aclk";
 
-			clkreq-gpios = <&gpio20 6 0 >, <&gpio27 3 0 >,
-				       <&gpio17 0 0 >;
-
 			/* vboost iboost pre post main */
 			hisilicon,eye-diagram-param = <0xFFFFFFFF 0xFFFFFFFF
 						       0xFFFFFFFF 0xFFFFFFFF
@@ -726,8 +723,40 @@ &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>;
-			reset-gpios = <&gpio7 0 0 >, <&gpio25 2 0 >,
-				      <&gpio3 1 0 >, <&gpio27 4 0 >;
+			reset-gpios = <&gpio7 0 0 >;
+
+			pcie@4 { // Lane 4: M.2
+				reg = <0 0 0 0 0>;
+				compatible = "pciclass,0604";
+				device_type = "pci";
+				reset-gpios = <&gpio7 1 0>;
+				clkreq-gpios = <&gpio27 3 0 >;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				ranges;
+			};
+
+			pcie@5 { // Lane 5: Mini PCIe
+				reg = <0 0 0 0 0>;
+				compatible = "pciclass,0604";
+				device_type = "pci";
+				reset-gpios = <&gpio7 2 0>;
+				clkreq-gpios = <&gpio17 0 0 >;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				ranges;
+			};
+
+			pcie@7 { // Lane 7: Ethernet
+				reg = <0 0 0 0 0>;
+				compatible = "pciclass,0604";
+				device_type = "pci";
+				reset-gpios = <&gpio7 3 0>;
+				clkreq-gpios = <&gpio20 0 0 >;
+				#address-cells = <3>;
+				#size-cells = <2>;
+				ranges;
+			};
 		};
 
 		/* UFS */
diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index 9dad14929538..5f515c4c6076 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -52,6 +52,19 @@
 #define PCIE_DEBOUNCE_PARAM	0xF0F400
 #define PCIE_OE_BYPASS		(0x3 << 28)
 
+/*
+ * Max number of connected PCI slots at an external PCI bridge
+ *
+ * This is used on HiKey 970, which has a PEX 8606 bridge with has
+ * 4 connected lanes (lane 0 upstream, and the other tree lanes,
+ * one connected to an in-board Ethernet adapter and the other two
+ * connected to M.2 and mini PCI slots.
+ *
+ * Each slot has a different clock source and uses a separate PERST#
+ * pin.
+ */
+#define MAX_PCI_SLOTS		3
+
 enum pcie_kirin_phy_type {
 	PCIE_KIRIN_INTERNAL_PHY,
 	PCIE_KIRIN_EXTERNAL_PHY
@@ -64,6 +77,18 @@ struct kirin_pcie {
 	struct regmap   *apb;
 	struct phy	*phy;
 	void		*phy_priv;	/* only for PCIE_KIRIN_INTERNAL_PHY */
+
+	/* DWC PERST# */
+	int		gpio_id_dwc_perst;
+
+	int		num_slots;
+	/* Per-slot PERST# */
+	int		gpio_id_reset[MAX_PCI_SLOTS];
+	const char	*reset_names[MAX_PCI_SLOTS];
+
+	/* Per-slot clkreq */
+	int		gpio_id_clkreq[MAX_PCI_SLOTS];
+	const char	*clkreq_names[MAX_PCI_SLOTS];
 };
 
 /*
@@ -108,7 +133,6 @@ struct hi3660_pcie_phy {
 	struct clk	*phy_ref_clk;
 	struct clk	*aclk;
 	struct clk	*aux_clk;
-	int		gpio_id_reset;
 };
 
 /* Registers in PCIePHY */
@@ -171,16 +195,6 @@ static int hi3660_pcie_phy_get_resource(struct hi3660_pcie_phy *phy)
 	if (IS_ERR(phy->sysctrl))
 		return PTR_ERR(phy->sysctrl);
 
-	/* gpios */
-	phy->gpio_id_reset = of_get_named_gpio(dev->of_node,
-					       "reset-gpios", 0);
-	if (phy->gpio_id_reset == -EPROBE_DEFER) {
-		return -EPROBE_DEFER;
-	} else if (!gpio_is_valid(phy->gpio_id_reset)) {
-		dev_err(phy->dev, "unable to get a valid gpio pin\n");
-		return -ENODEV;
-	}
-
 	return 0;
 }
 
@@ -297,16 +311,6 @@ static int hi3660_pcie_phy_power_on(struct kirin_pcie *pcie)
 	if (ret)
 		goto disable_clks;
 
-	/* 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:
 	hi3660_pcie_phy_clk_ctrl(phy, false);
 	return ret;
@@ -347,11 +351,54 @@ static const struct regmap_config pcie_kirin_regmap_conf = {
 	.reg_stride = 4,
 };
 
+static int kirin_pcie_parse_port(struct kirin_pcie *kirin_pcie,
+				 struct platform_device *pdev,
+				 struct device_node *node)
+{
+	struct device *dev = &pdev->dev;
+	int i = kirin_pcie->num_slots;
+	char name[32];
+
+	if (i + 1 > MAX_PCI_SLOTS) {
+		dev_err(dev, "Too many PCI slots!\n");
+		return -EINVAL;
+	}
+
+	kirin_pcie->num_slots++;
+
+	/* perst reset gpio */
+	kirin_pcie->gpio_id_reset[i] = of_get_named_gpio(node,
+							 "reset-gpios", 0);
+
+	if (kirin_pcie->gpio_id_reset[i] < 0) {
+		dev_err(dev, "%d: Missing PERST# for %pOF\n", i, node);
+		return kirin_pcie->gpio_id_reset[i];
+	}
+
+	/* clkreq gpio */
+	kirin_pcie->gpio_id_clkreq[i] = of_get_named_gpio(node,
+							  "clkreq-gpios", 0);
+	if (kirin_pcie->gpio_id_clkreq[i] < 0) {
+		dev_err(dev, "%d: Missing clqreq for %pOF\n", i, node);
+		return kirin_pcie->gpio_id_clkreq[i];
+	}
+
+	sprintf(name, "pcie_clkreq_%d", i);
+	kirin_pcie->clkreq_names[i] = devm_kstrdup_const(dev, name,
+							 GFP_KERNEL);
+	if (!kirin_pcie->clkreq_names[i])
+		return -ENOMEM;
+
+	return 0;
+}
+
 static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
 				    struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node, *child;
 	void __iomem *apb_base;
+	int ret;
 
 	apb_base = devm_platform_ioremap_resource_byname(pdev, "apb");
 	if (IS_ERR(apb_base))
@@ -362,6 +409,29 @@ static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
 	if (IS_ERR(kirin_pcie->apb))
 		return PTR_ERR(kirin_pcie->apb);
 
+	/* pcie internal PERST# gpio */
+	kirin_pcie->gpio_id_dwc_perst = of_get_named_gpio(dev->of_node,
+							  "reset-gpios", 0);
+	if (kirin_pcie->gpio_id_dwc_perst == -EPROBE_DEFER) {
+		return -EPROBE_DEFER;
+	} else if (!gpio_is_valid(kirin_pcie->gpio_id_dwc_perst)) {
+		dev_err(dev, "unable to get a valid gpio pin\n");
+		return -ENODEV;
+	}
+
+	/* Parse OF children */
+	for_each_available_child_of_node(node, child) {
+		ret = of_pci_get_devfn(child);
+		if (ret < 0) {
+			dev_info(dev, "failed to parse devfn: %d\n", ret);
+			return ret;
+		}
+
+		ret = kirin_pcie_parse_port(kirin_pcie, pdev, child);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -419,9 +489,31 @@ static int kirin_pcie_wr_own_conf(struct pci_bus *bus, unsigned int devfn,
 	return PCIBIOS_SUCCESSFUL;
 }
 
+static int kirin_pcie_add_bus(struct pci_bus *bus)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(bus->sysdata);
+	struct kirin_pcie *kirin_pcie = to_kirin_pcie(pci);
+	int i, ret;
+
+	/* perst assert Endpoint */
+	usleep_range(REF_2_PERST_MIN, REF_2_PERST_MAX);
+
+	/* Send PERST# to each slot */
+	for (i = 0; i < kirin_pcie->num_slots; i++) {
+		ret = gpio_direction_output(kirin_pcie->gpio_id_reset[i], 1);
+		if (ret)
+			return ret;
+	}
+	usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX);
+
+	return 0;
+}
+
+
 static struct pci_ops kirin_pci_ops = {
 	.read = kirin_pcie_rd_own_conf,
 	.write = kirin_pcie_wr_own_conf,
+	.add_bus = kirin_pcie_add_bus,
 };
 
 static u32 kirin_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
@@ -477,6 +569,46 @@ static int kirin_pcie_host_init(struct pcie_port *pp)
 	return 0;
 }
 
+static int kirin_pcie_gpio_request(struct kirin_pcie *kirin_pcie,
+				   struct device *dev)
+{
+	int ret, i;
+
+	for (i = 0; i < kirin_pcie->num_slots; i++) {
+		if (!gpio_is_valid(kirin_pcie->gpio_id_reset[i])) {
+			dev_err(dev, "unable to get a valid %s gpio\n",
+				kirin_pcie->reset_names[i]);
+			return -ENODEV;
+		}
+
+		ret = devm_gpio_request(dev, kirin_pcie->gpio_id_reset[i],
+					kirin_pcie->reset_names[i]);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; i < kirin_pcie->num_slots; i++) {
+		if (!gpio_is_valid(kirin_pcie->gpio_id_clkreq[i])) {
+			dev_err(dev, "unable to get a valid %s gpio\n",
+				kirin_pcie->clkreq_names[i]);
+			return -ENODEV;
+		}
+
+		ret = devm_gpio_request(dev, kirin_pcie->gpio_id_clkreq[i],
+					kirin_pcie->clkreq_names[i]);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; i < kirin_pcie->num_slots; i++) {
+		ret = gpio_direction_output(kirin_pcie->gpio_id_clkreq[i], 0);
+		if (ret)
+			return ret;
+	}
+
+	return ret;
+}
+
 static const struct dw_pcie_ops kirin_dw_pcie_ops = {
 	.read_dbi = kirin_pcie_read_dbi,
 	.write_dbi = kirin_pcie_write_dbi,
@@ -499,24 +631,43 @@ static int kirin_pcie_power_on(struct platform_device *pdev,
 		if (ret)
 			return ret;
 
-		return hi3660_pcie_phy_power_on(kirin_pcie);
+		ret = kirin_pcie_gpio_request(kirin_pcie, dev);
+		if (ret)
+			return ret;
+
+		ret = hi3660_pcie_phy_power_on(kirin_pcie);
+		if (ret)
+			return ret;
+	} else {
+		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 = phy_init(kirin_pcie->phy);
+		if (ret)
+			goto err;
+
+		ret = phy_power_on(kirin_pcie->phy);
+		if (ret)
+			goto err;
 	}
 
-	kirin_pcie->phy = devm_of_phy_get(dev, dev->of_node, NULL);
-	if (IS_ERR(kirin_pcie->phy))
-		return PTR_ERR(kirin_pcie->phy);
+	/* perst assert Endpoint */
+	usleep_range(REF_2_PERST_MIN, REF_2_PERST_MAX);
 
-	ret = phy_init(kirin_pcie->phy);
-	if (ret)
-		goto err;
+	if (!gpio_request(kirin_pcie->gpio_id_dwc_perst, "pcie_perst")) {
+		ret = gpio_direction_output(kirin_pcie->gpio_id_dwc_perst, 1);
+		if (ret)
+			goto err;
+	}
 
-	ret = phy_power_on(kirin_pcie->phy);
-	if (ret)
-		goto err;
+	usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX);
 
 	return 0;
 err:
-	phy_exit(kirin_pcie->phy);
+	if (kirin_pcie->type == PCIE_KIRIN_INTERNAL_PHY)
+		phy_exit(kirin_pcie->phy);
+
 	return ret;
 }
 
diff --git a/drivers/phy/hisilicon/phy-hi3670-pcie.c b/drivers/phy/hisilicon/phy-hi3670-pcie.c
index 82cc5fc4eac2..ac6052a05788 100644
--- a/drivers/phy/hisilicon/phy-hi3670-pcie.c
+++ b/drivers/phy/hisilicon/phy-hi3670-pcie.c
@@ -87,9 +87,6 @@
 #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 */
@@ -108,10 +105,6 @@
 #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
@@ -131,12 +124,6 @@ struct hi3670_pcie_phy {
 	struct clk	*phy_ref_clk;
 	struct clk	*aclk;
 	struct clk	*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];
 };
 
@@ -230,40 +217,6 @@ static void hi3670_pcie_set_eyeparam(struct hi3670_pcie_phy *phy)
 	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;
@@ -558,8 +511,6 @@ static int hi3670_pcie_get_resources_from_pcie(struct hi3670_pcie_phy *phy)
 	struct device_node *pcie_port;
 	struct device *dev = phy->dev;
 	struct device *pcie_dev;
-	char name[32];
-	int i;
 
 	pcie_port = of_get_child_by_name(dev->parent->of_node, "pcie");
 	if (!pcie_port) {
@@ -588,27 +539,6 @@ static int hi3670_pcie_get_resources_from_pcie(struct hi3670_pcie_phy *phy)
 		return -ENODEV;
 	}
 
-	/* perst reset gpios */
-	phy->n_gpio_resets = of_gpio_named_count(pcie_dev->of_node,
-						 "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(pcie_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;
-	}
-
 	return 0;
 }
 
@@ -663,7 +593,6 @@ static int kirin_pcie_clk_ctrl(struct hi3670_pcie_phy *phy, bool enable)
 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;
 
 	/*
@@ -681,15 +610,6 @@ static int hi3670_pcie_phy_init(struct phy *generic_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);
@@ -697,11 +617,13 @@ static int hi3670_pcie_phy_power_on(struct phy *generic_phy)
 
 	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;
-	}
+	return 0;
+}
+
+static int hi3670_pcie_phy_power_on(struct phy *generic_phy)
+{
+	struct hi3670_pcie_phy *phy = phy_get_drvdata(generic_phy);
+	int val, ret;
 
 	ret = kirin_pcie_clk_ctrl(phy, true);
 	if (ret)
@@ -732,15 +654,6 @@ static int hi3670_pcie_phy_power_on(struct phy *generic_phy)
 	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;
@@ -781,9 +694,6 @@ 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");
@@ -824,25 +734,6 @@ static int hi3670_pcie_phy_get_resources(struct hi3670_pcie_phy *phy,
 	if (IS_ERR(phy->base))
 		return PTR_ERR(phy->base);
 
-	/* 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;


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 08/10] arm64: dts: HiSilicon: Add support for HiKey 970 PCIe controller hardware
  2021-07-24  4:11       ` Manivannan Sadhasivam
@ 2021-08-03  4:25         ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 21+ messages in thread
From: Mauro Carvalho Chehab @ 2021-08-03  4:25 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: Vinod Koul, Bjorn Helgaas, Rob Herring, linuxarm, mauro.chehab,
	Krzysztof Wilczyński, Binghui Wang, Lorenzo Pieralisi,
	Rob Herring, Wei Xu, Xiaowei Song, devicetree, linux-arm-kernel,
	linux-kernel, linux-pci

Em Sat, 24 Jul 2021 09:41:50 +0530
Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> escreveu:

> On Fri, Jul 23, 2021 at 08:53:18AM +0200, Mauro Carvalho Chehab wrote:
> > Em Thu, 22 Jul 2021 19:06:28 +0530
> > Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> escreveu:
> >   
> > > On Wed, Jul 21, 2021 at 10:39:10AM +0200, Mauro Carvalho Chehab wrote:  
> > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > 
> > > > Add DTS bindings for the HiKey 970 board's PCIe hardware.
> > > > 
> > > > Co-developed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > > ---
> > > >  arch/arm64/boot/dts/hisilicon/hi3670.dtsi     | 71 +++++++++++++++++++
> > > >  .../boot/dts/hisilicon/hikey970-pmic.dtsi     |  1 -
> > > >  drivers/pci/controller/dwc/pcie-kirin.c       | 12 ----
> > > >  3 files changed, 71 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> > > > index 1f228612192c..6dfcfcfeedae 100644
> > > > --- a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> > > > +++ b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> > > > @@ -177,6 +177,12 @@ sctrl: sctrl@fff0a000 {
> > > >  			#clock-cells = <1>;
> > > >  		};
> > > >  
> > > > +		pmctrl: pmctrl@fff31000 {
> > > > +			compatible = "hisilicon,hi3670-pmctrl", "syscon";
> > > > +			reg = <0x0 0xfff31000 0x0 0x1000>;
> > > > +			#clock-cells = <1>;
> > > > +		};
> > > > +    
> > > 
> > > Irrelevant change to this patch.  
> > 
> > Huh?
> > 
> > This is used by PCIe PHY, as part of the power on procedures:
> > 
> > 	+static int hi3670_pcie_noc_power(struct hi3670_pcie_phy *phy, bool enable)
> > 	+{
> > 	+       struct device *dev = phy->dev;
> > 	+       u32 time = 100;
> > 	+       unsigned int val = NOC_PW_MASK;
> > 	+       int rst;
> > 	+
> > 	+       if (enable)
> > 	+               val = NOC_PW_MASK | NOC_PW_SET_BIT;
> > 	+       else
> > 	+               val = NOC_PW_MASK;
> > 	+       rst = enable ? 1 : 0;
> > 	+
> > 	+       regmap_write(phy->pmctrl, NOC_POWER_IDLEREQ_1, val);
> > 
> >   
> 
> Ah... you're hardcoding the syscon compatible in driver. Sorry missed that.
> 
> But if these syscon nodes are independent memory regions or belong to non
> PCI/PHY memory map, you could've fetched the reference through a DT property
> along with the offset then used it in driver.
> 
> Like,
> 
> 	pcie_phy: pcie-phy@fc000000 {
> 		...
> 		hisilicon,noc-power-regs = <&pmctrl 0x38c>;
> 		hisilicon,sctrl-cmos-regs = <&sctrl 0x60>;
> 		...
> 	};
> 
> The benefit of doing this way is, if the pmctrl, sctrl register layout changes
> in future, you can handle it without any issues.

Interesting approach, but probably overkill. I mean, the register mapping
here should be the same for all Kirin 970 PHY based devices. A PHY for a 
different SoC will likely have other differences than just those two regs.

Regards,
Mauro

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 08/10] arm64: dts: HiSilicon: Add support for HiKey 970 PCIe controller hardware
  2021-07-21  8:39 ` [PATCH v7 08/10] arm64: dts: HiSilicon: Add support for HiKey 970 PCIe controller hardware Mauro Carvalho Chehab
  2021-07-22 13:36   ` Manivannan Sadhasivam
@ 2021-08-16 18:26   ` Rob Herring
  1 sibling, 0 replies; 21+ messages in thread
From: Rob Herring @ 2021-08-16 18:26 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Vinod Koul, Bjorn Helgaas, Linuxarm, mauro.chehab,
	Manivannan Sadhasivam, Krzysztof Wilczyński, Binghui Wang,
	Lorenzo Pieralisi, Wei Xu, Xiaowei Song, devicetree,
	linux-arm-kernel, linux-kernel, PCI

On Wed, Jul 21, 2021 at 3:39 AM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>
> Add DTS bindings for the HiKey 970 board's PCIe hardware.
>
> Co-developed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  arch/arm64/boot/dts/hisilicon/hi3670.dtsi     | 71 +++++++++++++++++++
>  .../boot/dts/hisilicon/hikey970-pmic.dtsi     |  1 -
>  drivers/pci/controller/dwc/pcie-kirin.c       | 12 ----
>  3 files changed, 71 insertions(+), 13 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> index 1f228612192c..6dfcfcfeedae 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> @@ -177,6 +177,12 @@ sctrl: sctrl@fff0a000 {
>                         #clock-cells = <1>;
>                 };
>
> +               pmctrl: pmctrl@fff31000 {
> +                       compatible = "hisilicon,hi3670-pmctrl", "syscon";
> +                       reg = <0x0 0xfff31000 0x0 0x1000>;
> +                       #clock-cells = <1>;
> +               };
> +
>                 iomcu: iomcu@ffd7e000 {
>                         compatible = "hisilicon,hi3670-iomcu", "syscon";
>                         reg = <0x0 0xffd7e000 0x0 0x1000>;
> @@ -660,6 +666,71 @@ gpio28: gpio@fff1d000 {
>                         clock-names = "apb_pclk";
>                 };
>
> +               its_pcie: interrupt-controller@f4000000 {
> +                       compatible = "arm,gic-v3-its";
> +                       msi-controller;
> +                       reg = <0x0 0xf5100000 0x0 0x100000>;

How does this h/w have a GIC-400 (which is GICv2) and then a GIC v3 ITS?

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

This means the PCI host doesn't have a MSI controller...

> +                       #address-cells = <3>;
> +                       #size-cells = <2>;
> +                       device_type = "pci";
> +                       phys = <&pcie_phy>;
> +                       ranges = <0x02000000 0x0 0x00000000
> +                                 0x0 0xf6000000
> +                                 0x0 0x02000000>;
> +                       num-lanes = <1>;
> +                       #interrupt-cells = <1>;
> +                       interrupts = <0 283 4>;
> +                       interrupt-names = "msi";

But then this says it does...

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2021-08-16 18:27 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-21  8:39 [PATCH v7 00/10] Add support for Hikey 970 PCIe Mauro Carvalho Chehab
2021-07-21  8:39 ` [PATCH v7 05/10] dt-bindings: PCI: kirin: Fix compatible string Mauro Carvalho Chehab
2021-07-21  8:39 ` [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY Mauro Carvalho Chehab
2021-07-23 22:50   ` Rob Herring
2021-07-24  0:12     ` Mauro Carvalho Chehab
2021-07-26 21:37       ` Rob Herring
2021-07-26 23:50         ` Mauro Carvalho Chehab
2021-07-27  6:52           ` Mauro Carvalho Chehab
2021-07-27 22:17             ` Rob Herring
2021-07-28  7:38               ` Mauro Carvalho Chehab
2021-07-28 14:28                 ` Rob Herring
2021-07-29 10:12                   ` Mauro Carvalho Chehab
2021-07-21  8:39 ` [PATCH v7 08/10] arm64: dts: HiSilicon: Add support for HiKey 970 PCIe controller hardware Mauro Carvalho Chehab
2021-07-22 13:36   ` Manivannan Sadhasivam
2021-07-23  6:53     ` Mauro Carvalho Chehab
2021-07-24  4:11       ` Manivannan Sadhasivam
2021-08-03  4:25         ` Mauro Carvalho Chehab
2021-08-16 18:26   ` Rob Herring
2021-07-21  8:39 ` [PATCH v7 09/10] dt-bindings: PCI: kirin-pcie.txt: Convert it to yaml Mauro Carvalho Chehab
2021-07-23 22:56   ` Rob Herring
2021-07-21  8:39 ` [PATCH v7 10/10] phy-hi3670-pcie: Move reset-gpios to the PCIe DT schema Mauro Carvalho Chehab

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).